From 26d25be3c043effc9955fce0d47eedd82df2c0ec Mon Sep 17 00:00:00 2001 From: Sam Rawlins Date: Wed, 19 Aug 2020 09:53:21 -0700 Subject: [PATCH 1/2] Link const annotations to their docs --- lib/src/model/field.dart | 9 ++-- lib/src/model/model_element.dart | 70 ++++++++++++++++----------- lib/src/model/package_graph.dart | 12 ++--- test/model_test.dart | 32 +++++++++--- testing/test_package/lib/example.dart | 5 ++ 5 files changed, 82 insertions(+), 46 deletions(-) diff --git a/lib/src/model/field.dart b/lib/src/model/field.dart index ae8529cb1a..f22517b0bf 100644 --- a/lib/src/model/field.dart +++ b/lib/src/model/field.dart @@ -105,15 +105,14 @@ class Field extends ModelElement @override List get annotations { - var all_annotations = []; - all_annotations.addAll(super.annotations); + var allAnnotations = [...super.annotations]; if (element is PropertyInducingElement) { var pie = element as PropertyInducingElement; - all_annotations.addAll(annotationsFromMetadata(pie.getter?.metadata)); - all_annotations.addAll(annotationsFromMetadata(pie.setter?.metadata)); + allAnnotations.addAll(annotationsFromMetadata(pie.getter?.metadata)); + allAnnotations.addAll(annotationsFromMetadata(pie.setter?.metadata)); } - return all_annotations.toList(growable: false); + return allAnnotations; } @override diff --git a/lib/src/model/model_element.dart b/lib/src/model/model_element.dart index 0fc0edcfc9..de38ddf573 100644 --- a/lib/src/model/model_element.dart +++ b/lib/src/model/model_element.dart @@ -399,6 +399,10 @@ abstract class ModelElement extends Canonicalization List get annotations => annotationsFromMetadata(element.metadata); /// Returns linked annotations from a given metadata set, with escaping. + // TODO(srawlins): Attempt to revive constructor arguments in an annotation, + // akin to source_gen's Reviver, in order to link to inner components. For + // example, in `@Foo(const Bar(), baz: [Baz.one, Baz.two])`, link to + // `Foo`, `Bar`, `Baz`, `Baz.one`, and `Baz.two`. List annotationsFromMetadata(List md) { var annotationStrings = []; if (md == null) return annotationStrings; @@ -406,39 +410,52 @@ abstract class ModelElement extends Canonicalization var annotation = (const HtmlEscape()).convert(a.toSource()); var annotationElement = a.element; - ClassElement annotationClassElement; - if (annotationElement is ExecutableElement) { + if (annotationElement is ConstructorElement) { + // TODO(srawlins): I think we should actually link to the constructor, + // which may have details about parameters. For example, given the + // annotation `@Immutable('text')`, the constructor documents what the + // parameter is, and the class only references `immutable`. It's a + // lose-lose cycle of mis-direction. annotationElement = - (annotationElement as ExecutableElement).returnType.element; - } - if (annotationElement is ClassElement) { - annotationClassElement = annotationElement; + (annotationElement as ConstructorElement).returnType.element; + } else if (annotationElement is PropertyAccessorElement) { + annotationElement = + (annotationElement as PropertyAccessorElement).variable; } + + // Some annotations are intended to be invisible (such as `@pragma`). + if (!_shouldDisplayAnnotation(annotationElement)) continue; + var annotationModelElement = packageGraph.findCanonicalModelElementFor(annotationElement); - // annotationElement can be null if the element can't be resolved. - var annotationClass = packageGraph - .findCanonicalModelElementFor(annotationClassElement) as Class; - if (annotationClass == null && - annotationElement != null && - annotationClassElement != null) { - annotationClass = - ModelElement.fromElement(annotationClassElement, packageGraph) - as Class; - } - // Some annotations are intended to be invisible (@pragma) - if (annotationClass == null || - !packageGraph.invisibleAnnotations.contains(annotationClass)) { - if (annotationModelElement != null) { - annotation = annotation.replaceFirst( - annotationModelElement.name, annotationModelElement.linkedName); - } - annotationStrings.add(annotation); + if (annotationModelElement != null) { + annotation = annotation.replaceFirst( + annotationModelElement.name, annotationModelElement.linkedName); } + annotationStrings.add(annotation); } return annotationStrings; } + bool _shouldDisplayAnnotation(Element annotationElement) { + ClassElement annotationClassElement; + if (annotationElement is ClassElement) { + annotationClassElement = annotationElement; + } + // `annotationElement` can be null if the element can't be resolved. + var annotationClass = packageGraph + .findCanonicalModelElementFor(annotationClassElement) as Class; + if (annotationClass == null && + annotationElement != null && + annotationClassElement != null) { + annotationClass = + ModelElement.fromElement(annotationClassElement, packageGraph) + as Class; + } + return annotationClass == null || + packageGraph.isAnnotationVisible(annotationClass); + } + bool _isPublic; @override @@ -506,14 +523,13 @@ abstract class ModelElement extends Canonicalization .where((s) => s.isNotEmpty)); Set get features { - var allFeatures = {}; - allFeatures.addAll(annotations); + var allFeatures = {...annotations}; // Replace the @override annotation with a feature that explicitly // indicates whether an override has occurred. allFeatures.remove('@override'); - // Drop the plain "deprecated" annotation, that's indicated via + // Drop the plain "deprecated" annotation; that's indicated via // strikethroughs. Custom @Deprecated() will still appear. allFeatures.remove('@deprecated'); // const and static are not needed here because const/static elements get diff --git a/lib/src/model/package_graph.dart b/lib/src/model/package_graph.dart index 8d548cdc64..fbd35ca22e 100644 --- a/lib/src/model/package_graph.dart +++ b/lib/src/model/package_graph.dart @@ -641,13 +641,11 @@ class PackageGraph { /// Returns the set of [Class] objects that are similar to pragma /// in that we should never count them as documentable annotations. - Set get invisibleAnnotations { - if (_invisibleAnnotations == null) { - _invisibleAnnotations = {}; - _invisibleAnnotations.add(specialClasses[SpecialClass.pragma]); - } - return _invisibleAnnotations; - } + Set get invisibleAnnotations => + _invisibleAnnotations ??= {specialClasses[SpecialClass.pragma]}; + + bool isAnnotationVisible(Class class_) => + !invisibleAnnotations.contains(class_); @override String toString() { diff --git a/test/model_test.dart b/test/model_test.dart index 81d261be8c..88c37b5e21 100644 --- a/test/model_test.dart +++ b/test/model_test.dart @@ -3671,7 +3671,10 @@ String topLevelFunction(int param1, bool param2, Cool coolBeans, fakeLibrary.functions.firstWhere((f) => f.name == 'paintImage1'); var params = ParameterRendererHtml().renderLinkedParams(method.parameters); - expect(params, contains('@required')); + expect( + params, + contains( + '@required')); }); test('param exported in library', () { @@ -3685,8 +3688,9 @@ String topLevelFunction(int param1, bool param2, Cool coolBeans, .renderLinkedParams(methodWithTypedefParam.parameters); expect( params, - equals( - 'processMessage p')); + equals('' + 'processMessage ' + 'p')); }); }); @@ -3765,20 +3769,34 @@ String topLevelFunction(int param1, bool param2, Cool coolBeans, expect( forAnnotation.annotations.first, equals( - '@ForAnnotation('my value')')); + '@ForAnnotation' + '('my value')')); }); test('methods has the right annotation', () { var m = dog.instanceMethods.singleWhere((m) => m.name == 'getClassA'); expect(m.hasAnnotations, isTrue); - expect(m.annotations.first, equals('@deprecated')); + expect( + m.annotations.first, + equals( + '@deprecated')); }); - test('method annotations have the right link and are escaped', () { + test('constructor annotations have the right link and are escaped', () { expect( ctr.annotations[0], equals( - '@Deprecated("Internal use")')); + '@Deprecated' + '("Internal use")')); + }); + + test('const annotations have the right link and are escaped', () { + var createDog2 = + dog.staticMethods.firstWhere((c) => c.name == 'createDog2'); + expect( + createDog2.annotations[0], + equals( + '@deprecated')); }); }); diff --git a/testing/test_package/lib/example.dart b/testing/test_package/lib/example.dart index d441643311..3c7da8b255 100644 --- a/testing/test_package/lib/example.dart +++ b/testing/test_package/lib/example.dart @@ -447,6 +447,11 @@ class Dog implements Cat, E { return Dog.deprecatedCreate(s); } + @deprecated + static Dog createDog2(String s) { + return Dog.deprecatedCreate(s); + } + @override void abstractMethod() {} } From 27f6b3bf6c9fb89d01867f39a9c4f08fd6fa6c7a Mon Sep 17 00:00:00 2001 From: Sam Rawlins Date: Wed, 19 Aug 2020 14:33:37 -0700 Subject: [PATCH 2/2] Handle Members --- lib/src/model/model_element.dart | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/lib/src/model/model_element.dart b/lib/src/model/model_element.dart index de38ddf573..9e4706f9a6 100644 --- a/lib/src/model/model_element.dart +++ b/lib/src/model/model_element.dart @@ -422,6 +422,9 @@ abstract class ModelElement extends Canonicalization annotationElement = (annotationElement as PropertyAccessorElement).variable; } + if (annotationElement is Member) { + annotationElement = (annotationElement as Member).declaration; + } // Some annotations are intended to be invisible (such as `@pragma`). if (!_shouldDisplayAnnotation(annotationElement)) continue; @@ -438,22 +441,19 @@ abstract class ModelElement extends Canonicalization } bool _shouldDisplayAnnotation(Element annotationElement) { - ClassElement annotationClassElement; if (annotationElement is ClassElement) { - annotationClassElement = annotationElement; - } - // `annotationElement` can be null if the element can't be resolved. - var annotationClass = packageGraph - .findCanonicalModelElementFor(annotationClassElement) as Class; - if (annotationClass == null && - annotationElement != null && - annotationClassElement != null) { - annotationClass = - ModelElement.fromElement(annotationClassElement, packageGraph) - as Class; + var annotationClass = + packageGraph.findCanonicalModelElementFor(annotationElement) as Class; + if (annotationClass == null && annotationElement != null) { + annotationClass = + ModelElement.fromElement(annotationElement, packageGraph) as Class; + } + + return annotationClass == null || + packageGraph.isAnnotationVisible(annotationClass); } - return annotationClass == null || - packageGraph.isAnnotationVisible(annotationClass); + // We cannot resolve it, which does not prevent it from being displayed. + return true; } bool _isPublic;