Make precaching work throughout the documentationFrom tree (#1934)

Author: jcollins-g

lib/src/model.dart

@@ -1929,7 +1929,7 @@ abstract class GetterSetterCombo implements ModelElement {
         _documentationFrom.addAll(setter.documentationFrom);
       }
       if (_documentationFrom.length == 0 ||
-          _documentationFrom.every((e) => e.documentation == ''))
+          _documentationFrom.every((e) => e.documentationComment == ''))
         _documentationFrom = computeDocumentationFrom;
     }
     return _documentationFrom;
@@ -4697,16 +4697,26 @@ class PackageGraph {
 
   /// Generate a list of futures for any docs that actually require precaching.
   Iterable<Future> precacheLocalDocs() sync* {
+    // Prevent reentrancy.
+    Set<ModelElement> precachedElements = new Set();
+
+    Iterable<Future> precacheOneElement(ModelElement m) sync* {
+      for (ModelElement d in m.documentationFrom.where((d) => d.documentationComment != null)) {
+        if (needsPrecacheRegExp.hasMatch(d.documentationComment)
+            && !precachedElements.contains(d)) {
+          precachedElements.add(d);
+          yield d._precacheLocalDocs();
+        }
+      }
+    }
+
     for (ModelElement m in allModelElements) {
       // Skip if there is a canonicalModelElement somewhere else we can run this
       // for.  Not the same as allCanonicalModelElements since we need to run
       // for any ModelElement that might not have a canonical ModelElement,
       // too.
       if (m.canonicalModelElement != null && !m.isCanonical) continue;
-      if (m.documentationComment != null &&
-          needsPrecacheRegExp.hasMatch(m.documentationComment)) {
-        yield m._precacheLocalDocs();
-      }
+      yield* precacheOneElement(m);
     }
   }
 

test/model_test.dart

@@ -121,11 +121,14 @@ void main() {
   group('Tools', () {
     Class toolUser;
     Class _NonCanonicalToolUser, CanonicalToolUser, PrivateLibraryToolUser;
+    Class ImplementingClassForTool, CanonicalPrivateInheritedToolUser;
     Method invokeTool;
     Method invokeToolNoInput;
     Method invokeToolMultipleSections;
     Method invokeToolNonCanonical, invokeToolNonCanonicalSubclass;
     Method invokeToolPrivateLibrary, invokeToolPrivateLibraryOriginal;
+    Method invokeToolParentDoc, invokeToolParentDocOriginal;
+    final RegExp packageInvocationIndexRegexp = new RegExp(r'PACKAGE_INVOCATION_INDEX: (\d+)');
 
     setUpAll(() {
       _NonCanonicalToolUser = fakeLibrary.allClasses
@@ -134,6 +137,10 @@ void main() {
           .firstWhere((c) => c.name == 'CanonicalToolUser');
       PrivateLibraryToolUser = fakeLibrary.allClasses
           .firstWhere((c) => c.name == 'PrivateLibraryToolUser');
+      ImplementingClassForTool = fakeLibrary.allClasses
+          .firstWhere((c) => c.name == 'ImplementingClassForTool');
+      CanonicalPrivateInheritedToolUser = fakeLibrary.allClasses
+          .firstWhere((c) => c.name == 'CanonicalPrivateInheritedToolUser');
       toolUser = exLibrary.classes.firstWhere((c) => c.name == 'ToolUser');
       invokeTool =
           toolUser.allInstanceMethods.firstWhere((m) => m.name == 'invokeTool');
@@ -151,24 +158,48 @@ void main() {
           (invokeToolPrivateLibrary.definingEnclosingElement as Class)
               .allInstanceMethods
               .firstWhere((m) => m.name == 'invokeToolPrivateLibrary');
+      invokeToolParentDoc = CanonicalPrivateInheritedToolUser.allInstanceMethods
+          .firstWhere((m) => m.name == 'invokeToolParentDoc');
+      invokeToolParentDocOriginal = ImplementingClassForTool.allInstanceMethods
+          .firstWhere((m) => m.name == 'invokeToolParentDoc');
       packageGraph.allLocalModelElements.forEach((m) => m.documentation);
     });
 
-    test('does _not_ invoke a tool multiple times unnecessarily', () {
-      RegExp packageInvocationIndexRegexp =
-          new RegExp(r'PACKAGE_INVOCATION_INDEX: (\d+)');
-      expect(invokeToolNonCanonical.isCanonical, isFalse);
-      expect(invokeToolNonCanonicalSubclass.isCanonical, isTrue);
-      expect(
-          packageInvocationIndexRegexp
-              .firstMatch(invokeToolNonCanonical.documentation)
-              .group(1),
-          equals(packageInvocationIndexRegexp
-              .firstMatch(invokeToolNonCanonicalSubclass.documentation)
-              .group(1)));
-      expect(invokeToolPrivateLibrary.documentation, isNot(contains('{@tool')));
-      expect(
-          invokeToolPrivateLibraryOriginal.documentation, contains('{@tool'));
+    test('invokes tool when inherited documentation is the only means for it to be seen', () {
+      // Verify setup of the test is correct.
+      expect(invokeToolParentDoc.isCanonical, isTrue);
+      expect(invokeToolParentDoc.documentationComment, isNull);
+      // Error message here might look strange due to toString() on Methods, but if this
+      // fails that means we don't have the correct invokeToolParentDoc instance.
+      expect(invokeToolParentDoc.documentationFrom, contains(invokeToolParentDocOriginal));
+      // Tool should be substituted out here.
+      expect(invokeToolParentDoc.documentation, isNot(contains('{@tool')));
+    });
+
+    group('does _not_ invoke a tool multiple times unnecessarily', () {
+      test('non-canonical subclass case', () {
+        expect(invokeToolNonCanonical.isCanonical, isFalse);
+        expect(invokeToolNonCanonicalSubclass.isCanonical, isTrue);
+        expect(
+            packageInvocationIndexRegexp
+                .firstMatch(invokeToolNonCanonical.documentation)
+                .group(1),
+            equals(packageInvocationIndexRegexp
+                .firstMatch(invokeToolNonCanonicalSubclass.documentation)
+                .group(1)));
+        expect(invokeToolPrivateLibrary.documentation, isNot(contains('{@tool')));
+        expect(
+            invokeToolPrivateLibraryOriginal.documentation, contains('{@tool'));
+      });
+
+      test('Documentation borrowed from implementer case', () {
+        expect(packageInvocationIndexRegexp
+            .firstMatch(invokeToolParentDoc.documentation)
+            .group(1),
+        equals(packageInvocationIndexRegexp
+            .firstMatch(invokeToolParentDocOriginal.documentation)
+            .group(1)));
+      });
     });
 
     test('can invoke a tool and pass args and environment', () {

testing/test_package/lib/fake.dart

@@ -1021,3 +1021,20 @@ abstract class _NonCanonicalToolUser {
 }
 
 abstract class CanonicalToolUser extends _NonCanonicalToolUser {}
+
+abstract class ImplementingClassForTool {
+  /// Invokes a tool from inherited documentation via `implemented`
+  ///
+  /// {@tool drill}
+  /// This is some drill text right here.
+  /// {@end-tool}
+  void invokeToolParentDoc();
+}
+
+/// The method [invokeToolParentDoc] gets its documentation from an interface class.
+abstract class CanonicalPrivateInheritedToolUser implements ImplementingClassForTool {
+  @override
+  void invokeToolParentDoc() {
+    print('hello, tool world');
+  }
+}