Merge pull request #2772 from jcollins-g/constructor-tearoffs+typeparams

Allow embedded type parameters in references

Author: jcollins-g

lib/src/comment_references/parser.dart

@@ -93,7 +93,7 @@ class CommentReferenceParser {
   /// ```text
   ///   <rawCommentReference> ::= <prefix>?<commentReference><suffix>?
   ///
-  ///   <commentReference> ::= (<packageName> '.')? (<libraryName> '.')? <dartdocIdentifier> ('.' <identifier>)*
+  ///   <commentReference> ::= (<packageName> '.')? (<libraryName> '.')? <dartdocIdentifier> <typeArguments> ('.' <identifier> <typeArguments>)*
   /// ```
   List<CommentReferenceNode> _parseRawCommentReference() {
     var children = <CommentReferenceNode>[];
@@ -121,6 +121,17 @@ class CommentReferenceParser {
       } else if (identifierResult.type ==
           _IdentifierResultType.parsedIdentifier) {
         children.add(identifierResult.node);
+        var typeVariablesResult = _parseTypeVariables();
+        if (typeVariablesResult.type == _TypeVariablesResultType.endOfFile) {
+          break;
+        } else if (typeVariablesResult.type ==
+            _TypeVariablesResultType.notTypeVariables) {
+          // Do nothing, _index has not moved.
+          ;
+        } else if (typeVariablesResult.type ==
+            _TypeVariablesResultType.parsedTypeVariables) {
+          children.add(typeVariablesResult.node);
+        }
       }
       if (_atEnd || _thisChar != $dot) {
         break;
@@ -236,6 +247,22 @@ class CommentReferenceParser {
         IdentifierNode(codeRef.substring(startIndex, _index)));
   }
 
+  /// Parse a list of type variables (arguments or parameters).
+  ///
+  /// Dartdoc isolates these where present and potentially valid, but we don't
+  /// break them down.
+  _TypeVariablesParseResult _parseTypeVariables() {
+    if (_atEnd) {
+      return _TypeVariablesParseResult.endOfFile;
+    }
+    var startIndex = _index;
+    if (_matchBraces($lt, $gt)) {
+      return _TypeVariablesParseResult.ok(
+          TypeVariablesNode(codeRef.substring(startIndex + 1, _index - 1)));
+    }
+    return _TypeVariablesParseResult.notIdentifier;
+  }
+
   static const _callableHintSuffix = '()';
 
   /// ```text
@@ -267,7 +294,7 @@ class CommentReferenceParser {
     if ((_thisChar == $exclamation || _thisChar == $question) && _nextAtEnd) {
       return _SuffixParseResult.junk;
     }
-    if (_matchBraces($lparen, $rparen) || _matchBraces($lt, $gt)) {
+    if (_matchBraces($lparen, $rparen)) {
       return _SuffixParseResult.junk;
     }
 
@@ -331,8 +358,10 @@ class CommentReferenceParser {
     while (!_atEnd) {
       if (_thisChar == startChar) braceCount++;
       if (_thisChar == endChar) braceCount--;
-      ++_index;
-      if (braceCount == 0) return true;
+      _index++;
+      if (braceCount == 0) {
+        return true;
+      }
     }
     _index = startIndex;
     return false;
@@ -392,6 +421,32 @@ class _IdentifierParseResult {
       _IdentifierParseResult._(_IdentifierResultType.notIdentifier, null);
 }
 
+enum _TypeVariablesResultType {
+  endOfFile, // Found end of file instead of the beginning of a list of type
+  // variables.
+  notTypeVariables, // Found something, but it isn't type variables.
+  parsedTypeVariables, // Found type variables.
+}
+
+class _TypeVariablesParseResult {
+  final _TypeVariablesResultType type;
+
+  final TypeVariablesNode node;
+
+  const _TypeVariablesParseResult._(this.type, this.node);
+
+  factory _TypeVariablesParseResult.ok(TypeVariablesNode node) =>
+      _TypeVariablesParseResult._(
+          _TypeVariablesResultType.parsedTypeVariables, node);
+
+  static const _TypeVariablesParseResult endOfFile =
+      _TypeVariablesParseResult._(_TypeVariablesResultType.endOfFile, null);
+
+  static const _TypeVariablesParseResult notIdentifier =
+      _TypeVariablesParseResult._(
+          _TypeVariablesResultType.notTypeVariables, null);
+}
+
 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.
@@ -456,3 +511,19 @@ class IdentifierNode extends CommentReferenceNode {
   @override
   String toString() => 'Identifier["$text"]';
 }
+
+/// Represents one or more type variables, may be
+/// comma separated.
+class TypeVariablesNode extends CommentReferenceNode {
+  @override
+
+  /// Note that this will contain commas, spaces, and other text, as
+  /// generally type variables are a form of junk that comment references
+  /// should ignore.
+  final String text;
+
+  TypeVariablesNode(this.text);
+
+  @override
+  String toString() => 'TypeVariablesNode["$text"]';
+}

pubspec.yaml

@@ -7,7 +7,7 @@ environment:
   sdk: '>=2.11.99 <3.0.0'
 
 dependencies:
-  analyzer: ^2.1.0
+  analyzer: ^2.2.0
   args: ^2.0.0
   charcode: ^1.2.0
   collection: ^1.2.0

test/comment_referable/parser_test.dart

@@ -81,6 +81,18 @@ void main() {
           ['ThisThingy', '[]', 'parameter']);
     });
 
+    test('Check that embedded types within tearoff-like constructs parse', () {
+      expectParseEquivalent('this<stuff>.isValid', ['this', 'isValid']);
+      expectParseEquivalent(
+          'this<stuff, is, also>.isValid', ['this', 'isValid']);
+      expectParseEquivalent('this<stuff<that<is, real>, complicated>>.isValid',
+          ['this', 'isValid']);
+      expectParseError('this<stuff.isntValid');
+      expectParseError('this<stuff>, notBeingValid>.isntValid');
+      expectParseError('(AndThisMayBeValidDart.butIt).isntValidInReferences');
+      expectParseError('<NotActually>.valid');
+    });
+
     test('Basic negative tests', () {
       expectParseError(r'.');
       expectParseError(r'');

test/end2end/model_special_cases_test.dart

@@ -203,6 +203,31 @@ void main() {
         expect(referenceLookup(constructorTearoffs, 'Ft.new'),
             equals(MatchingLinkResult(Fnew)));
       });
+
+      test('we can use (ignored) type parameters in references', () {
+        expect(referenceLookup(E, 'D<String>.new'),
+            equals(MatchingLinkResult(Dnew)));
+        expect(referenceLookup(constructorTearoffs, 'F<T>.new'),
+            equals(MatchingLinkResult(Fnew)));
+        expect(
+            referenceLookup(
+                constructorTearoffs, 'F<InvalidThings, DoNotMatterHere>.new'),
+            equals(MatchingLinkResult(Fnew)));
+      });
+
+      test('negative tests', () {
+        // Mixins do not have constructors.
+        expect(referenceLookup(constructorTearoffs, 'M.new'),
+            equals(MatchingLinkResult(null)));
+        // These things aren't expressions, parentheses are still illegal.
+        expect(referenceLookup(constructorTearoffs, '(C).new'),
+            equals(MatchingLinkResult(null)));
+
+        // A bare new will still not work to reference constructors.
+        // TODO(jcollins-g): reconsider this if we remove "new" as a hint.
+        expect(referenceLookup(A, 'new'), equals(MatchingLinkResult(null)));
+        expect(referenceLookup(At, 'new'), equals(MatchingLinkResult(null)));
+      });
     }, skip: !_constructorTearoffsAllowed.allows(utils.platformVersion));
   });