Add package global and graph global lookups to new lookup code (#2684)

* Handle different name styles in constructor

* dartfmt

* Fixes seem to work.

* Rebuild

* rebuild post-merge

* fix analysis error in fake

* Package globals on

* Implement package global and graph global lookups

Author: jcollins-g

lib/src/comment_references/parser.dart

@@ -142,7 +142,6 @@ class CommentReferenceParser {
   }
 
   static const _constructorHintPrefix = 'new';
-
   static const _ignorePrefixes = ['const', 'final', 'var'];
 
   /// Implement parsing a prefix to a comment reference.
@@ -153,17 +152,19 @@ class CommentReferenceParser {
   ///
   /// <constructorPrefixHint> ::= 'new '
   ///
-  /// <leadingJunk> ::= ('const' | 'final' | 'var' | 'operator')(' '+)
+  /// <leadingJunk> ::= ('const' | 'final' | 'var')(' '+)
   /// ```
   _PrefixParseResult _parsePrefix() {
     if (_atEnd) {
       return _PrefixParseResult.endOfFile;
     }
-    if (_tryMatchLiteral(_constructorHintPrefix)) {
+    if (_tryMatchLiteral(_constructorHintPrefix,
+        requireTrailingNonidentifier: true)) {
       return _PrefixParseResult.ok(
           ConstructorHintStartNode(_constructorHintPrefix));
     }
-    if (_ignorePrefixes.any((p) => _tryMatchLiteral(p))) {
+    if (_ignorePrefixes
+        .any((p) => _tryMatchLiteral(p, requireTrailingNonidentifier: true))) {
       return _PrefixParseResult.junk;
     }
 
@@ -278,10 +279,12 @@ class CommentReferenceParser {
 
   /// Advances [_index] on match, preserves on non-match.
   bool _tryMatchLiteral(String characters,
-      {bool acceptTrailingWhitespace = true}) {
+      {bool acceptTrailingWhitespace = true,
+      bool requireTrailingNonidentifier = false}) {
     assert(acceptTrailingWhitespace != null);
     if (characters.length + _index > _referenceLength) return false;
-    for (var startIndex = _index;
+    int startIndex;
+    for (startIndex = _index;
         _index - startIndex < characters.length;
         _index++) {
       if (codeRef.codeUnitAt(_index) !=
@@ -290,6 +293,12 @@ class CommentReferenceParser {
         return false;
       }
     }
+    if (requireTrailingNonidentifier) {
+      if (_atEnd || !_nonIdentifierChars.contains(_thisChar)) {
+        _index = startIndex;
+        return false;
+      }
+    }
     if (acceptTrailingWhitespace) _walkPastWhitespace();
     return true;
   }

lib/src/markdown_processor.dart

@@ -191,23 +191,34 @@ class MatchingLinkResult {
 
   bool isEquivalentTo(MatchingLinkResult other) {
     if (this == other) return true;
-    if (modelElement?.canonicalModelElement ==
-        other.modelElement?.canonicalModelElement) return true;
+    var compareThis = modelElement;
+    var compareOther = other.modelElement;
+
+    if (compareThis is Accessor) {
+      compareThis = (compareThis as Accessor).enclosingCombo;
+    }
+
+    if (compareOther is Accessor) {
+      compareOther = (compareOther as Accessor).enclosingCombo;
+    }
+
+    if (compareThis?.canonicalModelElement ==
+        compareOther?.canonicalModelElement) return true;
     // The old implementation just throws away Parameter matches to avoid
     // problems with warning unnecessarily at higher levels of the code.
     // I'd like to fix this at a different layer with the new lookup, so treat
     // this as equivalent to a null type.
-    if (other.modelElement is Parameter && modelElement == null) {
+    if (compareOther is Parameter && compareThis == null) {
       return true;
     }
-    if (modelElement is Parameter && other.modelElement == null) {
+    if (compareThis is Parameter && compareOther == null) {
       return true;
     }
     // Same with TypeParameter.
-    if (other.modelElement is TypeParameter && modelElement == null) {
+    if (compareOther is TypeParameter && compareThis == null) {
       return true;
     }
-    if (modelElement is TypeParameter && other.modelElement == null) {
+    if (compareThis is TypeParameter && compareOther == null) {
       return true;
     }
     return false;
@@ -290,6 +301,10 @@ bool _rejectDefaultConstructors(CommentReferable referable) {
       referable.name == referable.enclosingElement.name) {
     return false;
   }
+  // Avoid accidentally preferring arguments of the default constructor.
+  if (referable is ModelElement && referable.enclosingElement is Constructor) {
+    return false;
+  }
   return true;
 }
 

lib/src/model/library.dart

@@ -662,9 +662,21 @@ class Library extends ModelElement with Categorization, TopLevelContainer {
   Map<String, CommentReferable> _referenceChildren;
   @override
   Map<String, CommentReferable> get referenceChildren {
-    return _referenceChildren ??= Map.fromEntries(
-        element.exportNamespace.definedNames.entries.map((entry) => MapEntry(
-            entry.key, ModelElement.fromElement(entry.value, packageGraph))));
+    if (_referenceChildren == null) {
+      _referenceChildren = Map.fromEntries(
+          element.exportNamespace.definedNames.entries.map((entry) => MapEntry(
+              entry.key, ModelElement.fromElement(entry.value, packageGraph))));
+      // TODO(jcollins-g): warn and get rid of this case where it shows up.
+      // If a user is hiding parts of a prefix import, the user should not
+      // refer to hidden members via the prefix, because that can be
+      // ambiguous.  dart-lang/dartdoc#2683.
+      for (var prefixEntry in prefixToLibrary.entries) {
+        if (!_referenceChildren.containsKey(prefixEntry.key)) {
+          _referenceChildren[prefixEntry.key] = prefixEntry.value.first;
+        }
+      }
+    }
+    return _referenceChildren;
   }
 
   @override

lib/src/model/package.dart

@@ -408,6 +408,18 @@ class Package extends LibraryContainer
       _referenceChildren = {};
       _referenceChildren
           .addEntries(allLibraries.map((l) => MapEntry(l.name, l)));
+      // Do not override any preexisting data, and insert based on the
+      // public library sort order.
+      // TODO(jcollins-g): warn when results require package-global
+      // lookups like this.
+      for (var lib in publicLibrariesSorted) {
+        for (var referableEntry in lib.referenceChildren.entries) {
+          // Avoiding tearoffs for performance reasons.
+          if (!_referenceChildren.containsKey(referableEntry.key)) {
+            _referenceChildren[referableEntry.key] = referableEntry.value;
+          }
+        }
+      }
     }
     return _referenceChildren;
   }

lib/src/model/package_graph.dart

@@ -1030,26 +1030,39 @@ class PackageGraph with CommentReferable, Nameable {
   Map<String, CommentReferable> get referenceChildren {
     if (_referenceChildren == null) {
       _referenceChildren = {};
-
+      // Packages are the top priority.
       _referenceChildren.addEntries(packages.map((p) => MapEntry(p.name, p)));
-      // TODO(jcollins-g): deprecate and start warning for anything needing these
-      // elements.
-      var librariesWithCanonicals =
-          packages.expand((p) => referenceChildren.entries.where((e) {
-                var v = e.value;
-                return v is ModelElement
-                    ? v.canonicalModelElement != null
-                    : true;
-              }));
-      var topLevelsWithCanonicals = librariesWithCanonicals
-          .expand((p) => referenceChildren.entries.where((e) {
-                var v = e.value;
-                return v is ModelElement
-                    ? v.canonicalModelElement != null
-                    : true;
-              }));
-      _referenceChildren.addEntries(librariesWithCanonicals);
-      _referenceChildren.addEntries(topLevelsWithCanonicals);
+
+      // Libraries are next.
+      // TODO(jcollins-g): Warn about directly referencing libraries out of
+      // scope?
+      for (var p in documentedPackages) {
+        for (var lib in p.publicLibrariesSorted) {
+          if (!_referenceChildren.containsKey(lib.name)) {
+            _referenceChildren[lib.name] = lib;
+          }
+        }
+      }
+      // TODO(jcollins-g): Warn about directly referencing top level items
+      // out of scope?
+      for (var p in documentedPackages) {
+        for (var lib in p.publicLibrariesSorted) {
+          for (var me in [
+            ...lib.publicConstants,
+            ...lib.publicFunctions,
+            ...lib.publicProperties,
+            ...lib.publicTypedefs,
+            ...lib.publicExtensions,
+            ...lib.publicClasses,
+            ...lib.publicEnums,
+            ...lib.publicMixins
+          ]) {
+            if (!_referenceChildren.containsKey(me.name)) {
+              _referenceChildren[me.name] = me;
+            }
+          }
+        }
+      }
     }
     return _referenceChildren;
   }

test/comment_referable/parser_test.dart

@@ -7,33 +7,36 @@ import 'package:test/test.dart';
 
 void main() {
   void expectParseEquivalent(String codeRef, List<String> parts,
-      {bool constructorHint = false}) {
+      {bool constructorHint = false, bool callableHint = false}) {
     var result = CommentReferenceParser(codeRef).parse();
-    var hasHint = result.isNotEmpty &&
-        (result.first is ConstructorHintStartNode ||
-            result.last is CallableHintEndNode);
+    var hasConstructorHint =
+        result.isNotEmpty && result.first is ConstructorHintStartNode;
+    var hasCallableHint =
+        result.isNotEmpty && result.last is CallableHintEndNode;
     var stringParts = result.whereType<IdentifierNode>().map((i) => i.text);
     expect(stringParts, equals(parts));
-    expect(hasHint, equals(constructorHint));
+    expect(hasConstructorHint, equals(constructorHint));
+    expect(hasCallableHint, equals(callableHint));
   }
 
   void expectParsePassthrough(String codeRef) =>
       expectParseEquivalent(codeRef, [codeRef]);
 
   void expectParseError(String codeRef) {
-    expect(CommentReferenceParser(codeRef).parse(), isEmpty);
+    expect(CommentReferenceParser(codeRef).parse().whereType<IdentifierNode>(),
+        isEmpty);
   }
 
   group('Basic comment reference parsing', () {
     test('Check that basic references parse', () {
       expectParseEquivalent('valid', ['valid']);
       expectParseEquivalent('new valid', ['valid'], constructorHint: true);
-      expectParseEquivalent('valid()', ['valid'], constructorHint: true);
-      expectParseEquivalent('const valid()', ['valid'], constructorHint: true);
+      expectParseEquivalent('valid()', ['valid'], callableHint: true);
+      expectParseEquivalent('const valid()', ['valid'], callableHint: true);
       expectParseEquivalent('final valid', ['valid']);
       expectParseEquivalent('this.is.valid', ['this', 'is', 'valid']);
       expectParseEquivalent('this.is.valid()', ['this', 'is', 'valid'],
-          constructorHint: true);
+          callableHint: true);
       expectParseEquivalent('new this.is.valid', ['this', 'is', 'valid'],
           constructorHint: true);
       expectParseEquivalent('const this.is.valid', ['this', 'is', 'valid']);
@@ -46,6 +49,19 @@ void main() {
       expectParseEquivalent('this.is.valid(things)', ['this', 'is', 'valid']);
     });
 
+    test('Check that cases dependent on prefix resolution not firing parse',
+        () {
+      expectParsePassthrough('constant');
+      expectParsePassthrough('newThingy');
+      expectParsePassthrough('operatorThingy');
+      expectParseEquivalent('operator+', ['+']);
+      expectParseError('const()');
+      // TODO(jcollins-g): might need to revisit these two with constructor
+      // tearoffs?
+      expectParsePassthrough('new');
+      expectParseError('new()');
+    });
+
     test('Check that operator references parse', () {
       expectParsePassthrough('[]');
       expectParsePassthrough('<=');

test/end2end/model_test.dart

@@ -2137,12 +2137,17 @@ void main() {
         nameWithSingleUnderscore,
         theOnlyThingInTheLibrary;
     Constructor aNonDefaultConstructor, defaultConstructor;
-    Class Apple, BaseClass, baseForDocComments, ExtraSpecialList, string;
+    Class Apple,
+        BaseClass,
+        baseForDocComments,
+        ExtraSpecialList,
+        string,
+        metaUseResult;
     Method doAwesomeStuff, anotherMethod;
     // ignore: unused_local_variable
     Operator bracketOperator, bracketOperatorOtherClass;
     Parameter doAwesomeStuffParam;
-    Field forInheriting, action, initializeMe;
+    Field forInheriting, action, initializeMe, somethingShadowy;
 
     setUpAll(() async {
       nameWithTwoUnderscores = fakeLibrary.constants
@@ -2153,6 +2158,10 @@ void main() {
           .firstWhere((e) => e.name == 'dart:core')
           .allClasses
           .firstWhere((c) => c.name == 'String');
+      metaUseResult = packageGraph.allLibraries.values
+          .firstWhere((e) => e.name == 'meta')
+          .allClasses
+          .firstWhere((c) => c.name == 'UseResult');
       baseForDocComments =
           fakeLibrary.classes.firstWhere((c) => c.name == 'BaseForDocComments');
       aNonDefaultConstructor = baseForDocComments.constructors.firstWhere(
@@ -2161,6 +2170,8 @@ void main() {
           .firstWhere((c) => c.name == 'BaseForDocComments');
       initializeMe = baseForDocComments.allFields
           .firstWhere((f) => f.name == 'initializeMe');
+      somethingShadowy = baseForDocComments.allFields
+          .firstWhere((f) => f.name == 'somethingShadowy');
       doAwesomeStuff = baseForDocComments.instanceMethods
           .firstWhere((m) => m.name == 'doAwesomeStuff');
       anotherMethod = baseForDocComments.instanceMethods
@@ -2248,12 +2259,23 @@ void main() {
           equals(MatchingLinkResult(aNonDefaultConstructor)));
     });
 
+    test('Deprecated lookup styles still function', () {
+      // dart-lang/dartdoc#2683
+      expect(bothLookup(baseForDocComments, 'aPrefix.UseResult'),
+          equals(MatchingLinkResult(metaUseResult)));
+    });
+
     test('Verify basic linking inside class', () {
       expect(
           bothLookup(
               baseForDocComments, 'BaseForDocComments.BaseForDocComments'),
           equals(MatchingLinkResult(defaultConstructor)));
 
+      // We don't want the parameter on the default constructor, here.
+      expect(
+          bothLookup(baseForDocComments, 'BaseForDocComments.somethingShadowy'),
+          equals(MatchingLinkResult(somethingShadowy)));
+
       expect(bothLookup(doAwesomeStuff, 'aNonDefaultConstructor'),
           equals(MatchingLinkResult(aNonDefaultConstructor)));
 
@@ -2311,8 +2333,7 @@ void main() {
           equals(MatchingLinkResult(theOnlyThingInTheLibrary)));
 
       // A name that exists in this package but is not imported.
-      // TODO(jcollins-g): package-wide lookups are not yet implemented with the new lookup code.
-      expect(originalLookup(doAwesomeStuff, 'doesStuff'),
+      expect(bothLookup(doAwesomeStuff, 'doesStuff'),
           equals(MatchingLinkResult(doesStuff)));
 
       // A name of a class from an import of a library that exported that name.
@@ -2339,8 +2360,7 @@ void main() {
           equals(MatchingLinkResult(forInheriting)));
 
       // Reference to an inherited member in another library via class name.
-      // TODO(jcollins-g): reference to non-imported symbols isn't implemented yet in new lookup.
-      expect(originalLookup(doAwesomeStuff, 'ExtendedBaseReexported.action'),
+      expect(bothLookup(doAwesomeStuff, 'ExtendedBaseReexported.action'),
           equals(MatchingLinkResult(action)));
     });
   });

testing/test_package/lib/fake.dart

@@ -55,6 +55,7 @@ import 'dart:collection';
 // ignore: uri_does_not_exist
 //import 'dart:json' as invalidPrefix;
 import 'package:meta/meta.dart' show Required;
+import 'package:meta/meta.dart' as aPrefix show Immutable;
 import 'csspub.dart' as css;
 import 'csspub.dart' as renamedLib2;
 import 'package:test_package/src//import_unusual.dart';
@@ -962,7 +963,9 @@ class BaseForDocComments {
 
   final bool initializeMe;
 
-  BaseForDocComments(this.initializeMe);
+  int somethingShadowy;
+
+  BaseForDocComments(this.initializeMe, [bool somethingShadowy]);
 
   BaseForDocComments.aNonDefaultConstructor(this.initializeMe);