SE-0286: Forward Scan for Trailing Closures

The review of SE-0286: Forward Scan for Trailing Closures begins now and runs through July 27th, 2020.

Reviews are an important part of the Swift evolution process. All review feedback should be either on this forum thread or, if you would like to keep your feedback private, directly to the review manager (via email or direct message in the Swift forums).

What goes into a review of a proposal?

The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of Swift.

When reviewing a proposal, here are some questions to consider:

  • What is your evaluation of the proposal?
  • Is the problem being addressed significant enough to warrant a change to Swift?
  • Does this proposal fit well with the feel and direction of Swift?
  • If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
  • How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

As always, thank you for your contribution to making Swift a better language.

John McCall
Review Manager

11 Likes

From a purely process-oriented perspective, I think this review is coming too soon.

The proposal was completely revamped and a new pitch thread for that version was created only yesterday.

There does not seem to have been sufficient opportunity for the community to discuss the second iteration yet.

7 Likes

This is your opportunity to discuss it. If this is going to be fixed in 5.3, the schedule needs to be a bit abbreviated.

11 Likes
  • What is your evaluation of the proposal?

+1. As Doug put it in the pitch thread, this is a much improved "resting place" from where Swift 5.3 currently stands.

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

Yes, the backward scan rule is the most complex and surprising aspect of multiple trailing closures. This proposal addresses that issue nicely.

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

Yes, it improves clarity at the call site.

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

N/A

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

An in depth study of the general feature area and participation in the discussion threads.

3 Likes

+1. I'm a big fan of the changes made from the first iteration of this pitch and the small tweaks from the second pitch to the final review. I'm glad this is targeted for 5.3.

1 Like

+1, Feedback practically the same as Matt above.

+1 for this pitch.
Looking forward to big 5.3 release.

  • What is your evaluation of the proposal?

+1. This is a clear improvement over SE-0279. I'm not a huge fan of the parsing complexity here, but I like this complexity more than the status quo.

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

Yes. The language was in an awkward place after SE-0279, so improving the situation within the 5.3 timeframe is important and appreciated.

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

Yes. This parsing rule is more intuitive than the backwards scan rule by itself. And, as outlined in the "Future Directions" section, it leaves the language in a position for better changes in the future.

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

N/A

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

Considerable "involvement" in the previous 0279 reviews and other threads, read this particular proposal carefully.

3 Likes
  • What is your evaluation of the proposal?

I think it's a step in a right direction. I'd like to discuss handling of cases where it's possible for forward matching to silently behave differently behavior from backward matching but still type-check.

Let's consider this example:

func executeQuery(
     onSuccess: ((Result) -> Void)? = nil, 
     onError: ((Error) -> Void)? = nil, 
     onCompletion: ((Either<Result, Error>) -> Void)? = nil) { 
  ...
}

executeQuery { log("Query: \(query)") }

Edit: onCompletion: parameter supposed to be onCompletion: ((Either<Result, Error>) -> Void)? = nil.

Based on the proposed rules forward matching would pick onSuccess: but backward one would pick onCompletion:, silently allowing such call to type-check would mean that in Swift today log message would be emitted for every query but in the future it would be emitted only for successful ones. Change in behavior like this could be a surprise for a developer working on that code that's why emitting at least a warning should be considered here to give users a clear indication that, because of the change in compiler, code might behave differently at runtime and they should provide a label to make it explicit for the future readers what is expected behavior in this case.

I think type-checker rule proposed here should be extended to attempt N possible positions where trailing closure is acceptable in "source compatibility mode" and, in this case, try to type-check all possibilities and if multiple match - emit a warning (or an error) if behavior of forward matching is different from "old" backward scanning and suggest the developer to add a label based on what is expected behavior here.

We could keep that until Swift 6 and afterwards switch to pure forward matching without attempting other arguments.

I'd like to add that IHMO executeQuery as written should be banned so instead of warning I think in cases like this it's more appropriate to produce an error asking developer to explicitly specify label (onSuccess: vs. onCompletion:) to define what behavior is expected from the call, but determining this would still require attempting to type-check trailing closure at all three positions with, in my opinion, is a valid trade-off to preserve correctness with added benefit if producing precise diagnostics.

Another example, case mentioned in the proposal:

init(startHandler: ((AOperation) -> Void)? = nil, 
     produceHandler: ((AOperation, Foundation.Operation) -> Void)? = nil,
     finishHandler: ((AOperation, [NSError]) -> Void)? = nil) {
  self.startHandler = startHandler
  self.produceHandler = produceHandler
  self.finishHandler = finishHandler
}

BlockObserver { (operation, errors) in
  print("finishHandler!")
}

Type-checker could produce a warning or error - trailing closure matches both 'produceHandler:' and 'finishHandler:' parameters, explicitly specify label to disambiguate and give two fix-its.

Note that if closure has explicitly declared parameter or result type e.g. (operation: AOperation, error: [NSError]) then trailing closure would have only matched produceHandler: and diagnostic would have to point out that neither startProducer: nor finishProducer: would match and explicit label is always required here to form a correct call.

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

Yes, backward scanning rule is a source of complexity and surprising behavior with multiple significant downsides.

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

Yes, it tries to make behavior at the call site consistent.

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

N/A

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

In-depth study.

3 Likes

I sympathize with @Nevin that this review has been rushed, but as it is effectively "patching" SE-0279 it is definitely desirable to take any fix as part of 5.3 and not have two versions of multiple trailing closures floating around in the wild.


Like @xedin, I'm very uncomfortable with the source compat break, especially now that binary frameworks with inlinable code are a thing. (I think the particular case he brought up is handled by the "skip a parameter if it has a default and there's a later one without a default" rule, but the general concern stands.) I would much prefer "warn whenever all remaining function-like parameters have defaults, compile with the old behavior" as an additional rule on top of the source-compat heuristic, with the justification that if there are many function-like parameters with defaults, it's already not really clear to a reader of the code which one is intended.

7 Likes

Could this be handled by choosing the matching rule based on the version listed on the // swift-compiler-version: line of the .swiftinterface file? Unfortunately it would also mean that some remnant of the old rule would have to remain indefinitely to support those old interface files, I suppose.

1 Like

Don't we use the parenthesized rather than trailing version in Swift interfaces? Given they prefer to make things explicit?

edit: apparently not

➜  Swift.swiftmodule grep 'map {' x86_64-apple-macos.swiftinterface  
  return source.map { $0 as! TargetElement }
    return (maybeNative as? _ContiguousArrayStorage<Element>).map {
    return try lastIndex(where: predicate).map { self[$0] }
      other.lazy.map { ($0, $1) }, uniquingKeysWith: combine)
      .map { _IndexBox(_base: $0) }
2 Likes

Yes, or even better do a minor bump of the interface version ("backwards-compatible" but possibly with new behavior). That's super subtle without a real language break, but you're right that it doesn't have to be something where 5.3 behaves exactly like 5.2 to preserve binary framework behavior.

1 Like

Interfaces currently just print statements and expressions as they were written originally, partly for implementation reasons and partly to "help" if people make formally-breaking-but-source-compatible changes in some safe-enough way. (That is, it's not obvious in practice that being explicit in expressions is actually safer, though I don't remember the details.)

I'm concerned about the compile-time performance impact of attempting N possible positions, but I absolutely agree that the potential for silently changing the meaning of code is a serious concern with this proposal. However, I think we can produce a meaningful warning by looking at the the type-checked call, which doesn't carry the same compile-time cost and should still be effective.

I went ahead and implemented a warning like you suggest, but without fully chasing down all of the solutions. It has a slightly higher chance of false positives because it doesn't check for complete wellformedness, but has a much lower implementation cost and risk. For your executeQuery example, it produces the following:

se0284.swift:12:14: warning: since Swift 5.3, unlabeled trailing closure argument matches parameter 'onSuccess' rather than parameter 'onCompletion'
executeQuery { _ in log("Query: \(query)") }
             ^
se0284.swift:12:14: note: label the argument with 'onCompletion' to retain the pre-Swift 5.3 behavior
executeQuery { _ in log("Query: \(query)") }
             ^
            (onCompletion:                  )
se0284.swift:12:14: note: label the argument with 'onSuccess' to silence this warning for Swift 5.3 and newer
executeQuery { _ in log("Query: \(query)") }
             ^
            (onSuccess:                     )
se0284.swift:5:6: note: 'executeQuery(onSuccess:onError:onCompletion:)' contains defaulted closure parameters 'onSuccess' and 'onCompletion'
func executeQuery(
     ^

Doug

4 Likes

Thanks for looking into this, @Douglas_Gregor!

Another issue I have found with new rule would silently change behavior mixed with overloading:

struct Result {}
struct Either<T, U: Error> {}

struct Query {
  func execute(
    onCompletion: (Either<Result, Error>) -> Void,
    onError: ((Error) -> Void)? = nil,
    onSuccess: ((Result) -> Void)? = nil) {
    print("execute 1")
  }

  func execute(
         retries: Int = 3,
         randomized: Bool = true,
         onSuccess: ((Result) -> Void)? = nil) {
    print("execute 2")
  }
}

func test(q: Query) {
  q.execute { _ in print("success!") }
}

Backward scan currently prefers second overload (Int, Bool, (Result) -> Void where forward matching (based on the toolchain) would choice the first overload and match onCompletion:.

This is something which is going to be much harder to detect and diagnose post type-check...

1 Like

I think this is a "perfect is the enemy of the good" situation. We can introduce complex type checking logic to make this warning perfect, but we risk destabilizing compile-time performance to do so.

Doug

2 Likes

Would it be feasible to at least gather data about whether this theoretical change in semantics is likely to occur in practice? E.g., are there any actual APIs/usages which exhibit the problem in the Swift source compatibility suite, or the larger test suite that you mention in the proposal?

IHMO this is about correctness, if the behavior is going to change we need to at least warn developers about it. It might be scary when it changes for a given overload but it’s even scarier when type-checker decides to silently pick a completely different overload. Test suite might not always catch that and runtime behavior is going to be very hard to debug. That’s why I think having more checking here is acceptable trade-off for a new behavior even if there is a performance impact, which there is no clear indication whether there is based on how such APIs are used, we need more data.

4 Likes

It might be interesting to turn new warning into an error and run source compatibility suite to see how many projects might have compiled differently.

1 Like
Terms of Service

Privacy Policy

Cookie Policy