Complain about incorrect this/super constructor initializers.

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

Index: pkg/front_end/lib/src/fasta/kernel/body_builder.dart
diff --git a/pkg/front_end/lib/src/fasta/kernel/body_builder.dart b/pkg/front_end/lib/src/fasta/kernel/body_builder.dart
index 70ba073f995587f99f56683c9ab4f979d50fe57d..bb233f93bc698e6fa67f5f30ff44988870734a8b 100644
--- a/pkg/front_end/lib/src/fasta/kernel/body_builder.dart
+++ b/pkg/front_end/lib/src/fasta/kernel/body_builder.dart
@@ -79,6 +79,17 @@ class BodyBuilder extends ScopeListener<JumpTarget> implements BuilderHelper {
   @override
   final Uri uri;
 
+  /// Only used when [member] is a constructor. It tracks if an implicit super
+  /// initializer is needed.
+  ///
+  /// An implicit super initializer isn't needed
+  ///
+  /// 1. if the current class is Object,
+  /// 2. if there is an explicit super initializer,
+  /// 3. if there is a redirecting (this) initializer, or
+  /// 4. if an initializer will always throw.

[Paul Berry, 2017/04/07 15:56:38]

It sounds like you're talking about a case like this:

class C {
  var x;
  C() : x = (throw new UnimplementedError());
}

But it looks from line 396 like this would be treated as an Initializer, not a
Throw.  I'm guessing what's actually happening here is that you're trying to
make sure that if a compile-time error prevented us from generating code for an
initializer, there won't be a cascading error when we try to generate the
implicit super initializer call.

Assuming I'm correct, consider changing this line to:

4. if a compile-time error prevented us from generating code for an initializer.
 This avoids cascading errors.

[ahe, 2017/04/07 18:19:18]

Will do.

+  bool needsImplicitSuperInitializer;
+
   Scope formalParameterScope;
 
   bool inInitializer = false;
@@ -118,6 +129,8 @@ class BodyBuilder extends ScopeListener<JumpTarget> implements BuilderHelper {
       : enclosingScope = scope,
         library = library,
         isDartLibrary = library.uri.scheme == "dart",
+        needsImplicitSuperInitializer =
+            coreTypes.objectClass != classBuilder?.cls,
         super(scope);
 
   bool get hasParserError => recoverableErrors.isNotEmpty;
@@ -385,13 +398,15 @@ class BodyBuilder extends ScopeListener<JumpTarget> implements BuilderHelper {
     } else if (node is FastaAccessor) {
       initializer = node.buildFieldInitializer(fieldInitializers);
     } else if (node is ConstructorInvocation) {
-      initializer = new SuperInitializer(node.target, node.arguments);
+      initializer =
+          buildSuperInitializer(node.target, node.arguments, token.charOffset);
     } else {
       if (node is! Throw) {
+        // TODO(ahe): This is probably an internal error.

[Paul Berry, 2017/04/07 15:56:38]

Why is this a TODO?

[ahe, 2017/04/07 18:19:18]

Because I want to try inserting a call to internalError here later. It takes me
about 30 minutes to run all the tests, and I consider it low-priority to test.

+        needsImplicitSuperInitializer = false;
         node = wrapInvalid(node);
       }
-      initializer =
-          new LocalInitializer(new VariableDeclaration.forValue(node));
+      initializer = buildInvalidIntializer(node, token.charOffset);
     }
     if (member is KernelConstructorBuilder) {
       member.addInitializer(initializer);
@@ -416,17 +431,6 @@ class BodyBuilder extends ScopeListener<JumpTarget> implements BuilderHelper {
       FormalParameters formals, AsyncMarker asyncModifier, Statement body) {
     debugEvent("finishFunction");
     KernelFunctionBuilder builder = member;
-    if (builder is KernelConstructorBuilder) {
-      if (asyncModifier != AsyncMarker.Sync) {
-        // TODO(ahe): Change this to a null check.
-        addCompileTimeError(body?.fileOffset,
-            "Can't be marked as ${asyncModifier}: ${builder.name}");
-      }
-    } else if (builder is KernelProcedureBuilder) {
-      builder.asyncModifier = asyncModifier;
-    } else {
-      internalError("Unhandled: ${builder.runtimeType}");
-    }
     builder.body = body;
     if (formals?.optional != null) {
       Iterator<FormalParameterBuilder> formalBuilders =
@@ -439,6 +443,60 @@ class BodyBuilder extends ScopeListener<JumpTarget> implements BuilderHelper {
         realParameter.initializer = initializer..parent = realParameter;
       }
     }
+    if (builder is KernelConstructorBuilder) {
+      finishConstructor(builder, asyncModifier);
+    } else if (builder is KernelProcedureBuilder) {
+      builder.asyncModifier = asyncModifier;
+    } else {
+      internalError("Unhandled: ${builder.runtimeType}");
+    }
+  }
+
+  void finishConstructor(
+      KernelConstructorBuilder builder, AsyncMarker asyncModifier) {
+    /// Quotes below are from [Dart Programming Language Specification, 4th
+    /// Edition](
+    /// https://ecma-international.org/publications/files/ECMA-ST/ECMA-408.pdf).
+    assert(builder == member);
+    Constructor constructor = builder.constructor;
+    if (asyncModifier != AsyncMarker.Sync) {
+      // TODO(ahe): Change this to a null check.
+      int offset = builder.body?.fileOffset ?? builder.charOffset;
+      constructor.initializers.add(buildInvalidIntializer(
+          buildCompileTimeError(
+              "A constructor can't be '${asyncModifier}'.", offset),
+          offset));
+    }
+    if (needsImplicitSuperInitializer) {
+      /// >If no superinitializer is provided, an implicit superinitializer
+      /// >of the form super() is added at the end of k’s initializer list,
+      /// >unless the enclosing class is class Object.
+      Constructor superTarget = lookupConstructor(emptyName, isSuper: true);
+      Initializer initializer;
+      Arguments arguments = new Arguments.empty();
+      if (superTarget == null ||
+          !checkArguments(
+              superTarget.function, arguments, const <TypeParameter>[])) {
+        String superclass = classBuilder.supertype.fullNameForErrors;
+        initializer = buildInvalidIntializer(
+            buildCompileTimeError(
+                "'$superclass' has no constructor that takes no arguments.",

[Paul Berry, 2017/04/07 15:56:38]

Given the following code:

class C {
  C.foo();
}
class D extends C {
  D();
}

The error will fire (which is correct--there should be an error), but it will be
misleading, since it will say "'C' has no constructor that takes no arguments.",
even though C *does* have a constructor that takes no arguments.

How about if we change the error message to something like:

"'$superclass' must have an unnamed constructor that takes no arguments."

Alternatively, we could do what analyzer does, which is to report a different
error message depending on whether an unnamed constructor exists, e.g.:

initializer = buildInvalidInitializer(
    buildCompileTimeError(
        superTarget == null ?
            "'$superclass' doesn't have an unnamed constructor." :
            "The unnamed constructor for '$superclass' requires arguments.",
        builder.charOffset),
    builder.charOffset);

[ahe, 2017/04/07 18:19:18]

Will do. Those message are much better.

+                builder.charOffset),
+            builder.charOffset);
+      } else {
+        initializer =
+            buildSuperInitializer(superTarget, arguments, builder.charOffset);
+      }
+      constructor.initializers.add(initializer);
+    }
+    setParents(constructor.initializers, constructor);
+    if (constructor.function.body == null) {
+      /// >If a generative constructor c is not a redirecting constructor
+      /// >and no body is provided, then c implicitly has an empty body {}.
+      /// We use an empty statement instead.
+      constructor.function.body = new EmptyStatement();
+      constructor.function.body.parent = constructor.function;
+    }
   }
 
   @override
@@ -1731,6 +1789,7 @@ class BodyBuilder extends ScopeListener<JumpTarget> implements BuilderHelper {
     }
   }
 
+  @override
   bool checkArguments(FunctionNode function, Arguments arguments,
       List<TypeParameter> typeParameters) {
     if (arguments.positional.length < function.requiredParameterCount ||
@@ -2429,9 +2488,29 @@ class BodyBuilder extends ScopeListener<JumpTarget> implements BuilderHelper {
   }
 
   @override
-  Initializer buildCompileTimeErrorIntializer(error, [int charOffset = -1]) {
-    return new LocalInitializer(new VariableDeclaration.forValue(
-        buildCompileTimeError(error, charOffset)));
+  Initializer buildInvalidIntializer(Expression expression,
+      [int charOffset = -1]) {
+    needsImplicitSuperInitializer = false;
+    return new LocalInitializer(new VariableDeclaration.forValue(expression))
+      ..fileOffset = charOffset;
+  }
+
+  @override
+  Initializer buildSuperInitializer(
+      Constructor constructor, Arguments arguments,
+      [int charOffset = -1]) {
+    needsImplicitSuperInitializer = false;
+    return new SuperInitializer(constructor, arguments)
+      ..fileOffset = charOffset;
+  }
+
+  @override
+  Initializer buildRedirectingInitializer(
+      Constructor constructor, Arguments arguments,
+      [int charOffset = -1]) {
+    needsImplicitSuperInitializer = false;
+    return new RedirectingInitializer(constructor, arguments)
+      ..fileOffset = charOffset;
   }
 
   @override