[ConstraintSystem] Avoid applying invalid solution with fixes

lib/Sema/CSApply.cpp

@@ -8025,6 +8025,17 @@ Optional<SolutionApplicationTarget> ConstraintSystem::applySolution(
     }
   }
 
+  // If there are no fixes recorded but score indicates that there
+  // should have been at least one, let's fail application and
+  // produce a fallback diagnostic to highlight the problem.
+  {
+    const auto &score = solution.getFixedScore();
+    if (score.Data[SK_Fix] > 0 || score.Data[SK_Hole] > 0) {
+      maybeProduceFallbackDiagnostic(target);
+      return None;
+    }
+  }
+
   ExprRewriter rewriter(*this, solution, shouldSuppressDiagnostics());
   ExprWalker walker(rewriter);
   auto resultTarget = walker.rewriteTarget(target);

lib/Sema/CSBindings.cpp

@@ -1086,6 +1086,10 @@ bool TypeVariableBinding::attempt(ConstraintSystem &cs) const {
     cs.DefaultedConstraints.push_back(srcLocator);
 
     if (type->isHole()) {
+      // Reflect in the score that this type variable couldn't be
+      // resolved and had to be bound to a placeholder "hole" type.
+      cs.increaseScore(SK_Hole);
+
       if (auto *GP = TypeVar->getImpl().getGenericParameter()) {
         auto path = dstLocator->getPath();
         // Drop `generic parameter` locator element so that all missing

lib/Sema/CSGen.cpp

@@ -931,6 +931,9 @@ namespace {
       = { nullptr, nullptr };
     unsigned currentEditorPlaceholderVariable = 0;
 
+    /// Keep track of acceptable DiscardAssignmentExpr's.
+    llvm::SmallPtrSet<DiscardAssignmentExpr*, 2> CorrectDiscardAssignmentExprs;
+
     /// Returns false and emits the specified diagnostic if the member reference
     /// base is a nil literal. Returns true otherwise.
     bool isValidBaseOfMemberRef(Expr *base, Diag<> diagnostic) {
@@ -3057,9 +3060,15 @@ namespace {
     }
 
     Type visitDiscardAssignmentExpr(DiscardAssignmentExpr *expr) {
+      /// Diagnose a '_' that isn't on the immediate LHS of an assignment.
+      if (!CorrectDiscardAssignmentExprs.count(expr)) {
+        auto &DE = CS.getASTContext().Diags;
+        DE.diagnose(expr->getLoc(), diag::discard_expr_outside_of_assignment);
+        return Type();
+      }
+
       auto locator = CS.getConstraintLocator(expr);
-      auto typeVar = CS.createTypeVariable(locator, TVO_CanBindToNoEscape |
-                                                    TVO_CanBindToHole);
+      auto typeVar = CS.createTypeVariable(locator, TVO_CanBindToNoEscape);
       return LValueType::get(typeVar);
     }
 
@@ -3095,6 +3104,25 @@ namespace {
       }
     }
 
+    /// Scout out the specified destination of an AssignExpr to recursively
+    /// identify DiscardAssignmentExpr in legal places.  We can only allow them
+    /// in simple pattern-like expressions, so we reject anything complex here.
+    void markAcceptableDiscardExprs(Expr *E) {
+      if (!E) return;
+
+      if (auto *PE = dyn_cast<ParenExpr>(E))
+        return markAcceptableDiscardExprs(PE->getSubExpr());
+      if (auto *TE = dyn_cast<TupleExpr>(E)) {
+        for (auto &elt : TE->getElements())
+          markAcceptableDiscardExprs(elt);
+        return;
+      }
+      if (auto *DAE = dyn_cast<DiscardAssignmentExpr>(E))
+        CorrectDiscardAssignmentExprs.insert(DAE);
+
+      // Otherwise, we can't support this.
+    }
+
     Type visitAssignExpr(AssignExpr *expr) {
       // Handle invalid code.
       if (!expr->getDest() || !expr->getSrc())
@@ -3924,6 +3952,9 @@ namespace {
           return { false, expr };
       }
 
+      if (auto *assignment = dyn_cast<AssignExpr>(expr))
+        CG.markAcceptableDiscardExprs(assignment->getDest());
+
       return { true, expr };
     }
 

lib/Sema/CSRanking.cpp

@@ -41,6 +41,10 @@ void ConstraintSystem::increaseScore(ScoreKind kind, unsigned value) {
       log.indent(solverState->depth * 2);
     log << "(increasing score due to ";
     switch (kind) {
+    case SK_Hole:
+      log << "hole in the constraint system";
+      break;
+
     case SK_Unavailable:
       log << "use of an unavailable declaration";
       break;

lib/Sema/CSSimplify.cpp

@@ -5073,7 +5073,6 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyConformsToConstraint(
     // func foo<T: BinaryInteger>(_: T) {}
     // foo(Foo.bar) <- if `Foo` doesn't have `bar` there is
     //                 no reason to complain about missing conformance.
-    increaseScore(SK_Fix);
     return SolutionKind::Solved;
   }
 
@@ -6466,7 +6465,6 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyMemberConstraint(
                baseObjTy->isHole()) {
       // If base type is a "hole" there is no reason to record any
       // more "member not found" fixes for chained member references.
-      increaseScore(SK_Fix);
       markMemberTypeAsPotentialHole(memberTy);
       return SolutionKind::Solved;
     }

lib/Sema/CSSolver.cpp

@@ -1139,23 +1139,6 @@ static bool debugConstraintSolverForTarget(
   return startBound != endBound;
 }
 
-/// If we aren't certain that we've emitted a diagnostic, emit a fallback
-/// diagnostic.
-static void maybeProduceFallbackDiagnostic(
-    ConstraintSystem &cs, SolutionApplicationTarget target) {
-  if (cs.Options.contains(ConstraintSystemFlags::SuppressDiagnostics))
-    return;
-
-  // Before producing fatal error here, let's check if there are any "error"
-  // diagnostics already emitted or waiting to be emitted. Because they are
-  // a better indication of the problem.
-  ASTContext &ctx = cs.getASTContext();
-  if (ctx.Diags.hadAnyError() || ctx.hasDelayedConformanceErrors())
-    return;
-
-  ctx.Diags.diagnose(target.getLoc(), diag::failed_to_produce_diagnostic);
-}
-
 Optional<std::vector<Solution>> ConstraintSystem::solve(
     SolutionApplicationTarget &target,
     ExprTypeCheckListener *listener,
@@ -1201,7 +1184,7 @@ Optional<std::vector<Solution>> ConstraintSystem::solve(
     }
 
     case SolutionResult::Error:
-      maybeProduceFallbackDiagnostic(*this, target);
+      maybeProduceFallbackDiagnostic(target);
       return None;
 
     case SolutionResult::TooComplex:

lib/Sema/ConstraintSystem.cpp

@@ -4387,3 +4387,20 @@ bool ConstraintSystem::isDeclUnavailable(const Decl *D,
   AvailabilityContext result = AvailabilityContext::alwaysAvailable();
   return !TypeChecker::isDeclAvailable(D, loc, DC, result);
 }
+
+/// If we aren't certain that we've emitted a diagnostic, emit a fallback
+/// diagnostic.
+void ConstraintSystem::maybeProduceFallbackDiagnostic(
+    SolutionApplicationTarget target) const {
+  if (Options.contains(ConstraintSystemFlags::SuppressDiagnostics))
+    return;
+
+  // Before producing fatal error here, let's check if there are any "error"
+  // diagnostics already emitted or waiting to be emitted. Because they are
+  // a better indication of the problem.
+  ASTContext &ctx = getASTContext();
+  if (ctx.Diags.hadAnyError() || ctx.hasDelayedConformanceErrors())
+    return;
+
+  ctx.Diags.diagnose(target.getLoc(), diag::failed_to_produce_diagnostic);
+}

lib/Sema/ConstraintSystem.h

@@ -634,6 +634,8 @@ enum ScoreKind {
 
   /// A fix needs to be applied to the source.
   SK_Fix,
+  /// A hole in the constraint system.
+  SK_Hole,
   /// A reference to an @unavailable declaration.
   SK_Unavailable,
   /// A use of a disfavored overload.
@@ -4643,6 +4645,10 @@ class ConstraintSystem {
                             SmallVectorImpl<unsigned> &Ordering,
                             SmallVectorImpl<unsigned> &PartitionBeginning);
 
+  /// If we aren't certain that we've emitted a diagnostic, emit a fallback
+  /// diagnostic.
+  void maybeProduceFallbackDiagnostic(SolutionApplicationTarget target) const;
+
   SWIFT_DEBUG_DUMP;
   SWIFT_DEBUG_DUMPER(dump(Expr *));
 

lib/Sema/MiscDiagnostics.cpp

@@ -68,9 +68,6 @@ static void diagSyntacticUseRestrictions(const Expr *E, const DeclContext *DC,
     SmallPtrSet<Expr*, 4> AlreadyDiagnosedMetatypes;
     SmallPtrSet<DeclRefExpr*, 4> AlreadyDiagnosedBitCasts;
 
-    /// Keep track of acceptable DiscardAssignmentExpr's.
-    SmallPtrSet<DiscardAssignmentExpr*, 2> CorrectDiscardAssignmentExprs;
-
     /// Keep track of the arguments to CallExprs.
     SmallPtrSet<Expr *, 2> CallArgs;
 
@@ -229,7 +226,6 @@ static void diagSyntacticUseRestrictions(const Expr *E, const DeclContext *DC,
       // If we have an assignment expression, scout ahead for acceptable _'s.
       if (auto *AE = dyn_cast<AssignExpr>(E)) {
         auto destExpr = AE->getDest();
-        markAcceptableDiscardExprs(destExpr);
         // If the user is assigning the result of a function that returns
         // Void to _ then warn, because that is redundant.
         if (auto DAE = dyn_cast<DiscardAssignmentExpr>(destExpr)) {
@@ -246,14 +242,6 @@ static void diagSyntacticUseRestrictions(const Expr *E, const DeclContext *DC,
         }
       }
 
-      /// Diagnose a '_' that isn't on the immediate LHS of an assignment.
-      if (auto *DAE = dyn_cast<DiscardAssignmentExpr>(E)) {
-        if (!CorrectDiscardAssignmentExprs.count(DAE) &&
-            !DAE->getType()->hasError())
-          Ctx.Diags.diagnose(DAE->getLoc(),
-                             diag::discard_expr_outside_of_assignment);
-      }
-
       // Diagnose 'self.init' or 'super.init' nested in another expression
       // or closure.
       if (auto *rebindSelfExpr = dyn_cast<RebindSelfInConstructorExpr>(E)) {
@@ -425,26 +413,6 @@ static void diagSyntacticUseRestrictions(const Expr *E, const DeclContext *DC,
       }
     }
 
-
-    /// Scout out the specified destination of an AssignExpr to recursively
-    /// identify DiscardAssignmentExpr in legal places.  We can only allow them
-    /// in simple pattern-like expressions, so we reject anything complex here.
-    void markAcceptableDiscardExprs(Expr *E) {
-      if (!E) return;
-
-      if (auto *PE = dyn_cast<ParenExpr>(E))
-        return markAcceptableDiscardExprs(PE->getSubExpr());
-      if (auto *TE = dyn_cast<TupleExpr>(E)) {
-        for (auto &elt : TE->getElements())
-          markAcceptableDiscardExprs(elt);
-        return;
-      }
-      if (auto *DAE = dyn_cast<DiscardAssignmentExpr>(E))
-        CorrectDiscardAssignmentExprs.insert(DAE);
-
-      // Otherwise, we can't support this.
-    }
-
     void checkMagicIdentifierMismatch(ConcreteDeclRef callee,
                                       unsigned uncurryLevel,
                                       unsigned argIndex,

test/Constraints/one_way_solve.swift

@@ -38,9 +38,9 @@ func testTernaryOneWayOverload(b: Bool) {
 
   // CHECK: solving component #1
   // CHECK: Initial bindings: $T11 := Int8
-  // CHECK: found solution 0 0 0 0 0 0 2 0 0 0 0 0
+  // CHECK: found solution 0 0 0 0 0 0 0 2 0 0 0 0 0
 
-  // CHECK: composed solution 0 0 0 0 0 0 2 0 0 0 0 0
-  // CHECK-NOT: composed solution 0 0 0 0 0 0 2 0 0 0 0 0
+  // CHECK: composed solution 0 0 0 0 0 0 0 2 0 0 0 0 0
+  // CHECK-NOT: composed solution 0 0 0 0 0 0 0 2 0 0 0 0 0
   let _: Int8 = b ? Builtin.one_way(int8Or16(17)) : Builtin.one_way(int8Or16(42))
 }

test/Constraints/rdar44770297.swift

@@ -4,11 +4,11 @@ protocol P {
   associatedtype A
 }
 
-func foo<T: P>(_: () throws -> T) -> T.A? {
+func foo<T: P>(_: () throws -> T) -> T.A? { // expected-note {{where 'T' = 'Never'}}
   fatalError()
 }
 
-// TODO(diagnostics): This expression is truly ambiguous because there is no conformance between `Never` and `P`
-// which means no associated type `A` and `nil` can't be an argument to any overload of `&` so we end
-// up generating at least 3 fixes per overload of `&`. But we could at least point to where the problems are.
-let _ = foo() {fatalError()} & nil // expected-error {{type of expression is ambiguous without more context}}
+let _ = foo() {fatalError()} & nil // expected-error {{global function 'foo' requires that 'Never' conform to 'P'}}
+// expected-error@-1 {{value of optional type 'Never.A?' must be unwrapped to a value of type 'Never.A'}}
+// expected-note@-2 {{force-unwrap}}
+// expected-note@-3 {{coalesce using '??'}}

test/Parse/enum.swift

@@ -488,7 +488,7 @@ enum SE0036 {
   func staticReferenceInSwitchInInstanceMethod() {
     switch self {
     case A: break // expected-error {{enum case 'A' cannot be used as an instance member}} {{10-10=.}}
-    case B(_): break // expected-error {{enum case 'B' cannot be used as an instance member}} {{10-10=.}}
+    case B(_): break // expected-error {{'_' can only appear in a pattern or on the left side of an assignment}}
     case C(let x): _ = x; break // expected-error {{enum case 'C' cannot be used as an instance member}} {{10-10=.}}
     }
   }
@@ -532,7 +532,7 @@ enum SE0036_Generic<T> {
 
   func foo() {
     switch self {
-    case A(_): break // expected-error {{enum case 'A' cannot be used as an instance member}} {{10-10=.}} expected-error {{missing argument label 'x:' in call}}
+    case A(_): break // expected-error {{'_' can only appear in a pattern or on the left side of an assignment}}
     }
 
     switch self {

test/Parse/matching_patterns.swift

@@ -28,20 +28,17 @@ case square(9):
 // 'var' and 'let' patterns.
 case var a:
   a = 1
-case let a: // expected-warning {{case is already handled by previous patterns; consider removing it}}
+case let a:
   a = 1         // expected-error {{cannot assign}}
 case var var a: // expected-error {{'var' cannot appear nested inside another 'var' or 'let' pattern}}
-                // expected-warning@-1 {{case is already handled by previous patterns; consider removing it}}
   a += 1
 case var let a: // expected-error {{'let' cannot appear nested inside another 'var' or 'let' pattern}}
-                // expected-warning@-1 {{case is already handled by previous patterns; consider removing it}}
   print(a, terminator: "")
 case var (var b): // expected-error {{'var' cannot appear nested inside another 'var'}}
-                  // expected-warning@-1 {{case is already handled by previous patterns; consider removing it}}
   b += 1
 
 // 'Any' pattern.
-case _: // expected-warning {{case is already handled by previous patterns; consider removing it}}
+case _:
   ()
 
 // patterns are resolved in expression-only positions are errors.
@@ -283,14 +280,12 @@ case (1, 2, 3):
 // patterns in expression-only positions are errors.
 case +++(_, var d, 3):
 // expected-error@-1{{'_' can only appear in a pattern or on the left side of an assignment}}
-// expected-error@-2{{'var' binding pattern cannot appear in an expression}}
   ()
 case (_, var e, 3) +++ (1, 2, 3):
-// expected-error@-1{{'_' can only appear in a pattern}}
-// expected-error@-2{{'var' binding pattern cannot appear in an expression}}
+// expected-error@-1{{'_' can only appear in a pattern or on the left side of an assignment}}
   ()
 case (let (_, _, _)) + 1:
-// expected-error@-1 {{expression pattern of type 'Int' cannot match values of type '(Int, Int, Int)'}}
+// expected-error@-1 {{'_' can only appear in a pattern or on the left side of an assignment}}
   ()
 }
 

test/decl/enum/enumtest.swift

@@ -36,8 +36,7 @@ func test1a() -> unionSearchFlags {
 
 func test1b(_ b : Bool) {
   _ = 123
-  _ = .description == 1 // expected-error {{instance member 'description' cannot be used on type 'Int'}}
-  // expected-error@-1 {{member 'description' in 'Int' produces result of type 'String', but context expects 'Int'}}
+  _ = .description == 1 // expected-error {{cannot infer contextual base in reference to member 'description'}}
 }
 
 enum MaybeInt {

test/expr/expressions.swift

@@ -709,7 +709,6 @@ func test() {
 func unusedExpressionResults() {
   // Unused l-value
   _ // expected-error{{'_' can only appear in a pattern or on the left side of an assignment}}
-  // expected-error@-1 {{expression resolves to an unused variable}}
 
   // <rdar://problem/20749592> Conditional Optional binding hides compiler error
   let optionalc:C? = nil