support generic methods in FunctionExpressionInvocation node

support generic methods in FunctionExpressionInvocation node
fixes part of #25175

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

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

[Jennifer Messerly, 2016-01-06 23:14:42]

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

[Jennifer Messerly, 2016-01-06 23:14:42]

This is a follow up to the other CL.

NOTE: It will need the same fix as the other CL about "only record propagated
type info if it is more specific". Once I figure that out for MethodInvocation,
I'll add it here too. :)

[Brian Wilkerson, 2016-01-06 23:44:57]

LGTM

[Jennifer Messerly, 2016-01-08 00:17:06]

I've updated this to: rebase it against latest & record the propagated type only
if better, like MethodInvocation does.

[Brian Wilkerson, 2016-01-08 01:02:22]

Still LGTM

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

https://codereview.chromium.org/1561233003/diff/20001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/element_resolver.dart:1341: // TODO(jmesserly):
should we return `dynamic` in this case?
No.

(1) We use `null` to mean that there is no type better than static, and doing so
gives us the ability to not do extra work when there's no need to. We don't
check for dynamic, so we'd end up carrying this type forward needlessly.

(2) When we ask for the 'best' type (for example, in code completion) we use the
propagated type when it's available (not null), and the static when it isn't. We
assume that the propagated type is always better without checking. Therefore,
returning dynamic would potentially worsen the type information used in lots of
other places.

https://codereview.chromium.org/1561233003/diff/20001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/element_resolver.dart:1741: DartType
_propagatedInvokeTypeIfBetter(
I think this is now being done in multiple places. It would be good to make it a
utility somewhere. (Perhaps a TODO and address it as part of another CL.)

https://codereview.chromium.org/1561233003/diff/20001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/element_resolver.dart:2077: DartType
_resolveGenericMethod(
Ditto

[Leaf, 2016-01-08 18:34:02]

lgtm!

[Jennifer Messerly, 2016-01-11 18:49:09]

thanks! I tried to improve the TODOs, feel free to take another look. I may also
submit and we can continue the TODO discussions after if that is okay with you
guys. (Always easy to send a CL to change TODO text)

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

https://codereview.chromium.org/1561233003/diff/20001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/element_resolver.dart:1341: // TODO(jmesserly):
should we return `dynamic` in this case?
On 2016/01/08 01:02:21, Brian Wilkerson wrote:
> No.
> 
> (1) We use `null` to mean that there is no type better than static, and doing
so
> gives us the ability to not do extra work when there's no need to. We don't
> check for dynamic, so we'd end up carrying this type forward needlessly.
> 
> (2) When we ask for the 'best' type (for example, in code completion) we use
the
> propagated type when it's available (not null), and the static when it isn't.
We
> assume that the propagated type is always better without checking. Therefore,
> returning dynamic would potentially worsen the type information used in lots
of
> other places.

So right now this method is used to populate "staticInvokeType" and
"propagatedInvokeType".

Does it ever make sense to have a `null` staticInvokeType? That was my main
concern that led to adding a TODO.

However, even for propagatedInvokeType: if we return `dynamic`, we will end up
storing `null`, because of the logic that discards propagatedInvokeType unless
it is better. `dynamic` can never be more specific than another type. Or at
least I would hope so.

Because of that I think returning `dynamic` would have no effect on the
propagated type code paths. Hence the TODO.

I added some more text in an attempt to clarify it.

https://codereview.chromium.org/1561233003/diff/20001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/element_resolver.dart:1741: DartType
_propagatedInvokeTypeIfBetter(
On 2016/01/08 01:02:21, Brian Wilkerson wrote:
> I think this is now being done in multiple places. It would be good to make it
a
> utility somewhere. (Perhaps a TODO and address it as part of another CL.)

Yeah, good idea, we can try and merge this logic with the other "propagated type
if better" logic. I'll add a TODO. (The reason I couldn't reuse the existing
code is it expects to set the Expression.propagatedType field. Likely with some
restructuring we could combine them but it didn't look totally straightforward,
so kept it out of this CL.)

https://codereview.chromium.org/1561233003/diff/20001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/element_resolver.dart:2077: DartType
_resolveGenericMethod(
On 2016/01/08 01:02:21, Brian Wilkerson wrote:
> Ditto

Yeah, I was trying to make this method be the shared utility. This logic doesn't
happen anywhere else, as far as I know. Perhaps it needs a better home, but it's
not clear where would be better, as the error reporting logic ties it to the
Resolver family (ElementResolver/Resolver/StaticTypeAnalyzer) for now.

I'm open to ideas but it's not clear what action to take, at least not yet.

[Brian Wilkerson, 2016-01-11 18:56:41]

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

https://codereview.chromium.org/1561233003/diff/20001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/element_resolver.dart:1341: // TODO(jmesserly):
should we return `dynamic` in this case?
> Does it ever make sense to have a `null` staticInvokeType? That was my main
> concern that led to adding a TODO.

No. I was distracted by the propagated path. We should always have static type
information, even if it's 'dynamic'.

> However, even for propagatedInvokeType: if we return `dynamic`, we will end up
> storing `null`, because of the logic that discards propagatedInvokeType unless
> it is better.

That's reasonable.

https://codereview.chromium.org/1561233003/diff/20001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/element_resolver.dart:2077: DartType
_resolveGenericMethod(
> ... it's not clear what action to take, at least not yet.

Agreed. A later CL is fine.

[Jennifer Messerly, 2016-01-11 20:56:11]

Description was changed from

==========
support generic methods in FunctionExpressionInvocation node
fixes part of #25175
==========

to

==========
support generic methods in FunctionExpressionInvocation node
fixes part of #25175

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

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

[Jennifer Messerly, 2016-01-11 20:56:11]

Committed patchset #3 (id:40001) manually as
8c1e862a9098a8d0c2ae3cd660ec85fd92411a31 (presubmit successful).