fix #25220, infer generic type from constructor arguments

fix #25220, infer generic type from constructor arguments

this makes it consistent in how constructors, generic functions, and literals are treated

includes changes in https://codereview.chromium.org/2115743002/

R=leafp@google.com

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

[Jennifer Messerly, 2016-07-02 00:07:27]

Patchset #1 (id:1) has been deleted

[Jennifer Messerly, 2016-07-02 00:15:53]

Description was changed from

==========
fix #25220, infer generic type from constructor arguments

this makes it consistent in how constructors, generic functions, and literals
are treated
==========

to

==========
fix #25220, infer generic type from constructor arguments

this makes it consistent in how constructors, generic functions, and literals
are treated

includes changes in https://codereview.chromium.org/2115743002/
==========

[Jennifer Messerly, 2016-07-02 00:15:53]

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

[Jennifer Messerly, 2016-07-02 00:25:31]

Hi Leaf,

Please take a look at this one. I've tried to comment all of the interesting
areas. There was a bit more than I expected ... seems to be my luck lately ;)

- John

[Jennifer Messerly, 2016-07-12 16:51:53]

@Leaf, friendly ping if you get a chance. No worries I know you've had a lot of
reviews to juggle lately. :)

[Leaf, 2016-07-12 17:23:26]

Very nice!  LGTM! 

Sorry about the delay - I put this aside since you were on vacation and then
forgot to come back to it.

I think this is a breaking change for strong mode.  We should at least keep
track of this as a potential source of breakage in the next release.

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

https://codereview.chromium.org/2115173002/diff/60001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1974: if
(originalElement is! ConstructorMember) {
Is this just (possibly justified :) paranoia, or can this happen?  In malformed
code maybe?  Worth a comment?

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

https://codereview.chromium.org/2115173002/diff/60001/pkg/analyzer/test/src/t...
pkg/analyzer/test/src/task/strong/inferred_type_test.dart:594: T t;
Maybe test a named constructor?  Redirecting constructor?  Const constructor?

https://codereview.chromium.org/2115173002/diff/60001/pkg/analyzer/test/src/t...
pkg/analyzer/test/src/task/strong/inferred_type_test.dart:599: int y = x.t;
Maybe some tests for some varieties of cases?  Specifically maybe:

num y;
C<int> x = new C(y);
C<num> x = new C(42);

var x = new C<dynamic>(42); // don't infer

https://codereview.chromium.org/2115173002/diff/60001/pkg/analyzer/test/src/t...
pkg/analyzer/test/src/task/strong/inferred_type_test.dart:599: int y = x.t;
Is the second line testing anything?  I think it would pass silently whether
inference happened or not.

[Jennifer Messerly, 2016-07-12 17:45:56]

Thanks Leaf. I'm going to follow up on the tests & re-send

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

https://codereview.chromium.org/2115173002/diff/60001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1974: if
(originalElement is! ConstructorMember) {
On 2016/07/12 17:23:26, Leaf wrote:
> Is this just (possibly justified :) paranoia, or can this happen?  In
malformed
> code maybe?  Worth a comment?

yeah great point, worth a comment for sure (I was confused too on reading this
again!)

I think it can happen because we haven't (yet) checked if the constructor is
generic or not. A non-generic constructor will just be a "raw"
ConstructorElement.

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

https://codereview.chromium.org/2115173002/diff/60001/pkg/analyzer/test/src/t...
pkg/analyzer/test/src/task/strong/inferred_type_test.dart:594: T t;
On 2016/07/12 17:23:26, Leaf wrote:
> Maybe test a named constructor?  Redirecting constructor?  Const constructor?

yes! Excellent ideas, I will add all of those (+ factory & redirecting-factory
constructors).

https://codereview.chromium.org/2115173002/diff/60001/pkg/analyzer/test/src/t...
pkg/analyzer/test/src/task/strong/inferred_type_test.dart:599: int y = x.t;
On 2016/07/12 17:23:26, Leaf wrote:
> Is the second line testing anything?  I think it would pass silently whether
> inference happened or not.

it's kind of in our "old" test style, where you do an assignment and verify that
there's no /*info:ASSIGNMENT_CAST*/ hint. Those hints don't show up to users but
they're part of our checker_test/inferred_type_test flow.

That said, I've been slowly converting them to where we get back the resolved
element model and check the variable (in this case, "x") has the expected type.
So I'll take this as encouragement to do that here =)


On 2016/07/12 17:23:26, Leaf wrote:
> Maybe some tests for some varieties of cases?  Specifically maybe:
> 
> num y;
> C<int> x = new C(y);
> C<num> x = new C(42);
> 
> var x = new C<dynamic>(42); // don't infer

Great ideas! I'll add these as well.

[Jennifer Messerly, 2016-07-12 17:52:50]

RE: breaking change ... yeah, great point too. I'll add a note to the CHANGELOG.

The good news is it should generally make compile/runtime errors go away, but it
will definitely break cases like:

    var list = new List.filled(1, "hi");
    list[0] = 42;

... similar to how `["hi"]..add(42)` is currently rejected by strong mode.

[Jennifer Messerly, 2016-07-12 19:16:45]

Tests added. One interesting case I'd be curious to hear your thoughts about.

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

https://codereview.chromium.org/2115173002/diff/80001/pkg/analyzer/test/src/t...
pkg/analyzer/test/src/task/strong/inferred_type_test.dart:604: C<num> c_num =
/*info:INFERRED_TYPE_ALLOCATION,info:INFERRED_TYPE_ALLOCATION*/new C(42);
So here we're actually tightening the constructed type to C<int>. That's
compatible with what generic method inference would do, due to our heuristics
(constraints are: int <: T <: num, but it picks `int` to make the return type
tighter). We may want to revisit that (or dig in now?), as we have some TODOs in
inference code about it.

[Leaf, 2016-07-12 19:54:54]

still lgtm.

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

https://codereview.chromium.org/2115173002/diff/80001/pkg/analyzer/test/src/t...
pkg/analyzer/test/src/task/strong/inferred_type_test.dart:604: C<num> c_num =
/*info:INFERRED_TYPE_ALLOCATION,info:INFERRED_TYPE_ALLOCATION*/new C(42);
On 2016/07/12 19:16:44, John Messerly wrote:
> So here we're actually tightening the constructed type to C<int>. That's
> compatible with what generic method inference would do, due to our heuristics
> (constraints are: int <: T <: num, but it picks `int` to make the return type
> tighter). We may want to revisit that (or dig in now?), as we have some TODOs
in
> inference code about it.

Yes, this is an interesting one.  My initial thought was that we should treat
the context type as user intent and stick with that, but now I'm not sure.  It's
also reasonable to try to pick the most specific type possible.   I don't feel
like I have a knockdown argument either way right now.  If we had sound
variance, we would have clear guidance, but with the existing unsound variance
no answer is always right.  I'd say run with this for now.

[Jennifer Messerly, 2016-07-12 20:36:06]

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

[Jennifer Messerly, 2016-07-12 20:36:06]

Hi Paul,

I added a test skip here for AST based summaries:
https://codereview.chromium.org/2115173002/diff2/80001:100001/pkg/analyzer/te...

Does this seem okay to you? I thought it would be based on our past
conversations, but I wanted to double check :)

There's a TODO in link.dart, _doInvokeConstructor, about using the argument
types, so it looks like it maybe wasn't doing inference there yet?

A related issue is maybe I shouldn't be verifying local variable types in my
test? It looks like AST summaries don't infer those (the variables come back as
"dynamic" not "C<dynamic>"). I could change the tests to include top-level
variables if that would be better.

Thanks!
- John


(also note-to-self, I keep forgetting about these tests until my final presubmit
pass, need to stop doing that!)

[Paul Berry, 2016-07-12 20:59:34]

On 2016/07/12 20:36:06, John Messerly wrote:
> Hi Paul,
> 
> I added a test skip here for AST based summaries:
>
https://codereview.chromium.org/2115173002/diff2/80001:100001/pkg/analyzer/te...
> 
> Does this seem okay to you? I thought it would be based on our past
> conversations, but I wanted to double check :)

For the short term I think this is ok.  We'll need to fix the tests before
switching on AST-based summaries (which I hope to do soon) but I don't mind
letting you move forward with the CL as is, and I can play catch-up once I have
time.

> 
> There's a TODO in link.dart, _doInvokeConstructor, about using the argument
> types, so it looks like it maybe wasn't doing inference there yet?

Yup, that's exactly what that TODO is about :)

> 
> A related issue is maybe I shouldn't be verifying local variable types in my
> test? It looks like AST summaries don't infer those (the variables come back
as
> "dynamic" not "C<dynamic>"). I could change the tests to include top-level
> variables if that would be better.

Yes, changing them to top level variables would be very helpful; AST-based
summaries don't infer types on local variables because local variable types
aren't visible outside the method (and hence can't affect type inference in
other libraries).  If we left the tests as is we would have a gap in our test
coverage for AST-based summaries.

Thanks!

[Jennifer Messerly, 2016-07-12 23:13:03]

On 2016/07/12 20:59:34, Paul Berry wrote:
> On 2016/07/12 20:36:06, John Messerly wrote:
> > Hi Paul,
> > 
> > I added a test skip here for AST based summaries:
> >
>
https://codereview.chromium.org/2115173002/diff2/80001:100001/pkg/analyzer/te...
> > 
> > Does this seem okay to you? I thought it would be based on our past
> > conversations, but I wanted to double check :)
> 
> For the short term I think this is ok.  We'll need to fix the tests before
> switching on AST-based summaries (which I hope to do soon) but I don't mind
> letting you move forward with the CL as is, and I can play catch-up once I
have
> time.
> 
> > 
> > There's a TODO in link.dart, _doInvokeConstructor, about using the argument
> > types, so it looks like it maybe wasn't doing inference there yet?
> 
> Yup, that's exactly what that TODO is about :)
> 
> > 
> > A related issue is maybe I shouldn't be verifying local variable types in my
> > test? It looks like AST summaries don't infer those (the variables come back
> as
> > "dynamic" not "C<dynamic>"). I could change the tests to include top-level
> > variables if that would be better.
> 
> Yes, changing them to top level variables would be very helpful; AST-based
> summaries don't infer types on local variables because local variable types
> aren't visible outside the method (and hence can't affect type inference in
> other libraries).  If we left the tests as is we would have a gap in our test
> coverage for AST-based summaries.
> 
> Thanks!

Thanks Paul! I've updated the tests to be summary friendly (move the variables
to top level).

Also found an interesting case of our "inferred type hints" getting swallowed,
but Leaf said he's familiar with it and it isn't new. Fortunately these tests
are not actually relying on those hints for correctness. :)

Fingers crossed but I'm going to take a shot at landing this. :)

Let me know if I can help with any AST-based inference. (Oh, and it's super cool
that reusing the typeSystem.infer* methods worked. Neat to know the same
generic-method-inference code kicks in for both :) )

[Jennifer Messerly, 2016-07-12 23:16:30]

Description was changed from

==========
fix #25220, infer generic type from constructor arguments

this makes it consistent in how constructors, generic functions, and literals
are treated

includes changes in https://codereview.chromium.org/2115743002/
==========

to

==========
fix #25220, infer generic type from constructor arguments

this makes it consistent in how constructors, generic functions, and literals
are treated

includes changes in https://codereview.chromium.org/2115743002/

R=leafp@google.com

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

[Jennifer Messerly, 2016-07-12 23:16:34]

Committed patchset #6 (id:120001) manually as
8d2c6d033afd3427a28b3e0dc2d53649db404db8 (presubmit successful).