fix #26990, incorrect substitution of upper bound

fix #26990, incorrect substitution of upper bound

We were handling the "extends" clause too early, when we did not have enough information to correctly substitute it. So we tried to substitute later on, but this breaks cases where we (correctly) inferred using the argument types.

The fix is to move the "extends" clause handling to a place where we have the inferred types for our preceding type variables available.

R=leafp@google.com

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

[Jennifer Messerly, 2016-08-02 21:50:20]

Description was changed from

==========
fix #26990, incorrect substitution of upper bound
==========

to

==========
fix #26990, incorrect substitution of upper bound

We were handling the "extends" clause too early, when we did not have enough
information to correctly substitute it. So we tried to substitute later on, but
this breaks cases where we (correctly) inferred using the argument types.

The fix is to move the "extends" clause handling to a place where we have the
inferred types for our preceding type variables available.
==========

[Jennifer Messerly, 2016-08-02 21:50:20]

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

[Leaf, 2016-08-02 23:20:27]

lgtm

https://codereview.chromium.org/2205993002/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/type_system.dart (right):

https://codereview.chromium.org/2205993002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/type_system.dart:1280: _TypeParameterBound bound
= _bounds[typeParam];
Move this down to keep it by the assignment to inferredTypes[i]?  It's a little
confusing that it's read out, then side effected, then used.

https://codereview.chromium.org/2205993002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/type_system.dart:1302:
_inferTypeParameterSubtypeOf(typeParam,
I have a small twinge here about f-bounded subtyping cases, e.g. the <T extends
Clonable<T>> case here, since we don't have anything to substitute for the T in
that case.  I haven't thought through this fully though.  If you have a good
explanation of why this does the right thing, then commenting why this does the
right thing in that case would be great.  If not, we should make a note to sit
down and hash this out.

https://codereview.chromium.org/2205993002/diff/1/pkg/analyzer/test/src/task/...
File pkg/analyzer/test/src/task/strong/inferred_type_test.dart (right):

https://codereview.chromium.org/2205993002/diff/1/pkg/analyzer/test/src/task/...
pkg/analyzer/test/src/task/strong/inferred_type_test.dart:737: void
test_constructors_reverseParameters() {
Are there already tests for inference that takes advantage of the bounds?  If
not, could you file an issue (feel free to assign to me) to fill out the tests
on this?

[Jennifer Messerly, 2016-08-03 00:12:41]

Hey Leaf, you may want to take another look at this for the f-bounded handling.
Or if you want I can land it. Cheers.

https://codereview.chromium.org/2205993002/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/type_system.dart (right):

https://codereview.chromium.org/2205993002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/type_system.dart:1280: _TypeParameterBound bound
= _bounds[typeParam];
On 2016/08/02 23:20:27, Leaf wrote:
> Move this down to keep it by the assignment to inferredTypes[i]?  It's a
little
> confusing that it's read out, then side effected, then used.

good catch! Done

https://codereview.chromium.org/2205993002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/type_system.dart:1302:
_inferTypeParameterSubtypeOf(typeParam,
On 2016/08/02 23:20:27, Leaf wrote:
> I have a small twinge here about f-bounded subtyping cases, e.g. the <T
extends
> Clonable<T>> case here, since we don't have anything to substitute for the T
in
> that case.  I haven't thought through this fully though.  If you have a good
> explanation of why this does the right thing, then commenting why this does
the
> right thing in that case would be great.  If not, we should make a note to sit
> down and hash this out.

Fixed! As we chatted, it now handles f-bounded cases by substituting with
`dynamic` ... I hit fun recursive bug in my test, which is now fixed too.

https://codereview.chromium.org/2205993002/diff/1/pkg/analyzer/test/src/task/...
File pkg/analyzer/test/src/task/strong/inferred_type_test.dart (right):

https://codereview.chromium.org/2205993002/diff/1/pkg/analyzer/test/src/task/...
pkg/analyzer/test/src/task/strong/inferred_type_test.dart:737: void
test_constructors_reverseParameters() {
On 2016/08/02 23:20:27, Leaf wrote:
> Are there already tests for inference that takes advantage of the bounds?  If
> not, could you file an issue (feel free to assign to me) to fill out the tests
> on this?

Yeah there's a few of them in resolver tests, but it's probably still worth
more.

https://github.com/dart-lang/sdk/issues/27009

I added a test case for the f-bounded fallback

[Leaf, 2016-08-03 00:25:03]

lgtm, thanks!

[Jennifer Messerly, 2016-08-03 15:31:17]

Description was changed from

==========
fix #26990, incorrect substitution of upper bound

We were handling the "extends" clause too early, when we did not have enough
information to correctly substitute it. So we tried to substitute later on, but
this breaks cases where we (correctly) inferred using the argument types.

The fix is to move the "extends" clause handling to a place where we have the
inferred types for our preceding type variables available.
==========

to

==========
fix #26990, incorrect substitution of upper bound

We were handling the "extends" clause too early, when we did not have enough
information to correctly substitute it. So we tried to substitute later on, but
this breaks cases where we (correctly) inferred using the argument types.

The fix is to move the "extends" clause handling to a place where we have the
inferred types for our preceding type variables available.

R=leafp@google.com

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

[Jennifer Messerly, 2016-08-03 15:31:18]

Committed patchset #3 (id:40001) manually as
0fe54a637a16b5875fbe18c4dbb628602db75783 (presubmit successful).