Refactor annotations handling, update to 0.9. (#1349)

This refactoring of annotations handling drops adding @s from templates in some cases, unifies code for pulling data out of analyzer, and eliminates hard to maintain and bug-causing returns in the middle of complex functions.  It fixes multiple issues, including #1081, #1268, #1162 and more.

Author: jcollins-g

CHANGELOG.md

@@ -1,3 +1,7 @@
+## 0.9.13
+
+* fix multiple issues in annotation/feature list handling (#1268, #1162, #1081)
+
 ## 0.9.12
 
 * add print styles

lib/dartdoc.dart

@@ -40,7 +40,7 @@ export 'src/package_meta.dart';
 
 const String name = 'dartdoc';
 // Update when pubspec version changes.
-const String version = '0.9.12';
+const String version = '0.9.13';
 
 final String defaultOutDir = path.join('doc', 'api');
 

lib/resources/styles.css

@@ -456,10 +456,6 @@ nav .self-name {
   display: inline;
 }
 
-.annotation-list li:before {
-  content: "@";
-}
-
 .comma-separated {
   list-style: none;
   padding: 0;

lib/src/model.dart

@@ -9,7 +9,8 @@ import 'dart:convert';
 import 'dart:io';
 
 import 'package:analyzer/dart/ast/ast.dart'
-    show AnnotatedNode, Annotation, Declaration;
+    show AnnotatedNode, Declaration, FormalParameter, FieldDeclaration,
+        VariableDeclaration, VariableDeclarationList;
 import 'package:analyzer/dart/element/element.dart';
 import 'package:analyzer/dart/element/type.dart';
 import 'package:analyzer/src/generated/resolver.dart'
@@ -913,9 +914,23 @@ class Field extends ModelElement
 
   bool get writeOnly => hasSetter && !hasGetter;
 
+  @override
+  List<String> get annotations {
+    List<String> all_annotations = new List<String>();
+    all_annotations.addAll(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));
+    }
+    return all_annotations.toList(growable: false);
+  }
+
   @override
   Set<String> get features {
     Set<String> all_features = super.features;
+    all_features.addAll(annotations);
     /// final/const implies read-only, so don't display both strings.
     if (readOnly && !isFinal && !isConst) all_features.add('read-only');
     if (writeOnly) all_features.add('write-only');
@@ -1475,43 +1490,56 @@ abstract class ModelElement implements Comparable, Nameable, Documentable {
   }
 
   List<String> get annotations {
-    // Check https://code.google.com/p/dart/issues/detail?id=23181
-    // If that is fixed, this code might get a lot easier
-    if (element.computeNode() != null &&
-        element.computeNode() is AnnotatedNode) {
-      return (element.computeNode() as AnnotatedNode)
-          .metadata
-          .map((Annotation a) {
-        var annotationString = a.toSource().substring(1); // remove the @
-        var e = a.element;
-        if (e != null && (e is ConstructorElement)) {
-          var me = new ModelElement.from(
-              e.enclosingElement, package._getLibraryFor(e.enclosingElement));
-          if (me.href != null) {
-            return annotationString.replaceAll(me.name, me.linkedName);
-          }
-        }
-        return annotationString;
-      }).toList(growable: false);
-    } else {
-      return element.metadata.map((ElementAnnotation a) {
-        // TODO link to the element's href
-        return a.element.name;
-      }).toList(growable: false);
-    }
+    List<dynamic> metadata;
+    if (element.computeNode() is AnnotatedNode) {
+      AnnotatedNode node = element.computeNode() as AnnotatedNode;
+
+      // Declarations are contained inside FieldDeclarations, and that is where
+      // the actual annotations are.
+      while ((node is VariableDeclaration || node is VariableDeclarationList) &&
+          node is! FieldDeclaration) {
+        assert (null != (node as AnnotatedNode).parent);
+        node = node.parent;
+      }
+      metadata = node.metadata;
+    } else if (element.computeNode() is! FormalParameter) {
+      // TODO(jcollins-g): This is special cased to suppress annotations for
+      //                   parameters in constructor documentation.  Do we
+      //                   want to do this?
+      metadata = element.metadata;
+    }
+    return annotationsFromMetadata(metadata);
+  }
+
+  /// Returns annotations from a given metadata set, with escaping.
+  /// md is a dynamic parameter since ElementAnnotation and Annotation have no
+  /// common class for calling toSource() and element.
+  List<String> annotationsFromMetadata(List<dynamic> md) {
+    if (md == null) md = new List<dynamic>();
+    return md.map((dynamic a) {
+      String annotation = (const HtmlEscape()).convert(a.toSource());
+      if (a.element is ConstructorElement) {
+        var me = new ModelElement.from(a.element.enclosingElement,
+            package._getLibraryFor(a.element.enclosingElement));
+        annotation = annotation.replaceFirst(me.name, me.linkedName);
+      }
+      return annotation;
+    }).toList(growable: false);
   }
 
-  /// const and static are not needed here because const/static elements get
-  /// their own sections in the doc.
   Set<String> get features {
     Set<String> all_features = new Set<String>();
     all_features.addAll(annotations);
-    /// override as an annotation should be replaced with direct information
-    /// from the analyzer if we decide to display it at this level.
-    all_features.remove('override');
-    /// Drop the plain "deprecated" annotation, that's indicated via
-    /// strikethroughs. Custom @Deprecated() will still appear.
-    all_features.remove('deprecated');
+
+    // override as an annotation should be replaced with direct information
+    // from the analyzer if we decide to display it at this level.
+    all_features.remove('@override');
+
+    // Drop the plain "deprecated" annotation, that's indicated via
+    // strikethroughs. Custom @Deprecated() will still appear.
+    all_features.remove('@deprecated');
+    // const and static are not needed here because const/static elements get
+    // their own sections in the doc.
     if (isFinal) all_features.add('final');
     return all_features;
   }
@@ -1749,7 +1777,7 @@ abstract class ModelElement implements Comparable, Nameable, Documentable {
       buf.write('<span class="parameter" id="${param.htmlId}">');
       if (showMetadata && param.hasAnnotations) {
         param.annotations.forEach((String annotation) {
-          buf.write('<span>@$annotation</span> ');
+          buf.write('<span>$annotation</span> ');
         });
       }
       if (param.modelType.isFunctionType) {
@@ -2575,6 +2603,7 @@ class TopLevelVariable extends ModelElement
   @override
   Set<String> get features {
     Set<String> all_features = super.features;
+
     /// final/const implies read-only, so don't display both strings.
     if (readOnly && !isFinal && !isConst) all_features.add('read-only');
     if (writeOnly) all_features.add('write-only');

pubspec.lock

@@ -6,7 +6,7 @@ packages:
       name: analyzer
       url: "https://pub.dartlang.org"
     source: hosted
-    version: "0.29.0"
+    version: "0.29.8"
   ansicolor:
     description:
       name: ansicolor
@@ -43,6 +43,12 @@ packages:
       url: "https://pub.dartlang.org"
     source: hosted
     version: "1.1.0"
+  cli_util:
+    description:
+      name: cli_util
+      url: "https://pub.dartlang.org"
+    source: hosted
+    version: "0.0.1+2"
   collection:
     description:
       name: collection
@@ -301,6 +307,18 @@ packages:
       url: "https://pub.dartlang.org"
     source: hosted
     version: "1.0.4"
+  when:
+    description:
+      name: when
+      url: "https://pub.dartlang.org"
+    source: hosted
+    version: "0.2.0"
+  which:
+    description:
+      name: which
+      url: "https://pub.dartlang.org"
+    source: hosted
+    version: "0.1.3"
   yaml:
     description:
       name: yaml

pubspec.yaml

@@ -7,7 +7,7 @@ homepage: https://github.com/dart-lang/dartdoc
 environment:
   sdk: '>=1.14.0 <2.0.0'
 dependencies:
-  analyzer: ^0.29.0
+  analyzer: ^0.29.8
   args: ^0.13.0
   collection: ^1.2.0
   html: '>=0.12.1 <0.14.0'

test/model_test.dart

@@ -1645,24 +1645,24 @@ String topLevelFunction(int param1, bool param2, Cool coolBeans,
     test('has one annotation',
         () => expect(forAnnotation.annotations, hasLength(1)));
 
-    test('has the right annotation', () {
+    test('has the right annotation and is escaped', () {
       expect(
           forAnnotation.annotations.first,
           equals(
-              '<a href="ex/ForAnnotation-class.html">ForAnnotation</a>(\'my value\')'));
+              '@<a href="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('@deprecated'));
     });
 
-    test('method annotations have the right link', () {
+    test('method annotations have the right link and are escaped', () {
       expect(
           ctr.annotations[0],
           equals(
-              '<a href="ex/Deprecated-class.html">Deprecated</a>("Internal use")'));
+              '@<a href="ex/Deprecated-class.html">Deprecated</a>(&quot;Internal use&quot;)'));
     });
   });
 

testing/test_package_docs/ex/B-class.html

@@ -198,7 +198,7 @@ <h2>Properties</h2>
         </dt>
         <dd>
           <p></p>
-          <div class="features">read-only</div>
+          <div class="features">@override, read-only</div>
 </dd>
         <dt id="list" class="property">
           <span class="name"><a href="ex/B/list.html">list</a></span>
@@ -214,7 +214,7 @@ <h2>Properties</h2>
         </dt>
         <dd>
           <p></p>
-          <div class="features">read-only</div>
+          <div class="features">@override, read-only</div>
 </dd>
         <dt id="fieldWithTypedef" class="property inherited">
           <span class="name"><a href="ex/Apple/fieldWithTypedef.html">fieldWithTypedef</a></span>

testing/test_package_docs/ex/B/abstractMethod.html

@@ -116,7 +116,7 @@ <h5><a href="ex/B-class.html">B</a></h5>
     <section class="multi-line-signature">
       <div>
         <ol class="annotation-list">
-          <li>override</li>
+          <li>@override</li>
         </ol>
       </div>
       <span class="returntype">void</span>

testing/test_package_docs/ex/B/doNothing.html

@@ -116,7 +116,7 @@ <h5><a href="ex/B-class.html">B</a></h5>
     <section class="multi-line-signature">
       <div>
         <ol class="annotation-list">
-          <li>deprecated</li>
+          <li>@deprecated</li>
         </ol>
       </div>
       <span class="returntype">Future</span>