Merge pull request swiftlang#30230 from DougGregor/function-builder-switch-checking

[Constraint system] Semantic checking for switch statements in function builders

Author: DougGregor

lib/Sema/BuilderTransform.cpp

@@ -1114,6 +1114,7 @@ class BuilderClosureRewriter
     }
 
     // Translate all of the cases.
+    bool limitExhaustivityChecks = false;
     assert(target.kind == FunctionBuilderTarget::TemporaryVar);
     auto temporaryVar = target.captured.first;
     unsigned caseIndex = 0;
@@ -1124,10 +1125,18 @@ class BuilderClosureRewriter
               temporaryVar, {target.captured.second[caseIndex]})))
         return nullptr;
 
+      // Check restrictions on '@unknown'.
+      if (caseStmt->hasUnknownAttr()) {
+        checkUnknownAttrRestrictions(
+            cs.getASTContext(), caseStmt, /*fallthroughDest=*/nullptr,
+            limitExhaustivityChecks);
+      }
+
       ++caseIndex;
     }
 
-    TypeChecker::checkSwitchExhaustiveness(switchStmt, dc, /*limited=*/false);
+    TypeChecker::checkSwitchExhaustiveness(
+        switchStmt, dc, limitExhaustivityChecks);
 
     return switchStmt;
   }

lib/Sema/TypeCheckStmt.cpp

@@ -1035,40 +1035,6 @@ class StmtChecker : public StmtVisitor<StmtChecker, Stmt*> {
     std::swap(*prevCaseDecls, *nextCaseDecls);
   }
 
-  void checkUnknownAttrRestrictions(CaseStmt *caseBlock,
-                                    bool &limitExhaustivityChecks) {
-    if (caseBlock->getCaseLabelItems().size() != 1) {
-      assert(!caseBlock->getCaseLabelItems().empty() &&
-             "parser should not produce case blocks with no items");
-      getASTContext().Diags.diagnose(caseBlock->getLoc(),
-                                     diag::unknown_case_multiple_patterns)
-          .highlight(caseBlock->getCaseLabelItems()[1].getSourceRange());
-      limitExhaustivityChecks = true;
-    }
-
-    if (FallthroughDest != nullptr) {
-      if (!caseBlock->isDefault())
-        getASTContext().Diags.diagnose(caseBlock->getLoc(),
-                                       diag::unknown_case_must_be_last);
-      limitExhaustivityChecks = true;
-    }
-
-    const auto &labelItem = caseBlock->getCaseLabelItems().front();
-    if (labelItem.getGuardExpr() && !labelItem.isDefault()) {
-      getASTContext().Diags.diagnose(labelItem.getStartLoc(),
-                                     diag::unknown_case_where_clause)
-          .highlight(labelItem.getGuardExpr()->getSourceRange());
-    }
-
-    const Pattern *pattern =
-        labelItem.getPattern()->getSemanticsProvidingPattern();
-    if (!isa<AnyPattern>(pattern)) {
-      getASTContext().Diags.diagnose(labelItem.getStartLoc(),
-                                     diag::unknown_case_must_be_catchall)
-          .highlight(pattern->getSourceRange());
-    }
-  }
-
   void checkFallthroughPatternBindingsAndTypes(CaseStmt *caseBlock,
                                                CaseStmt *previousBlock) {
     auto firstPattern = caseBlock->getCaseLabelItems()[0].getPattern();
@@ -1236,7 +1202,9 @@ class StmtChecker : public StmtVisitor<StmtChecker, Stmt*> {
 
       // Check restrictions on '@unknown'.
       if (caseBlock->hasUnknownAttr()) {
-        checkUnknownAttrRestrictions(caseBlock, limitExhaustivityChecks);
+        checkUnknownAttrRestrictions(
+            getASTContext(), caseBlock, FallthroughDest,
+            limitExhaustivityChecks);
       }
 
       // If the previous case fellthrough, similarly check that that case's
@@ -2067,23 +2035,81 @@ void TypeChecker::typeCheckTopLevelCodeDecl(TopLevelCodeDecl *TLCD) {
   performTopLevelDeclDiagnostics(TLCD);
 }
 
+void swift::checkUnknownAttrRestrictions(
+    ASTContext &ctx, CaseStmt *caseBlock, CaseStmt *fallthroughDest,
+    bool &limitExhaustivityChecks) {
+  if (caseBlock->getCaseLabelItems().size() != 1) {
+    assert(!caseBlock->getCaseLabelItems().empty() &&
+           "parser should not produce case blocks with no items");
+    ctx.Diags.diagnose(caseBlock->getLoc(),
+                       diag::unknown_case_multiple_patterns)
+        .highlight(caseBlock->getCaseLabelItems()[1].getSourceRange());
+    limitExhaustivityChecks = true;
+  }
+
+  if (fallthroughDest != nullptr) {
+    if (!caseBlock->isDefault())
+      ctx.Diags.diagnose(caseBlock->getLoc(),
+                         diag::unknown_case_must_be_last);
+    limitExhaustivityChecks = true;
+  }
+
+  const auto &labelItem = caseBlock->getCaseLabelItems().front();
+  if (labelItem.getGuardExpr() && !labelItem.isDefault()) {
+    ctx.Diags.diagnose(labelItem.getStartLoc(),
+                                   diag::unknown_case_where_clause)
+        .highlight(labelItem.getGuardExpr()->getSourceRange());
+  }
+
+  const Pattern *pattern =
+      labelItem.getPattern()->getSemanticsProvidingPattern();
+  if (!isa<AnyPattern>(pattern)) {
+    ctx.Diags.diagnose(labelItem.getStartLoc(),
+                       diag::unknown_case_must_be_catchall)
+        .highlight(pattern->getSourceRange());
+  }
+}
+
 void swift::bindSwitchCasePatternVars(CaseStmt *caseStmt) {
-  llvm::SmallDenseMap<Identifier, VarDecl *, 4> latestVars;
+  llvm::SmallDenseMap<Identifier, std::pair<VarDecl *, bool>, 4> latestVars;
   auto recordVar = [&](VarDecl *var) {
     if (!var->hasName())
       return;
 
     // If there is an existing variable with this name, set it as the
     // parent of this new variable.
     auto &entry = latestVars[var->getName()];
-    if (entry) {
+    if (entry.first) {
       assert(!var->getParentVarDecl() ||
-             var->getParentVarDecl() == entry);
-      var->setParentVarDecl(entry);
+             var->getParentVarDecl() == entry.first);
+      var->setParentVarDecl(entry.first);
+
+      // Check for a mutability mismatch.
+      if (entry.second != var->isLet()) {
+        // Find the original declaration.
+        auto initialCaseVarDecl = entry.first;
+        while (auto parentVar = initialCaseVarDecl->getParentVarDecl())
+          initialCaseVarDecl = parentVar;
+
+        auto diag = var->diagnose(diag::mutability_mismatch_multiple_pattern_list,
+                                  var->isLet(), initialCaseVarDecl->isLet());
+
+        VarPattern *foundVP = nullptr;
+        var->getParentPattern()->forEachNode([&](Pattern *P) {
+          if (auto *VP = dyn_cast<VarPattern>(P))
+            if (VP->getSingleVar() == var)
+              foundVP = VP;
+        });
+        if (foundVP)
+          diag.fixItReplace(foundVP->getLoc(),
+                            initialCaseVarDecl->isLet() ? "let" : "var");
+      }
+    } else {
+      entry.second = var->isLet();
     }
 
     // Record this variable as the latest with this name.
-    entry = var;
+    entry.first = var;
   };
 
   // Wire up the parent var decls for each variable that occurs within

lib/Sema/TypeChecker.h

@@ -1654,6 +1654,12 @@ bool areGenericRequirementsSatisfied(const DeclContext *DC,
                                      SubstitutionMap Substitutions,
                                      bool isExtension);
 
+/// Check for restrictions on the use of the @unknown attribute on a
+/// case statement.
+void checkUnknownAttrRestrictions(
+    ASTContext &ctx, CaseStmt *caseBlock, CaseStmt *fallthroughDest,
+    bool &limitExhaustivityChecks);
+
 /// Bind all of the pattern variables that occur within a case statement and
 /// all of its case items to their "parent" pattern variables, forming chains
 /// of variables with the same name.

test/Constraints/function_builder.swift

@@ -593,7 +593,8 @@ testSwitch(getE(1))
 // CHECK-SAME: second("just 42")
 testSwitch(getE(2))
 
-func testSwitchCombined(_ e: E) {
+func testSwitchCombined(_ eIn: E) {
+  var e = eIn
   tuplify(true) { c in
     "testSwitchCombined"
     switch e {

test/Constraints/function_builder_diags.swift

@@ -427,3 +427,42 @@ struct TestConstraintGenerationErrors {
     }
   }
 }
+
+// Check @unknown
+func testUnknownInSwitchSwitch(e: E) {
+    tuplify(true) { c in
+    "testSwitch"
+    switch e {
+    @unknown case .a: // expected-error{{'@unknown' is only supported for catch-all cases ("case _")}}
+      "a"
+    case .b(let i, let s?):
+      i * 2
+      s + "!"
+    default:
+      "nothing"
+    }
+  }
+}
+
+// Check for mutability mismatches when there are multiple case items
+// referring to same-named variables.
+enum E3 {
+  case a(Int, String)
+  case b(String, Int)
+  case c(String, Int)
+}
+
+func testCaseMutabilityMismatches(e: E3) {
+    tuplify(true) { c in
+    "testSwitch"
+    switch e {
+    case .a(let x, var y),
+         .b(let y, // expected-error{{'let' pattern binding must match previous 'var' pattern binding}}
+            var x), // expected-error{{'var' pattern binding must match previous 'let' pattern binding}}
+         .c(let y, // expected-error{{'let' pattern binding must match previous 'var' pattern binding}}
+            var x): // expected-error{{'var' pattern binding must match previous 'let' pattern binding}}
+      x
+      y += "a"
+    }
+  }
+}