SE-0286: Forward Scan for Trailing Closures

Evolution Proposal Reviews
21

I don’t understand why this needs to be slightly source breaking for a minor release.

+1 for the change but it should be opt in via an declaration attribute during Swift 5.x then make it the default for Swift 6.

22

Hi all. I've updated the toolchain links in the proposal after adding a form of the warning suggested by @xedin and fixing a few bugs:

Please try it out to see if this behaves how you expect, and evaluate the source-compatibility impact yourself.

Doug

1 Like
23

We discussed this in the first pitch, and the attributes make a bit of a long-term mess if we add them. Given the relatively-small source compatibility effect of the change as proposed, and the desire not to have a bunch of APIs designed that try to take advantage of SE-0279 multiple trailing closures but can't (without the attribute), I feel this is the best balance.

Doug

10 Likes
24

I love the spirit behind this proposal.

I am worried that this proposal could result in some mission critical program with poor test coverage silently doing something bad and newsworthy--like moving money into the wrong account, or shutting off some real-world service. I suppose any program of that nature should have good test coverage that would catch a change in behavior, but staking Swift's reputation on it is a real risk.

Can we include a more robust set of checks in the compiler behind a flag so that developers working with existing mission critical programs can set the flag and run those checks at least once on their code? Can we publicize that feature to strongly encourage its use? And, can we package that all in a way that casts Swift in the best light possible?

If we don't make this change, does SE-0279 result in the possibility of behavior changing silently?

--Matt

4 Likes
25

The worry here seems... overwrought. Literally any time we extend the capabilities of Swift, there are subtle edge cases where we can change behavior but won't be able to catch. Just with a quick browse of the Swift Evolution dashboard:

  • SE-0283:

    func f<T: Equatable>(_: T) { } // #1
func f(_: Any) { } // #2
f((1, 2)) // used to call #2, now calls #1

  • SE-0280: too much to type now, but put a static function into a protocol extension with the same signature as an enum case. Before SE-0280, it would pick the static function; now it will pick the enum case.

  • SE-0268: literally a change in semantics

  • SE-0242:

    struct X {
  let a: Int = 17
  let b: Int = 42
}

extension X {
  init(a: Any) { ... } 
}

X(a: 5) // used to call the extension initializer, now calls the implicit member wise initializer

That's just intentional changes that go through evolution; never mind the little bug fixes and tweaks that are routine for a living code base.

I understand the desire to have absolutely certainty that a change in semantics won't cause problems, but that doesn't exist. Even with a perfect solution to detect every change in overload-resolution behavior from this proposal, I guarantee there are other changes that won't be noticed.

Doug

10 Likes
26

Guilty, as charged. I do have a flair for the dramatic.

Nevertheless:

All of those proposals were advertised as being fully source compatible, with one minor exception involving buggy behavior. The unanticipated edge cases you describe cropped up, and yes, the sky did not fall. And, I agree, perfection is not feasible.

The difference, here, is that we have known cases that can/will cause behavior to change. You've provided some tools to explore the source-compatibility impact, which is ... great! Thank you.

All I'm saying is: This process is rushed (with good reason), and that heightens the risk. Let's make sure we explore the problem set, try to identify the patterns that can cause changes in behavior, try to figure out whether those patterns exist in the wild, and, if they do, make a plan for how to migrate those patterns. I know you are working on all of that already, but I'm not sure we have the sort of clear plan that we might otherwise have when proceeding at a more measured pace.

Beside testing our codebases against the tool chain and brainstorming, is there anything we, out in the community, can do to help? How best to explore for changed behavior in the compatibility suite?

1 Like
27
  • What is your evaluation of the proposal?

I'm generally in favor.

  • Is the problem being addressed significant enough to warrant a change to Swift?

Yes, the forward scanning rule is significantly more intuitive and teachable. I think it will also steer users towards designing APIs which work well with multiple trailing closures.

  • Does this proposal fit well with the feel and direction of Swift?

This proposal is a larger source break than what I'd usually like to see in a point release, and I agree with others that introducing it so late in the release process is risky. That said, based on the source compatibility information that's been gathered, the heuristics used to mitigate impact, and the warnings which have been implemented without significantly complicating typechecking, I think in this case the benefits outweigh the risks.

  • If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?

I can't think of any languages I've used with features similar to trailing closure syntax.

  • How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

I've followed the discussions around SE-0279 and this proposal closely

1 Like
28

I’m really +1 for this change. I think that it will definitely help Swift to be clearer and easier to reason about, especially for newcomers.
Apart from that, I cannot add much to this discussion, as everything I would have wanted to say has already been said.

29

Aside from implementing type checker support to attempt both alternatives, which I'm actively experimenting with, the toolchain is likely our best option---try a project and run its tests to see if behavior changed.

Doug

30

As I stated before during the pitch phase, it's clear that a forward scan rule for trailing closures is almost inevitable. I support the envisioned final design for matching closures to parameters.

This particular revision of the proposal is greatly improved from the first draft; I think it is a very positive development that we are not exposing so many knobs and dials to end users for what essentially are language rules.

The heuristic is very clever.

That said, my overall evaluation of the proposal is that it is inconsistent with the core team's stated principles regarding the evolution of trailing closures. A different roadmap for staging in this change that is not source breaking in a minor version would, I think, be compatible with those principles--and I happen to believe that it would be simpler to use and teach.

Yes, for the reasons outlined in the proposal text itself.

In its final design—yes.

In the proposed path to stage in that change—no; in fact, it is in direct conflict with the core team's sole sine qua non requirement for proposals in this area:

(Emphasis original.)

Therefore, although the heuristic is very clever and does greatly cut down on the degree of source breakage, its adoption for the next minor version of Swift runs counter to the articulated must.

Instead, I would suggest the following migration story:

  • In the next minor version of Swift, implement forward scan and backward scan; where the two "scans" do not match, backward scan "wins" (i.e., all code continues to work as it currently does) but the compiler emits a warning.
  • (If we had the option of labeling the first trailing closure, then the warning could then be silenced by explicitly labeling, but we don't have that option at least currently.) For now, the warning can be silenced by not using trailing closure notation. This is not necessary convenient to write, but a fix-it can help automate the task. When the fix-it is applied, the code is made future-proof for a future version of Swift.
  • In Swift 6 (or whatever the next major version is called), forward scan "wins" (unless the compiler is used in a Swift 5 compatibility mode).
  • When Swift 5 compatibility mode is dropped, so too is backward scan from the codebase.

I don't know of another language where this issue arises.
I have done an in-depth study of the issues and participated at all stages of this proposal and of related proposals.

9 Likes
31

Aren't you comparing against SE-0279 as it was accepted? We don't need compatibility with that as it won't ship until Swift 5.3 and this proposal is intended to land before 5.3 ships (if accepted). I can't speak for the core team, but I don't think it's reasonable to hold us to no breaking changes while iterating on a single release.

32

No, this proposal would break existing single trailing closures. See the proposal text for details.

33

Ahh, that's right. Thanks for reminding me.

34

Thank you for the thoughtful review. The migration story you lay out does indeed maintain source compatibility better than the proposal under review. I've been experimenting with doing both scan directions and warning if the backward scan was chosen. For example, this allows the compiler to handle pick between the two based on what works:

func trailingClosureEitherDirection(
  f: (Int) -> Int = { $0 }, g: (Int, Int) -> Int = { $0 + $1 }
) { }

trailingClosureEitherDirection { -$0 }      // only forward scan works, choose that
trailingClosureEitherDirection { $0 * $1 }  // only backward scan works, choose that

For the latter case, there is a warning with a Fix-It to point out that you're doing something that will break:

warning: backward matching of the unlabeled trailing closure is deprecated; label the argument with 'g' 
         to suppress this warning
trailingClosureEitherDirection { $0 * $1 }
                               ^
                              (g:         )

For functions where both scans produce meaningful results, I biased toward the forward scan (i.e., the model we want in Swift 6), producing the warnings shown in this message.

This approach reduces the source-breaking impact of SE-0286 as written (ModelAssistant and SwiftUI-related failures from the source compatibility suite pass with this change), but doesn't really change the proposal itself---it's in the realm of backward-compatibility tricks we tend to do with any Swift release. However, some programs can still break with this scheme, because some amount of code that type-checked as backward will type-check differently as forward.

Note that nearly the same implementation can be used to bias toward the backward scan, matching your migration story. If we go that route, I would like us to be certain that we won't fall into the same trap that SE-0279 did, where we provided great source compatibility but left behind a feature that didn't work as intended. All of the examples from the proposal work properly when biasing toward the backward scan, albeit for different reasons:

  • UIView.animate(withDuration:animations:completion:) works because only the forward scan comes up with an answer. This style of API---required closure first, followed by optional ones---fits well with the forward scan.
  • View.sheet(isPresented:onDismiss:content:) works because the forward scan's heuristic skips onDismiss, so the forward and backward scans produce the same result.
  • BlockObserver.init(startHandler:produceHandler:finishHandler:) works because the type signatures of the various closure parameters are different enough; especially important is that the first and last closure have a different number of parameters, so the type checker can decide on first vs. last. The same occurs with SwiftUI's TextField.init(_:text:onEditingChanged:onCommit:).

I'm somewhat concerned that we've been getting lucky with that third bullet. Is there a class of APIs out there that will be hard to write properly to deal with the mix of forward and backward scanning rules?

Your suggestion does (necessarily) retain the weakness that the unlabeled trailing closure can move from the "last" closure (when the backward rule applies) to an earlier closure (when one uses multiple trailing closures). However, at least the compiler will produce a warning in the former case.

[EDIT #1: I implemented @xwu's suggestion. It's going through testing now]
[EDIT #2: Toolchains for macOS and Linux are now available to implement @xwu's suggestion]

Doug

10 Likes
35

After a bit more thought and experimentation, I've concluded that Xiaodi's proposed revision to SE-0286 is a better direction. It provides better source compatibility that the proposal under review, retains the ability to write APIs that work well with multiple trailing closures, and offers a path to completely eliminating the backward scan in Swift 6. It addresses the concerns raised by @xedin about silently changing the behavior of existing code, by both not changing behavior in Swift < 6 and also warning now about a future behavior change.

To that end, I have opened a pull request with a revision to SE-0286. The forward scan and "heuristic" remain the same, but I've added the backward scan back for Swift < 6. Please take a look and tell me what you think!

Doug

16 Likes
36

The above fix it will not work for single trailing closures.

error: consecutive statements on a line must be separated by ';'

trailingClosureEitherDirection g:{ $0 * $1 } 
                              ^
                              ;
37

The Fix-It is adding parentheses as well as the label, eliminating the use of the trailing closure entirely. Did you try it with the toolchain or are you guessing from the warning output?

Doug

38

Ah no. I didn't try the fix it directly. Just what I though the fix it was telling me to do. The update to the proposal makes this clear though. Thanks! Its great to see.

trailingClosureBothDirections(g: { $0 * $1 })
39

Why do we still need the heuristic? Shouldn't attempting both the forward scan and the backward scan (and preferring the backward scan in ambiguous cases) be sufficient for source compatibility?

40

As the SE-0286 review manager, I'd interested in hearing the community's thoughts about whether a second review would be useful here. It seems to me that Doug's revisions are broadly consistent with the feedback we've received already: while people seem to feel that the forward-scan rule is a clear improvement, there is some serious concern about the source-compatibility impact, and so a more conservative approach in the short term might be warranted. When a revision is this close to the community's apparent consensus, the core team sometimes just accepts the revision rather than running a second review. Is there anyone who would provide significantly different feedback given a second review?

7 Likes