add library and package disambiguation in new lookup code (#2700)

* Beginnings of grandparent override scoping

* intermediate

* implement basic overrides, constructors still broken

* intermediate

* this seems to work much better

* dartfmt

* This now seems to work with tests

* rebuild && dartfmt

* fix up tests a bit

* add comment

* fix package / library cases

* move test to correct group

* remove assert

* starting to work on test problems

* Fix some errors in the tests

Author: jcollins-g

lib/src/comment_references/parser.dart

@@ -158,6 +158,7 @@ class CommentReferenceParser {
     if (_atEnd) {
       return _PrefixParseResult.endOfFile;
     }
+    _walkPastWhitespace();
     if (_tryMatchLiteral(_constructorHintPrefix,
         requireTrailingNonidentifier: true)) {
       return _PrefixParseResult.ok(

lib/src/generator/templates.runtime_renderers.dart

@@ -2581,6 +2581,25 @@ class _Renderer_CommentReferable extends RendererBase<CommentReferable> {
                         parent: r, getters: _invisibleGetters['Element']);
                   },
                 ),
+                'href': Property(
+                  getValue: (CT_ c) => c.href,
+                  renderVariable:
+                      (CT_ c, Property<CT_> self, List<String> remainingNames) {
+                    if (remainingNames.isEmpty) {
+                      return self.getValue(c).toString();
+                    }
+                    var name = remainingNames.first;
+                    var nextProperty =
+                        _Renderer_String.propertyMap().getValue(name);
+                    return nextProperty.renderVariable(self.getValue(c),
+                        nextProperty, [...remainingNames.skip(1)]);
+                  },
+                  isNullValue: (CT_ c) => c.href == null,
+                  renderValue: (CT_ c, RendererBase<CT_> r,
+                      List<MustachioNode> ast, StringSink sink) {
+                    _render_String(c.href, ast, r.template, sink, parent: r);
+                  },
+                ),
                 'library': Property(
                   getValue: (CT_ c) => c.library,
                   renderVariable:
@@ -14592,6 +14611,7 @@ const _invisibleGetters = {
   },
   'CommentReferable': {
     'scope',
+    'href',
     'referenceChildren',
     'referenceParents',
     'referenceGrandparentOverrides',

lib/src/markdown_processor.dart

@@ -174,25 +174,24 @@ final List<md.BlockSyntax> _markdownBlockSyntaxes = [
 final RegExp _hideSchemes = RegExp('^(http|https)://');
 
 class MatchingLinkResult {
-  final ModelElement modelElement;
+  final CommentReferable commentReferable;
   final bool warn;
 
-  MatchingLinkResult(this.modelElement, {this.warn = true});
+  MatchingLinkResult(this.commentReferable, {this.warn = true});
 
   @override
   bool operator ==(Object other) {
     return other is MatchingLinkResult &&
-        modelElement == other.modelElement &&
+        commentReferable == other.commentReferable &&
         warn == other.warn;
   }
 
   @override
-  int get hashCode => hash2(modelElement, warn);
+  int get hashCode => hash2(commentReferable, warn);
 
   bool isEquivalentTo(MatchingLinkResult other) {
-    if (this == other) return true;
-    var compareThis = modelElement;
-    var compareOther = other.modelElement;
+    var compareThis = commentReferable;
+    var compareOther = other.commentReferable;
 
     if (compareThis is Accessor) {
       compareThis = (compareThis as Accessor).enclosingCombo;
@@ -202,8 +201,16 @@ class MatchingLinkResult {
       compareOther = (compareOther as Accessor).enclosingCombo;
     }
 
-    if (compareThis?.canonicalModelElement ==
-        compareOther?.canonicalModelElement) return true;
+    if (compareThis is ModelElement &&
+        compareThis.canonicalModelElement != null) {
+      compareThis = (compareThis as ModelElement).canonicalModelElement;
+    }
+    if (compareOther is ModelElement &&
+        compareOther.canonicalModelElement != null) {
+      compareOther = (compareOther as ModelElement).canonicalModelElement;
+    }
+    if (compareThis == compareOther) 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
@@ -221,12 +228,13 @@ class MatchingLinkResult {
     if (compareThis is TypeParameter && compareOther == null) {
       return true;
     }
+
     return false;
   }
 
   @override
   String toString() {
-    return 'element: [${modelElement is Constructor ? 'new ' : ''}${modelElement?.fullyQualifiedName}] warn: $warn';
+    return 'element: [${commentReferable is Constructor ? 'new ' : ''}${commentReferable?.fullyQualifiedName}] warn: $warn';
   }
 }
 
@@ -354,12 +362,6 @@ MatchingLinkResult _getMatchingLinkElementCommentReferable(
   var lookupResult =
       warnable.referenceBy(commentReference.referenceBy, filter: filter);
 
-  // TODO(jcollins-g): Referring to packages or other non-[ModelElement]s
-  // might be needed here.  Determine if that's the case.
-  if (!(lookupResult is ModelElement)) {
-    lookupResult = null;
-  }
-
   // TODO(jcollins-g): Consider prioritizing analyzer resolution before custom.
   return MatchingLinkResult(lookupResult);
 }
@@ -1001,11 +1003,11 @@ const _referenceLookupWarnings = {
 md.Node _makeLinkNode(String codeRef, Warnable warnable) {
   var result = getMatchingLinkElement(warnable, codeRef);
   var textContent = htmlEscape.convert(codeRef);
-  var linkedElement = result.modelElement;
+  var linkedElement = result.commentReferable;
   if (linkedElement != null) {
     if (linkedElement.href != null) {
       var anchor = md.Element.text('a', textContent);
-      if (linkedElement.isDeprecated) {
+      if (linkedElement is ModelElement && linkedElement.isDeprecated) {
         anchor.attributes['class'] = 'deprecated';
       }
       anchor.attributes['href'] = linkedElement.href;
@@ -1041,11 +1043,11 @@ MatchingLinkResult getMatchingLinkElement(Warnable warnable, String codeRef,
   if (doComparison) {
     resultNew = _getMatchingLinkElementCommentReferable(codeRef, warnable);
     resultOld = _getMatchingLinkElementLegacy(codeRef, warnable);
-    if (resultNew.modelElement != null) {
+    if (resultNew.commentReferable != null) {
       markdownStats.resolvedNewLookupReferences++;
     }
     result = experimentalReferenceLookup ? resultNew : resultOld;
-    if (resultOld.modelElement != null) {
+    if (resultOld.commentReferable != null) {
       markdownStats.resolvedOldLookupReferences++;
     }
   } else {
@@ -1059,12 +1061,13 @@ MatchingLinkResult getMatchingLinkElement(Warnable warnable, String codeRef,
     if (resultOld.isEquivalentTo(resultNew)) {
       markdownStats.resolvedEquivalentlyReferences++;
     } else {
-      if (resultNew.modelElement == null && resultOld.modelElement != null) {
+      if (resultNew.commentReferable == null &&
+          resultOld.commentReferable != null) {
         warnable.warn(PackageWarning.referenceLookupMissingWithNew,
             message: '[$codeRef] => ' + resultOld.toString(),
             referredFrom: warnable.documentationFrom);
-      } else if (resultNew.modelElement != null &&
-          resultOld.modelElement == null) {
+      } else if (resultNew.commentReferable != null &&
+          resultOld.commentReferable == null) {
         warnable.warn(PackageWarning.referenceLookupFoundWithNew,
             message: '[$codeRef] => ' + resultNew.toString(),
             referredFrom: warnable.documentationFrom);
@@ -1077,7 +1080,7 @@ MatchingLinkResult getMatchingLinkElement(Warnable warnable, String codeRef,
     }
   }
   markdownStats.totalReferences++;
-  if (result.modelElement != null) markdownStats.resolvedReferences++;
+  if (result.commentReferable != null) markdownStats.resolvedReferences++;
   return result;
 }
 
@@ -1095,16 +1098,21 @@ final RegExp allAfterLastNewline = RegExp(r'\n.*$', multiLine: true);
 // https://github.com/dart-lang/dartdoc/issues/1250#issuecomment-269257942
 void showWarningsForGenericsOutsideSquareBracketsBlocks(
     String text, Warnable element) {
-  for (var position in findFreeHangingGenericsPositions(text)) {
-    var priorContext =
-        '${text.substring(max(position - maxPriorContext, 0), position)}';
-    var postContext =
-        '${text.substring(position, min(position + maxPostContext, text.length))}';
-    priorContext = priorContext.replaceAll(allBeforeFirstNewline, '');
-    postContext = postContext.replaceAll(allAfterLastNewline, '');
-    var errorMessage = '$priorContext$postContext';
-    // TODO(jcollins-g):  allow for more specific error location inside comments
-    element.warn(PackageWarning.typeAsHtml, message: errorMessage);
+  // Skip this if not warned for performance and for dart-lang/sdk#46419.
+  if (element.config.packageWarningOptions
+          .warningModes[PackageWarning.typeAsHtml] !=
+      PackageWarningMode.ignore) {
+    for (var position in findFreeHangingGenericsPositions(text)) {
+      var priorContext =
+          '${text.substring(max(position - maxPriorContext, 0), position)}';
+      var postContext =
+          '${text.substring(position, min(position + maxPostContext, text.length))}';
+      priorContext = priorContext.replaceAll(allBeforeFirstNewline, '');
+      postContext = postContext.replaceAll(allAfterLastNewline, '');
+      var errorMessage = '$priorContext$postContext';
+      // TODO(jcollins-g):  allow for more specific error location inside comments
+      element.warn(PackageWarning.typeAsHtml, message: errorMessage);
+    }
   }
 }
 

lib/src/model/comment_referable.dart

@@ -42,6 +42,8 @@ mixin CommentReferable implements Nameable {
   /// lookups via [referenceChildren].  Can be cached.
   Scope get scope => null;
 
+  String get href => null;
+
   /// Look up a comment reference by its component parts.  If [tryParents] is
   /// true, try looking up the same reference in any parents of [this].
   /// Will skip over results that do not pass a given [filter] and keep

lib/src/model/package_graph.dart

@@ -1031,7 +1031,8 @@ class PackageGraph with CommentReferable, Nameable {
     if (_referenceChildren == null) {
       _referenceChildren = {};
       // Packages are the top priority.
-      _referenceChildren.addEntries(packages.map((p) => MapEntry(p.name, p)));
+      _referenceChildren
+          .addEntries(packages.map((p) => MapEntry('package:${p.name}', p)));
 
       // Libraries are next.
       // TODO(jcollins-g): Warn about directly referencing libraries out of

test/comment_referable/parser_test.dart

@@ -47,6 +47,7 @@ void main() {
       expectParseEquivalent('this.is.valid<>', ['this', 'is', 'valid']);
       expectParseEquivalent('this.is.valid<stuff>', ['this', 'is', 'valid']);
       expectParseEquivalent('this.is.valid(things)', ['this', 'is', 'valid']);
+      expectParseEquivalent('\nthis.is.valid', ['this', 'is', 'valid']);
     });
 
     test('Check that cases dependent on prefix resolution not firing parse',

test/end2end/model_test.dart

@@ -2129,10 +2129,10 @@ void main() {
     /// as the original lookup code returns canonicalized results and the
     /// new lookup code is only guaranteed to return equivalent results.
     MatchingLinkResult definingLinkResult(MatchingLinkResult originalResult) {
-      if (originalResult.modelElement != null) {
+      if (originalResult.commentReferable?.element != null) {
         return MatchingLinkResult(
-            ModelElement.fromElement(originalResult.modelElement.element,
-                originalResult.modelElement.packageGraph),
+            ModelElement.fromElement(originalResult.commentReferable.element,
+                originalResult.commentReferable.packageGraph),
             warn: originalResult.warn);
       }
       return originalResult;
@@ -2228,7 +2228,10 @@ void main() {
       });
 
       test('Linking for inherited field from reexport context', () {
-        expect(bothLookup(aField, 'anotherNotReexportedVariable'),
+        // While this can work in the old code at times, it is using the
+        // analyzer and the new test harness can't leverage the analyzer
+        // when testing the old lookup code.
+        expect(newLookup(aField, 'anotherNotReexportedVariable'),
             equals(MatchingLinkResult(anotherNotReexportedVariable)));
       });
 
@@ -2250,6 +2253,8 @@ void main() {
     });
 
     group('Ordinary namespace cases', () {
+      Package DartPackage;
+      Library Dart;
       ModelFunction doesStuff, function1, topLevelFunction;
       TopLevelVariable incorrectDocReference,
           incorrectDocReferenceFromEx,
@@ -2276,6 +2281,9 @@ void main() {
           aConstructorShadowedField;
 
       setUpAll(() async {
+        Dart = packageGraph.allLibraries.values
+            .firstWhere((l) => l.name == 'Dart');
+        DartPackage = packageGraph.packages.firstWhere((p) => p.name == 'Dart');
         nameWithTwoUnderscores = fakeLibrary.constants
             .firstWhere((v) => v.name == 'NAME_WITH_TWO_UNDERSCORES');
         nameWithSingleUnderscore = fakeLibrary.constants
@@ -2348,6 +2356,14 @@ void main() {
             .firstWhere((f) => f.name == 'aConstructorShadowed');
       });
 
+      test('Referring to libraries and packages with the same name is fine',
+          () {
+        expect(bothLookup(Apple, 'Dart'), equals(MatchingLinkResult(Dart)));
+        // New feature: allow disambiguation if you really want to specify a package.
+        expect(newLookup(Apple, 'package:Dart'),
+            equals(MatchingLinkResult(DartPackage)));
+      });
+
       test('Verify basic linking inside a constructor', () {
         // Field formal parameters worked sometimes by accident in the old code,
         // but should work reliably now.
@@ -2378,7 +2394,10 @@ void main() {
 
       test('Deprecated lookup styles still function', () {
         // dart-lang/dartdoc#2683
-        expect(bothLookup(baseForDocComments, 'aPrefix.UseResult'),
+        // We can't check the old code this way as this is one of the few
+        // cases in the old code where it relies on the analyzer's resolution of
+        // the doc comment -- the test harness can't do that for the old code.
+        expect(newLookup(baseForDocComments, 'aPrefix.UseResult'),
             equals(MatchingLinkResult(metaUseResult)));
       });
 

test/src/utils.dart

@@ -19,7 +19,7 @@ import 'package:pub_semver/pub_semver.dart';
 /// The number of public libraries in testing/test_package, minus 2 for
 /// the excluded libraries listed in the initializers for _testPackageGraphMemo
 /// and minus 1 for the <nodoc> tag in the 'excluded' library.
-const int kTestPackagePublicLibraries = 23;
+const int kTestPackagePublicLibraries = 24;
 
 final _resourceProvider = pubPackageMetaProvider.resourceProvider;
 final _pathContext = _resourceProvider.pathContext;

testing/test_package/lib/dart.dart

@@ -0,0 +1,2 @@
+/// A bold name for a library to test comment reference resolution.
+library Dart;