initial generic method comment parsing

pkg/analyzer/lib/src/generated/parser.dart

Index: pkg/analyzer/lib/src/generated/parser.dart
diff --git a/pkg/analyzer/lib/src/generated/parser.dart b/pkg/analyzer/lib/src/generated/parser.dart
index 76f9dc7e76e32af10ddc3113de95be5c22ab1a81..d2c0c7c4601e8bb72fcfd79febf26a6fd5630ae7 100644
--- a/pkg/analyzer/lib/src/generated/parser.dart
+++ b/pkg/analyzer/lib/src/generated/parser.dart
@@ -2123,6 +2123,12 @@ class Parser {
   bool parseGenericMethods = false;
 
   /**
+   * A flag indicating whether to parse generic method comments, of the form
+   * `/*=T*/` and `/*<T>*/`.
+   */
+  bool parseGenericMethodComments = false;

[Jennifer Messerly, 2015/11/11 18:28:06]

aside, we don't actually need this flag in the parser -- it just tells us we
don't need to look for the special TokenTypes that the scanner will have
produced. After chatting with Leaf & Vijay, it sounds like maybe we can remove
this flag here, if that sounds good.

[Paul Berry, 2015/11/11 19:26:53]

My two cents: I'd actually lean slightly toward leaving the flag here, since it
will make it easier to find the code we need to remove once generic methods are
implemented for real and we want to get rid of the comment syntax.

But I don't feel terribly strongly either way.

[Jennifer Messerly, 2015/11/11 19:30:25]

good point.

[Brian Wilkerson, 2015/11/11 21:53:42]

I agree, I'd leave the flag in. You'll probably eventually want to add it to
AnalysisOptions and use that to initialize the field, then we can plumb it all
the way through to the .analysis_options file so that you can test the code more
easily.

That said, we had a conversation in the DDC meeting this morning about whether
this should be a separate flag, or whether it should be implied by strongMode.
I'm fine with the way you have it.

+
+  /**
    * Initialize a newly created parser to parse the content of the given
    * [_source] and to report any errors that are found to the given
    * [_errorListener].
@@ -2515,6 +2521,7 @@ class Parser {
           parseFormalParameterList());
     } else if (_tokenMatches(_peek(), TokenType.OPEN_PAREN)) {
       SimpleIdentifier methodName = parseSimpleIdentifier();
+      TypeParameterList typeParameters = _parseGenericCommentTypeParameters();
       FormalParameterList parameters = parseFormalParameterList();
       if (_matches(TokenType.COLON) ||
           modifiers.factoryKeyword != null ||
@@ -2537,7 +2544,7 @@ class Parser {
           modifiers.staticKeyword,
           null,
           methodName,
-          null,
+          typeParameters,
           parameters);
     } else if (_peek()
         .matchesAny([TokenType.EQ, TokenType.COMMA, TokenType.SEMICOLON])) {
@@ -2617,6 +2624,7 @@ class Parser {
       }
     } else if (_tokenMatches(_peek(), TokenType.OPEN_PAREN)) {
       SimpleIdentifier methodName = parseSimpleIdentifier();
+      TypeParameterList typeParameters = _parseGenericCommentTypeParameters();
       FormalParameterList parameters = parseFormalParameterList();
       if (methodName.name == className) {
         _reportErrorForNode(ParserErrorCode.CONSTRUCTOR_WITH_RETURN_TYPE, type);
@@ -2638,7 +2646,7 @@ class Parser {
           modifiers.staticKeyword,
           type,
           methodName,
-          null,
+          typeParameters,
           parameters);
     } else if (parseGenericMethods && _tokenMatches(_peek(), TokenType.LT)) {
       return _parseMethodDeclarationAfterReturnType(commentAndMetadata,
@@ -3135,10 +3143,7 @@ class Parser {
    *         typeParameters? formalParameterList functionExpressionBody
    */
   FunctionExpression parseFunctionExpression() {
-    TypeParameterList typeParameters = null;
-    if (parseGenericMethods && _matches(TokenType.LT)) {
-      typeParameters = parseTypeParameterList();
-    }
+    TypeParameterList typeParameters = _parseGenericMethodTypeParameters();
     FormalParameterList parameters = parseFormalParameterList();
     _validateFormalParameterList(parameters);
     FunctionBody body =
@@ -3265,10 +3270,7 @@ class Parser {
       period = _expect(TokenType.PERIOD);
     }
     SimpleIdentifier identifier = parseSimpleIdentifier();
-    TypeParameterList typeParameters = null;
-    if (parseGenericMethods && _matches(TokenType.LT)) {
-      typeParameters = parseTypeParameterList();
-    }
+    TypeParameterList typeParameters = _parseGenericMethodTypeParameters();
     if (_matches(TokenType.OPEN_PAREN)) {
       FormalParameterList parameters = parseFormalParameterList();
       if (thisKeyword == null) {
@@ -3476,6 +3478,18 @@ class Parser {
    *         qualified typeArguments?
    */
   TypeName parseTypeName() {
+    TypeName result = _parseTypeName();
+    // If this is followed by a generic method type comment, allow the comment
+    // type to replace the real type name.
+    // TODO(jmesserly): this feels like a big hammer. Can we restrict it to
+    // only work inside generic methods?

[Jennifer Messerly, 2015/11/11 18:28:06]

This feature weirds me out. I kind of wonder if it would be nicer to handle
these comments later, after we already know we're in a generic method, and we
know what type parameter names to expect. Then we could only match comments that
exactly refer to those type parameters.

That said, this is all a very temporary measure, and searching our corpus of
code didn't find any other uses of comments of the form /*=T*/.

[Brian Wilkerson, 2015/11/11 21:53:42]

Given that this only applies to return types and parameter types, perhaps the
extra work should be done there rather than for all type names.

[Paul Berry, 2015/11/11 22:01:58]

I would think it could potentially apply to other types names, such as those
used in local variable declarations, e.g.:

void f/*<T>*/() {
  var/*=T*/ x;
}

[Jennifer Messerly, 2015/11/11 22:04:12]

EDIT: Paul beat me to it. But here was my comment.

in theory it's to cover all places generic type parameters can appear. Some
examples:

    List<num/*=T*/> foo;

    foo = new List<num/*=T*/>();
    
   <String, dynamic/*=V*/>{}

    if (x is dynamic/*=K*/) { ... }

    var firstClassType = dynamic/*=T*/;

(I suspect the best choice for the "replaced" type will be whatever the upper
type bound is. Typically `dynamic` but can be other types if `T extends
SomeType` ... I wish this could be enforced but doesn't seem possible in the
parser.)

+    if (_injectGenericCommentTypeAssign()) {
+      return _parseTypeName();
+    }
+    return result;
+  }
+
+  TypeName _parseTypeName() {
     Identifier typeName;
     if (_matchesKeyword(Keyword.VAR)) {
       _reportErrorForCurrentToken(ParserErrorCode.VAR_AS_TYPE_NAME);
@@ -3486,10 +3500,7 @@ class Parser {
       typeName = _createSyntheticIdentifier();
       _reportErrorForCurrentToken(ParserErrorCode.EXPECTED_TYPE_NAME);
     }
-    TypeArgumentList typeArguments = null;
-    if (_matches(TokenType.LT)) {
-      typeArguments = parseTypeArgumentList();
-    }
+    TypeArgumentList typeArguments = _maybeParseTypeArguments();
     return new TypeName(typeName, typeArguments);
   }
 
@@ -3909,6 +3920,47 @@ class Parser {
     return null;
   }
 
+  bool _injectGenericComment(TokenType type, int prefixLen) {
+    if (parseGenericMethodComments) {

[Jennifer Messerly, 2015/11/11 18:28:06]

as noted above, this guard shouldn't really be necessary, but it lets us skip
the comment search. Premature optimization?

+      CommentToken t = _currentToken.precedingComments;
+      for (; t != null; t = t.next) {
+        if (t.type == TokenType.GENERIC_METHOD_TYPE_LIST) {

[Jennifer Messerly, 2015/11/11 20:52:27]

Bug here. the /*=T*/ comments weren't working because of it. The tests weren't
checking the tree well enough to catch it. Fixing this & improving the test
coverage by validating the parameter and return types.

+          String comment = t.lexeme.substring(prefixLen, t.lexeme.length - 2);
+          Token list = _scanGenericMethodComment(comment, t.offset + prefixLen);

[Paul Berry, 2015/11/11 19:26:53]

What will happen if the comment is matched by
Scanner._matchGenericMethodCommentType() but is not actually grammatically valid
(e.g. "/*<hi, mom!>*/").  Will the parser get horribly confused?

[Jennifer Messerly, 2015/11/11 19:30:25]

Should be exactly as if you wrote that without the comment, a parse error of
some sort.

That's a good point though. We could use some more negative tests for these
cases in parser_test. I'll add some.

+          if (list != null) {
+            // TODO(jmesserly): detach the old comment token?

[Brian Wilkerson, 2015/11/11 21:53:42]

It shouldn't actually hurt anything if you leave it.

[Jennifer Messerly, 2015/11/12 00:20:00]

Sounds good. Removed the TODO.

+            // Insert the tokens into the stream.
+            _injectTokenList(list);
+            return true;
+          }
+        }
+      }
+    }
+    return false;
+  }
+
+  /**
+     * Matches a generic comment type substitution and injects it into the token

[Jennifer Messerly, 2015/11/11 18:28:06]

oops, fixed this locally. Not sure how it happened

+     * stream. Returns true if a match was injected, otherwise false.
+     *
+     * These comments are of the form `/*=T*/`, in other words, a [TypeName]
+     * inside a slash-star comment, preceded by equals sign.
+     */
+  bool _injectGenericCommentTypeAssign() {
+    return _injectGenericComment(TokenType.GENERIC_METHOD_TYPE_ASSIGN, 3);
+  }
+
+  /**
+   * Matches a generic comment type parameters and injects them into the token
+   * stream. Returns true if a match was injected, otherwise false.
+   *
+   * These comments are of the form `/*<K, V>*/`, in other words, a
+   * [TypeParameterList] or [TypeArgumentList] inside a slash-star comment.
+   */
+  bool _injectGenericCommentTypeList() {
+    return _injectGenericComment(TokenType.GENERIC_METHOD_TYPE_LIST, 2);
+  }
+
   /**
    * Inject the given [token] into the token stream immediately before the
    * current token.
@@ -3920,6 +3972,19 @@ class Parser {
     return token;
   }
 
+  void _injectTokenList(Token firstToken) {

[Jennifer Messerly, 2015/11/11 18:28:06]

I used the same pattern as the existing _injectToken.

Is there anything to be worried about here? It is mutating the token linked
list, but we seem to do that in other places. Not sure if I should be marking
each token as synthetic or something like that?

Anyway something to try out in the UI -- what does this colorize like.

+    // Scanner creates a cyclic EOF token.
+    Token lastToken = firstToken;
+    while (lastToken.next.type != TokenType.EOF) {
+      lastToken = lastToken.next;
+    }
+    // Inject these new tokens into the stream.
+    Token previous = _currentToken.previous;
+    lastToken.setNext(_currentToken);
+    previous.setNext(firstToken);
+    _currentToken = firstToken;
+  }
+
   /**
    * Return `true` if the current token appears to be the beginning of a
    * function declaration.
@@ -4271,6 +4336,17 @@ class Parser {
           _currentToken.lexeme == identifier;
 
   /**
+   * Parse a [TypeArgumentList] if present, otherwise return null.
+   * This also supports the comment form, if enabled: `/*<T>*/`
+   */
+  TypeArgumentList _maybeParseTypeArguments() {

[Brian Wilkerson, 2015/11/11 21:53:42]

For consistency with "_parseOptionalReturnType", perhaps
"_parseOptionalTypeArguments"?

[Jennifer Messerly, 2015/11/12 00:20:00]

Done.

+    if (_matches(TokenType.LT) || _injectGenericCommentTypeList()) {
+      return parseTypeArgumentList();
+    }
+    return null;
+  }
+
+  /**
    * If the current token has the given [type], then advance to the next token
    * and return `true`. Otherwise, return `false` without advancing. This method
    * should not be invoked with an argument value of [TokenType.GT].
@@ -4361,10 +4437,7 @@ class Parser {
     bool isOptional = primaryAllowed || expression is SimpleIdentifier;
     while (true) {
       while (_isLikelyParameterList()) {
-        TypeArgumentList typeArguments = null;
-        if (_matches(TokenType.LT)) {
-          typeArguments = parseTypeArgumentList();
-        }
+        TypeArgumentList typeArguments = _maybeParseTypeArguments();
         ArgumentList argumentList = parseArgumentList();
         if (expression is SimpleIdentifier) {
           expression = new MethodInvocation(null, null,
@@ -4573,10 +4646,7 @@ class Parser {
         (expression != null && functionName == null));
     if (_isLikelyParameterList()) {
       while (_isLikelyParameterList()) {
-        TypeArgumentList typeArguments = null;
-        if (_matches(TokenType.LT)) {
-          typeArguments = parseTypeArgumentList();
-        }
+        TypeArgumentList typeArguments = _maybeParseTypeArguments();
         if (functionName != null) {
           expression = new MethodInvocation(expression, period, functionName,
               typeArguments, parseArgumentList());
@@ -4604,10 +4674,7 @@ class Parser {
         expression = selector;
         progress = true;
         while (_isLikelyParameterList()) {
-          TypeArgumentList typeArguments = null;
-          if (_matches(TokenType.LT)) {
-            typeArguments = parseTypeArgumentList();
-          }
+          TypeArgumentList typeArguments = _maybeParseTypeArguments();
           if (expression is PropertyAccess) {
             PropertyAccess propertyAccess = expression as PropertyAccess;
             expression = new MethodInvocation(
@@ -6025,10 +6092,7 @@ class Parser {
       keyword = getAndAdvance();
     }
     SimpleIdentifier name = parseSimpleIdentifier();
-    TypeParameterList typeParameters = null;
-    if (parseGenericMethods && _matches(TokenType.LT)) {
-      typeParameters = parseTypeParameterList();
-    }
+    TypeParameterList typeParameters = _parseGenericMethodTypeParameters();
     FormalParameterList parameters = null;
     if (!isGetter) {
       if (_matches(TokenType.OPEN_PAREN)) {
@@ -6182,6 +6246,36 @@ class Parser {
   }
 
   /**
+     * Parses generic type parameters from a comment.
+     *
+     * Normally this is handled by [_parseGenericMethodTypeParameters], but if the

[Jennifer Messerly, 2015/11/11 18:28:06]

fixed this locally, will be in next upload

+     * code already handles the normal generic type parameters, the comment
+     * matcher can be called directly. For example, we may have already tried
+     * matching `<` (less than sign) in a method declaration, and be currently
+     * on the `(` (open paren) because we didn't find it. In that case, this
+     * function will parse the preceding comment such as `/*<T, R>*/`.
+     */
+  TypeParameterList _parseGenericCommentTypeParameters() {
+    if (_injectGenericCommentTypeList()) {
+      return parseTypeParameterList();
+    }
+    return null;
+  }
+
+  /**
+   * Parse the generic method or function's type parameters.
+   *
+   * For backwards compatibility this can optionally use comments.
+   * See [parseGenericMethodComments].
+   */
+  TypeParameterList _parseGenericMethodTypeParameters() {
+    if (parseGenericMethods && _matches(TokenType.LT) ||
+        _injectGenericCommentTypeList()) {
+      return parseTypeParameterList();
+    }
+  }
+
+  /**
    * Parse a getter. The [commentAndMetadata] is the documentation comment and
    * metadata to be associated with the declaration. The externalKeyword] is the
    * 'external' token. The staticKeyword] is the static keyword, or `null` if
@@ -6473,10 +6567,7 @@ class Parser {
    *       | mapLiteral
    */
   TypedLiteral _parseListOrMapLiteral(Token modifier) {
-    TypeArgumentList typeArguments = null;
-    if (_matches(TokenType.LT)) {
-      typeArguments = parseTypeArgumentList();
-    }
+    TypeArgumentList typeArguments = _maybeParseTypeArguments();
     if (_matches(TokenType.OPEN_CURLY_BRACKET)) {
       return _parseMapLiteral(modifier, typeArguments);
     } else if (_matches(TokenType.OPEN_SQUARE_BRACKET) ||
@@ -6610,10 +6701,7 @@ class Parser {
       Token staticKeyword,
       TypeName returnType) {
     SimpleIdentifier methodName = parseSimpleIdentifier();
-    TypeParameterList typeParameters = null;
-    if (parseGenericMethods && _matches(TokenType.LT)) {
-      typeParameters = parseTypeParameterList();
-    }
+    TypeParameterList typeParameters = _parseGenericMethodTypeParameters();
     FormalParameterList parameters;
     if (!_matches(TokenType.OPEN_PAREN) &&
         (_matches(TokenType.OPEN_CURLY_BRACKET) ||
@@ -7081,10 +7169,7 @@ class Parser {
         (parseGenericMethods && _matches(TokenType.LT))) {
       do {
         if (_isLikelyParameterList()) {
-          TypeArgumentList typeArguments = null;
-          if (_matches(TokenType.LT)) {
-            typeArguments = parseTypeArgumentList();
-          }
+          TypeArgumentList typeArguments = _maybeParseTypeArguments();
           ArgumentList argumentList = parseArgumentList();
           if (operand is PropertyAccess) {
             PropertyAccess access = operand as PropertyAccess;
@@ -8084,6 +8169,23 @@ class Parser {
   }
 
   /**
+   * Scans the generic method comment, and returns the tokens, otherwise
+   * returns null.
+   */
+  Token _scanGenericMethodComment(String code, int offset) {
+    BooleanErrorListener listener = new BooleanErrorListener();
+    Scanner scanner =
+        new Scanner(null, new SubSequenceReader(code, offset), listener);
+    scanner.setSourceStart(1, 1);
+    Token firstToken = scanner.tokenize();
+    if (listener.errorReported) {
+      // TODO(jmesserly): should we report these messages?

[Brian Wilkerson, 2015/11/11 21:53:42]

It would help you find bugs while using this feature, but given that it is
short-lived it doesn't seem necessary.

[Jennifer Messerly, 2015/11/12 00:20:00]

Good point. Removed the TODO.

+      return null;
+    }
+    return firstToken;
+  }
+
+  /**
    * Skips a block with all containing blocks.
    */
   void _skipBlock() {
@@ -8420,7 +8522,8 @@ class Parser {
    */
   Token _skipTypeArgumentList(Token startToken) {
     Token token = startToken;
-    if (!_tokenMatches(token, TokenType.LT)) {
+    if (!_tokenMatches(token, TokenType.LT) &&
+        !_injectGenericCommentTypeList()) {
       return null;
     }
     token = _skipTypeName(token.next);