Merge pull request #40321 from apple/revert-39989-more-diag-improvements-for-multi-stmt-closures

Revert "[TypeChecker] SE-0326: Enable multi-statement closure inference by default"

Author: xedin

include/swift/Basic/LangOptions.h

@@ -688,7 +688,7 @@ namespace swift {
 
     /// Enable experimental support for type inference through multi-statement
     /// closures.
-    bool EnableMultiStatementClosureInference = true;
+    bool EnableMultiStatementClosureInference = false;
 
     /// See \ref FrontendOptions.PrintFullConvention
     bool PrintFullConvention = false;

include/swift/Sema/CSBindings.h

@@ -338,14 +338,6 @@ class BindingSet {
 
   TypeVariableType *getTypeVariable() const { return Info.TypeVar; }
 
-  /// Check whether this binding set belongs to a type variable
-  /// that represents a result type of a closure.
-  bool forClosureResult() const;
-
-  /// Check whether this binding set belongs to a type variable
-  /// that represents a generic parameter.
-  bool forGenericParameter() const;
-
   bool canBeNil() const;
 
   /// If this type variable doesn't have any viable bindings, or

include/swift/Sema/CSFix.h

@@ -2305,10 +2305,6 @@ class SpecifyClosureParameterType final : public ConstraintFix {
 
   bool diagnose(const Solution &solution, bool asNote = false) const override;
 
-  bool diagnoseForAmbiguity(CommonFixesArray commonFixes) const override {
-    return diagnose(*commonFixes.front().first);
-  }
-
   static SpecifyClosureParameterType *create(ConstraintSystem &cs,
                                              ConstraintLocator *locator);
 

include/swift/Sema/Constraint.h

@@ -442,8 +442,6 @@ class Constraint final : public llvm::ilist_node<Constraint>,
       ASTNode Element;
       /// Contextual information associated with the element (if any).
       ContextualTypeInfo Context;
-      /// Identifies whether result of this node is unused.
-      bool IsDiscarded;
     } ClosureElement;
   };
 
@@ -497,7 +495,7 @@ class Constraint final : public llvm::ilist_node<Constraint>,
              SmallPtrSetImpl<TypeVariableType *> &typeVars);
 
   /// Construct a closure body element constraint.
-  Constraint(ASTNode node, ContextualTypeInfo context, bool isDiscarded,
+  Constraint(ASTNode node, ContextualTypeInfo context,
              ConstraintLocator *locator,
              SmallPtrSetImpl<TypeVariableType *> &typeVars);
 
@@ -587,14 +585,12 @@ class Constraint final : public llvm::ilist_node<Constraint>,
 
   static Constraint *createClosureBodyElement(ConstraintSystem &cs,
                                               ASTNode node,
-                                              ConstraintLocator *locator,
-                                              bool isDiscarded = false);
+                                              ConstraintLocator *locator);
 
   static Constraint *createClosureBodyElement(ConstraintSystem &cs,
                                               ASTNode node,
                                               ContextualTypeInfo context,
-                                              ConstraintLocator *locator,
-                                              bool isDiscarded = false);
+                                              ConstraintLocator *locator);
 
   /// Determine the kind of constraint.
   ConstraintKind getKind() const { return Kind; }
@@ -861,11 +857,6 @@ class Constraint final : public llvm::ilist_node<Constraint>,
     return ClosureElement.Context;
   }
 
-  bool isDiscardedElement() const {
-    assert(Kind == ConstraintKind::ClosureBodyElement);
-    return ClosureElement.IsDiscarded;
-  }
-
   /// For an applicable function constraint, retrieve the trailing closure
   /// matching rule.
   Optional<TrailingClosureMatching> getTrailingClosureMatching() const;

include/swift/Sema/ConstraintSystem.h

@@ -4854,8 +4854,8 @@ class ConstraintSystem {
   /// Simplify a closure body element constraint by generating required
   /// constraints to represent the given element in constraint system.
   SolutionKind simplifyClosureBodyElementConstraint(
-      ASTNode element, ContextualTypeInfo context, bool isDiscarded,
-      TypeMatchOptions flags, ConstraintLocatorBuilder locator);
+      ASTNode element, ContextualTypeInfo context, TypeMatchOptions flags,
+      ConstraintLocatorBuilder locator);
 
 public: // FIXME: Public for use by static functions.
   /// Simplify a conversion constraint with a fix applied to it.
@@ -5014,8 +5014,7 @@ class ConstraintSystem {
   /// \param replaceInvalidRefsWithErrors Indicates whether it's allowed
   /// to replace any discovered invalid member references with `ErrorExpr`.
   static bool preCheckExpression(Expr *&expr, DeclContext *dc,
-                                 bool replaceInvalidRefsWithErrors,
-                                 bool leaveClosureBodiesUnchecked);
+                                 bool replaceInvalidRefsWithErrors);
 
   /// Solve the system of constraints generated from provided target.
   ///
@@ -5246,16 +5245,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.
-  void forEachExpr(Expr *expr, llvm::function_ref<Expr *(Expr *)> callback);
-
   SWIFT_DEBUG_DUMP;
   SWIFT_DEBUG_DUMPER(dump(Expr *));
 
@@ -6080,6 +6069,18 @@ BraceStmt *applyResultBuilderTransform(
           constraints::SolutionApplicationTarget)>
             rewriteTarget);
 
+/// Determine whether the given closure expression should be type-checked
+/// within the context of its enclosing expression. Otherwise, it will be
+/// separately type-checked once its enclosing expression has determined all
+/// of the parameter and result types without looking at the body.
+bool shouldTypeCheckInEnclosingExpression(ClosureExpr *expr);
+
+/// 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.
+void forEachExprInConstraintSystem(
+    Expr *expr, llvm::function_ref<Expr *(Expr *)> callback);
+
 /// Whether the given parameter requires an argument.
 bool parameterRequiresArgument(
     ArrayRef<AnyFunctionType::Param> params,

include/swift/Sema/IDETypeChecking.h

@@ -47,8 +47,7 @@ namespace swift {
   struct PrintOptions;
 
   /// Typecheck binding initializer at \p bindingIndex.
-  void typeCheckPatternBinding(PatternBindingDecl *PBD, unsigned bindingIndex,
-                               bool leaveClosureBodiesUnchecked);
+  void typeCheckPatternBinding(PatternBindingDecl *PBD, unsigned bindingIndex);
 
   /// Check if T1 is convertible to T2.
   ///

lib/IDE/ExprContextAnalysis.cpp

@@ -98,8 +98,7 @@ void swift::ide::typeCheckContextAt(DeclContext *DC, SourceLoc Loc) {
             [](VarDecl *VD) { (void)VD->getInterfaceType(); });
         if (PBD->getInit(i)) {
           if (!PBD->isInitializerChecked(i))
-            typeCheckPatternBinding(PBD, i,
-                                    /*LeaveClosureBodyUnchecked=*/true);
+            typeCheckPatternBinding(PBD, i);
         }
       }
     } else if (auto *defaultArg = dyn_cast<DefaultArgumentInitializer>(DC)) {

lib/Sema/BuilderTransform.cpp

@@ -1932,9 +1932,7 @@ class PreCheckResultBuilderApplication : public ASTWalker {
       DiagnosticTransaction transaction(diagEngine);
 
       HasError |= ConstraintSystem::preCheckExpression(
-          E, DC, /*replaceInvalidRefsWithErrors=*/true,
-          /*leaveClosureBodiesUnchecked=*/false);
-
+          E, DC, /*replaceInvalidRefsWithErrors=*/true);
       HasError |= transaction.hasErrors();
 
       if (!HasError)

lib/Sema/CSApply.cpp

@@ -4056,7 +4056,8 @@ namespace {
       if (expr->isLiteralInit()) {
         auto *literalInit = expr->getSubExpr();
         if (auto *call = dyn_cast<CallExpr>(literalInit)) {
-          cs.forEachExpr(call->getFn(), [&](Expr *subExpr) -> Expr * {
+          forEachExprInConstraintSystem(call->getFn(),
+                                        [&](Expr *subExpr) -> Expr * {
             auto *TE = dyn_cast<TypeExpr>(subExpr);
             if (!TE)
               return subExpr;

lib/Sema/CSBindings.cpp

@@ -25,14 +25,6 @@ using namespace swift;
 using namespace constraints;
 using namespace inference;
 
-bool BindingSet::forClosureResult() const {
-  return Info.TypeVar->getImpl().isClosureResultType();
-}
-
-bool BindingSet::forGenericParameter() const {
-  return bool(Info.TypeVar->getImpl().getGenericParameter());
-}
-
 bool BindingSet::canBeNil() const {
   auto &ctx = CS.getASTContext();
   return Literals.count(

lib/Sema/CSClosure.cpp

@@ -16,7 +16,6 @@
 //
 //===----------------------------------------------------------------------===//
 
-#include "MiscDiagnostics.h"
 #include "TypeChecker.h"
 #include "swift/Sema/ConstraintSystem.h"
 
@@ -163,8 +162,8 @@ static bool isViableElement(ASTNode element) {
   return true;
 }
 
-using ElementInfo = std::tuple<ASTNode, ContextualTypeInfo,
-                               /*isDiscarded=*/bool, ConstraintLocator *>;
+using ElementInfo =
+    std::tuple<ASTNode, ContextualTypeInfo, ConstraintLocator *>;
 
 static void createConjunction(ConstraintSystem &cs,
                               ArrayRef<ElementInfo> elements,
@@ -191,8 +190,7 @@ static void createConjunction(ConstraintSystem &cs,
   for (const auto &entry : elements) {
     ASTNode element = std::get<0>(entry);
     ContextualTypeInfo context = std::get<1>(entry);
-    bool isDiscarded = std::get<2>(entry);
-    ConstraintLocator *elementLoc = std::get<3>(entry);
+    ConstraintLocator *elementLoc = std::get<2>(entry);
 
     if (!isViableElement(element))
       continue;
@@ -203,8 +201,8 @@ static void createConjunction(ConstraintSystem &cs,
     if (isIsolated)
       element.walk(paramCollector);
 
-    constraints.push_back(Constraint::createClosureBodyElement(
-        cs, element, context, elementLoc, isDiscarded));
+    constraints.push_back(
+        Constraint::createClosureBodyElement(cs, element, context, elementLoc));
   }
 
   // It's possible that there are no viable elements in the body,
@@ -222,9 +220,8 @@ static void createConjunction(ConstraintSystem &cs,
 }
 
 ElementInfo makeElement(ASTNode node, ConstraintLocator *locator,
-                        ContextualTypeInfo context = ContextualTypeInfo(),
-                        bool isDiscarded = false) {
-  return std::make_tuple(node, context, isDiscarded, locator);
+                        ContextualTypeInfo context = ContextualTypeInfo()) {
+  return std::make_tuple(node, context, locator);
 }
 
 static ProtocolDecl *getSequenceProtocol(ASTContext &ctx, SourceLoc loc,
@@ -754,8 +751,6 @@ class ClosureConstraintGenerator
 
   void visitBraceStmt(BraceStmt *braceStmt) {
     if (isSupportedMultiStatementClosure()) {
-      auto &ctx = cs.getASTContext();
-
       if (isChildOf(StmtKind::Case)) {
         auto *caseStmt = cast<CaseStmt>(
             locator->castLastElementTo<LocatorPathElt::ClosureBodyElement>()
@@ -770,15 +765,10 @@ class ClosureConstraintGenerator
 
       SmallVector<ElementInfo, 4> elements;
       for (auto element : braceStmt->getElements()) {
-        bool isDiscarded =
-            element.is<Expr *>() &&
-            (!ctx.LangOpts.Playground && !ctx.LangOpts.DebuggerSupport);
-
         elements.push_back(makeElement(
             element,
             cs.getConstraintLocator(
-                locator, LocatorPathElt::ClosureBodyElement(element)),
-            /*contextualInfo=*/{}, isDiscarded));
+                locator, LocatorPathElt::ClosureBodyElement(element))));
       }
 
       createConjunction(cs, elements, locator);
@@ -855,7 +845,7 @@ class ClosureConstraintGenerator
 
   bool isSupportedMultiStatementClosure() const {
     return !closure->hasSingleExpressionBody() &&
-           cs.participatesInInference(closure);
+           shouldTypeCheckInEnclosingExpression(closure);
   }
 
 #define UNSUPPORTED_STMT(STMT) void visit##STMT##Stmt(STMT##Stmt *) { \
@@ -886,7 +876,7 @@ class ClosureConstraintGenerator
 bool ConstraintSystem::generateConstraints(ClosureExpr *closure) {
   auto &ctx = closure->getASTContext();
 
-  if (participatesInInference(closure)) {
+  if (shouldTypeCheckInEnclosingExpression(closure)) {
     ClosureConstraintGenerator generator(*this, closure,
                                          getConstraintLocator(closure));
     generator.visit(closure->getBody());
@@ -935,22 +925,28 @@ bool isConditionOfStmt(ConstraintLocatorBuilder locator) {
 
 ConstraintSystem::SolutionKind
 ConstraintSystem::simplifyClosureBodyElementConstraint(
-    ASTNode element, ContextualTypeInfo context, bool isDiscarded,
-    TypeMatchOptions flags, ConstraintLocatorBuilder locator) {
+    ASTNode element, ContextualTypeInfo context, TypeMatchOptions flags,
+    ConstraintLocatorBuilder locator) {
   auto *closure = castToExpr<ClosureExpr>(locator.getAnchor());
 
   ClosureConstraintGenerator generator(*this, closure,
                                        getConstraintLocator(locator));
 
   if (auto *expr = element.dyn_cast<Expr *>()) {
-    SolutionApplicationTarget target(expr, closure, context.purpose,
-                                     context.getType(), isDiscarded);
+    if (context.purpose != CTP_Unused) {
+      SolutionApplicationTarget target(expr, closure, context.purpose,
+                                       context.getType(),
+                                       /*isDiscarded=*/false);
 
-    if (generateConstraints(target, FreeTypeVariableBinding::Disallow))
-      return SolutionKind::Error;
+      if (generateConstraints(target, FreeTypeVariableBinding::Disallow))
+        return SolutionKind::Error;
+
+      setSolutionApplicationTarget(expr, target);
+      return SolutionKind::Solved;
+    }
 
-    setSolutionApplicationTarget(expr, target);
-    return SolutionKind::Solved;
+    if (!generateConstraints(expr, closure, /*isInputExpression=*/false))
+      return SolutionKind::Error;
   } else if (auto *stmt = element.dyn_cast<Stmt *>()) {
     generator.visit(stmt);
   } else if (auto *cond = element.dyn_cast<StmtCondition *>()) {
@@ -1164,17 +1160,12 @@ class ClosureConstraintApplication
 
     auto forEachTarget =
         rewriteTarget(*cs.getSolutionApplicationTarget(forEachStmt));
-
     if (!forEachTarget)
       hadError = true;
 
     auto body = visit(forEachStmt->getBody()).get<Stmt *>();
     forEachStmt->setBody(cast<BraceStmt>(body));
 
-    // Check to see if the sequence expr is throwing (in async context),
-    // if so require the stmt to have a `try`.
-    hadError |= diagnoseUnhandledThrowsInAsyncContext(closure, forEachStmt);
-
     return forEachStmt;
   }
 
@@ -1250,32 +1241,13 @@ class ClosureConstraintApplication
   }
 
   ASTNode visitBraceStmt(BraceStmt *braceStmt) {
-    auto &cs = solution.getConstraintSystem();
-
-    // Diagnose defer statement being last one in block.
-    if (!braceStmt->empty()) {
-      if (auto stmt = braceStmt->getLastElement().dyn_cast<Stmt *>()) {
-        if (auto deferStmt = dyn_cast<DeferStmt>(stmt)) {
-          auto &diags = closure->getASTContext().Diags;
-          diags
-              .diagnose(deferStmt->getStartLoc(), diag::defer_stmt_at_block_end)
-              .fixItReplace(deferStmt->getStartLoc(), "do");
-        }
-      }
-    }
-
     for (auto &node : braceStmt->getElements()) {
       if (auto expr = node.dyn_cast<Expr *>()) {
         // Rewrite the expression.
-        auto target = *cs.getSolutionApplicationTarget(expr);
-        if (auto rewrittenTarget = rewriteTarget(target)) {
-          node = rewrittenTarget->getAsExpr();
-
-          if (target.isDiscardedExpr())
-            TypeChecker::checkIgnoredExpr(castToExpr(node));
-        } else {
+        if (auto rewrittenExpr = rewriteExpr(expr))
+          node = rewrittenExpr;
+        else
           hadError = true;
-        }
       } else if (auto stmt = node.dyn_cast<Stmt *>()) {
         node = visit(stmt);
       } else {