fix #25794, infer parameter type from default value

fix #25794, infer parameter type from default value

R=leafp@google.com

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

[Jennifer Messerly, 2016-06-23 20:14:45]

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

[Jennifer Messerly, 2016-06-23 20:14:46]

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

https://codereview.chromium.org/2092873002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/resolver.dart:6309:
InferenceContext.setType(node.defaultValue, element.type);
I verified with an assert that `node.parameter.element == node.element` for all
of Analyzer's tests.
(which matched my understanding of the element/AST structure)

https://codereview.chromium.org/2092873002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/resolver.dart:6318: element is!
FieldFormalParameterElement) {
I experimented with allowing field formal parameter to be inferred to have a
tighter type than its field -- there is another error code that will catch
incompatible types (between the field and parameter) for us if that happens. But
it seemed like mostly an unhelpful feature (why would you want your parameter to
be more restricted than its backing field? seems to only work for if you plan to
mutate that field later to a wider type.), so removed it.

[Leaf, 2016-06-23 22:21:17]

lgtm

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

https://codereview.chromium.org/2092873002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/resolver.dart:6321: if (!type.isBottom &&
!type.isDynamic) {
Are we quite sure that type can't be null here?

https://codereview.chromium.org/2092873002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/resolver.dart:6322: (element as
ParameterElementImpl).type = type;
I'm been trying to record all inference events, even though we're not really
using them yet except for testing.  Probably worth doing so here.

[Jennifer Messerly, 2016-06-24 17:35:35]

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

[Jennifer Messerly, 2016-06-24 18:33:59]

Hi Paul, wanted to get your thoughts on this change.

I've fixed a bug you originally caught: we weren't correctly inferring parameter
types from default values. I fixed it in Resolver and it seems to be fixed in
the Element-based summaries too.

I haven't figured out how to fix it yet in AST based summaries. Would one option
would be to land it with that test marked failing? Or if you have any advice on
how to fix it there. I think I figured out how to get the inferredTypeSlot in
the unlinkedSummary, but I haven't figured out how to fix
ParameterElementForLink to compute its type yet. It seems that perhaps I can
take a similar approach to VariableElementForLink, but I might be way out of my
depth here :)

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

https://codereview.chromium.org/2092873002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/resolver.dart:6321: if (!type.isBottom &&
!type.isDynamic) {
On 2016/06/23 22:21:17, Leaf wrote:
> Are we quite sure that type can't be null here?

I think so? :) after resolving an expression, the invariant is supposed to be
that staticType is non-null. We visit the default value right above.

That said.... I wouldn't say I'm quite sure that any field in Analyzer
AST/Element model isn't null :)

I'm fine adding a check if you think it would be a good idea. Or we can leave it
off and see if it ever gets hit.

https://codereview.chromium.org/2092873002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/resolver.dart:6322: (element as
ParameterElementImpl).type = type;
On 2016/06/23 22:21:17, Leaf wrote:
> I'm been trying to record all inference events, even though we're not really
> using them yet except for testing.  Probably worth doing so here.

Yeah that's a good idea ... the only strange part is recordInference is
currently typed to require Expressions, and DefaultFormalParameter isn't an
Expression. So I widened it to accept any AstNode. It looks like that works, but
not sure if this is breaking some invariant that was supposed to be there.

[Paul Berry, 2016-06-24 18:48:20]

On 2016/06/24 18:33:59, John Messerly wrote:
> Hi Paul, wanted to get your thoughts on this change.
> 
> I've fixed a bug you originally caught: we weren't correctly inferring
parameter
> types from default values. I fixed it in Resolver and it seems to be fixed in
> the Element-based summaries too.
> 
> I haven't figured out how to fix it yet in AST based summaries. Would one
option
> would be to land it with that test marked failing? Or if you have any advice
on
> how to fix it there. I think I figured out how to get the inferredTypeSlot in
> the unlinkedSummary, but I haven't figured out how to fix
> ParameterElementForLink to compute its type yet. It seems that perhaps I can
> take a similar approach to VariableElementForLink, but I might be way out of
my
> depth here :)

I think you're on the right track but I haven't touched the AST-based summary
code in a few weeks so my knowledge is swapped out to long-term storage :)

I think it's fine to land the CL as is for now and work on the AST-based summary
logic in a follow-up commit.  If you get stuck let me or Konstantin know and we
can help.

[Leaf, 2016-06-24 21:48:53]

still lgtm

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

https://codereview.chromium.org/2092873002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/resolver.dart:6321: if (!type.isBottom &&
!type.isDynamic) {
On 2016/06/24 18:33:59, John Messerly wrote:
> On 2016/06/23 22:21:17, Leaf wrote:
> > Are we quite sure that type can't be null here?
> 
> I think so? :) after resolving an expression, the invariant is supposed to be
> that staticType is non-null. We visit the default value right above.
> 
> That said.... I wouldn't say I'm quite sure that any field in Analyzer
> AST/Element model isn't null :)
> 
> I'm fine adding a check if you think it would be a good idea. Or we can leave
it
> off and see if it ever gets hit.

 I'm ok leaving it.  Just wanted to check since we didn't used to inspect the
type.

[Jennifer Messerly, 2016-06-24 22:25:15]

Thanks! I'll take a shot at landing this but will keep working on the AST
summaries (have made some progress, I think, in another branch :) )

[Jennifer Messerly, 2016-06-24 22:29:07]

Description was changed from

==========
fix #25794, infer parameter type from default value
==========

to

==========
fix #25794, infer parameter type from default value

R=leafp@google.com

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

[Jennifer Messerly, 2016-06-24 22:29:13]

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

[Jennifer Messerly, 2016-06-28 19:50:07]

Hmmm, this change regressed https://github.com/dart-lang/sdk/issues/26785,
apparently Dart allows cyclic default values(!) despite not supporting cyclic
constants generally.

Thoughts? Maybe I should revert this.

[Paul Berry, 2016-06-28 21:18:33]

On 2016/06/28 19:50:07, John Messerly wrote:
> Hmmm, this change regressed https://github.com/dart-lang/sdk/issues/26785,
> apparently Dart allows cyclic default values(!) despite not supporting cyclic
> constants generally.
> 
> Thoughts? Maybe I should revert this.

Oh, that's unfortunate.

In the long run I think we could address this situation similarly to how we
address circularities among toplevel variables (just let everything involved in
the circularity be dynamic).  But if that's hard to achieve in the short term I
would be ok with reverting the CL for now.