diff mbox

Go patch committed: Do not declare type switch variable outside case statements

Message ID CAOyqgcX-0DTSj6VyppPYBLgsbPnX-i58V9b2ynJp4PTFZw4pdQ@mail.gmail.com
State New
Headers show

Commit Message

Ian Lance Taylor March 6, 2015, 12:27 a.m. UTC
This patch by Chris Manghane fixes a bug for cases like this:
    switch x := v.(type) {
    case *x:
in which the type name in the case happens to be the same as the
variable name in the type switch.  This is rather confusing code, but
it should work.  This is http://golang.org/issue/10047 .  Bootstrapped
and ran Go testsuite on x86_64-unknown-linux-gnu.  Committed to
mainline.

Ian
diff mbox

Patch

diff -r d42a0819e2eb go/gogo.cc
--- a/go/gogo.cc	Fri Feb 06 08:17:54 2015 -0800
+++ b/go/gogo.cc	Thu Mar 05 16:21:29 2015 -0800
@@ -6030,6 +6030,7 @@ 
   Type* type = this->type_;
   Expression* init = this->init_;
   if (this->is_type_switch_var_
+      && type != NULL
       && this->type_->is_nil_constant_as_type())
     {
       Type_guard_expression* tge = this->init_->type_guard_expression();
@@ -6103,7 +6104,9 @@ 
   // type here.  It will have an initializer which is a type guard.
   // We want to initialize it to the value without the type guard, and
   // use the type of that value as well.
-  if (this->is_type_switch_var_ && this->type_->is_nil_constant_as_type())
+  if (this->is_type_switch_var_
+      && this->type_ != NULL
+      && this->type_->is_nil_constant_as_type())
     {
       Type_guard_expression* tge = this->init_->type_guard_expression();
       go_assert(tge != NULL);
diff -r d42a0819e2eb go/parse.cc
--- a/go/parse.cc	Fri Feb 06 08:17:54 2015 -0800
+++ b/go/parse.cc	Thu Mar 05 16:21:29 2015 -0800
@@ -50,8 +50,7 @@ 
     break_stack_(NULL),
     continue_stack_(NULL),
     iota_(0),
-    enclosing_vars_(),
-    type_switch_vars_()
+    enclosing_vars_()
 {
 }
 
@@ -4596,32 +4595,33 @@ 
 Parse::type_switch_body(Label* label, const Type_switch& type_switch,
 			Location location)
 {
-  Named_object* switch_no = NULL;
-  if (!type_switch.name.empty())
-    {
-      if (Gogo::is_sink_name(type_switch.name))
-	error_at(type_switch.location,
-		 "no new variables on left side of %<:=%>");
+  Expression* init = type_switch.expr;
+  std::string var_name = type_switch.name;
+  if (!var_name.empty())
+    {
+      if (Gogo::is_sink_name(var_name))
+        {
+          error_at(type_switch.location,
+                   "no new variables on left side of %<:=%>");
+          var_name.clear();
+        }
       else
 	{
-	  Variable* switch_var = new Variable(NULL, type_switch.expr, false,
-					      false, false,
-					      type_switch.location);
-	  switch_no = this->gogo_->add_variable(type_switch.name, switch_var);
+          Location loc = type_switch.location;
+	  Temporary_statement* switch_temp =
+              Statement::make_temporary(NULL, init, loc);
+	  this->gogo_->add_statement(switch_temp);
+          init = Expression::make_temporary_reference(switch_temp, loc);
 	}
     }
 
   Type_switch_statement* statement =
-    Statement::make_type_switch_statement(switch_no,
-					  (switch_no == NULL
-					   ? type_switch.expr
-					   : NULL),
-					  location);
-
+      Statement::make_type_switch_statement(var_name, init, location);
   this->push_break_statement(statement, label);
 
   Type_case_clauses* case_clauses = new Type_case_clauses();
   bool saw_default = false;
+  std::vector<Named_object*> implicit_vars;
   while (!this->peek_token()->is_op(OPERATOR_RCURLY))
     {
       if (this->peek_token()->is_eof())
@@ -4629,7 +4629,8 @@ 
 	  error_at(this->location(), "missing %<}%>");
 	  return NULL;
 	}
-      this->type_case_clause(switch_no, case_clauses, &saw_default);
+      this->type_case_clause(var_name, init, case_clauses, &saw_default,
+                             &implicit_vars);
     }
   this->advance_token();
 
@@ -4637,14 +4638,36 @@ 
 
   this->pop_break_statement();
 
+  // If there is a type switch variable implicitly declared in each case clause,
+  // check that it is used in at least one of the cases.
+  if (!var_name.empty())
+    {
+      bool used = false;
+      for (std::vector<Named_object*>::iterator p = implicit_vars.begin();
+	   p != implicit_vars.end();
+	   ++p)
+	{
+	  if ((*p)->var_value()->is_used())
+	    {
+	      used = true;
+	      break;
+	    }
+	}
+      if (!used)
+	error_at(type_switch.location, "%qs declared and not used",
+		 Gogo::message_name(var_name).c_str());
+    }
   return statement;
 }
 
 // TypeCaseClause  = TypeSwitchCase ":" [ StatementList ] .
+// IMPLICIT_VARS is the list of variables implicitly declared for each type
+// case if there is a type switch variable declared.
 
 void
-Parse::type_case_clause(Named_object* switch_no, Type_case_clauses* clauses,
-			bool* saw_default)
+Parse::type_case_clause(const std::string& var_name, Expression* init,
+                        Type_case_clauses* clauses, bool* saw_default,
+			std::vector<Named_object*>* implicit_vars)
 {
   Location location = this->location();
 
@@ -4661,24 +4684,21 @@ 
   if (this->statement_list_may_start_here())
     {
       this->gogo_->start_block(this->location());
-      if (switch_no != NULL && types.size() == 1)
+      if (!var_name.empty())
 	{
-	  Type* type = types.front();
-	  Expression* init = Expression::make_var_reference(switch_no,
-							    location);
-	  init = Expression::make_type_guard(init, type, location);
+	  Type* type = NULL;
+          Location var_loc = init->location();
+	  if (types.size() == 1)
+	    {
+	      type = types.front();
+	      init = Expression::make_type_guard(init, type, location);
+	    }
+
 	  Variable* v = new Variable(type, init, false, false, false,
-				     location);
+				     var_loc);
+	  v->set_is_used();
 	  v->set_is_type_switch_var();
-	  Named_object* no = this->gogo_->add_variable(switch_no->name(), v);
-
-	  // We don't want to issue an error if the compiler
-	  // introduced special variable is not used.  Instead we want
-	  // to issue an error if the variable defined by the switch
-	  // is not used.  That is handled via type_switch_vars_ and
-	  // Parse::mark_var_used.
-	  v->set_is_used();
-	  this->type_switch_vars_[no] = switch_no;
+	  implicit_vars->push_back(this->gogo_->add_variable(var_name, v));
 	}
       this->statement_list();
       statements = this->gogo_->finish_block(this->location());
@@ -5752,15 +5772,5 @@ 
 Parse::mark_var_used(Named_object* no)
 {
   if (no->is_variable())
-    {
-      no->var_value()->set_is_used();
-
-      // When a type switch uses := to define a variable, then for
-      // each case with a single type we introduce a new variable with
-      // the appropriate type.  When we do, if the newly introduced
-      // variable is used, then the type switch variable is used.
-      Type_switch_vars::iterator p = this->type_switch_vars_.find(no);
-      if (p != this->type_switch_vars_.end())
-	p->second->var_value()->set_is_used();
-    }
+    no->var_value()->set_is_used();
 }
diff -r d42a0819e2eb go/parse.h
--- a/go/parse.h	Fri Feb 06 08:17:54 2015 -0800
+++ b/go/parse.h	Thu Mar 05 16:21:29 2015 -0800
@@ -156,11 +156,6 @@ 
   // break or continue statement with no label.
   typedef std::vector<std::pair<Statement*, Label*> > Bc_stack;
 
-  // Map from type switch variables to the variables they mask, so
-  // that a use of the type switch variable can become a use of the
-  // real variable.
-  typedef Unordered_map(Named_object*, Named_object*) Type_switch_vars;
-
   // Parser nonterminals.
   void identifier_list(Typed_identifier_list*);
   Expression_list* expression_list(Expression*, bool may_be_sink,
@@ -259,7 +254,8 @@ 
   void expr_case_clause(Case_clauses*, bool* saw_default);
   Expression_list* expr_switch_case(bool*);
   Statement* type_switch_body(Label*, const Type_switch&, Location);
-  void type_case_clause(Named_object*, Type_case_clauses*, bool* saw_default);
+  void type_case_clause(const std::string&, Expression*, Type_case_clauses*,
+                        bool* saw_default, std::vector<Named_object*>*);
   void type_switch_case(std::vector<Type*>*, bool*);
   void select_stat(Label*);
   void comm_clause(Select_clauses*, bool* saw_default);
@@ -327,8 +323,6 @@ 
   // References from the local function to variables defined in
   // enclosing functions.
   Enclosing_vars enclosing_vars_;
-  // Map from type switch variables to real variables.
-  Type_switch_vars type_switch_vars_;
 };
 
 
diff -r d42a0819e2eb go/statements.cc
--- a/go/statements.cc	Fri Feb 06 08:17:54 2015 -0800
+++ b/go/statements.cc	Thu Mar 05 16:21:29 2015 -0800
@@ -4279,11 +4279,8 @@ 
 int
 Type_switch_statement::do_traverse(Traverse* traverse)
 {
-  if (this->var_ == NULL)
-    {
-      if (this->traverse_expression(traverse, &this->expr_) == TRAVERSE_EXIT)
-	return TRAVERSE_EXIT;
-    }
+  if (this->traverse_expression(traverse, &this->expr_) == TRAVERSE_EXIT)
+    return TRAVERSE_EXIT;
   if (this->clauses_ != NULL)
     return this->clauses_->traverse(traverse);
   return TRAVERSE_CONTINUE;
@@ -4306,10 +4303,7 @@ 
 
   Block* b = new Block(enclosing, loc);
 
-  Type* val_type = (this->var_ != NULL
-		    ? this->var_->var_value()->type()
-		    : this->expr_->type());
-
+  Type* val_type = this->expr_->type();
   if (val_type->interface_type() == NULL)
     {
       if (!val_type->is_error())
@@ -4326,15 +4320,10 @@ 
   // descriptor_temp = ifacetype(val_temp) FIXME: This should be
   // inlined.
   bool is_empty = val_type->interface_type()->is_empty();
-  Expression* ref;
-  if (this->var_ == NULL)
-    ref = this->expr_;
-  else
-    ref = Expression::make_var_reference(this->var_, loc);
   Expression* call = Runtime::make_call((is_empty
 					 ? Runtime::EFACETYPE
 					 : Runtime::IFACETYPE),
-					loc, 1, ref);
+					loc, 1, this->expr_);
   Temporary_reference_expression* lhs =
     Expression::make_temporary_reference(descriptor_temp, loc);
   lhs->set_is_lvalue();
@@ -4384,7 +4373,9 @@ 
     const
 {
   ast_dump_context->print_indent();
-  ast_dump_context->ostream() << "switch " << this->var_->name() << " = ";
+  ast_dump_context->ostream() << "switch ";
+  if (!this->name_.empty())
+    ast_dump_context->ostream() << this->name_ << " = ";
   ast_dump_context->dump_expression(this->expr_);
   ast_dump_context->ostream() << " .(type)";
   if (ast_dump_context->dump_subblocks())
@@ -4399,10 +4390,10 @@ 
 // Make a type switch statement.
 
 Type_switch_statement*
-Statement::make_type_switch_statement(Named_object* var, Expression* expr,
+Statement::make_type_switch_statement(const std::string& name, Expression* expr,
 				      Location location)
 {
-  return new Type_switch_statement(var, expr, location);
+  return new Type_switch_statement(name, expr, location);
 }
 
 // Class Send_statement.
diff -r d42a0819e2eb go/statements.h
--- a/go/statements.h	Fri Feb 06 08:17:54 2015 -0800
+++ b/go/statements.h	Thu Mar 05 16:21:29 2015 -0800
@@ -250,7 +250,7 @@ 
 
   // Make a type switch statement.
   static Type_switch_statement*
-  make_type_switch_statement(Named_object* var, Expression*, Location);
+  make_type_switch_statement(const std::string&, Expression*, Location);
 
   // Make a send statement.
   static Send_statement*
@@ -1607,11 +1607,11 @@ 
 class Type_switch_statement : public Statement
 {
  public:
-  Type_switch_statement(Named_object* var, Expression* expr,
+  Type_switch_statement(const std::string& name, Expression* expr,
 			Location location)
     : Statement(STATEMENT_TYPE_SWITCH, location),
-      var_(var), expr_(expr), clauses_(NULL), break_label_(NULL)
-  { go_assert(var == NULL || expr == NULL); }
+      name_(name), expr_(expr), clauses_(NULL), break_label_(NULL)
+  { }
 
   // Add the clauses.
   void
@@ -1643,8 +1643,9 @@ 
   do_may_fall_through() const;
 
  private:
-  // The variable holding the value we are switching on.
-  Named_object* var_;
+  // The name of the variable declared in the type switch guard.  Empty if there
+  // is no variable declared.
+  std::string name_;
   // The expression we are switching on if there is no variable.
   Expression* expr_;
   // The type case clauses.