Implement override-based type inference for instance methods.

Implement override-based type inference for instance methods.

Also, fix a rare infinite loop bug in analyzer exposed by one of the
tests.

R=brianwilkerson@google.com, sigmund@google.com

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

[Paul Berry, 2017-06-21 16:07:03]

paulberry@google.com changed reviewers:
+ ahe@google.com, brianwilkerson@google.com, sigmund@google.com

[Paul Berry, 2017-06-21 16:07:04]

Siggi or Peter to review front_end changes
Brian to review analzer change

[Siggi Cherem (dart-lang), 2017-06-21 20:19:08]

lgtm

https://codereview.chromium.org/2946273002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/kernel/kernel_procedure_builder.dart (right):

https://codereview.chromium.org/2946273002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/kernel_procedure_builder.dart:251: // For
now, we skip inferred return types of setters.
aren't they always void (otherwise it is an error)?

https://codereview.chromium.org/2946273002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/type_inference/type_inference_engine.dart
(right):

https://codereview.chromium.org/2946273002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/type_inference/type_inference_engine.dart:277:
return const DynamicType();
report an error instead? (same thing when the name is not found in
getNamedParameterType?)

Do we want to check this preemtively when we call recordOverride? or we prefer
to do it here so we can still infer a type for the other arguments that do
match?

(also, I'm not sure if this is already specified in Leaf doc, or if we need to
add it there)

https://codereview.chromium.org/2946273002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/type_inference/type_inference_engine.dart:529: //
TODO(paulberry): Types don't match.  Report an error.
Random questions:

When not doing inference, are there checks elsewhere about types being
consistent? When doing inference, does this mean we compute the overriden types
twice (once here and once to do the check later in the compiler)? If so, do we
care or want to try to avoid doing it twice? I can imagine we could compute
overriden types lazily as needed here and rely on the regular checker to detect
when types don't match, but I'm not sure if that's really feasible and how much
it helps (it might make it harder to produce good error messages). 

I don't believe there is anything to change here, but just wanted to ask in case
you already have plans in mind.

https://codereview.chromium.org/2946273002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/type_inference/type_schema_environment.dart
(right):

https://codereview.chromium.org/2946273002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/type_inference/type_schema_environment.dart:29:
typeParameters: newTypeParameters,
Just making sure I follow the change here. Before you had substitutionMap.keys,
was that basically using the old type-parameters instead of the new ones?

[Brian Wilkerson, 2017-06-21 20:54:40]

Analyzer changes lgtm

[Paul Berry, 2017-06-21 20:54:47]

https://codereview.chromium.org/2946273002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/kernel/kernel_procedure_builder.dart (right):

https://codereview.chromium.org/2946273002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/kernel_procedure_builder.dart:251: // For
now, we skip inferred return types of setters.
On 2017/06/21 20:19:08, Siggi Cherem (dart-lang) wrote:
> aren't they always void (otherwise it is an error)?

Not quite.

In strong mode if no type is specified, the inferred type is `void`; if a type
is specified and it's not `void`, that's an error.

In spec mode if no type is specified, the inferred type is `dynamic`; if a type
is specified and it's not `void`, that's a static warning.

I plan to implement the proper strong mode rules in a follow-up CL, hopefully
later today.

https://codereview.chromium.org/2946273002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/type_inference/type_inference_engine.dart
(right):

https://codereview.chromium.org/2946273002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/type_inference/type_inference_engine.dart:277:
return const DynamicType();
On 2017/06/21 20:19:08, Siggi Cherem (dart-lang) wrote:
> report an error instead? (same thing when the name is not found in
> getNamedParameterType?)
> 
> Do we want to check this preemtively when we call recordOverride? or we prefer
> to do it here so we can still infer a type for the other arguments that do
> match?
> 
> (also, I'm not sure if this is already specified in Leaf doc, or if we need to
> add it there)

Good question--I should have been clearer here.  Leaf's doc says:

"If there is no corresponding parameter position in the overridden method to
infer from and the signatures are compatible, it is treated as dynamic (e.g.
overriding a one parameter method with a method that takes a second optional
parameter).  Note: if there is no corresponding parameter position in the
overriden method to infer from and the signatures are incompatible (e.g.
overriding a one parameter method with a method that takes a second non-optional
parameter), the inference result is not defined and tools are free to either
emit an error, or to defer the error to override checking."

So I think we're ok.  For the case where the parameter is optional, we're
treating the type as dynamic, as per spec.  For the case where the parameter is
non-optional, we're deferring the error to override checking, which the spec
allows.

I've added a comment to clarify.  The comment is at the call site because in a
follow up CL I'm planning to move getPositionalParameterType out of this file to
reduce code duplication.

https://codereview.chromium.org/2946273002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/type_inference/type_inference_engine.dart:529: //
TODO(paulberry): Types don't match.  Report an error.
On 2017/06/21 20:19:08, Siggi Cherem (dart-lang) wrote:
> Random questions:
> 
> When not doing inference, are there checks elsewhere about types being
> consistent? When doing inference, does this mean we compute the overriden
types
> twice (once here and once to do the check later in the compiler)? If so, do we
> care or want to try to avoid doing it twice? I can imagine we could compute
> overriden types lazily as needed here and rely on the regular checker to
detect
> when types don't match, but I'm not sure if that's really feasible and how
much
> it helps (it might make it harder to produce good error messages). 
> 
> I don't believe there is anything to change here, but just wanted to ask in
case
> you already have plans in mind.
> 

Peter has added some checks, but I believe that he has been focusing on things
that are spec mode errors, so probably most of the checks you are talking about
are not written yet.

I haven't thought too hard about how those checks should work yet.  My suspicion
is that the computational cost of computing the overridden types twice is going
to be small (since the slow cases all involve generics, which are used less
frequently), but I'm open to trying to avoid redundant work if it turns out to
be a bottleneck.

https://codereview.chromium.org/2946273002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/type_inference/type_schema_environment.dart
(right):

https://codereview.chromium.org/2946273002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/type_inference/type_schema_environment.dart:29:
typeParameters: newTypeParameters,
On 2017/06/21 20:19:08, Siggi Cherem (dart-lang) wrote:
> Just making sure I follow the change here. Before you had
substitutionMap.keys,
> was that basically using the old type-parameters instead of the new ones?

That's right.  It was a bug which happened to be benign, because prior to this
CL, the only caller of this function didn't actually depend on the type
parameters being correct.  But now that I'm using this function to actually set
an inferred type, it actually matters that the type parameters are correct :)

[Paul Berry, 2017-06-21 20:56:10]

Description was changed from

==========
Implement override-based type inference for instance methods.

Also, fix a rare infinite loop bug in analyzer exposed by one of the
tests.
==========

to

==========
Implement override-based type inference for instance methods.

Also, fix a rare infinite loop bug in analyzer exposed by one of the
tests.

R=brianwilkerson@google.com, sigmund@google.com

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

[Paul Berry, 2017-06-21 20:56:12]

Committed patchset #1 (id:1) manually as
b2c4b0ac0abbf80ce0bd647f78aad78702678d3c (presubmit successful).