Merge pull request #76297 from hamishknight/together-forever

[Sema] Remove separate closure type-checking logic

Author: hamishknight

include/swift/AST/AnyFunctionRef.h

@@ -130,7 +130,7 @@ class AnyFunctionRef {
     auto *ACE = TheFunction.get<AbstractClosureExpr *>();
     if (auto *CE = dyn_cast<ClosureExpr>(ACE)) {
       CE->setBody(stmt);
-      CE->setBodyState(ClosureExpr::BodyState::ReadyForTypeChecking);
+      CE->setBodyState(ClosureExpr::BodyState::Parsed);
       return;
     }
 
@@ -146,7 +146,7 @@ class AnyFunctionRef {
     auto *ACE = TheFunction.get<AbstractClosureExpr *>();
     if (auto *CE = dyn_cast<ClosureExpr>(ACE)) {
       CE->setBody(stmt);
-      CE->setBodyState(ClosureExpr::BodyState::TypeCheckedWithSignature);
+      CE->setBodyState(ClosureExpr::BodyState::TypeChecked);
       return;
     }
 

include/swift/AST/DiagnosticsSema.def

@@ -1605,6 +1605,11 @@ NOTE(candidate_with_extraneous_args,none,
      "candidate %0 has %1 parameter%s1, but context %2 has %3",
      (Type, unsigned, Type, unsigned))
 
+ERROR(result_builder_missing_element,none,
+      "expected expression in result builder %0", (DeclName))
+ERROR(result_builder_missing_element_of_type,none,
+      "expected expression of type %0 in result builder %1", (Type, DeclName))
+
 ERROR(no_accessible_initializers,none,
       "%0 cannot be constructed because it has no accessible initializers",
       (Type))

include/swift/AST/Expr.h

@@ -4119,20 +4119,11 @@ class ClosureExpr : public AbstractClosureExpr {
 
 public:
   enum class BodyState {
-    /// The body was parsed, but not ready for type checking because
-    /// the closure parameters haven't been type checked.
+    /// The body was parsed.
     Parsed,
 
-    /// The type of the closure itself was type checked. But the body has not
-    /// been type checked yet.
-    ReadyForTypeChecking,
-
-    /// The body was typechecked with the enclosing closure.
-    /// i.e. single expression closure or result builder closure.
-    TypeCheckedWithSignature,
-
-    /// The body was type checked separately from the enclosing closure.
-    SeparatelyTypeChecked,
+    /// The body was type-checked.
+    TypeChecked,
   };
 
 private:
@@ -4370,13 +4361,6 @@ class ClosureExpr : public AbstractClosureExpr {
     ExplicitResultTypeAndBodyState.setInt(v);
   }
 
-  /// Whether this closure's body is/was type checked separately from its
-  /// enclosing expression.
-  bool isSeparatelyTypeChecked() const {
-    return getBodyState() == BodyState::SeparatelyTypeChecked ||
-           getBodyState() == BodyState::ReadyForTypeChecking;
-  }
-
   static bool classof(const Expr *E) {
     return E->getKind() == ExprKind::Closure;
   }

include/swift/Sema/ConstraintSystem.h

@@ -5687,11 +5687,6 @@ class ConstraintSystem {
   /// imported from C/ObjectiveC.
   bool isArgumentOfImportedDecl(ConstraintLocatorBuilder locator);
 
-  /// Check whether given closure should participate in inference e.g.
-  /// if it's a single-expression closure - it always does, but
-  /// multi-statement closures require special flags.
-  bool participatesInInference(ClosureExpr *closure) const;
-
   /// Visit each subexpression that will be part of the constraint system
   /// of the given expression, including those in closure bodies that will be
   /// part of the constraint system.

lib/Sema/CSApply.cpp

@@ -8853,28 +8853,21 @@ namespace {
         // Note that in this mode `ClosuresToTypeCheck` acts
         // as a stack because multi-statement closures could
         // have other multi-statement closures in the body.
-        if (cs.participatesInInference(closure)) {
-          hadError |= cs.applySolutionToBody(
-              solution, closure, Rewriter.dc,
-              [&](SyntacticElementTarget target) {
-                auto resultTarget = rewriteTarget(target);
-                if (resultTarget) {
-                  if (auto expr = resultTarget->getAsExpr())
-                    solution.setExprTypes(expr);
-                }
-
-                return resultTarget;
-              });
+        hadError |= cs.applySolutionToBody(
+            solution, closure, Rewriter.dc, [&](SyntacticElementTarget target) {
+              auto resultTarget = rewriteTarget(target);
+              if (resultTarget) {
+                if (auto expr = resultTarget->getAsExpr())
+                  solution.setExprTypes(expr);
+              }
 
-          if (!hadError) {
-            TypeChecker::checkClosureAttributes(closure);
-            TypeChecker::checkParameterList(closure->getParameters(), closure);
-          }
+              return resultTarget;
+            });
 
-          continue;
+        if (!hadError) {
+          TypeChecker::checkClosureAttributes(closure);
+          TypeChecker::checkParameterList(closure->getParameters(), closure);
         }
-
-        hadError |= TypeChecker::typeCheckClosureBody(closure);
       }
 
       return hadError;

lib/Sema/CSDiagnostics.cpp

@@ -177,6 +177,21 @@ FailureDiagnostic::getArgumentListFor(ConstraintLocator *locator) const {
   return S.getArgumentList(locator);
 }
 
+StringRef FailureDiagnostic::getEditorPlaceholder(
+    StringRef description, Type ty,
+    llvm::SmallVectorImpl<char> &scratch) const {
+  llvm::raw_svector_ostream OS(scratch);
+  OS << "<#";
+  if (!ty || ty->is<UnresolvedType>()) {
+    OS << description;
+  } else {
+    OS << "T##";
+    ty.print(OS);
+  }
+  OS << "#>";
+  return StringRef(scratch.data(), scratch.size());
+}
+
 Expr *FailureDiagnostic::getBaseExprFor(const Expr *anchor) const {
   if (!anchor)
     return nullptr;
@@ -5177,6 +5192,9 @@ bool MissingArgumentsFailure::diagnoseAsError() {
     return true;
   }
 
+  if (diagnoseMissingResultBuilderElement())
+    return true;
+
   if (diagnoseInvalidTupleDestructuring())
     return true;
 
@@ -5514,6 +5532,55 @@ bool MissingArgumentsFailure::diagnoseClosure(const ClosureExpr *closure) {
   return true;
 }
 
+bool MissingArgumentsFailure::diagnoseMissingResultBuilderElement() const {
+  auto &ctx = getASTContext();
+
+  // Only handle a single missing argument in an empty builder for now. This
+  // should be the most common case though since most builders support N >= 1
+  // elements.
+  if (SynthesizedArgs.size() != 1)
+    return false;
+
+  auto *call = getAsExpr<CallExpr>(getRawAnchor());
+  if (!call || !call->isImplicit() || !call->getArgs()->empty())
+    return false;
+
+  auto *UDE = dyn_cast<UnresolvedDotExpr>(call->getFn());
+  if (!UDE || !isResultBuilderMethodReference(ctx, UDE))
+    return false;
+
+  auto overload = getCalleeOverloadChoiceIfAvailable(getLocator());
+  if (!overload)
+    return false;
+
+  auto *decl = overload->choice.getDeclOrNull();
+  if (!decl || decl->getBaseName() != ctx.Id_buildBlock)
+    return false;
+
+  auto resultBuilder =
+      getType(UDE->getBase())->getMetatypeInstanceType()->getAnyNominal();
+  if (!resultBuilder)
+    return false;
+
+  auto paramType = resolveType(SynthesizedArgs.front().param.getPlainType());
+
+  SmallString<64> scratch;
+  auto fixIt = getEditorPlaceholder("result", paramType, scratch);
+  auto fixItLoc = call->getStartLoc();
+
+  if (paramType->is<UnresolvedType>()) {
+    emitDiagnostic(diag::result_builder_missing_element,
+                   resultBuilder->getName())
+        .fixItInsertAfter(fixItLoc, fixIt);
+  } else {
+    emitDiagnostic(diag::result_builder_missing_element_of_type, paramType,
+                   resultBuilder->getName())
+        .fixItInsertAfter(fixItLoc, fixIt);
+  }
+  emitDiagnosticAt(decl, diag::decl_declared_here, decl);
+  return true;
+}
+
 bool MissingArgumentsFailure::diagnoseInvalidTupleDestructuring() const {
   auto *locator = getLocator();
   if (!locator->isLastElement<LocatorPathElt::ApplyArgument>())

lib/Sema/CSDiagnostics.h

@@ -203,6 +203,11 @@ class FailureDiagnostic {
           [](GenericTypeParamType *, Type) {});
 
   bool conformsToKnownProtocol(Type type, KnownProtocolKind protocol) const;
+
+  /// Retrieve an editor placeholder with a given description, or a given
+  /// type if specified.
+  StringRef getEditorPlaceholder(StringRef description, Type ty,
+                                 llvm::SmallVectorImpl<char> &scratch) const;
 };
 
 /// Base class for all of the diagnostics related to generic requirement
@@ -1505,6 +1510,9 @@ class MissingArgumentsFailure final : public FailureDiagnostic {
   /// let's produce tailored diagnostics.
   bool diagnoseClosure(const ClosureExpr *closure);
 
+  /// Diagnose a single missing argument to a buildBlock call.
+  bool diagnoseMissingResultBuilderElement() const;
+
   /// Diagnose cases when instead of multiple distinct arguments
   /// call got a single tuple argument with expected arity/types.
   bool diagnoseInvalidTupleDestructuring() const;

lib/Sema/CSGen.cpp

@@ -2555,10 +2555,7 @@ namespace {
         // If this is a multi-statement closure, let's mark result
         // as potential hole right away.
         return Type(CS.createTypeVariable(
-            resultLocator,
-            (!CS.participatesInInference(closure) || allowResultBindToHole)
-                ? TVO_CanBindToHole
-                : 0));
+            resultLocator, allowResultBindToHole ? TVO_CanBindToHole : 0));
       }();
 
       // Determine the isolation of the closure.

lib/Sema/CSSyntacticElement.cpp

@@ -1171,18 +1171,6 @@ class SyntacticElementConstraintGenerator
         for (const auto &capture : captureList->getCaptureList())
           visitPatternBinding(capture.PBD, elements);
       }
-
-      // Let's not walk into the body if empty or multi-statement closure
-      // doesn't participate in inference.
-      if (!cs.participatesInInference(closure)) {
-        // Although the body doesn't participate in inference we still
-        // want to type-check captures to make sure that the context
-        // is valid.
-        if (captureList)
-          createConjunction(elements, locator);
-
-        return;
-      }
     }
 
     if (isChildOf(StmtKind::Case)) {
@@ -2598,7 +2586,6 @@ SolutionApplicationToFunctionResult ConstraintSystem::applySolution(
 
   // Otherwise, we need to delay type checking of the closure until later.
   solution.setExprTypes(closure);
-  closure->setBodyState(ClosureExpr::BodyState::ReadyForTypeChecking);
   return SolutionApplicationToFunctionResult::Delay;
 }
 

lib/Sema/ConstraintSystem.cpp

@@ -7432,16 +7432,6 @@ bool ConstraintSystem::isArgumentGenericFunction(Type argType, Expr *argExpr) {
   return false;
 }
 
-bool ConstraintSystem::participatesInInference(ClosureExpr *closure) const {
-  if (getAppliedResultBuilderTransform(closure))
-    return true;
-
-  if (closure->hasEmptyBody())
-    return false;
-
-  return true;
-}
-
 ProtocolConformanceRef
 ConstraintSystem::lookupConformance(Type type, ProtocolDecl *protocol) {
   auto cacheKey = std::make_pair(type.getPointer(), protocol);

lib/Sema/TypeCheckAvailability.cpp

@@ -1822,11 +1822,8 @@ static void findAvailabilityFixItNodes(
   InnermostAncestorFinder::MatchPredicate IsGuardable =
       [](ASTNode Node, ASTWalker::ParentTy Parent) {
         if (Expr *ParentExpr = Parent.getAsExpr()) {
-          auto *ParentClosure = dyn_cast<ClosureExpr>(ParentExpr);
-          if (!ParentClosure ||
-              ParentClosure->isSeparatelyTypeChecked()) {
+          if (!isa<ClosureExpr>(ParentExpr))
             return false;
-          }
         } else if (auto *ParentStmt = Parent.getAsStmt()) {
           if (!isa<BraceStmt>(ParentStmt)) {
             return false;

lib/Sema/TypeCheckConstraints.cpp

@@ -2363,10 +2363,6 @@ void ConstraintSystem::forEachExpr(
       if (!NewE)
         return Action::Stop();
 
-      if (auto closure = dyn_cast<ClosureExpr>(E)) {
-        if (!CS.participatesInInference(closure))
-          return Action::SkipNode(NewE);
-      }
       return Action::Continue(NewE);
     }
 

lib/Sema/TypeCheckStmt.cpp

@@ -104,12 +104,7 @@ namespace {
 
       if (auto CE = dyn_cast<ClosureExpr>(E)) {
         CE->setParent(ParentDC);
-
-        // If the closure was type checked within its enclosing context,
-        // we need to walk into it. Otherwise, it'll have been separately
-        // type-checked.
-        if (!CE->isSeparatelyTypeChecked())
-          CE->getBody()->walk(ContextualizeClosuresAndMacros(CE));
+        CE->getBody()->walk(ContextualizeClosuresAndMacros(CE));
 
         TypeChecker::computeCaptures(CE);
         return Action::SkipNode(E);
@@ -283,18 +278,14 @@ namespace {
         if(CE->getRawDiscriminator() == ClosureExpr::InvalidDiscriminator)
           CE->setDiscriminator(NextClosureDiscriminator++);
 
-        // If the closure was type checked within its enclosing context,
-        // we need to walk into it with a new sequence.
-        // Otherwise, it'll have been separately type-checked.
-        if (!CE->isSeparatelyTypeChecked()) {
-          SetLocalDiscriminators innerVisitor;
-          if (auto params = CE->getParameters()) {
-            for (auto *param : *params) {
-              innerVisitor.setLocalDiscriminator(param);
-            }
+        // We need to walk into closure bodies with a new sequence.
+        SetLocalDiscriminators innerVisitor;
+        if (auto params = CE->getParameters()) {
+          for (auto *param : *params) {
+            innerVisitor.setLocalDiscriminator(param);
           }
-          CE->getBody()->walk(innerVisitor);
         }
+        CE->getBody()->walk(innerVisitor);
 
         return Action::SkipNode(E);
       }
@@ -2978,25 +2969,6 @@ TypeCheckFunctionBodyRequest::evaluate(Evaluator &eval,
   return hadError ? errorBody() : body;
 }
 
-bool TypeChecker::typeCheckClosureBody(ClosureExpr *closure) {
-  TypeChecker::checkClosureAttributes(closure);
-  TypeChecker::checkParameterList(closure->getParameters(), closure);
-
-  BraceStmt *body = closure->getBody();
-
-  std::optional<FunctionBodyTimer> timer;
-  const auto &tyOpts = closure->getASTContext().TypeCheckerOpts;
-  if (tyOpts.DebugTimeFunctionBodies || tyOpts.WarnLongFunctionBodies)
-    timer.emplace(closure);
-
-  bool HadError = StmtChecker(closure).typeCheckBody(body);
-  if (body) {
-    closure->setBody(body);
-  }
-  closure->setBodyState(ClosureExpr::BodyState::SeparatelyTypeChecked);
-  return HadError;
-}
-
 bool TypeChecker::typeCheckTapBody(TapExpr *expr, DeclContext *DC) {
   // We intentionally use typeCheckStmt instead of typeCheckBody here
   // because we want to contextualize TapExprs with the body they're in.

lib/Sema/TypeChecker.h

@@ -463,8 +463,6 @@ std::optional<BraceStmt *> applyResultBuilderBodyTransform(FuncDecl *func,
 /// Find the return statements within the body of the given function.
 std::vector<ReturnStmt *> findReturnStatements(AnyFunctionRef fn);
 
-bool typeCheckClosureBody(ClosureExpr *closure);
-
 bool typeCheckTapBody(TapExpr *expr, DeclContext *DC);
 
 Type typeCheckParameterDefault(Expr *&defaultValue, DeclContext *DC,