improve static type analysis for `await for`

improve static type analysis for `await for`

This tries to do a full fix for https://github.com/dart-lang/sdk/issues/24191

The refactoring tries to unify how generic type substitutions are handled, so they always go through the (correct) mechanisms in the Element model.

This additionally should make it easier to add generic method type parameters in a future CL.

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

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

[Jennifer Messerly, 2015-10-28 20:35:29]

Description was changed from

==========
improve static type analysis for `await for`
==========

to

==========
improve static type analysis for `await for`
This tries to do a full fix for http://dartbug.com/24191
==========

[Jennifer Messerly, 2015-10-28 20:35:45]

Description was changed from

==========
improve static type analysis for `await for`
This tries to do a full fix for http://dartbug.com/24191
==========

to

==========
improve static type analysis for `await for`
This tries to do a full fix for https://github.com/dart-lang/sdk/issues/24191
==========

[Jennifer Messerly, 2015-10-28 20:46:52]

Description was changed from

==========
improve static type analysis for `await for`
This tries to do a full fix for https://github.com/dart-lang/sdk/issues/24191
==========

to

==========
improve static type analysis for `await for`

This tries to do a full fix for https://github.com/dart-lang/sdk/issues/24191

The refactoring tries to unify how generic type substitutions are handled, so
they always go through the (correct) mechanisms in the Element model.

This additionally should make it easier to add generic method type parameters in
a future CL.
==========

[Jennifer Messerly, 2015-10-28 20:46:52]

jmesserly@google.com changed reviewers:
+ brianwilkerson@google.com, paulberry@google.com

[Brian Wilkerson, 2015-10-29 15:32:01]

I'm a little concerned that this introduces some functional regressions in the
places where the use of InheritanceManager has been replaced, but we obviously
don't have any tests that catch those cases.

I also don't know what the full implication of those changes really is. I know
that we might compute the wrong type for the inherited member in cases where the
inherited member is defined in multiple interfaces, but I don't know what user
visible change that will make.

However, this is a much cleaner implementation for the places where we were not
yet using the InheritanceManager, and I'm guessing that the corner cases covered
by the InheritanceManager are rare, so, LGTM.

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

https://codereview.chromium.org/1405143006/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/element.dart:6910: new HashSet<ClassElement>(),
(t) => t.getGetter(name));
We fully type everything in the analyzer, so I'd prefer to have a type on 't',
here and elsewhere.

https://codereview.chromium.org/1405143006/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/element.dart:7248: InterfaceType targetType,
nit: This was probably copy/pasted, but this method could now be an instance
method with one fewer parameters.

https://codereview.chromium.org/1405143006/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/element.dart:7258: if
(visitedInterfaces.contains(targetClass)) {
nit: This was probably copy/pasted, but it would be cleaner if written as:

    if (!visitedInterfaces.add(targetClass)) {
      return;
    }

[Paul Berry, 2015-10-29 16:30:00]

A few minor nits--take 'em or leave 'em as you see fit.

Otherwise lgtm.

Thanks for taking this on!

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

https://codereview.chromium.org/1405143006/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/element.dart:6250: * and mixed in classes, and by
default including [thisType]. If the search
Change this comment to reflect the fact that for
`lookUpInheritedGetterOrMethod`, there is no `thisType` parameter--we *always*
include "this" type.

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

https://codereview.chromium.org/1405143006/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/resolver.dart:12816: return callMethod != null ?
callMethod.type : null;
Nit: these two lines could be collapsed to:

return t.lookupInheritedMethod("call")?.type;

https://codereview.chromium.org/1405143006/diff/1/pkg/analyzer/lib/src/task/s...
File pkg/analyzer/lib/src/task/strong/rules.dart (right):

https://codereview.chromium.org/1405143006/diff/1/pkg/analyzer/lib/src/task/s...
pkg/analyzer/lib/src/task/strong/rules.dart:61: return callMethod != null ?
callMethod.type : null;
Similar nit here.

[Jennifer Messerly, 2015-10-29 16:36:41]

oh shoot! I was not ready to send this out yet. My apologies!!!

https://codereview.chromium.org/1405143006/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/element.dart (left):

https://codereview.chromium.org/1405143006/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/element.dart:7524: void invalidateLibraryCycles()
{
I didn't change this method ... it was moved by sorting members.
If it helps I'm happy to make a separate CL for this.

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

https://codereview.chromium.org/1405143006/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/element.dart:6245: PropertyAccessorElement
lookUpInheritedGetter(String name,
the intent of these methods is to expose the same logic resolution would do.
This makes it easier to correctly do lookups for members like "iterator",
"current", "call", etc. They were moved from the resolver, without any changes
(other than some additional code reuse, explained below).

https://codereview.chromium.org/1405143006/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/element.dart:7247: static ExecutableElement
_lookUpMemberInInterfaces(
this method replaced 4 variants that existed previously. The 4 variants were:

* getters
* setters
* methods
* getters or methods

The "getMember" callback handles the difference, while otherwise allowing the
same code to be used.

https://codereview.chromium.org/1405143006/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/element_resolver.dart (left):

https://codereview.chromium.org/1405143006/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/element_resolver.dart:1578: if (target is
SuperExpression) {
all of this deleted code moved into element.dart

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

https://codereview.chromium.org/1405143006/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/element_resolver.dart:1577: getterName, library:
_definingLibrary, thisType: target is! SuperExpression);
oops, I'll format this file

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

https://codereview.chromium.org/1405143006/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/resolver.dart:12816: return callMethod != null ?
callMethod.type : null;
not sure if I can use null aware ops yet

https://codereview.chromium.org/1405143006/diff/1/pkg/analyzer/test/generated...
File pkg/analyzer/test/generated/resolver_test.dart (right):

https://codereview.chromium.org/1405143006/diff/1/pkg/analyzer/test/generated...
pkg/analyzer/test/generated/resolver_test.dart:257: (listenMethod.type as
FunctionTypeImpl).typeArguments =
The mock SDK's element model wasn't fully populated, which prevented access to
the substituted type.

https://codereview.chromium.org/1405143006/diff/1/pkg/analyzer/test/generated...
pkg/analyzer/test/generated/resolver_test.dart:13127: abstract class
MyCustomStream<T> implements Stream<List<T>> {}
thanks to Paul for this test case :)

[Paul Berry, 2015-10-29 16:43:11]

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

https://codereview.chromium.org/1405143006/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/resolver.dart:12816: return callMethod != null ?
callMethod.type : null;
On 2015/10/29 16:36:40, John Messerly wrote:
> not sure if I can use null aware ops yet

There was a brief period where we couldn't use them in analyzer due to some
Jenkins installations that were analyzing with the latest analyzer and an old
Dart VM.  But that's no longer the case.  Null aware ops are fine now.

[Jennifer Messerly, 2015-10-29 16:45:25]

But I'll send out another version addressing your comments, thank you!

I will also restore substitution path in ErrorVerifier/InheritanceManager.

It is needed, although very very rarely (exactly one place in the entire SDK
code base, in dart2js). To be honest, I don't understand why that is the case.
It worries me if we are somehow getting a partially substituted member in rare
cases, because that will affect other places we use lookupOverrides (like the
overridden member type inference in strong mode). I spent a good while debugging
it yesterday, but with the state of our debug tools, was making very slow
progress =\ ... anyway, in the meantime I've restored substitute* in inheritance
manager, which fixes the problem.

The fix for Stream listen should still work -- basically, trying to make sure we
resolve it the same way in `await for` as if the user wrote:
`stream.listen(...)` in their code. At least, that's how I think the fix works
:)

[Jennifer Messerly, 2015-10-29 17:24:33]

Okay, it's ready now! PTAL. And sorry bout that--forgot I had filled out the R=
line before the CL was ready.

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

https://codereview.chromium.org/1405143006/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/element.dart:6250: * and mixed in classes, and by
default including [thisType]. If the search
On 2015/10/29 16:29:59, Paul Berry wrote:
> Change this comment to reflect the fact that for
> `lookUpInheritedGetterOrMethod`, there is no `thisType` parameter--we *always*
> include "this" type.

Done.

https://codereview.chromium.org/1405143006/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/element.dart:6910: new HashSet<ClassElement>(),
(t) => t.getGetter(name));
On 2015/10/29 15:32:01, Brian Wilkerson wrote:
> We fully type everything in the analyzer, so I'd prefer to have a type on 't',
> here and elsewhere.

Oops, good catch. Done! 

I wonder if we can turn on "strong mode" soon, and use inference for lambdas?
WDYT?

https://codereview.chromium.org/1405143006/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/element.dart:7248: InterfaceType targetType,
On 2015/10/29 15:32:01, Brian Wilkerson wrote:
> nit: This was probably copy/pasted, but this method could now be an instance
> method with one fewer parameters.

yeah ... actually tried that first, the problem is: that would put it on
InterfaceTypeImpl, so the recursive calls to _lookUpMemberInInterfaces aren't
safe anymore, and would need a cast. It felt a bit odd to use an unsafe cast
there, so that's why I left it like this. But I'm willing to change it, WDYT?

https://codereview.chromium.org/1405143006/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/element.dart:7258: if
(visitedInterfaces.contains(targetClass)) {
On 2015/10/29 15:32:01, Brian Wilkerson wrote:
> nit: This was probably copy/pasted, but it would be cleaner if written as:
> 
>     if (!visitedInterfaces.add(targetClass)) {
>       return;
>     }

good catch! Done.

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

https://codereview.chromium.org/1405143006/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/resolver.dart:12816: return callMethod != null ?
callMethod.type : null;
On 2015/10/29 16:43:10, Paul Berry wrote:
> On 2015/10/29 16:36:40, John Messerly wrote:
> > not sure if I can use null aware ops yet
> 
> There was a brief period where we couldn't use them in analyzer due to some
> Jenkins installations that were analyzing with the latest analyzer and an old
> Dart VM.  But that's no longer the case.  Null aware ops are fine now.

Ah thank you!

BTW, I didn't intend to reply to you with that comment ... that was from before
I sent this out! I had a bunch of draft comments yesterday. DOH. Anyway, just
wanted to say I totally trust your comments :) ... that was yesterday-John, who
was very confused! :)

[Paul Berry, 2015-10-29 17:57:47]

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

https://codereview.chromium.org/1405143006/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/element.dart:6250: * and mixed in classes, and by
default including [thisType]. If the search
On 2015/10/29 17:24:33, John Messerly wrote:
> On 2015/10/29 16:29:59, Paul Berry wrote:
> > Change this comment to reflect the fact that for
> > `lookUpInheritedGetterOrMethod`, there is no `thisType` parameter--we
*always*
> > include "this" type.
> 
> Done.

Looks like maybe you overfixed this?  You've removed the documentation for
[thisType] from lookUpInheritedGetter, lookUpInheritedGetterOrMethod, and
lookUpInheritedMethod.  But lookUpInheritedGetterOrMethod is the only method
that lacks a [thisType] parameter.

https://codereview.chromium.org/1405143006/diff/40001/pkg/analyzer/lib/src/ta...
File pkg/analyzer/lib/src/task/strong/rules.dart (right):

https://codereview.chromium.org/1405143006/diff/40001/pkg/analyzer/lib/src/ta...
pkg/analyzer/lib/src/task/strong/rules.dart:61: return callMethod != null ?
callMethod.type : null;
These two lines could also be collapsed down to just:

return t.lookupMethod("call", null)?.type;

[Brian Wilkerson, 2015-10-29 17:57:54]

LGTM

https://codereview.chromium.org/1405143006/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/element.dart (left):

https://codereview.chromium.org/1405143006/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/element.dart:7524: void invalidateLibraryCycles()
{
> I didn't change this method ... it was moved by sorting members.
> If it helps I'm happy to make a separate CL for this.

No need. I've learned to recognize and ignore churn :-)

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

https://codereview.chromium.org/1405143006/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/element.dart:6910: new HashSet<ClassElement>(),
(t) => t.getGetter(name));
> I wonder if we can turn on "strong mode" soon, and use inference for lambdas?
> WDYT?

I use type annotations in order to express my intent, so that intent is violated
I'll know about it. It seems to me that dropping the type, even in this limited
case, weakens the ability of the tools to tell me if my assumptions are being
violated. So, as much as I want to use strong mode for other reasons, I don't
really want to drop types as a result of using it.

https://codereview.chromium.org/1405143006/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/element.dart:7248: InterfaceType targetType,
> It felt a bit odd to use an unsafe cast
> there, so that's why I left it like this. But I'm willing to change it, WDYT?

I agree. Leave it as it is and we can address it later. When we rewrite this to
use the InheritanceManager that problem will probably go away.

[Jennifer Messerly, 2015-10-29 18:12:42]

thanks!

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

https://codereview.chromium.org/1405143006/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/element.dart:6250: * and mixed in classes, and by
default including [thisType]. If the search
On 2015/10/29 17:57:47, Paul Berry wrote:
> On 2015/10/29 17:24:33, John Messerly wrote:
> > On 2015/10/29 16:29:59, Paul Berry wrote:
> > > Change this comment to reflect the fact that for
> > > `lookUpInheritedGetterOrMethod`, there is no `thisType` parameter--we
> *always*
> > > include "this" type.
> > 
> > Done.
> 
> Looks like maybe you overfixed this?  You've removed the documentation for
> [thisType] from lookUpInheritedGetter, lookUpInheritedGetterOrMethod, and
> lookUpInheritedMethod.  But lookUpInheritedGetterOrMethod is the only method
> that lacks a [thisType] parameter.

oops. Yes :)

https://codereview.chromium.org/1405143006/diff/40001/pkg/analyzer/lib/src/ta...
File pkg/analyzer/lib/src/task/strong/rules.dart (right):

https://codereview.chromium.org/1405143006/diff/40001/pkg/analyzer/lib/src/ta...
pkg/analyzer/lib/src/task/strong/rules.dart:61: return callMethod != null ?
callMethod.type : null;
On 2015/10/29 17:57:47, Paul Berry wrote:
> These two lines could also be collapsed down to just:
> 
> return t.lookupMethod("call", null)?.type;

Doh! Could swear I fixed this ... maybe in another place?

[Jennifer Messerly, 2015-10-29 18:17:22]

Committed patchset #4 (id:60001) manually as
2503cb8eeb844b86ad05e9620778655c4c553571 (presubmit successful).