Add a separate scope for switch

src/parser.cc

Index: src/parser.cc
diff --git a/src/parser.cc b/src/parser.cc
index a7a31e7173593e67036cf4ffb9de14bb79f2e478..d6e3c94439cc13529f819155192a425db7372c66 100644
--- a/src/parser.cc
+++ b/src/parser.cc
@@ -2964,31 +2964,82 @@ CaseClause* Parser::ParseCaseClause(bool* default_seen_ptr, bool* ok) {
 }
 
 
-SwitchStatement* Parser::ParseSwitchStatement(
-    ZoneList<const AstRawString*>* labels, bool* ok) {
+Statement* Parser::ParseSwitchStatement(ZoneList<const AstRawString*>* labels,
+                                        bool* ok) {
   // SwitchStatement ::
   //   'switch' '(' Expression ')' '{' CaseClause* '}'
+  // In order to get the CaseClauses to execute in their own lexical scope,
+  // but without requiring downstream code to have special scope handling
+  // code for switch statements, desugar into blocks as follows:
+  // {  // To group the statements--harmless to evaluate Expression in scope
+  //   .tag_variable = Expression;
+  //   {  // To give CaseClauses a scope
+  //     switch (.tag_variable) { CaseClause* }
+  //   }
+  // }
 
-  SwitchStatement* statement =
-      factory()->NewSwitchStatement(labels, peek_position());
-  Target target(&this->target_stack_, statement);
+  Block* switch_block =
+      factory()->NewBlock(labels, 2, true, RelocInfo::kNoPosition);

[adamk, 2015/08/21 01:19:08]

Seems weird that you pass labels both here and to the switch statement. Are they
both needed? Given that all the blocks end at the same place, it seems like you
should only need the labels (and the target) in one place. Probably on the
switch?

Can you add some tests with labeled breaks inside switch, with labels at various
places (on a block containing a switch, on the switch itself, on a block
contained in the switch)?

[Dan Ehrenberg, 2015/08/21 23:06:14]

Done

+  Scope* switch_scope = NewScope(scope_, BLOCK_SCOPE);

[adamk, 2015/08/21 01:19:08]

As discussed, I don't think you need this scope (and in fact it'll already be
dropped for you). It's fine to have a block without a scope.

[Dan Ehrenberg, 2015/08/21 23:06:14]

Removed it.

+  int switch_pos = peek_position();
 
-  Expect(Token::SWITCH, CHECK_OK);
-  Expect(Token::LPAREN, CHECK_OK);
-  Expression* tag = ParseExpression(true, CHECK_OK);
-  Expect(Token::RPAREN, CHECK_OK);
+  {
+    BlockState switch_block_state(&scope_, switch_scope);
+    // This target exists in case the tag has a do-expression

[adamk, 2015/08/21 01:19:08]

This comment likely goes away if you simplify the way labels/targets are used
(and it should go away, as do-expressions are not yet a thing)

[Dan Ehrenberg, 2015/08/21 23:06:14]

Done

+    Target target(&this->target_stack_, switch_block);
 
-  bool default_seen = false;
-  ZoneList<CaseClause*>* cases = new(zone()) ZoneList<CaseClause*>(4, zone());
-  Expect(Token::LBRACE, CHECK_OK);
-  while (peek() != Token::RBRACE) {
-    CaseClause* clause = ParseCaseClause(&default_seen, CHECK_OK);
-    cases->Add(clause, zone());
+    Expect(Token::SWITCH, CHECK_OK);
+    Expect(Token::LPAREN, CHECK_OK);
+    Expression* tag = ParseExpression(true, CHECK_OK);
+    Expect(Token::RPAREN, CHECK_OK);
+
+    Variable* tag_variable =
+        scope_->NewTemporary(ast_value_factory()->dot_switch_tag_string());
+    Assignment* tag_assign = factory()->NewAssignment(
+        Token::ASSIGN, factory()->NewVariableProxy(tag_variable), tag,
+        tag->position());
+    Statement* tag_statement =
+        factory()->NewExpressionStatement(tag_assign, RelocInfo::kNoPosition);
+    switch_block->AddStatement(tag_statement, zone());
+
+    Block* cases_block =
+        factory()->NewBlock(NULL, 1, true, RelocInfo::kNoPosition);
+    Scope* cases_scope = NewScope(scope_, BLOCK_SCOPE);
+
+    SwitchStatement* switch_statement =
+        factory()->NewSwitchStatement(labels, switch_pos);

[adamk, 2015/08/21 01:19:07]

See above, I'd drop the labels here (pass NULL).

[Dan Ehrenberg, 2015/08/21 23:06:14]

Passed the labels here, as you suggested above.

+
+    ZoneList<CaseClause*>* cases;
+    cases_scope->set_start_position(scanner()->location().beg_pos);
+    {
+      BlockState cases_block_state(&scope_, cases_scope);
+      Target target(&this->target_stack_, switch_statement);
+
+      Expression* tag_read = factory()->NewVariableProxy(tag_variable);
+
+      bool default_seen = false;
+      cases = new (zone()) ZoneList<CaseClause*>(4, zone());

[adamk, 2015/08/21 01:19:08]

Can you move this initialization up to where cases is declared? Seems like
that'd be simpler.

[Dan Ehrenberg, 2015/08/21 23:06:14]

Moved the declaration down to here.

+      Expect(Token::LBRACE, CHECK_OK);
+      while (peek() != Token::RBRACE) {
+        CaseClause* clause = ParseCaseClause(&default_seen, CHECK_OK);
+        cases->Add(clause, zone());
+      }
+      switch_statement->Initialize(tag_read, cases);
+      cases_block->AddStatement(switch_statement, zone());
+    }
+    cases_scope->set_end_position(scanner()->location().end_pos);
+    cases_scope = cases_scope->FinalizeBlockScope();
+    cases_block->set_scope(cases_scope);
+
+    Expect(Token::RBRACE, CHECK_OK);

[adamk, 2015/08/21 01:19:08]

I think this should move above the scope stuff just above, the closing brace is
effectively the end of the scope.

Can you update the comment about end positions in scopes.h to mention switch
while you're at it?

[Dan Ehrenberg, 2015/08/21 23:06:14]

Done

+
+    switch_block->AddStatement(cases_block, zone());
   }
-  Expect(Token::RBRACE, CHECK_OK);
+  switch_scope->set_end_position(scanner()->location().end_pos);
+  switch_scope = switch_scope->FinalizeBlockScope();
+  switch_block->set_scope(switch_scope);
 
-  if (statement) statement->Initialize(tag, cases);
-  return statement;
+  return switch_block;
 }