Resynthesize constructors lazily.

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 7c1e056917cbf7af237cbf95e6229ffb651e92c1..a0c0c4ccd46b510bf509cff6af3b9f2a214a17c0 100644
--- a/pkg/analyzer/lib/src/dart/element/element.dart
+++ b/pkg/analyzer/lib/src/dart/element/element.dart
@@ -527,11 +527,26 @@ class ClassElementImpl extends AbstractClassElementImpl
 
   @override
   List<ConstructorElement> get constructors {
-    if (!isMixinApplication) {
-      assert(_constructors != null);
-      return _constructors ?? ConstructorElement.EMPTY_LIST;
+    if (isMixinApplication) {
+      return _computeMixinAppConstructors();
     }
-    return _computeMixinAppConstructors();
+    if (_unlinkedClass != null && _constructors == null) {
+      _constructors = _unlinkedClass.executables
+          .where((e) => e.kind == UnlinkedExecutableKind.constructor)
+          .map((e) => new ConstructorElementImpl.forSerialized(e, this))
+          .toList(growable: false);
+      // Ensure at least implicit default constructor.
+      if (_constructors.isEmpty) {
+        ConstructorElementImpl constructor = new ConstructorElementImpl('', -1);
+        constructor.enclosingElement = this;
+        constructor.synthetic = true;
+        constructor.type = new FunctionTypeImpl.elementWithNameAndArgs(
+            constructor, null, type.typeArguments, false);
+        _constructors = <ConstructorElement>[constructor];
+      }
+    }
+    assert(_constructors != null);
+    return _constructors ?? const <ConstructorElement>[];

[Brian Wilkerson, 2016/06/03 16:06:34]

This doesn't make sense. If _constructors is not null why have the null guard?

[scheglov, 2016/06/03 16:10:38]

I agree, but this change just keeps the previous semantics.
I guess the idea was that at development time assert will fail.
But for clients we will silently return an empty list.
I don't have strong opinion whether we need this.

[Brian Wilkerson, 2016/06/03 16:18:15]

Ok, I see. Are we doing the same everywhere that we're lazily populating a field
in the element model? Seems like we ought to be consistent.

[Paul Berry, 2016/06/03 16:21:50]

Yes, that was my intention when I wrote this code, and I believe we should keep
it this way.  Consider the alternatives:

1. Defensive--hides bugs and makes it unclear to the reader whether
_constructors is intended to be nullable:

    return _constructors ?? const<ConstructorElement>[];

2. Fail fast--finds bugs, but in the event that a bug slips out to production,
is more likely to manifest in an unrecoverable crash:

    assert(_constructors != null);
    return _constructors;

To get both the defensiveness and the ability to find bugs quickly we need both
the assert and the null guard.

   }
 
   /**
@@ -540,6 +555,7 @@ class ClassElementImpl extends AbstractClassElementImpl
    * Should only be used for class elements that are not mixin applications.
    */
   void set constructors(List<ConstructorElement> constructors) {
+    assert(_unlinkedClass == null);
     assert(!isMixinApplication);
     for (ConstructorElement constructor in constructors) {
       (constructor as ConstructorElementImpl).enclosingElement = this;
@@ -1678,11 +1694,6 @@ class ConstLocalVariableElementImpl extends LocalVariableElementImpl
 class ConstructorElementImpl extends ExecutableElementImpl
     implements ConstructorElement {
   /**
-   * Set of slots corresponding to const constructors that are part of cycles.
-   */
-  final Set<int> _constCycles;
-
-  /**
    * The constructor to which this constructor is redirecting.
    */
   ConstructorElement _redirectedConstructor;
@@ -1714,22 +1725,18 @@ class ConstructorElementImpl extends ExecutableElementImpl
    * Initialize a newly created constructor element to have the given [name] and
    * [offset].
    */
-  ConstructorElementImpl(String name, int offset)
-      : _constCycles = null,
-        super(name, offset);
+  ConstructorElementImpl(String name, int offset) : super(name, offset);
 
   /**
    * Initialize a newly created constructor element to have the given [name].
    */
-  ConstructorElementImpl.forNode(Identifier name)
-      : _constCycles = null,
-        super.forNode(name);
+  ConstructorElementImpl.forNode(Identifier name) : super.forNode(name);
 
   /**
    * Initialize using the given serialized information.
    */
-  ConstructorElementImpl.forSerialized(UnlinkedExecutable serializedExecutable,
-      this._constCycles, ClassElementImpl enclosingClass)
+  ConstructorElementImpl.forSerialized(
+      UnlinkedExecutable serializedExecutable, ClassElementImpl enclosingClass)
       : super.forSerialized(serializedExecutable, enclosingClass);
 
   /**
@@ -1782,7 +1789,8 @@ class ConstructorElementImpl extends ExecutableElementImpl
   bool get isCycleFree {
     if (serializedExecutable != null) {
       return serializedExecutable.isConst &&
-          !_constCycles.contains(serializedExecutable.constCycleSlot);
+          !enclosingUnit.resynthesizerContext
+              .isInConstCycle(serializedExecutable.constCycleSlot);
     }
     return _isCycleFree;
   }
@@ -7394,6 +7402,11 @@ abstract class ResynthesizerContext {
   UnitExplicitTopLevelVariables buildTopLevelVariables();
 
   /**
+   * Return `true` if the given const constructor [slot] is a part of cycle.
+   */
+  bool isInConstCycle(int slot);
+
+  /**
    * Resolve an [EntityRef] into a constructor.  If the reference is
    * unresolved, return `null`.
    */