Check types of case expressions

runtime/vm/parser.cc

 // Copyright (c) 2012, 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.
 
 #include "vm/parser.h"
 
 #include "lib/invocation_mirror.h"
 #include "vm/bigint_operations.h"
 #include "vm/bootstrap.h"
 #include "vm/class_finalizer.h"
@@ skipping 6026 matching lines @@
   if (label != NULL) {
     current_block_->statements->Add(if_node);
     SequenceNode* sequence = CloseBlock();
     sequence->set_label(label);
     if_node = sequence;
   }
   return if_node;
 }
 
 
+// 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;
   const intptr_t case_pos = TokenPos();
   // The case expressions node sequence does not own the enclosing scope.
   SequenceNode* case_expressions = new SequenceNode(case_pos, NULL);
   while (CurrentToken() == Token::kCASE || CurrentToken() == Token::kDEFAULT) {
     if (CurrentToken() == Token::kCASE) {
       if (default_seen) {
         ErrorMsg("default clause must be last case");
       }
       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,
                                                     Token::kEQ,
                                                     expr,
                                                     switch_expr_load);
       case_expressions->Add(case_comparison);
     } else {
       if (default_seen) {
         ErrorMsg("only one default clause is allowed");
@@ skipping 68 matching lines @@
       new LocalVariable(expr_pos,
                         Symbols::SwitchExpr(),
                         Type::ZoneHandle(Type::DynamicType()));
   current_block_->scope->AddVariable(temp_variable);
   AstNode* save_switch_expr =
       new StoreLocalNode(expr_pos, temp_variable, switch_expr);
   current_block_->statements->Add(save_switch_expr);
 
   // Parse case clauses
   bool default_seen = false;
+  GrowableArray<LiteralNode*> case_expr_values;
   while (true) {
     // Check for statement label
     SourceLabel* case_label = NULL;
     if (IsIdentifier() && LookaheadToken(1) == Token::kCOLON) {
       // Case statements start with a label.
       String* label_name = CurrentLiteral();
       const intptr_t label_pos = TokenPos();
       ConsumeToken();  // Consume label identifier.
       ConsumeToken();  // Consume colon.
       case_label = current_block_->scope->LocalLookupLabel(*label_name);
@@ skipping 10 matching lines @@
         ErrorMsg(label_pos, "label '%s' already exists in scope",
                  label_name->ToCString());
       }
       ASSERT(case_label->kind() == SourceLabel::kCase);
     }
     if (CurrentToken() == Token::kCASE ||
         CurrentToken() == Token::kDEFAULT) {
       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) {
       ErrorMsg("'case' or '}' expected");
     } else if (case_label != NULL) {
       ErrorMsg("expecting at least one case clause after label");
     } else {
       break;
     }
   }
 
-  // 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 =
       current_block_->scope->CheckUnresolvedLabels();
   if (unresolved_label != NULL) {
     ErrorMsg("unresolved reference to label '%s'",
              unresolved_label->name().ToCString());
   }
 
   SequenceNode* switch_body = CloseBlock();
@@ skipping 4342 matching lines @@
 void Parser::SkipQualIdent() {
   ASSERT(IsIdentifier());
   ConsumeToken();
   if (CurrentToken() == Token::kPERIOD) {
     ConsumeToken();  // Consume the kPERIOD token.
     ExpectIdentifier("identifier expected after '.'");
   }
 }
 
 }  // namespace dart