Use type variables on Typedef correctly.

Use type variables on Typedef correctly.

R=johnniwinther@google.com, paulberry@google.com

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

[ahe, 2017-07-04 12:26:53]

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

[ahe, 2017-07-04 12:26:53]

Initially, when I implemented typedefs, I implemented them incorrectly (not
based on a single function type, but based on formals and returnType).

As an aid in understanding what I did before, think of it this way: when I saw:

    typedef Foo<A> = A Function(A a);

I would effectively rewrite it to:

    typedef Foo = A Function<A>(A a);

This CL changes that, and as a side-effect fixes some semantic issues related to
typedefs.

This prepares for improved handling of type variables in situations like this:

    typedef Foo<A> = A Function<T>(T t);

[ahe, 2017-07-04 13:49:59]

Patchset #4 (id:60001) has been deleted

[ahe, 2017-07-04 13:50:23]

Sorry, I forgot to update expectations. I've uploaded a new patch set.

[Johnni Winther, 2017-07-04 14:07:39]

lgtm

[Paul Berry, 2017-07-04 14:48:03]

lgtm

https://codereview.chromium.org/2967923002/diff/80001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/kernel/kernel_function_type_builder.dart
(right):

https://codereview.chromium.org/2967923002/diff/80001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/kernel_function_type_builder.dart:47: if
(formal.isRequired) requiredParameterCount++;
If the constraints on parameter kinds are violated (i.e. required parameter
follows optional parameter, or both positional optional and named optional
parameters are present), do we need to report the error here?  Or is that
handled somewhere else?

[ahe, 2017-07-04 16:26:09]

Thank you, Paul and Johnni!

https://codereview.chromium.org/2967923002/diff/80001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/kernel/kernel_function_type_builder.dart
(right):

https://codereview.chromium.org/2967923002/diff/80001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/kernel_function_type_builder.dart:47: if
(formal.isRequired) requiredParameterCount++;
On 2017/07/04 14:48:03, Paul Berry wrote:
> If the constraints on parameter kinds are violated (i.e. required parameter
> follows optional parameter, or both positional optional and named optional
> parameters are present), do we need to report the error here?  Or is that
> handled somewhere else?

Good question. I thought the syntactic constraints would be handled by the
parser, but I just checked, and it seems like they aren't. I'll investigate more
and file a bug or something.

[ahe, 2017-07-05 07:41:39]

I found some additional issues during testing. PTAL

[ahe, 2017-07-05 07:55:24]

I don't know why this test wasn't on my short list already, it must have kinda
worked.

https://codereview.chromium.org/2967923002/diff/100001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/fasta/kernel/kernel_function_type_alias_builder.dart
(right):

https://codereview.chromium.org/2967923002/diff/100001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/kernel/kernel_function_type_alias_builder.dart:62:
DartType builtType = type?.build(library) ?? const DynamicType();
These tests were crashing without handling type being null:

language/generic_function_typedef2_test/00: Pass -> DartkCrash
language/generic_function_typedef2_test/01: Pass -> DartkCrash
language/generic_function_typedef2_test/02: Pass -> DartkCrash
language/generic_function_typedef2_test/03: Pass -> DartkCrash

https://codereview.chromium.org/2967923002/diff/100001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/fasta/kernel/kernel_type_variable_builder.dart
(right):

https://codereview.chromium.org/2967923002/diff/100001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/kernel/kernel_type_variable_builder.dart:36:
library.addWarning(arguments.first.charOffset,
This test uses a type variable with arguments and wasn't exercised before:

language/generic_function_typedef2_test/04: Pass -> CompileTimeError

[Johnni Winther, 2017-07-05 10:35:58]

lgtm

https://codereview.chromium.org/2967923002/diff/100001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/fasta/kernel/kernel_function_type_alias_builder.dart
(right):

https://codereview.chromium.org/2967923002/diff/100001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/kernel/kernel_function_type_alias_builder.dart:62:
DartType builtType = type?.build(library) ?? const DynamicType();
On 2017/07/05 07:55:23, ahe wrote:
> These tests were crashing without handling type being null:
> 
> language/generic_function_typedef2_test/00: Pass -> DartkCrash
> language/generic_function_typedef2_test/01: Pass -> DartkCrash
> language/generic_function_typedef2_test/02: Pass -> DartkCrash
> language/generic_function_typedef2_test/03: Pass -> DartkCrash

Shouldn't the type be const InvalidType() is type is null?

[ahe, 2017-07-05 11:11:32]

Thank you, Johnni and Paul!

https://codereview.chromium.org/2967923002/diff/100001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/fasta/kernel/kernel_function_type_alias_builder.dart
(right):

https://codereview.chromium.org/2967923002/diff/100001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/kernel/kernel_function_type_alias_builder.dart:62:
DartType builtType = type?.build(library) ?? const DynamicType();
On 2017/07/05 10:35:57, Johnni Winther wrote:
> On 2017/07/05 07:55:23, ahe wrote:
> > These tests were crashing without handling type being null:
> > 
> > language/generic_function_typedef2_test/00: Pass -> DartkCrash
> > language/generic_function_typedef2_test/01: Pass -> DartkCrash
> > language/generic_function_typedef2_test/02: Pass -> DartkCrash
> > language/generic_function_typedef2_test/03: Pass -> DartkCrash
> 
> Shouldn't the type be const InvalidType() is type is null?

The short answer is no, the longer answer is:

For now, I avoid using the Invalid classes because they don't give nice error
messages, and I've actually made a subclass of the verifier that complains if it
finds them.

I only use InvalidType temporarily here during resolution to detect cyclic
typedefs. It's supposed to be gone once the type is fully resolved.

[ahe, 2017-07-05 11:12:33]

Description was changed from

==========
Use type variables on Typedef correctly.
==========

to

==========
Use type variables on Typedef correctly.

R=johnniwinther@google.com, paulberry@google.com

Committed:
https://github.com/dart-lang/sdk/commit/6bb89e4fbf8c5a73ec4dbc436074f7d92681967f
==========

[ahe, 2017-07-05 11:12:34]

Committed patchset #5 (id:100001) manually as
6bb89e4fbf8c5a73ec4dbc436074f7d92681967f (presubmit successful).