fix #25740 again, making sure we have the substituted bound in inference

fix #25740 again, making sure we have the substituted bound in inference

The first fix only addressed part of the problem. Unfortunately we go back and forth between .element and .type, assuming they always return the other one, which was not the case for TypeParameterMember.

R=leafp@google.com

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

[Jennifer Messerly, 2016-08-30 15:32:03]

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

[Leaf, 2016-08-30 16:34:51]

lgtm

https://codereview.chromium.org/2291103004/diff/1/pkg/analyzer/lib/src/dart/e...
File pkg/analyzer/lib/src/dart/element/member.dart (right):

https://codereview.chromium.org/2291103004/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/member.dart:961: bool operator ==(obj) =>
Why is this the right equality for these?  I don't know how much these get used
or how, but it worries me that if I put two of these with different bounds into
a hash table they'll get de-duped.

[Jennifer Messerly, 2016-08-30 23:53:39]

https://codereview.chromium.org/2291103004/diff/1/pkg/analyzer/lib/src/dart/e...
File pkg/analyzer/lib/src/dart/element/member.dart (right):

https://codereview.chromium.org/2291103004/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/member.dart:961: bool operator ==(obj) =>
On 2016/08/30 16:34:51, Leaf wrote:
> Why is this the right equality for these?  I don't know how much these get
used
> or how, but it worries me that if I put two of these with different bounds
into
> a hash table they'll get de-duped.

Good point, I'm going to investigate this further. It appears to be papering
over another bug, where parameters that have type parameter T as their type are
not getting the substituted bound for T.

[Jennifer Messerly, 2016-08-31 03:55:33]

Hi Leaf, addressed your earlier comment & this version is passing.

I think there's a better fix here (along the lines of the suggestion in
https://github.com/dart-lang/sdk/issues/27040), which is to introduce a
"DartType get bound" on TypeParameterType and switch over code to use that.

If you want I can try it after this or I can try it now. But it will expand the
change to touch more code so it might be easier to stage on its own.

I'd also be tempted to change "get typeFormals" to return TypeParameterTypes
instead of TypeParameterElements (sigh) and then we could eliminate
TypeParameterMember. But for now, it's a way to fake the change described above.

[Jennifer Messerly, 2016-08-31 04:12:50]

On 2016/08/31 03:55:33, John Messerly wrote:
> Hi Leaf, addressed your earlier comment & this version is passing.
> 
> I think there's a better fix here (along the lines of the suggestion in
> https://github.com/dart-lang/sdk/issues/27040), which is to introduce a
> "DartType get bound" on TypeParameterType and switch over code to use that.
> 
> If you want I can try it after this or I can try it now. But it will expand
the
> change to touch more code so it might be easier to stage on its own.
> 
> I'd also be tempted to change "get typeFormals" to return TypeParameterTypes
> instead of TypeParameterElements (sigh) and then we could eliminate
> TypeParameterMember. But for now, it's a way to fake the change described
above.

Scratch that. It's failing on some of the language tests (but not Analyzer
tests) so will take some time to track down. If I had to guess, recursive
bounds.

So I'm kind of getting lost in other bugs here & this fix is snowballing. I
think the initial partial fix (patch set 1) might be a safer place to start. The
equality is flawed, but in some sense, that problem already existed (because it
has never considered the correct bound for equality purposes). I can file a new
bug about the general issues around type parameter bounds & generic methods.

[Leaf, 2016-08-31 16:35:26]

On 2016/08/31 04:12:50, John Messerly wrote:
> On 2016/08/31 03:55:33, John Messerly wrote:
> > Hi Leaf, addressed your earlier comment & this version is passing.
> > 
> > I think there's a better fix here (along the lines of the suggestion in
> > https://github.com/dart-lang/sdk/issues/27040), which is to introduce a
> > "DartType get bound" on TypeParameterType and switch over code to use that.
> > 
> > If you want I can try it after this or I can try it now. But it will expand
> the
> > change to touch more code so it might be easier to stage on its own.
> > 
> > I'd also be tempted to change "get typeFormals" to return TypeParameterTypes
> > instead of TypeParameterElements (sigh) and then we could eliminate
> > TypeParameterMember. But for now, it's a way to fake the change described
> above.
> 
> Scratch that. It's failing on some of the language tests (but not Analyzer
> tests) so will take some time to track down. If I had to guess, recursive
> bounds.
> 
> So I'm kind of getting lost in other bugs here & this fix is snowballing. I
> think the initial partial fix (patch set 1) might be a safer place to start.
The
> equality is flawed, but in some sense, that problem already existed (because
it
> has never considered the correct bound for equality purposes). I can file a
new
> bug about the general issues around type parameter bounds & generic methods.

Sure, fine if you want to land the original patch, maybe add a TODO to sort out
the equality?

[Jennifer Messerly, 2016-08-31 17:58:39]

On 2016/08/31 16:35:26, Leaf wrote:
> On 2016/08/31 04:12:50, John Messerly wrote:
> > On 2016/08/31 03:55:33, John Messerly wrote:
> > > Hi Leaf, addressed your earlier comment & this version is passing.
> > > 
> > > I think there's a better fix here (along the lines of the suggestion in
> > > https://github.com/dart-lang/sdk/issues/27040), which is to introduce a
> > > "DartType get bound" on TypeParameterType and switch over code to use
that.
> > > 
> > > If you want I can try it after this or I can try it now. But it will
expand
> > the
> > > change to touch more code so it might be easier to stage on its own.
> > > 
> > > I'd also be tempted to change "get typeFormals" to return
TypeParameterTypes
> > > instead of TypeParameterElements (sigh) and then we could eliminate
> > > TypeParameterMember. But for now, it's a way to fake the change described
> > above.
> > 
> > Scratch that. It's failing on some of the language tests (but not Analyzer
> > tests) so will take some time to track down. If I had to guess, recursive
> > bounds.
> > 
> > So I'm kind of getting lost in other bugs here & this fix is snowballing. I
> > think the initial partial fix (patch set 1) might be a safer place to start.
> The
> > equality is flawed, but in some sense, that problem already existed (because
> it
> > has never considered the correct bound for equality purposes). I can file a
> new
> > bug about the general issues around type parameter bounds & generic methods.
> 
> Sure, fine if you want to land the original patch, maybe add a TODO to sort
out
> the equality?

Sounds great. Actually I'll file a bug too:
https://github.com/dart-lang/sdk/issues/27210

[Jennifer Messerly, 2016-08-31 18:10:25]

Description was changed from

==========
fix #25740 again, making sure we have the substituted bound in inference

The first fix only addressed part of the problem. Unfortunately we go back and
forth between .element and .type, assuming they always return the other one,
which was not the case for TypeParameterMember.
==========

to

==========
fix #25740 again, making sure we have the substituted bound in inference

The first fix only addressed part of the problem. Unfortunately we go back and
forth between .element and .type, assuming they always return the other one,
which was not the case for TypeParameterMember.

R=leafp@google.com

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

[Jennifer Messerly, 2016-08-31 18:10:26]

Committed patchset #4 (id:60001) manually as
63046da4be09ee9bb95f2356b22392dbdf8e199d (presubmit successful).