Complain about incorrect this/super constructor initializers.

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

 // Copyright (c) 2016, 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.
 
 library fasta.body_builder;
 
 import '../fasta_codes.dart'
     show FastaMessage, codeExpectedButGot, codeExpectedFunctionBody;
 
 import '../parser/parser.dart' show FormalParameterType, optional;
@@ skipping 61 matching lines @@
   final Map<String, FieldInitializer> fieldInitializers =
       <String, FieldInitializer>{};
 
   final Scope enclosingScope;
 
   final bool isDartLibrary;
 
   @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;
 
   bool inCatchClause = false;
 
   bool inCatchBlock = false;
 
   int functionNestingLevel = 0;
 
@@ skipping 19 matching lines @@
       Scope scope,
       this.formalParameterScope,
       this.hierarchy,
       this.coreTypes,
       this.classBuilder,
       this.isInstanceMember,
       this.uri)
       : enclosingScope = scope,
         library = library,
         isDartLibrary = library.uri.scheme == "dart",
+        needsImplicitSuperInitializer =
+            coreTypes.objectClass != classBuilder?.cls,
         super(scope);
 
   bool get hasParserError => recoverableErrors.isNotEmpty;
 
   bool get inConstructor {
     return functionNestingLevel == 0 && member is KernelConstructorBuilder;
   }
 
   bool get isInstanceContext {
     return isInstanceMember || member is KernelConstructorBuilder;
@@ skipping 247 matching lines @@
     debugEvent("endInitializer");
     assert(!inInitializer);
     final member = this.member;
     var node = pop();
     Initializer initializer;
     if (node is Initializer) {
       initializer = node;
     } 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);
     } else {
       addCompileTimeError(
           token.charOffset, "Can't have initializers: ${member.name}");
     }
   }
 
   @override
   void handleNoInitializers() {
     debugEvent("NoInitializers");
   }
 
   @override
   void endInitializers(int count, Token beginToken, Token endToken) {
     debugEvent("Initializers");
   }
 
   @override
   void finishFunction(
       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 =
           builder.formals.skip(formals.required.length).iterator;
       for (VariableDeclaration parameter in formals.optional.formals) {
         bool hasMore = formalBuilders.moveNext();
         assert(hasMore);
         VariableDeclaration realParameter = formalBuilders.current.target;
         Expression initializer = parameter.initializer ?? new NullLiteral();
         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
   void endExpressionStatement(Token token) {
     debugEvent("ExpressionStatement");
     push(new ExpressionStatement(popForEffect()));
   }
 
   @override
   void endArguments(int count, Token beginToken, Token endToken) {
@@ skipping 1272 matching lines @@
       return new ConstructorInvocation(target, arguments)
         ..isConst = isConst
         ..fileOffset = charOffset;
     } else {
       return new StaticInvocation(target, arguments)
         ..isConst = isConst
         ..fileOffset = charOffset;
     }
   }
 
+  @override
   bool checkArguments(FunctionNode function, Arguments arguments,
       List<TypeParameter> typeParameters) {
     if (arguments.positional.length < function.requiredParameterCount ||
         arguments.positional.length > function.positionalParameters.length) {
       return false;
     }
     Map<String, VariableDeclaration> names;
     if (function.namedParameters.isNotEmpty) {
       names = <String, VariableDeclaration>{};
       for (VariableDeclaration parameter in function.namedParameters) {
@@ skipping 678 matching lines @@
     Builder constructor = library.loader.getAbstractClassInstantiationError();
     return new Throw(buildStaticInvocation(constructor.target,
         new Arguments(<Expression>[new StringLiteral(className)])));
   }
 
   Statement buildCompileTimeErrorStatement(error, [int charOffset = -1]) {
     return new ExpressionStatement(buildCompileTimeError(error, charOffset));
   }
 
   @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
   Expression buildProblemExpression(ProblemBuilder builder, int charOffset) {
     return buildCompileTimeError(builder.message, charOffset);
   }
 
   @override
   void handleOperator(Token token) {
     debugEvent("Operator");
     push(new Operator(token.stringValue)..fileOffset = token.charOffset);
   }
 
@@ skipping 513 matching lines @@
   } else if (node is PrefixBuilder) {
     return node.name;
   } else if (node is ThisAccessor) {
     return node.isSuper ? "super" : "this";
   } else if (node is FastaAccessor) {
     return node.plainNameForRead;
   } else {
     return internalError("Unhandled: ${node.runtimeType}");
   }
 }