[Refactoring] Support refactoring calls to async if a variable or function is used as completion handler

[ahoppen]

Previously, we only supported refactoring a function to call the async alternative if a closure was used for the callback parameter. With this change, we also support calling a arbitrary function (or variable with function type) that is passed to the completion handler argument.

The implementation basically re-uses the code we already have to create the legacy function’s body (which calls the newly created async version and then forwards the arguments to the legacy completion handler).

To describe the completion handler that the result is being forwarded to, I’m also using AsyncHandlerDesc, but since the completion handler may be a variable, it doesn’t necessarily have an Index within a function decl that declares it. Because of this, I split the AsyncHandlerDesc up into a context-free AsyncHandlerDesc (without an Index) and AsyncHandlerParamDesc (which includes the Index). It turns out that AsyncHandlerDesc is sufficient in most places.

Resolves rdar://77460524 [SR-14573]

---

This PR consists of three commits, which might be easiest to review on their own. After each commit, the source should be in a buildable state that passes all tests

1. Merge LegacyAlternativeBodyCreator into AsyncConverter

This will later allow us to reuse parts of LegacyAlternativeBodyCreator from AsyncConverter when refactoring calls to an async alternative if they pass a variable as the completion handler.

2. Replace usage of constant strings with tok:: wherever possible

The code moved from LegacyAlternativeBodyCreator was using constant strings a lot. Use tok:: instead to match the style of AsyncConverter.

3. Support refactoring calls to async if a variable or function is used as completion handler

The main change described above.

[ahoppen]

@swift-ci Please test

[bnbarham]

Maybe we should have a file with all the declarations of to-be-async functions and then split basic.swift into multiple files that then compile with that file? It's getting... quite large. Can do in another PR though I guess.

[ahoppen]

I can split it in a follow-up PR once your and my changes are merged.

[bnbarham]

IMO I'd name completionHandler something else for these tests, since otherwise they look like completion handler functions, but we're using convert call instead of convert function. maybe runAfter or something?

func asyncThen(runAfter: (String) -> Void) async {
  let result = await simple()
  runAfter(result)
}

The function as-is looks like it should just be converted to:

func someCompletionHandler() async -> String {
  return simple()
}

The member test cases look good 👍

[bnbarham]

Also, if these were treated as completion handler functions, what happens when converting/adding async? eg.

func simple(_ completionHandler: () -> Void) {}

// Convert/add on `delay`
func delay(_ function: () -> Void) {
  simple(function)
}

[ahoppen]

IMO I'd name completionHandler something else for these tests, since otherwise they look like completion handler functions, but we're using convert call instead of convert function. maybe runAfter or something?

testSimpleWithVariableCompletionHandler is essentially just a wrapper around simple. The user might want to do some logging here, convert arguments etc, so I think completionHandler is the correct name. I
deliberately didn’t use completion to make sure we are not picking up the variable name from the simple function declaration.

The function as-is looks like it should just be converted to

I don’t think I follow. Is this the same point as the one below?

Also, if these were treated as completion handler functions, what happens when converting/adding async? eg.

Good point. With this change, we rewrite

// RUN: %refactor -convert-to-async -dump-rewritten -source-filename %s -pos=%(line+1):1
func testSimpleWithVariableCompletionHandler(completionHandler: (String) -> Void) {
  simple(completion: completionHandler)
}

to

func testSimpleWithVariableCompletionHandler() async -> String {
  let result = await simple()
  completionHandler(result)
}

which is definitely not correct.

[bnbarham]

Yeah, basically the same point. I'm not really sure what this should be converted to though.

[ahoppen]

OK, so I’ve moved all my new test cases to variable_as_callback.swift, so that basic.swift doesn’t get larger still. All other changes are in subsequent commits (hope this makes it easier to review)

• An improvement to the creation of legacy bodies: If a parameter to the completion handler is non-optional, don’t bail out entirely, but put in a placeholder that the user can fill with a sensible default value. This is important because we’re using the same logic to also convert calls with variables as completion handlers and don’t want to stop the refactoring entirely if one of the completion handler’s arguments is not Optional
• The case discussed above where a call to a removed completion handler should be promoted to a return statement. For example

func foo(completion: (String) -> Void) {
  bar(completion)
}

now becomes

func foo() async -> String {
  return await bar()
}

• Some cleanup to the test (rename CHECK strings and check that the refactored code compiles)

[ahoppen]

@swift-ci Please test

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - 0dfa4fdcc2d4b159eab7e331f2b23bac647a1e01

[bnbarham]

Other than that one comment, looks good to me! You're probably going to beat me to merge, I realised there was another bug with the shadowing change - it wasn't renaming hoisted decls when the call was eg. within an if :(. So I'll probably be rebasing :)

[bnbarham]

Should this be noParams()?

[ahoppen]

Yes, it should.

[ahoppen]

@swift-ci Please test