Check types of case expressions

runtime/vm/parser.cc

Index: runtime/vm/parser.cc
===================================================================
--- runtime/vm/parser.cc	(revision 27964)
+++ runtime/vm/parser.cc	(working copy)
@@ -6044,7 +6044,55 @@
 }
 
 
+// Check that all case expressions are of the same type, either int, String,
+// double or any other class that does not override the == operator.
+// The expressions are compile-time constants and are thus in the form
+// of a LiteralNode.
+void Parser::CheckCaseExpressions(const GrowableArray<LiteralNode*>& values) {
+  const intptr_t num_expressions = values.length();
+  if (num_expressions == 0) {
+    return;
+  }
+  const Instance& first_value = values[0]->literal();
+  for (intptr_t i = 0; i < num_expressions; i++) {

[Ivan Posva, 2013/09/27 00:24:59]

Why don't you start with i = 1? Otherwise you just compare the first_value with
itself.

[hausner, 2013/09/27 00:31:34]

Because I still need to check that the first value is either int, String or
double or doesn't implement ==. If there is only 1 case expression, that would
not get checked.

+    const Instance& val = values[i]->literal();
+    const intptr_t val_pos = values[i]->token_pos();
+    if (first_value.IsInteger()) {
+      if (!val.IsInteger()) {
+        ErrorMsg(val_pos, "expected case expression of type int");
+      }
+      continue;
+    }
+    if (first_value.IsString()) {
+      if (!val.IsString()) {
+        ErrorMsg(val_pos, "expected case expression of type String");
+      }
+      continue;
+    }
+    if (first_value.IsDouble()) {
+      if (!val.IsDouble()) {
+        ErrorMsg(val_pos, "expected case expression of type double");
+      }
+      continue;
+    }
+    if (val.clazz() != first_value.clazz()) {
+      ErrorMsg(val_pos, "all case expressions must be of same type");
+    }
+    Class& cls = Class::Handle(val.clazz());
+    const Function& equal_op = Function::Handle(
+        Resolver::ResolveDynamicAnyArgs(cls, Symbols::EqualOperator()));

[Ivan Posva, 2013/09/27 00:24:59]

Can't you hoist this check out of the loop? Once you have determined that the
non-special class does not implement the operator == it should be fine for all
of them.

[hausner, 2013/09/27 00:31:34]

Good point. If I hoist it out I have to duplicate the int/String/double checks,
but I can skip this on subsequent iterations after it's checked once.

Will do in a separate checkin.

[Ivan Posva, 2013/09/27 00:44:45]

You can hoist the whole testing out. Most of the time the val.clazz() will be
first_value.clazz() even for int, strings and doubles. If for some reason that
is not the case (e.g. mixing one and two byte strings) it can be easily checked
in a slow case.

[hausner, 2013/09/27 00:49:16]

The whole reason why int and String are handled separately is because there are
several implentation classes: Smi, Mint, Integer, one-, two- and four-byte
Strings... Also, I think the way I check makes the error messages quite clear
(int expected, String expected, etc).

I think it's cleaner this way :)

+    ASSERT(!equal_op.IsNull());
+    cls = equal_op.Owner();
+    if (!cls.IsObjectClass()) {
+      ErrorMsg(val_pos,
+               "type class of case expression must not implement operator ==");
+    }
+  }
+}
+
+
 CaseNode* Parser::ParseCaseClause(LocalVariable* switch_expr_value,
+                                  GrowableArray<LiteralNode*>* case_expr_values,
                                   SourceLabel* case_label) {
   TRACE_PARSER("ParseCaseClause");
   bool default_seen = false;
@@ -6059,6 +6107,9 @@
       ConsumeToken();  // Keyword case.
       const intptr_t expr_pos = TokenPos();
       AstNode* expr = ParseExpr(kRequireConst, kConsumeCascades);
+      ASSERT(expr->IsLiteralNode());
+      case_expr_values->Add(expr->AsLiteralNode());
+
       AstNode* switch_expr_load = new LoadLocalNode(case_pos,
                                                     switch_expr_value);
       AstNode* case_comparison = new ComparisonNode(expr_pos,
@@ -6147,6 +6198,7 @@
 
   // Parse case clauses
   bool default_seen = false;
+  GrowableArray<LiteralNode*> case_expr_values;
   while (true) {
     // Check for statement label
     SourceLabel* case_label = NULL;
@@ -6177,7 +6229,8 @@
       if (default_seen) {
         ErrorMsg("no case clauses allowed after default clause");
       }
-      CaseNode* case_clause = ParseCaseClause(temp_variable, case_label);
+      CaseNode* case_clause =
+          ParseCaseClause(temp_variable, &case_expr_values, case_label);
       default_seen = case_clause->contains_default();
       current_block_->statements->Add(case_clause);
     } else if (CurrentToken() != Token::kRBRACE) {
@@ -6189,8 +6242,9 @@
     }
   }
 
-  // TODO(hausner): Check that all expressions in case clauses are
-  // of the same class, or implement int or String (issue 7307).
+  // Check that all expressions in case clauses are of the same class,
+  // or implement int, double or String.
+  CheckCaseExpressions(case_expr_values);
 
   // Check for unresolved label references.
   SourceLabel* unresolved_label =