Forward matching of trailing closure arguments.

Introsuce a new "forward" algorithm for trailing closures where
the unlabeled trailing closure argument matches the next parameter in
the parameter list that can accept an unlabeled trailing closure.

The "can accept an unlabeled trailing closure" criteria looks at the
parameter itself. The parameter accepts an unlabeled trailing closure
if all of the following are true:

* The parameter is not 'inout'
* The adjusted type of the parameter (defined below) is a function type

The adjusted type of the parameter is the parameter's type as
declared, after performing two adjustments:

* If the parameter is an @autoclosure, use the result type of the
parameter's declared (function) type, before performing the second
adjustment.
* Remove all outer "optional" types.

For example, the following function illustrates both adjustments to
determine that the parameter "body" accepts an unlabeled trailing
closure:

    func doSomething(body: @autoclosure () -> (((Int) -> String)?))

This is a source-breaking change. However, there is a "fuzzy" matching
rule that that addresses the source break we've observed in practice,
where a defaulted closure parameter precedes a non-defaulted closure
parameter:

    func doSomethingElse(
       onError: ((Error) -> Void)? = nil,
       onCompletion: (Int) -> Void
    ) { }

    doSomethingElse { x in
      print(x)
    }

With the existing "backward" scan rule, the trailing closure matches
onCompletion, and onError is given the default of "nil". With the
forward scanning rule, the trailing closure matches onError, and there
is no "onCompletion" argument, so the call fails.

The fuzzy matching rule proceeds as follows:
* if the call has a single, unlabeled trailing closure argument, and
* the parameter that would match the unlabeled trailing closure
argument has a default, and
* there are parameters *after* that parameter that require an argument
(i.e., they are not variadic and do not have a default argument)

then the forward scan skips this parameter and considers the next
parameter that could accept the unlabeled trailing closure.

Note that APIs like doSomethingElse(onError:onCompletion:) above
should probably be reworked to put the defaulted parameters at the
end, which works better with the forward scan and with multiple
trailing closures:

    func doSomethingElseBetter(
       onCompletion: (Int) -> Void,
       onError: ((Error) -> Void)? = nil
    ) { }

    doSomethingElseBetter { x in
      print(x)
    }

    doSomethingElseBetter { x in
      print(x)
    } onError: { error in
      throw error
    }

Author: DougGregor

include/swift/AST/Attr.h

@@ -55,6 +55,7 @@ class GenericFunctionType;
 class LazyConformanceLoader;
 class LazyMemberLoader;
 class PatternBindingInitializer;
+enum class TrailingClosureMatching: uint8_t;
 class TrailingWhereClause;
 class TypeExpr;
 

include/swift/AST/Types.h

@@ -3499,6 +3499,7 @@ END_CAN_TYPE_WRAPPER(FunctionType, AnyFunctionType)
 /// has a default argument.
 struct ParameterListInfo {
   SmallBitVector defaultArguments;
+  SmallBitVector acceptsUnlabeledTrailingClosures;
 
 public:
   ParameterListInfo() { }
@@ -3509,6 +3510,10 @@ struct ParameterListInfo {
   /// Whether the parameter at the given index has a default argument.
   bool hasDefaultArgument(unsigned paramIdx) const;
 
+  /// Whether the parameter accepts an unlabeled trailing closure argument
+  /// according to the "forward-scan" rule.
+  bool acceptsUnlabeledTrailingClosureArgument(unsigned paramIdx) const;
+
   /// Retrieve the number of non-defaulted parameters.
   unsigned numNonDefaultedParameters() const {
     return defaultArguments.count();

include/swift/Basic/LangOptions.h

@@ -271,6 +271,14 @@ namespace swift {
     /// behavior. This is a staging flag, and will be removed in the future.
     bool EnableNewOperatorLookup = false;
 
+    /// Whether to enable the "fuzzy" forward-scanning behavior for trailing
+    /// closure matching, which skips over defaulted closure parameters
+    /// to match later (non-defaulted) closure parameters
+    ///
+    /// This is a backward-compatibility hack for unlabeled trailing closures,
+    /// to be disabled in Swift 6+.
+    bool EnableFuzzyForwardScanTrailingClosureMatching = true;
+
     /// Use Clang function types for computing canonical types.
     /// If this option is false, the clang function types will still be computed
     /// but will not be used for checking type equality.

include/swift/Option/Options.td

@@ -546,6 +546,16 @@ def enable_experimental_concise_pound_file : Flag<["-"],
   Flags<[FrontendOption, ModuleInterfaceOption]>,
   HelpText<"Enable experimental concise '#file' identifier">;
 
+def disable_fuzzy_forward_scan_trailing_closure_matching : Flag<["-"],
+    "disable-fuzzy-forward-scan-trailing-closure-matching">,
+  Flags<[FrontendOption]>,
+  HelpText<"Disable fuzzy forward-scan trailing closure matching">;
+
+def enable_fuzzy_forward_scan_trailing_closure_matching : Flag<["-"],
+    "enable-fuzzy-forward-scan-trailing-closure-matching">,
+  Flags<[FrontendOption]>,
+  HelpText<"Enable fuzzy forward-scan trailing closure matching">;
+
 def enable_direct_intramodule_dependencies : Flag<["-"],
     "enable-direct-intramodule-dependencies">,
   Flags<[FrontendOption, HelpHidden]>,

lib/AST/Type.cpp

@@ -832,16 +832,42 @@ Type TypeBase::replaceCovariantResultType(Type newResultType,
   return FunctionType::get(inputType, resultType, fnType->getExtInfo());
 }
 
+/// Whether this parameter accepts an unlabeled trailing closure argument
+/// using the more-restrictive forward-scan rule.
+static bool allowsUnlabeledTrailingClosureParameter(const ParamDecl *param) {
+  // inout parameters never allow an unlabeled trailing closure.
+  if (param->isInOut())
+    return false;
+
+  Type paramType =
+      param->getInterfaceType()->getRValueType()->lookThroughAllOptionalTypes();
+
+  // For autoclosure parameters, look through the autoclosure result type
+  // to get the actual argument type.
+  if (param->isAutoClosure()) {
+    auto fnType = paramType->getAs<AnyFunctionType>();
+    if (!fnType)
+      return false;
+
+    paramType = fnType->getResult()->lookThroughAllOptionalTypes();
+  }
+
+  // After lookup through all optional types, this parameter allows an
+  // unlabeled trailing closure if it is (structurally) a function type.
+  return paramType->is<AnyFunctionType>();
+}
+
 ParameterListInfo::ParameterListInfo(
     ArrayRef<AnyFunctionType::Param> params,
     const ValueDecl *paramOwner,
     bool skipCurriedSelf) {
   defaultArguments.resize(params.size());
+  acceptsUnlabeledTrailingClosures.resize(params.size());
 
   // No parameter owner means no parameter list means no default arguments
   // - hand back the zeroed bitvector.
   //
-  // FIXME: We ought to not request default argument info in this case.
+  // FIXME: We ought to not request paramer list info in this case.
   if (!paramOwner)
     return;
 
@@ -869,12 +895,17 @@ ParameterListInfo::ParameterListInfo(
   if (params.size() != paramList->size())
     return;
 
-  // Note which parameters have default arguments and/or function builders.
+  // Note which parameters have default arguments and/or accept unlabeled
+  // trailing closure arguments with the forward-scan rule.
   for (auto i : range(0, params.size())) {
     auto param = paramList->get(i);
     if (param->isDefaultArgument()) {
       defaultArguments.set(i);
     }
+
+    if (allowsUnlabeledTrailingClosureParameter(param)) {
+      acceptsUnlabeledTrailingClosures.set(i);
+    }
   }
 }
 
@@ -883,6 +914,12 @@ bool ParameterListInfo::hasDefaultArgument(unsigned paramIdx) const {
       : false;
 }
 
+bool ParameterListInfo::acceptsUnlabeledTrailingClosureArgument(
+    unsigned paramIdx) const {
+  return paramIdx < acceptsUnlabeledTrailingClosures.size() &&
+      acceptsUnlabeledTrailingClosures[paramIdx];
+}
+
 /// Turn a param list into a symbolic and printable representation that does not
 /// include the types, something like (_:, b:, c:)
 std::string swift::getParamListAsString(ArrayRef<AnyFunctionType::Param> params) {

lib/Driver/ToolChains.cpp

@@ -264,6 +264,10 @@ void ToolChain::addCommonFrontendArgs(const OutputInfo &OI,
   inputArgs.AddLastArg(arguments, options::OPT_disable_parser_lookup);
   inputArgs.AddLastArg(arguments,
                        options::OPT_enable_experimental_concise_pound_file);
+  inputArgs.AddLastArg(
+      arguments,
+      options::OPT_enable_fuzzy_forward_scan_trailing_closure_matching,
+      options::OPT_disable_fuzzy_forward_scan_trailing_closure_matching);
   inputArgs.AddLastArg(arguments,
                        options::OPT_verify_incremental_dependencies);
   inputArgs.AddLastArg(arguments,

lib/Frontend/CompilerInvocation.cpp

@@ -572,6 +572,10 @@ static bool ParseLangArgs(LangOptions &Opts, ArgList &Args,
 
   Opts.EnableConcisePoundFile =
       Args.hasArg(OPT_enable_experimental_concise_pound_file);
+  Opts.EnableFuzzyForwardScanTrailingClosureMatching =
+      Args.hasFlag(OPT_enable_fuzzy_forward_scan_trailing_closure_matching,
+                   OPT_disable_fuzzy_forward_scan_trailing_closure_matching,
+                   !Opts.EffectiveLanguageVersion.isVersionAtLeast(6));
 
   Opts.EnableCrossImportOverlays =
       Args.hasFlag(OPT_enable_cross_import_overlays,

lib/Sema/CSApply.cpp

@@ -1529,16 +1529,13 @@ namespace {
     /// \param callee The callee for the function being applied.
     /// \param apply The ApplyExpr that forms the call.
     /// \param argLabels The argument labels provided for the call.
-    /// \param hasTrailingClosure Whether the last argument is a trailing
-    /// closure.
     /// \param locator Locator used to describe where in this expression we are.
     ///
     /// \returns the coerced expression, which will have type \c ToType.
     Expr *
     coerceCallArguments(Expr *arg, AnyFunctionType *funcType,
                         ConcreteDeclRef callee, ApplyExpr *apply,
                         ArrayRef<Identifier> argLabels,
-                        bool hasTrailingClosure,
                         ConstraintLocatorBuilder locator);
 
     /// Coerce the given object argument (e.g., for the base of a
@@ -1749,7 +1746,7 @@ namespace {
       auto fullSubscriptTy = openedFullFnType->getResult()
                                   ->castTo<FunctionType>();
       index = coerceCallArguments(index, fullSubscriptTy, subscriptRef, nullptr,
-                                  argLabels, hasTrailingClosure,
+                                  argLabels,
                                   locator.withPathElement(
                                     ConstraintLocator::ApplyArgument));
       if (!index)
@@ -2573,7 +2570,6 @@ namespace {
       auto newArg = coerceCallArguments(
           expr->getArg(), fnType, witness,
           /*applyExpr=*/nullptr, expr->getArgumentLabels(),
-          expr->hasTrailingClosure(),
           cs.getConstraintLocator(expr, ConstraintLocator::ApplyArgument));
 
       expr->setInitializer(witness);
@@ -4926,7 +4922,7 @@ namespace {
       auto *newIndexExpr =
           coerceCallArguments(indexExpr, subscriptType, ref,
                               /*applyExpr*/ nullptr, labels,
-                              /*hasTrailingClosure*/ false, locator);
+                              locator);
 
       // We need to be able to hash the captured index values in order for
       // KeyPath itself to be hashable, so check that all of the subscript
@@ -5459,12 +5455,12 @@ static bool hasCurriedSelf(ConstraintSystem &cs, ConcreteDeclRef callee,
   return false;
 }
 
-Expr *ExprRewriter::coerceCallArguments(Expr *arg, AnyFunctionType *funcType,
-                                        ConcreteDeclRef callee,
-                                        ApplyExpr *apply,
-                                        ArrayRef<Identifier> argLabels,
-                                        bool hasTrailingClosure,
-                                        ConstraintLocatorBuilder locator) {
+Expr *ExprRewriter::coerceCallArguments(
+    Expr *arg, AnyFunctionType *funcType,
+    ConcreteDeclRef callee,
+    ApplyExpr *apply,
+    ArrayRef<Identifier> argLabels,
+    ConstraintLocatorBuilder locator) {
   auto &ctx = getConstraintSystem().getASTContext();
   auto params = funcType->getParams();
 
@@ -5517,9 +5513,11 @@ Expr *ExprRewriter::coerceCallArguments(Expr *arg, AnyFunctionType *funcType,
 
   MatchCallArgumentListener listener;
   SmallVector<ParamBinding, 4> parameterBindings;
+  auto unlabeledTrailingClosureIndex =
+      arg->getUnlabeledTrailingClosureIndexOfPackedArgument();
   bool failed = constraints::matchCallArguments(args, params,
                                                 paramInfo,
-                       arg->getUnlabeledTrailingClosureIndexOfPackedArgument(),
+                                                unlabeledTrailingClosureIndex,
                                                 /*allowFixes=*/false, listener,
                                                 parameterBindings);
 
@@ -5748,7 +5746,7 @@ Expr *ExprRewriter::coerceCallArguments(Expr *arg, AnyFunctionType *funcType,
       (params[0].getValueOwnership() == ValueOwnership::Default ||
        params[0].getValueOwnership() == ValueOwnership::InOut)) {
     assert(newArgs.size() == 1);
-    assert(!hasTrailingClosure);
+    assert(!unlabeledTrailingClosureIndex);
     return newArgs[0];
   }
 
@@ -5761,8 +5759,9 @@ Expr *ExprRewriter::coerceCallArguments(Expr *arg, AnyFunctionType *funcType,
       argParen->setSubExpr(newArgs[0]);
     } else {
       bool isImplicit = arg->isImplicit();
-      arg = new (ctx)
-          ParenExpr(lParenLoc, newArgs[0], rParenLoc, hasTrailingClosure);
+      arg = new (ctx) ParenExpr(
+          lParenLoc, newArgs[0], rParenLoc,
+          static_cast<bool>(unlabeledTrailingClosureIndex));
       arg->setImplicit(isImplicit);
     }
   } else {
@@ -5776,8 +5775,8 @@ Expr *ExprRewriter::coerceCallArguments(Expr *arg, AnyFunctionType *funcType,
       }
     } else {
       // Build a new TupleExpr, re-using source location information.
-      arg = TupleExpr::create(ctx, lParenLoc, newArgs, newLabels, newLabelLocs,
-                              rParenLoc, hasTrailingClosure,
+      arg = TupleExpr::create(ctx, lParenLoc, rParenLoc, newArgs, newLabels,
+                              newLabelLocs, unlabeledTrailingClosureIndex,
                               /*implicit=*/arg->isImplicit());
     }
   }
@@ -7128,9 +7127,6 @@ Expr *ExprRewriter::finishApply(ApplyExpr *apply, Type openedType,
   auto *arg = apply->getArg();
   auto *fn = apply->getFn();
 
-  bool hasTrailingClosure =
-    isa<CallExpr>(apply) && cast<CallExpr>(apply)->hasTrailingClosure();
-
   auto finishApplyOfDeclWithSpecialTypeCheckingSemantics
     = [&](ApplyExpr *apply,
           ConcreteDeclRef declRef,
@@ -7146,7 +7142,6 @@ Expr *ExprRewriter::finishApply(ApplyExpr *apply, Type openedType,
         arg = coerceCallArguments(arg, fnType, declRef,
                                   apply,
                                   apply->getArgumentLabels(argLabelsScratch),
-                                  hasTrailingClosure,
                                   locator.withPathElement(
                                     ConstraintLocator::ApplyArgument));
         if (!arg) {
@@ -7350,7 +7345,6 @@ Expr *ExprRewriter::finishApply(ApplyExpr *apply, Type openedType,
   if (auto fnType = cs.getType(fn)->getAs<FunctionType>()) {
     arg = coerceCallArguments(arg, fnType, callee, apply,
                               apply->getArgumentLabels(argLabelsScratch),
-                              hasTrailingClosure,
                               locator.withPathElement(
                                   ConstraintLocator::ApplyArgument));
     if (!arg) {