[SE-0286] Forward matching of trailing closure arguments #33092
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@swift-ci please test |
|
@swift-ci please test source compatibility |
|
@swift-ci please build toolchain |
|
Build failed |
|
Linux Toolchain (Ubuntu 16.04) Install command |
|
Build failed |
|
macOS Toolchain Install command |
If we're matching the trailing closure at the end but haven't seen any mismatches yet, consider the trailing closure to be a mismatch. This function probably needs to be rewritten in terms of matchCallArguments(), because it is performing an incorrect approximation of label matching that doesn't work for multiple trailing closures.
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 }
… closures The "fuzzy" forward scan matching algorithm was only applied when there was a single, unlabeled trailing closure, but was disabled in the presence of multiple trailing closures. Extend the "fuzzy" match to account for multiple trailing closures, by restricting the search for "a later parameter that needs an argument" to stop when we find a parameter that matches the first (labeled) trailing closure.
…parameters. Once the first argument for a variadic function-typed parameter has been matched, allow an unlabeled trailing closure to match, rather than banning all uses of the unlabeled trailing closure with variadic parameters.
When we are performing a call through a value of function type, rather than calling a particular declaration that has parameter declarations, allow any parameter to accept trailing closures.
The change to the forward-scanning rule regressed some diagnostics, because we no longer generated the special "trailing closure mismatch" diagnostic. Reinstate the special-case "trailing closure mismatch" diagnostic, but this time do so as part of the normal argument mismatch diagnostics so it is based on type information. While here, clean up the handling of missing-argument diagnostics to deal with (multiple) trailing closures properly, so that we can (e.g) suggest adding a new labeled trailing closure at the end, rather than producing nonsensical Fix-Its. And, note that SR-12291 is broken (again) by the forward-scan matching rules.
Diagnosis for invalid uses of trailing closures has been folded in with argument-matching diagnostics, so remove all of the machinery around the syntactic "mismatched trailing closure" logic.
SE-0286 states that the "fuzz" heuristic is part of the new language behavior, so don't automatically disable it in Swift 6+ mode.
SE-0248 changes the backward-scan matching behavior for the unlabeled
trailing closure into a forward scan. In circumstances where this
could silently change the meaning of a call to a particular
function, i.e., when there are two defaulted closure parameters such
that a given closure to match either one of them, produce an warning
that describes the change in behavior. For example:
t4.swift:2:24: warning: since Swift 5.3, unlabeled trailing
closure argument matches parameter 'x' rather than parameter 'z'
trailingClosureSingle2 { $0 }
^
t4.swift:2:24: note: label the argument with 'z' to retain the
pre-Swift 5.3 behavior
trailingClosureSingle2 { $0 }
^
(z: )
t4.swift:2:24: note: label the argument with 'x' to silence this
warning for Swift 5.3 and newer
trailingClosureSingle2 { $0 }
^
(x: )
t4.swift:1:6: note: 'trailingClosureSingle2(x:y:z:)' contains
defaulted closure parameters 'x' and 'z'
func trailingClosureSingle2(x: (Int) -> Int = { $0 } , y: (Int) ->
Int = { $0 }, z: (Int) -> Int = { $0 }) {}
^ ~
This explains the (rare) case where SE-0286 silently changes the
meaning of a program, offering Fix-Its to either restore the
pre-SE-0286 behavior or silence the warning, as appropriate.
To better preserve source compatibility, teach the constraint solver to try both the new forward scanning rule as well as the backward scanning rule when matching a single, unlabeled trailing closure. In the extreme case, where the unlabeled trailing closure matches different parameters with the different rules, and yet both produce a potential match, introduce a disjunction to explore both possibilities. Prefer solutions that involve forward scans to those that involve backward scans, so we only use the backward scan as a fallback.
Whenever we form a call that relies on the deprecated "backward" scan,
produce a warning to note the deprecation along with a Fix-It to label
the parameter appropriately (and suppress the warning). For example:
warning: backward matching of the unlabeled trailing closure is
deprecated; label the argument with 'g' to suppress this warning
trailingClosureEitherDirection { $0 * $1 }
^
(g: )
My experiment to improve source compatibility by also performing a backward scan removed the SE-0286 heuristic that skipped binding the unlabeled trailing closure to a defaulted parameter when that would fail. Reinstate that heuristic, which makes more existing code work with the forward-scan behavior. This makes my source-compatibility improvements a quality-of-implementation
This approach, suggested by Xiaodi Wu, provides better source compatibility for existing Swift code, by breaking ties in favor of the existing Swift semantics. Each time the backward-scan rule is needed (and differs from the forward-scan result), we will produce a warning + Fix-It to prepare for Swift 6 where the backward rule can be removed.
…ange. With the constraint solver preferring backward scanning to forward scanning, there is no need to point out the ambiguity: we will always, consistently warn about backward scanning when it produced a result that was different from the forward scan.
DougGregor
force-pushed
the
forward-trailing-closure-matching-sourcecompat-backward-bias
branch
from
3017087 to
2979d4a
Compare
|
The toolchains are good, but I failed to update some diagnostics and now there's a conflict. Rebasing! |
|
@swift-ci please smoke test |
|
@swift-ci please test source compatibility |
DougGregor
changed the title
DougGregor
deleted the
forward-trailing-closure-matching-sourcecompat-backward-bias
branch
theblixguy
added
the
swift evolution approved
xedin
reviewed
Jul 29, 2020
| /// For locators associated with call expressions, the trailing closure | ||
| /// matching rule that was applied. | ||
| llvm::SmallMapVector<ConstraintLocator*, TrailingClosureMatching, 4> | ||
| trailingClosureMatchingChoices; |
There was a problem hiding this comment.
I think that this could be avoided if the matching of the argument in Backward mode always resulted in the "warning" fix being recorded in the constraint system. Such fixes do not interfere with solution application and at the same time would record all of the information necessary to produce a warning e.g. which parameter did get bound to the trailing closure. This would also help remove most of the new logic from CSApply which tried to determine what happened with trailing closure.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Implementation of SE-0286 as accepted.