Check const map literal keys and switch case exprs using type of constant.

editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/verifier/ConstantVerifier.java

Index: editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/verifier/ConstantVerifier.java
diff --git a/editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/verifier/ConstantVerifier.java b/editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/verifier/ConstantVerifier.java
index de9a2e27e45a9b0a7443a8518979b94cbcc96ca6..4eb3584609bb1f305b343a598612cee03e7840ba 100644
--- a/editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/verifier/ConstantVerifier.java
+++ b/editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/verifier/ConstantVerifier.java
@@ -34,11 +34,16 @@ import com.google.dart.engine.ast.RedirectingConstructorInvocation;
 import com.google.dart.engine.ast.SimpleIdentifier;
 import com.google.dart.engine.ast.SuperConstructorInvocation;
 import com.google.dart.engine.ast.SwitchCase;
+import com.google.dart.engine.ast.SwitchMember;
+import com.google.dart.engine.ast.SwitchStatement;
 import com.google.dart.engine.ast.VariableDeclaration;
 import com.google.dart.engine.ast.visitor.RecursiveAstVisitor;
 import com.google.dart.engine.constant.DartObject;
+import com.google.dart.engine.element.ClassElement;
 import com.google.dart.engine.element.ConstructorElement;
 import com.google.dart.engine.element.Element;
+import com.google.dart.engine.element.LibraryElement;
+import com.google.dart.engine.element.MethodElement;
 import com.google.dart.engine.element.ParameterElement;
 import com.google.dart.engine.error.CompileTimeErrorCode;
 import com.google.dart.engine.error.ErrorCode;
@@ -105,12 +110,19 @@ public class ConstantVerifier extends RecursiveAstVisitor<Void> {
   private InterfaceType stringType;
 
   /**
+   * The current library that is being analyzed.
+   */
+  private LibraryElement currentLibrary;
+
+  /**
    * Initialize a newly created constant verifier.
    * 
    * @param errorReporter the error reporter by which errors will be reported
    */
-  public ConstantVerifier(ErrorReporter errorReporter, TypeProvider typeProvider) {
+  public ConstantVerifier(ErrorReporter errorReporter, LibraryElement currentLibrary,
+      TypeProvider typeProvider) {
     this.errorReporter = errorReporter;
+    this.currentLibrary = currentLibrary;
     this.typeProvider = typeProvider;
     this.boolType = typeProvider.getBoolType();
     this.intType = typeProvider.getIntType();
@@ -215,6 +227,13 @@ public class ConstantVerifier extends RecursiveAstVisitor<Void> {
           } else {
             keys.add(value);
           }
+          Type type = value.getType();
+          if (implementsEqualsWhenNotAllowed(type)) {
+            errorReporter.reportErrorForNode(
+                CompileTimeErrorCode.CONST_MAP_KEY_EXPRESSION_TYPE_IMPLEMENTS_EQUALS,
+                key,
+                type.getDisplayName());
+          }
         }
       } else {
         EvaluationResultImpl result = key.accept(new ConstantVisitor(typeProvider));
@@ -246,18 +265,43 @@ public class ConstantVerifier extends RecursiveAstVisitor<Void> {
   }
 
   @Override
-  public Void visitSwitchCase(SwitchCase node) {
-    super.visitSwitchCase(node);
-    Expression expression = node.getExpression();
-    EvaluationResultImpl result = validate(
-        expression,
-        CompileTimeErrorCode.NON_CONSTANT_CASE_EXPRESSION);
-    if (result instanceof ValidResult) {
-      reportErrorIfFromDeferredLibrary(
-          expression,
-          CompileTimeErrorCode.NON_CONSTANT_CASE_EXPRESSION_FROM_DEFERRED_LIBRARY);
+  public Void visitSwitchStatement(SwitchStatement node) {
+    // TODO(jwren) Revisit this algorithm, should there up to n-1 errors?

[Paul Berry, 2014/05/03 13:33:06]

Jaime: is this still an open issue?

[Brian Wilkerson, 2014/05/03 15:03:34]

Yes, it's still an issue. I think what we really want to do, when there are
conflicting types, is to report the smallest number of errors (which we
currently don't). For example, given

  switch(...) {
    case 3: ...
    case "a": ...
    case "b": ...
    case "c": ...
  }

we will produce 3 errors (on all but the first case) when I think we should
produce one error (on the first case).

[Paul Berry, 2014/05/03 17:14:56]

Ah, ok.  I've changed the comment to "TODO(paulberry): to minimize error
messages, it would be nice to compare all types with the most popular type
rather than the first type."

[jwren, 2014/05/03 23:03:59]

Sounds good, although as far as priority this is a nice to have, not a
defect-bug.



On 2014/05/03 17:14:56, Paul Berry wrote:

+    NodeList<SwitchMember> switchMembers = node.getMembers();
+    boolean foundError = false;
+    Type firstType = null;
+    for (SwitchMember switchMember : switchMembers) {
+      if (switchMember instanceof SwitchCase) {
+        SwitchCase switchCase = (SwitchCase) switchMember;
+        Expression expression = switchCase.getExpression();
+        EvaluationResultImpl caseResult = validate(
+            expression,
+            CompileTimeErrorCode.NON_CONSTANT_CASE_EXPRESSION);
+        if (caseResult instanceof ValidResult) {
+          reportErrorIfFromDeferredLibrary(
+              expression,
+              CompileTimeErrorCode.NON_CONSTANT_CASE_EXPRESSION_FROM_DEFERRED_LIBRARY);
+          DartObject value = ((ValidResult) caseResult).getValue();
+          if (firstType == null) {
+            firstType = value.getType();
+          } else {
+            Type nType = value.getType();
+            if (!firstType.equals(nType)) {
+              errorReporter.reportErrorForNode(
+                  CompileTimeErrorCode.INCONSISTENT_CASE_EXPRESSION_TYPES,
+                  expression,
+                  expression.toSource(),
+                  firstType.getDisplayName());
+              foundError = true;
+            }
+          }
+        }
+      }
     }
-    return null;
+    if (!foundError) {
+      checkForCaseExpressionTypeImplementsEquals(node, firstType);
+    }
+    return super.visitSwitchStatement(node);
   }
 
   @Override
@@ -290,6 +334,60 @@ public class ConstantVerifier extends RecursiveAstVisitor<Void> {
   }
 
   /**
+   * This verifies that the passed switch statement does not have a case expression with the
+   * operator '==' overridden.
+   * 
+   * @param node the switch statement to evaluate
+   * @param type the common type of all 'case' expressions
+   * @return {@code true} if and only if an error code is generated on the passed node
+   * @see CompileTimeErrorCode#CASE_EXPRESSION_TYPE_IMPLEMENTS_EQUALS
+   */
+  private boolean checkForCaseExpressionTypeImplementsEquals(SwitchStatement node, Type type) {
+    if (!implementsEqualsWhenNotAllowed(type)) {
+      return false;
+    }
+    // report error
+    errorReporter.reportErrorForToken(
+        CompileTimeErrorCode.CASE_EXPRESSION_TYPE_IMPLEMENTS_EQUALS,
+        node.getKeyword(),
+        type.getDisplayName());
+    return true;
+  }
+
+  /**
+   * @return {@code true} if given {@link Type} implements operator <i>==</i>, and it is not
+   *         <i>int</i> or <i>String</i>.
+   */
+  private boolean implementsEqualsWhenNotAllowed(Type type) {
+    // ignore int or String
+    if (type == null || type.equals(intType) || type.equals(typeProvider.getStringType())) {

[jwren, 2014/05/03 23:03:59]

What about other nums such as the num type, double, etc.?

[Brian Wilkerson, 2014/05/04 04:39:12]

The specification explicitly states:

It is a compile-time error if the class C has an implementation of the operator
== other than the one inherited from Object unless the value of the expression
is a string or integer.

num and double are not explicitly mentioned. (And actually (a) we need to handle
double specially in next condition and (b) num is an abstract class so there
won't ever be an instance of it.)

+      return false;
+    } else if (type.equals(typeProvider.getDoubleType())) {
+      return true;
+    }
+    // prepare ClassElement
+    Element element = type.getElement();
+    if (!(element instanceof ClassElement)) {
+      return false;
+    }
+    ClassElement classElement = (ClassElement) element;
+    // lookup for ==
+    MethodElement method = classElement.lookUpMethod("==", currentLibrary);
+    while (method != null && method.isAbstract()) {
+      ClassElement definingClass = method.getEnclosingElement();
+      if (definingClass == null) {
+        return false;
+      }
+      method = definingClass.lookUpInheritedMethod("==", currentLibrary);
+    }
+    if (method == null || method.getEnclosingElement().getType().isObject()) {
+      return false;
+    }
+    // there is == that we don't like
+    return true;
+  }
+
+  /**
    * Given some computed {@link Expression}, this method generates the passed {@link ErrorCode} on
    * the node if its' value consists of information from a deferred library.
    *