Fix name lookup and parser so that where clauses decl refs in case statements correctly bind to the current pattern instead of always to the first pattern. Thus the hacky var decl juggling in where clauses in SILGen can be deleted.

Author: gregomni

lib/AST/NameLookupImpl.h

@@ -214,21 +214,20 @@ class FindLocalVal : public StmtVisitor<FindLocalVal> {
       return;
     // Pattern names aren't visible in the patterns themselves,
     // just in the body or in where guards.
-    auto body = S->getBody();
     bool inPatterns = isReferencePointInRange(S->getLabelItemsRange());
     auto items = S->getCaseLabelItems();
     if (inPatterns) {
       for (const auto &CLI : items) {
         auto guard = CLI.getGuardExpr();
         if (guard && isReferencePointInRange(guard->getSourceRange())) {
-          inPatterns = false;
+          checkPattern(CLI.getPattern(), DeclVisibilityKind::LocalVariable);
           break;
         }
       }
     }
     if (!inPatterns && !items.empty())
       checkPattern(items[0].getPattern(), DeclVisibilityKind::LocalVariable);
-    visit(body);
+    visit(S->getBody());
   }
 
   void visitDoCatchStmt(DoCatchStmt *S) {

lib/Parse/ParseStmt.cpp

@@ -968,6 +968,47 @@ namespace {
   };
 } // unnamed namespace
 
+static void parseWhereGuard(Parser &P, GuardedPattern &result,
+                            ParserStatus &status,
+                            GuardedPatternContext parsingContext,
+                            bool isExprBasic) {
+  if (P.Tok.is(tok::kw_where)) {
+    SyntaxParsingContext WhereClauseCtxt(P.SyntaxContext,
+                                         SyntaxKind::WhereClause);
+    result.WhereLoc = P.consumeToken(tok::kw_where);
+    SourceLoc startOfGuard = P.Tok.getLoc();
+
+    auto diagKind = [=]() -> Diag<> {
+      switch (parsingContext) {
+      case GuardedPatternContext::Case:
+        return diag::expected_case_where_expr;
+      case GuardedPatternContext::Catch:
+        return diag::expected_catch_where_expr;
+      }
+      llvm_unreachable("bad context");
+    }();
+    ParserResult<Expr> guardResult = P.parseExprImpl(diagKind, isExprBasic);
+    status |= guardResult;
+
+    // Use the parsed guard expression if possible.
+    if (guardResult.isNonNull()) {
+      result.Guard = guardResult.get();
+
+      // Otherwise, fake up an ErrorExpr.
+    } else {
+      // If we didn't consume any tokens failing to parse the
+      // expression, don't put in the source range of the ErrorExpr.
+      SourceRange errorRange;
+      if (startOfGuard == P.Tok.getLoc()) {
+        errorRange = result.WhereLoc;
+      } else {
+        errorRange = SourceRange(startOfGuard, P.PreviousLoc);
+      }
+      result.Guard = new (P.Context) ErrorExpr(errorRange);
+    }
+  }
+}
+
 /// Parse a pattern-matching clause for a case or catch statement,
 /// including the guard expression:
 ///
@@ -1045,7 +1086,6 @@ static void parseGuardedPattern(Parser &P, GuardedPattern &result,
     patternResult = makeParserResult(varPattern);
   }
 
-
   // Okay, if the special code-completion didn't kick in, parse a
   // matching pattern.
   if (patternResult.isNull()) {
@@ -1074,26 +1114,28 @@ static void parseGuardedPattern(Parser &P, GuardedPattern &result,
       if (VD->hasName()) P.addToScope(VD);
       boundDecls.push_back(VD);
     });
+
+    // Now that we have them, mark them as being initialized without a PBD.
+    for (auto VD : boundDecls)
+      VD->setHasNonPatternBindingInit();
+
+    // Parse the optional 'where' guard.
+    parseWhereGuard(P, result, status, parsingContext, isExprBasic);
   } else {
     // If boundDecls already contains variables, then we must match the
     // same number and same names in this pattern as were declared in a
     // previous pattern (and later we will make sure they have the same
     // types).
+    Scope guardScope(&P, ScopeKind::CaseVars);
     SmallVector<VarDecl*, 4> repeatedDecls;
     patternResult.get()->forEachVariable([&](VarDecl *VD) {
       if (!VD->hasName())
         return;
 
-      for (auto repeat : repeatedDecls)
-        if (repeat->getName() == VD->getName())
-          P.addToScope(VD); // will diagnose a duplicate declaration
-
       bool found = false;
       for (auto previous : boundDecls) {
         if (previous->hasName() && previous->getName() == VD->getName()) {
           found = true;
-          // Use the same local discriminator.
-          VD->setLocalDiscriminator(previous->getLocalDiscriminator());
           break;
         }
       }
@@ -1103,6 +1145,9 @@ static void parseGuardedPattern(Parser &P, GuardedPattern &result,
         status.setIsParseError();
       }
       repeatedDecls.push_back(VD);
+      P.setLocalDiscriminator(VD);
+      if (VD->hasName())
+        P.addToScope(VD);
     });
 
     for (auto previous : boundDecls) {
@@ -1124,47 +1169,10 @@ static void parseGuardedPattern(Parser &P, GuardedPattern &result,
       VD->setHasNonPatternBindingInit();
       VD->setImplicit();
     }
-  }
-  
-  // Now that we have them, mark them as being initialized without a PBD.
-  for (auto VD : boundDecls)
-    VD->setHasNonPatternBindingInit();
-
-  // Parse the optional 'where' guard.
-  if (P.Tok.is(tok::kw_where)) {
-    SyntaxParsingContext WhereClauseCtxt(P.SyntaxContext,
-                                         SyntaxKind::WhereClause);
-    result.WhereLoc = P.consumeToken(tok::kw_where);
-    SourceLoc startOfGuard = P.Tok.getLoc();
 
-    auto diagKind = [=]() -> Diag<> {
-      switch (parsingContext) {
-      case GuardedPatternContext::Case:
-        return diag::expected_case_where_expr;
-      case GuardedPatternContext::Catch:
-        return diag::expected_catch_where_expr;
-      }
-      llvm_unreachable("bad context");
-    }();
-    ParserResult<Expr> guardResult = P.parseExprImpl(diagKind, isExprBasic);
-    status |= guardResult;
-
-    // Use the parsed guard expression if possible.
-    if (guardResult.isNonNull()) {
-      result.Guard = guardResult.get();
-
-    // Otherwise, fake up an ErrorExpr.
-    } else {
-      // If we didn't consume any tokens failing to parse the
-      // expression, don't put in the source range of the ErrorExpr.
-      SourceRange errorRange;
-      if (startOfGuard == P.Tok.getLoc()) {
-        errorRange = result.WhereLoc;
-      } else {
-        errorRange = SourceRange(startOfGuard, P.PreviousLoc);
-      }
-      result.Guard = new (P.Context) ErrorExpr(errorRange);
-    }
+    // Parse the optional 'where' guard, with this particular pattern's bound
+    // vars in scope.
+    parseWhereGuard(P, result, status, parsingContext, isExprBasic);
   }
 }
 

lib/SILGen/SILGenPattern.cpp

@@ -451,10 +451,9 @@ class PatternMatchEmission {
 
   void bindRefutablePatterns(const ClauseRow &row, ArgArray args,
                              const FailureHandler &failure);
+
   void emitGuardBranch(SILLocation loc, Expr *guard,
-                       const FailureHandler &failure,
-                       Pattern *usingImplicitVariablesFromPattern,
-                       CaseStmt *usingImplicitVariablesFromStmt);
+                       const FailureHandler &failure);
 
   void bindIrrefutablePatterns(const ClauseRow &row, ArgArray args,
                                bool forIrrefutableRow, bool hasMultipleItems);
@@ -1076,19 +1075,17 @@ void PatternMatchEmission::emitWildcardDispatch(ClauseMatrix &clauses,
   auto stmt = clauses[row].getClientData<Stmt>();
   assert(isa<CaseStmt>(stmt) || isa<CatchStmt>(stmt));
 
-  bool hasMultipleItems = false;
-  if (auto *caseStmt = dyn_cast<CaseStmt>(stmt)) {
-    hasMultipleItems = clauses[row].hasFallthroughTo() ||
-      caseStmt->getCaseLabelItems().size() > 1;
-  }
+  auto *caseStmt = dyn_cast<CaseStmt>(stmt);
+  bool hasMultipleItems =
+      caseStmt && (clauses[row].hasFallthroughTo() ||
+                   caseStmt->getCaseLabelItems().size() > 1);
 
   // Bind the rest of the patterns.
   bindIrrefutablePatterns(clauses[row], args, !hasGuard, hasMultipleItems);
 
   // Emit the guard branch, if it exists.
   if (guardExpr) {
-    this->emitGuardBranch(guardExpr, guardExpr, failure,
-                      clauses[row].getCasePattern(), dyn_cast<CaseStmt>(stmt));
+    this->emitGuardBranch(guardExpr, guardExpr, failure);
   }
 
   // Enter the row.
@@ -1120,8 +1117,7 @@ bindRefutablePatterns(const ClauseRow &row, ArgArray args,
       FullExpr scope(SGF.Cleanups, CleanupLocation(pattern));
       bindVariable(pattern, exprPattern->getMatchVar(), args[i],
                    /*isForSuccess*/ false, /* hasMultipleItems */ false);
-      emitGuardBranch(pattern, exprPattern->getMatchExpr(), failure, nullptr,
-                      nullptr);
+      emitGuardBranch(pattern, exprPattern->getMatchExpr(), failure);
       break;
     }
     default:
@@ -1197,26 +1193,15 @@ void PatternMatchEmission::bindVariable(Pattern *pattern, VarDecl *var,
 /// Evaluate a guard expression and, if it returns false, branch to
 /// the given destination.
 void PatternMatchEmission::emitGuardBranch(SILLocation loc, Expr *guard,
-                                   const FailureHandler &failure,
-                                   Pattern *usingImplicitVariablesFromPattern,
-                                   CaseStmt *usingImplicitVariablesFromStmt) {
+                                           const FailureHandler &failure) {
   SILBasicBlock *falseBB = SGF.B.splitBlockForFallthrough();
   SILBasicBlock *trueBB = SGF.B.splitBlockForFallthrough();
 
   // Emit the match test.
   SILValue testBool;
   {
     FullExpr scope(SGF.Cleanups, CleanupLocation(guard));
-    auto emitTest = [&]{
-      testBool = SGF.emitRValueAsSingleValue(guard).getUnmanagedValue();
-    };
-    
-    if (usingImplicitVariablesFromPattern)
-      SGF.usingImplicitVariablesForPattern(usingImplicitVariablesFromPattern,
-                                           usingImplicitVariablesFromStmt,
-                                           emitTest);
-    else
-      emitTest();
+    testBool = SGF.emitRValueAsSingleValue(guard).getUnmanagedValue();
   }
 
   SGF.B.createCondBranch(loc, testBool, trueBB, falseBB);
@@ -2442,47 +2427,6 @@ class Lowering::PatternMatchContext {
   PatternMatchEmission &Emission;
 };
 
-void SILGenFunction::usingImplicitVariablesForPattern(Pattern *pattern, CaseStmt *stmt,
-                                                      const llvm::function_ref<void(void)> &f) {
-  // Early exit for CatchStmt
-  if (!stmt) {
-    f();
-    return;
-  }
-
-  ArrayRef<CaseLabelItem> labelItems = stmt->getCaseLabelItems();
-  auto expectedPattern = labelItems[0].getPattern();
-  
-  if (labelItems.size() <= 1 || pattern == expectedPattern) {
-    f();
-    return;
-  }
-
-  // Remap vardecls that the case body is expecting to the pattern var locations
-  // for the given pattern, emit whatever, and switch back.
-  SmallVector<VarDecl *, 4> Vars;
-  expectedPattern->collectVariables(Vars);
-  
-  auto variableSwapper = [&] {
-    pattern->forEachVariable([&](VarDecl *VD) {
-      if (!VD->hasName())
-        return;
-      for (auto expected : Vars) {
-        if (expected->hasName() && expected->getName() == VD->getName()) {
-          auto swap = VarLocs[expected];
-          VarLocs[expected] = VarLocs[VD];
-          VarLocs[VD] = swap;
-          return;
-        }
-      }
-    });
-  };
-  
-  variableSwapper();
-  f();
-  variableSwapper();
-}
-
 static void emitDiagnoseOfUnexpectedEnumCaseValue(SILGenFunction &SGF,
                                                   SILLocation loc,
                                                   ManagedValue value,

test/refactoring/rename/Outputs/local/casebind_2.swift.expected

@@ -21,7 +21,7 @@ func test3(arg: Int?) {
   case let .some(x) where x == 0:
     print(x)
   case let .some(xRenamed) where xRenamed == 1,
-       let .some(x) where xRenamed == 2: // FIXME: This 'x' in '.some(x)' isn't properly renamed in 'casebind_2' case.
+       let .some(x) where x == 2: // FIXME: This 'x' in '.some(x)' isn't properly renamed in 'casebind_2' case.
     print(xRenamed)
   default:
     break