Consider initializing formals during top-level type inference

The previous implementation assumed that initializing formals without
type annotations could be inferred after the rest of top-level type
inference was completely done.  This is not necessarily the case
because inferred types for initializing formals depend on inferred
types for fields which in turn depend on inferred types for
constructors used in their initializers.

Fixes #32866

Change-Id: I9de0b865c740d7542e5f5ad3d62c4c47c4532266
Reviewed-on: https://dart-review.googlesource.com/60140
Commit-Queue: Kevin Millikin <[email protected]>
Reviewed-by: Dmitry Stefantsov <[email protected]>

Author: Kevin Millikin

pkg/front_end/lib/src/fasta/kernel/kernel_formal_parameter_builder.dart

@@ -46,10 +46,6 @@ class KernelFormalParameterBuilder
           isFieldFormal: hasThis,
           isCovariant: isCovariant)
         ..fileOffset = charOffset;
-      if (type == null && hasThis) {
-        library.loader.typeInferenceEngine
-            .recordInitializingFormal(declaration);
-      }
     }
     return declaration;
   }

pkg/front_end/lib/src/fasta/kernel/kernel_procedure_builder.dart

@@ -444,6 +444,15 @@ class KernelConstructorBuilder extends KernelFunctionBuilder {
     return isRedirectingGenerativeConstructorImplementation(constructor);
   }
 
+  bool get isEligibleForTopLevelInference {
+    if (formals != null) {
+      for (var formal in formals) {
+        if (formal.type == null && formal.hasThis) return true;
+      }
+    }
+    return false;
+  }
+
   Constructor build(SourceLibraryBuilder library) {
     if (constructor.name == null) {
       constructor.function = buildFunction(library);
@@ -455,6 +464,14 @@ class KernelConstructorBuilder extends KernelFunctionBuilder {
       constructor.isExternal = isExternal;
       constructor.name = new Name(name, library.target);
     }
+    if (!library.disableTypeInference && isEligibleForTopLevelInference) {
+      for (KernelFormalParameterBuilder formal in formals) {
+        if (formal.type == null && formal.hasThis) {
+          formal.declaration.type = null;
+        }
+      }
+      library.loader.typeInferenceEngine.toBeInferred[constructor] = library;
+    }
     return constructor;
   }
 

pkg/front_end/lib/src/fasta/kernel/kernel_shadow_ast.dart

@@ -34,7 +34,10 @@ import '../../base/instrumentation.dart'
         InstrumentationValueForTypeArgs;
 
 import '../fasta_codes.dart'
-    show noLength, templateCantUseSuperBoundedTypeForInstanceCreation;
+    show
+        noLength,
+        templateCantInferTypeDueToCircularity,
+        templateCantUseSuperBoundedTypeForInstanceCreation;
 
 import '../problems.dart' show unhandled, unsupported;
 
@@ -607,6 +610,36 @@ class ShadowConstructorInvocation extends ConstructorInvocation
 
   @override
   DartType _inferExpression(ShadowTypeInferrer inferrer, DartType typeContext) {
+    var library = inferrer.engine.beingInferred[target];
+    if (library != null) {
+      // There is a cyclic dependency where inferring the types of the
+      // initializing formals of a constructor required us to infer the
+      // corresponding field type which required us to know the type of the
+      // constructor.
+      var name = target.enclosingClass.name;
+      if (target.name.name != '') name += '.${target.name.name}';
+      library.addProblem(
+          templateCantInferTypeDueToCircularity.withArguments(name),
+          target.fileOffset,
+          name.length,
+          target.fileUri);
+      for (var declaration in target.function.positionalParameters) {
+        declaration.type ??= const DynamicType();
+      }
+      for (var declaration in target.function.namedParameters) {
+        declaration.type ??= const DynamicType();
+      }
+    } else if ((library = inferrer.engine.toBeInferred[target]) != null) {
+      inferrer.engine.toBeInferred.remove(target);
+      inferrer.engine.beingInferred[target] = library;
+      for (var declaration in target.function.positionalParameters) {
+        inferrer.engine.inferInitializingFormal(declaration, target);
+      }
+      for (var declaration in target.function.namedParameters) {
+        inferrer.engine.inferInitializingFormal(declaration, target);
+      }
+      inferrer.engine.beingInferred.remove(target);
+    }
     var inferredType = inferrer.inferInvocation(
         typeContext,
         fileOffset,
@@ -807,7 +840,7 @@ class ShadowFactoryConstructorInvocation extends StaticInvocation
 /// Concrete shadow object representing a field in kernel form.
 class ShadowField extends Field implements ShadowMember {
   @override
-  InferenceNode _inferenceNode;
+  InferenceNode inferenceNode;
 
   ShadowTypeInferrer _typeInferrer;
 
@@ -823,13 +856,13 @@ class ShadowField extends Field implements ShadowMember {
   }
 
   static bool hasTypeInferredFromInitializer(ShadowField field) =>
-      field._inferenceNode is FieldInitializerInferenceNode;
+      field.inferenceNode is FieldInitializerInferenceNode;
 
   static bool isImplicitlyTyped(ShadowField field) => field._isImplicitlyTyped;
 
   static void setInferenceNode(ShadowField field, InferenceNode node) {
-    assert(field._inferenceNode == null);
-    field._inferenceNode = node;
+    assert(field.inferenceNode == null);
+    field.inferenceNode = node;
   }
 }
 
@@ -1428,18 +1461,18 @@ class ShadowMapLiteral extends MapLiteral implements ShadowExpression {
 abstract class ShadowMember implements Member {
   Uri get fileUri;
 
-  InferenceNode get _inferenceNode;
+  InferenceNode get inferenceNode;
 
-  void set _inferenceNode(InferenceNode value);
+  void set inferenceNode(InferenceNode value);
 
   void setInferredType(
       TypeInferenceEngine engine, Uri uri, DartType inferredType);
 
   static void resolveInferenceNode(Member member) {
     if (member is ShadowMember) {
-      if (member._inferenceNode != null) {
-        member._inferenceNode.resolve();
-        member._inferenceNode = null;
+      if (member.inferenceNode != null) {
+        member.inferenceNode.resolve();
+        member.inferenceNode = null;
       }
     }
   }
@@ -1568,7 +1601,7 @@ class ShadowNullLiteral extends NullLiteral implements ShadowExpression {
 /// Concrete shadow object representing a procedure in kernel form.
 class ShadowProcedure extends Procedure implements ShadowMember {
   @override
-  InferenceNode _inferenceNode;
+  InferenceNode inferenceNode;
 
   final bool _hasImplicitReturnType;
 
@@ -1757,9 +1790,9 @@ class ShadowStaticAssignment extends ShadowComplexAssignment {
     if (write is StaticSet) {
       writeContext = write.target.setterType;
       writeMember = write.target;
-      if (writeMember is ShadowField && writeMember._inferenceNode != null) {
-        writeMember._inferenceNode.resolve();
-        writeMember._inferenceNode = null;
+      if (writeMember is ShadowField && writeMember.inferenceNode != null) {
+        writeMember.inferenceNode.resolve();
+        writeMember.inferenceNode = null;
       }
     }
     var inferredResult = _inferRhs(inferrer, readType, writeContext);
@@ -1776,9 +1809,9 @@ class ShadowStaticGet extends StaticGet implements ShadowExpression {
   @override
   DartType _inferExpression(ShadowTypeInferrer inferrer, DartType typeContext) {
     var target = this.target;
-    if (target is ShadowField && target._inferenceNode != null) {
-      target._inferenceNode.resolve();
-      target._inferenceNode = null;
+    if (target is ShadowField && target.inferenceNode != null) {
+      target.inferenceNode.resolve();
+      target.inferenceNode = null;
     }
     var type = target.getterType;
     if (target is Procedure && target.kind == ProcedureKind.Method) {

pkg/front_end/lib/src/fasta/kernel/kernel_target.dart

@@ -514,10 +514,12 @@ class KernelTarget extends TargetImplementation {
       Constructor constructor, Map<TypeParameter, DartType> substitutionMap) {
     VariableDeclaration copyFormal(VariableDeclaration formal) {
       // TODO(ahe): Handle initializers.
-      return new VariableDeclaration(formal.name,
-          type: substitute(formal.type, substitutionMap),
-          isFinal: formal.isFinal,
-          isConst: formal.isConst);
+      var copy = new VariableDeclaration(formal.name,
+          isFinal: formal.isFinal, isConst: formal.isConst);
+      if (formal.type != null) {
+        copy.type = substitute(formal.type, substitutionMap);
+      }
+      return copy;
     }
 
     List<VariableDeclaration> positionalParameters = <VariableDeclaration>[];

pkg/front_end/lib/src/fasta/type_inference/type_inference_engine.dart

@@ -4,29 +4,26 @@
 
 import 'package:kernel/ast.dart'
     show
-        Class,
+        Constructor,
         DartType,
         DartTypeVisitor,
         DynamicType,
         FunctionType,
         InterfaceType,
-        Location,
         TypeParameter,
         TypeParameterType,
-        TypedefType;
+        TypedefType,
+        VariableDeclaration;
 import 'package:kernel/class_hierarchy.dart';
 import 'package:kernel/core_types.dart';
 
 import '../../base/instrumentation.dart';
 
 import '../builder/library_builder.dart';
 
-import '../deprecated_problems.dart' show Crash;
-
 import '../kernel/kernel_shadow_ast.dart';
 
-import '../messages.dart'
-    show getLocationFromNode, noLength, templateCantInferTypeDueToCircularity;
+import '../messages.dart' show noLength, templateCantInferTypeDueToCircularity;
 
 import '../source/source_library_builder.dart';
 
@@ -211,7 +208,20 @@ abstract class TypeInferenceEngine {
 
   final staticInferenceNodes = <FieldInitializerInferenceNode>[];
 
-  final initializingFormals = <ShadowVariableDeclaration>[];
+  /// A map containing constructors with initializing formals whose types
+  /// need to be inferred.
+  ///
+  /// This is represented as a map from a constructor to its library
+  /// builder because the builder is used to report errors due to cyclic
+  /// inference dependencies.
+  final Map<Constructor, LibraryBuilder> toBeInferred = {};
+
+  /// A map containing constructors in the process of being inferred.
+  ///
+  /// This is used to detect cyclic inference dependencies.  It is represented
+  /// as a map from a constructor to its library builder because the builder
+  /// is used to report errors.
+  final Map<Constructor, LibraryBuilder> beingInferred = {};
 
   final Instrumentation instrumentation;
 
@@ -248,20 +258,37 @@ abstract class TypeInferenceEngine {
   }
 
   /// Performs the third phase of top level inference, which is to visit all
-  /// initializing formals and infer their types (if necessary) from the
-  /// corresponding fields.
+  /// constructors still needing inference and infer the types of their
+  /// initializing formals from the corresponding fields.
   void finishTopLevelInitializingFormals() {
-    for (ShadowVariableDeclaration formal in initializingFormals) {
-      try {
-        formal.type = _inferInitializingFormalType(formal);
-      } catch (e, s) {
-        Location location = getLocationFromNode(formal);
-        if (location == null) {
-          rethrow;
-        } else {
-          throw new Crash(location.file, formal.fileOffset, e, s);
+    // Field types have all been inferred so there cannot be a cyclic
+    // dependency.
+    for (Constructor constructor in toBeInferred.keys) {
+      for (var declaration in constructor.function.positionalParameters) {
+        inferInitializingFormal(declaration, constructor);
+      }
+      for (var declaration in constructor.function.namedParameters) {
+        inferInitializingFormal(declaration, constructor);
+      }
+    }
+    toBeInferred.clear();
+  }
+
+  void inferInitializingFormal(VariableDeclaration formal, Constructor parent) {
+    if (formal.type == null) {
+      for (ShadowField field in parent.enclosingClass.fields) {
+        if (field.name.name == formal.name) {
+          if (field.inferenceNode != null) {
+            field.inferenceNode.resolve();
+          }
+          formal.type = field.type;
+          return;
         }
       }
+      // We did not find the corresponding field, so the program is erroneous.
+      // The error should have been reported elsewhere and type inference
+      // should continue by inferring dynamic.
+      formal.type = const DynamicType();
     }
   }
 
@@ -274,33 +301,11 @@ abstract class TypeInferenceEngine {
         new TypeSchemaEnvironment(coreTypes, hierarchy, strongMode);
   }
 
-  /// Records that the given initializing [formal] will need top level type
-  /// inference.
-  void recordInitializingFormal(ShadowVariableDeclaration formal) {
-    initializingFormals.add(formal);
-  }
-
   /// Records that the given static [field] will need top level type inference.
   void recordStaticFieldInferenceCandidate(
       ShadowField field, LibraryBuilder library) {
     var node = new FieldInitializerInferenceNode(this, field, library);
     ShadowField.setInferenceNode(field, node);
     staticInferenceNodes.add(node);
   }
-
-  DartType _inferInitializingFormalType(ShadowVariableDeclaration formal) {
-    assert(ShadowVariableDeclaration.isImplicitlyTyped(formal));
-    var enclosingClass = formal.parent?.parent?.parent;
-    if (enclosingClass is Class) {
-      for (var field in enclosingClass.fields) {
-        if (field.name.name == formal.name) {
-          return field.type;
-        }
-      }
-    }
-    // No matching field, or something else has gone wrong (e.g. initializing
-    // formal outside of a class declaration).  The error should be reported
-    // elsewhere, so just infer `dynamic`.
-    return const DynamicType();
-  }
 }

pkg/front_end/testcases/circularity-via-initializing-formal.dart

@@ -0,0 +1,18 @@
+// Copyright (c) 2018, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+// This test is intended to trigger a circular top-level type-inference
+// dependency involving an initializing formal where the circularity is detected
+// when inferring the type of the constructor.
+//
+// The compiler should generate an error message and it should be properly
+// formatted including offset and lenght of the constructor.
+var x = new C._circular(null);
+
+class C {
+  var f = new C._circular(null);
+  C._circular(this.f);
+}
+
+main() {}

pkg/front_end/testcases/circularity-via-initializing-formal.dart.direct.expect

@@ -0,0 +1,12 @@
+library;
+import self as self;
+import "dart:core" as core;
+
+class C extends core::Object {
+  field dynamic f = new self::C::_circular(null);
+  constructor _circular(dynamic f) → void
+    : self::C::f = f, super core::Object::•()
+    ;
+}
+static field dynamic x = new self::C::_circular(null);
+static method main() → dynamic {}

pkg/front_end/testcases/circularity-via-initializing-formal.dart.direct.transformed.expect

@@ -0,0 +1,12 @@
+library;
+import self as self;
+import "dart:core" as core;
+
+class C extends core::Object {
+  field dynamic f = new self::C::_circular(null);
+  constructor _circular(dynamic f) → void
+    : self::C::f = f, super core::Object::•()
+    ;
+}
+static field dynamic x = new self::C::_circular(null);
+static method main() → dynamic {}

pkg/front_end/testcases/circularity-via-initializing-formal.dart.outline.expect

@@ -0,0 +1,12 @@
+library;
+import self as self;
+import "dart:core" as core;
+
+class C extends core::Object {
+  field dynamic f;
+  constructor _circular(dynamic f) → void
+    ;
+}
+static field dynamic x;
+static method main() → dynamic
+  ;