Check for compile-time constants in DartCompiler

compiler/java/com/google/dart/compiler/resolver/CompileTimeConstVisitor.java

+// Copyright (c) 2011, 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.
+
+package com.google.dart.compiler.resolver;
+
+import com.google.dart.compiler.DartCompilerErrorCode;
+import com.google.dart.compiler.ast.DartArrayAccess;
+import com.google.dart.compiler.ast.DartArrayLiteral;
+import com.google.dart.compiler.ast.DartBinaryExpression;
+import com.google.dart.compiler.ast.DartContext;
+import com.google.dart.compiler.ast.DartExpression;
+import com.google.dart.compiler.ast.DartFunction;
+import com.google.dart.compiler.ast.DartFunctionObjectInvocation;
+import com.google.dart.compiler.ast.DartIdentifier;
+import com.google.dart.compiler.ast.DartIntegerLiteral;
+import com.google.dart.compiler.ast.DartInvocation;
+import com.google.dart.compiler.ast.DartMapLiteral;
+import com.google.dart.compiler.ast.DartMethodInvocation;
+import com.google.dart.compiler.ast.DartNewExpression;
+import com.google.dart.compiler.ast.DartNode;
+import com.google.dart.compiler.ast.DartNullLiteral;
+import com.google.dart.compiler.ast.DartParenthesizedExpression;
+import com.google.dart.compiler.ast.DartPropertyAccess;
+import com.google.dart.compiler.ast.DartRedirectConstructorInvocation;
+import com.google.dart.compiler.ast.DartStringInterpolation;
+import com.google.dart.compiler.ast.DartStringLiteral;
+import com.google.dart.compiler.ast.DartSuperExpression;
+import com.google.dart.compiler.ast.DartThisExpression;
+import com.google.dart.compiler.ast.DartUnaryExpression;
+import com.google.dart.compiler.ast.DartUnqualifiedInvocation;
+import com.google.dart.compiler.ast.DartVisitor;
+import com.google.dart.compiler.type.Type;
+
+/**
+ * Given an expression, Determines if the expression matches all the rules for a
+ * compile-time constant expression and emits a resolution error if not.
+ *
+ * This script doesn't just resolve expressions, it also sets types to the
+ * extent needed to validate compile-time constant expressions (boolean, int,
+ * double, and string types might be set)
+ *
+ */
+public class CompileTimeConstVisitor extends DartVisitor {

[ngeoffray, 2011/10/14 09:26:56]

Please use the DartNodeTraverser instead. I think DartVisitor was supposed to go
away.

[zundel, 2011/10/14 20:59:52]

Done.

+
+  static CompileTimeConstVisitor create(CoreTypeProvider typeProvider, ResolutionContext context) {
+    return new CompileTimeConstVisitor(typeProvider, context);
+  }
+
+  private final ResolutionContext context;
+
+  private final Type boolType;
+  private final Type doubleType;
+  private final Type intType;
+  private final Type numType;
+  private final Type stringType;
+  private final Type dynamicType;
+
+
+  private CompileTimeConstVisitor(CoreTypeProvider typeProvider, ResolutionContext context) {
+    this.context = context;
+    this.boolType = typeProvider.getBoolType();
+    this.doubleType = typeProvider.getDoubleType();
+    this.intType = typeProvider.getIntType();
+    this.numType = typeProvider.getNumType();
+    this.stringType = typeProvider.getStringType();
+    this.dynamicType = typeProvider.getDynamicType();
+  }
+
+  private boolean checkBoolean(DartNode x, Type type) {
+    if (!type.equals(boolType)) {
+      context.resolutionError(x, DartCompilerErrorCode.EXPECTED_CONSTANT_EXPRESSION_BOOLEAN,
+                              type.toString());
+      return false;
+    }
+    return true;
+  }
+
+  private boolean checkInt(DartNode x, Type type) {
+    if (!type.equals(intType)) {
+      context.resolutionError(x, DartCompilerErrorCode.EXPECTED_CONSTANT_EXPRESSION_INT,
+                              type.toString());
+      return false;
+    }
+    return true;
+  }
+
+  private boolean checkNumber(DartNode x, Type type) {
+    if (!(type.equals(numType) || type.equals(intType) || type.equals(doubleType))) {
+      context.resolutionError(x, DartCompilerErrorCode.EXPECTED_CONSTANT_EXPRESSION_NUMBER,
+                              type.toString());
+      return false;
+    }
+    return true;
+  }
+
+  private boolean checkNumberBooleanOrStringType(DartNode x, Type type) {
+    if (!type.equals(intType) && !type.equals(boolType)
+        && !type.equals(numType) && !type.equals(doubleType) && !type.equals(stringType)) {
+      context.resolutionError(x,
+          DartCompilerErrorCode.EXPECTED_CONSTANT_EXPRESSION_STRING_NUMBER_BOOL,
+          type.toString());
+      return false;
+    }
+    return true;
+  }
+
+  @Override
+  public void endVisit(DartBinaryExpression x, DartContext ctx) {
+    DartExpression lhs = x.getArg1();
+    DartExpression rhs = x.getArg2();
+    Type lhsType = getMostSpecificType(lhs);
+    Type rhsType = getMostSpecificType(rhs);
+    lhsType.getClass(); // fast null check
+    rhsType.getClass(); // fast null check

[ngeoffray, 2011/10/14 09:26:56]

you're doing the null checks and then checking for null line 117. I think
getMostSpecificType should never return null (expressions should be of type
dynamic), so if you want to do the null check, I would do it in
getMostSpecifictype.

[zundel, 2011/10/14 20:59:52]

Done.

+    boolean isConst = true;

[ngeoffray, 2011/10/14 09:26:56]

No need for this variable if lhsType and rhsType can never be null.

[zundel, 2011/10/14 20:59:52]

Done.

+    if (lhsType == null || rhsType == null) {
+      isConst = false;
+    }
+    if (isConst) {
+      switch (x.getOperator()) {
+        case NE:
+        case EQ:
+        case NE_STRICT:
+        case EQ_STRICT:
+          if (checkNumberBooleanOrStringType(lhs, lhsType) &&

[ngeoffray, 2011/10/14 09:26:56]

Nit: && should be on a newline, right below check...

[zundel, 2011/10/14 20:59:52]

Done.

+              checkNumberBooleanOrStringType(rhs, rhsType)) {
+            setType(x, boolType);
+          }
+          break;
+
+        case AND:
+        case OR:
+          if (checkBoolean(lhs, lhsType)
+          && checkBoolean(rhs, rhsType)) {

[ngeoffray, 2011/10/14 09:26:56]

weird indentation.

[zundel, 2011/10/14 20:59:52]

Done.

+            setType(x, boolType);
+          }
+          break;
+
+        case BIT_NOT:
+        case TRUNC:
+        case BIT_XOR:
+        case BIT_AND:
+        case BIT_OR:
+        case SAR:
+        case SHL:
+          if (checkInt(lhs, lhsType) &&
+              checkInt(rhs, rhsType)) {
+            setType(x, intType);
+            }

[ngeoffray, 2011/10/14 09:26:56]

ditto.

[zundel, 2011/10/14 20:59:52]

Done.

+          break;
+
+        case ADD:
+        case SUB:
+        case MUL:
+        case DIV:
+        case MOD:
+          if (checkNumber(lhs, lhsType) && checkNumber(rhs, rhsType)) {
+            setType(x, numType);
+        }
+        break;
+        case LT:
+        case GT:
+        case LTE:
+        case GTE:
+          if (checkNumber(lhs, lhsType) && checkNumber(rhs, rhsType)) {
+            setType(x, boolType);
+        }
+        break;
+
+        default:
+          // all other operators...
+          expectedConstant(x);
+      }
+    }
+  }
+
+  @Override
+  public void endVisit(DartParenthesizedExpression x, DartContext ctx) {
+    Type type = getMostSpecificType(x.getExpression());
+    setType(x, type);
+  }
+
+  @Override
+  public void endVisit(DartPropertyAccess x, DartContext ctx) {

[floitsch, 2011/10/14 14:36:58]

Does this make sure that PropertyAccess only goes to classes (or prefixes)?
(i.e. can an instance-field have the "isConstant" set?)
"const Point.origin().x" is not allowed (for instance).

[zundel, 2011/10/14 20:59:52]

Done.

+    Element element = x.getName().getSymbol();
+    if (element != null && !element.getModifiers().isConstant()) {
+      expectedConstant(x);
+    }
+    Type type = getMostSpecificType(x.getName());
+    setType(x, type);
+  }
+
+  @Override
+  public boolean visit(DartRedirectConstructorInvocation x, DartContext ctx) {
+    if (!x.getSymbol().getModifiers().isConstant()) {
+      expectedConstant(x);
+    }
+    return true;
+  }
+
+  @Override
+  public boolean visit(DartStringInterpolation x, DartContext ctx) {
+    expectedConstant(x);
+    return false;
+  }
+
+  @Override
+  public boolean visit(DartStringLiteral x, DartContext ctx) {
+    return false;

[floitsch, 2011/10/14 14:36:58]

I'm not really familiar with the visitor, but why does this return "false"?

[zundel, 2011/10/14 20:59:52]

returns false to stop traversing children.

I switched to DartNodeTraverser

+  }
+
+  @Override
+  public boolean visit(DartSuperExpression x, DartContext ctx) {
+    expectedConstant(x);
+    return false;
+  }
+
+  @Override
+  public void endVisit(DartUnaryExpression x, DartContext ctx) {
+    Type type = getMostSpecificType(x.getArg());
+    switch (x.getOperator()) {
+      case NOT:
+        if (checkBoolean(x, type)) {
+          x.setType(boolType);
+        }
+        break;
+      case SUB:
+        if (checkNumber(x, type)) {
+          x.setType(numType);
+        }
+        break;
+      case BIT_NOT:
+        if (checkInt(x, type)) {
+          x.setType(intType);
+        }
+        break;
+      default:
+        expectedConstant(x);
+    }
+  }
+
+
+  private void expectedConstant(DartNode x) {
+    context.resolutionError(x, DartCompilerErrorCode.EXPECTED_CONSTANT_EXPRESSION);
+  }
+
+  /**
+   * Determine the most specific type assigned to an expression node. Prefer the
+   * setting in the expression's symbol if present. Otherwise, use a type tagged
+   * in the expression node itself.
+   *
+   * @return a non <code>null</code> type value. Dynamic if none other can be
+   * determined.
+   */
+  private Type getMostSpecificType(DartExpression expr) {
+    Element element = (Element)expr.getSymbol();
+    switch (ElementKind.of(element)) {
+      case FIELD:
+        return ((FieldElement)element).getType();
+      case METHOD:
+        return ((MethodElement)element).getType();
+      case VARIABLE:
+        return((VariableElement)element).getType();
+    }

[ngeoffray, 2011/10/14 09:26:56]

Maybe inline the following code into a 'default' case? Should it be a case NONE:
? and default should throw an AssertionError?

[zundel, 2011/10/14 20:59:52]

Done.

+
+    if (expr.getType() != null) {
+      return expr.getType();
+    }
+
+    return dynamicType;
+  }
+
+  private void setType(DartExpression x, Type type) {
+    Element element = (Element)x.getSymbol();
+    if (element != null) {
+      Elements.setType(element, type);
+    }
+    // Also set on the expression node itself.  Not every expression has a symbol.
+    x.setType(type);
+  }
+
+  @Override
+  public boolean visit(DartArrayAccess x, DartContext ctx) {
+    // TODO(zundel): remove me if there is nothing to do
+    return true;
+  }
+
+  @Override
+  public boolean visit(DartArrayLiteral x, DartContext ctx) {
+    if (!x.isConst()) {
+      expectedConstant(x);
+    }
+    return true;
+  }
+
+  @Override
+  public boolean visit(DartFunction x, DartContext ctx) {
+    expectedConstant(x);
+    return true;
+  }
+
+  @Override
+  public boolean visit(DartFunctionObjectInvocation x, DartContext ctx) {
+    expectedConstant(x);
+    return false;
+  }
+
+  @Override
+  public boolean visit(DartIdentifier x, DartContext ctx) {
+    Element element = x.getSymbol();
+    switch(ElementKind.of(element)) {
+      case FIELD:
+      case CONSTRUCTOR:
+      case VARIABLE:
+      case PARAMETER:
+      if (!element.getModifiers().isConstant()) {
+        expectedConstant(x);
+      } else {
+        setType(x, getMostSpecificType(x));
+      }
+      break;
+      default:
+    }
+    return true;
+  }
+
+  @Override
+  public boolean visit(DartIntegerLiteral x, DartContext ctx) {
+    return false;
+  }
+
+  @Override
+  public boolean visit(DartInvocation x, DartContext ctx) {
+    expectedConstant(x);
+    return false; // No need to check further
+  }
+
+  @Override
+  public boolean visit(DartMapLiteral x, DartContext ctx) {
+    if (!x.isConst()) {
+      expectedConstant(x);
+    }
+    return true;
+  }
+
+  @Override
+  public boolean visit(DartMethodInvocation x, DartContext ctx) {
+    expectedConstant(x);
+    return false;
+  }
+
+  @Override
+  public boolean visit(DartNewExpression x, DartContext ctx) {
+    boolean isConst = false;
+    Element element = x.getSymbol();
+    switch (ElementKind.of(element)) {

[ngeoffray, 2011/10/14 09:26:56]

Please add a comment that factory methods cannot be CONSTRUCTOR.

[zundel, 2011/10/14 20:59:52]

I'm keying off of the 'const' reserved word now, so the code doesn't even need
to analyze the symbol.

+      case CONSTRUCTOR:
+        ConstructorElement constructorElement = (ConstructorElement) element;
+        if (constructorElement.getModifiers().isConstant()) {
+          isConst = true;
+        }
+        break;
+    }
+    if (!isConst) {
+      expectedConstant(x);
+    }
+    return true;
+  }
+
+  @Override
+  public boolean visit(DartNullLiteral x, DartContext ctx) {
+    return true;
+  }
+
+  @Override
+  public boolean visit(DartThisExpression x, DartContext ctx) {
+    expectedConstant(x);
+    return true;
+  }
+
+  @Override
+  public boolean visit(DartUnqualifiedInvocation x, DartContext ctx) {
+    expectedConstant(x);
+    return false;
+  }
+}
+