fix #25425, more inference of generic methods

fix #25425, more inference of generic methods
this adds support for function expr invocation, as well as other element types besides executable elements

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

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

[Jennifer Messerly, 2016-01-13 22:32:45]

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

[Jennifer Messerly, 2016-01-13 22:32:46]

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

https://codereview.chromium.org/1586813002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/element_resolver.dart:659: methodName.staticType
= staticInvokeType;
This stores the element's type in the methodName node. We were already doing
that for variables, and in strong mode we used to do it always. This just makes
it happen always for consistency and ease of use for resolved ASTs.

Note, this change is not strictly necessary. We could put "getInvokeType" on
Resolver and call it from ElementResolver and StaticTypeAnalyzer. I actually
started down that route, but then recalled discussions yesterday and Leaf's
prodding, which led me to just populate it in the AST.

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

https://codereview.chromium.org/1586813002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:719: DartType
staticType = variable.type;
This code is no longer needed, as ElementResolver computes it.

The new code is fixing a bug too... the type we recorded in methodName node was
not taking into account type promotion that might apply to the variable. I'm
guessing that was not intended. (type promotion *was* correctly taken into
account for the parameters and return type, so in practice it would be hard to
notice the difference)

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

https://codereview.chromium.org/1586813002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:720:
_inferMethodInvocation(node);
moved up so we don't need to overwrite the return type. Instead we let it be
computed naturally a few lines down.

https://codereview.chromium.org/1586813002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1815: node.staticType =
inferred.returnType;
oops, I'll remove this line. It is no longer needed

https://codereview.chromium.org/1586813002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1826: //
TODO(jmesserly): if we had a common AST interface between these two nodes,
this method is virtually identical to the one above.

BTW, I actually fixed this TODO and added a common AST interface, but ended up
backing out of that change to keep the CL here smaller and avoid API changes to
ast.dart. But let me know if you would like it restored.

https://codereview.chromium.org/1586813002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1926: DartType nodeType
= node.staticInvokeType;
the "_inferMethodInvocationObject" method didn't work until return type was
computed, which is why it was done in a strange place in visitMethodInvocation

[Jennifer Messerly, 2016-01-13 22:33:15]

https://codereview.chromium.org/1586813002/diff/20001/pkg/analyzer/test/gener...
File pkg/analyzer/test/generated/resolver_test.dart (right):

https://codereview.chromium.org/1586813002/diff/20001/pkg/analyzer/test/gener...
pkg/analyzer/test/generated/resolver_test.dart:13292: void
test_genericMethod_functionExpressionInvocation_explicit() {
no changes here, just "fail_" to "test_" :)

[Jennifer Messerly, 2016-01-13 22:35:56]

https://codereview.chromium.org/1586813002/diff/20001/pkg/analyzer/test/gener...
File pkg/analyzer/test/generated/resolver_test.dart (right):

https://codereview.chromium.org/1586813002/diff/20001/pkg/analyzer/test/gener...
pkg/analyzer/test/generated/resolver_test.dart:13463:
expect(map1.staticType.toString(), '<T>((dynamic) → T) → T');
this change is because we're populating the methodName's staticType.

[Leaf, 2016-01-13 23:59:41]

lgtm, thanks!

[Brian Wilkerson, 2016-01-14 18:35:19]

LGTM. Sorry for the delay, I missed the e-mail. Feel free to ping me if I take
this long because it likely means that I missed the review request.

https://codereview.chromium.org/1586813002/diff/20001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/element_resolver.dart (right):

https://codereview.chromium.org/1586813002/diff/20001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/element_resolver.dart:1360: * For getters it is
the type that they return.
Interesting that getters and methods are different. It would be good to document
why that is.

https://codereview.chromium.org/1586813002/diff/20001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/static_type_analyzer.dart (right):

https://codereview.chromium.org/1586813002/diff/20001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1826: // we could
remove this duplicated code.
I'm working on separating the AST's interface from it's implementation, which
would make this (and several other things) easier to do.

https://codereview.chromium.org/1586813002/diff/20001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1827: bool
_inferFunctionInvocationGeneric(FunctionExpressionInvocation node) {
nit: please sort the file

[Jennifer Messerly, 2016-01-14 21:24:29]

Thanks!

https://codereview.chromium.org/1586813002/diff/20001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/element_resolver.dart (right):

https://codereview.chromium.org/1586813002/diff/20001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/element_resolver.dart:1360: * For getters it is
the type that they return.
On 2016/01/14 18:35:18, Brian Wilkerson wrote:
> Interesting that getters and methods are different. It would be good to
document
> why that is.

Done.

https://codereview.chromium.org/1586813002/diff/20001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/static_type_analyzer.dart (right):

https://codereview.chromium.org/1586813002/diff/20001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1826: // we could
remove this duplicated code.
On 2016/01/14 18:35:18, Brian Wilkerson wrote:
> I'm working on separating the AST's interface from it's implementation, which
> would make this (and several other things) easier to do.

Nice!

https://codereview.chromium.org/1586813002/diff/20001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1827: bool
_inferFunctionInvocationGeneric(FunctionExpressionInvocation node) {
On 2016/01/14 18:35:18, Brian Wilkerson wrote:
> nit: please sort the file

Done.

[Jennifer Messerly, 2016-01-14 21:27:38]

Description was changed from

==========
fix #25425, more inference of generic methods
this adds support for function expr invocation, as well as other element types
besides executable elements
==========

to

==========
fix #25425, more inference of generic methods
this adds support for function expr invocation, as well as other element types
besides executable elements

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

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

[Jennifer Messerly, 2016-01-14 21:27:39]

Committed patchset #4 (id:60001) manually as
9098ae50aebe1a41fbd9001d1e5a1ad10524525a (presubmit successful).