Link const annotations to their docs (dart-lang#2313)

Link const annotations to their docs

Author: srawlins

lib/src/model/field.dart

@@ -105,15 +105,14 @@ class Field extends ModelElement
 
   @override
   List<String> get annotations {
-    var all_annotations = <String>[];
-    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

lib/src/model/model_element.dart

@@ -399,46 +399,63 @@ abstract class ModelElement extends Canonicalization
   List<String> 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>[Baz.one, Baz.two])`, link to
+  // `Foo`, `Bar`, `Baz`, `Baz.one`, and `Baz.two`.
   List<String> annotationsFromMetadata(List<ElementAnnotation> md) {
     var annotationStrings = <String>[];
     if (md == null) return annotationStrings;
     for (var a in md) {
       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;
+            (annotationElement as ConstructorElement).returnType.element;
+      } else if (annotationElement is PropertyAccessorElement) {
+        annotationElement =
+            (annotationElement as PropertyAccessorElement).variable;
       }
-      if (annotationElement is ClassElement) {
-        annotationClassElement = annotationElement;
+      if (annotationElement is Member) {
+        annotationElement = (annotationElement as Member).declaration;
       }
+
+      // 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) {
+    if (annotationElement is ClassElement) {
+      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);
+    }
+    // We cannot resolve it, which does not prevent it from being displayed.
+    return true;
+  }
+
   bool _isPublic;
 
   @override
@@ -506,14 +523,13 @@ abstract class ModelElement extends Canonicalization
           .where((s) => s.isNotEmpty));
 
   Set<String> get features {
-    var allFeatures = <String>{};
-    allFeatures.addAll(annotations);
+    var allFeatures = <String>{...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

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<Class> get invisibleAnnotations {
-    if (_invisibleAnnotations == null) {
-      _invisibleAnnotations = {};
-      _invisibleAnnotations.add(specialClasses[SpecialClass.pragma]);
-    }
-    return _invisibleAnnotations;
-  }
+  Set<Class> get invisibleAnnotations =>
+      _invisibleAnnotations ??= {specialClasses[SpecialClass.pragma]};
+
+  bool isAnnotationVisible(Class class_) =>
+      !invisibleAnnotations.contains(class_);
 
   @override
   String toString() {

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(
+              '@<a href="${HTMLBASE_PLACEHOLDER}fake/required-constant.html">required</a>'));
     });
 
     test('param exported in library', () {
@@ -3685,8 +3688,9 @@ String topLevelFunction(int param1, bool param2, Cool coolBeans,
           .renderLinkedParams(methodWithTypedefParam.parameters);
       expect(
           params,
-          equals(
-              '<span class="parameter" id="methodWithTypedefParam-param-p"><span class="type-annotation"><a href="${HTMLBASE_PLACEHOLDER}ex/processMessage.html">processMessage</a></span> <span class="parameter-name">p</span></span><wbr>'));
+          equals('<span class="parameter" id="methodWithTypedefParam-param-p">'
+              '<span class="type-annotation"><a href="${HTMLBASE_PLACEHOLDER}ex/processMessage.html">processMessage</a></span> '
+              '<span class="parameter-name">p</span></span><wbr>'));
     });
   });
 
@@ -3765,20 +3769,34 @@ String topLevelFunction(int param1, bool param2, Cool coolBeans,
       expect(
           forAnnotation.annotations.first,
           equals(
-              '@<a href="${HTMLBASE_PLACEHOLDER}ex/ForAnnotation-class.html">ForAnnotation</a>(&#39;my value&#39;)'));
+              '@<a href="${HTMLBASE_PLACEHOLDER}ex/ForAnnotation-class.html">ForAnnotation</a>'
+              '(&#39;my value&#39;)'));
     });
 
     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(
+              '@<a href="${HTMLBASE_PLACEHOLDER}ex/deprecated-constant.html">deprecated</a>'));
     });
 
-    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(
-              '@<a href="${HTMLBASE_PLACEHOLDER}ex/Deprecated-class.html">Deprecated</a>(&quot;Internal use&quot;)'));
+              '@<a href="${HTMLBASE_PLACEHOLDER}ex/Deprecated-class.html">Deprecated</a>'
+              '(&quot;Internal use&quot;)'));
+    });
+
+    test('const annotations have the right link and are escaped', () {
+      var createDog2 =
+          dog.staticMethods.firstWhere((c) => c.name == 'createDog2');
+      expect(
+          createDog2.annotations[0],
+          equals(
+              '@<a href="${HTMLBASE_PLACEHOLDER}ex/deprecated-constant.html">deprecated</a>'));
     });
   });
 

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() {}
 }