Minimize number of tool execution runs (#1898)

* canonicalFor must be declared literally in the library

* Add tests for minimizing tool execution

* dartfmt

* Workaround problem with mixins on 2.1.0 stable

* Review comments

Author: jcollins-g

lib/src/model.dart

@@ -1300,7 +1300,7 @@ abstract class Documentable extends Nameable {
 abstract class Categorization implements ModelElement {
   @override
   String _buildDocumentationAddition(String rawDocs) =>
-      _stripAndSetDartdocCategories(rawDocs);
+      _stripAndSetDartdocCategories(rawDocs ??= '');
 
   /// Parse {@category ...} and related information in API comments, stripping
   /// out that information from the given comments and returning the stripped
@@ -2263,7 +2263,7 @@ class Library extends ModelElement with Categorization, TopLevelContainer {
   Set<String> get canonicalFor {
     if (_canonicalFor == null) {
       // TODO(jcollins-g): restructure to avoid using side effects.
-      documentation;
+      _buildDocumentationAddition(documentationComment);
     }
     return _canonicalFor;
   }
@@ -2273,14 +2273,14 @@ class Library extends ModelElement with Categorization, TopLevelContainer {
   ///
   /// Example:
   ///   {@canonicalFor libname.ClassName}
-  String _setCanonicalFor(String rawDocs) {
-    if (_canonicalFor == null) {
-      _canonicalFor = new Set();
-    }
+  @override
+  String _buildDocumentationAddition(String rawDocs) {
+    rawDocs = super._buildDocumentationAddition(rawDocs);
+    Set<String> newCanonicalFor = new Set();
     Set<String> notFoundInAllModelElements = new Set();
     final canonicalRegExp = new RegExp(r'{@canonicalFor\s([^}]+)}');
     rawDocs = rawDocs.replaceAllMapped(canonicalRegExp, (Match match) {
-      canonicalFor.add(match.group(1));
+      newCanonicalFor.add(match.group(1));
       notFoundInAllModelElements.add(match.group(1));
       return '';
     });
@@ -2290,16 +2290,12 @@ class Library extends ModelElement with Categorization, TopLevelContainer {
     for (String notFound in notFoundInAllModelElements) {
       warn(PackageWarning.ignoredCanonicalFor, message: notFound);
     }
-    return rawDocs;
-  }
-
-  String _libraryDocs;
-  @override
-  String get documentation {
-    if (_libraryDocs == null) {
-      _libraryDocs = _setCanonicalFor(super.documentation);
+    // TODO(jcollins-g): warn if a macro/tool _does_ generate an unexpected
+    // canonicalFor?
+    if (_canonicalFor == null) {
+      _canonicalFor = newCanonicalFor;
     }
-    return _libraryDocs;
+    return rawDocs;
   }
 
   /// Libraries are not enclosed by anything.
@@ -3250,13 +3246,14 @@ abstract class ModelElement extends Canonicalization
 
   /// Override this to add more features to the documentation builder in a
   /// subclass.
-  String _buildDocumentationAddition(String docs) => docs;
+  String _buildDocumentationAddition(String docs) => docs ??= '';
 
   /// Separate from _buildDocumentationLocal for overriding.
   String _buildDocumentationBaseSync() {
-    assert(_rawDocs == null);
+    assert(_rawDocs == null, 'reentrant calls to _buildDocumentation* not allowed');
     // Do not use the sync method if we need to evaluate tools or templates.
-    assert(packageGraph._localDocumentationBuilt);
+    assert(!isCanonical ||
+        !needsPrecacheRegExp.hasMatch(documentationComment ?? ''));
     if (config.dropTextFrom.contains(element.library.name)) {
       _rawDocs = '';
     } else {
@@ -3273,7 +3270,7 @@ abstract class ModelElement extends Canonicalization
   /// Separate from _buildDocumentationLocal for overriding.  Can only be
   /// used as part of [PackageGraph.setUpPackageGraph].
   Future<String> _buildDocumentationBase() async {
-    assert(_rawDocs == null);
+    assert(_rawDocs == null, 'reentrant calls to _buildDocumentation* not allowed');
     // Do not use the sync method if we need to evaluate tools or templates.
     if (config.dropTextFrom.contains(element.library.name)) {
       _rawDocs = '';
@@ -4037,6 +4034,8 @@ abstract class ModelElement extends Canonicalization
               'LIBRARY_NAME': library?.fullyQualifiedName,
               'ELEMENT_NAME': fullyQualifiedNameWithoutLibrary,
               'INVOCATION_INDEX': invocationIndex.toString(),
+              'PACKAGE_INVOCATION_INDEX':
+                  (package.toolInvocationIndex++).toString(),
             }..removeWhere((key, value) => value == null));
       });
     } else {
@@ -4688,6 +4687,11 @@ class PackageGraph {
   /// Generate a list of futures for any docs that actually require precaching.
   Iterable<Future> precacheLocalDocs() sync* {
     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();
@@ -5800,6 +5804,9 @@ class Package extends LibraryContainer
   @override
   Library get canonicalLibrary => null;
 
+  /// Number of times we have invoked a tool for this package.
+  int toolInvocationIndex = 0;
+
   /// Pieces of the location split by [locationSplitter] (removing package: and
   /// slashes).
   @override

test/model_test.dart

@@ -84,20 +84,57 @@ void main() {
 
   group('Tools', () {
     Class toolUser;
+    Class _NonCanonicalToolUser, CanonicalToolUser, PrivateLibraryToolUser;
     Method invokeTool;
     Method invokeToolNoInput;
     Method invokeToolMultipleSections;
+    Method invokeToolNonCanonical, invokeToolNonCanonicalSubclass;
+    Method invokeToolPrivateLibrary, invokeToolPrivateLibraryOriginal;
 
     setUpAll(() {
+      _NonCanonicalToolUser = fakeLibrary.allClasses
+          .firstWhere((c) => c.name == '_NonCanonicalToolUser');
+      CanonicalToolUser = fakeLibrary.allClasses
+          .firstWhere((c) => c.name == 'CanonicalToolUser');
+      PrivateLibraryToolUser = fakeLibrary.allClasses
+          .firstWhere((c) => c.name == 'PrivateLibraryToolUser');
       toolUser = exLibrary.classes.firstWhere((c) => c.name == 'ToolUser');
       invokeTool =
           toolUser.allInstanceMethods.firstWhere((m) => m.name == 'invokeTool');
+      invokeToolNonCanonical = _NonCanonicalToolUser.allInstanceMethods
+          .firstWhere((m) => m.name == 'invokeToolNonCanonical');
+      invokeToolNonCanonicalSubclass = CanonicalToolUser.allInstanceMethods
+          .firstWhere((m) => m.name == 'invokeToolNonCanonical');
       invokeToolNoInput = toolUser.allInstanceMethods
           .firstWhere((m) => m.name == 'invokeToolNoInput');
       invokeToolMultipleSections = toolUser.allInstanceMethods
           .firstWhere((m) => m.name == 'invokeToolMultipleSections');
+      invokeToolPrivateLibrary = PrivateLibraryToolUser.allInstanceMethods
+          .firstWhere((m) => m.name == 'invokeToolPrivateLibrary');
+      invokeToolPrivateLibraryOriginal =
+          (invokeToolPrivateLibrary.definingEnclosingElement as Class)
+              .allInstanceMethods
+              .firstWhere((m) => m.name == 'invokeToolPrivateLibrary');
       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('can invoke a tool and pass args and environment', () {
       expect(invokeTool.documentation, contains('--file=<INPUT_FILE>'));
       expect(

testing/test_package/bin/drill.dart

@@ -36,6 +36,7 @@ void main(List<String> argList) {
     'LIBRARY_NAME',
     'ELEMENT_NAME',
     'INVOCATION_INDEX',
+    'PACKAGE_INVOCATION_INDEX',
   ]);
   Map<String, String> env = <String, String>{}..addAll(Platform.environment);
   env.removeWhere((String key, String value) => !variableNames.contains(key));

testing/test_package/lib/fake.dart

@@ -60,6 +60,8 @@ import 'two_exports.dart' show BaseClass;
 export 'package:test_package_imported/categoryExporting.dart'
     show IAmAClassWithCategories;
 
+export 'src/tool.dart';
+
 // Explicitly export ourselves, because why not.
 // ignore: uri_does_not_exist
 export 'package:test_package/fake.dart';
@@ -1004,3 +1006,14 @@ abstract class MIEEMixin<K, V> implements MIEEThing<K, V> {
 abstract class MIEEThing<K, V> {
   void operator []=(K key, V value);
 }
+
+abstract class _NonCanonicalToolUser {
+  /// Invokes a tool without the $INPUT token or args.
+  ///
+  /// {@tool drill}
+  /// Some text in the drill that references [noInvokeTool].
+  /// {@end-tool}
+  void invokeToolNonCanonical();
+}
+
+abstract class CanonicalToolUser extends _NonCanonicalToolUser {}

testing/test_package/lib/src/tool.dart

@@ -0,0 +1,10 @@
+library test_package.tool;
+
+abstract class PrivateLibraryToolUser {
+  /// Invokes a tool from a private library.
+  ///
+  /// {@tool drill}
+  /// Some text in the drill.
+  /// {@end-tool}
+  void invokeToolPrivateLibrary();
+}