[Async Refactoring] Split ambiguous (error + success) completion handler calls into success and error case #38227

ahoppen · 2021-07-02T13:46:38Z

Previously, when a completion handler call had arguments for both the success and error parameters, we were always interpreting it as an error call. In case the error argument was an Optional, this could cause us to generate code that didn't compile, because we throwed the Error? or passed it to continuation.resume(throwing), both of which didn't work.

We now generate an if let statement that checks if the error is nil. If it is, the error is thrown. Otherwise, we interpret the call as a success call and return the result.

Example 1 (convert to continuation)

Base Code

func test(completionHandler: (Int?, Error?) -> Void) {
  withoutAsyncAlternativeThrowing { (theValue, theError) in
    completionHandler(theValue, theError)
  }
}

Old Refactoring Result

func test() async throws -> Int {
  return try await withCheckedThrowingContinuation { continuation in
    withoutAsyncAlternativeThrowing { (theValue, theError) in
      continuation.resume(throwing: theError) // error: Argument type 'Error?' does not conform to expected type 'Error'
    }
  }
}

New Refactoring Result

func testThrowingContinuationRelayingErrorAndResult() async throws -> Int {
  return try await withCheckedThrowingContinuation { continuation in 
    withoutAsyncAlternativeThrowing { (theValue, theError) in
      if let error = theError {
        continuation.resume(throwing: error)
      } else {
        guard let theValue = theValue else {
          fatalError("Expected non-nil success argument 'theValue' for nil error")
        }
        continuation.resume(returning: theValue)
      }
    }
  }
}

Example 2 (convert to async/await)

Base Code

func test(completion: (String?, Error?) -> Void) {
  simpleErr() { (res, err) in
    completion(res, err)
  }
}

Old Refactoring Result

func test() async throws -> String {
  let res = try await simpleErr()
  throw <#err#>
}

New Refactoring Result

func test() async throws -> String {
  let res = try await simpleErr()
  return res
}

hamishknight

Nice!

lib/IDE/Refactoring.cpp

ahoppen · 2021-07-05T09:12:30Z

@swift-ci Please smoke test

ahoppen · 2021-07-05T13:47:10Z

To align the generated code with "Add Async Wrapper", I changed it to use guard let else { fatalError } to check that all success parameters are non-nil when the error is nil.

I also ran git-clang-format for #38193 because I forgot it there…

ahoppen · 2021-07-05T14:09:36Z

@swift-ci Please smoke test

hamishknight

A few more comments, but otherwise LGTM!

lib/IDE/Refactoring.cpp

ahoppen · 2021-07-05T15:37:27Z

@swift-ci Please smoke test

hamishknight · 2021-07-05T15:47:28Z

lib/IDE/Refactoring.cpp

+          } else {
+            // We are returning a tuple. Name the result elements 'result' +
+            // index in tuple.
+            ArgName = createUniqueName("result" + std::to_string(I)).str();


Should this be changed to be I + 1 after the indexing change? IMO that's a little more natural for naming (and I believe it's also how we do it in other places). It seems we also don't have a test case that exercises this, would it be worth adding one?

That’s why I had I be 1-based initially. But then I realized that if you have a tuple let result: (Int, Int), you also access it 0-based (e.g. result.0), so I thought that it’s also natural to use 0-based indexing here.

And I thought I added a test case for this but apparently but apparently I didn’t. Did so now.

Hmm, yeah that's a fair point, I don't feel too strongly about it either way

Let’s leave it 0-based then. I think it looks rather pretty in the new test case

continuation.resume(returning: (result0, result1))

is almost like

continuation.resume(returning: (result.0, result.1))

ahoppen · 2021-07-05T15:55:05Z

@swift-ci Please smoke test

bnbarham · 2021-07-05T22:51:35Z

lib/IDE/Refactoring.cpp

@@ -6122,7 +6160,9 @@ class AsyncConverter : private SourceEntityWalker {
    return true;
  }

-  void convertNodes(const NodesToPrint &ToPrint) {
+  void convertNodes(const NodesToPrint &ToPrint, bool InSuccessBlock) {
+    llvm::SaveAndRestore<bool> InSuccesBranch(ConvertingSuccessBlocks,


"Succes" -> "Success"

bnbarham · 2021-07-05T23:26:55Z

test/refactoring/ConvertAsync/convert_function.swift

+// RELAY-ERROR-AND-RESULT-NEXT:   let res = try await simpleErr(arg: "test")
+// RELAY-ERROR-AND-RESULT-NEXT:   return res
+// RELAY-ERROR-AND-RESULT-NEXT: }


Add a test for non-nil error here? Ie.

simple { res in completion(res, MyError()) }

Added one and realized that we were actually dropping the error there (because we had an ambiguous call inside the success block). I changed the heuristic to

only interpret an ambiguous call as a success call if it isn the success branch and the error has been placeholdered

add asserts that the success arguments are nil if we continue to interpret the ambiguous call as an error call

bnbarham · 2021-07-05T23:30:23Z

test/refactoring/ConvertAsync/convert_to_continuation.swift

+// RUN: %refactor -convert-to-async -dump-text -source-filename %s -pos=%(line+1):1 | %FileCheck -check-prefix=THROWING-CONTINUATION-ALWAYS-RETURNING-ERROR-AND-RESULT %s
+func testAlwaysReturnBothResultAndCompletionHandler(completionHandler: (Int?, Error?) -> Void) {
+  withoutAsyncAlternativeBecauseOfMismatchedCompletionHandlerName { theValue in
+    completionHandler(theValue, MyError())


What do you think about just turning this into eg. continuation.resume(throwing: MyError()) without any of the ifs/guards? Ie. If the error value is non-optional, always use it. Or we could go the other way, ie. if the success value is non-optional, always use it. I think the error one makes more sense though.

And in that case I'd probably not worry about adding the assert for any of the cases + only add the guard let for the success when they are optional.

Good idea. I’m still in favor of keeping the asserts because it makes assumptions about our generated code explicit so that we don’t silently drop results.

ahoppen · 2021-07-06T08:40:39Z

Thanks for you feedback @bnbarham and @hamishknight. I’ve

Changed the heuristic to interpret an ambiguous completion call as a success call only if we are in the success branch and the error has been placeholdered
Added asserts verifying that all success parameters are nil if we are interpreting an ambiguous completion call as an error call
Added some more test cases
Renamed 'param' to 'argument' in all assert and fatalError messages because 'param' is an abbreviation that might be unknown for developers outside the compiler business and technically we are asserting on the argument (the thing that is being passed to the function) rather than the parameter (the thing that the function takes)

Here’s a diff between the last and the latest version: https://github.com/ahoppen/swift/compare/80506eb7071b38e302a493520cc11f2b12671890..7a460d87232ca73181c0bfd6b360cac3df10390a

ahoppen · 2021-07-06T09:06:48Z

@swift-ci Please smoke test

bnbarham · 2021-07-06T11:15:04Z

test/refactoring/ConvertAsync/convert_function.swift

+// ALWAYS-THROW-ERROR-AND-RETURN-RESULT-NEXT:   let res = await simple()
+// ALWAYS-THROW-ERROR-AND-RETURN-RESULT-NEXT:   assert(res == nil, "The code this function was generated from passed a result and an error argument to the completion handler. Since this function just throws the error, it is assumed that the result was nil.")
+// ALWAYS-THROW-ERROR-AND-RETURN-RESULT-NEXT:   throw CustomError.Bad
+// ALWAYS-THROW-ERROR-AND-RETURN-RESULT-NEXT: }


Could you make sure we have test cases for a (String?, Error?) -> Void completion handler where:

Result is non-optional and error is optional

Result is optional and error is non-optional

Both are optional

Both are non-optional
Ideally with simple closure args/aliases as well as unknown expressions (ie. a random call/error/whatever).

I don't see a test for (nil, nil) anywhere so if you could add that one that'd be great :).

Finally, in regards to this test specifically, I'd prefer a placeholder and comment rather than the assert (since the compiler error could be confusing for people). Suggested wording could be:

// The original completion handler call passed a non-optional error. An additional result cannot be represented by the async function and just a throws has been generated instead. Note that this drops the result. <#throw CustomError.Bad#>

Note that if the error expression is optional we could save it to a variable and then do an if instead (ie. the same as the continuation refactoring).

Yeah I also think a placeholder + comment would be a little clearer to the user in that case (though I hope it doesn't come up too often)

…e args to std::vector The `std::vector` keeps its elements alive. Currently, we might be accessing uninitalized memory if the call of `callArgs(CE)` (which owns its elements in case there is a single element) goes out of scope before `Args`.

ahoppen · 2021-07-06T16:56:51Z

OK, so I’ve completely rewritten the entire implementation. The logic for determining how to interpret a completion handler call is now shared between the async/await and continuation refactoring in convertHandlerCall. IMHO it has a fairly concise description how the interpretation works instead of the hot-patching I had earlier.

The general rule is now: If there is a non-nil error, then we throw it. If there are any non-nil results, we ignore them (without any assertions or whatsoever). If the error is nil, we return the result. If the result is nil, this results in a compile or runtime error. IIRC this exactly matches what the clang importer is doing.

Consequently, I also removed the assertions for the async wrapper I added in #38259.

ahoppen · 2021-07-06T16:56:57Z

@swift-ci Please smoke test

hamishknight

LGTM!

bnbarham

Had a few small comments but otherwise the new changes LGTM :)!

bnbarham · 2021-07-06T22:34:39Z

test/refactoring/ConvertAsync/convert_function.swift

+// NON-OPTIONAL-RESULT-AND-NON-OPTIONAL-ERROR:      func nonOptionalResultAndNonOptionalError() async throws -> String {
+// NON-OPTIONAL-RESULT-AND-NON-OPTIONAL-ERROR-NEXT:   throw CustomError.Bad
+// NON-OPTIONAL-RESULT-AND-NON-OPTIONAL-ERROR-NEXT: }


Thanks for adding all these tests! Much better coverage now IMO.

Just thought of another test as well - simple arguments wrapped in parentheses.

Since the logic is exactly the same as for the continuation refactoring, the parenthesis case is already covered by #38261. Added a test case in convert_function.swift anyway.

bnbarham · 2021-07-06T22:39:20Z

test/refactoring/ConvertAsync/convert_to_continuation.swift

+// MIXED-OPTIONAL-AND-NON-OPTIONAL-RESULT-NEXT:   return try await withCheckedThrowingContinuation { continuation in 
+// MIXED-OPTIONAL-AND-NON-OPTIONAL-RESULT-NEXT:     withoutAsyncAlternativeThrowing { (theResult, error) in
+// MIXED-OPTIONAL-AND-NON-OPTIONAL-RESULT-NEXT:       guard let theResult1 = theResult else {
+// MIXED-OPTIONAL-AND-NON-OPTIONAL-RESULT-NEXT:         fatalError("Expected non-nil result 'theResult1' for nil error")


The message here is a bit odd since error is completely ignored... Not sure it's worth changing in this case though.

What do you think about changing all messages to one of

Expected non-nil result '...' in the success case

Expected non-nil result '...' in the non-error case

Expected non-nil result '...' if no error is thrown

I think even in the

if let error = error { continuation.resume(throwing: error) } else { guard let first1 = first else { fatalError("Expected non-nil result 'first1' for nil error") } continuation.resume(returning: first1) }

case it’s not entirely clear what nil error refers to.

Thinking about it, I went with the Expected non-nil result '...' in the non-error case message.

I also added a special case to omit the result name if it is "result". I.e. instead of Expected non-nil result 'result' in non-error case, we now emit Expected non-nil result in non-error case. If the result variable has any other name, we still emit it.

bnbarham · 2021-07-06T22:40:36Z

test/refactoring/ConvertAsync/convert_to_continuation.swift

+// PASS-SAME-RESULT-TWICE:      func testPassSameResultTwice() async throws -> (Int, Int) {
+// PASS-SAME-RESULT-TWICE-NEXT:   return try await withCheckedThrowingContinuation { continuation in 
+// PASS-SAME-RESULT-TWICE-NEXT:     withoutAsyncAlternativeThrowing { (theResult, error) in
+// PASS-SAME-RESULT-TWICE-NEXT:       guard let theResult1 = theResult else {


How hard would this be to avoid? If not much effort it'd be good to fix, but probably an uncommon case so I'm fine with leaving.

Probably not too much if we only worry about the exact same argument being passed to the completion handler twice. But I think there’s very little benefit in it (who passes the same argument to the completion handler twice but me?) and I’m more worried that the logic will introduce bugs in the common cases.

bnbarham · 2021-07-06T22:48:16Z

lib/IDE/Refactoring.cpp

+        // interpret it as an error call.
+        AddConvertedHandlerCall(Result);
+      } else {
+        // The call was truely amiguous. Add an


truely -> truly
amiguous -> ambiguous

…ler calls into success and error case Previously, when a completion handler call had arguments for both the success and error parameters, we were always interpreting it as an error call. In case the error argument was an `Optional`, this could cause us to generate code that didn't compile, because we `throw`ed the `Error?` or passed it to `continuation.resume(throwing)`, both of which didn't work. We now generate an `if let` statement that checks if the error is `nil`. If it is, the error is thrown. Otherwise, we interpret the call as a success call and return the result. - Example 1 (convert to continuation) -- Base Code ```swift func test(completionHandler: (Int?, Error?) -> Void) { withoutAsyncAlternativeThrowing { (theValue, theError) in completionHandler(theValue, theError) } } ``` -- Old Refactoring Result ```swift func test() async throws -> Int { return try await withCheckedThrowingContinuation { continuation in withoutAsyncAlternativeThrowing { (theValue, theError) in continuation.resume(throwing: theError) // error: Argument type 'Error?' does not conform to expected type 'Error' } } } -- New Refactoring Result ```swift func testThrowingContinuationRelayingErrorAndResult() async throws -> Int { return try await withCheckedThrowingContinuation { continuation in withoutAsyncAlternativeThrowing { (theValue, theError) in if let error = theError { continuation.resume(throwing: error) } else { guard let theValue = theValue else { fatalError("Expected non-nil success argument 'theValue' for nil error") } continuation.resume(returning: theValue) } } } } ``` - Example 2 (convert to async/await) -- Base Code ```swift func test(completion: (String?, Error?) -> Void) { simpleErr() { (res, err) in completion(res, err) } } ``` -- Old Refactoring Result ```swift func test() async throws -> String { let res = try await simpleErr() throw <#err#> } ``` -- New Refactoring Result ```swift func test() async throws -> String { let res = try await simpleErr() return res } ```

…wrapper Since we aren’t checking that all result arguments are `nil` if there was an `error` in all the other refactorings, also remove the assertions in the async wrapper.

…atalError` messages The 'success param/argument' is a terminology we use internally in the refactoring and not one we want to present to the user. Just name it 'result' instead.

…ents in continuation In the previous `fatalError` message `Expected non-nil result 'result' for nil error`, it wasn’t exactly clear wht the `error` referred to. In some cases, when the error was ignored, there wasn’t even an `error` variable in the refactored code. The new `Expected non-nil result '...' in the non-error case` is a little more generic and also fits if an error is ignored.

ahoppen · 2021-07-07T10:08:16Z

For easier review, here’s the diff between the last and current version: https://github.com/ahoppen/swift/compare/8fc37f8..cd31fa0

ahoppen · 2021-07-07T10:14:27Z

@swift-ci Please smoke test

ahoppen requested review from bnbarham and hamishknight July 2, 2021 13:46

hamishknight reviewed Jul 2, 2021

View reviewed changes

lib/IDE/Refactoring.cpp Outdated Show resolved Hide resolved

lib/IDE/Refactoring.cpp Outdated Show resolved Hide resolved

ahoppen force-pushed the pr/ambiguous-completion-handler-calls branch from 146e8fe to 6a3b1db Compare July 5, 2021 09:12

ahoppen marked this pull request as ready for review July 5, 2021 09:12

hamishknight reviewed Jul 5, 2021

View reviewed changes

lib/IDE/Refactoring.cpp Show resolved Hide resolved

lib/IDE/Refactoring.cpp Outdated Show resolved Hide resolved

lib/IDE/Refactoring.cpp Outdated Show resolved Hide resolved

lib/IDE/Refactoring.cpp Outdated Show resolved Hide resolved

lib/IDE/Refactoring.cpp Show resolved Hide resolved

ahoppen force-pushed the pr/ambiguous-completion-handler-calls branch from ce4ea7e to 69bbd88 Compare July 5, 2021 15:37

hamishknight reviewed Jul 5, 2021

View reviewed changes

ahoppen force-pushed the pr/ambiguous-completion-handler-calls branch from 69bbd88 to 80506eb Compare July 5, 2021 15:54

hamishknight approved these changes Jul 5, 2021

View reviewed changes

bnbarham reviewed Jul 5, 2021

View reviewed changes

ahoppen force-pushed the pr/ambiguous-completion-handler-calls branch from 80506eb to 0c6b9d5 Compare July 6, 2021 08:35

ahoppen force-pushed the pr/ambiguous-completion-handler-calls branch from 7a460d8 to 43e3cab Compare July 6, 2021 09:06

bnbarham reviewed Jul 6, 2021

View reviewed changes

ahoppen force-pushed the pr/ambiguous-completion-handler-calls branch 2 times, most recently from bef04ce to 8fc37f8 Compare July 6, 2021 16:50

hamishknight approved these changes Jul 6, 2021

View reviewed changes

bnbarham approved these changes Jul 6, 2021

View reviewed changes

ahoppen added 2 commits July 7, 2021 11:54

[Async Refactoring] Remove assertions for nil result values in async …

06219cd

…wrapper Since we aren’t checking that all result arguments are `nil` if there was an `error` in all the other refactorings, also remove the assertions in the async wrapper.

ahoppen added 2 commits July 7, 2021 11:54

[Async Refactoring] Rename 'success param/argument' to 'result' in `f…

edc1393

…atalError` messages The 'success param/argument' is a terminology we use internally in the refactoring and not one we want to present to the user. Just name it 'result' instead.

ahoppen force-pushed the pr/ambiguous-completion-handler-calls branch from 8fc37f8 to cd31fa0 Compare July 7, 2021 10:06

ahoppen merged commit ca26a1a into swiftlang:main Jul 7, 2021

ahoppen mentioned this pull request Jul 8, 2021

[5.5][Async Refactoring] Split ambiguous (error + success) completion handler calls into success and error case #38307

Merged

ahoppen deleted the pr/ambiguous-completion-handler-calls branch July 9, 2021 10:39

[Async Refactoring] Split ambiguous (error + success) completion handler calls into success and error case #38227

[Async Refactoring] Split ambiguous (error + success) completion handler calls into success and error case #38227

Uh oh!

Conversation

ahoppen commented Jul 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Example 1 (convert to continuation)

Base Code

Old Refactoring Result

New Refactoring Result

Example 2 (convert to async/await)

Base Code

Old Refactoring Result

New Refactoring Result

Uh oh!

hamishknight left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ahoppen commented Jul 5, 2021

Uh oh!

ahoppen commented Jul 5, 2021

Uh oh!

ahoppen commented Jul 5, 2021

Uh oh!

hamishknight left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ahoppen commented Jul 5, 2021

Uh oh!

hamishknight Jul 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahoppen Jul 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahoppen commented Jul 5, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahoppen commented Jul 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ahoppen commented Jul 6, 2021

Uh oh!

bnbarham Jul 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahoppen commented Jul 6, 2021

Uh oh!

ahoppen commented Jul 6, 2021

Uh oh!

hamishknight left a comment

Choose a reason for hiding this comment

Uh oh!

ahoppen commented Jul 2, 2021 •

edited

Loading

hamishknight Jul 5, 2021 •

edited

Loading

ahoppen Jul 5, 2021 •

edited

Loading

ahoppen commented Jul 6, 2021 •

edited

Loading

bnbarham Jul 6, 2021 •

edited

Loading

ahoppen Jul 7, 2021 •

edited

Loading

ahoppen Jul 7, 2021 •

edited

Loading

ahoppen commented Jul 7, 2021 •

edited

Loading