[CS] Diagnose UnresolvedPatternExprs as part of constraint solving

Instead of diagnosing in CSApply, let's create a
fix and diagnose in the solver instead.
Additionally, make sure we assign ErrorTypes to
any VarDecls bound by the invalid pattern, which
fixes a crash.

rdar://110638279

Author: hamishknight

include/swift/Sema/CSFix.h

@@ -306,6 +306,9 @@ enum class FixKind : uint8_t {
   /// This issue should already have been diagnosed elsewhere.
   IgnoreUnresolvedPatternVar,
 
+  /// Ignore a nested UnresolvedPatternExpr in an ExprPattern, which is invalid.
+  IgnoreInvalidPatternInExpr,
+
   /// Resolve type of `nil` by providing a contextual type.
   SpecifyContextualTypeForNil,
 
@@ -2959,6 +2962,33 @@ class IgnoreUnresolvedPatternVar final : public ConstraintFix {
   }
 };
 
+class IgnoreInvalidPatternInExpr final : public ConstraintFix {
+  Pattern *P;
+
+  IgnoreInvalidPatternInExpr(ConstraintSystem &cs, Pattern *pattern,
+                             ConstraintLocator *locator)
+      : ConstraintFix(cs, FixKind::IgnoreInvalidPatternInExpr, locator),
+        P(pattern) {}
+
+public:
+  std::string getName() const override {
+    return "ignore invalid Pattern nested in Expr";
+  }
+
+  bool diagnose(const Solution &solution, bool asNote = false) const override;
+
+  bool diagnoseForAmbiguity(CommonFixesArray commonFixes) const override {
+    return diagnose(*commonFixes.front().first);
+  }
+
+  static IgnoreInvalidPatternInExpr *
+  create(ConstraintSystem &cs, Pattern *pattern, ConstraintLocator *locator);
+
+  static bool classof(const ConstraintFix *fix) {
+    return fix->getKind() == FixKind::IgnoreInvalidPatternInExpr;
+  }
+};
+
 class SpecifyContextualTypeForNil final : public ConstraintFix {
   SpecifyContextualTypeForNil(ConstraintSystem &cs,
                               ConstraintLocator *locator)

lib/Sema/CSApply.cpp

@@ -4604,24 +4604,11 @@ namespace {
     Expr *visitDiscardAssignmentExpr(DiscardAssignmentExpr *expr) {
       return simplifyExprType(expr);
     }
-    
+
     Expr *visitUnresolvedPatternExpr(UnresolvedPatternExpr *expr) {
-      // If we end up here, we should have diagnosed somewhere else
-      // already.
-      Expr *simplified = simplifyExprType(expr);
-      // Invalidate 'VarDecl's inside the pattern.
-      expr->getSubPattern()->forEachVariable([](VarDecl *VD) {
-        VD->setInvalid();
-      });
-      if (!SuppressDiagnostics
-          && !cs.getType(simplified)->is<UnresolvedType>()) {
-        auto &de = cs.getASTContext().Diags;
-        de.diagnose(simplified->getLoc(), diag::pattern_in_expr,
-                    expr->getSubPattern()->getDescriptiveKind());
-      }
-      return simplified;
+      llvm_unreachable("Should have diagnosed");
     }
-    
+
     Expr *visitBindOptionalExpr(BindOptionalExpr *expr) {
       return simplifyExprType(expr);
     }

lib/Sema/CSDiagnostics.cpp

@@ -8303,6 +8303,32 @@ bool InvalidEmptyKeyPathFailure::diagnoseAsError() {
   return true;
 }
 
+bool InvalidPatternInExprFailure::diagnoseAsError() {
+  // Check to see if we have something like 'case <fn>(let foo)', where <fn>
+  // has a fix associated with it. In such a case, it's more likely than not
+  // that the user is trying to write an EnumElementPattern, but has made some
+  // kind of mistake in the function expr that causes it to be treated as an
+  // ExprPattern. Emitting an additional error for the out of place 'let foo' is
+  // just noise in that case, so let's avoid diagnosing.
+  llvm::SmallPtrSet<Expr *, 4> fixAnchors;
+  for (auto *fix : getSolution().Fixes) {
+    if (auto *anchor = getAsExpr(fix->getAnchor()))
+      fixAnchors.insert(anchor);
+  }
+  {
+    auto *E = castToExpr(getLocator()->getAnchor());
+    while (auto *parent = findParentExpr(E)) {
+      if (auto *CE = dyn_cast<CallExpr>(parent)) {
+        if (fixAnchors.contains(CE->getFn()))
+          return false;
+      }
+      E = parent;
+    }
+  }
+  emitDiagnostic(diag::pattern_in_expr, P->getDescriptiveKind());
+  return true;
+}
+
 bool MissingContextualTypeForNil::diagnoseAsError() {
   auto *expr = castToExpr<NilLiteralExpr>(getAnchor());
 

lib/Sema/CSDiagnostics.h

@@ -2531,6 +2531,22 @@ class InvalidEmptyKeyPathFailure final : public FailureDiagnostic {
   bool diagnoseAsError() override;
 };
 
+/// Diagnose an invalid Pattern node in an ExprPattern.
+///
+/// \code
+/// if case foo(let x) = y {}
+/// \endcode
+class InvalidPatternInExprFailure final : public FailureDiagnostic {
+  Pattern *P;
+
+public:
+  InvalidPatternInExprFailure(const Solution &solution, Pattern *pattern,
+                              ConstraintLocator *locator)
+      : FailureDiagnostic(solution, locator), P(pattern) {}
+
+  bool diagnoseAsError() override;
+};
+
 /// Diagnose situations where there is no context to determine a
 /// type of `nil` literal e.g.
 ///

lib/Sema/CSFix.cpp

@@ -2080,6 +2080,19 @@ IgnoreInvalidASTNode *IgnoreInvalidASTNode::create(ConstraintSystem &cs,
   return new (cs.getAllocator()) IgnoreInvalidASTNode(cs, locator);
 }
 
+bool IgnoreInvalidPatternInExpr::diagnose(const Solution &solution,
+                                          bool asNote) const {
+  InvalidPatternInExprFailure failure(solution, P, getLocator());
+  return failure.diagnose(asNote);
+}
+
+IgnoreInvalidPatternInExpr *
+IgnoreInvalidPatternInExpr::create(ConstraintSystem &cs, Pattern *pattern,
+                                   ConstraintLocator *locator) {
+  return new (cs.getAllocator())
+      IgnoreInvalidPatternInExpr(cs, pattern, locator);
+}
+
 bool SpecifyContextualTypeForNil::diagnose(const Solution &solution,
                                            bool asNote) const {
   MissingContextualTypeForNil failure(solution, getLocator());

lib/Sema/CSGen.cpp

@@ -3478,16 +3478,18 @@ namespace {
     }
 
     Type visitUnresolvedPatternExpr(UnresolvedPatternExpr *expr) {
-      // If there are UnresolvedPatterns floating around after pattern type
-      // checking, they are definitely invalid. However, we will
-      // diagnose that condition elsewhere; to avoid unnecessary noise errors,
-      // just plop an open type variable here.
-      
-      auto locator = CS.getConstraintLocator(expr);
-      auto typeVar = CS.createTypeVariable(locator,
-                                           TVO_CanBindToLValue |
-                                           TVO_CanBindToNoEscape);
-      return typeVar;
+      // Encountering an UnresolvedPatternExpr here means we have an invalid
+      // ExprPattern with a Pattern node like 'let x' nested in it. Record a
+      // fix, and assign ErrorTypes to any VarDecls bound.
+      auto *locator = CS.getConstraintLocator(expr);
+      auto *P = expr->getSubPattern();
+      CS.recordFix(IgnoreInvalidPatternInExpr::create(CS, P, locator));
+
+      P->forEachVariable([&](VarDecl *VD) {
+        CS.setType(VD, ErrorType::get(CS.getASTContext()));
+      });
+
+      return CS.createTypeVariable(locator, TVO_CanBindToHole);
     }
 
     /// Get the type T?

lib/Sema/CSSimplify.cpp

@@ -14871,6 +14871,7 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyFixConstraint(
   case FixKind::AddExplicitExistentialCoercion:
   case FixKind::DestructureTupleToMatchPackExpansionParameter:
   case FixKind::AllowValueExpansionWithoutPackReferences:
+  case FixKind::IgnoreInvalidPatternInExpr:
   case FixKind::IgnoreMissingEachKeyword:
     llvm_unreachable("handled elsewhere");
   }

test/Constraints/issue-66553.swift

@@ -0,0 +1,12 @@
+// RUN: %target-typecheck-verify-swift
+
+// https://github.com/apple/swift/issues/66553
+
+func baz(y: [Int], z: Int) -> Int {
+  switch z {
+  case y[let z]: // expected-error {{'let' binding pattern cannot appear in an expression}}
+    z
+  default:
+    z
+  }
+}

test/Constraints/rdar105782480.swift

@@ -12,6 +12,7 @@ func foo(value: MyEnum) {
     switch value {
     case .second(let drag).invalid:
       // expected-error@-1 {{value of type 'MyEnum' has no member 'invalid'}}
+      // expected-error@-2 {{'let' binding pattern cannot appear in an expression}}
       break
     }
   }

test/Constraints/rdar106598067.swift

@@ -3,9 +3,10 @@
 enum E: Error { case e }
 
 // rdar://106598067 – Make sure we don't crash.
-// FIXME: Bad diagnostic (the issue is that it should be written 'as', not 'as?')
+// FIXME: We ought to have a tailored diagnostic to change to 'as' instead of 'as?'
 let fn = {
-  // expected-error@-1 {{unable to infer closure type without a type annotation}}
   do {} catch let x as? E {}
-  // expected-warning@-1 {{'catch' block is unreachable because no errors are thrown in 'do' block}}
+  // expected-error@-1 {{pattern variable binding cannot appear in an expression}}
+  // expected-error@-2 {{expression pattern of type 'E?' cannot match values of type 'any Error'}}
+  // expected-warning@-3 {{'catch' block is unreachable because no errors are thrown in 'do' block}}
 }