Merge pull request swiftlang#30592 from xedin/fallback-diag-for-holes

[ConstraintSystem] Avoid applying invalid solution with fixes

Author: xedin

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))
 }