Resynthesize field formal parameters from Kernel.

pkg/analyzer/lib/src/dart/element/element.dart

Index: pkg/analyzer/lib/src/dart/element/element.dart
diff --git a/pkg/analyzer/lib/src/dart/element/element.dart b/pkg/analyzer/lib/src/dart/element/element.dart
index 9563b27f54ee02b4b848e4d9d07c6670b114ce1d..44bf2ad6e5072c0e4c8f8e8e765ccff552a30423 100644
--- a/pkg/analyzer/lib/src/dart/element/element.dart
+++ b/pkg/analyzer/lib/src/dart/element/element.dart
@@ -1909,6 +1909,13 @@ class ConstFieldElementImpl extends FieldElementImpl with ConstVariableElement {
   ConstFieldElementImpl(String name, int offset) : super(name, offset);
 
   /**
+   * Initialize using the given kernel.
+   */
+  ConstFieldElementImpl.forKernel(
+      ElementImpl enclosingElement, kernel.Field kernel)
+      : super.forKernel(enclosingElement, kernel);
+
+  /**
    * Initialize a newly created field element to have the given [name].
    */
   ConstFieldElementImpl.forNode(Identifier name) : super.forNode(name);
@@ -1919,13 +1926,6 @@ class ConstFieldElementImpl extends FieldElementImpl with ConstVariableElement {
   ConstFieldElementImpl.forSerialized(
       UnlinkedVariable unlinkedVariable, ElementImpl enclosingElement)
       : super.forSerialized(unlinkedVariable, enclosingElement);
-
-  /**
-   * Initialize using the given kernel.
-   */
-  ConstFieldElementImpl.forKernel(
-      ElementImpl enclosingElement, kernel.Field kernel)
-      : super.forKernel(enclosingElement, kernel);
 }
 
 /**
@@ -2551,6 +2551,13 @@ class DefaultFieldFormalParameterElementImpl
       : super(name, nameOffset);
 
   /**
+   * Initialize using the given kernel.
+   */
+  DefaultFieldFormalParameterElementImpl.forKernel(ElementImpl enclosingElement,
+      kernel.VariableDeclaration kernel, ParameterKind parameterKind)
+      : super.forKernel(enclosingElement, kernel, parameterKind);
+
+  /**
    * Initialize a newly created parameter element to have the given [name].
    */
   DefaultFieldFormalParameterElementImpl.forNode(Identifier name)
@@ -2577,6 +2584,13 @@ class DefaultParameterElementImpl extends ParameterElementImpl
       : super(name, nameOffset);
 
   /**
+   * Initialize using the given kernel.
+   */
+  DefaultParameterElementImpl.forKernel(ElementImpl enclosingElement,
+      kernel.VariableDeclaration kernel, ParameterKind parameterKind)
+      : super.forKernel(enclosingElement, kernel, parameterKind);
+
+  /**
    * Initialize a newly created parameter element to have the given [name].
    */
   DefaultParameterElementImpl.forNode(Identifier name) : super.forNode(name);
@@ -4441,6 +4455,13 @@ class FieldFormalParameterElementImpl extends ParameterElementImpl
       : super(name, nameOffset);
 
   /**
+   * Initialize using the given kernel.
+   */
+  FieldFormalParameterElementImpl.forKernel(ElementImpl enclosingElement,
+      kernel.VariableDeclaration kernel, ParameterKind parameterKind)
+      : super.forKernel(enclosingElement, kernel, parameterKind);
+
+  /**
    * Initialize a newly created parameter element to have the given [name].
    */
   FieldFormalParameterElementImpl.forNode(Identifier name)
@@ -7937,19 +7958,59 @@ class ParameterElementImpl extends VariableElementImpl
       ElementImpl enclosingElement, kernel.FunctionNode function) {
     if (function.positionalParameters.isNotEmpty ||
         function.namedParameters.isNotEmpty) {
+      // Prepare all field formal parameters if a constructor.
+      var fieldFormalParameters = <kernel.VariableDeclaration>[];
+      {
+        var procedure = function.parent;
+        if (procedure is kernel.Constructor) {
+          for (kernel.Initializer initializer in procedure.initializers) {
+            if (initializer is kernel.FieldInitializer) {
+              kernel.Expression value = initializer.value;
+              if (value is kernel.VariableGet) {
+                fieldFormalParameters.add(value.variable);

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

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.

+              }
+            }
+          }
+        }
+      }
+
       var parameters = <ParameterElement>[];
+
+      // Add positional required and optional parameters.
       for (int i = 0; i < function.positionalParameters.length; i++) {
-        parameters.add(new ParameterElementImpl.forKernel(
-            enclosingElement,
-            function.positionalParameters[i],
-            i < function.requiredParameterCount
-                ? ParameterKind.REQUIRED
-                : ParameterKind.POSITIONAL));
+        kernel.VariableDeclaration parameter = function.positionalParameters[i];
+        bool isFieldFormal = fieldFormalParameters.contains(parameter);
+        if (i < function.requiredParameterCount) {
+          if (isFieldFormal) {
+            parameters.add(new FieldFormalParameterElementImpl.forKernel(
+                enclosingElement, parameter, ParameterKind.REQUIRED));
+          } else {
+            parameters.add(new ParameterElementImpl.forKernel(
+                enclosingElement, parameter, ParameterKind.REQUIRED));
+          }
+        } else {
+          if (isFieldFormal) {
+            parameters.add(new DefaultFieldFormalParameterElementImpl.forKernel(
+                enclosingElement, parameter, ParameterKind.POSITIONAL));
+          } else {
+            parameters.add(new DefaultParameterElementImpl.forKernel(
+                enclosingElement, parameter, ParameterKind.POSITIONAL));
+          }
+        }
       }
+
+      // Add named parameters.
       for (var k in function.namedParameters) {
-        parameters.add(new ParameterElementImpl.forKernel(
-            enclosingElement, k, ParameterKind.NAMED));
+        bool isFieldFormal = fieldFormalParameters.contains(k);
+        if (isFieldFormal) {
+          parameters.add(new DefaultFieldFormalParameterElementImpl.forKernel(
+              enclosingElement, k, ParameterKind.NAMED));
+        } else {
+          parameters.add(new DefaultParameterElementImpl.forKernel(
+              enclosingElement, k, ParameterKind.NAMED));
+        }
       }
+
       return parameters;
     } else {
       return const <ParameterElement>[];