Various errors on constant expressions.

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 e3f51e31854852acb79ad76547c6090afbbfd0c8..2a262149fce60fc3fa30fc65e77506aa33f38b8f 100644
--- a/pkg/front_end/lib/src/fasta/kernel/body_builder.dart
+++ b/pkg/front_end/lib/src/fasta/kernel/body_builder.dart
@@ -182,6 +182,10 @@ class BodyBuilder extends ScopeListener<JumpTarget> implements BuilderHelper {
         return buildCompileTimeError(
             "Type variables can only be used in instance methods.");
       } else {
+        if (constantExpressionRequired) {
+          addCompileTimeError(-1,
+              "Type variable can't be used as a constant expression $type.");

[Paul Berry, 2017/03/22 16:18:04]

I have two concerns about how this technique for detecting compile time errors
is going to integrate with analyzer:

1. Analyzer (and other Google-internal tools which consume analyzer output)
assumes that each error is associated with a pseudo-enum ErrorCode (see
pkg/front_end/lib/src/base/errors.dart) and that the error message is formed
using a template stored in ErrorCode.  If we encode the error message as a
simple string here, we aren't going to be able to translate it to analyzer's
representation later.

2. If we put error detection logic here in BodyBuilder, we will have to have
duplicate error detection logic in AstBuilder (or in a later analysis step). 
Not only does this mean duplicate coding work, it also increases the risk of
unintended differences between analyzer-reported errors and compiler-reported
errors.

Can we talk about this next time we meet, and brainstorm ways to do the error
reporting that will work well with analyzer integration?

Until then it's ok with me if you want to go ahead with this CL--I don't want to
block you from fixing tests :)

[ahe, 2017/03/22 16:58:02]

I'm aware of that, and I've started work on error codes in parallel. It's a cost
I'm pushing ahead of me and I know I have to pay.
I share your concern about this, and I only have vague ideas so far.

+        }
         return new TypeLiteral(type);
       }
     } else if (node is TypeDeclarationBuilder) {
@@ -534,6 +538,9 @@ class BodyBuilder extends ScopeListener<JumpTarget> implements BuilderHelper {
   @override
   finishSend(Object receiver, Arguments arguments, int charOffset) {
     if (receiver is BuilderAccessor) {
+      if (constantExpressionRequired) {
+        addCompileTimeError(charOffset, "Not a constant expression.");

[Paul Berry, 2017/03/22 16:18:04]

Idea: when we report a "Not a constant expression" compile time error, set
constantExpressionRequired to false so that the user gets a single error rather
than a pile of errors.

[ahe, 2017/03/22 16:58:02]

That's probably a good idea.

[ahe, 2017/03/23 12:22:13]

I'm going to follow up on this in separate CL.

+      }
       return receiver.doInvocation(charOffset, arguments);
     } else if (receiver is UnresolvedIdentifier) {
       return throwNoSuchMethodError(
@@ -741,6 +748,18 @@ class BodyBuilder extends ScopeListener<JumpTarget> implements BuilderHelper {
       Builder builder = scope.lookup(name, token.charOffset, uri);
       push(builderToFirstExpression(builder, name, token.charOffset));
     } else {
+      if (constantExpressionRequired) {
+        if (context != IdentifierContext.namedArgumentReference &&

[Paul Berry, 2017/03/22 16:18:05]

Rather than a bunch of != checks, we should add a getter to IdentifierContext so
that this can be something like:

if (constantExpressionRequired && !context.allowedInConstantExpression) {
  addCompileTimeError(...);
}

[ahe, 2017/03/22 16:58:02]

Good idea.

[ahe, 2017/03/23 12:22:13]

I'll do this in a separate CL.

+            context != IdentifierContext.constructorReferenceContinuation &&
+            context != IdentifierContext.expressionContinuation &&
+            context != IdentifierContext.typeReferenceContinuation &&
+            context !=
+                IdentifierContext
+                    .constructorReferenceContinuationAfterTypeArguments) {
+          addCompileTimeError(
+              token.charOffset, "Not a constant expression: $context");
+        }
+      }
       push(new Identifier(name)..fileOffset = token.charOffset);
     }
   }
@@ -752,15 +771,30 @@ class BodyBuilder extends ScopeListener<JumpTarget> implements BuilderHelper {
       Name n = new Name(name, library.library);
       if (!isPrefix && isInstanceContext) {
         assert(builder == null);
+        if (constantExpressionRequired) {
+          addCompileTimeError(charOffset, "Not a constant expression.");
+        }
         return new ThisPropertyAccessor(this, charOffset, n, null, null);
       } else {
+        if (constantExpressionRequired) {
+          addCompileTimeError(charOffset, "Not a constant expression.");
+        }
         return new UnresolvedIdentifier(n)..fileOffset = charOffset;
       }
     } else if (builder.isTypeDeclaration) {
+      if (constantExpressionRequired && builder.isTypeVariable) {
+        addCompileTimeError(charOffset, "Not a constant expression.");
+      }
       return builder;
     } else if (builder.isLocal) {
+      if (constantExpressionRequired && !builder.isConst) {
+        addCompileTimeError(charOffset, "Not a constant expression.");
+      }
       return new VariableAccessor(this, charOffset, builder.target);
     } else if (builder.isInstanceMember) {
+      if (constantExpressionRequired) {
+        addCompileTimeError(charOffset, "Not a constant expression.");
+      }
       return new ThisPropertyAccessor(
           this, charOffset, new Name(name, library.library), null, null);
     } else if (builder.isRegularMethod) {
@@ -769,6 +803,9 @@ class BodyBuilder extends ScopeListener<JumpTarget> implements BuilderHelper {
     } else if (builder is PrefixBuilder) {
       return builder;
     } else if (builder is MixedAccessor) {
+      if (constantExpressionRequired && !builder.getter.target.isConst) {
+        addCompileTimeError(charOffset, "Not a constant expression.");
+      }
       return new StaticAccessor(
           this, charOffset, builder.getter.target, builder.setter.target);
     } else {
@@ -781,7 +818,15 @@ class BodyBuilder extends ScopeListener<JumpTarget> implements BuilderHelper {
       } else if (builder.isField && !builder.isFinal) {
         setter = builder;
       }
-      return new StaticAccessor.fromBuilder(this, builder, charOffset, setter);
+      StaticAccessor accessor =
+          new StaticAccessor.fromBuilder(this, builder, charOffset, setter);
+      if (constantExpressionRequired) {
+        Member readTarget = accessor.readTarget;
+        if (!(readTarget is Field && readTarget.isConst)) {
+          addCompileTimeError(charOffset, "Not a constant expression.");
+        }
+      }
+      return accessor;
     }
   }
 
@@ -1201,6 +1246,9 @@ class BodyBuilder extends ScopeListener<JumpTarget> implements BuilderHelper {
 
   DartType kernelTypeFromBuilder(
       Builder builder, List<DartType> arguments, int charOffset) {
+    if (constantExpressionRequired && builder is TypeVariableBuilder) {
+      addCompileTimeError(charOffset, "Not a constant expression.");
+    }
     if (builder is TypeDeclarationBuilder) {
       return builder.buildTypesWithBuiltArguments(library, arguments);
     } else if (builder.hasProblem) {
@@ -1258,6 +1306,10 @@ class BodyBuilder extends ScopeListener<JumpTarget> implements BuilderHelper {
       warning("'${name.name}' isn't a type.", beginToken.charOffset);
       push(const DynamicType());
     } else if (name is TypeVariableBuilder) {
+      if (constantExpressionRequired) {
+        addCompileTimeError(
+            beginToken.charOffset, "Not a constant expression.");
+      }
       push(name.buildTypesWithBuiltArguments(library, arguments));
     } else if (name is TypeDeclarationBuilder) {
       push(name.buildTypesWithBuiltArguments(library, arguments));
@@ -2347,6 +2399,15 @@ class BodyBuilder extends ScopeListener<JumpTarget> implements BuilderHelper {
   }
 
   @override
+  void warning(String message, [int charOffset = -1]) {
+    if (constantExpressionRequired) {
+      addCompileTimeError(charOffset, message);

[Paul Berry, 2017/03/22 16:18:04]

This seems really scary to me.  Are we sure that all builder-detected warnings
should be treated as errors inside constant expressions?  I can't think of any
counterexamples off the top of my head, but this seems like an accident waiting
to happen.

[ahe, 2017/03/22 16:58:02]

There are a few warnings that don't follow this rule, in particular, const
<Unresolved>[]. So I've been thinking about adding named parameter to this
method to say if this warning is a compile-time error in constants (in most
cases, I think it is).

+    } else {
+      super.warning(message, charOffset);
+    }
+  }
+
+  @override
   void debugEvent(String name) {
     // printEvent(name);
   }