Adjusts dart2js backend to handle method type arguments.

Adjusts dart2js backend to handle method type arguments.

This CL has the same purpose as 1969753002, stated in the title line,
but it works for checked mode execution (where 1969753002 was broken),
and for usage of method type arguments in additional situations.

Approach: `DartType` and subtypes have been extended with new methods
to erase `MethodTypeVariableType` to `DynamicType`. This is done early
(after resolution, before SsaBuilder), such that only few parts of the
compiler need to deal with anything new. This approach handles method
type variables in checked mode, in `new` expressions, and in function
typed parameters. Finally, tests are added for the new cases, and
status files updated.

UPDATE: See message #10 and #12 for an updated description of the approach.

R=johnniwinther@google.com

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

[eernst, 2016-05-13 16:34:06]

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

[eernst, 2016-05-13 16:34:06]

Johnni and Karl: Both of you gave input to this, so you may wish to comment on
the result.

Note that the erasure (`eraseMethodTypeVariableType`,
`containsMethodTypeVariableType`) of `MethodTypeVariableType` from `DartType`
may be costly, so there may be a need to cache the result, or store in a field
whether it is guaranteed to be a no-op, or something like that. What do you
think?

I haven't measured the performance impact systematically, but the first
impression is that it does not matter. Will produce better data before this is
landed.

[floitsch, 2016-05-13 17:59:58]

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

[floitsch, 2016-05-13 17:59:59]

DBC high level comment.
I wouldn't add `TODO(eernst)` for changes that will be needed when generic
arguments have semantic effect.
Maybe just find a good place and explain what would roughly still need to
happen, and specify that the known locations are marked with a specific keyword
or sentence.
It's currently not a TODO, but with this approach a future developer could get a
headstart when implementing semantic support.

[eernst, 2016-05-17 09:51:30]

Changed 'TODO(eernst)' to 'GENERIC_METHODS' to mark comments about things that
need to be done for full method type argument support, and added a "global"
comment on the issue in front of the declaration of the option string.

[Johnni Winther, 2016-05-17 10:27:05]

lgtm

https://codereview.chromium.org/1976213002/diff/1/pkg/compiler/lib/src/dart_t...
File pkg/compiler/lib/src/dart_types.dart (right):

https://codereview.chromium.org/1976213002/diff/1/pkg/compiler/lib/src/dart_t...
pkg/compiler/lib/src/dart_types.dart:530: DartType get
eraseMethodTypeVariableType {
Move this to GenericType and use [createInstantiation] to create the type at the
end.

https://codereview.chromium.org/1976213002/diff/1/pkg/compiler/lib/src/ssa/bu...
File pkg/compiler/lib/src/ssa/builder.dart (right):

https://codereview.chromium.org/1976213002/diff/1/pkg/compiler/lib/src/ssa/bu...
pkg/compiler/lib/src/ssa/builder.dart:4779: } else if (type is
MethodTypeVariableType) {
When is this used?

[eernst, 2016-05-17 14:48:54]

PTAL: The updates needed in order to handle `is`, `as`, and type expressions
(e.g., `Type f<T>() => T;`) were significant.

https://codereview.chromium.org/1976213002/diff/1/pkg/compiler/lib/src/dart_t...
File pkg/compiler/lib/src/dart_types.dart (right):

https://codereview.chromium.org/1976213002/diff/1/pkg/compiler/lib/src/dart_t...
pkg/compiler/lib/src/dart_types.dart:530: DartType get
eraseMethodTypeVariableType {
On 2016/05/17 10:27:05, Johnni Winther wrote:
> Move this to GenericType and use [createInstantiation] to create the type at
the
> end.

Done.

https://codereview.chromium.org/1976213002/diff/1/pkg/compiler/lib/src/ssa/bu...
File pkg/compiler/lib/src/ssa/builder.dart (right):

https://codereview.chromium.org/1976213002/diff/1/pkg/compiler/lib/src/ssa/bu...
pkg/compiler/lib/src/ssa/builder.dart:4779: } else if (type is
MethodTypeVariableType) {
On 2016/05/17 10:27:05, Johnni Winther wrote:
> When is this used?

I believe that it is not used any more: with the new approach, every
`MethodTypeVariableType` has been replaced by a `DynamicType` when this point is
reached. Rerunning tests confirm that there are no known situations where it is
needed. Removed the whole `else if (..) {..}` such that the pre-previous-CL form
of `addTypeVariableReference` is restored.

[karlklose, 2016-05-18 06:40:59]

DBC

https://codereview.chromium.org/1976213002/diff/20001/pkg/compiler/lib/src/da...
File pkg/compiler/lib/src/dart_backend/backend_ast_to_frontend_ast.dart (right):

https://codereview.chromium.org/1976213002/diff/20001/pkg/compiler/lib/src/da...
pkg/compiler/lib/src/dart_backend/backend_ast_to_frontend_ast.dart:498: //
GENERIC_METHODS: In order to support generic methods fully,
Or consider filing a bug and change this to TODO(<issue number>). That way you
have already a tracking bug and it is easy to find the places that you already
know need to be changed.

[floitsch, 2016-05-18 09:47:27]

On 2016/05/18 06:40:59, karlklose wrote:
> DBC
> 
>
https://codereview.chromium.org/1976213002/diff/20001/pkg/compiler/lib/src/da...
> File pkg/compiler/lib/src/dart_backend/backend_ast_to_frontend_ast.dart
(right):
> 
>
https://codereview.chromium.org/1976213002/diff/20001/pkg/compiler/lib/src/da...
> pkg/compiler/lib/src/dart_backend/backend_ast_to_frontend_ast.dart:498: //
> GENERIC_METHODS: In order to support generic methods fully,
> Or consider filing a bug and change this to TODO(<issue number>). That way you
> have already a tracking bug and it is easy to find the places that you already
> know need to be changed.

I don't like to put these into TODOs. We probably want to do this, but there is
no concrete plan, and it's not something that can currently be done.

[eernst, 2016-05-19 08:34:18]

PTAL.

This CL makes several changes in order to ensure that method type variables
behave differently at runtime: When evaluated as type expressions they throw a
`TypeError` (but being nested in a type expression, say List<T>, they still
become `dynamic`), and when used in `is` and `as` they also throw a `TypeError`.
A `MessageKind` was added to explain that the type error is caused by lack of
reification.

This was achieved by splitting `eraseMethodTypeVariableType` to two methods
named `dynamify..` and `malform..`; the former works as `erase..` did, and the
latter replaces each method type variable type by a malformed type. There is
some code duplication, but it seemed better to avoid many evaluations of `if`
guards during compilation than saving a few lines of code.

`buildTypeConversion` now uses both `dynamify..` and `malform..` (due to the
needs of `is` vs. checked mode), and `generateTypeVariableLiteral` changes
method type variables to `TypeError`s.

[eernst, 2016-05-19 08:40:13]

Description was changed from

==========
Adjusts dart2js backend to handle method type arguments.

This CL has the same purpose as 1969753002, stated in the title line,
but it works for checked mode execution (where 1969753002 was broken),
and for usage of method type arguments in additional situations.

Approach: `DartType` and subtypes have been extended with new methods
to erase `MethodTypeVariableType` to `DynamicType`. This is done early
(after resolution, before SsaBuilder), such that only few parts of the
compiler need to deal with anything new. This approach handles method
type variables in checked mode, in `new` expressions, and in function
typed parameters. Finally, tests are added for the new cases, and
status files updated.
==========

to

==========
Adjusts dart2js backend to handle method type arguments.

This CL has the same purpose as 1969753002, stated in the title line,
but it works for checked mode execution (where 1969753002 was broken),
and for usage of method type arguments in additional situations.

Approach: `DartType` and subtypes have been extended with new methods
to erase `MethodTypeVariableType` to `DynamicType`. This is done early
(after resolution, before SsaBuilder), such that only few parts of the
compiler need to deal with anything new. This approach handles method
type variables in checked mode, in `new` expressions, and in function
typed parameters. Finally, tests are added for the new cases, and
status files updated.

UPDATE: See message #10 for an updated description of the approach.
==========

[eernst, 2016-05-20 14:49:28]

PTAL, significant changes again.

This CL changes `MethodTypeVariableType` to have a true `isMalformed`, and
adjusts many locations where a test for `isMalformed` can no longer assume that
the given type is a `MalformedType`. Based on this change, it eliminates the
`malformTypeVariableType` method.

The CL corrects the examples for
`MessageKind.TYPE_VARIABLE_FROM_METHOD_NOT_REIFIED`, then comments them out
because we cannot (as far as I know) pass --generic-method-syntax such that they
can succeed when used by tests in tests/compiler/dart2js.


Finally, 'generic_methods_type_expression_test' was extended with cases for
method type variables used in type expressions as type arguments.

[eernst, 2016-05-20 14:50:17]

Description was changed from

==========
Adjusts dart2js backend to handle method type arguments.

This CL has the same purpose as 1969753002, stated in the title line,
but it works for checked mode execution (where 1969753002 was broken),
and for usage of method type arguments in additional situations.

Approach: `DartType` and subtypes have been extended with new methods
to erase `MethodTypeVariableType` to `DynamicType`. This is done early
(after resolution, before SsaBuilder), such that only few parts of the
compiler need to deal with anything new. This approach handles method
type variables in checked mode, in `new` expressions, and in function
typed parameters. Finally, tests are added for the new cases, and
status files updated.

UPDATE: See message #10 for an updated description of the approach.
==========

to

==========
Adjusts dart2js backend to handle method type arguments.

This CL has the same purpose as 1969753002, stated in the title line,
but it works for checked mode execution (where 1969753002 was broken),
and for usage of method type arguments in additional situations.

Approach: `DartType` and subtypes have been extended with new methods
to erase `MethodTypeVariableType` to `DynamicType`. This is done early
(after resolution, before SsaBuilder), such that only few parts of the
compiler need to deal with anything new. This approach handles method
type variables in checked mode, in `new` expressions, and in function
typed parameters. Finally, tests are added for the new cases, and
status files updated.

UPDATE: See message #10 and #12 for an updated description of the approach.
==========

[Johnni Winther, 2016-05-23 12:50:41]

https://codereview.chromium.org/1976213002/diff/80001/pkg/compiler/lib/src/co...
File pkg/compiler/lib/src/compile_time_constants.dart (right):

https://codereview.chromium.org/1976213002/diff/80001/pkg/compiler/lib/src/co...
pkg/compiler/lib/src/compile_time_constants.dart:241: if (elementType is
MalformedType) {
Add a TODO for me here; I'm not sure we should generate an error here.

https://codereview.chromium.org/1976213002/diff/80001/pkg/compiler/lib/src/da...
File pkg/compiler/lib/src/dart_types.dart (right):

https://codereview.chromium.org/1976213002/diff/80001/pkg/compiler/lib/src/da...
pkg/compiler/lib/src/dart_types.dart:264: ErroneousElement get
toMalformedElement =>
Remove this (unused)

https://codereview.chromium.org/1976213002/diff/80001/pkg/compiler/lib/src/di...
File pkg/compiler/lib/src/diagnostics/messages.dart (right):

https://codereview.chromium.org/1976213002/diff/80001/pkg/compiler/lib/src/di...
pkg/compiler/lib/src/diagnostics/messages.dart:1160: "type variable, or
dynamic."
You can add an [options] argument to MessageTemplate to make the examples work.

And I think you need to add checks for and use fo this message template in
resolution/members.dart in handleIs, handleAs, and
handleTypeVariableTypeLiteralAccess

https://codereview.chromium.org/1976213002/diff/80001/pkg/compiler/lib/src/re...
File pkg/compiler/lib/src/resolution/constructors.dart (right):

https://codereview.chromium.org/1976213002/diff/80001/pkg/compiler/lib/src/re...
pkg/compiler/lib/src/resolution/constructors.dart:722: if (type.isMalformed) {
If you move `if (type.isTypeVariable) ...` before `if (type.isMalformed) ...` no
other changes are needed.

[Johnni Winther, 2016-05-23 14:16:44]

lgtm

[eernst, 2016-05-23 14:30:24]

Updates in response to review, as discussed IRL already.

https://codereview.chromium.org/1976213002/diff/20001/pkg/compiler/lib/src/da...
File pkg/compiler/lib/src/dart_backend/backend_ast_to_frontend_ast.dart (right):

https://codereview.chromium.org/1976213002/diff/20001/pkg/compiler/lib/src/da...
pkg/compiler/lib/src/dart_backend/backend_ast_to_frontend_ast.dart:498: //
GENERIC_METHODS: In order to support generic methods fully,
On 2016/05/18 06:40:59, karlklose wrote:
> Or consider filing a bug and change this to TODO(<issue number>). That way you
> have already a tracking bug and it is easy to find the places that you already
> know need to be changed.

(This topic was discussed and finalized outside this comment thread).

https://codereview.chromium.org/1976213002/diff/80001/pkg/compiler/lib/src/co...
File pkg/compiler/lib/src/compile_time_constants.dart (right):

https://codereview.chromium.org/1976213002/diff/80001/pkg/compiler/lib/src/co...
pkg/compiler/lib/src/compile_time_constants.dart:241: if (elementType is
MalformedType) {
On 2016/05/23 12:50:41, Johnni Winther wrote:
> Add a TODO for me here; I'm not sure we should generate an error here.

Done.

https://codereview.chromium.org/1976213002/diff/80001/pkg/compiler/lib/src/da...
File pkg/compiler/lib/src/dart_types.dart (right):

https://codereview.chromium.org/1976213002/diff/80001/pkg/compiler/lib/src/da...
pkg/compiler/lib/src/dart_types.dart:264: ErroneousElement get
toMalformedElement =>
On 2016/05/23 12:50:41, Johnni Winther wrote:
> Remove this (unused)

(OK, unused after fixing constructor.dart:739), done.

https://codereview.chromium.org/1976213002/diff/80001/pkg/compiler/lib/src/di...
File pkg/compiler/lib/src/diagnostics/messages.dart (right):

https://codereview.chromium.org/1976213002/diff/80001/pkg/compiler/lib/src/di...
pkg/compiler/lib/src/diagnostics/messages.dart:1160: "type variable, or
dynamic."
On 2016/05/23 12:50:41, Johnni Winther wrote:
> You can add an [options] argument to MessageTemplate to make the examples
work.
>
> And I think you need to add checks for and use fo this message template in
> resolution/members.dart in handleIs, handleAs, and
> handleTypeVariableTypeLiteralAccess

Marked as TODOs for the next CL.

https://codereview.chromium.org/1976213002/diff/80001/pkg/compiler/lib/src/re...
File pkg/compiler/lib/src/resolution/constructors.dart (right):

https://codereview.chromium.org/1976213002/diff/80001/pkg/compiler/lib/src/re...
pkg/compiler/lib/src/resolution/constructors.dart:722: if (type.isMalformed) {
On 2016/05/23 12:50:41, Johnni Winther wrote:
> If you move `if (type.isTypeVariable) ...` before `if (type.isMalformed) ...`
no
> other changes are needed.

Done.

[eernst, 2016-05-23 15:17:40]

Description was changed from

==========
Adjusts dart2js backend to handle method type arguments.

This CL has the same purpose as 1969753002, stated in the title line,
but it works for checked mode execution (where 1969753002 was broken),
and for usage of method type arguments in additional situations.

Approach: `DartType` and subtypes have been extended with new methods
to erase `MethodTypeVariableType` to `DynamicType`. This is done early
(after resolution, before SsaBuilder), such that only few parts of the
compiler need to deal with anything new. This approach handles method
type variables in checked mode, in `new` expressions, and in function
typed parameters. Finally, tests are added for the new cases, and
status files updated.

UPDATE: See message #10 and #12 for an updated description of the approach.
==========

to

==========
Adjusts dart2js backend to handle method type arguments.

This CL has the same purpose as 1969753002, stated in the title line,
but it works for checked mode execution (where 1969753002 was broken),
and for usage of method type arguments in additional situations.

Approach: `DartType` and subtypes have been extended with new methods
to erase `MethodTypeVariableType` to `DynamicType`. This is done early
(after resolution, before SsaBuilder), such that only few parts of the
compiler need to deal with anything new. This approach handles method
type variables in checked mode, in `new` expressions, and in function
typed parameters. Finally, tests are added for the new cases, and
status files updated.

UPDATE: See message #10 and #12 for an updated description of the approach.

R=johnniwinther@google.com

Committed:
https://github.com/dart-lang/sdk/commit/04e45eb634fe06c293cdb9fbd973ed2614431bed
==========

[eernst, 2016-05-23 15:17:42]

Committed patchset #5 (id:80001) manually as
04e45eb634fe06c293cdb9fbd973ed2614431bed (presubmit successful).