fixes #25477, downward inference of generic methods

fixes #25477, downward inference of generic methods

* we now push the context return type down when inferring
* we also take this into account in our initial downards inference for arguments
* adds a common AST interface for FunctionExpressionInvocation and MethodInvocation, so we can handle these more uniformly

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

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

[Jennifer Messerly, 2016-02-19 20:30:36]

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

[Jennifer Messerly, 2016-02-19 21:09:52]

In case it's not apparent, there's two stages to the inference. Given something
like:

    class C {}
    T f<T, S>(List<T> x, S y);

    dynamic x;
    int y;
    C c = f([x], y);

1. we will consider only the return type, and infer `(List<C>, *) -> C`.
2. we will therefore infer `<C>[x]`, and because of that, `x` will need an
implicit cast.
3. on the way back up, we'll infer again, now taking into account the second
argument and concluding `(List<C>, int) -> C`

This is not the fully unified upwards+downwards. Ideally, we could push down
unsolved type parameters, then solve them on the way back up. But that would
require visiting the argument subtrees twice (once during inference, then again
to fill in the solved types). That's a little beyond where I wanted to go with
this right now.

[Jennifer Messerly, 2016-02-19 21:21:08]

On 2016/02/19 21:09:52, John Messerly wrote:
> In case it's not apparent, there's two stages to the inference. Given
something
> like:
> 
>     class C {}
>     T f<T, S>(List<T> x, S y);
> 
>     dynamic x;
>     int y;
>     C c = f([x], y);
> 
> 1. we will consider only the return type, and infer `(List<C>, *) -> C`.
> 2. we will therefore infer `<C>[x]`, and because of that, `x` will need an
> implicit cast.
> 3. on the way back up, we'll infer again, now taking into account the second
> argument and concluding `(List<C>, int) -> C`
> 
> This is not the fully unified upwards+downwards. Ideally, we could push down
> unsolved type parameters, then solve them on the way back up. But that would
> require visiting the argument subtrees twice (once during inference, then
again
> to fill in the solved types). That's a little beyond where I wanted to go with
> this right now.

BTW, should we be recording generic method inference (with `recordInference`)?
We aren't doing that in the existing code. Let me know, I can either roll it in
here or do a follow up.

[Leaf, 2016-02-19 23:19:04]

lgtm.   I think I would like to see us recording the generic type argument
inferences eventually.  The only current use is for testing, but I'm pretty sure
that this is something we'll want to surface to users at some point in various
ways.  I'm fine with punting this off to a future CL, or just filing a bug on it
though.

https://codereview.chromium.org/1720433002/diff/60001/pkg/analyzer/lib/dart/a...
File pkg/analyzer/lib/dart/ast/ast.dart (right):

https://codereview.chromium.org/1720433002/diff/60001/pkg/analyzer/lib/dart/a...
pkg/analyzer/lib/dart/ast/ast.dart:4992: DartType staticInvokeType;
Sort fields?

https://codereview.chromium.org/1720433002/diff/60001/pkg/analyzer/lib/dart/a...
pkg/analyzer/lib/dart/ast/ast.dart:5015: * In either case, the
[invocationTarget.staticType] will indicate the
Not sure that "indicate" is right here?  "be" or maybe "be equivalent to"?

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

https://codereview.chromium.org/1720433002/diff/60001/pkg/analyzer/test/src/t...
pkg/analyzer/test/src/task/strong/inferred_type_test.dart:1697: => list.fold('',
/*info:INFERRED_TYPE_CLOSURE*/(x, y) => x + y.toString()));
Nice!

https://codereview.chromium.org/1720433002/diff/60001/pkg/analyzer/test/src/t...
pkg/analyzer/test/src/task/strong/inferred_type_test.dart:1860: String x =
f(42);
What tells us that this test succeeds?  I guess maybe there'd be a downcast
message otherwise?

[Jennifer Messerly, 2016-02-20 01:14:12]

Thanks Leaf! I'll give Brian a chance to take a look too, at the new AST nodes
especially.

https://codereview.chromium.org/1720433002/diff/60001/pkg/analyzer/lib/dart/a...
File pkg/analyzer/lib/dart/ast/ast.dart (right):

https://codereview.chromium.org/1720433002/diff/60001/pkg/analyzer/lib/dart/a...
pkg/analyzer/lib/dart/ast/ast.dart:4992: DartType staticInvokeType;
On 2016/02/19 23:19:03, Leaf wrote:
> Sort fields?

Hmm. I did sort, but I believe it puts fields before getters/setters.

https://codereview.chromium.org/1720433002/diff/60001/pkg/analyzer/lib/dart/a...
pkg/analyzer/lib/dart/ast/ast.dart:5015: * In either case, the
[invocationTarget.staticType] will indicate the
On 2016/02/19 23:19:03, Leaf wrote:
> Not sure that "indicate" is right here?  "be" or maybe "be equivalent to"?

Done.

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

https://codereview.chromium.org/1720433002/diff/60001/pkg/analyzer/test/src/t...
pkg/analyzer/test/src/task/strong/inferred_type_test.dart:1697: => list.fold('',
/*info:INFERRED_TYPE_CLOSURE*/(x, y) => x + y.toString()));
On 2016/02/19 23:19:04, Leaf wrote:
> Nice!

yeah, I was happy to see that some basic `fold` cases will work now :)

https://codereview.chromium.org/1720433002/diff/60001/pkg/analyzer/test/src/t...
pkg/analyzer/test/src/task/strong/inferred_type_test.dart:1860: String x =
f(42);
On 2016/02/19 23:19:03, Leaf wrote:
> What tells us that this test succeeds?  I guess maybe there'd be a downcast
> message otherwise?

yup, exactly. If T was `dynamic` we'd get a downcast message.

[Jennifer Messerly, 2016-02-22 21:22:52]

Hey Brian, if you're around, figured you might want to take a look at the new
AST interface.

[Brian Wilkerson, 2016-02-22 22:13:26]

LGTM, but the files need to be sorted. Yes, fields sort before getters/setters,
but "p" should still sort before "s".

Also, thanks for the ping. I didn't see the CL go by.

https://codereview.chromium.org/1720433002/diff/80001/pkg/analyzer/lib/dart/a...
File pkg/analyzer/lib/dart/ast/ast.dart (right):

https://codereview.chromium.org/1720433002/diff/80001/pkg/analyzer/lib/dart/a...
pkg/analyzer/lib/dart/ast/ast.dart:4009: ArgumentList get argumentList;
We should remove the members that are now defined by InvocationExpression (both
here and in MethodInvocation).

https://codereview.chromium.org/1720433002/diff/80001/pkg/analyzer/lib/dart/a...
pkg/analyzer/lib/dart/ast/ast.dart:4981: * Common interface for
[FunctionExpressionInvocation] and [MethodInvocation].
nit: "Common interface for" --> "The invocation of a function or method; either
a"

non-nit: Please include the disclaimer:

    Clients may not extend, implement or mix-in this class.

https://codereview.chromium.org/1720433002/diff/80001/pkg/analyzer/lib/dart/a...
pkg/analyzer/lib/dart/ast/ast.dart:4985: * The function type of the method
invocation, or `null` if the AST
nit: "The function type of the method invocation" --> "Return the function type
of the invocation based on the static type information"

https://codereview.chromium.org/1720433002/diff/80001/pkg/analyzer/lib/dart/a...
pkg/analyzer/lib/dart/ast/ast.dart:4992: DartType staticInvokeType;
I have used only getter and setters in the public API (in part to discourage
extending one of these classes). As per below, let's think about whether we
actually need the setter to be defined on this class, or whether we can leave it
on the subclasses.

https://codereview.chromium.org/1720433002/diff/80001/pkg/analyzer/lib/dart/a...
pkg/analyzer/lib/dart/ast/ast.dart:4995: * Like [staticInvokeType], but reflects
propagated type information.
nit: I'd prefer the full documentation here because most clients don't make it
easy to follow links in hover text.

https://codereview.chromium.org/1720433002/diff/80001/pkg/analyzer/lib/dart/a...
pkg/analyzer/lib/dart/ast/ast.dart:5007: void set argumentList(ArgumentList
argumentList);
Do we need this setter to be defined here, or are the subclasses good enough?
Over time I would prefer to have the public API be read-only and to remove all
of the setters, but I can't do that today without introducing breaking changes.
However, given that this is a new class, omitting setters wouldn't be a change.

https://codereview.chromium.org/1720433002/diff/80001/pkg/analyzer/lib/dart/a...
pkg/analyzer/lib/dart/ast/ast.dart:5020: Expression get invocationTarget;
I was a bit confused when I first read the comment because I interpreted
`target` mean the same thing as MethodInvocation.target. Perhaps replace "The
target of the invocation node." with something like "The expression that
identifies the function or method being invoked."

The overloaded terminology is a bit unfortunate. Could we use "function" (from
FunctionInvocationExpression), or would that be weird?

We could probably drop "target will be" in the comments in the examples.

https://codereview.chromium.org/1720433002/diff/80001/pkg/analyzer/lib/dart/a...
pkg/analyzer/lib/dart/ast/ast.dart:5032: void set typeArguments(TypeArgumentList
typeArguments);
Do we need this?

https://codereview.chromium.org/1720433002/diff/80001/pkg/analyzer/lib/src/da...
File pkg/analyzer/lib/src/dart/ast/ast.dart (right):

https://codereview.chromium.org/1720433002/diff/80001/pkg/analyzer/lib/src/da...
pkg/analyzer/lib/src/dart/ast/ast.dart:6139: DartType propagatedInvokeType;
nit: We generally (always?) place non-AstNode typed fields after AstNode typed
fields.

[Jennifer Messerly, 2016-02-22 22:23:45]

On 2016/02/22 22:13:26, Brian Wilkerson wrote:
> LGTM, but the files need to be sorted. Yes, fields sort before
getters/setters,
> but "p" should still sort before "s".
> 
> Also, thanks for the ping. I didn't see the CL go by.
> 
>
https://codereview.chromium.org/1720433002/diff/80001/pkg/analyzer/lib/dart/a...
> File pkg/analyzer/lib/dart/ast/ast.dart (right):
> 
>
https://codereview.chromium.org/1720433002/diff/80001/pkg/analyzer/lib/dart/a...
> pkg/analyzer/lib/dart/ast/ast.dart:4009: ArgumentList get argumentList;
> We should remove the members that are now defined by InvocationExpression
(both
> here and in MethodInvocation).
> 
>
https://codereview.chromium.org/1720433002/diff/80001/pkg/analyzer/lib/dart/a...
> pkg/analyzer/lib/dart/ast/ast.dart:4981: * Common interface for
> [FunctionExpressionInvocation] and [MethodInvocation].
> nit: "Common interface for" --> "The invocation of a function or method;
either
> a"
> 
> non-nit: Please include the disclaimer:
> 
>     Clients may not extend, implement or mix-in this class.
> 
>
https://codereview.chromium.org/1720433002/diff/80001/pkg/analyzer/lib/dart/a...
> pkg/analyzer/lib/dart/ast/ast.dart:4985: * The function type of the method
> invocation, or `null` if the AST
> nit: "The function type of the method invocation" --> "Return the function
type
> of the invocation based on the static type information"
> 
>
https://codereview.chromium.org/1720433002/diff/80001/pkg/analyzer/lib/dart/a...
> pkg/analyzer/lib/dart/ast/ast.dart:4992: DartType staticInvokeType;
> I have used only getter and setters in the public API (in part to discourage
> extending one of these classes). As per below, let's think about whether we
> actually need the setter to be defined on this class, or whether we can leave
it
> on the subclasses.
> 
>
https://codereview.chromium.org/1720433002/diff/80001/pkg/analyzer/lib/dart/a...
> pkg/analyzer/lib/dart/ast/ast.dart:4995: * Like [staticInvokeType], but
reflects
> propagated type information.
> nit: I'd prefer the full documentation here because most clients don't make it
> easy to follow links in hover text.
> 
>
https://codereview.chromium.org/1720433002/diff/80001/pkg/analyzer/lib/dart/a...
> pkg/analyzer/lib/dart/ast/ast.dart:5007: void set argumentList(ArgumentList
> argumentList);
> Do we need this setter to be defined here, or are the subclasses good enough?
> Over time I would prefer to have the public API be read-only and to remove all
> of the setters, but I can't do that today without introducing breaking
changes.
> However, given that this is a new class, omitting setters wouldn't be a
change.
> 
>
https://codereview.chromium.org/1720433002/diff/80001/pkg/analyzer/lib/dart/a...
> pkg/analyzer/lib/dart/ast/ast.dart:5020: Expression get invocationTarget;
> I was a bit confused when I first read the comment because I interpreted
> `target` mean the same thing as MethodInvocation.target. Perhaps replace "The
> target of the invocation node." with something like "The expression that
> identifies the function or method being invoked."
> 
> The overloaded terminology is a bit unfortunate. Could we use "function" (from
> FunctionInvocationExpression), or would that be weird?
> 
> We could probably drop "target will be" in the comments in the examples.
> 
>
https://codereview.chromium.org/1720433002/diff/80001/pkg/analyzer/lib/dart/a...
> pkg/analyzer/lib/dart/ast/ast.dart:5032: void set
typeArguments(TypeArgumentList
> typeArguments);
> Do we need this?
> 
>
https://codereview.chromium.org/1720433002/diff/80001/pkg/analyzer/lib/src/da...
> File pkg/analyzer/lib/src/dart/ast/ast.dart (right):
> 
>
https://codereview.chromium.org/1720433002/diff/80001/pkg/analyzer/lib/src/da...
> pkg/analyzer/lib/src/dart/ast/ast.dart:6139: DartType propagatedInvokeType;
> nit: We generally (always?) place non-AstNode typed fields after AstNode typed
> fields.

FYI ... I've tried running "Code > Sort Members In Dart file" on ast.dart under
lib, multiple times, and it refused to do anything :(.

Anyway, I just fixed it manually. Maybe it doesn't know how to sort fields?

[Jennifer Messerly, 2016-02-22 22:45:55]

Thanks! All requested changes were made. Great idea on using "function" name.

https://codereview.chromium.org/1720433002/diff/80001/pkg/analyzer/lib/dart/a...
File pkg/analyzer/lib/dart/ast/ast.dart (right):

https://codereview.chromium.org/1720433002/diff/80001/pkg/analyzer/lib/dart/a...
pkg/analyzer/lib/dart/ast/ast.dart:4009: ArgumentList get argumentList;
On 2016/02/22 22:13:25, Brian Wilkerson wrote:
> We should remove the members that are now defined by InvocationExpression
(both
> here and in MethodInvocation).

Done.

https://codereview.chromium.org/1720433002/diff/80001/pkg/analyzer/lib/dart/a...
pkg/analyzer/lib/dart/ast/ast.dart:4981: * Common interface for
[FunctionExpressionInvocation] and [MethodInvocation].
On 2016/02/22 22:13:25, Brian Wilkerson wrote:
> nit: "Common interface for" --> "The invocation of a function or method;
either
> a"
> 
> non-nit: Please include the disclaimer:
> 
>     Clients may not extend, implement or mix-in this class.

Done.

https://codereview.chromium.org/1720433002/diff/80001/pkg/analyzer/lib/dart/a...
pkg/analyzer/lib/dart/ast/ast.dart:4985: * The function type of the method
invocation, or `null` if the AST
On 2016/02/22 22:13:25, Brian Wilkerson wrote:
> nit: "The function type of the method invocation" --> "Return the function
type
> of the invocation based on the static type information"

Done. but, FYI, this comment already existed, code simply moved.

https://codereview.chromium.org/1720433002/diff/80001/pkg/analyzer/lib/dart/a...
pkg/analyzer/lib/dart/ast/ast.dart:4995: * Like [staticInvokeType], but reflects
propagated type information.
On 2016/02/22 22:13:25, Brian Wilkerson wrote:
> nit: I'd prefer the full documentation here because most clients don't make it
> easy to follow links in hover text.

Done.

But in general: I very strongly prefer to avoid copy+paste, *especially* in
documentation. It leads to stale content, and reduces incentives to ever improve
docs, because you have to maintain O(copies) instead of O(1).

I don't know what the answer is, though. You're right that how difficult it is
depends on the client. My theory when I originally wrote this (this isn't new
code -- just moved) was that it was an okay tradeoff for maintainability.

https://codereview.chromium.org/1720433002/diff/80001/pkg/analyzer/lib/dart/a...
pkg/analyzer/lib/dart/ast/ast.dart:5007: void set argumentList(ArgumentList
argumentList);
On 2016/02/22 22:13:25, Brian Wilkerson wrote:
> Do we need this setter to be defined here, or are the subclasses good enough?
> Over time I would prefer to have the public API be read-only and to remove all
> of the setters, but I can't do that today without introducing breaking
changes.
> However, given that this is a new class, omitting setters wouldn't be a
change.

Okay, moved setters back down.

https://codereview.chromium.org/1720433002/diff/80001/pkg/analyzer/lib/dart/a...
pkg/analyzer/lib/dart/ast/ast.dart:5020: Expression get invocationTarget;
On 2016/02/22 22:13:25, Brian Wilkerson wrote:
> I was a bit confused when I first read the comment because I interpreted
> `target` mean the same thing as MethodInvocation.target. Perhaps replace "The
> target of the invocation node." with something like "The expression that
> identifies the function or method being invoked."
> 
> The overloaded terminology is a bit unfortunate. Could we use "function" (from
> FunctionInvocationExpression), or would that be weird?
> 
> We could probably drop "target will be" in the comments in the examples.

Nice, yeah "function" is a good idea. Done. (& updated comment as well per your
suggestions)

https://codereview.chromium.org/1720433002/diff/80001/pkg/analyzer/lib/dart/a...
pkg/analyzer/lib/dart/ast/ast.dart:5032: void set typeArguments(TypeArgumentList
typeArguments);
On 2016/02/22 22:13:25, Brian Wilkerson wrote:
> Do we need this?

Done, moved down to subclasses.

https://codereview.chromium.org/1720433002/diff/80001/pkg/analyzer/lib/src/da...
File pkg/analyzer/lib/src/dart/ast/ast.dart (right):

https://codereview.chromium.org/1720433002/diff/80001/pkg/analyzer/lib/src/da...
pkg/analyzer/lib/src/dart/ast/ast.dart:6139: DartType propagatedInvokeType;
On 2016/02/22 22:13:26, Brian Wilkerson wrote:
> nit: We generally (always?) place non-AstNode typed fields after AstNode typed
> fields.

Sorry about that. I didn't realize that "sort members" ignores fields.

[Jennifer Messerly, 2016-02-22 22:48:44]

Description was changed from

==========
fixes #25477, downward inference of generic methods

* we now push the context return type down when inferring
* we also take this into account in our initial downards inference for arguments
* adds a common AST interface for FunctionExpressionInvocation and
MethodInvocation, so we can handle these more uniformly
==========

to

==========
fixes #25477, downward inference of generic methods

* we now push the context return type down when inferring
* we also take this into account in our initial downards inference for arguments
* adds a common AST interface for FunctionExpressionInvocation and
MethodInvocation, so we can handle these more uniformly

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

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

[Jennifer Messerly, 2016-02-22 22:48:45]

Committed patchset #6 (id:100001) manually as
0c4e64c8efafd9ac6d8fd004706ee69639b0d04f (presubmit successful).

[Brian Wilkerson, 2016-02-22 22:50:52]

LGTM

https://codereview.chromium.org/1720433002/diff/100001/pkg/analyzer/lib/dart/...
File pkg/analyzer/lib/dart/ast/ast.dart (right):

https://codereview.chromium.org/1720433002/diff/100001/pkg/analyzer/lib/dart/...
pkg/analyzer/lib/dart/ast/ast.dart:4985: DartType propagatedInvokeType;
I'd forgotten that we decided to not sort fields (because sometimes fields are
grouped for semantic purposes). But if we make these getters instead of fields,
that problem goes away :-)

[Jennifer Messerly, 2016-02-22 22:58:40]

https://codereview.chromium.org/1720433002/diff/100001/pkg/analyzer/lib/dart/...
File pkg/analyzer/lib/dart/ast/ast.dart (right):

https://codereview.chromium.org/1720433002/diff/100001/pkg/analyzer/lib/dart/...
pkg/analyzer/lib/dart/ast/ast.dart:4985: DartType propagatedInvokeType;
On 2016/02/22 22:50:52, Brian Wilkerson wrote:
> I'd forgotten that we decided to not sort fields (because sometimes fields are
> grouped for semantic purposes). But if we make these getters instead of
fields,
> that problem goes away :-)

yeah I can do that. I'll send you a CL.

I was mainly hoping to avoid yet more doc comment expansion ... "Return the
function type of the invocation ... <paragraph>"

"Sets the function type of the invocation ... <paragraph>"