From 5555f2ba2309b4fd6a11112498abd5122ccdfd90 Mon Sep 17 00:00:00 2001
From: Sam Rawlins <srawlins@google.com>
Date: Fri, 10 May 2024 09:05:52 -0700
Subject: [PATCH] Fix bug around ref resolution of type mentioned in type
 arguments

---
 lib/src/markdown_processor.dart               | 33 +++-------
 lib/src/model/comment_referable.dart          | 45 ++++++--------
 lib/src/model/method.dart                     | 18 +++++-
 .../comment_referable_test.dart               | 24 ++------
 test/enum_test.dart                           | 60 +++++++++++++++++++
 5 files changed, 108 insertions(+), 72 deletions(-)

diff --git a/lib/src/markdown_processor.dart b/lib/src/markdown_processor.dart
index ee574c7d5a..e6405aafeb 100644
--- a/lib/src/markdown_processor.dart
+++ b/lib/src/markdown_processor.dart
@@ -165,30 +165,15 @@ MatchingLinkResult _getMatchingLinkElement(
     String referenceText, Warnable element) {
   var commentReference = ModelCommentReference(referenceText);
 
-  // A filter to be used by [CommentReferable.referenceBy].
-  bool Function(CommentReferable?) filter;
-
-  // An "allow tree" filter to be used by [CommentReferable.referenceBy].
-  bool Function(CommentReferable?) allowTree;
-
-  if (commentReference.hasCallableHint) {
-    allowTree = (_) => true;
-    // Trailing parens indicate we are looking for a callable.
-    filter = _requireCallable;
-  } else {
-    allowTree = (_) => true;
-    // Neither reject, nor require, an unnamed constructor in the event the
-    // comment reference structure implies one. (We cannot require it in case a
-    // library name is the same as a member class name and the class is the
-    // intended lookup).
-    filter = commentReference.hasCallableHint
-        ? _requireCallable
-        // Without hints, reject unnamed constructors and their parameters to
-        // force resolution to the class.
-        : filter = _rejectUnnamedAndShadowingConstructors;
-  }
-  var lookupResult = element.referenceBy(commentReference.referenceBy,
-      allowTree: allowTree, filter: filter);
+  var filter = commentReference.hasCallableHint
+      // Trailing parens indicate we are looking for a callable.
+      ? _requireCallable
+      // Without hints, reject unnamed constructors and their parameters to
+      // force resolution to the class.
+      : _rejectUnnamedAndShadowingConstructors;
+
+  var lookupResult =
+      element.referenceBy(commentReference.referenceBy, filter: filter);
 
   // TODO(jcollins-g): Consider prioritizing analyzer resolution before custom.
   return MatchingLinkResult(lookupResult);
diff --git a/lib/src/model/comment_referable.dart b/lib/src/model/comment_referable.dart
index 1a5c276e07..ba8fac49d5 100644
--- a/lib/src/model/comment_referable.dart
+++ b/lib/src/model/comment_referable.dart
@@ -31,7 +31,6 @@ class _ReferenceChildrenLookup {
 
 /// Support comment reference lookups on a Nameable object.
 mixin CommentReferable implements Nameable {
-  //, ModelBuilderInterface {
   /// For any [CommentReferable] where an analyzer [Scope] exists (or can
   /// be constructed), implement this.  This will take priority over
   /// lookups via [referenceChildren].  Can be cached.
@@ -39,17 +38,15 @@ mixin CommentReferable implements Nameable {
 
   String? get href => null;
 
-  /// Look up a comment reference by its component parts.
+  /// Looks 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 searching.  Will skip over entire subtrees whose parent node does not
-  /// pass [allowTree].
+  /// keep searching.
   @nonVirtual
   CommentReferable? referenceBy(
     List<String> reference, {
     required bool Function(CommentReferable?) filter,
-    required bool Function(CommentReferable?) allowTree,
     bool tryParents = true,
     Iterable<CommentReferable>? parentOverrides,
   }) {
@@ -57,11 +54,9 @@ mixin CommentReferable implements Nameable {
     if (reference.isEmpty) {
       return tryParents ? null : this;
     }
-
     for (var referenceLookup in _childLookups(reference)) {
       if (scope != null) {
-        var result = _lookupViaScope(referenceLookup,
-            filter: filter, allowTree: allowTree);
+        var result = _lookupViaScope(referenceLookup, filter: filter);
         if (result != null) {
           return result;
         }
@@ -69,8 +64,11 @@ mixin CommentReferable implements Nameable {
       final referenceChildren = this.referenceChildren;
       final childrenResult = referenceChildren[referenceLookup.lookup];
       if (childrenResult != null) {
-        var result = _recurseChildrenAndFilter(referenceLookup, childrenResult,
-            allowTree: allowTree, filter: filter);
+        var result = _recurseChildrenAndFilter(
+          referenceLookup,
+          childrenResult,
+          filter: filter,
+        );
         if (result != null) {
           return result;
         }
@@ -79,11 +77,12 @@ mixin CommentReferable implements Nameable {
     // If we can't find it in children, try searching parents if allowed.
     if (tryParents) {
       for (var parent in parentOverrides) {
-        var result = parent.referenceBy(reference,
-            tryParents: true,
-            parentOverrides: referenceGrandparentOverrides,
-            allowTree: allowTree,
-            filter: filter);
+        var result = parent.referenceBy(
+          reference,
+          tryParents: true,
+          parentOverrides: referenceGrandparentOverrides,
+          filter: filter,
+        );
         if (result != null) return result;
       }
     }
@@ -99,7 +98,6 @@ mixin CommentReferable implements Nameable {
   CommentReferable? _lookupViaScope(
     _ReferenceChildrenLookup referenceLookup, {
     required bool Function(CommentReferable?) filter,
-    required bool Function(CommentReferable?) allowTree,
   }) {
     final resultElement = scope!.lookupPreferGetter(referenceLookup.lookup);
     if (resultElement == null) return null;
@@ -122,9 +120,7 @@ mixin CommentReferable implements Nameable {
       );
       return null;
     }
-    if (!allowTree(result)) return null;
-    return _recurseChildrenAndFilter(referenceLookup, result,
-        allowTree: allowTree, filter: filter);
+    return _recurseChildrenAndFilter(referenceLookup, result, filter: filter);
   }
 
   /// Given a [result] found in an implementation of [_lookupViaScope] or
@@ -134,19 +130,14 @@ mixin CommentReferable implements Nameable {
     _ReferenceChildrenLookup referenceLookup,
     CommentReferable result, {
     required bool Function(CommentReferable?) filter,
-    required bool Function(CommentReferable?) allowTree,
   }) {
     CommentReferable? returnValue = result;
     if (referenceLookup.remaining.isNotEmpty) {
-      if (allowTree(result)) {
-        returnValue = result.referenceBy(referenceLookup.remaining,
-            tryParents: false, allowTree: allowTree, filter: filter);
-      } else {
-        returnValue = null;
-      }
+      returnValue = result.referenceBy(referenceLookup.remaining,
+          tryParents: false, filter: filter);
     } else if (!filter(result)) {
       returnValue = result.referenceBy([referenceLookup.lookup],
-          tryParents: false, allowTree: allowTree, filter: filter);
+          tryParents: false, filter: filter);
     }
     if (!filter(returnValue)) {
       returnValue = null;
diff --git a/lib/src/model/method.dart b/lib/src/model/method.dart
index fd1c530f13..1b5ce53119 100644
--- a/lib/src/model/method.dart
+++ b/lib/src/model/method.dart
@@ -138,10 +138,24 @@ class Method extends ModelElement
       return from.referenceChildren;
     }
     return _referenceChildren ??= <String, CommentReferable>{
-      ...modelType.returnType.typeArguments.explicitOnCollisionWith(this),
-      ...modelType.typeArguments.explicitOnCollisionWith(this),
+      // If we want to include all types referred to in the signature of this
+      // method, this is woefully incomplete. Notice we don't currently include
+      // the element of the returned type itself, nor nested type arguments,
+      // nor other nested types e.g. in the case of function types or record
+      // types. But this is all being replaced with analyzer's resolution soon.
+      ...modelType.returnType.typeArguments.modelElements
+          .explicitOnCollisionWith(this),
       ...parameters.explicitOnCollisionWith(this),
       ...typeParameters.explicitOnCollisionWith(this),
     };
   }
 }
+
+extension on Iterable<ElementType> {
+  /// The [ModelElement] associated with each type, for each type that is a
+  /// [DefinedElementType].
+  List<ModelElement> get modelElements => [
+        for (var type in this)
+          if (type is DefinedElementType) type.modelElement,
+      ];
+}
diff --git a/test/comment_referable/comment_referable_test.dart b/test/comment_referable/comment_referable_test.dart
index e10bbfa7f8..3ebf1efae0 100644
--- a/test/comment_referable/comment_referable_test.dart
+++ b/test/comment_referable/comment_referable_test.dart
@@ -23,11 +23,11 @@ abstract class Base with Nameable, CommentReferable {
   /// Returns the added (or already existing) [Base].
   Base add(String newName);
 
-  CommentReferable? lookup<T extends CommentReferable?>(String value,
-      {bool Function(CommentReferable?)? allowTree,
-      bool Function(CommentReferable?)? filter}) {
-    return referenceBy(value.split(_separator),
-        allowTree: allowTree ?? (_) => true, filter: filter ?? (_) => true);
+  CommentReferable? lookup<T extends CommentReferable?>(
+    String value, {
+    bool Function(CommentReferable?)? filter,
+  }) {
+    return referenceBy(value.split(_separator), filter: filter ?? (_) => true);
   }
 
   @override
@@ -165,20 +165,6 @@ void main() {
           isA<GenericChild>());
     });
 
-    test('Check that allowTree works', () {
-      referable.add('lib4');
-      var lib4lib4 = referable.add('lib4.lib4');
-      var tooDeepSub1 = referable.add('lib4.lib4.sub1');
-      var sub1 = referable.add('lib4.sub1');
-      var sub2 = referable.add('lib4.sub2');
-      expect(sub2.lookup('lib4.lib4'), equals(lib4lib4));
-      expect(sub2.lookup('lib4.sub1'), equals(tooDeepSub1));
-      expect(
-          sub2.lookup('lib4.sub1',
-              allowTree: (r) => r is Base && (r.parent is Top)),
-          equals(sub1));
-    });
-
     test('Check that grandparent overrides work', () {
       referable.add('lib4');
       var i1 = referable.add('lib4.intermediate1');
diff --git a/test/enum_test.dart b/test/enum_test.dart
index 4347cb9ca5..71ca8af6ed 100644
--- a/test/enum_test.dart
+++ b/test/enum_test.dart
@@ -641,4 +641,64 @@ class C {}
       '<a href="$linkPrefix/E/values-constant.html">E.values</a>.</p>',
     );
   }
+
+  void test_canBeReferenced_whenFoundAsReturnType() async {
+    var library = await bootPackageWithLibrary('''
+enum E {
+  one, two
+}
+
+class C {
+  /// [E] can be referenced.
+  E m() => E.one;
+}
+''');
+    var m = library.classes.named('C').instanceMethods.named('m');
+
+    expect(m.hasDocumentationComment, true);
+    expect(
+      m.documentationAsHtml,
+      '<p><a href="$linkPrefix/E.html">E</a> can be referenced.</p>',
+    );
+  }
+
+  void test_canBeReferenced_whenFoundAsReturnType_typeArgument() async {
+    var library = await bootPackageWithLibrary('''
+enum E {
+  one, two
+}
+
+class C {
+  /// [E] can be referenced.
+  Future<E> m() async => E.one;
+}
+''');
+    var m = library.classes.named('C').instanceMethods.named('m');
+
+    expect(m.hasDocumentationComment, true);
+    expect(
+      m.documentationAsHtml,
+      '<p><a href="$linkPrefix/E.html">E</a> can be referenced.</p>',
+    );
+  }
+
+  void test_canBeReferenced_whenFoundAsParameterType() async {
+    var library = await bootPackageWithLibrary('''
+enum E {
+  one, two
+}
+
+class C {
+  /// [E] can be referenced.
+  void m(E p) {}
+}
+''');
+    var m = library.classes.named('C').instanceMethods.named('m');
+
+    expect(m.hasDocumentationComment, true);
+    expect(
+      m.documentationAsHtml,
+      '<p><a href="$linkPrefix/E.html">E</a> can be referenced.</p>',
+    );
+  }
 }