Fix macro detection for macros declared in non-public symbols (#1584)

* Add basic support for private macros

* update test package docs

* dartfmt

* rebuild test package docs post-merge

Author: jcollins-g

lib/src/model.dart

@@ -2718,24 +2718,28 @@ abstract class ModelElement extends Canonicalization
     return docFrom;
   }
 
-  String __documentationLocal() {
+  String _documentationLocal() {
     if (config.dropTextFrom.contains(element.library.name)) {
       return '';
     }
     String _rawDocs = computeDocumentationComment ?? '';
     _rawDocs = stripComments(_rawDocs) ?? '';
     _rawDocs = _injectExamples(_rawDocs);
     _rawDocs = _stripMacroTemplatesAndAddToIndex(_rawDocs);
-    _rawDocs = _injectMacros(_rawDocs);
     return _rawDocs;
   }
 
-  String get _documentationLocal => _memoizer.memoized(__documentationLocal);
+  /// Returns the documentation for this literal element unless
+  /// [config.dropTextFrom] indicates it should not be returned.  Macro
+  /// definitions are stripped, but macros themselves are not injected.  This
+  /// is a two stage process to avoid ordering problems.
+  String get documentationLocal => _memoizer.memoized(_documentationLocal);
 
   /// Returns the docs, stripped of their leading comments syntax.
   @override
   String get documentation {
-    return documentationFrom.map((e) => e._documentationLocal).join('<p>');
+    return _injectMacros(
+        documentationFrom.map((e) => e.documentationLocal).join('<p>'));
   }
 
   Library get definingLibrary => package.findOrCreateLibraryFor(element);
@@ -3376,7 +3380,7 @@ abstract class ModelElement extends Canonicalization
   ///
   /// Example:
   ///
-  /// You define the template anywhere in the comments like:
+  /// You define the template in any comment for a documentable entity like:
   ///
   ///     {@template foo}
   ///     Foo contents!
@@ -3397,7 +3401,11 @@ abstract class ModelElement extends Canonicalization
   String _injectMacros(String rawDocs) {
     final macroRegExp = new RegExp(r'{@macro\s+([^}]+)}');
     return rawDocs.replaceAllMapped(macroRegExp, (match) {
-      return package.getMacro(match[1]);
+      String macro = package.getMacro(match[1]);
+      if (macro == null) {
+        warn(PackageWarning.unknownMacro, message: match[1]);
+      }
+      return macro;
     });
   }
 
@@ -3414,7 +3422,7 @@ abstract class ModelElement extends Canonicalization
         r'[ ]*{@template\s+(.+?)}([\s\S]+?){@endtemplate}[ ]*\n?',
         multiLine: true);
     return rawDocs.replaceAllMapped(templateRegExp, (match) {
-      package.addMacro(match[1].trim(), match[2].trim());
+      package._addMacro(match[1].trim(), match[2].trim());
       return "";
     });
   }
@@ -3690,6 +3698,7 @@ class Package extends Canonicalization with Nameable, Warnable, Memoizeable {
   final Map<Element, Library> _elementToLibrary = {};
   final Map<String, String> _macros = {};
   bool allLibrariesAdded = false;
+  bool _macrosAdded = false;
 
   Package(Iterable<LibraryElement> libraryElements, this.packageMeta,
       this._packageWarningOptions, this.context,
@@ -3712,10 +3721,10 @@ class Package extends Canonicalization with Nameable, Warnable, Memoizeable {
     });
 
     _implementors.values.forEach((l) => l.sort());
-    // Go through docs of every model element in package to prebuild the macros index
-    // TODO(jcollins-g): move index building into a cached-on-demand generation
-    // like most other bits in [Package].
-    allCanonicalModelElements.forEach((m) => m.documentation);
+    // Go through docs of every model element in package to prebuild the macros
+    // index.
+    allModelElements.forEach((m) => m.documentationLocal);
+    _macrosAdded = true;
   }
 
   Set<String> _allRootDirs() =>
@@ -3877,6 +3886,9 @@ class Package extends Canonicalization with Nameable, Warnable, Memoizeable {
         }
         referredFromPrefix = 'in documentation inherited from';
         break;
+      case PackageWarning.unknownMacro:
+        warningMessage = "undefined macro [${message}]";
+        break;
       case PackageWarning.brokenLink:
         warningMessage = 'dartdoc generated a broken link to: ${message}';
         warnablePrefix = 'to element';
@@ -4325,9 +4337,13 @@ class Package extends Canonicalization with Nameable, Warnable, Memoizeable {
   Iterable<ModelElement> get allCanonicalModelElements =>
       _memoizer.memoized(_allCanonicalModelElements);
 
-  String getMacro(String name) => _macros[name];
+  String getMacro(String name) {
+    assert(_macrosAdded);
+    return _macros[name];
+  }
 
-  void addMacro(String name, String content) {
+  void _addMacro(String name, String content) {
+    assert(!_macrosAdded);
     _macros[name] = content;
   }
 }

lib/src/warnings.dart

@@ -64,6 +64,10 @@ final Map<PackageWarning, PackageWarningHelpText> packageWarningText = const {
       PackageWarning.brokenLink,
       "brokenLink",
       "Dartdoc generated a link to a non-existent file"),
+  PackageWarning.unknownMacro: const PackageWarningHelpText(
+      PackageWarning.unknownMacro,
+      "unknownMacro",
+      "A comment reference contains an unknown macro"),
   PackageWarning.orphanedFile: const PackageWarningHelpText(
       PackageWarning.orphanedFile,
       "orphanedFile",
@@ -115,6 +119,7 @@ enum PackageWarning {
   noLibraryLevelDocs,
   categoryOrderGivesMissingPackageName,
   unresolvedDocReference,
+  unknownMacro,
   brokenLink,
   orphanedFile,
   unknownFile,

test/model_test.dart

@@ -312,14 +312,18 @@ void main() {
 
   group('Macros', () {
     Class dog;
-    Method withMacro, withMacro2;
+    Method withMacro, withMacro2, withPrivateMacro, withUndefinedMacro;
 
     setUp(() {
       dog = exLibrary.classes.firstWhere((c) => c.name == 'Dog');
       withMacro =
           dog.allInstanceMethods.firstWhere((m) => m.name == 'withMacro');
       withMacro2 =
           dog.allInstanceMethods.firstWhere((m) => m.name == 'withMacro2');
+      withPrivateMacro =
+          dog.allInstanceMethods.firstWhere((m) => m.name == 'withPrivateMacro');
+      withUndefinedMacro =
+          dog.allInstanceMethods.firstWhere((m) => m.name == 'withUndefinedMacro');
       package.allModelElements.forEach((m) => m.documentation);
     });
 
@@ -332,6 +336,14 @@ void main() {
         () {
       expect(withMacro2.documentation, equals("Foo macro content"));
     });
+
+    test("renders a macro defined in a private symbol", () {
+      expect(withPrivateMacro.documentation, contains("Private macro content"));
+    });
+
+    test("a warning is generated for unknown macros", () {
+      expect(package.packageWarningCounter.hasWarning(withUndefinedMacro, PackageWarning.unknownMacro, 'ThatDoesNotExist'), isTrue);
+    });
   });
 
   group('MultiplyInheritedExecutableElement handling', () {
@@ -808,7 +820,7 @@ void main() {
     });
 
     test('get methods', () {
-      expect(Dog.publicInstanceMethods, hasLength(9));
+      expect(Dog.publicInstanceMethods, hasLength(12));
     });
 
     test('get operators', () {
@@ -860,7 +872,6 @@ void main() {
     });
 
     test('F has many inherited methods', () {
-      expect(F.publicInheritedMethods, hasLength(12));
       expect(
           F.publicInheritedMethods.map((im) => im.name),
           equals([
@@ -875,7 +886,9 @@ void main() {
             'testMethod',
             'toString',
             'withMacro',
-            'withMacro2'
+            'withMacro2',
+            'withPrivateMacro',
+            'withUndefinedMacro',
           ]));
     });
 

testing/test_package/lib/example.dart

@@ -343,6 +343,17 @@ class Dog implements Cat, E {
   /// {@macro foo}
   void withMacro2() {}
 
+  /// {@template private}
+  /// Private macro content
+  /// {@endtemplate}
+  void _macroDefinedPrivately() {}
+
+  /// Use a privately defined macro: {@macro private}
+  void withPrivateMacro() {}
+
+  /// Don't define this:  {@macro ThatDoesNotExist}
+  void withUndefinedMacro() {}
+
   void testGeneric(Map<String, dynamic> args) {}
 
   void testMethod(Iterable it) {}

testing/test_package_docs/ex/Dog-class.html

@@ -330,6 +330,24 @@ <h2>Methods</h2>
         <dd>
           Foo macro content
 
+</dd>
+        <dt id="withPrivateMacro" class="callable">
+          <span class="name"><a href="ex/Dog/withPrivateMacro.html">withPrivateMacro</a></span><span class="signature">(<wbr>)
+            <span class="returntype parameter">&#8594; void</span>
+          </span>
+        </dt>
+        <dd>
+          Use a privately defined macro: Private macro content
+          
+</dd>
+        <dt id="withUndefinedMacro" class="callable">
+          <span class="name"><a href="ex/Dog/withUndefinedMacro.html">withUndefinedMacro</a></span><span class="signature">(<wbr>)
+            <span class="returntype parameter">&#8594; void</span>
+          </span>
+        </dt>
+        <dd>
+          Don't define this:  null
+          
 </dd>
         <dt id="noSuchMethod" class="callable inherited">
           <span class="name"><a href="ex/E/noSuchMethod.html">noSuchMethod</a></span><span class="signature">(<wbr><span class="parameter" id="noSuchMethod-param-invocation"><span class="type-annotation">Invocation</span> <span class="parameter-name">invocation</span></span>)
@@ -466,6 +484,8 @@ <h2>Constants</h2>
       <li><a href="ex/Dog/testMethod.html">testMethod</a></li>
       <li><a href="ex/Dog/withMacro.html">withMacro</a></li>
       <li><a href="ex/Dog/withMacro2.html">withMacro2</a></li>
+      <li><a href="ex/Dog/withPrivateMacro.html">withPrivateMacro</a></li>
+      <li><a href="ex/Dog/withUndefinedMacro.html">withUndefinedMacro</a></li>
       <li class="inherited"><a href="ex/E/noSuchMethod.html">noSuchMethod</a></li>
       <li class="inherited"><a href="ex/E/toString.html">toString</a></li>
 

testing/test_package_docs/ex/Dog/Dog.deprecatedCreate.html

@@ -67,6 +67,8 @@ <h5>Dog class</h5>
       <li><a href="ex/Dog/testMethod.html">testMethod</a></li>
       <li><a href="ex/Dog/withMacro.html">withMacro</a></li>
       <li><a href="ex/Dog/withMacro2.html">withMacro2</a></li>
+      <li><a href="ex/Dog/withPrivateMacro.html">withPrivateMacro</a></li>
+      <li><a href="ex/Dog/withUndefinedMacro.html">withUndefinedMacro</a></li>
       <li class="inherited"><a href="ex/E/noSuchMethod.html">noSuchMethod</a></li>
       <li class="inherited"><a href="ex/E/toString.html">toString</a></li>
 

testing/test_package_docs/ex/Dog/Dog.html

@@ -67,6 +67,8 @@ <h5>Dog class</h5>
       <li><a href="ex/Dog/testMethod.html">testMethod</a></li>
       <li><a href="ex/Dog/withMacro.html">withMacro</a></li>
       <li><a href="ex/Dog/withMacro2.html">withMacro2</a></li>
+      <li><a href="ex/Dog/withPrivateMacro.html">withPrivateMacro</a></li>
+      <li><a href="ex/Dog/withUndefinedMacro.html">withUndefinedMacro</a></li>
       <li class="inherited"><a href="ex/E/noSuchMethod.html">noSuchMethod</a></li>
       <li class="inherited"><a href="ex/E/toString.html">toString</a></li>
 

testing/test_package_docs/ex/Dog/aFinalField.html

@@ -67,6 +67,8 @@ <h5>Dog class</h5>
       <li><a href="ex/Dog/testMethod.html">testMethod</a></li>
       <li><a href="ex/Dog/withMacro.html">withMacro</a></li>
       <li><a href="ex/Dog/withMacro2.html">withMacro2</a></li>
+      <li><a href="ex/Dog/withPrivateMacro.html">withPrivateMacro</a></li>
+      <li><a href="ex/Dog/withUndefinedMacro.html">withUndefinedMacro</a></li>
       <li class="inherited"><a href="ex/E/noSuchMethod.html">noSuchMethod</a></li>
       <li class="inherited"><a href="ex/E/toString.html">toString</a></li>
 

testing/test_package_docs/ex/Dog/aGetterReturningRandomThings.html

@@ -67,6 +67,8 @@ <h5>Dog class</h5>
       <li><a href="ex/Dog/testMethod.html">testMethod</a></li>
       <li><a href="ex/Dog/withMacro.html">withMacro</a></li>
       <li><a href="ex/Dog/withMacro2.html">withMacro2</a></li>
+      <li><a href="ex/Dog/withPrivateMacro.html">withPrivateMacro</a></li>
+      <li><a href="ex/Dog/withUndefinedMacro.html">withUndefinedMacro</a></li>
       <li class="inherited"><a href="ex/E/noSuchMethod.html">noSuchMethod</a></li>
       <li class="inherited"><a href="ex/E/toString.html">toString</a></li>
 

testing/test_package_docs/ex/Dog/aName-constant.html

@@ -67,6 +67,8 @@ <h5>Dog class</h5>
       <li><a href="ex/Dog/testMethod.html">testMethod</a></li>
       <li><a href="ex/Dog/withMacro.html">withMacro</a></li>
       <li><a href="ex/Dog/withMacro2.html">withMacro2</a></li>
+      <li><a href="ex/Dog/withPrivateMacro.html">withPrivateMacro</a></li>
+      <li><a href="ex/Dog/withUndefinedMacro.html">withUndefinedMacro</a></li>
       <li class="inherited"><a href="ex/E/noSuchMethod.html">noSuchMethod</a></li>
       <li class="inherited"><a href="ex/E/toString.html">toString</a></li>