Add support for the new function-type syntax.

Add support for the new function-type syntax.

Fixes #27967
BUG= http://dartbug.com/27967
R=ahe@google.com, johnniwinther@google.com, paulberry@google.com, sigmund@google.com

Committed: https://github.com/dart-lang/sdk/commit/85227ba8a60332a18ae92cb1b87bf15d52a909bd
Reverted: https://github.com/dart-lang/sdk/commit/f5fc210f4f79f7e6b7ccefc24e7565e810e9dc7d
Committed: https://github.com/dart-lang/sdk/commit/e4999a631093690febb2eb90cbdbe1f9d5183455
Reverted: https://github.com/dart-lang/sdk/commit/c64c35153905fc2bf8a74c02297220385cfcbc87
Committed: https://github.com/dart-lang/sdk/commit/c289af39c36d9a5f7130a3e69b7891052652a64e
Reverted: https://github.com/dart-lang/sdk/commit/c66479f40767744d0bdd027c004583d6e32fe50f
Committed: https://github.com/dart-lang/sdk/commit/979183decfa3770236fe8f8525d0ad9e23f5acfb

[floitsch, 2016-12-12 19:05:52]

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

[floitsch, 2016-12-12 19:05:52]

This is still WIP, but I would appreciate feedback.

1. where should I put the methods. For now it's a bit random.
2. how should I fix the `Function` detection? Should I keep using a
string-comparison?
3. any suggestions for the name of the `NewFunctionType` class?
4. are you ok with allocating the `NewFunctionType` and then discarding it, to
fill the `Typedef` node (with the `isNewSyntax` flag on)?
5. Suppose I have the function type: `Function(String)`. Any tips on how to
resolve the formals-list `(String)` to `String` (and not the identifier
`String`). Should I introduce a synthetic identifier to have a `TypeAnnotation`
and be uniform?

[Siggi Cherem (dart-lang), 2016-12-12 23:20:39]

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

[Siggi Cherem (dart-lang), 2016-12-12 23:20:40]

> 1. where should I put the methods. For now it's a bit random.

so far, nothing stands out as out of place. 

> 2. how should I fix the `Function` detection? Should I keep using a
string-comparison?

seems fine for now, eventually should we make it a pseudo in the future? Maybe
Johhni/Peter have a better answer.

> 3. any suggestions for the name of the `NewFunctionType` class?

How about `FunctionTypeDeclaration`?

> 4. are you ok with allocating the `NewFunctionType` and then discarding it, to
fill the `Typedef` node (with the `isNewSyntax` flag on)?

Currently since it's only used in typedefs, you could however not pop the stack
until endTypeAlias (and that way skip the allocation). In the future, though, I
expect FunctionTypeDeclarations to appear in other contexts, so I'm OK
allocating the extra node.

Another idea: make it a FunctionTypeDeclarationBody that you store as a member
of a typedef (I'd do this if you see it possible to convert the old syntax into
the new structure).


> 5. Suppose I have the function type: `Function(String)`. Any tips on how to
resolve the formals-list `(String)` to `String` (and not the identifier
`String`). Should I introduce a synthetic identifier to have a `TypeAnnotation`
and be uniform?

Good question - how much of SignatureResolver.analyzer are we reusing? Would it
be better to have separate logic just for type-defs? (a lot of checks there seem
to only apply to functionExpressions)


If it makes sense to specialize the logic, I'd prefer not introducing the extra
synthetic nodes.

Another thought... do we actually use the identifiers in anywhere with the old
syntax? Would it be possible to change the old typedef to get rid of the names?
... we might have to use them for error reporting though, so I'm not sure this
is viable.

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

https://codereview.chromium.org/2567133002/diff/20001/pkg/compiler/lib/src/pa...
pkg/compiler/lib/src/parser/parser.dart:378:
listener.endFunctionTypeAlias(typedefKeyword, token);
a suggestion to make this match closer to what the parser does in a few other
places:
 - keep a single begin/end for FunctionTypeAlias
 - add an argument to the `endFunctionTypeAlias` (and maybe to the begin as
well) that passes in the '=' token.

Listeners can then make the code behave differently depending on that.


(We do this in a bunch of places, see for example `endForIn` how it decides what
to do depending on the presence of the `await` token).

https://codereview.chromium.org/2567133002/diff/20001/pkg/compiler/lib/src/pa...
pkg/compiler/lib/src/parser/parser.dart:830: // is currently not set up to deal
with upper-case characters.
can this count as a reason to switch to `fun` or `function` or `<-`?

;-)

https://codereview.chromium.org/2567133002/diff/20001/pkg/compiler/lib/src/re...
File pkg/compiler/lib/src/resolution/signatures.dart (right):

https://codereview.chromium.org/2567133002/diff/20001/pkg/compiler/lib/src/re...
pkg/compiler/lib/src/resolution/signatures.dart:305: bool isNewSyntax: false,
while you are here, would you mind updating the dartdoc above. I would had
assumed that this was only used for function expressions and not for typedefs.

https://codereview.chromium.org/2567133002/diff/20001/pkg/compiler/lib/src/tr...
File pkg/compiler/lib/src/tree/nodes.dart (right):

https://codereview.chromium.org/2567133002/diff/20001/pkg/compiler/lib/src/tr...
pkg/compiler/lib/src/tree/nodes.dart:2855: final bool isNewSyntax;
I'd avoid terms like `new` and `old`, since in a few months it wont be as new :)

suggestion: isGeneralizedTypeAlias?

https://codereview.chromium.org/2567133002/diff/20001/pkg/compiler/lib/src/tr...
pkg/compiler/lib/src/tree/nodes.dart:2893: // TODO(floitsch): find a better
name. "FunctionType" is already taken by
how about FunctionTypeDeclaration ?

[floitsch, 2016-12-29 19:57:47]

PTAL.

I'm not really proud of the hack to get type-arguments in scope, but there was
just no easy way to create the corresponding elements.

At the moment the new syntax can be used everywhere. In theory that should be
behind a flag...

I uploaded the two versions of the generated tests. The typedef-only version is
in patch set 10. The full suite is in patch set 11.

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

https://codereview.chromium.org/2567133002/diff/20001/pkg/compiler/lib/src/pa...
pkg/compiler/lib/src/parser/parser.dart:378:
listener.endFunctionTypeAlias(typedefKeyword, token);
On 2016/12/12 23:20:40, Siggi Cherem (dart-lang) wrote:
> a suggestion to make this match closer to what the parser does in a few other
> places:
>  - keep a single begin/end for FunctionTypeAlias
>  - add an argument to the `endFunctionTypeAlias` (and maybe to the begin as
> well) that passes in the '=' token.
> 
> Listeners can then make the code behave differently depending on that.
> 
> 
> (We do this in a bunch of places, see for example `endForIn` how it decides
what
> to do depending on the presence of the `await` token).

Done.

https://codereview.chromium.org/2567133002/diff/20001/pkg/compiler/lib/src/pa...
pkg/compiler/lib/src/parser/parser.dart:830: // is currently not set up to deal
with upper-case characters.
On 2016/12/12 23:20:40, Siggi Cherem (dart-lang) wrote:
> can this count as a reason to switch to `fun` or `function` or `<-`?
> 
> ;-)

nice try ;)

[Siggi Cherem (dart-lang), 2016-12-29 22:46:58]

I like the changes so far. The new approach adding an argument to endTypedef in
the parser seems to work well.

I wrote more detailed comments below, but in summary these are my general
questions:
 - can we do something about the scopes before landing this (Johnni might have
some suggestions)?
 - alternatives to `NoIdentifier`?
 - upload the generated tests?

https://codereview.chromium.org/2567133002/diff/200001/pkg/compiler/lib/src/p...
File pkg/compiler/lib/src/parser/parser.dart (right):

https://codereview.chromium.org/2567133002/diff/200001/pkg/compiler/lib/src/p...
pkg/compiler/lib/src/parser/parser.dart:476: //    int Function(int bar(String
x)).
should we report an error instead in that case?

(e.g. check that the next token is not '(' or '<' when inFunctionType is true)?

https://codereview.chromium.org/2567133002/diff/200001/pkg/compiler/lib/src/p...
pkg/compiler/lib/src/parser/parser.dart:864: if
(isGeneralizedFunctionType(token)) {
is this branch ever executed?

https://codereview.chromium.org/2567133002/diff/200001/pkg/compiler/lib/src/p...
pkg/compiler/lib/src/parser/parser.dart:1389: peek = token.next;
suggestion: consider introducing 2 functions to make it simpler to follow:
 - peekAfterNominalType (for what used to be peekAfterType)
 - peekAfterFunctionType (for the new code)

https://codereview.chromium.org/2567133002/diff/200001/pkg/compiler/lib/src/p...
pkg/compiler/lib/src/parser/parser.dart:1411: if
(isGeneralizedFunctionType(peek)) {
do we need a loop here, like you had in parseType?

If so, then let's definitely add the helper functions. I imagine the code would
look more like:

Token peekAfterType(Token token) {
  Token peek = !isGeneralizedFunctionType(token)
      ? peekAfterNominalType(token)
      : token;

  // We are looking at "type Function ..."
  while (isGeneralizedFunctionType(peek)) {
    peek = peekAfterFunctionType(peek);
  }

  return peek;

https://codereview.chromium.org/2567133002/diff/200001/pkg/compiler/lib/src/p...
pkg/compiler/lib/src/parser/parser.dart:1412: peek = peek.next;  // Skip over
the Function.
nit: reword "the Function" to "the `Function` token"

https://codereview.chromium.org/2567133002/diff/200001/pkg/compiler/lib/src/p...
pkg/compiler/lib/src/parser/parser.dart:1413: if (identical(peek.kind,
LT_TOKEN)) {
consider adding here and below comments describing what we are looking at
(similar to those above):

  // We are looking at "type Function '<' ... '>' ..."
  // We are looking at "type Function type-args '(' ... ')' ..."

https://codereview.chromium.org/2567133002/diff/200001/pkg/compiler/lib/src/p...
pkg/compiler/lib/src/parser/parser.dart:1973:
isGeneralizedFunctionType(identifier)) {
why do we need this case? could this be handled by the changes to peekAfterType?

https://codereview.chromium.org/2567133002/diff/200001/pkg/compiler/lib/src/r...
File pkg/compiler/lib/src/resolution/class_hierarchy.dart (right):

https://codereview.chromium.org/2567133002/diff/200001/pkg/compiler/lib/src/r...
pkg/compiler/lib/src/resolution/class_hierarchy.dart:444: DartType
resolveSupertype(ClassElement cls, NominalTypeAnnotation superclass) {
long line (we just run dartfmt before submitting)

https://codereview.chromium.org/2567133002/diff/200001/pkg/compiler/lib/src/r...
File pkg/compiler/lib/src/resolution/signatures.dart (right):

https://codereview.chromium.org/2567133002/diff/200001/pkg/compiler/lib/src/r...
pkg/compiler/lib/src/resolution/signatures.dart:97: FormalElementX element =
definition == new NoIdentifier()
nit: this is more of a readability nit, than anything else. I understand that
the `new NoIdentifier` is returning a singleton instance, but seeing the `new`
here does make you think twice about what this code is doing.

Other options:
  - definition == NoIdentifier.singleton
  - definition.isNoIdentifier // or .isIdentifierPlaceholder
  - definition == null ?

https://codereview.chromium.org/2567133002/diff/200001/pkg/compiler/lib/src/r...
File pkg/compiler/lib/src/resolution/type_resolver.dart (right):

https://codereview.chromium.org/2567133002/diff/200001/pkg/compiler/lib/src/r...
pkg/compiler/lib/src/resolution/type_resolver.dart:294: // to dynamic.
oh yikes - this does feel very hackish.

Can you tell me more about what you run into? From your earlier comment it
appears like you tried creating a scope for the type variables but it wasn't
easy to create the actual TypeVariableElements for them?

https://codereview.chromium.org/2567133002/diff/200001/pkg/compiler/lib/src/t...
File pkg/compiler/lib/src/tree/nodes.dart (right):

https://codereview.chromium.org/2567133002/diff/200001/pkg/compiler/lib/src/t...
pkg/compiler/lib/src/tree/nodes.dart:1774: final Node typeName;
Any reason this changed to Node?

https://codereview.chromium.org/2567133002/diff/200001/pkg/compiler/lib/src/t...
pkg/compiler/lib/src/tree/nodes.dart:2917: final NodeList templateParameters;
nit: +dartdoc, I'm not sure what is the distinction here between
templateParameters and typeParameters

https://codereview.chromium.org/2567133002/diff/200001/tests/language/general...
File tests/language/generalized_function_type_test.dart (right):

https://codereview.chromium.org/2567133002/diff/200001/tests/language/general...
tests/language/generalized_function_type_test.dart:5: import
'generated_function_syntax_tests.dart';
missing `git add` for this file?

https://codereview.chromium.org/2567133002/diff/200001/tests/language/generic...
File tests/language/generic_function_type_test.dart (right):

https://codereview.chromium.org/2567133002/diff/200001/tests/language/generic...
tests/language/generic_function_type_test.dart:9: try {
should we add this to package:expect? I've seen it a few times.

Any reason not to use the pattern you had in the generated tests?

  isCheckedMode() {
    var result = false;
    assert(result = true);
    return result;
  }

https://codereview.chromium.org/2567133002/diff/200001/tests/language/generic...
tests/language/generic_function_type_test.dart:36: Expect.isTrue(got_type_error
== isCheckedMode());
is it important to make the check with check mode, or would it be sufficient to
test this with an `is` check? i.e.:

  Expect.isFalse(f() is FListInt);

https://codereview.chromium.org/2567133002/diff/200001/tests/language/generic...
File tests/language/generic_function_typedef2_test.dart (right):

https://codereview.chromium.org/2567133002/diff/200001/tests/language/generic...
tests/language/generic_function_typedef2_test.dart:12: typedef G = F; /// 00:
compile-time error
I expect most of these compile-time errors will be OK in the future when we get
generalized type aliases, correct?

[floitsch, 2016-12-30 14:55:48]

Thanks for the great review.

would be nice if there was a good way to fix the scopes for types. Hopefully
Johnni has an idea. I think he mentioned that he was aware of some weirdness
there.

I could remove NoIdentifier and replace it with `null`. When developing I was
hoping that `NoIdentifier` would be more explicit and easier to get right.
The latest patch set removes `NoIdentifier` and just uses `null` instead.

Uploaded the test.

https://codereview.chromium.org/2567133002/diff/200001/pkg/compiler/lib/src/p...
File pkg/compiler/lib/src/parser/parser.dart (right):

https://codereview.chromium.org/2567133002/diff/200001/pkg/compiler/lib/src/p...
pkg/compiler/lib/src/parser/parser.dart:476: //    int Function(int bar(String
x)).
On 2016/12/29 22:46:57, Siggi Cherem (dart-lang) wrote:
> should we report an error instead in that case?
> 
> (e.g. check that the next token is not '(' or '<' when inFunctionType is
true)?

In some sense that's what's happening. Just automatically by the parser.
Example output:

Error: 'void' is a reserved word and can't be used here.
Try using a different name.
typedef F = Function(void f());
                     ^^^^
Error: Expected ')' before this.
typedef F = Function(f());
                      ^

https://codereview.chromium.org/2567133002/diff/200001/pkg/compiler/lib/src/p...
pkg/compiler/lib/src/parser/parser.dart:864: if
(isGeneralizedFunctionType(token)) {
On 2016/12/29 22:46:58, Siggi Cherem (dart-lang) wrote:
> is this branch ever executed?

Nope. that was old code that I didn't clean up.
Good catch. thanks.
done.

https://codereview.chromium.org/2567133002/diff/200001/pkg/compiler/lib/src/p...
pkg/compiler/lib/src/parser/parser.dart:1389: peek = token.next;
On 2016/12/29 22:46:58, Siggi Cherem (dart-lang) wrote:
> suggestion: consider introducing 2 functions to make it simpler to follow:
>  - peekAfterNominalType (for what used to be peekAfterType)
>  - peekAfterFunctionType (for the new code)

Done.

https://codereview.chromium.org/2567133002/diff/200001/pkg/compiler/lib/src/p...
pkg/compiler/lib/src/parser/parser.dart:1411: if
(isGeneralizedFunctionType(peek)) {
On 2016/12/29 22:46:58, Siggi Cherem (dart-lang) wrote:
> do we need a loop here, like you had in parseType?
> 
> If so, then let's definitely add the helper functions. I imagine the code
would
> look more like:
> 
> Token peekAfterType(Token token) {
>   Token peek = !isGeneralizedFunctionType(token)
>       ? peekAfterNominalType(token)
>       : token;
> 
>   // We are looking at "type Function ..."
>   while (isGeneralizedFunctionType(peek)) {
>     peek = peekAfterFunctionType(peek);
>   }
> 
>   return peek;

I'm not sure the loop is necessary, but I don't want to prove it isn't. It's
definitely cleaner and less error-prone doing the loop.
Code looks better now.
thanks.
done.

https://codereview.chromium.org/2567133002/diff/200001/pkg/compiler/lib/src/p...
pkg/compiler/lib/src/parser/parser.dart:1412: peek = peek.next;  // Skip over
the Function.
On 2016/12/29 22:46:57, Siggi Cherem (dart-lang) wrote:
> nit: reword "the Function" to "the `Function` token"

Done.

https://codereview.chromium.org/2567133002/diff/200001/pkg/compiler/lib/src/p...
pkg/compiler/lib/src/parser/parser.dart:1413: if (identical(peek.kind,
LT_TOKEN)) {
On 2016/12/29 22:46:58, Siggi Cherem (dart-lang) wrote:
> consider adding here and below comments describing what we are looking at
> (similar to those above):
> 
>   // We are looking at "type Function '<' ... '>' ..."
>   // We are looking at "type Function type-args '(' ... ')' ..."

Done.

https://codereview.chromium.org/2567133002/diff/200001/pkg/compiler/lib/src/p...
pkg/compiler/lib/src/parser/parser.dart:1973:
isGeneralizedFunctionType(identifier)) {
On 2016/12/29 22:46:58, Siggi Cherem (dart-lang) wrote:
> why do we need this case? could this be handled by the changes to
peekAfterType?

Yes. thanks for figuring this out.
I should have paid more attention when I ran into this issue in my tests.

https://codereview.chromium.org/2567133002/diff/200001/pkg/compiler/lib/src/r...
File pkg/compiler/lib/src/resolution/class_hierarchy.dart (right):

https://codereview.chromium.org/2567133002/diff/200001/pkg/compiler/lib/src/r...
pkg/compiler/lib/src/resolution/class_hierarchy.dart:444: DartType
resolveSupertype(ClassElement cls, NominalTypeAnnotation superclass) {
On 2016/12/29 22:46:57, Siggi Cherem (dart-lang) wrote:
> long line (we just run dartfmt before submitting)

done.
didn't know that dartfmt was accepted in dart2js.

https://codereview.chromium.org/2567133002/diff/200001/pkg/compiler/lib/src/r...
File pkg/compiler/lib/src/resolution/signatures.dart (right):

https://codereview.chromium.org/2567133002/diff/200001/pkg/compiler/lib/src/r...
pkg/compiler/lib/src/resolution/signatures.dart:97: FormalElementX element =
definition == new NoIdentifier()
On 2016/12/29 22:46:57, Siggi Cherem (dart-lang) wrote:
> nit: this is more of a readability nit, than anything else. I understand that
> the `new NoIdentifier` is returning a singleton instance, but seeing the `new`
> here does make you think twice about what this code is doing.
> 
> Other options:
>   - definition == NoIdentifier.singleton
>   - definition.isNoIdentifier // or .isIdentifierPlaceholder
>   - definition == null ?

changed to NoIdentifier.singleton
done.

https://codereview.chromium.org/2567133002/diff/200001/pkg/compiler/lib/src/r...
File pkg/compiler/lib/src/resolution/type_resolver.dart (right):

https://codereview.chromium.org/2567133002/diff/200001/pkg/compiler/lib/src/r...
pkg/compiler/lib/src/resolution/type_resolver.dart:294: // to dynamic.
On 2016/12/29 22:46:58, Siggi Cherem (dart-lang) wrote:
> oh yikes - this does feel very hackish.
> 
> Can you tell me more about what you run into? From your earlier comment it
> appears like you tried creating a scope for the type variables but it wasn't
> easy to create the actual TypeVariableElements for them?

Yes. The problem is, that the function type doesn't have an element, but that we
would need one as an enclosing element for the TypeVariableElement.
Whenever I tried to create one artificially, I ran into issues in other places
of the code.

Also, I wasn't really able to add scopes to the system.

https://codereview.chromium.org/2567133002/diff/200001/pkg/compiler/lib/src/t...
File pkg/compiler/lib/src/tree/nodes.dart (right):

https://codereview.chromium.org/2567133002/diff/200001/pkg/compiler/lib/src/t...
pkg/compiler/lib/src/tree/nodes.dart:1774: final Node typeName;
On 2016/12/29 22:46:58, Siggi Cherem (dart-lang) wrote:
> Any reason this changed to Node?

Old change, when I hadn't subclassed `TypeAnnotation` yet.
Changed it back.

https://codereview.chromium.org/2567133002/diff/200001/pkg/compiler/lib/src/t...
pkg/compiler/lib/src/tree/nodes.dart:2917: final NodeList templateParameters;
On 2016/12/29 22:46:58, Siggi Cherem (dart-lang) wrote:
> nit: +dartdoc, I'm not sure what is the distinction here between
> templateParameters and typeParameters

Done.

https://codereview.chromium.org/2567133002/diff/200001/tests/language/general...
File tests/language/generalized_function_type_test.dart (right):

https://codereview.chromium.org/2567133002/diff/200001/tests/language/general...
tests/language/generalized_function_type_test.dart:5: import
'generated_function_syntax_tests.dart';
On 2016/12/29 22:46:57, Siggi Cherem (dart-lang) wrote:
> missing `git add` for this file?

yes :(
done.

https://codereview.chromium.org/2567133002/diff/200001/tests/language/generic...
File tests/language/generic_function_type_test.dart (right):

https://codereview.chromium.org/2567133002/diff/200001/tests/language/generic...
tests/language/generic_function_type_test.dart:9: try {
On 2016/12/29 22:46:58, Siggi Cherem (dart-lang) wrote:
> should we add this to package:expect? I've seen it a few times.
> 
> Any reason not to use the pattern you had in the generated tests?
> 
>   isCheckedMode() {
>     var result = false;
>     assert(result = true);
>     return result;
>   }

Actually that file is an exact copy of `function_type_test.dart`.
I copied it, and wanted to change it, but then accidentally named my new test
`generalized_function_type_test.dart`.

Removed this file.

Agreed: the `isCheckedMode` should be in expect.dart. Will create a different CL
for that.

https://codereview.chromium.org/2567133002/diff/200001/tests/language/generic...
tests/language/generic_function_type_test.dart:36: Expect.isTrue(got_type_error
== isCheckedMode());
On 2016/12/29 22:46:58, Siggi Cherem (dart-lang) wrote:
> is it important to make the check with check mode, or would it be sufficient
to
> test this with an `is` check? i.e.:
> 
>   Expect.isFalse(f() is FListInt);

obsolete comment (since I deleted that file).

https://codereview.chromium.org/2567133002/diff/200001/tests/language/generic...
File tests/language/generic_function_typedef2_test.dart (right):

https://codereview.chromium.org/2567133002/diff/200001/tests/language/generic...
tests/language/generic_function_typedef2_test.dart:12: typedef G = F; /// 00:
compile-time error
On 2016/12/29 22:46:57, Siggi Cherem (dart-lang) wrote:
> I expect most of these compile-time errors will be OK in the future when we
get
> generalized type aliases, correct?

Correct. But for now they are not accepted.

[Johnni Winther, 2017-01-02 12:46:53]

https://codereview.chromium.org/2567133002/diff/240001/pkg/compiler/lib/src/k...
File pkg/compiler/lib/src/kernel/kernel_visitor.dart (right):

https://codereview.chromium.org/2567133002/diff/240001/pkg/compiler/lib/src/k...
pkg/compiler/lib/src/kernel/kernel_visitor.dart:1092: // Shouldn't be called, as
the resolver have already resolved types and
As below.

https://codereview.chromium.org/2567133002/diff/240001/pkg/compiler/lib/src/k...
pkg/compiler/lib/src/kernel/kernel_visitor.dart:1099: // Shouldn't be called, as
the resolver have already resolved types and
'have' -> 'has'

https://codereview.chromium.org/2567133002/diff/240001/pkg/compiler/lib/src/k...
pkg/compiler/lib/src/kernel/kernel_visitor.dart:1106: // Shouldn't be called, as
the resolver have already resolved types and
ditto

https://codereview.chromium.org/2567133002/diff/240001/pkg/compiler/lib/src/p...
File pkg/compiler/lib/src/parser/parser.dart (right):

https://codereview.chromium.org/2567133002/diff/240001/pkg/compiler/lib/src/p...
pkg/compiler/lib/src/parser/parser.dart:370: equals = expect('=', token);
We are only using [equals] as a marker, but with this its value is the token
after '='.

https://codereview.chromium.org/2567133002/diff/240001/pkg/compiler/lib/src/p...
pkg/compiler/lib/src/parser/parser.dart:460: // TODO(ahe): Validate that there
are formal parameters if void.
Remove. This is checked in resolution.

https://codereview.chromium.org/2567133002/diff/240001/pkg/compiler/lib/src/p...
pkg/compiler/lib/src/parser/parser.dart:465: // constructors, and not for
function-typed arguments.
Ditto.

https://codereview.chromium.org/2567133002/diff/240001/pkg/compiler/lib/src/r...
File pkg/compiler/lib/src/resolution/type_resolver.dart (right):

https://codereview.chromium.org/2567133002/diff/240001/pkg/compiler/lib/src/r...
pkg/compiler/lib/src/resolution/type_resolver.dart:163: List<DartType> nameTypes
= const <DartType>[];
[nameTypes] -> [namedTypes]

https://codereview.chromium.org/2567133002/diff/240001/pkg/compiler/lib/src/r...
pkg/compiler/lib/src/resolution/type_resolver.dart:193: {malformedIsError:
false, bool deferredIsMalformed: true}) {
`malformedIsError` -> `bool malformedIsError`.

https://codereview.chromium.org/2567133002/diff/240001/pkg/compiler/lib/src/r...
pkg/compiler/lib/src/resolution/type_resolver.dart:224: {malformedIsError:
false, bool deferredIsMalformed: true}) {
Ditto.

https://codereview.chromium.org/2567133002/diff/240001/pkg/compiler/lib/src/t...
File pkg/compiler/lib/src/tree/nodes.dart (right):

https://codereview.chromium.org/2567133002/diff/240001/pkg/compiler/lib/src/t...
pkg/compiler/lib/src/tree/nodes.dart:280: R visitTypeAnnotation(TypeAnnotation
node, A arg) => visistNode(node, arg);
`visistNode` -> `visitNode`

[floitsch, 2017-01-02 13:48:48]

https://codereview.chromium.org/2567133002/diff/240001/pkg/compiler/lib/src/k...
File pkg/compiler/lib/src/kernel/kernel_visitor.dart (right):

https://codereview.chromium.org/2567133002/diff/240001/pkg/compiler/lib/src/k...
pkg/compiler/lib/src/kernel/kernel_visitor.dart:1092: // Shouldn't be called, as
the resolver have already resolved types and
On 2017/01/02 12:46:53, Johnni Winther wrote:
> As below.

Done.

https://codereview.chromium.org/2567133002/diff/240001/pkg/compiler/lib/src/k...
pkg/compiler/lib/src/kernel/kernel_visitor.dart:1099: // Shouldn't be called, as
the resolver have already resolved types and
On 2017/01/02 12:46:53, Johnni Winther wrote:
> 'have' -> 'has'

Done.

https://codereview.chromium.org/2567133002/diff/240001/pkg/compiler/lib/src/k...
pkg/compiler/lib/src/kernel/kernel_visitor.dart:1106: // Shouldn't be called, as
the resolver have already resolved types and
On 2017/01/02 12:46:53, Johnni Winther wrote:
> ditto

Done.

https://codereview.chromium.org/2567133002/diff/240001/pkg/compiler/lib/src/k...
pkg/compiler/lib/src/kernel/kernel_visitor.dart:1113: // Shouldn't be called, as
the resolver have already resolved types and
done.

https://codereview.chromium.org/2567133002/diff/240001/pkg/compiler/lib/src/p...
File pkg/compiler/lib/src/parser/parser.dart (right):

https://codereview.chromium.org/2567133002/diff/240001/pkg/compiler/lib/src/p...
pkg/compiler/lib/src/parser/parser.dart:370: equals = expect('=', token);
On 2017/01/02 12:46:53, Johnni Winther wrote:
> We are only using [equals] as a marker, but with this its value is the token
> after '='.

Right. Good catch.
done.

https://codereview.chromium.org/2567133002/diff/240001/pkg/compiler/lib/src/p...
pkg/compiler/lib/src/parser/parser.dart:460: // TODO(ahe): Validate that there
are formal parameters if void.
On 2017/01/02 12:46:53, Johnni Winther wrote:
> Remove. This is checked in resolution.

Done.

https://codereview.chromium.org/2567133002/diff/240001/pkg/compiler/lib/src/p...
pkg/compiler/lib/src/parser/parser.dart:465: // constructors, and not for
function-typed arguments.
On 2017/01/02 12:46:53, Johnni Winther wrote:
> Ditto.

Done.

https://codereview.chromium.org/2567133002/diff/240001/pkg/compiler/lib/src/r...
File pkg/compiler/lib/src/resolution/type_resolver.dart (right):

https://codereview.chromium.org/2567133002/diff/240001/pkg/compiler/lib/src/r...
pkg/compiler/lib/src/resolution/type_resolver.dart:163: List<DartType> nameTypes
= const <DartType>[];
On 2017/01/02 12:46:53, Johnni Winther wrote:
> [nameTypes] -> [namedTypes]

Done.

https://codereview.chromium.org/2567133002/diff/240001/pkg/compiler/lib/src/r...
pkg/compiler/lib/src/resolution/type_resolver.dart:193: {malformedIsError:
false, bool deferredIsMalformed: true}) {
On 2017/01/02 12:46:53, Johnni Winther wrote:
> `malformedIsError` -> `bool malformedIsError`.

Done.

https://codereview.chromium.org/2567133002/diff/240001/pkg/compiler/lib/src/r...
pkg/compiler/lib/src/resolution/type_resolver.dart:224: {malformedIsError:
false, bool deferredIsMalformed: true}) {
On 2017/01/02 12:46:53, Johnni Winther wrote:
> Ditto.

Done.

https://codereview.chromium.org/2567133002/diff/240001/pkg/compiler/lib/src/t...
File pkg/compiler/lib/src/tree/nodes.dart (right):

https://codereview.chromium.org/2567133002/diff/240001/pkg/compiler/lib/src/t...
pkg/compiler/lib/src/tree/nodes.dart:280: R visitTypeAnnotation(TypeAnnotation
node, A arg) => visistNode(node, arg);
On 2017/01/02 12:46:53, Johnni Winther wrote:
> `visistNode` -> `visitNode`

ouch. thanks.
done.

[Siggi Cherem (dart-lang), 2017-01-03 23:23:54]

nice!

Changes in parser look great! I'll brainstorm with Johnni tomorrow about
alternatives for the resolution hack, but I'm OK with the current solution for
the time being. Keep us posted on what the final decision is about the flag, I'm
happy either way :)

https://codereview.chromium.org/2567133002/diff/200001/pkg/compiler/lib/src/r...
File pkg/compiler/lib/src/resolution/class_hierarchy.dart (right):

https://codereview.chromium.org/2567133002/diff/200001/pkg/compiler/lib/src/r...
pkg/compiler/lib/src/resolution/class_hierarchy.dart:444: DartType
resolveSupertype(ClassElement cls, NominalTypeAnnotation superclass) {
On 2016/12/30 14:55:47, floitsch wrote:
> On 2016/12/29 22:46:57, Siggi Cherem (dart-lang) wrote:
> > long line (we just run dartfmt before submitting)
> 
> done.
> didn't know that dartfmt was accepted in dart2js.

:)

https://codereview.chromium.org/2567133002/diff/200001/tests/language/general...
File tests/language/generalized_function_type_test.dart (right):

https://codereview.chromium.org/2567133002/diff/200001/tests/language/general...
tests/language/generalized_function_type_test.dart:5: import
'generated_function_syntax_tests.dart';
On 2016/12/30 14:55:47, floitsch wrote:
> On 2016/12/29 22:46:57, Siggi Cherem (dart-lang) wrote:
> > missing `git add` for this file?
> 
> yes :(
> done.

I was still not able to find it :(

[floitsch, 2017-01-04 13:14:12]

On 2017/01/03 23:23:54, Siggi Cherem (dart-lang) wrote:
> nice!
> 
> Changes in parser look great! I'll brainstorm with Johnni tomorrow about
> alternatives for the resolution hack, but I'm OK with the current solution for
> the time being. Keep us posted on what the final decision is about the flag,
I'm
> happy either way :)
> 
>
https://codereview.chromium.org/2567133002/diff/200001/pkg/compiler/lib/src/r...
> File pkg/compiler/lib/src/resolution/class_hierarchy.dart (right):
> 
>
https://codereview.chromium.org/2567133002/diff/200001/pkg/compiler/lib/src/r...
> pkg/compiler/lib/src/resolution/class_hierarchy.dart:444: DartType
> resolveSupertype(ClassElement cls, NominalTypeAnnotation superclass) {
> On 2016/12/30 14:55:47, floitsch wrote:
> > On 2016/12/29 22:46:57, Siggi Cherem (dart-lang) wrote:
> > > long line (we just run dartfmt before submitting)
> > 
> > done.
> > didn't know that dartfmt was accepted in dart2js.
> 
> :)
> 
>
https://codereview.chromium.org/2567133002/diff/200001/tests/language/general...
> File tests/language/generalized_function_type_test.dart (right):
> 
>
https://codereview.chromium.org/2567133002/diff/200001/tests/language/general...
> tests/language/generalized_function_type_test.dart:5: import
> 'generated_function_syntax_tests.dart';
> On 2016/12/30 14:55:47, floitsch wrote:
> > On 2016/12/29 22:46:57, Siggi Cherem (dart-lang) wrote:
> > > missing `git add` for this file?
> > 
> > yes :(
> > done.
> 
> I was still not able to find it :(

It doesn't want to upload it :(

"Not uploading the patch for tests/language/generated_function_syntax_tests.dart
because the file is too large."

I uploaded the current version into a gist:
https://gist.github.com/floitschG/df9c8f6d121bc5f3cf3850487f4f3b66

[floitsch, 2017-01-04 13:48:37]

Changed it such that named arguments must have a type (in function types).

This is a spec change. (VM and analyzer have already been informed).

[Siggi Cherem (dart-lang), 2017-01-04 17:02:37]

thanks!

https://codereview.chromium.org/2567133002/diff/270001/tests/language/general...
File tests/language/generalized_function_type_test.dart (right):

https://codereview.chromium.org/2567133002/diff/270001/tests/language/general...
tests/language/generalized_function_type_test.dart:544: // Copyright (c) 2016,
the Dart project authors.  Please see the AUTHORS file
minor nits: I just looked through the output, it might be worth doing a couple
minor tweaks to make the generated code easier to read:

(a) fix indentation
  - make every `cls.tests.writeln` start with 4 spaces
  - make every class member +2 spaces 
  - remove the extra space before $tests in line 521 above, and before $fields
and $methods (since each of those is indented internally)

(b) consider splitting each test function further:
  - add an empty line between each FunctionType that you are testing, or
  - consider splitting each of those in smaller test functions. (I would prefer
this option)

(c) Consider adding something to make test failures more informative:
  - maybe add a reason to some of the expect cases that will print the function
types that are being tested
  - name each test function from (b) with something informative or use use
package:test to provide that readable test name

(d) consider playing with the order of declarations
  - put members of each class above its runTests
  - maybe put typedefs + top-level functions on the top
  - maybe put the top-level runTest first?
  - maybe split into 3 files: typedefs, top-levels, classes?

[floitsch, 2017-01-11 21:37:38]

Generated tests should look much nicer now.

I kept one file. (It's just so much easier), but did the rest of the
suggestions.

I updated the gist:
https://gist.github.com/floitschG/df9c8f6d121bc5f3cf3850487f4f3b66

[Siggi Cherem (dart-lang), 2017-01-11 22:24:27]

lgtm

[Siggi Cherem (dart-lang), 2017-01-11 22:25:33]

sorry: LGTM! :)

Hope we don't need the flag :)

[floitsch, 2017-01-12 16:47:43]

Looks ready for commit now.
Maybe PTAL at the diff between patch set 20 and 21 in
pkg/compiler/lib/src/serialization/modelz.dart and
pkg/compiler/lib/src/tree/nodes.dart

I had to slightly change them to fix warnings.

Without response I will commit tomorrow.

[Siggi Cherem (dart-lang), 2017-01-12 17:41:52]

lgtm!

[floitsch, 2017-01-13 14:57:15]

Description was changed from

==========
Add support for the new function-type syntax.

Fixes #27967
BUG= http://dartbug.com/27967
==========

to

==========
Add support for the new function-type syntax.

Fixes #27967
BUG= http://dartbug.com/27967
R=sigmund@google.com

Committed:
https://github.com/dart-lang/sdk/commit/85227ba8a60332a18ae92cb1b87bf15d52a909bd
==========

[floitsch, 2017-01-13 14:57:17]

Committed patchset #21 (id:300001) manually as
85227ba8a60332a18ae92cb1b87bf15d52a909bd (presubmit successful).

[floitsch, 2017-01-13 17:37:00]

Description was changed from

==========
Add support for the new function-type syntax.

Fixes #27967
BUG= http://dartbug.com/27967
R=sigmund@google.com

Committed:
https://github.com/dart-lang/sdk/commit/85227ba8a60332a18ae92cb1b87bf15d52a909bd
==========

to

==========
Add support for the new function-type syntax.

Fixes #27967
BUG= http://dartbug.com/27967
R=sigmund@google.com

Committed:
https://github.com/dart-lang/sdk/commit/85227ba8a60332a18ae92cb1b87bf15d52a909bd
==========

[floitsch, 2017-01-13 17:37:32]

Description was changed from

==========
Add support for the new function-type syntax.

Fixes #27967
BUG= http://dartbug.com/27967
R=sigmund@google.com

Committed:
https://github.com/dart-lang/sdk/commit/85227ba8a60332a18ae92cb1b87bf15d52a909bd
==========

to

==========
Add support for the new function-type syntax.

Fixes #27967
BUG= http://dartbug.com/27967
R=sigmund@google.com

Committed:
https://github.com/dart-lang/sdk/commit/85227ba8a60332a18ae92cb1b87bf15d52a909bd

Reverted:
https://github.com/dart-lang/sdk/commit/f5fc210f4f79f7e6b7ccefc24e7565e810e9dc7d
==========

[floitsch, 2017-01-13 17:45:35]

Updated the status file and fixed a small bug in the generated tests.
Relanding.
If you have comments/feedback I will address it in a follow-up CL.

[floitsch, 2017-01-13 17:45:57]

Description was changed from

==========
Add support for the new function-type syntax.

Fixes #27967
BUG= http://dartbug.com/27967
R=sigmund@google.com

Committed:
https://github.com/dart-lang/sdk/commit/85227ba8a60332a18ae92cb1b87bf15d52a909bd

Reverted:
https://github.com/dart-lang/sdk/commit/f5fc210f4f79f7e6b7ccefc24e7565e810e9dc7d
==========

to

==========
Add support for the new function-type syntax.

Fixes #27967
BUG= http://dartbug.com/27967
R=sigmund@google.com

Committed:
https://github.com/dart-lang/sdk/commit/85227ba8a60332a18ae92cb1b87bf15d52a909bd

Reverted:
https://github.com/dart-lang/sdk/commit/f5fc210f4f79f7e6b7ccefc24e7565e810e9dc7d
Committed:
https://github.com/dart-lang/sdk/commit/e4999a631093690febb2eb90cbdbe1f9d5183455
==========

[floitsch, 2017-01-13 17:45:58]

Committed patchset #22 (id:310001) manually as
e4999a631093690febb2eb90cbdbe1f9d5183455 (presubmit successful).

[siva, 2017-01-18 23:48:50]

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

[siva, 2017-01-18 23:48:51]

https://codereview.chromium.org/2567133002/diff/310001/tests/language/general...
File tests/language/generalized_function_type_test.dart (right):

https://codereview.chromium.org/2567133002/diff/310001/tests/language/general...
tests/language/generalized_function_type_test.dart:5: import
'generated_function_syntax_tests.dart';
This test is very huge and causes timeouts when run in --checked mode on various
architectures, can we split this generated test into multiple parts so that the
time for running it falls within the allotted time for a test by the test script

[floitsch, 2017-02-07 15:01:53]

Peter rebased the CL.
I had to modify it slightly. (There was an error).

The tests are now split.

[floitsch, 2017-02-07 15:04:42]

Description was changed from

==========
Add support for the new function-type syntax.

Fixes #27967
BUG= http://dartbug.com/27967
R=sigmund@google.com

Committed:
https://github.com/dart-lang/sdk/commit/85227ba8a60332a18ae92cb1b87bf15d52a909bd

Reverted:
https://github.com/dart-lang/sdk/commit/f5fc210f4f79f7e6b7ccefc24e7565e810e9dc7d
Committed:
https://github.com/dart-lang/sdk/commit/e4999a631093690febb2eb90cbdbe1f9d5183455
==========

to

==========
Add support for the new function-type syntax.

Fixes #27967
BUG= http://dartbug.com/27967
R=sigmund@google.com

Committed:
https://github.com/dart-lang/sdk/commit/85227ba8a60332a18ae92cb1b87bf15d52a909bd

Reverted:
https://github.com/dart-lang/sdk/commit/f5fc210f4f79f7e6b7ccefc24e7565e810e9dc7d

Committed:
https://github.com/dart-lang/sdk/commit/e4999a631093690febb2eb90cbdbe1f9d5183455

Reverted:
https://github.com/dart-lang/sdk/commit/c64c35153905fc2bf8a74c02297220385cfcbc87
==========

[Johnni Winther, 2017-02-08 09:31:32]

lgtm

[floitsch, 2017-02-10 19:25:17]

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

[floitsch, 2017-02-10 19:25:20]

+Peter.
Should have better recovery now. Where should I add a test?
Function is now allowed as keyword.

[ahe, 2017-02-13 10:15:44]

On 2017/02/10 19:25:20, floitsch wrote:
> +Peter.
> Should have better recovery now. Where should I add a test?
> Function is now allowed as keyword.

For error-recovery, the best place is probably pkg/front_end, specifically, add
a test to pkg/front_end/test/fasta/.

It should just be a Dart file that contains an error, and then you run the test
with this command:

DART_CONFIGURATION=ReleaseX64 dart pkg/testing/bin/testing.dart
--config=pkg/front_end/test/fasta/testing.json outline compile kompile

This should also generate expectation files (as they don't exist already).

[ahe, 2017-02-13 10:17:02]

Would it be possible to land the tests as a separate CL?

[floitsch, 2017-02-13 16:24:38]

On 2017/02/13 10:17:02, ahe wrote:
> Would it be possible to land the tests as a separate CL?

I can just commit the test generator, and generate the actual test files in a
different CL. That would be easy and should give you what you want.

[floitsch, 2017-02-13 16:31:04]

Added test and removed the generated tests.

[ahe, 2017-02-14 12:43:02]

There are no action item for you right now, I'm still working on understanding
the changes. But I wanted to let you know that the scanner changes look very
promising. I want to run some local benchmarks (mostly `dart
pkg/testing/bin/testing.dart
--config=pkg/front_end/test/fasta/scanner/testing.json scanner`).

I'm not done understanding the the parser changes.

I have a few minor comments below. I would prefer not renaming the
function-type-alias event to typedef.

https://codereview.chromium.org/2567133002/diff/350001/pkg/compiler/lib/src/d...
File pkg/compiler/lib/src/diagnostics/messages.dart (right):

https://codereview.chromium.org/2567133002/diff/350001/pkg/compiler/lib/src/d...
pkg/compiler/lib/src/diagnostics/messages.dart:3326: howToFix: "Change the
inline function type to a prefixed "
"Change" -> "Try changing"

I think this message could use more work. I don't think most who see this
message will be familiar with what inline function type vs prefixed function
type means.

https://codereview.chromium.org/2567133002/diff/350001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/fasta/parser/listener.dart (right):

https://codereview.chromium.org/2567133002/diff/350001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/parser/listener.dart:227: void beginTypedef(Token
token) {}
Why rename this?

https://codereview.chromium.org/2567133002/diff/350001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/parser/listener.dart:559: void endFunctionType(Token
functionToken, Token endToken) {}
I can't find a matching beginFunctionType.

[floitsch, 2017-02-14 13:56:17]

https://codereview.chromium.org/2567133002/diff/350001/pkg/compiler/lib/src/d...
File pkg/compiler/lib/src/diagnostics/messages.dart (right):

https://codereview.chromium.org/2567133002/diff/350001/pkg/compiler/lib/src/d...
pkg/compiler/lib/src/diagnostics/messages.dart:3326: howToFix: "Change the
inline function type to a prefixed "
On 2017/02/14 12:43:02, ahe wrote:
> "Change" -> "Try changing"
> 
> I think this message could use more work. I don't think most who see this
> message will be familiar with what inline function type vs prefixed function
> type means.

Changed to:
Try changing the inline function type (as in 'int f()') to a prefixed function
type using the `Function` keyword (as in 'int Function() f').

Not sure that's better...

https://codereview.chromium.org/2567133002/diff/350001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/fasta/parser/listener.dart (right):

https://codereview.chromium.org/2567133002/diff/350001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/parser/listener.dart:227: void beginTypedef(Token
token) {}
On 2017/02/14 12:43:02, ahe wrote:
> Why rename this?

Because it's the end of a `typedef`, and we are considering allowing typedefs
for other types as well.

https://codereview.chromium.org/2567133002/diff/350001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/parser/listener.dart:559: void endFunctionType(Token
functionToken, Token endToken) {}
On 2017/02/14 12:43:02, ahe wrote:
> I can't find a matching beginFunctionType.

There is none. The parser doesn't see when a function type starts. In other
words, it may start reading a type (say "int") before it realizes that a
"Function" is there. At that point it would be too late to call the
"beginFunctionType", since "int" is also part of the function type.

[floitsch, 2017-02-14 15:08:07]

Updated to cover function types with void return types.

[ahe, 2017-02-15 11:50:17]

Fasta changes lgtm, with the following caveats:

1. I would prefer to keep the FunctionTypeAlias event name.

2. The parser doesn't recover well from these example:

A:

class Function<T> {
}

Function<String> m() => null;

main() {
  m();
}

B:

class Function<T> {
}

main() {
  Function<String> m() => null;
}

I consider that problematic because these used to be a valid programs, and we
should be able to recover from such cases and tell a user what to do instead.

https://codereview.chromium.org/2567133002/diff/350001/pkg/compiler/lib/src/d...
File pkg/compiler/lib/src/diagnostics/messages.dart (right):

https://codereview.chromium.org/2567133002/diff/350001/pkg/compiler/lib/src/d...
pkg/compiler/lib/src/diagnostics/messages.dart:3326: howToFix: "Change the
inline function type to a prefixed "
On 2017/02/14 13:56:17, floitsch wrote:
> On 2017/02/14 12:43:02, ahe wrote:
> > "Change" -> "Try changing"
> > 
> > I think this message could use more work. I don't think most who see this
> > message will be familiar with what inline function type vs prefixed function
> > type means.
> 
> Changed to:
> Try changing the inline function type (as in 'int f()') to a prefixed function
> type using the `Function` keyword (as in 'int Function() f').
> 
> Not sure that's better...

It's tricky. Ideally, we would say "Try this: '...'" where we have converted the
inline syntax to prefix syntax. But that's definitely future work.

https://codereview.chromium.org/2567133002/diff/350001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/fasta/parser/listener.dart (right):

https://codereview.chromium.org/2567133002/diff/350001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/parser/listener.dart:227: void beginTypedef(Token
token) {}
On 2017/02/14 13:56:17, floitsch wrote:
> On 2017/02/14 12:43:02, ahe wrote:
> > Why rename this?
> 
> Because it's the end of a `typedef`, and we are considering allowing typedefs
> for other types as well.

That's precisely why I'd like to keep calling this a function-type alias: I
assume there are other kinds of typedefs, and this event is about function-type
aliases.

https://codereview.chromium.org/2567133002/diff/350001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/parser/listener.dart:559: void endFunctionType(Token
functionToken, Token endToken) {}
On 2017/02/14 13:56:17, floitsch wrote:
> On 2017/02/14 12:43:02, ahe wrote:
> > I can't find a matching beginFunctionType.
> 
> There is none. The parser doesn't see when a function type starts. In other
> words, it may start reading a type (say "int") before it realizes that a
> "Function" is there. At that point it would be too late to call the
> "beginFunctionType", since "int" is also part of the function type.

Could you document this here and on beginType? It's an exception to the overview
on lines 22-24.

https://codereview.chromium.org/2567133002/diff/390001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/fasta/parser/parser.dart (right):

https://codereview.chromium.org/2567133002/diff/390001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/parser/parser.dart:1526: * token points to the
second `Function` token.
Here's a thought: how about always passing in the first token *after* Function?
This way one can trivially inspect the loop in lines 1472-1474 and conclude that
it doesn't lead to an infinite loop.

[floitsch, 2017-02-16 16:30:15]

I agree that the `class Function` examples are annoying.
However, we did already ship a deprecation warning in 1.22 that triggers when
users have `class Function` (or similar) in their sources.

I think we should rather fail on the `class Function` than on the
`Function<String> m` line.

https://codereview.chromium.org/2567133002/diff/350001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/fasta/parser/listener.dart (right):

https://codereview.chromium.org/2567133002/diff/350001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/parser/listener.dart:559: void endFunctionType(Token
functionToken, Token endToken) {}
On 2017/02/15 11:50:17, ahe wrote:
> On 2017/02/14 13:56:17, floitsch wrote:
> > On 2017/02/14 12:43:02, ahe wrote:
> > > I can't find a matching beginFunctionType.
> > 
> > There is none. The parser doesn't see when a function type starts. In other
> > words, it may start reading a type (say "int") before it realizes that a
> > "Function" is there. At that point it would be too late to call the
> > "beginFunctionType", since "int" is also part of the function type.
> 
> Could you document this here and on beginType? It's an exception to the
overview
> on lines 22-24.

Changed to 'handleFunctionType' (same for "handleType").

Also added 'logEvent' to `handleNoName' and 'handleFunctionType'.

https://codereview.chromium.org/2567133002/diff/390001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/fasta/parser/parser.dart (right):

https://codereview.chromium.org/2567133002/diff/390001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/parser/parser.dart:1526: * token points to the
second `Function` token.
On 2017/02/15 11:50:17, ahe wrote:
> Here's a thought: how about always passing in the first token *after*
Function?
> This way one can trivially inspect the loop in lines 1472-1474 and conclude
that
> it doesn't lead to an infinite loop.

Done.

[floitsch, 2017-02-22 15:53:55]

Description was changed from

==========
Add support for the new function-type syntax.

Fixes #27967
BUG= http://dartbug.com/27967
R=sigmund@google.com

Committed:
https://github.com/dart-lang/sdk/commit/85227ba8a60332a18ae92cb1b87bf15d52a909bd

Reverted:
https://github.com/dart-lang/sdk/commit/f5fc210f4f79f7e6b7ccefc24e7565e810e9dc7d

Committed:
https://github.com/dart-lang/sdk/commit/e4999a631093690febb2eb90cbdbe1f9d5183455

Reverted:
https://github.com/dart-lang/sdk/commit/c64c35153905fc2bf8a74c02297220385cfcbc87
==========

to

==========
Add support for the new function-type syntax.

Fixes #27967
BUG= http://dartbug.com/27967
R=ahe@google.com, johnniwinther@google.com, sigmund@google.com

Committed:
https://github.com/dart-lang/sdk/commit/85227ba8a60332a18ae92cb1b87bf15d52a909bd

Reverted:
https://github.com/dart-lang/sdk/commit/f5fc210f4f79f7e6b7ccefc24e7565e810e9dc7d

Committed:
https://github.com/dart-lang/sdk/commit/e4999a631093690febb2eb90cbdbe1f9d5183455

Reverted:
https://github.com/dart-lang/sdk/commit/c64c35153905fc2bf8a74c02297220385cfcbc87
Committed:
https://github.com/dart-lang/sdk/commit/c289af39c36d9a5f7130a3e69b7891052652a64e
==========

[floitsch, 2017-02-22 15:53:57]

Committed patchset #30 (id:430001) manually as
c289af39c36d9a5f7130a3e69b7891052652a64e (presubmit successful).

[floitsch, 2017-02-22 16:21:07]

Description was changed from

==========
Add support for the new function-type syntax.

Fixes #27967
BUG= http://dartbug.com/27967
R=ahe@google.com, johnniwinther@google.com, sigmund@google.com

Committed:
https://github.com/dart-lang/sdk/commit/85227ba8a60332a18ae92cb1b87bf15d52a909bd

Reverted:
https://github.com/dart-lang/sdk/commit/f5fc210f4f79f7e6b7ccefc24e7565e810e9dc7d

Committed:
https://github.com/dart-lang/sdk/commit/e4999a631093690febb2eb90cbdbe1f9d5183455

Reverted:
https://github.com/dart-lang/sdk/commit/c64c35153905fc2bf8a74c02297220385cfcbc87
Committed:
https://github.com/dart-lang/sdk/commit/c289af39c36d9a5f7130a3e69b7891052652a64e
==========

to

==========
Add support for the new function-type syntax.

Fixes #27967
BUG= http://dartbug.com/27967
R=ahe@google.com, johnniwinther@google.com, sigmund@google.com

Committed:
https://github.com/dart-lang/sdk/commit/85227ba8a60332a18ae92cb1b87bf15d52a909bd
Reverted:
https://github.com/dart-lang/sdk/commit/f5fc210f4f79f7e6b7ccefc24e7565e810e9dc7d
Committed:
https://github.com/dart-lang/sdk/commit/e4999a631093690febb2eb90cbdbe1f9d5183455
Reverted:
https://github.com/dart-lang/sdk/commit/c64c35153905fc2bf8a74c02297220385cfcbc87
Committed:
https://github.com/dart-lang/sdk/commit/c289af39c36d9a5f7130a3e69b7891052652a64e
Reverted:
https://github.com/dart-lang/sdk/commit/c66479f40767744d0bdd027c004583d6e32fe50f
==========

[Paul Berry, 2017-02-22 18:47:23]

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

[Paul Berry, 2017-02-22 18:47:26]

https://codereview.chromium.org/2567133002/diff/470001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart (right):

https://codereview.chromium.org/2567133002/diff/470001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart:1125: Node type = pop();
Shouldn't the type of `type` be TypeAnnotation?

https://codereview.chromium.org/2567133002/diff/470001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart:1129: 
Duplicate lines 1113-1114 here.

https://codereview.chromium.org/2567133002/diff/470001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart:1134:
commentAndMetadata.comment,
Change to "comment"

https://codereview.chromium.org/2567133002/diff/470001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart:1135:
commentAndMetadata.metadata,
Change to "metadata"

https://codereview.chromium.org/2567133002/diff/470001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart:1136: keyword,
Change to "toAnalyzerToken(typedefKeyword)"

https://codereview.chromium.org/2567133002/diff/470001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart:1139: equals,
Change to "toAnalyzerToken(equals)"

https://codereview.chromium.org/2567133002/diff/470001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart:1141: semicolon));
Change to "toAnalyzerToken(endToken)"

https://codereview.chromium.org/2567133002/diff/470001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart:1143:
FunctionTypeAnnotation functionType = type;
I'm confused by this.  Since the if-test on line 1130 is "if type is!
GenericFunctionType)", the type of `type` is now known to be
GenericFunctionType.  I don't know what FunctionTypeAnnotation is.

Suggestion: rework the if-test to the form:

if (type is Foo) {
  ...
} else if (type is Bar) {
  ...
} else {
  internalError('Unexpected kind of type: ${type.runtimeType}');
}

https://codereview.chromium.org/2567133002/diff/470001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart:1145:
commentAndMetadata.comment,
Change to "comment"

https://codereview.chromium.org/2567133002/diff/470001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart:1146:
commentAndMetadata.metadata,
Change to "metadata"

https://codereview.chromium.org/2567133002/diff/470001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart:1147: keyword,
Change to "toAnalyzerToken(typedefKeyword)"

https://codereview.chromium.org/2567133002/diff/470001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart:1150: equals,
Change to "toAnalyzerToken(equals)"

https://codereview.chromium.org/2567133002/diff/470001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart:1152: semicolon));
Change to "toAnalyzerToken(endToken)"

[floitsch, 2017-02-22 19:21:40]

https://codereview.chromium.org/2567133002/diff/470001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart (right):

https://codereview.chromium.org/2567133002/diff/470001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart:1125: Node type = pop();
On 2017/02/22 18:47:25, Paul Berry wrote:
> Shouldn't the type of `type` be TypeAnnotation?

Maybe. I wasn't sure.
Changed to TypeAnnotation.

This only happens if it can't parse a function-type, and I don't know how the
error-recovery is implemented, and what type is produced if things go wrong. I'm
not even sure they can go wrong...

https://codereview.chromium.org/2567133002/diff/470001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart:1129: 
On 2017/02/22 18:47:25, Paul Berry wrote:
> Duplicate lines 1113-1114 here.

Done.

https://codereview.chromium.org/2567133002/diff/470001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart:1134:
commentAndMetadata.comment,
On 2017/02/22 18:47:26, Paul Berry wrote:
> Change to "comment"

Acknowledged.

https://codereview.chromium.org/2567133002/diff/470001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart:1135:
commentAndMetadata.metadata,
On 2017/02/22 18:47:25, Paul Berry wrote:
> Change to "metadata"

Acknowledged.

https://codereview.chromium.org/2567133002/diff/470001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart:1136: keyword,
On 2017/02/22 18:47:26, Paul Berry wrote:
> Change to "toAnalyzerToken(typedefKeyword)"

Acknowledged.

https://codereview.chromium.org/2567133002/diff/470001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart:1139: equals,
On 2017/02/22 18:47:26, Paul Berry wrote:
> Change to "toAnalyzerToken(equals)"

Acknowledged.

https://codereview.chromium.org/2567133002/diff/470001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart:1141: semicolon));
On 2017/02/22 18:47:26, Paul Berry wrote:
> Change to "toAnalyzerToken(endToken)"

Acknowledged.

https://codereview.chromium.org/2567133002/diff/470001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart:1143:
FunctionTypeAnnotation functionType = type;
On 2017/02/22 18:47:26, Paul Berry wrote:
> I'm confused by this.  Since the if-test on line 1130 is "if type is!
> GenericFunctionType)", the type of `type` is now known to be
> GenericFunctionType.  I don't know what FunctionTypeAnnotation is.
> 
> Suggestion: rework the if-test to the form:
> 
> if (type is Foo) {
>   ...
> } else if (type is Bar) {
>   ...
> } else {
>   internalError('Unexpected kind of type: ${type.runtimeType}');
> }

Changed the whole thing.

https://codereview.chromium.org/2567133002/diff/470001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart:1145:
commentAndMetadata.comment,
On 2017/02/22 18:47:25, Paul Berry wrote:
> Change to "comment"

Done.

https://codereview.chromium.org/2567133002/diff/470001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart:1146:
commentAndMetadata.metadata,
On 2017/02/22 18:47:25, Paul Berry wrote:
> Change to "metadata"

Done.

https://codereview.chromium.org/2567133002/diff/470001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart:1147: keyword,
On 2017/02/22 18:47:25, Paul Berry wrote:
> Change to "toAnalyzerToken(typedefKeyword)"

Done.

https://codereview.chromium.org/2567133002/diff/470001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart:1150: equals,
On 2017/02/22 18:47:26, Paul Berry wrote:
> Change to "toAnalyzerToken(equals)"

Done.

https://codereview.chromium.org/2567133002/diff/470001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart:1152: semicolon));
On 2017/02/22 18:47:25, Paul Berry wrote:
> Change to "toAnalyzerToken(endToken)"

Done.

[Paul Berry, 2017-02-22 19:28:36]

ast_builder.dart changes lgtm.

https://codereview.chromium.org/2567133002/diff/490001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart (right):

https://codereview.chromium.org/2567133002/diff/490001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart:1132: //
TODO(brianwilkerson) Generate an error and recover (better than
Feel free to assign this TODO to me.

[floitsch, 2017-02-23 12:15:20]

https://codereview.chromium.org/2567133002/diff/490001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart (right):

https://codereview.chromium.org/2567133002/diff/490001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart:1132: //
TODO(brianwilkerson) Generate an error and recover (better than
On 2017/02/22 19:28:36, Paul Berry wrote:
> Feel free to assign this TODO to me.

Done.

[floitsch, 2017-02-23 12:36:35]

Description was changed from

==========
Add support for the new function-type syntax.

Fixes #27967
BUG= http://dartbug.com/27967
R=ahe@google.com, johnniwinther@google.com, sigmund@google.com

Committed:
https://github.com/dart-lang/sdk/commit/85227ba8a60332a18ae92cb1b87bf15d52a909bd
Reverted:
https://github.com/dart-lang/sdk/commit/f5fc210f4f79f7e6b7ccefc24e7565e810e9dc7d
Committed:
https://github.com/dart-lang/sdk/commit/e4999a631093690febb2eb90cbdbe1f9d5183455
Reverted:
https://github.com/dart-lang/sdk/commit/c64c35153905fc2bf8a74c02297220385cfcbc87
Committed:
https://github.com/dart-lang/sdk/commit/c289af39c36d9a5f7130a3e69b7891052652a64e
Reverted:
https://github.com/dart-lang/sdk/commit/c66479f40767744d0bdd027c004583d6e32fe50f
==========

to

==========
Add support for the new function-type syntax.

Fixes #27967
BUG= http://dartbug.com/27967
R=ahe@google.com, johnniwinther@google.com, paulberry@google.com,
sigmund@google.com

Committed:
https://github.com/dart-lang/sdk/commit/85227ba8a60332a18ae92cb1b87bf15d52a909bd
Reverted:
https://github.com/dart-lang/sdk/commit/f5fc210f4f79f7e6b7ccefc24e7565e810e9dc7d
Committed:
https://github.com/dart-lang/sdk/commit/e4999a631093690febb2eb90cbdbe1f9d5183455
Reverted:
https://github.com/dart-lang/sdk/commit/c64c35153905fc2bf8a74c02297220385cfcbc87
Committed:
https://github.com/dart-lang/sdk/commit/c289af39c36d9a5f7130a3e69b7891052652a64e
Reverted:
https://github.com/dart-lang/sdk/commit/c66479f40767744d0bdd027c004583d6e32fe50f
Committed:
https://github.com/dart-lang/sdk/commit/979183decfa3770236fe8f8525d0ad9e23f5acfb
==========

[floitsch, 2017-02-23 12:36:38]

Committed patchset #36 (id:550001) manually as
979183decfa3770236fe8f8525d0ad9e23f5acfb (presubmit successful).

[ahe, 2017-04-27 12:50:21]

https://codereview.chromium.org/2567133002/diff/550001/tests/language/generic...
File tests/language/generic_function_typedef_test.dart (right):

https://codereview.chromium.org/2567133002/diff/550001/tests/language/generic...
tests/language/generic_function_typedef_test.dart:1: // Copyright (c) 2012, the
Dart project authors.  Please see the AUTHORS file
Wrong year.

[floitsch, 2017-04-27 13:24:53]

https://codereview.chromium.org/2567133002/diff/550001/tests/language/generic...
File tests/language/generic_function_typedef_test.dart (right):

https://codereview.chromium.org/2567133002/diff/550001/tests/language/generic...
tests/language/generic_function_typedef_test.dart:1: // Copyright (c) 2012, the
Dart project authors.  Please see the AUTHORS file
On 2017/04/27 12:50:20, ahe wrote:
> Wrong year.

done in https://codereview.chromium.org/2850473002/

[ahe, 2017-04-27 13:28:15]

https://codereview.chromium.org/2567133002/diff/550001/tests/language/generic...
File tests/language/generic_function_typedef2_test.dart (right):

https://codereview.chromium.org/2567133002/diff/550001/tests/language/generic...
tests/language/generic_function_typedef2_test.dart:1: // Copyright (c) 2012, the
Dart project authors.  Please see the AUTHORS file
:-)

[floitsch, 2017-04-27 13:30:14]

https://codereview.chromium.org/2567133002/diff/550001/tests/language/generic...
File tests/language/generic_function_typedef2_test.dart (right):

https://codereview.chromium.org/2567133002/diff/550001/tests/language/generic...
tests/language/generic_function_typedef2_test.dart:1: // Copyright (c) 2012, the
Dart project authors.  Please see the AUTHORS file
On 2017/04/27 13:28:15, ahe wrote:
> :-)

done in https://codereview.chromium.org/2849523002