Resynthesize field formal parameters from Kernel.

Resynthesize field formal parameters from Kernel.

R=brianwilkerson@google.com, paulberry@google.com
BUG=

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

[Brian Wilkerson, 2017-07-19 21:45:02]

lgtm

[Paul Berry, 2017-07-19 22:11:59]

https://codereview.chromium.org/2978353002/diff/1/pkg/analyzer/lib/src/dart/e...
File pkg/analyzer/lib/src/dart/element/element.dart (right):

https://codereview.chromium.org/2978353002/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/element.dart:7970:
fieldFormalParameters.add(value.variable);
It looks like what you're doing is: if you see `C(x) : this.x = x;` you assume
the user actually said `C(this.x);` and it got desugared to `C(x) : this.x = x`,
so you create a FieldFormalParameterElementImpl in the element model.

But what if the user really did say `C(x) : this.x = x` in the first place? 
Then the element model will be incorrect.

Another problem: for a non-const constructor I don't think we'll be storing the
constructor initializer expressions in the outline.  Which means that for
non-const constructors this approach will eventually stop working, once we start
using kernel summaries that have method bodies and constructor initializer
expressions stripped out.

In the end it shouldn't really matter, since the two ways of writing the
constructor are equivalent from an API perspective.  Probably most clients won't
care about the difference, and the few clients that do really ought to be using
the AST to examine the constructor rather than the element model.

But the people writing client code probably don't know that--they'll just do
whatever is easiest and appears to work.  Which means that if we try to guess
whether a parameter is a field formal parameter, and we don't always do it
reliably, people are likely to write bad client code without realizing it, since
their simple test cases may pass anyhow.  That seems like a dangerous situation.

I think that when resynthesizing from kernel, we should always resynthesize the
element model as though the user's code was `C(x) : this.x = x`.  If this causes
clients to break, at least it will break them consistently so they'll probably
notice it in testing, rather than having bizarre bugs in the field that are hard
to track down.  And if they complain to us that we've taken away functionality
they need, we can always explain why we had to remove it, and tell them that for
info like this they need to look at the constructor's AST.

[scheglov, 2017-07-19 23:19:01]

On 2017/07/19 22:11:59, Paul Berry wrote:
>
https://codereview.chromium.org/2978353002/diff/1/pkg/analyzer/lib/src/dart/e...
> File pkg/analyzer/lib/src/dart/element/element.dart (right):
> 
>
https://codereview.chromium.org/2978353002/diff/1/pkg/analyzer/lib/src/dart/e...
> pkg/analyzer/lib/src/dart/element/element.dart:7970:
> fieldFormalParameters.add(value.variable);
> It looks like what you're doing is: if you see `C(x) : this.x = x;` you assume
> the user actually said `C(this.x);` and it got desugared to `C(x) : this.x =
x`,
> so you create a FieldFormalParameterElementImpl in the element model.
> 
> But what if the user really did say `C(x) : this.x = x` in the first place? 
> Then the element model will be incorrect.
> 
> Another problem: for a non-const constructor I don't think we'll be storing
the
> constructor initializer expressions in the outline.  Which means that for
> non-const constructors this approach will eventually stop working, once we
start
> using kernel summaries that have method bodies and constructor initializer
> expressions stripped out.
> 
> In the end it shouldn't really matter, since the two ways of writing the
> constructor are equivalent from an API perspective.  Probably most clients
won't
> care about the difference, and the few clients that do really ought to be
using
> the AST to examine the constructor rather than the element model.
> 
> But the people writing client code probably don't know that--they'll just do
> whatever is easiest and appears to work.  Which means that if we try to guess
> whether a parameter is a field formal parameter, and we don't always do it
> reliably, people are likely to write bad client code without realizing it,
since
> their simple test cases may pass anyhow.  That seems like a dangerous
situation.
> 
> I think that when resynthesizing from kernel, we should always resynthesize
the
> element model as though the user's code was `C(x) : this.x = x`.  If this
causes
> clients to break, at least it will break them consistently so they'll probably
> notice it in testing, rather than having bizarre bugs in the field that are
hard
> to track down.  And if they complain to us that we've taken away functionality
> they need, we can always explain why we had to remove it, and tell them that
for
> info like this they need to look at the constructor's AST.

Thank you. Paul.

I have the same thoughts about clients - the only one which really cares is
ErrorVerifier, and it has access to nodes. There are several places in Analysis
Server where we also use FieldFormalParameterElement.field, but most of these
places also have AST. So, I actually started removing
FieldFormalParameterElement completely. But there was an issue with indexing, so
I stopped. However given that we don't have access to initializers, we have to
remove it.

I will send CLs for migration from FieldFormalParameterElement and its removal.

[scheglov, 2017-07-20 17:16:09]

PTAL

I tried to get FieldFormalParameterElement removed, but it is used to access
documentation for named parameters. So, I added
VariableDeclaration.isFieldFormal to make it explicit.

[Paul Berry, 2017-07-20 17:44:55]

lgtm, thanks!

[scheglov, 2017-07-20 17:59:59]

Description was changed from

==========
Resynthesize field formal parameters from Kernel.

R=brianwilkerson@google.com, paulberry@google.com
BUG=
==========

to

==========
Resynthesize field formal parameters from Kernel.

R=brianwilkerson@google.com, paulberry@google.com
BUG=

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

[scheglov, 2017-07-20 18:00:00]

Committed patchset #2 (id:20001) manually as
25e695a71ad868bf8435575fe52f30440b82475f (presubmit successful).