First step of support for parsing and ignoring generic methods.

First step of support for parsing and ignoring generic methods.

This CL introduces support for generic method syntax: It is recognized
by the parser but no listener actions are invoked during that parsing
process. The following constructs have been added: type parameters can
be declared on function and method declarations, with the same syntax
as type parameters on class declarations, and type arguments can be
delivered in message send constructs.

Positive tests have been added, showing a range of different correct
usages. (Negative tests will be added in a follow-up CL).

Status files have been adjusted to indicate that the new tests are
`CompileTimeError`s in all cases (compilers + analyzer). If the
`--enable-generic-methods` flag is passed, `dartj2s` will not have
a compile time error.

Relative to the DEP #22 proposal, there is no support for currying
invocations (that is, we cannot use `myMethod<T,S>` as an closure
which is non-generic and which will pass the actual values of `T`
and `S` in the given context as type arguments).

The well-known syntactic conflict associated with relative expressions
is resolved in favor of generic invocations (for instance, with
`foo(m<B, C>(3))` we parse it as an invocation of `m` with type
arguments `B, C` and value argument `3`, not as two boolean
arguments passed to `foo`. It is expected that this situation is
unlikely to occur often in practice, especially because a comma or
')' will delimit the expression where we have `3` in the example,
which means that it won't need to be wrapped in parentheses. Of
course, the work-around `(C>(3))` is available if it should still
occur.

Neither the proposal in DEP #22 nor this CL adds any support for type
parameterization of literal functions (i.e., expressions like
`(x) => x`).

[eernst, 2016-02-22 12:00:57]

eernst@google.com changed reviewers:
+ ahe@google.com, johnniwinther@google.com

[eernst, 2016-02-22 12:00:57]

Johnni and Peter: I'm not sure who's going to be back first, so I'm sending this
review to both of you, but it should be fine if one takes it and the other
ignores it.

[floitsch, 2016-02-22 12:45:19]

floitsch@google.com changed reviewers:
+ floitsch@google.com

[floitsch, 2016-02-22 12:45:20]

LGTM with comments.
Please add tests that focus on the new feature. For example, there is no test
for ">>", or "f(g<int, String>(3))".

As discussed offline: have two test-files.
1. (probably) semantically valid.
2. valid syntax, but non-sensical (calling a method with a too many generic
arguments, ...)

https://codereview.chromium.org/1723443003/diff/1/pkg/compiler/lib/src/parser...
File pkg/compiler/lib/src/parser/parser.dart (right):

https://codereview.chromium.org/1723443003/diff/1/pkg/compiler/lib/src/parser...
pkg/compiler/lib/src/parser/parser.dart:574: case COMMA_TOKEN: break;
couldn't you treat the "comma" the same way as the "." ?

https://codereview.chromium.org/1723443003/diff/1/pkg/compiler/lib/src/parser...
pkg/compiler/lib/src/parser/parser.dart:884: Token
parseAndIgnoreTypeParametersOpt(Token token) {
All these duplicated parseAndIgnore make me think that it's easier if you just
ignore them in the listener.

https://codereview.chromium.org/1723443003/diff/1/pkg/compiler/lib/src/serial...
File pkg/compiler/lib/src/serialization/modelz.dart (right):

https://codereview.chromium.org/1723443003/diff/1/pkg/compiler/lib/src/serial...
pkg/compiler/lib/src/serialization/modelz.dart:857: bool
implementsFunction(CoreClasses coreClasses) {
How is this change related to generic methods?

https://codereview.chromium.org/1723443003/diff/1/tests/language/language.status
File tests/language/language.status (right):

https://codereview.chromium.org/1723443003/diff/1/tests/language/language.sta...
tests/language/language.status:174: [ ($compiler == none || $compiler ==
precompiler || $compiler == dart2app) ]
Don't add a new section if it already exists. (same for the analyzer file).

https://codereview.chromium.org/1723443003/diff/1/tests/language/language.sta...
tests/language/language.status:176: generic_methods_test: CompiletimeError # Not
yet implemented.
Failing tests should have an issue number. If it doesn't exist yet, create one.

(same for the analyzer file).

https://codereview.chromium.org/1723443003/diff/1/tests/language/language_ana...
File tests/language/language_analyzer2.status (right):

https://codereview.chromium.org/1723443003/diff/1/tests/language/language_ana...
tests/language/language_analyzer2.status:525: generic_functions_test:
CompileTimeError # Not yet implemented.
The analyzer already supports generic methods. not sure if they expose a flag,
though.

https://codereview.chromium.org/1723443003/diff/1/tests/language/language_dar...
File tests/language/language_dart2js.status (right):

https://codereview.chromium.org/1723443003/diff/1/tests/language/language_dar...
tests/language/language_dart2js.status:322: # Experimental feature: Generic
methods.
Make the test add the flag.

[floitsch, 2016-02-22 12:46:06]

Also: wait for Johnni and/or Peter.

[eernst, 2016-02-22 15:16:57]

Some extra information about the test failures I mentioned (to Florian):

Running the `test-all.sh` script I got from Florian a while ago,
there are 16 occasions where we get a 'The compiler is broken' printout,
involving 22 `Internal Error` messages in total.

This situation is identical (same messages, not just same number of
messages) with the 'master' branch (sha1 
270ad3f5521ad0b63a220c77f5663a6d19e0d5fd) and the
'polymorphic_methods_ignore' branch starting from there that corresponds
to this CL, i.e., it does not seem to be related to this CL.

[eernst, 2016-02-22 18:02:52]

A question is embedded in the review response, search for 'added'.

https://codereview.chromium.org/1723443003/diff/1/pkg/compiler/lib/src/parser...
File pkg/compiler/lib/src/parser/parser.dart (right):

https://codereview.chromium.org/1723443003/diff/1/pkg/compiler/lib/src/parser...
pkg/compiler/lib/src/parser/parser.dart:574: case COMMA_TOKEN: break;
On 2016/02/22 12:45:20, floitsch wrote:
> couldn't you treat the "comma" the same way as the "." ?

Not immediately: The comma separates complex constructs (types) whereas the
period only separates identifiers. This means that we can make `identifier` and
`identifier.identifier` work as "atoms" such that we don't accept things like
`<.identifier`, which makes the over-approximation of the actual language more
tight. I don't think there is an easy analogous way to make it tighter around
commas.

It is possible, though, that we should use a general parsing strategy rather
than a "near regular expression" check, if the over-approximation turns out to
be too loose. But that would presumably require a duplication of a lot of
parsing methods, such that we can parse `typeArguments` in general without
splitting ['>>'] to ['>', '>']. Alternatively, we could split '>>', but remember
where we did it and then undo it if the `isValidTypeArguments` call returns
false.

https://codereview.chromium.org/1723443003/diff/1/pkg/compiler/lib/src/parser...
pkg/compiler/lib/src/parser/parser.dart:884: Token
parseAndIgnoreTypeParametersOpt(Token token) {
On 2016/02/22 12:45:20, floitsch wrote:
> All these duplicated parseAndIgnore make me think that it's easier if you just
> ignore them in the listener.

Will probably have to do that in the next step anyway, more or less, since it
needs to know about the declared types and make them work as `dynamic`.
Presumably it makes sense to postpone it until I've studied enough of the
compiler to know how to do that.

https://codereview.chromium.org/1723443003/diff/1/pkg/compiler/lib/src/serial...
File pkg/compiler/lib/src/serialization/modelz.dart (right):

https://codereview.chromium.org/1723443003/diff/1/pkg/compiler/lib/src/serial...
pkg/compiler/lib/src/serialization/modelz.dart:857: bool
implementsFunction(CoreClasses coreClasses) {
On 2016/02/22 12:45:20, floitsch wrote:
> How is this change related to generic methods?

It is unrelated. The IDE complained about the type of this overriding method
declaration. The argument is unused here, and there is no obvious reason why the
argument type should differ from the inherited one. So I fixed it, considering
it to be a typo, in order to make the IDE happy (and, presumably, avoid a
potential checked-mode error that makes no sense anyway).

Should it be a separate CL?

https://codereview.chromium.org/1723443003/diff/1/tests/language/language.status
File tests/language/language.status (right):

https://codereview.chromium.org/1723443003/diff/1/tests/language/language.sta...
tests/language/language.status:174: [ ($compiler == none || $compiler ==
precompiler || $compiler == dart2app) ]
On 2016/02/22 12:45:20, floitsch wrote:
> Don't add a new section if it already exists. (same for the analyzer file).

Done.

The 'language_analyzer2.status' file contains only a single section, so I did
not change that. The 'language_dart2js.status' file contains several sections
guarded by `[ $compiler == dart2js ]`, I moved the new tests to the first one.

https://codereview.chromium.org/1723443003/diff/1/tests/language/language.sta...
tests/language/language.status:176: generic_methods_test: CompiletimeError # Not
yet implemented.
On 2016/02/22 12:45:20, floitsch wrote:
> Failing tests should have an issue number. If it doesn't exist yet, create
one.
> 
> (same for the analyzer file).

Done, #25835.

https://codereview.chromium.org/1723443003/diff/1/tests/language/language_ana...
File tests/language/language_analyzer2.status (right):

https://codereview.chromium.org/1723443003/diff/1/tests/language/language_ana...
tests/language/language_analyzer2.status:525: generic_functions_test:
CompileTimeError # Not yet implemented.
On 2016/02/22 12:45:20, floitsch wrote:
> The analyzer already supports generic methods. not sure if they expose a flag,
> though.

Haven't been able to spot a suitable issue number for the analyzer. Sent mail to
Brian in order to learn more about the possible flag and the issue number.

https://codereview.chromium.org/1723443003/diff/1/tests/language/language_dar...
File tests/language/language_dart2js.status (right):

https://codereview.chromium.org/1723443003/diff/1/tests/language/language_dar...
tests/language/language_dart2js.status:322: # Experimental feature: Generic
methods.
On 2016/02/22 12:45:20, floitsch wrote:
> Make the test add the flag.

How can the flag be added?

[floitsch, 2016-02-22 18:40:27]

https://codereview.chromium.org/1723443003/diff/1/pkg/compiler/lib/src/comman...
File pkg/compiler/lib/src/commandline_options.dart (right):

https://codereview.chromium.org/1723443003/diff/1/pkg/compiler/lib/src/comman...
pkg/compiler/lib/src/commandline_options.dart:52: static const String
genericMethods = '--enable-generic-methods';
drop the "enable".

https://codereview.chromium.org/1723443003/diff/1/pkg/compiler/lib/src/serial...
File pkg/compiler/lib/src/serialization/modelz.dart (right):

https://codereview.chromium.org/1723443003/diff/1/pkg/compiler/lib/src/serial...
pkg/compiler/lib/src/serialization/modelz.dart:857: bool
implementsFunction(CoreClasses coreClasses) {
On 2016/02/22 18:02:51, eernst wrote:
> On 2016/02/22 12:45:20, floitsch wrote:
> > How is this change related to generic methods?
> 
> It is unrelated. The IDE complained about the type of this overriding method
> declaration. The argument is unused here, and there is no obvious reason why
the
> argument type should differ from the inherited one. So I fixed it, considering
> it to be a typo, in order to make the IDE happy (and, presumably, avoid a
> potential checked-mode error that makes no sense anyway).
> 
> Should it be a separate CL?

https://codereview.chromium.org/1723653002

https://codereview.chromium.org/1723443003/diff/1/tests/language/language_ana...
File tests/language/language_analyzer2.status (right):

https://codereview.chromium.org/1723443003/diff/1/tests/language/language_ana...
tests/language/language_analyzer2.status:525: generic_functions_test:
CompileTimeError # Not yet implemented.
On 2016/02/22 18:02:51, eernst wrote:
> On 2016/02/22 12:45:20, floitsch wrote:
> > The analyzer already supports generic methods. not sure if they expose a
flag,
> > though.
> 
> Haven't been able to spot a suitable issue number for the analyzer. Sent mail
to
> Brian in order to learn more about the possible flag and the issue number.

The options file supports it:
$ cat /tmp/options
analyzer:
  language:
    enableGenericMethods: true

$ dartanalyzer --options=/tmp/options gen_methods.dart

Note, that the analyzer needs to change its behavior (or provide a different
flag), since it currently does semantic checks too.
The syntactic parser exists, though.

https://codereview.chromium.org/1723443003/diff/1/tests/language/language_dar...
File tests/language/language_dart2js.status (right):

https://codereview.chromium.org/1723443003/diff/1/tests/language/language_dar...
tests/language/language_dart2js.status:322: # Experimental feature: Generic
methods.
On 2016/02/22 18:02:51, eernst wrote:
> On 2016/02/22 12:45:20, floitsch wrote:
> > Make the test add the flag.
> 
> How can the flag be added?

// DartOptions=--generic-methods
see config_import_test.dart

[Leaf, 2016-02-22 19:18:02]

leafp@google.com changed reviewers:
+ leafp@google.com

[Leaf, 2016-02-22 19:18:03]

Great to see this happening!  A couple of comments.

Gilad and I hashed out some changes that he wanted to see to the DEP last summer
that expand the scope a bit beyond what I originally wrote up in the DEP. You
can see a draft of these in terms of the spec here:

https://codereview.chromium.org/1177073002/

I will update the DEP writeup to reflect these changes to avoid confusion and to
provide a more textual reference since there is now active work on this.  

The notable change from a syntactic standpoint is that with first class generic
functions there are a few more syntactic forms to support. 

1) Generic function expressions (which weren't useful in my original
formulation) are supported in the draft spec, as ```<T0 extends B0, ..., Tn
extends Bn> f```, where f is a standard (non-generic) function expression.

2) Generic function typed parameters are now allowed e.g.:  
  int applyId(T id<T>(T x)) => id<int>(3);

Note that local generic functions continue to be supported as well (not sure if
those are covered by this CL or not).

[ahe, 2016-02-23 08:05:27]

Some of the information contained in this CL's description belongs as comments
in the code, in particular the paragraph on "The well-known syntactic conflict".

Generally, descriptions should not be so long, and I suggest you go through each
paragraph and decide where it belongs as a comment in code. Some of the
paragraphs, like the one on changes to status files is more a note to your
reviewers. So it belongs in the "CL message". Same applies to paragraph on
"positive tests".

Not creating listener actions is problematic, as it will make it harder to
support creating AST nodes, which I assume is the long term goal. As far as I
can tell, you'll have to rewrite this approach completely to support AST nodes,
and I see little advantage of not completing this work up front. In particular,
I think adding the listeners will address the TODO in the parse method.

Please reach out to me on a video call, and I'll be happy to walk you through my
concerns (and proposed solution) in more details.

https://codereview.chromium.org/1723443003/diff/1/pkg/compiler/lib/src/parser...
File pkg/compiler/lib/src/parser/element_listener.dart (right):

https://codereview.chromium.org/1723443003/diff/1/pkg/compiler/lib/src/parser...
pkg/compiler/lib/src/parser/element_listener.dart:71: class ParserOptions {
Is this class really needed?

https://codereview.chromium.org/1723443003/diff/1/pkg/compiler/lib/src/parser...
File pkg/compiler/lib/src/parser/parser.dart (right):

https://codereview.chromium.org/1723443003/diff/1/pkg/compiler/lib/src/parser...
pkg/compiler/lib/src/parser/parser.dart:537: // sublanguage is so tiny:
The scanner should already have computed this for you. See:

ArrayBasedScanner.appendGtGt

For example, in the case `foo(a < 0, b >> 2)`, the token representing `<`, will
have endGroup null, and thus cannot be a type argument.

In the case, `foo(a < 0, b > 2)`, again you don't need to count the angle
brackets. All you need to do is look at endGroup.next.

Once you take advantage of the fact that you can look ahead to see what comes
after the closing '>', you should be able to recursively answer this question
which should be simpler and let you remove the TODO. In addition, you'll be
making it easier to actually create the AST nodes required long term.

https://codereview.chromium.org/1723443003/diff/1/pkg/compiler/lib/src/parser...
pkg/compiler/lib/src/parser/parser.dart:564: // Invariant: `angleBracketLevel`
is #'<' - #'>' from initial token
What does #'<' mean?

https://codereview.chromium.org/1723443003/diff/1/pkg/compiler/lib/src/parser...
pkg/compiler/lib/src/parser/parser.dart:565: // to `token`, both included, and
considering ['>>'] as ['>', '>'].
I don't think ['>>'] is a valid reference.

[floitsch, 2016-02-23 10:40:48]

https://codereview.chromium.org/1723443003/diff/1/pkg/compiler/lib/src/comman...
File pkg/compiler/lib/src/commandline_options.dart (right):

https://codereview.chromium.org/1723443003/diff/1/pkg/compiler/lib/src/comman...
pkg/compiler/lib/src/commandline_options.dart:52: static const String
genericMethods = '--enable-generic-methods';
Think about it: make sure this contains "syntax" in the name:
--generic-method-syntax

Otherwise you conflict with the existing flag from the analyzer, and it will
become harder to add the semantic flag (which I would then call
'--generic-methods'.

[eernst, 2016-02-23 11:58:47]

On 2016/02/23 10:40:48, floitsch wrote:
>
https://codereview.chromium.org/1723443003/diff/1/pkg/compiler/lib/src/comman...
> File pkg/compiler/lib/src/commandline_options.dart (right):
> 
>
https://codereview.chromium.org/1723443003/diff/1/pkg/compiler/lib/src/comman...
> pkg/compiler/lib/src/commandline_options.dart:52: static const String
> genericMethods = '--enable-generic-methods';
> Think about it: make sure this contains "syntax" in the name:
> --generic-method-syntax
> 
> Otherwise you conflict with the existing flag from the analyzer, and it will
> become harder to add the semantic flag (which I would then call
> '--generic-methods'.

OK, made it '--generic-method-syntax'.

I was thinking about the set of flags as a somewhat limited resource and
planned for reusing them for several different levels of support (so
'--generic-methods' would mean "just skip over this syntax" right now,
later it would mean "parse this syntax, but consider all the declared type
variables to have type `dynamic`", and later again "provide full support
for generic methods").

But of course we can have one flag for each level, and that's probably less
confusing. As far as I can see on the list of strings in
'commandline_options.dart', it is possible to deprecate and then delete them
again.

[eernst, 2016-02-23 12:40:45]

Need to clarify how to create the right issues for the vm, precompiler,
dart2app, and possible more.

Several discussions continued.

PTAL.

https://codereview.chromium.org/1723443003/diff/1/pkg/compiler/lib/src/comman...
File pkg/compiler/lib/src/commandline_options.dart (right):

https://codereview.chromium.org/1723443003/diff/1/pkg/compiler/lib/src/comman...
pkg/compiler/lib/src/commandline_options.dart:52: static const String
genericMethods = '--enable-generic-methods';
On 2016/02/22 18:40:27, floitsch wrote:
> drop the "enable".

Done.

https://codereview.chromium.org/1723443003/diff/1/pkg/compiler/lib/src/parser...
File pkg/compiler/lib/src/parser/element_listener.dart (right):

https://codereview.chromium.org/1723443003/diff/1/pkg/compiler/lib/src/parser...
pkg/compiler/lib/src/parser/element_listener.dart:71: class ParserOptions {
On 2016/02/23 08:05:26, ahe wrote:
> Is this class really needed?

Here's how I concluded that this class was needed:

In several locations a sub-parser is created (partial_elements.dart line 433 and
500, diet_parser_task.dart line 46, etc.). Such a sub-parser will need the same
customization as existing parsers (for instance, a `ClassElementParser` should
support generic methods if it is created in a situation where generic methods
are supported in general), but no information is available in certain of these
locations (e.g., sub-parsers are created outside the scope of any existing
parser in 'partial_elements.dart', so there is no `enableGenericMethods` field
in scope).

I made an attempt to pass the required information along in arguments, but this
causes arguments to be threaded through listeners and through lots of
`parseNode` methods, and there is seemingly no end to the places where extra
arguments have to be added.

In contrast, extending `Parsing` with the parser option information solves the
problem completely and concisely. In line with the `scannerOptions` of
`Parsing`, I felt that the most consistent approach would be to let it have a
similar set of `parserOptions`, and consequently the `ScannerOptions` class gets
a `ParserOptions` sibling.

I believe that future parser options could easily and comprehensibly be passed
around using this setup (so I preferred that over an alternative like having
`enableGenericMethods` as a field directly in `Parsing`).

An alternative worth considering would of course be to unify `ParserOptions` and
`ScannerOptions` and keep all those syntax related options in a single
`syntaxOptions` of type `SyntaxOptions`.

https://codereview.chromium.org/1723443003/diff/1/pkg/compiler/lib/src/parser...
File pkg/compiler/lib/src/parser/parser.dart (right):

https://codereview.chromium.org/1723443003/diff/1/pkg/compiler/lib/src/parser...
pkg/compiler/lib/src/parser/parser.dart:537: // sublanguage is so tiny:
On 2016/02/23 08:05:27, ahe wrote:
> The scanner should already have computed this for you. See:
> 
> ArrayBasedScanner.appendGtGt

Ah, yes, I hadn't looked at that.

> For example, in the case `foo(a < 0, b >> 2)`, the token representing `<`,
will
> have endGroup null, and thus cannot be a type argument.

But in `foo(a<B, C<D, E>>(2))`, the second '<' will have `endGroup` null, so I
cannot immediately use `endGroup == null` to determine whether or not a given
'<' matches any '>'. I would then declare "Nope,  `<B, C<D, E>>` is not a
`typeArguments`" and give up, incorrectly.

> 
> In the case, `foo(a < 0, b > 2)`, again you don't need to count the angle
> brackets. All you need to do is look at endGroup.next.

But even if I do confirm at '<' that there is a matching '>', it does not help
me much. Example: with `foo(a < b, 2 > c)` we need to detect that the matching
'>' occurs after '2', because this means that `< b, 2 >` could not be a
`typeArguments`, whereas `<B, C>` in `foo(a<B, C>(d))` should be accepted. The
point is that we need to check that we have a '<'/'>'-balanced token sequence
consisting of allowed tokens (and we haven't yet seen the counter-example '2'
when we see '<', so we must introduce accounting like "make sure that we step
far enough to have seen the matching '>' for any '<' seen so far" as well as
"make sure we don't have any non-allowed tokens like '2' during this stepping
process".

I'm afraid it would be more complex to take into account that '<' has a matching
'>' and then do all the accounting to make sure that we can detect the
difference between these two examples.

In any case, we cannot just "jump to the end" of anything, because we need to
confirm, token by token, that we are looking at a sequence of
type-arguments-compatible tokens.

I'll think about it, but I cannot immediately see how the `endGroup` info can be
of much help here.

> 
> Once you take advantage of the fact that you can look ahead to see what comes
> after the closing '>', you should be able to recursively answer this question
> which should be simpler and let you remove the TODO. In addition, you'll be
> making it easier to actually create the AST nodes required long term.

Certainly, we'll need to create the ASTs very soon, but that won't solve this
problem: What we are doing here is checking at the point where the potential
`typeArguments` begins that it does indeed parse correctly as a `typeArguments`,
such that we can call `parseTypeArguments`; if it does not parse correctly then
we just back up and parse it as a binary expression ('<' as "less-than").

There is no finite limit on the amount of lookahead we need in order to do this,
so we need to use something like `isValidTypeArguments` which is able to peek an
unlimited number of tokens into the future. We could of course use a recursive
descent approach (as we would surely have to do if we were to implement
something like `isValidMethod` or even `isValidExpression`), but in order to do
that we'd have to find a solution to the problem that parsing can change the
token sequence ['>>'] into ['>', '>'], whereas `isValid..` cannot have
side-effects like that (I think).

https://codereview.chromium.org/1723443003/diff/1/pkg/compiler/lib/src/parser...
pkg/compiler/lib/src/parser/parser.dart:564: // Invariant: `angleBracketLevel`
is #'<' - #'>' from initial token
On 2016/02/23 08:05:27, ahe wrote:
> What does #'<' mean?

The number of '<' tokens. Added an explanation to the comment.

https://codereview.chromium.org/1723443003/diff/1/pkg/compiler/lib/src/parser...
pkg/compiler/lib/src/parser/parser.dart:565: // to `token`, both included, and
considering ['>>'] as ['>', '>'].
On 2016/02/23 08:05:27, ahe wrote:
> I don't think ['>>'] is a valid reference.

Ah, of course not. It is intended to be the sequence of tokens of length one
whose only element is '>>'. Rephrased to use words.

https://codereview.chromium.org/1723443003/diff/1/pkg/compiler/lib/src/serial...
File pkg/compiler/lib/src/serialization/modelz.dart (right):

https://codereview.chromium.org/1723443003/diff/1/pkg/compiler/lib/src/serial...
pkg/compiler/lib/src/serialization/modelz.dart:857: bool
implementsFunction(CoreClasses coreClasses) {
On 2016/02/22 18:40:27, floitsch wrote:
> On 2016/02/22 18:02:51, eernst wrote:
> > On 2016/02/22 12:45:20, floitsch wrote:
> > > How is this change related to generic methods?
> > 
> > It is unrelated. The IDE complained about the type of this overriding method
> > declaration. The argument is unused here, and there is no obvious reason why
> the
> > argument type should differ from the inherited one. So I fixed it,
considering
> > it to be a typo, in order to make the IDE happy (and, presumably, avoid a
> > potential checked-mode error that makes no sense anyway).
> > 
> > Should it be a separate CL?
> 
> https://codereview.chromium.org/1723653002

OK, reverted it here.

https://codereview.chromium.org/1723443003/diff/1/tests/language/language.status
File tests/language/language.status (right):

https://codereview.chromium.org/1723443003/diff/1/tests/language/language.sta...
tests/language/language.status:176: generic_methods_test: CompiletimeError # Not
yet implemented.
On 2016/02/22 18:02:51, eernst wrote:
> On 2016/02/22 12:45:20, floitsch wrote:
> > Failing tests should have an issue number. If it doesn't exist yet, create
> one.
> > 
> > (same for the analyzer file).
> 
> Done, #25835.

Oops, that was in 'language_dart2js.status'; in 'language.status' it is
currently not fixed, I need to find out how to create the issue corresponding to
all the relevant targets, and where.

$compiler == none: sdk, area-vm
$compiler == precompiler: ?
$compiler == dart2app: ?
How about all the `$runtime == ..` cases?

https://codereview.chromium.org/1723443003/diff/1/tests/language/language_ana...
File tests/language/language_analyzer2.status (right):

https://codereview.chromium.org/1723443003/diff/1/tests/language/language_ana...
tests/language/language_analyzer2.status:525: generic_functions_test:
CompileTimeError # Not yet implemented.
On 2016/02/22 18:40:27, floitsch wrote:
> On 2016/02/22 18:02:51, eernst wrote:
> > On 2016/02/22 12:45:20, floitsch wrote:
> > > The analyzer already supports generic methods. not sure if they expose a
> flag,
> > > though.
> > 
> > Haven't been able to spot a suitable issue number for the analyzer. Sent
mail
> to
> > Brian in order to learn more about the possible flag and the issue number.
> 
> The options file supports it:
> $ cat /tmp/options
> analyzer:
>   language:
>     enableGenericMethods: true
> 
> $ dartanalyzer --options=/tmp/options gen_methods.dart
> 
> Note, that the analyzer needs to change its behavior (or provide a different
> flag), since it currently does semantic checks too.
> The syntactic parser exists, though.

Using that, checking that it does not produce any diagnostics.

We do fail to test the cases where programs are syntactically correct but fail
during static semantic checking, but that seems
to be an unimportant case: Checking that a semantically correct program is also
syntactically correct should be enough, and it is the only combination which
remains interesting also when the implementation has been made more complete.

[eernst, 2016-02-23 16:50:03]

On 2016/02/22 19:18:03, Leaf wrote:
> Great to see this happening!  A couple of comments.
> 
> Gilad and I hashed out some changes that he wanted to see to the DEP last
summer
> that expand the scope a bit beyond what I originally wrote up in the DEP. You
> can see a draft of these in terms of the spec here:
> 
> https://codereview.chromium.org/1177073002/
> 
> I will update the DEP writeup to reflect these changes to avoid confusion and
to
> provide a more textual reference since there is now active work on this.  
> 
> The notable change from a syntactic standpoint is that with first class
generic
> functions there are a few more syntactic forms to support. 
> 
> 1) Generic function expressions (which weren't useful in my original
> formulation) are supported in the draft spec, as ```<T0 extends B0, ..., Tn
> extends Bn> f```, where f is a standard (non-generic) function expression.
> 
> 2) Generic function typed parameters are now allowed e.g.:  
>   int applyId(T id<T>(T x)) => id<int>(3);
> 
> Note that local generic functions continue to be supported as well (not sure
if
> those are covered by this CL or not).

Very good! - I'm currently looking into
https://codereview.chromium.org/1177073002.

I'd want to make this happen in steps (rather than big-bang), so the extra
syntactic forms won't be added in this CL, but it's probably a good idea
to get all the syntactic forms working before I take the next step (which is
to tell the rest of the compiler that it must consider all the type variables
introduced by generic methods as `dynamic`).

[eernst, 2016-02-23 16:54:23]

On 2016/02/23 08:05:27, ahe wrote:
> Some of the information contained in this CL's description belongs as comments
> in the code, in particular the paragraph on "The well-known syntactic
conflict".
> 
> Generally, descriptions should not be so long, and I suggest you go through
each
> paragraph and decide where it belongs as a comment in code. Some of the
> paragraphs, like the one on changes to status files is more a note to your
> reviewers. So it belongs in the "CL message". Same applies to paragraph on
> "positive tests".

Makes sense, will do that tomorrow.

> Not creating listener actions is problematic, as it will make it harder to
> support creating AST nodes, which I assume is the long term goal. As far as I
> can tell, you'll have to rewrite this approach completely to support AST
nodes,
> and I see little advantage of not completing this work up front. In
particular,
> I think adding the listeners will address the TODO in the parse method.

All true. I just wanted to get started with something that behaves correctly.
Next step will be to learn how listeners work, and how to create AST nodes.
Presumably, it should be trivial to make the rest of the compiler ignore the
added AST nodes, because it is typed and hence no code will make any attempts
to access them; but we'll see. ;-)

> Please reach out to me on a video call, and I'll be happy to walk you through
my
> concerns (and proposed solution) in more details.

Sounds good! -- will do that tomorrow, too (if you're still wfh).

[eernst, 2016-02-24 11:02:46]

About moving parts of the CL description into the code as comments:

Any permanent software documentation should describe the whole, not
the increment, so information a la "this CL adds.." seems to fit better
in the CL description. It is up to the spec to describe the syntactic
constructs in the language as a whole. That's the first paragraph.

The paragraphs about negative tests and status files should have been
in the CL message (I never really knew how to distinguish the CL
description and the 'Publish+Mail' text field, but this would be one
useful hint about that ;-).

The remaining paragraphs seem to fit as documentation, possibly as
comments in 'parser.dart'. Issues to consider:

  - A few methods in 'parser.dart' have dartdoc comments, e.g.,
indicating the syntactic form (e.g., `/// library qualified ';'`); the
only exception is `findMemberName` which has a longer explanation.

  - The dartdoc comment on `Parser` describes the main concepts
(`parse..` and `peek..` methods, listeners, `Scanner`), but no location
in the file seems to comment on finer details concerned with more than
one method. Nevertheless, this dartdoc comment is the closest we get.

So I put two of the last three paragraphs into the `Parser` dartdoc 
comment and adjusted them. They break the style there, but I cannot find
any other place where they don't do that.

[eernst, 2016-02-24 11:05:35]

Will rebase this CL unto the current tip of the master branch,
retest on `language`, and land it.

[floitsch, 2016-02-24 14:09:09]

On 2016/02/24 11:05:35, eernst wrote:
> Will rebase this CL unto the current tip of the master branch,
> retest on `language`, and land it.

Given that you made significant changes, please give reviewers the time to have
a look before committing.
There is no rush.

[ahe, 2016-02-24 14:13:22]

I don't think this CL should be landed yet.

[eernst, 2016-02-24 14:50:58]

On 2016/02/24 14:13:22, ahe wrote:
> I don't think this CL should be landed yet.

OK, will return to it on Tuesday when I'm back from the ECOOP PC meeting.

[Johnni Winther, 2016-02-29 10:18:45]

Initial comments.

https://codereview.chromium.org/1723443003/diff/60001/pkg/compiler/lib/src/co...
File pkg/compiler/lib/src/compiler.dart (right):

https://codereview.chromium.org/1723443003/diff/60001/pkg/compiler/lib/src/co...
pkg/compiler/lib/src/compiler.dart:526: enableGenericMethods:
enableGenericMethods),
Pass [parserOptions] instead. (Also below)

https://codereview.chromium.org/1723443003/diff/60001/pkg/compiler/lib/src/pa...
File pkg/compiler/lib/src/parser/class_element_parser.dart (right):

https://codereview.chromium.org/1723443003/diff/60001/pkg/compiler/lib/src/pa...
pkg/compiler/lib/src/parser/class_element_parser.dart:18: bool
enableGenericMethods: false})
Replace with one [ParserOptions] argument.

https://codereview.chromium.org/1723443003/diff/60001/pkg/compiler/lib/src/pa...
File pkg/compiler/lib/src/parser/diet_parser_task.dart (right):

https://codereview.chromium.org/1723443003/diff/60001/pkg/compiler/lib/src/pa...
pkg/compiler/lib/src/parser/diet_parser_task.dart:30: {bool
enableConditionalDirectives: false,
Ditto

https://codereview.chromium.org/1723443003/diff/60001/pkg/compiler/lib/src/pa...
File pkg/compiler/lib/src/parser/parser.dart (right):

https://codereview.chromium.org/1723443003/diff/60001/pkg/compiler/lib/src/pa...
pkg/compiler/lib/src/parser/parser.dart:577: if (optional('<', token)) {
Negate this to avoid the else branch:

if (!optional('<', token)) {
  return false;
}

int angleBracketLevel = 1....

https://codereview.chromium.org/1723443003/diff/60001/pkg/compiler/lib/src/pa...
File pkg/compiler/lib/src/parser/parser_task.dart (right):

https://codereview.chromium.org/1723443003/diff/60001/pkg/compiler/lib/src/pa...
pkg/compiler/lib/src/parser/parser_task.dart:34: bool enableGenericMethods:
false})
Use one [ParserOptions] argument instead.

[eernst, 2016-03-09 16:27:13]

Coming back to this, I've looked at the comments I received so far and
I've extracted a number of issues that need to be addressed in order to
make some progress on this.

Executive summary:
  - adjustments in Johnni's review done
  - placement of comment on `Parser` class needs resolution
  - other tasks mentioned below can be done in future CLs

Details:

  - there is a missing test file containing syntactically correct, but
    semantically wrong constructs (originally planned for a future CL)

  - there is a controversy about using `parseAndIgnoreX` methods; it does
    cause some code duplication, but I did it that way because it was a
    very non-invasive way to parse and ignore certain constructs, and
    because it is very easy to eliminate these methods and call the plain
    `parseX` methods at the next step (where listener events will be
    generated and a bigger AST created, with the new parts ignored)

  - the testing situation is unclear, because a run of `test-all.sh` in the
    branch 'master' produces 22 internal errors; I can try to recognize
    that I get exactly 22 every time, but I got the impression that there
    shouldn't be any.

  - the method `isValidTypeArguments` is used to recognize that the
    rest of the program some times cannot possibly be a valid actual type
    argument list, which means that we can proceed and make the other
    choice in a parsing method. There are cases where this method will
    return true even though it does not look at a valid actual type
    argument list; I have made the claim in the code that this will
    only be true for wrong programs (so we won't start parsing something
    as an actual type argument list when it is actually a correct
    construct of some other kind, it will always be wrong). This hasn't
    been proven. The alternative is to use general parsing (and ignoring)
    for `isValidTypeArguments`, but that would potentially be much more
    costly because of the potential back-tracking.

  - Leaf mentioned generic function literals and generic function type
    annotations in https://codereview.chromium.org/1177073002/, and they
    are not yet covered by this CL. Also, the spec language on polymorphic
    methods discussed there seems to imply that it must be possible to
    call any non-generic method with arbitrary type arguments; this
    conflicts with several things that we have discussed so far, including
    the named parameter based implementation strategy. Finally, Leaf
    points out the difficulty of handling universal types as actual type
    arguments, and argues that they should be coerced into `dynamic`.
    None of these things require any actions right now, for this CL, but
    the do point out new syntax: function literals and types.

  - The "broad" comments (pertaining to more than one declaration) which
    were moved from the CL description to the source code (parser.dart)
    are not well-placed in the class dartdoc comment. Need to find a
    reasonable placement.

  - Florian created multiple issues, but it is not obvious to me whether
    it already covers, say, dart2app. (Re: 'Need to clarify how to create
    the right issues for the vm, precompiler, dart2app, and possible more.')

None of these tasks must be completed in order to reach a point where tests
succeed to the same degree as on branch 'master'.

Presumably, the next task would be to eliminate the `parseAndIgnoreX`
methods and achieve the same result via modified listeners, or to support
the remaining syntactic forms.

Submission of an issue for `dart2app` may be needed, I don't know what
that is.

[eernst, 2016-03-09 16:28:13]

Review response

https://codereview.chromium.org/1723443003/diff/60001/pkg/compiler/lib/src/co...
File pkg/compiler/lib/src/compiler.dart (right):

https://codereview.chromium.org/1723443003/diff/60001/pkg/compiler/lib/src/co...
pkg/compiler/lib/src/compiler.dart:526: enableGenericMethods:
enableGenericMethods),
On 2016/02/29 10:18:45, Johnni Winther wrote:
> Pass [parserOptions] instead. (Also below)

Right, it is clearly better to keep multiple options together in a single
options object, so this means that it will replace both `enable..` arguments.
The `enableConditionalDirectives` argument is false by default here, and the
same holds for the only call site, so it should not change the semantics. The
`parserOptions` argument will be mandatory, such that we avoid the
complexity/redundancy of building a default one as well as providing default
values for each of its fields.

Done.

https://codereview.chromium.org/1723443003/diff/60001/pkg/compiler/lib/src/pa...
File pkg/compiler/lib/src/parser/class_element_parser.dart (right):

https://codereview.chromium.org/1723443003/diff/60001/pkg/compiler/lib/src/pa...
pkg/compiler/lib/src/parser/class_element_parser.dart:18: bool
enableGenericMethods: false})
On 2016/02/29 10:18:45, Johnni Winther wrote:
> Replace with one [ParserOptions] argument.

Done: Same treatment as in 'compiler.dart'.

https://codereview.chromium.org/1723443003/diff/60001/pkg/compiler/lib/src/pa...
File pkg/compiler/lib/src/parser/diet_parser_task.dart (right):

https://codereview.chromium.org/1723443003/diff/60001/pkg/compiler/lib/src/pa...
pkg/compiler/lib/src/parser/diet_parser_task.dart:30: {bool
enableConditionalDirectives: false,
On 2016/02/29 10:18:45, Johnni Winther wrote:
> Ditto

Done, as described in comment for 'compiler.dart'.

https://codereview.chromium.org/1723443003/diff/60001/pkg/compiler/lib/src/pa...
File pkg/compiler/lib/src/parser/parser.dart (right):

https://codereview.chromium.org/1723443003/diff/60001/pkg/compiler/lib/src/pa...
pkg/compiler/lib/src/parser/parser.dart:577: if (optional('<', token)) {
On 2016/02/29 10:18:45, Johnni Winther wrote:
> Negate this to avoid the else branch:
> 
> if (!optional('<', token)) {
>   return false;
> }
> 
> int angleBracketLevel = 1....

Done.

https://codereview.chromium.org/1723443003/diff/60001/pkg/compiler/lib/src/pa...
File pkg/compiler/lib/src/parser/parser_task.dart (right):

https://codereview.chromium.org/1723443003/diff/60001/pkg/compiler/lib/src/pa...
pkg/compiler/lib/src/parser/parser_task.dart:34: bool enableGenericMethods:
false})
On 2016/02/29 10:18:45, Johnni Winther wrote:
> Use one [ParserOptions] argument instead.

Done, as described in comment on 'compiler.dart'.

[ahe, 2016-03-09 16:45:15]

Not lgtm. Please address my concerns and/or reach out to me for details.

[eernst, 2016-03-09 16:50:50]

On 2016/03/09 16:45:15, ahe wrote:
> Not lgtm. Please address my concerns and/or reach out to me for details.

Let's discuss it irl.

[eernst, 2016-03-10 13:54:25]

Peter convinced me that a recursive approach to `isValidTypeArguments`
doesn't have to incur an O(n^2) cost, so I can get rid of the state
machine and use a style that fits much better into the parser.

I also learned how the `parseAndIgnore..` methods can be eliminated using
rather small adjustments to the listeners.

He finally suggested a number of ways to cut this CL into smaller parts.
I will abandon this CL and create several new CLs in its place:

  - Add tests and adjust status files.

  - Introduce `ParserOptions` and pass it along with `Parsing`.

  - Reimplement the given parsing extensions (with modified listeners,
    not `parseAndIgnore..`) using the previous two.

Things we discussed which are not present in this CL are coming after
that, e.g., add the remaining parsing extensions that Leaf mentioned.

[eernst, 2016-04-08 11:17:37]

Closing this CL --- all reusable elements have been transferred to newer CL's
doing similar things one thing at a time. (So this one will never be landed).

[eernst, 2016-04-08 11:17:49]

Description was changed from

==========
First step of support for parsing and ignoring generic methods.

This CL introduces support for generic method syntax: It is recognized
by the parser but no listener actions are invoked during that parsing
process. The following constructs have been added: type parameters can
be declared on function and method declarations, with the same syntax
as type parameters on class declarations, and type arguments can be
delivered in message send constructs.

Positive tests have been added, showing a range of different correct
usages. (Negative tests will be added in a follow-up CL).

Status files have been adjusted to indicate that the new tests are
`CompileTimeError`s in all cases (compilers + analyzer). If the
`--enable-generic-methods` flag is passed, `dartj2s` will not have
a compile time error.

Relative to the DEP #22 proposal, there is no support for currying
invocations (that is, we cannot use `myMethod<T,S>` as an closure
which is non-generic and which will pass the actual values of `T`
and `S` in the given context as type arguments).

The well-known syntactic conflict associated with relative expressions
is resolved in favor of generic invocations (for instance, with
`foo(m<B, C>(3))` we parse it as an invocation of `m` with type
arguments `B, C` and value argument `3`, not as two boolean
arguments passed to `foo`. It is expected that this situation is
unlikely to occur often in practice, especially because a comma or
')' will delimit the expression where we have `3` in the example,
which means that it won't need to be wrapped in parentheses. Of
course, the work-around `(C>(3))` is available if it should still
occur.

Neither the proposal in DEP #22 nor this CL adds any support for type
parameterization of literal functions (i.e., expressions like
`(x) => x`).
==========

to

==========
First step of support for parsing and ignoring generic methods.

This CL introduces support for generic method syntax: It is recognized
by the parser but no listener actions are invoked during that parsing
process. The following constructs have been added: type parameters can
be declared on function and method declarations, with the same syntax
as type parameters on class declarations, and type arguments can be
delivered in message send constructs.

Positive tests have been added, showing a range of different correct
usages. (Negative tests will be added in a follow-up CL).

Status files have been adjusted to indicate that the new tests are
`CompileTimeError`s in all cases (compilers + analyzer). If the
`--enable-generic-methods` flag is passed, `dartj2s` will not have
a compile time error.

Relative to the DEP #22 proposal, there is no support for currying
invocations (that is, we cannot use `myMethod<T,S>` as an closure
which is non-generic and which will pass the actual values of `T`
and `S` in the given context as type arguments).

The well-known syntactic conflict associated with relative expressions
is resolved in favor of generic invocations (for instance, with
`foo(m<B, C>(3))` we parse it as an invocation of `m` with type
arguments `B, C` and value argument `3`, not as two boolean
arguments passed to `foo`. It is expected that this situation is
unlikely to occur often in practice, especially because a comma or
')' will delimit the expression where we have `3` in the example,
which means that it won't need to be wrapped in parentheses. Of
course, the work-around `(C>(3))` is available if it should still
occur.

Neither the proposal in DEP #22 nor this CL adds any support for type
parameterization of literal functions (i.e., expressions like
`(x) => x`).
==========