[Async Refactoring] Wrap code in a continuation if conversion doesn't yield reasonable results #38193

ahoppen · 2021-07-01T14:00:32Z

If we are requested to convert a function to async, but the call in the function’s body that eventually calls the completion handler doesn’t have an async alternative, we are currently copying the call as-is, replacing any calls to the completion handler by placeholders.

Example 1

func testDispatch(completionHandler: @escaping (Int) -> Void) {
  DispatchQueue.global.async {
     completionHandler(longSyncFunc())
  }
}

becomes

func testDispatch() async -> Int  {
  DispatchQueue.global.async {
     <#completionHandler#>(longSyncFunc())
  }
}

Example 2

func testUrlSession(completionHandler: @escaping (Data) -> Void) {
  let task = URLSession.shared.dataTask(with: request) { data, response, error in
    completion(data!)
  }
  task.resume()
}

becomes

func testUrlSession() async -> Data {
  let task = URLSession.shared.dataTask(with: request) { data, response, error in
    <#completion#>(data!)
  }
  task.resume()
}

Both of these are better modelled using continuations. Thus, if we find an expression that contains a call to the completion handler and can’t be hoisted to an await statement, we are wrapping the rest of the current scope in a withChecked(Throwing)Continuation, producing the following results:

Example 1

func testDispatch() async -> Int {
  return await withCheckedContinuation { (continuation: CheckedContinuation<Int, Never>) in
    DispatchQueue.global.async {
      continuation.resume(returning: longSyncFunc())
    }
  }
}

Example 2

func testDataTask() async -> Int?
  return await withCheckedContinuation { (continuation: CheckedContinuation<Data, Never>) in
    let task = URLSession.shared.dataTask { data, response, error in
      continuation.resume(returning: data!)
    }
    task.resume()
  }
}

I think both are much closer to what the developer is actually expecting.

Resolves rdar://79304583

ahoppen · 2021-07-01T14:00:46Z

@swift-ci Please test

ktoso

I discussed this one quite a bit with Ben, looks good!

Thankfully we have the checked continuations so any abuse will crash in expected ways which is fine... Agree that it's more what users are expecting from such refactoring 👍

bnbarham

Actually ended up not too bad at all. Thanks Alex!

bnbarham · 2021-07-02T00:55:15Z

lib/IDE/Refactoring.cpp

+      //  - the current expression is nested (we can't start hoisting in the
+      //    middle of an expression)
+      //  - the current scope is wrapped in a continuation (we can't have await
+      //    calls in the contiuation block)


"contiuation" -> "continuation"

bnbarham · 2021-07-02T00:57:02Z

lib/IDE/Refactoring.cpp

@@ -6331,6 +6463,15 @@ class AsyncConverter : private SourceEntityWalker {
    return false;
  }

+  /// Insert custom text at the given \p Loc that shouldn't replace any existing
+  /// source code.
+  bool insertCustom(SourceLoc Loc, std::function<void()> Custom = {}) {


Can use llvm::function_ref since we're just calling it here

I guess https://github.com/apple/swift/blob/aa3ede15389ced5e0cdc017702906d5e2a4e953d/lib/IDE/Refactoring.cpp#L6327 should also be an llvm:function_ref then, right?

bnbarham · 2021-07-02T01:05:02Z

lib/IDE/Refactoring.cpp

+      // Carry over the continuation name.
+      Identifier PreviousContinuationName = Scopes.back().ContinuationName;
+      Scopes.emplace_back(PreviousContinuationName);
+    }
    for (auto DeclAndNumRefs : Decls) {


While you're here would you mind fixing this up to just "D" or something? I assume it was a map of Decl -> number of references at some point?

bnbarham · 2021-07-02T01:06:50Z

test/refactoring/ConvertAsync/convert_function.swift

@@ -143,13 +143,14 @@ func asyncUnhandledCompletion(_ completion: (String) -> Void) {
 }
 // ASYNC-UNHANDLED: func asyncUnhandledCompletion() async -> String {


Rename since it is handled now?

bnbarham · 2021-07-02T01:07:02Z

test/refactoring/ConvertAsync/convert_function.swift

+// ASYNC-UNHANDLED-NEXT:   }
+// ASYNC-UNHANDLED-NEXT:   if !success {
+// ASYNC-UNHANDLED-NEXT:     continuation.resume(returning: "bad")
+// ASYNC-UNHANDLED-NEXT:   }
 // ASYNC-UNHANDLED-NEXT: }


Needs one more } here

bnbarham · 2021-07-02T01:16:11Z

test/refactoring/ConvertAsync/convert_function.swift

-// ASYNC-UNHANDLED-NEXT: if !success {
-// ASYNC-UNHANDLED-NEXT: {{^}} return "bad"{{$}}
-// ASYNC-UNHANDLED-NEXT: }
+// ASYNC-UNHANDLED-NEXT: return await withCheckedContinuation { (continuation: CheckedContinuation<String, Never>) in


Is the type ever required in the closure? If not I'd prefer just return await withCheckedContinuation { continuation in

Excellent question. I needed it when I forgot to write return in front of it but it’s not needed anymore. Thanks!

bnbarham · 2021-07-02T01:20:28Z

test/refactoring/ConvertAsync/convert_to_continuation.swift

+func withoutAsyncAlternativeBecauseOfReturnValue(completionHandler: (Int) -> Void) -> Bool {}
+func withoutAsyncAlternativeThrowing(closure: (Int?, Error?) -> Void) {}
+func asyncVoidWithoutAlternative(completionHandler2: () -> Void) {}
+


Need some tests for a completion handler with a Result argument and then also handling to use continuation(with: result) as well

Good catch, the entire Result case wasn’t covered by the implementation yet. I added two test cases now.

bnbarham · 2021-07-02T01:22:18Z

test/refactoring/ConvertAsync/convert_to_continuation.swift

+// THROWING-CONTINUATION-NEXT:   }
+// THROWING-CONTINUATION-NEXT: }
+
+// We can't relay both the result and the error through the continuationinuation. Converting the following results in a compiler error complaining that theError (of type Error?) can't be passed to `continuation.resume(throwing)`.


"continuationinuation" 😆

bnbarham · 2021-07-02T01:26:11Z

test/refactoring/ConvertAsync/convert_to_continuation.swift

+// THROWING-CONTINUATION-RELAYING-ERROR-AND-RESULT:      func testThrowingContinuationRelayingErrorAndResult() async throws -> Int {
+// THROWING-CONTINUATION-RELAYING-ERROR-AND-RESULT-NEXT:   return try await withCheckedThrowingContinuation { (continuation: CheckedContinuation<Int, Error>) in 
+// THROWING-CONTINUATION-RELAYING-ERROR-AND-RESULT-NEXT:     withoutAsyncAlternativeThrowing { (theValue, theError) in
+// THROWING-CONTINUATION-RELAYING-ERROR-AND-RESULT-NEXT:       continuation.resume(throwing: theError)


Would it be possible to check for this case specifically and add our own handling? Ie. convert this into:

if let err = theError { continuation.resume(throwing: theError) } else { continuation.resume(returning: theValue) // or (a, b, c, d) if multiple }

After discussing this with you, I'm happy to just leave the ambiguous case to another PR. We have a similar problem with the regular hoisting where we end up with throw <#err#>. This is better than what we have at the moment anyway.

bnbarham · 2021-07-02T01:51:24Z

test/refactoring/ConvertAsync/convert_to_continuation.swift

+func testDataTask(_ completion: @escaping (Int?) -> Void) {
+  let dataTask = URLSession.shared.dataTask { (data, error) in
+    guard let data = data else {
+      return // Yes, there is a bug about not calling completion here, but that's copied from the Twitter link above


At least we have checked continuations I guess 😨. Ends up being a log, which is better than nothing...

I mean, it was broken before. There’s not much more we can do 🤷‍♂️

Yeah, arguably the log is better than what it was 😆

ahoppen · 2021-07-02T09:30:09Z

@swift-ci Please smoke test

bnbarham

LGTM

ahoppen · 2021-07-02T09:45:00Z

@swift-ci Please smoke test

hamishknight

Looks great!

lib/IDE/Refactoring.cpp

… yield reasonable results If we are requested to convert a function to async, but the call in the function’s body that eventually calls the completion handler doesn’t have an async alternative, we are currently copying the call as-is, replacing any calls to the completion handler by placeholders. For example, ```swift func testDispatch(completionHandler: @escaping (Int) -> Void) { DispatchQueue.global.async { completionHandler(longSyncFunc()) } } ``` becomes ```swift func testDispatch() async -> Int { DispatchQueue.global.async { <#completionHandler#>(longSyncFunc()) } } ``` and ```swift func testUrlSession(completionHandler: @escaping (Data) -> Void) { let task = URLSession.shared.dataTask(with: request) { data, response, error in completion(data!) } task.resume() } ``` becomes ```swift func testUrlSession() async -> Data { let task = URLSession.shared.dataTask(with: request) { data, response, error in <#completion#>(data!) } task.resume() } ``` Both of these are better modelled using continuations. Thus, if we find an expression that contains a call to the completion handler and can’t be hoisted to an await statement, we are wrapping the rest of the current scope in a `withChecked(Throwing)Continuation`, producing the following results: ```swift func testDispatch() async -> Int { return await withCheckedContinuation { (continuation: CheckedContinuation<Int, Never>) in DispatchQueue.global.async { continuation.resume(returning: syncComputation()) } } } ``` and ```swift func testDataTask() async -> Int? return await withCheckedContinuation { (continuation: CheckedContinuation<Data, Never>) in let task = URLSession.shared.dataTask { data, response, error in continuation.resume(returning: data!) } task.resume() } } ``` I think both are much closer to what the developer is actually expecting. Resolves rdar://79304583

ahoppen · 2021-07-02T13:49:11Z

@swift-ci Please smoke test

[Async Refactoring] Code style improvements

d0472e1

ahoppen requested review from bnbarham and hamishknight July 1, 2021 14:00

ktoso approved these changes Jul 1, 2021

View reviewed changes

bnbarham reviewed Jul 2, 2021

View reviewed changes

ahoppen force-pushed the pr/use-continuation branch from 199f845 to 3ded59e Compare July 2, 2021 09:29

bnbarham approved these changes Jul 2, 2021

View reviewed changes

ahoppen force-pushed the pr/use-continuation branch from 3ded59e to 1744d8e Compare July 2, 2021 09:44

hamishknight approved these changes Jul 2, 2021

View reviewed changes

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

ahoppen mentioned this pull request Jul 2, 2021

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

Merged

ahoppen force-pushed the pr/use-continuation branch from 1744d8e to 54fcc90 Compare July 2, 2021 13:48

ahoppen merged commit c67a62d into swiftlang:main Jul 5, 2021

ahoppen deleted the pr/use-continuation branch July 5, 2021 08:42

ahoppen mentioned this pull request Jul 5, 2021

[5.5][Async Refactoring] Wrap code in a continuation if conversion doesn't yield reasonable results #38256

Merged

		@@ -143,13 +143,14 @@ func asyncUnhandledCompletion(_ completion: (String) -> Void) {
		}
		// ASYNC-UNHANDLED: func asyncUnhandledCompletion() async -> String {

[Async Refactoring] Wrap code in a continuation if conversion doesn't yield reasonable results #38193

[Async Refactoring] Wrap code in a continuation if conversion doesn't yield reasonable results #38193

Uh oh!

Conversation

ahoppen commented Jul 1, 2021

Example 1

Example 2

Example 1

Example 2

Uh oh!

ahoppen commented Jul 1, 2021

Uh oh!

ktoso left a comment

Choose a reason for hiding this comment

Uh oh!

bnbarham left a comment

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!

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!

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!

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 2, 2021

Uh oh!

bnbarham left a comment

Choose a reason for hiding this comment

Uh oh!

ahoppen commented Jul 2, 2021

Uh oh!

hamishknight left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ahoppen commented Jul 2, 2021

Uh oh!

Uh oh!