Adds support for --generic-method-syntax

pkg/compiler/lib/src/parser/parser.dart

Index: pkg/compiler/lib/src/parser/parser.dart
diff --git a/pkg/compiler/lib/src/parser/parser.dart b/pkg/compiler/lib/src/parser/parser.dart
index 80e6de224e45c5fdb06da3c0743ddd3477add81b..93a696552f27493e427c54a2f4aa00287d7fc62b 100644
--- a/pkg/compiler/lib/src/parser/parser.dart
+++ b/pkg/compiler/lib/src/parser/parser.dart
@@ -41,6 +41,8 @@ import '../tokens/token_constants.dart' show
     EOF_TOKEN,
     EQ_TOKEN,
     FUNCTION_TOKEN,
+    GT_TOKEN,
+    GT_GT_TOKEN,
     HASH_TOKEN,
     HEXADECIMAL_TOKEN,
     IDENTIFIER_TOKEN,
@@ -103,8 +105,12 @@ class Parser {
   bool mayParseFunctionExpressions = true;
   bool asyncAwaitKeywordsEnabled;
 
-  Parser(this.listener, this.parserOptions,
-      {this.asyncAwaitKeywordsEnabled: false});
+  final bool _enableGenericMethodSyntax;

[ahe, 2016/04/08 09:53:33]

Please avoid private fields, a subclass outside this library may need access to
this for diet parsing, testing, etc.

[eernst, 2016/04/08 17:07:10]

I'm not sure what the right approach would be here. This value is available as
'parserOptions.enableGenericMethodSyntax' and the only reason why I do not use
that expression in the few locations where this value is needed is that it is
(presumably) a rather performance critical set of locations (parsing an
expression will happen very, very frequently, and checking whether it is a
message send with type arguments will happen very frequently, too), so I wanted
use a field in the same class rather than a getter invocation on another object.

But clients outside the parser itself would hardly need to worry so much about
micro-performance.

So is the extra field introduced here worthwhile at all, just to avoid that
getter invocation? And if this kind of micro-optimization is justified (inside
the parser), would it be useful to export such a field to the world?

At this point I just made it public.

+
+  Parser(this.listener, ParserOptions _parserOptions,

[ahe, 2016/04/08 09:53:32]

Remove _ from parameter.

[eernst, 2016/04/08 17:07:10]

Done.

+      {this.asyncAwaitKeywordsEnabled: false}) :
+          parserOptions = _parserOptions,

[ahe, 2016/04/08 09:53:32]

In:

  parserOption = parserOptions

The left-hand side refers to the field, the right hand side refers to the
parameter because parameters aren't in scope on the left-hand side. If you want
to be explicit about that, use:

  this.parserOptions = parserOptions

But it isn't necessary.

[eernst, 2016/04/08 17:07:10]

Acknowledged.

+          _enableGenericMethodSyntax = _parserOptions.enableGenericMethodSyntax;
 
   Token parseUnit(Token token) {
     listener.beginCompilationUnit(token);
@@ -525,6 +531,83 @@ class Parser {
     return false;
   }
 
+  /// Checks for '<' type (',' type)* '>', returns null iff failing.

[ahe, 2016/04/08 09:53:32]

Avoid using academic short-hand and notation in documentation. "iff" is an
abbreviation of "if and only" that matters a lot when being in mathematics, but
in documentation, simply using "if" is better.

[ahe, 2016/04/08 09:53:33]

How about:

Returns next token after match token if [token] matches `'<' type (',' type)*
'>'`.
Otherwise returns null.

[eernst, 2016/04/08 17:07:10]

Done.

+  /// Does not produce listener events. In this case the final '>' may be
+  /// the first character in a '>>' token, in which case the `endGroup` of
+  /// the initial '<' will be null, so we cannot rely on `endGroup`.
+  Token tryParseTypeArgumentsNested(Token token) {

[ahe, 2016/04/08 09:53:32]

Below I question if this method is necessary. However, if it really is, it
should be shared with tryParseTypeArguments, like so:

Token tryParseTypeArguments(Token token, bool isNested) {
}

[eernst, 2016/04/08 17:07:10]

The old (abandoned) CL did express the same distinction using a
boolean argument, but the two bodies actually differ in several
ways:

-----------------------------------------------------------
*   if (!identical(token?.kind, LT_TOKEN)) return null;
*   token = token.next;
    if (token == null) return null;
    token = tryParseType(token);
*   while (token != null && identical(token.kind, COMMA_TOKEN)) {
*     token = token.next;
*     if (token == null) return null;
*     token = tryParseType(token);
*   }
    if (token == null) return null;
    if (identical(token.kind, GT_TOKEN)) return token.next;
    if (!identical(token.kind, GT_GT_TOKEN)) return null;
    Token syntheticToken = new SymbolToken(GT_INFO, token.charOffset + 1);
    syntheticToken.next = token.next;
    return syntheticToken;
-----------------------------------------------------------
*   if (!identical(token?.kind, LT_TOKEN)) return null;
    BeginGroupToken beginToken = token;
    Token endToken = beginToken.endGroup;
*   token = token.next;
    if (token == null || endToken == null ||
        !identical(endToken.next?.kind, OPEN_PAREN_TOKEN)) return null;
    token = tryParseType(token);
*   while (token != null && identical(token.kind, COMMA_TOKEN)) {
*     token = token.next;
*     if (token == null) return null;
*     token = tryParseType(token);
*   }
    if (!identical(token?.kind, GT_TOKEN)) return null;
    return token.next;
-----------------------------------------------------------

I guess the readability of a single method would be lower than the readability
of these two methods separately, and it would also cost
a little bit extra at runtime when we keep testing that boolean
argument.

So do you still think it's better to merge them?

+    final kind = token.kind;

[ahe, 2016/04/08 09:53:33]

Remove this variable. It's confusing that token is overwritten but kind isn't.

[eernst, 2016/04/08 17:07:11]

Done.

+    if (!identical(kind, LT_TOKEN)) return null;
+    token = token.next;
+    if (token == null) return null;
+    token = tryParseType(token);
+    while (token != null && identical(token.kind, COMMA_TOKEN)) {
+      token = token.next;
+      if (token == null) return null;
+      token = tryParseType(token);
+    }
+    if (token == null) return null;
+    if (identical(token.kind, GT_TOKEN)) return token.next;
+    if (!identical(token.kind, GT_GT_TOKEN)) return null;
+    // [token] is '>>' of which the final '>' that we are parsing is the first
+    // character. In order to keep the parsing process on track we must return
+    // a synthetic '>' corresponding to the second character of that '>>'.
+    Token syntheticToken = new SymbolToken(GT_INFO, token.charOffset + 1);
+    syntheticToken.next = token.next;
+    return syntheticToken;
+  }
+
+  /// Checks for identifier ('.' identifier)?, returns null iff failing.

[ahe, 2016/04/08 09:53:33]

iff -> if.

What does "failing" mean? How about:

Returns next token if [token] matches `identifier ('.' identifier)?`.
Otherwise returns null.

[eernst, 2016/04/08 17:07:10]

Rephrased in a similar manner as several earlier doc comments.

+  /// Does not produce listener events.
+  Token tryParseQualified(Token token) {

[ahe, 2016/04/08 09:53:32]

There's probably suboptimal error recovery here. For example, it's natural for
people to try to access type variables as fields of classes or instances:

lib a;
class Foo<T> {
}

lib b;
import 'a.dart' as a;

main() {
  m<a.Foo.T>();
}

[eernst, 2016/04/08 17:07:10]

It would be nice if they _could_ do that. ;)

For improved error recovery, we will discuss in depth what to do in about a
week, so I did not take any actions here at this point.

+    if (!identical(token.kind, IDENTIFIER_TOKEN)) return null;
+    token = token.next;
+    if (!identical(token.kind, PERIOD_TOKEN)) return token;
+    token = token.next;
+    if (!identical(token.kind, IDENTIFIER_TOKEN)) return null;
+    return token.next;
+  }
+
+  /// Checks for typeName typeArguments? and returns null iff failing.

[ahe, 2016/04/08 09:53:32]

iff -> if, but what about:

Returns next token after match if [token] matches `typeName typeArguments?`.
Otherwise returns null.

[eernst, 2016/04/08 17:07:11]

Rephrased in a similar manner as several earlier doc comments.

+  /// Does not produce listener events.
+  Token tryParseType(Token token) {
+    token = tryParseQualified(token);
+    if (token == null) return null;
+    Token tokenAfterQualified = token;
+    token = tryParseTypeArgumentsNested(token);
+    return token == null ? tokenAfterQualified : token;
+  }
+
+  /// Checks for '<' type (',' type)* '>' '(' and returns null iff failing.

[ahe, 2016/04/08 09:53:33]

iff -> if, but what about:

Returns last token of match if [token] matches `'<' type (',' type)* '>' '('.
Otherwise returns null.

[eernst, 2016/04/08 17:07:10]

Rephrased in a similar manner as several earlier doc comments.

+  /// Does not produce listener events. With respect to the final '(', please
+  /// see the description of [isValidTypeArguments].
+  Token tryParseTypeArguments(Token token) {
+    final kind = token.kind;
+    if (!identical(kind, LT_TOKEN)) return null;
+    BeginGroupToken beginToken = token;
+    Token endToken = beginToken.endGroup;
+    token = token.next;
+    if (token == null || endToken == null ||
+        !identical(endToken.next?.kind, OPEN_PAREN_TOKEN)) return null;
+    token = tryParseType(token);

[ahe, 2016/04/08 09:53:32]

Is this really necessary? If so, perhaps push back on the specification to
ensure that it isn't.

I'm concerned that this approach will make it hard to produce good error
messages.

As far as I understand, the syntax is introduced with a known ambiguity that is
resolved in favor of generic arguments based on the argument that m<...>(...) is
infrequent.

If that is indeed the case, it follows that if the parser sees m<...>(...), the
user is likely attempting to pass arguments to a generic method. If so, what
happens if you accidentally forget or have an extra comma?

m<a b>(c);

m<a,b,>(c);

In these cases, you'll probably get some weird errors and the parser will not
recover well from the error.

+    while (token != null && identical(token.kind, COMMA_TOKEN)) {
+      token = token.next;
+      if (token == null) return null;
+      token = tryParseType(token);
+    }
+    if (!identical(token?.kind, GT_TOKEN)) return null;
+    return token.next;
+  }
+
+  /// Returns true iff the tokens starting from [token] match

[ahe, 2016/04/08 09:53:32]

iff->if.

We normally emit "otherwise returns false."

[eernst, 2016/04/08 17:07:10]

Rephrased in a similar manner as several earlier doc comments, which includes
the "otherwise returns false" phrase.

+  /// '<' type (',' type)* '>' '('. The final '(' is not part of the
+  /// grammar construct `typeArguments`, but it is required here such that
+  /// type arguments in generic method invocations can be recognized, and
+  /// as few as possible other constructs will pass (e.g., 'a < C, D > 3').
+  bool isValidTypeArguments(Token token) {

[ahe, 2016/04/08 09:53:32]

Rename to "isValidMethodTypeArguments" to signal that a trailing parenthesis is
required?

Can I assume that you can't "curry" type arguments as part of a tear-off
operation? That is, this doesn't work (assuming m is a generic method):

var f = m<a, b>;
f();

[eernst, 2016/04/08 17:07:10]

Makes sense, done. (This deviates from the naming of parsing methods after the
grammar non-terminals---but we're beyond that anyway, because of the trailing
'(').
 
Right, Florian did not want to support that, and Leaf seems to be relatively
willing to leave out the curried construct (a rather
short lambda can be used as a work-around).

[floitsch, 2016/04/11 11:00:13]

Fwiw, this CL only adds syntactic support. The semantic meaning is independent,
and both options are still open (although I would like to avoid currying).

+    return tryParseTypeArguments(token) != null;
+  }

[ahe, 2016/04/08 09:53:32]

Please reverse the order of all these methods. It's easier to read code if you
start with the main entry point (isValidTypeArguments) followed by the methods
it uses. Generally, try to sort methods so that f comes before g if f calls g.

[eernst, 2016/04/08 17:07:10]

Ah, that's funny. The literate programming credo (and a core design idea behind
Pascal) was "Don't use anything before it is defined". You would then be able to
read the code top-down, and you would never encounter an unknown "word" along
the way, because all definitions appear before their first use. Of course,
mutual recursion calls for an extra trick, but the main idea would be bottom up:
Define "small" things first, and then go to "bigger and bigger" ones, ending in
`MAIN`.

Your preference is obviously top-down, possibly based on a reading where we get
the overview first, and each of the smaller pieces are understood (better) when
we get to them, because we know in which context they are being used.

I'm not sure I've heard any specific policies on this before; various
preferences about class member declaration order have been discussed, and those
discussions were somewhat bottom-up oriented: fields and constructors come first
(a bottom-up element), and private methods go just before the public method that
uses them (also bottom-up), but the ordering of static vs instance members etc.
makes the whole issue less clear-cut. My general impression was, however, that
bottom-up is the most common preference.

Anyway, I've transformed these methods to use the top-down style.

+
   Token parseQualified(Token token) {
     token = parseIdentifier(token);
     while (optional('.', token)) {
@@ -1011,6 +1094,11 @@ class Parser {
     }
     Token token = parseIdentifier(name);
 
+    if (_enableGenericMethodSyntax && getOrSet == null) {
+      token = parseTypeVariablesOpt(token);
+    } else {
+      listener.handleNoTypeVariables(token);
+    }
     token = parseFormalParametersOpt(token);
     bool previousAsyncAwaitKeywordsEnabled = asyncAwaitKeywordsEnabled;
     token = parseAsyncModifier(token);
@@ -1319,7 +1407,8 @@ class Parser {
       // error handling.
       final String value = token.stringValue;
       if ((identical(value, '(')) || (identical(value, '.'))
-          || (identical(value, '{')) || (identical(value, '=>'))) {
+          || (identical(value, '{')) || (identical(value, '=>'))
+          || (_enableGenericMethodSyntax && identical(value, '<'))) {
         isField = false;
         break;
       } else if (identical(value, ';')) {
@@ -1420,6 +1509,11 @@ class Parser {
     }
 
     token = parseQualifiedRestOpt(token);
+    if (_enableGenericMethodSyntax && getOrSet == null) {
+      token = parseTypeVariablesOpt(token);
+    } else {
+      listener.handleNoTypeVariables(token);
+    }
     token = parseFormalParametersOpt(token);
     token = parseInitializersOpt(token);
     bool previousAsyncAwaitKeywordsEnabled = asyncAwaitKeywordsEnabled;
@@ -1506,6 +1600,11 @@ class Parser {
     }
     token = parseQualifiedRestOpt(token);
     listener.endFunctionName(token);
+    if (_enableGenericMethodSyntax && getOrSet == null) {
+      token = parseTypeVariablesOpt(token);
+    } else {
+      listener.handleNoTypeVariables(token);
+    }
     token = parseFormalParametersOpt(token);
     token = parseInitializersOpt(token);
     bool previousAsyncAwaitKeywordsEnabled = asyncAwaitKeywordsEnabled;
@@ -1546,6 +1645,11 @@ class Parser {
     listener.beginFunctionName(token);
     token = parseIdentifier(token);
     listener.endFunctionName(token);
+    if (_enableGenericMethodSyntax) {
+      token = parseTypeVariablesOpt(token);
+    } else {
+      listener.handleNoTypeVariables(token);
+    }
     token = parseFormalParameters(token);
     listener.handleNoInitializers();
     bool previousAsyncAwaitKeywordsEnabled = asyncAwaitKeywordsEnabled;
@@ -2011,6 +2115,7 @@ class Parser {
         listener.handleIndexedExpression(openSquareBracket, token);
         token = expect(']', token);
       } else if (optional('(', token)) {
+        listener.handleNoTypeArguments(token);
         token = parseArguments(token);
         listener.endSend(token);
       } else {
@@ -2108,6 +2213,7 @@ class Parser {
     token = token.next;
     if (optional('(', token)) {
       // Constructor forwarding.
+      listener.handleNoTypeArguments(token);
       token = parseArguments(token);
       listener.endSend(token);
     }
@@ -2119,6 +2225,7 @@ class Parser {
     token = token.next;
     if (optional('(', token)) {
       // Super constructor.
+      listener.handleNoTypeArguments(token);
       token = parseArguments(token);
       listener.endSend(token);
     }
@@ -2335,6 +2442,11 @@ class Parser {
   Token parseSend(Token token) {
     listener.beginSend(token);
     token = parseIdentifier(token);
+    if (_enableGenericMethodSyntax && isValidTypeArguments(token)) {
+      token = parseTypeArgumentsOpt(token);
+    } else {
+      listener.handleNoTypeArguments(token);
+    }
     token = parseArgumentsOpt(token);
     listener.endSend(token);
     return token;