Fix constructors and type parameters in the new lookup code (#2682)

* Handle different name styles in constructor

* dartfmt

* Fixes seem to work.

* Rebuild

* rebuild post-merge

* fix analysis error in fake

Author: jcollins-g

lib/src/comment_references/model_comment_reference.dart

@@ -15,6 +15,7 @@ abstract class ModelCommentReference {
   bool get allowDefaultConstructor;
   String get codeRef;
   bool get hasConstructorHint;
+  bool get hasCallableHint;
   List<String> get referenceBy;
   Element get staticElement;
 
@@ -32,22 +33,42 @@ abstract class ModelCommentReference {
 /// information needed for Dartdoc.  Drops link to the [CommentReference]
 /// and [ResourceProvider] after construction.
 class _ModelCommentReferenceImpl implements ModelCommentReference {
+  bool _allowDefaultConstructor;
   @override
   bool get allowDefaultConstructor {
-    if (parsed.length >= 2) {
-      return parsed[parsed.length - 2] == parsed[parsed.length - 1];
+    if (_allowDefaultConstructor == null) {
+      _allowDefaultConstructor = false;
+      var foundFirst = false;
+      String checkText;
+      // Check for two identically named identifiers at the end of the
+      // parse list, skipping over any junk or other nodes.
+      for (var node in parsed.reversed.whereType<IdentifierNode>()) {
+        if (foundFirst) {
+          if (checkText == node.text) {
+            _allowDefaultConstructor = true;
+          }
+          break;
+        } else {
+          foundFirst = true;
+          checkText = node.text;
+        }
+      }
     }
-    return false;
+    return _allowDefaultConstructor;
   }
 
   @override
   final String codeRef;
 
   @override
-  bool get hasConstructorHint =>
+  bool get hasCallableHint =>
       parsed.isNotEmpty &&
       (parsed.first is ConstructorHintStartNode ||
-          parsed.last is ConstructorHintEndNode);
+          parsed.last is CallableHintEndNode);
+
+  @override
+  bool get hasConstructorHint =>
+      parsed.isNotEmpty && parsed.first is ConstructorHintStartNode;
 
   @override
   List<String> get referenceBy => parsed

lib/src/comment_references/parser.dart

@@ -133,7 +133,7 @@ class CommentReferenceParser {
     if (suffixResult.type == _SuffixResultType.notSuffix) {
       // Invalid trailing junk; reject the reference.
       return [];
-    } else if (suffixResult.type == _SuffixResultType.parsedConstructorHint) {
+    } else if (suffixResult.type == _SuffixResultType.parsedCallableHint) {
       children.add(suffixResult.node);
     }
 
@@ -234,10 +234,10 @@ class CommentReferenceParser {
         IdentifierNode(codeRef.substring(startIndex, _index)));
   }
 
-  static const _constructorHintSuffix = '()';
+  static const _callableHintSuffix = '()';
 
   /// ```text
-  /// <suffix> ::= <constructorPostfixHint>
+  /// <suffix> ::= <callableHintSuffix>
   ///   | <trailingJunk>
   ///
   /// <trailingJunk> ::= '<'<CHARACTER>*'>'
@@ -246,18 +246,18 @@ class CommentReferenceParser {
   ///   | '?'
   ///   | '!'
   ///
-  /// <constructorPostfixHint> ::= '()'
+  /// <callableHintSuffix> ::= '()'
   /// ```
   _SuffixParseResult _parseSuffix() {
     var startIndex = _index;
     _walkPastWhitespace();
     if (_atEnd) {
       return _SuffixParseResult.missing;
     }
-    if (_tryMatchLiteral(_constructorHintSuffix)) {
+    if (_tryMatchLiteral(_callableHintSuffix)) {
       if (_atEnd) {
         return _SuffixParseResult.ok(
-            ConstructorHintEndNode(codeRef.substring(startIndex, _index)));
+            CallableHintEndNode(codeRef.substring(startIndex, _index)));
       }
       return _SuffixParseResult.notSuffix;
     }
@@ -386,10 +386,10 @@ enum _SuffixResultType {
   junk, // Found known types of junk it is OK to ignore.
   missing, // There is no suffix here.  Same as EOF as this is a suffix.
   notSuffix, // Found something, but not a valid suffix.
-  parsedConstructorHint, // Parsed a [ConstructorHintEndNode].
+  parsedCallableHint, // Parsed a [CallableHintEndNode].
 }
 
-/// The result of attempting to parse a prefix to a comment reference.
+/// The result of attempting to parse a suffix to a comment reference.
 class _SuffixParseResult {
   final _SuffixResultType type;
 
@@ -398,7 +398,7 @@ class _SuffixParseResult {
   const _SuffixParseResult._(this.type, this.node);
 
   factory _SuffixParseResult.ok(CommentReferenceNode node) =>
-      _SuffixParseResult._(_SuffixResultType.parsedConstructorHint, node);
+      _SuffixParseResult._(_SuffixResultType.parsedCallableHint, node);
 
   static const _SuffixParseResult junk =
       _SuffixParseResult._(_SuffixResultType.junk, null);
@@ -426,14 +426,14 @@ class ConstructorHintStartNode extends CommentReferenceNode {
   String toString() => 'ConstructorHintStartNode["$text"]';
 }
 
-class ConstructorHintEndNode extends CommentReferenceNode {
+class CallableHintEndNode extends CommentReferenceNode {
   @override
   final String text;
 
-  ConstructorHintEndNode(this.text);
+  CallableHintEndNode(this.text);
 
   @override
-  String toString() => 'ConstructorHintEndNode["$text"]';
+  String toString() => 'CallableHintEndNode["$text"]';
 }
 
 /// Represents an identifier.

lib/src/generator/templates.runtime_renderers.dart

@@ -2155,6 +2155,19 @@ class _Renderer_Class extends RendererBase<Class> {
                             parent: r));
                   },
                 ),
+                'referenceChildren': Property(
+                  getValue: (CT_ c) => c.referenceChildren,
+                  renderVariable: (CT_ c, Property<CT_> self,
+                          List<String> remainingNames) =>
+                      self.renderSimpleVariable(
+                          c, remainingNames, 'Map<String, CommentReferable>'),
+                  isNullValue: (CT_ c) => c.referenceChildren == null,
+                  renderValue: (CT_ c, RendererBase<CT_> r,
+                      List<MustachioNode> ast, StringSink sink) {
+                    renderSimple(c.referenceChildren, ast, r.template, sink,
+                        parent: r, getters: _invisibleGetters['Map']);
+                  },
+                ),
                 'referenceParents': Property(
                   getValue: (CT_ c) => c.referenceParents,
                   renderVariable: (CT_ c, Property<CT_> self,

lib/src/markdown_processor.dart

@@ -203,6 +203,13 @@ class MatchingLinkResult {
     if (modelElement is Parameter && other.modelElement == null) {
       return true;
     }
+    // Same with TypeParameter.
+    if (other.modelElement is TypeParameter && modelElement == null) {
+      return true;
+    }
+    if (modelElement is TypeParameter && other.modelElement == null) {
+      return true;
+    }
     return false;
   }
 
@@ -286,7 +293,12 @@ bool _rejectDefaultConstructors(CommentReferable referable) {
   return true;
 }
 
-/// Return false unless the passed [referable] is a [Constructor].
+/// Return false unless the passed [referable] represents a callable object.
+/// Allows constructors but does not require them.
+bool _requireCallable(CommentReferable referable) =>
+    referable is ModelElement && referable.isCallable;
+
+/// Return false unless the passed [referable] represents a constructor.
 bool _requireConstructor(CommentReferable referable) =>
     referable is Constructor;
 
@@ -295,17 +307,29 @@ MatchingLinkResult _getMatchingLinkElementCommentReferable(
     String codeRef, Warnable warnable) {
   var commentReference =
       warnable.commentRefs[codeRef] ?? ModelCommentReference.synthetic(codeRef);
-  var filter = commentReference.hasConstructorHint
-      ? _requireConstructor
-      : _rejectDefaultConstructors;
-
-  /// Neither reject, nor require, a default constructor in the event
-  /// the comment reference structure implies one.  (We can not require it
-  /// in case a library name is the same as a member class name and the class
-  /// is the intended lookup).
+
+  bool Function(CommentReferable) filter;
+
   if (commentReference.allowDefaultConstructor) {
-    filter = null;
+    // Neither reject, nor require, a default constructor in the event
+    // the comment reference structure implies one.  (We can not 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 : null;
+  } else if (commentReference.hasConstructorHint &&
+      commentReference.hasCallableHint) {
+    // This takes precedence over the callable hint if both are present --
+    // pick a constructor and only constructor if we see `new`.
+    filter = _requireConstructor;
+  } else if (commentReference.hasCallableHint) {
+    // Trailing parens indicate we are looking for a callable.
+    filter = _requireCallable;
+  } else {
+    // Without hints, reject default constructors to force resolution to the
+    // class.
+    filter = _rejectDefaultConstructors;
   }
+
   var lookupResult =
       warnable.referenceBy(commentReference.referenceBy, filter: filter);
 

lib/src/model/class.dart

@@ -605,6 +605,22 @@ class Class extends Container
   @override
   Iterable<Field> get constantFields => allFields.where((f) => f.isConst);
 
+  Map<String, CommentReferable> _referenceChildren;
+  @override
+  Map<String, CommentReferable> get referenceChildren {
+    if (_referenceChildren == null) {
+      _referenceChildren = super.referenceChildren;
+      for (var constructor in constructors) {
+        // For default constructors this is a no-op, but other constructors
+        // sometimes have a prefix attached to them in their "real name".
+        // TODO(jcollins-g): consider disallowing that, and only using
+        // the element name as here?
+        _referenceChildren[constructor.element.name] = constructor;
+      }
+    }
+    return _referenceChildren;
+  }
+
   @override
   Iterable<CommentReferable> get referenceParents => <CommentReferable>[
         ...super.referenceParents,

lib/src/model/constructor.dart

@@ -127,8 +127,16 @@ class Constructor extends ModelElement
   Map<String, CommentReferable> get referenceChildren {
     if (_referenceChildren == null) {
       _referenceChildren = {};
-      _referenceChildren
-          .addEntries(allParameters.map((p) => MapEntry(p.name, p)));
+      for (var param in allParameters) {
+        var paramElement = param.element;
+        if (paramElement is FieldFormalParameterElement) {
+          var fieldFormal =
+              ModelElement.fromElement(paramElement.field, packageGraph);
+          _referenceChildren[paramElement.name] = fieldFormal;
+        } else {
+          _referenceChildren[param.name] = param;
+        }
+      }
       _referenceChildren
           .addEntries(typeParameters.map((p) => MapEntry(p.name, p)));
     }

lib/src/model/container.dart

@@ -262,14 +262,29 @@ abstract class Container extends ModelElement with TypeParameters {
     if (_referenceChildren == null) {
       _referenceChildren = {};
       for (var modelElement in allModelElements) {
+        // Never directly look up accessors.
         if (modelElement is Accessor) continue;
+        if (modelElement is Constructor) {
+          // Populate default constructor names so they make sense for the
+          // new lookup code.
+          var constructorName = modelElement.element.name;
+          if (constructorName == '') {
+            constructorName = name;
+          }
+          _referenceChildren[constructorName] = modelElement;
+          continue;
+        }
         if (modelElement is Operator) {
           // TODO(jcollins-g): once todo in [Operator.name] is fixed, remove
           // this special case.
           _referenceChildren[modelElement.element.name] = modelElement;
         } else {
           _referenceChildren[modelElement.name] = modelElement;
         }
+      }
+      // Process unscoped parameters last to make sure they don't override
+      // other options.
+      for (var modelElement in allModelElements) {
         // Don't complain about references to parameter names, but prefer
         // referring to anything else.
         // TODO(jcollins-g): Figure out something good to do in the ecosystem

test/comment_referable/parser_test.dart

@@ -11,7 +11,7 @@ void main() {
     var result = CommentReferenceParser(codeRef).parse();
     var hasHint = result.isNotEmpty &&
         (result.first is ConstructorHintStartNode ||
-            result.last is ConstructorHintEndNode);
+            result.last is CallableHintEndNode);
     var stringParts = result.whereType<IdentifierNode>().map((i) => i.text);
     expect(stringParts, equals(parts));
     expect(hasHint, equals(constructorHint));

test/end2end/model_test.dart

@@ -2136,12 +2136,13 @@ void main() {
         nameWithTwoUnderscores,
         nameWithSingleUnderscore,
         theOnlyThingInTheLibrary;
+    Constructor aNonDefaultConstructor, defaultConstructor;
     Class Apple, BaseClass, baseForDocComments, ExtraSpecialList, string;
     Method doAwesomeStuff, anotherMethod;
     // ignore: unused_local_variable
     Operator bracketOperator, bracketOperatorOtherClass;
     Parameter doAwesomeStuffParam;
-    Field forInheriting, action;
+    Field forInheriting, action, initializeMe;
 
     setUpAll(() async {
       nameWithTwoUnderscores = fakeLibrary.constants
@@ -2154,6 +2155,12 @@ void main() {
           .firstWhere((c) => c.name == 'String');
       baseForDocComments =
           fakeLibrary.classes.firstWhere((c) => c.name == 'BaseForDocComments');
+      aNonDefaultConstructor = baseForDocComments.constructors.firstWhere(
+          (c) => c.name == 'BaseForDocComments.aNonDefaultConstructor');
+      defaultConstructor = baseForDocComments.constructors
+          .firstWhere((c) => c.name == 'BaseForDocComments');
+      initializeMe = baseForDocComments.allFields
+          .firstWhere((f) => f.name == 'initializeMe');
       doAwesomeStuff = baseForDocComments.instanceMethods
           .firstWhere((m) => m.name == 'doAwesomeStuff');
       anotherMethod = baseForDocComments.instanceMethods
@@ -2228,7 +2235,33 @@ void main() {
       return newLookupResult;
     }
 
+    test('Verify basic linking inside a constructor', () {
+      // Field formal parameters worked sometimes by accident in the old code,
+      // but should work reliably now.
+      expect(newLookup(aNonDefaultConstructor, 'initializeMe'),
+          equals(MatchingLinkResult(initializeMe)));
+      expect(newLookup(aNonDefaultConstructor, 'aNonDefaultConstructor'),
+          equals(MatchingLinkResult(aNonDefaultConstructor)));
+      expect(
+          bothLookup(aNonDefaultConstructor,
+              'BaseForDocComments.aNonDefaultConstructor'),
+          equals(MatchingLinkResult(aNonDefaultConstructor)));
+    });
+
     test('Verify basic linking inside class', () {
+      expect(
+          bothLookup(
+              baseForDocComments, 'BaseForDocComments.BaseForDocComments'),
+          equals(MatchingLinkResult(defaultConstructor)));
+
+      expect(bothLookup(doAwesomeStuff, 'aNonDefaultConstructor'),
+          equals(MatchingLinkResult(aNonDefaultConstructor)));
+
+      expect(
+          bothLookup(
+              doAwesomeStuff, 'BaseForDocComments.aNonDefaultConstructor'),
+          equals(MatchingLinkResult(aNonDefaultConstructor)));
+
       expect(bothLookup(doAwesomeStuff, 'this'),
           equals(MatchingLinkResult(baseForDocComments)));
 
@@ -2287,7 +2320,6 @@ void main() {
           equals(MatchingLinkResult(BaseClass)));
 
       // A bracket operator within this class.
-      // TODO(jcollins-g): operator lookups not yet implemented with the new lookup code.
       expect(bothLookup(doAwesomeStuff, 'operator []'),
           equals(MatchingLinkResult(bracketOperator)));