Various errors on constant expressions.

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 '../parser/parser.dart' show FormalParameterType, optional;
 
 import '../parser/error_kind.dart' show ErrorKind;
 
@@ skipping 164 matching lines @@
           node.name.name, new Arguments.empty(), node.fileOffset,
           isGetter: true);
     } else if (node is BuilderAccessor) {
       return node.buildSimpleRead();
     } else if (node is TypeVariableBuilder) {
       TypeParameterType type = node.buildTypesWithBuiltArguments(library, null);
       if (!isInstanceContext && type.parameter.parent is Class) {
         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) {
       return new TypeLiteral(node.buildTypesWithBuiltArguments(library, null));
     } else if (node is KernelTypeBuilder) {
       return new TypeLiteral(node.build(library));
     } else if (node is Expression) {
       return node;
     } else if (node is PrefixBuilder) {
       return buildCompileTimeError("A library can't be used as an expression.");
@@ skipping 332 matching lines @@
     } else if (arguments == null) {
       push(receiver);
     } else {
       push(finishSend(receiver, arguments, beginToken.charOffset));
     }
   }
 
   @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(
           receiver.name.name, arguments, receiver.fileOffset);
     } else {
       return buildMethodInvocation(
           toValue(receiver), callName, arguments, charOffset);
     }
   }
 
@@ skipping 187 matching lines @@
     String name = token.lexeme;
     if (context.isScopeReference) {
       assert(!inInitializer ||
           this.scope == enclosingScope ||
           this.scope.parent == enclosingScope);
       // This deals with this kind of initializer: `C(a) : a = a;`
       Scope scope = inInitializer ? enclosingScope : this.scope;
       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);
     }
   }
 
   @override
   builderToFirstExpression(Builder builder, String name, int charOffset,
       {bool isPrefix: false}) {
     if (builder == null || (!isInstanceContext && builder.isInstanceMember)) {
       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) {
       assert(builder.isStatic || builder.isTopLevel);
       return new StaticAccessor(this, charOffset, builder.target, null);
     } 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 {
       if (builder.hasProblem && builder is! AccessErrorBuilder) return builder;
       Builder setter;
       if (builder.isSetter) {
         setter = builder;
       } else if (builder.isGetter) {
         setter = scope.lookupSetter(name, charOffset, uri);
       } 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;
     }
   }
 
   @override
   void handleQualified(Token period) {
     debugEvent("Qualified");
     Identifier name = pop();
     var receiver = pop();
     push([receiver, name]);
   }
@@ skipping 399 matching lines @@
     if (builder == null) {
       warning("Type not found: '$name'.", charOffset);
       return const DynamicType();
     } else {
       return kernelTypeFromBuilder(builder, arguments, charOffset);
     }
   }
 
   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) {
       ProblemBuilder problem = builder;
       addCompileTimeError(charOffset, problem.message);
     } else {
       warning("Not a type: '${builder.fullNameForErrors}'.", charOffset);
     }
     // TODO(ahe): Create an error somehow.
     return const DynamicType();
@@ skipping 37 matching lines @@
     if (name is Identifier) {
       name = name.name;
     }
     if (name is BuilderAccessor) {
       warning("'${beginToken.lexeme}' isn't a type.", beginToken.charOffset);
       push(const DynamicType());
     } else if (name is UnresolvedIdentifier) {
       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));
     } else if (name is TypeBuilder) {
       push(name.build(library));
     } else if (name is Builder) {
       push(kernelTypeFromBuilder(name, arguments, beginToken.charOffset));
     } else if (name is String) {
       push(kernelTypeFromString(name, arguments, beginToken.charOffset));
     } else {
@@ skipping 1069 matching lines @@
   @override
   void handleInvalidFunctionBody(Token token) {
     if (member.isNative) {
       push(NullValue.FunctionBody);
     } else {
       push(new Block(<Statement>[new InvalidStatement()]));
     }
   }
 
   @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);
   }
 }
 
 // TODO(ahe): Shouldn't need to be an expression.
 class UnresolvedIdentifier extends InvalidExpression {
   final Name name;
 
   UnresolvedIdentifier(this.name);
@@ skipping 438 matching lines @@
   } else if (node is PrefixBuilder) {
     return node.name;
   } else if (node is ThisAccessor) {
     return node.isSuper ? "super" : "this";
   } else if (node is BuilderAccessor) {
     return node.plainNameForRead;
   } else {
     return internalError("Unhandled: ${node.runtimeType}");
   }
 }