[ConstraintSystem] Extend invalid function body fix to cover constraint generation failures

Ignore function builder body if it emits at least one diagnostic
during constraint generation.

Resolves: rdar://problem/65983237

Author: xedin

include/swift/AST/DiagnosticEngine.h

@@ -1025,10 +1025,19 @@ namespace swift {
       }
     }
 
-    bool hasDiagnostics() const {
-      return std::distance(Engine.TentativeDiagnostics.begin() +
-                               PrevDiagnostics,
-                           Engine.TentativeDiagnostics.end()) > 0;
+    bool hasErrors() const {
+      ArrayRef<Diagnostic> diagnostics(Engine.TentativeDiagnostics.begin() +
+                                           PrevDiagnostics,
+                                       Engine.TentativeDiagnostics.end());
+
+      for (auto &diagnostic : diagnostics) {
+        auto behavior = Engine.state.determineBehavior(diagnostic.getID());
+        if (behavior == DiagnosticState::Behavior::Fatal ||
+            behavior == DiagnosticState::Behavior::Error)
+          return true;
+      }
+
+      return false;
     }
 
     /// Abort and close this transaction and erase all diagnostics

lib/Sema/BuilderTransform.cpp

@@ -1523,6 +1523,14 @@ ConstraintSystem::matchFunctionBuilder(
   assert(builder && "Bad function builder type");
   assert(builder->getAttrs().hasAttribute<FunctionBuilderAttr>());
 
+  if (InvalidFunctionBuilderBodies.count(fn)) {
+    auto *closure = cast<ClosureExpr>(fn.getAbstractClosureExpr());
+    (void)recordFix(
+        IgnoreInvalidFunctionBuilderBody::duringConstraintGeneration(
+            *this, getConstraintLocator(closure)));
+    return getTypeMatchSuccess();
+  }
+
   // Pre-check the body: pre-check any expressions in it and look
   // for return statements.
   auto request = PreCheckFunctionBuilderRequest{fn, fn.getBody(),
@@ -1539,7 +1547,7 @@ ConstraintSystem::matchFunctionBuilder(
 
     if (auto *closure =
             dyn_cast_or_null<ClosureExpr>(fn.getAbstractClosureExpr())) {
-      auto failed = recordFix(IgnoreInvalidFunctionBuilderBody::create(
+      auto failed = recordFix(IgnoreInvalidFunctionBuilderBody::duringPreCheck(
           *this, getConstraintLocator(closure)));
       return failed ? getTypeMatchFailure(locator) : getTypeMatchSuccess();
     }
@@ -1598,9 +1606,27 @@ ConstraintSystem::matchFunctionBuilder(
   BuilderClosureVisitor visitor(getASTContext(), this, dc, builderType,
                                 bodyResultType);
 
-  auto applied = visitor.apply(fn.getBody());
-  if (!applied)
-    return getTypeMatchFailure(locator);
+  Optional<AppliedBuilderTransform> applied = None;
+  {
+    DiagnosticTransaction transaction(dc->getASTContext().Diags);
+
+    applied = visitor.apply(fn.getBody());
+    if (!applied)
+      return getTypeMatchFailure(locator);
+
+    if (transaction.hasErrors()) {
+      if (auto *closure =
+              dyn_cast_or_null<ClosureExpr>(fn.getAbstractClosureExpr())) {
+        InvalidFunctionBuilderBodies.insert(fn);
+        auto failed = recordFix(
+            IgnoreInvalidFunctionBuilderBody::duringConstraintGeneration(
+                *this, getConstraintLocator(closure)));
+        return failed ? getTypeMatchFailure(locator) : getTypeMatchSuccess();
+      }
+
+      return getTypeMatchFailure(locator);
+    }
+  }
 
   Type transformedType = getType(applied->returnExpr);
   assert(transformedType && "Missing type");
@@ -1687,7 +1713,7 @@ class PreCheckFunctionBuilderApplication : public ASTWalker {
 
       HasError |= ConstraintSystem::preCheckExpression(
           E, DC, /*replaceInvalidRefsWithErrors=*/false);
-      HasError |= transaction.hasDiagnostics();
+      HasError |= transaction.hasErrors();
 
       if (SuppressDiagnostics)
         transaction.abort();

lib/Sema/CSFix.cpp

@@ -1366,6 +1366,14 @@ bool SpecifyKeyPathRootType::diagnose(const Solution &solution,
 
 bool IgnoreInvalidFunctionBuilderBody::diagnose(const Solution &solution,
                                                 bool asNote) const {
+  switch (Phase) {
+  // Handled below
+  case ErrorInPhase::PreCheck:
+    break;
+  case ErrorInPhase::ConstraintGeneration:
+    return true; // Already diagnosed by `matchFunctionBuilder`.
+  }
+
   auto *S = cast<ClosureExpr>(getAnchor())->getBody();
 
   class PreCheckWalker : public ASTWalker {
@@ -1392,7 +1400,7 @@ bool IgnoreInvalidFunctionBuilderBody::diagnose(const Solution &solution,
     }
 
     bool diagnosed() const {
-      return Transaction.hasDiagnostics();
+      return Transaction.hasErrors();
     }
   };
 
@@ -1403,8 +1411,8 @@ bool IgnoreInvalidFunctionBuilderBody::diagnose(const Solution &solution,
   return walker.diagnosed();
 }
 
-IgnoreInvalidFunctionBuilderBody *
-IgnoreInvalidFunctionBuilderBody::create(ConstraintSystem &cs,
-                                         ConstraintLocator *locator) {
-  return new (cs.getAllocator()) IgnoreInvalidFunctionBuilderBody(cs, locator);
+IgnoreInvalidFunctionBuilderBody *IgnoreInvalidFunctionBuilderBody::create(
+    ConstraintSystem &cs, ErrorInPhase phase, ConstraintLocator *locator) {
+  return new (cs.getAllocator())
+      IgnoreInvalidFunctionBuilderBody(cs, phase, locator);
 }

lib/Sema/CSFix.h

@@ -1844,9 +1844,17 @@ class SpecifyKeyPathRootType final : public ConstraintFix {
 };
 
 class IgnoreInvalidFunctionBuilderBody final : public ConstraintFix {
-  IgnoreInvalidFunctionBuilderBody(ConstraintSystem &cs,
+  enum class ErrorInPhase {
+    PreCheck,
+    ConstraintGeneration,
+  };
+
+  ErrorInPhase Phase;
+
+  IgnoreInvalidFunctionBuilderBody(ConstraintSystem &cs, ErrorInPhase phase,
                                    ConstraintLocator *locator)
-      : ConstraintFix(cs, FixKind::IgnoreInvalidFunctionBuilderBody, locator) {}
+      : ConstraintFix(cs, FixKind::IgnoreInvalidFunctionBuilderBody, locator),
+        Phase(phase) {}
 
 public:
   std::string getName() const override {
@@ -1859,8 +1867,19 @@ class IgnoreInvalidFunctionBuilderBody final : public ConstraintFix {
     return diagnose(*commonFixes.front().first);
   }
 
-  static IgnoreInvalidFunctionBuilderBody *create(ConstraintSystem &cs,
-                                                  ConstraintLocator *locator);
+  static IgnoreInvalidFunctionBuilderBody *
+  duringPreCheck(ConstraintSystem &cs, ConstraintLocator *locator) {
+    return create(cs, ErrorInPhase::PreCheck, locator);
+  }
+
+  static IgnoreInvalidFunctionBuilderBody *
+  duringConstraintGeneration(ConstraintSystem &cs, ConstraintLocator *locator) {
+    return create(cs, ErrorInPhase::ConstraintGeneration, locator);
+  }
+
+private:
+  static IgnoreInvalidFunctionBuilderBody *
+  create(ConstraintSystem &cs, ErrorInPhase phase, ConstraintLocator *locator);
 };
 
 } // end namespace constraints

lib/Sema/ConstraintSystem.h

@@ -1727,6 +1727,13 @@ class ConstraintSystem {
   /// from declared parameters/result and body.
   llvm::MapVector<const ClosureExpr *, FunctionType *> ClosureTypes;
 
+  /// This is a *global* list of all function builder bodies that have
+  /// been determined to be incorrect by failing constraint generation.
+  ///
+  /// Tracking this information is useful to avoid producing duplicate
+  /// diagnostics when function builder has multiple overloads.
+  llvm::SmallDenseSet<AnyFunctionRef> InvalidFunctionBuilderBodies;
+
   /// Maps node types used within all portions of the constraint
   /// system, instead of directly using the types on the
   /// nodes themselves. This allows us to typecheck and

test/Constraints/function_builder_diags.swift

@@ -606,6 +606,14 @@ struct MyView {
     } // expected-error {{expected identifier after '.' expression}}
   }
 
+  @TupleBuilder var invalidCaseWithoutDot: some P {
+    switch Optional.some(1) {
+    case none: 42 // expected-error {{cannot find 'none' in scope}}
+    case .some(let x):
+      0
+    }
+  }
+
   @TupleBuilder var invalidConversion: Int {
     "" // expected-error {{cannot convert value of type 'String' to specified type 'Int'}}
   }