Sema: Rip out some code for diagnosing invalid patterns

The problem here is that we would just emit 'invalid pattern'
instead of digging deeper, which meant that the fix-it for
qualified enum element access wasn't getting inserted for
more complex patterns, such as 'case X(let x)'.

Unfortunately, in the matching_patterns.swift test, we emit
too many diagnostics that are not really useful to figuring
out the problem, and the old 'invalid pattern' made more
sense. I'll work on some CSDiag tweaks to address this --
I think it makes more sense to dig there than just emit a
general 'invalid pattern' diagnostic anyway.

Fixes <rdar://problem/27684266>.

Author: slavapestov

lib/Sema/CSApply.cpp

@@ -3388,7 +3388,13 @@ namespace {
     }
 
     Expr *visitUnresolvedPatternExpr(UnresolvedPatternExpr *expr) {
-      llvm_unreachable("should have been eliminated during name binding");
+      // If we end up here, we should have diagnosed somewhere else
+      // already.
+      if (!SuppressDiagnostics) {
+        cs.TC.diagnose(expr->getLoc(), diag::pattern_in_expr,
+                       expr->getSubPattern()->getKind());
+      }
+      return expr;
     }
 
     Expr *visitBindOptionalExpr(BindOptionalExpr *expr) {

lib/Sema/CSGen.cpp

@@ -2627,9 +2627,7 @@ namespace {
     Type visitUnresolvedPatternExpr(UnresolvedPatternExpr *expr) {
       // If there are UnresolvedPatterns floating around after name binding,
       // they are pattern productions in invalid positions.
-      CS.TC.diagnose(expr->getLoc(), diag::pattern_in_expr,
-                     expr->getSubPattern()->getKind());
-      return Type();
+      return ErrorType::get(CS.getASTContext());
     }
 
     /// Get the type T?

lib/Sema/TypeCheckPattern.cpp

@@ -247,35 +247,18 @@ class ResolvePattern : public ASTVisitor<ResolvePattern,
 public:
   TypeChecker &TC;
   DeclContext *DC;
-  bool &DiagnosedError;
 
-  ResolvePattern(TypeChecker &TC, DeclContext *DC, bool &DiagnosedError)
-    : TC(TC), DC(DC), DiagnosedError(DiagnosedError) {}
+  ResolvePattern(TypeChecker &TC, DeclContext *DC) : TC(TC), DC(DC) {}
 
   // Convert a subexpression to a pattern if possible, or wrap it in an
   // ExprPattern.
   Pattern *getSubExprPattern(Expr *E) {
     if (Pattern *p = visit(E))
       return p;
 
-    foundUnknownExpr(E);
-    
     return new (TC.Context) ExprPattern(E, nullptr, nullptr);
   }
 
-  void foundUnknownExpr(Expr *E) {
-    // If we find unresolved pattern, diagnose this as an illegal pattern.  Sema
-    // does later checks for UnresolvedPatternExpr's in arbitrary places, but
-    // rejecting these early is good because we can provide better up-front
-    // diagnostics and can recover better from it.
-    if (!UnresolvedPatternFinder::hasAny(E) || DiagnosedError) return;
-    
-    TC.diagnose(E->getStartLoc(), diag::invalid_pattern)
-      .highlight(E->getSourceRange());
-    DiagnosedError = true;
-  }
-  
-
   // Handle productions that are always leaf patterns or are already resolved.
 #define ALWAYS_RESOLVED_PATTERN(Id) \
   Pattern *visit##Id##Pattern(Id##Pattern *P) { return P; }
@@ -295,7 +278,7 @@ class ResolvePattern : public ASTVisitor<ResolvePattern,
 
     // If the var pattern has no variables bound underneath it, then emit a
     // warning that the var/let is pointless.
-    if (!DiagnosedError && !P->isImplicit()) {
+    if (!P->isImplicit()) {
       bool HasVariable = false;
       P->forEachVariable([&](VarDecl *VD) { HasVariable = true; });
 
@@ -328,7 +311,6 @@ class ResolvePattern : public ASTVisitor<ResolvePattern,
     Pattern *exprAsPattern = visit(P->getSubExpr());
     // If we failed, keep the ExprPattern as is.
     if (!exprAsPattern) {
-      foundUnknownExpr(P->getSubExpr());
       P->setResolved(true);
       return P;
     }
@@ -606,10 +588,7 @@ class ResolvePattern : public ASTVisitor<ResolvePattern,
 /// disambiguate semantics-dependent pattern forms.
 Pattern *TypeChecker::resolvePattern(Pattern *P, DeclContext *DC,
                                      bool isStmtCondition) {
-  bool DiagnosedError = false;
-  P = ResolvePattern(*this, DC, DiagnosedError).visit(P);
-
-  if (DiagnosedError) return nullptr;
+  P = ResolvePattern(*this, DC).visit(P);
 
   // If the entire pattern is "(pattern_expr (type_expr SomeType))", then this
   // is an invalid pattern.  If it were actually a value comparison (with ~=)

test/Parse/enum.swift

@@ -479,7 +479,7 @@ enum SE0036 {
     switch self {
     case A: break // expected-error {{enum element 'A' cannot be referenced as an instance member}} {{10-10=.}}
     case B(_): break // expected-error {{enum element 'B' cannot be referenced as an instance member}} {{10-10=.}}
-    case C(let x): _ = x; break // expected-error {{invalid pattern}}
+    case C(let x): _ = x; break // expected-error {{enum element 'C' cannot be referenced as an instance member}} {{10-10=.}}
     }
   }
 

test/Parse/matching_patterns.swift

@@ -261,9 +261,14 @@ case (1, 2, 3):
   ()
 
 // patterns in expression-only positions are errors.
-case +++(_, var d, 3): // expected-error{{invalid pattern}}
+case +++(_, var d, 3):
+// expected-error@-1{{'+++' is not a prefix unary operator}}
   ()
-case (_, var e, 3) +++ (1, 2, 3): // expected-error{{invalid pattern}}
+case (_, var e, 3) +++ (1, 2, 3):
+// expected-error@-1{{binary operator '+++' cannot be applied to operands of type '(_, <<error type>>, Int)' and '(Int, Int, Int)'}}
+// expected-note@-2{{expected an argument list of type '((Int, Int, Int), (Int, Int, Int))'}}
+// expected-error@-3{{'var' binding pattern cannot appear in an expression}}
+// expected-error@-4{{'var' binding pattern cannot appear in an expression}}
   ()
 }
 

test/Sema/enum_equatable_hashable.swift

@@ -96,7 +96,8 @@ private enum Bar<T> {
 
   mutating func value() -> T {
     switch self {
-    case E(let x): // expected-error{{invalid pattern}}
+    // FIXME: Should diagnose here that '.' needs to be inserted, but E has an ErrorType at this point
+    case E(let x):
       return x.value
     }
   }