Skip to content

Commit 7e4b66e

Browse files
hamishknighthamishknight
authored
Merge pull request #37131 from hamishknight/out-of-the-void
[Refactoring] Improve Void handling for async conversion
2 parents 39c80ee + c1e4dc3 commit 7e4b66e

File tree

4 files changed

+200
-50
lines changed

4 files changed

+200
-50
lines changed

‎lib/IDE/Refactoring.cpp

Lines changed: 98 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -4141,6 +4141,14 @@ struct AsyncHandlerDesc {
41414141
return Ty->getParams();
41424142
}
41434143

4144+
/// Retrieve the parameters relevant to a successful return from the
4145+
/// completion handler. This drops the Error parameter if present.
4146+
ArrayRef<AnyFunctionType::Param> getSuccessParams() const {
4147+
if (HasError && Type == HandlerType::PARAMS)
4148+
return params().drop_back();
4149+
return params();
4150+
}
4151+
41444152
/// The `CallExpr` if the given node is a call to the `Handler`
41454153
CallExpr *getAsHandlerCall(ASTNode Node) const {
41464154
if (!isValid())
@@ -4162,13 +4170,16 @@ struct AsyncHandlerDesc {
41624170
auto Args = ArgList.ref();
41634171

41644172
if (Type == HandlerType::PARAMS) {
4165-
if (!HasError)
4166-
return HandlerResult(Args);
4167-
4168-
if (!isa<NilLiteralExpr>(Args.back()))
4173+
// If there's an error parameter and the user isn't passing nil to it,
4174+
// assume this is the error path.
4175+
if (HasError && !isa<NilLiteralExpr>(Args.back()))
41694176
return HandlerResult(Args.back(), true);
41704177

4171-
return HandlerResult(Args.drop_back());
4178+
// We can drop the args altogether if they're just Void.
4179+
if (willAsyncReturnVoid())
4180+
return HandlerResult();
4181+
4182+
return HandlerResult(HasError ? Args.drop_back() : Args);
41724183
} else if (Type == HandlerType::RESULT) {
41734184
if (Args.size() != 1)
41744185
return HandlerResult(Args);
@@ -4187,12 +4198,62 @@ struct AsyncHandlerDesc {
41874198
return HandlerResult(Args);
41884199

41894200
auto ResultArgList = callArgs(ResultCE);
4190-
return HandlerResult(ResultArgList.ref()[0],
4191-
D->getNameStr() == StringRef("failure"));
4201+
auto isFailure = D->getNameStr() == StringRef("failure");
4202+
4203+
// We can drop the arg altogether if it's just Void.
4204+
if (!isFailure && willAsyncReturnVoid())
4205+
return HandlerResult();
4206+
4207+
// Otherwise the arg gets the .success() or .failure() call dropped.
4208+
return HandlerResult(ResultArgList.ref()[0], isFailure);
41924209
}
41934210

41944211
llvm_unreachable("Unhandled result type");
41954212
}
4213+
4214+
// Convert the type of a success parameter in the completion handler function
4215+
// to a return type suitable for an async function. If there is an error
4216+
// parameter present e.g (T?, Error?) -> Void, this unwraps a level of
4217+
// optionality from T?. If this is a Result<T, U> type, returns the success
4218+
// type T.
4219+
swift::Type getSuccessParamAsyncReturnType(swift::Type Ty) const {
4220+
switch (Type) {
4221+
case HandlerType::PARAMS: {
4222+
// If there's an Error parameter in the handler, the success branch can
4223+
// be unwrapped.
4224+
if (HasError)
4225+
Ty = Ty->lookThroughSingleOptionalType();
4226+
4227+
return Ty;
4228+
}
4229+
case HandlerType::RESULT: {
4230+
// Result<T, U> maps to T.
4231+
return Ty->castTo<BoundGenericType>()->getGenericArgs()[0];
4232+
}
4233+
case HandlerType::INVALID:
4234+
llvm_unreachable("Invalid handler type");
4235+
}
4236+
}
4237+
4238+
/// Gets the return value types for the async equivalent of this handler.
4239+
ArrayRef<swift::Type>
4240+
getAsyncReturnTypes(SmallVectorImpl<swift::Type> &Scratch) const {
4241+
for (auto &Param : getSuccessParams()) {
4242+
auto Ty = Param.getParameterType();
4243+
Scratch.push_back(getSuccessParamAsyncReturnType(Ty));
4244+
}
4245+
return Scratch;
4246+
}
4247+
4248+
/// Whether the async equivalent of this handler returns Void.
4249+
bool willAsyncReturnVoid() const {
4250+
// If all of the success params will be converted to Void return types,
4251+
// this will be a Void async function.
4252+
return llvm::all_of(getSuccessParams(), [&](auto &param) {
4253+
auto Ty = param.getParameterType();
4254+
return getSuccessParamAsyncReturnType(Ty)->isVoid();
4255+
});
4256+
}
41964257
};
41974258

41984259
enum class ConditionType { INVALID, NIL, NOT_NIL };
@@ -4911,42 +4972,24 @@ class AsyncConverter : private SourceEntityWalker {
49114972
return;
49124973
}
49134974

4914-
auto HandlerParams = TopHandler.params();
4915-
if (TopHandler.Type == HandlerType::PARAMS && TopHandler.HasError) {
4916-
HandlerParams = HandlerParams.drop_back();
4917-
}
4918-
4919-
if (HandlerParams.empty()) {
4975+
SmallVector<Type, 2> Scratch;
4976+
auto ReturnTypes = TopHandler.getAsyncReturnTypes(Scratch);
4977+
if (ReturnTypes.empty()) {
49204978
OS << " ";
49214979
return;
49224980
}
49234981

4924-
OS << " -> ";
4982+
// Print the function result type, making sure to omit a '-> Void' return.
4983+
if (!TopHandler.willAsyncReturnVoid()) {
4984+
OS << " -> ";
4985+
if (ReturnTypes.size() > 1)
4986+
OS << "(";
49254987

4926-
if (HandlerParams.size() > 1) {
4927-
OS << "(";
4928-
}
4929-
for (size_t I = 0, E = HandlerParams.size(); I < E; ++I) {
4930-
if (I > 0) {
4931-
OS << ", ";
4932-
}
4988+
llvm::interleave(
4989+
ReturnTypes, [&](Type Ty) { Ty->print(OS); }, [&]() { OS << ", "; });
49334990

4934-
auto &Param = HandlerParams[I];
4935-
if (TopHandler.Type == HandlerType::PARAMS) {
4936-
Type ToPrint = Param.getPlainType();
4937-
if (TopHandler.HasError)
4938-
ToPrint = ToPrint->lookThroughSingleOptionalType();
4939-
ToPrint->print(OS);
4940-
} else if (TopHandler.Type == HandlerType::RESULT) {
4941-
auto ResultTy = Param.getPlainType()->getAs<BoundGenericType>();
4942-
assert(ResultTy && "Result must have generic type");
4943-
ResultTy->getGenericArgs()[0]->print(OS);
4944-
} else {
4945-
llvm_unreachable("Unhandled handler type");
4946-
}
4947-
}
4948-
if (HandlerParams.size() > 1) {
4949-
OS << ")";
4991+
if (ReturnTypes.size() > 1)
4992+
OS << ")";
49504993
}
49514994

49524995
if (FD->hasBody())
@@ -5073,8 +5116,7 @@ class AsyncConverter : private SourceEntityWalker {
50735116
addFallbackVars(CallbackParams->getArray(), Blocks);
50745117
addDo();
50755118
addAwaitCall(CE, ArgList.ref(), Blocks.SuccessBlock, SuccessParams,
5076-
/*HasError=*/HandlerDesc.HasError,
5077-
/*AddDeclarations=*/!HandlerDesc.HasError);
5119+
HandlerDesc, /*AddDeclarations=*/!HandlerDesc.HasError);
50785120
addFallbackCatch(ErrParam);
50795121
OS << "\n";
50805122
convertNodes(CallbackBody);
@@ -5107,10 +5149,9 @@ class AsyncConverter : private SourceEntityWalker {
51075149

51085150
setNames(Blocks.SuccessBlock, SuccessParams);
51095151
addAwaitCall(CE, ArgList.ref(), Blocks.SuccessBlock, SuccessParams,
5110-
/*HasError=*/HandlerDesc.HasError,
5111-
/*AddDeclarations=*/true);
5152+
HandlerDesc, /*AddDeclarations=*/true);
51125153

5113-
prepareNamesForBody(HandlerDesc.Type, SuccessParams, ErrParams);
5154+
prepareNamesForBody(HandlerDesc, SuccessParams, ErrParams);
51145155
convertNodes(Blocks.SuccessBlock.nodes());
51155156

51165157
if (RequireDo) {
@@ -5120,7 +5161,7 @@ class AsyncConverter : private SourceEntityWalker {
51205161
HandlerDesc.Type != HandlerType::RESULT);
51215162
addCatch(ErrParam);
51225163

5123-
prepareNamesForBody(HandlerDesc.Type, ErrParams, SuccessParams);
5164+
prepareNamesForBody(HandlerDesc, ErrParams, SuccessParams);
51245165
addCatchBody(ErrParam, Blocks.ErrorBlock);
51255166
}
51265167

@@ -5129,9 +5170,11 @@ class AsyncConverter : private SourceEntityWalker {
51295170

51305171
void addAwaitCall(const CallExpr *CE, ArrayRef<Expr *> Args,
51315172
const ClassifiedBlock &SuccessBlock,
5132-
ArrayRef<const ParamDecl *> SuccessParams, bool HasError,
5133-
bool AddDeclarations) {
5134-
if (!SuccessParams.empty()) {
5173+
ArrayRef<const ParamDecl *> SuccessParams,
5174+
const AsyncHandlerDesc &HandlerDesc, bool AddDeclarations) {
5175+
// Print the bindings to match the completion handler success parameters,
5176+
// making sure to omit in the case of a Void return.
5177+
if (!SuccessParams.empty() && !HandlerDesc.willAsyncReturnVoid()) {
51355178
if (AddDeclarations) {
51365179
if (SuccessBlock.allLet()) {
51375180
OS << tok::kw_let;
@@ -5153,7 +5196,7 @@ class AsyncConverter : private SourceEntityWalker {
51535196
OS << " " << tok::equal << " ";
51545197
}
51555198

5156-
if (HasError) {
5199+
if (HandlerDesc.HasError) {
51575200
OS << tok::kw_try << " ";
51585201
}
51595202
OS << "await ";
@@ -5199,16 +5242,21 @@ class AsyncConverter : private SourceEntityWalker {
51995242
OS << "\n" << tok::r_brace;
52005243
}
52015244

5202-
void prepareNamesForBody(HandlerType ResultType,
5245+
void prepareNamesForBody(const AsyncHandlerDesc &HandlerDesc,
52035246
ArrayRef<const ParamDecl *> CurrentParams,
52045247
ArrayRef<const ParamDecl *> OtherParams) {
5205-
switch (ResultType) {
5248+
switch (HandlerDesc.Type) {
52065249
case HandlerType::PARAMS:
52075250
for (auto *Param : CurrentParams) {
5208-
if (Param->getType()->getOptionalObjectType()) {
5251+
auto Ty = Param->getType();
5252+
if (Ty->getOptionalObjectType()) {
52095253
Unwraps.insert(Param);
52105254
Placeholders.insert(Param);
52115255
}
5256+
// Void parameters get omitted where possible, so turn any reference
5257+
// into a placeholder, as its usage is unlikely what the user wants.
5258+
if (HandlerDesc.getSuccessParamAsyncReturnType(Ty)->isVoid())
5259+
Placeholders.insert(Param);
52125260
}
52135261
// Use of the other params is invalid within the current body
52145262
Placeholders.insert(OtherParams.begin(), OtherParams.end());

‎test/refactoring/ConvertAsync/basic.swift

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -190,6 +190,18 @@ func blockConvention(completion: @convention(block) () -> Void) { }
190190
func cConvention(completion: @convention(c) () -> Void) { }
191191
// C-CONVENTION: func cConvention() async { }
192192

193+
// RUN: %refactor -add-async-alternative -dump-text -source-filename %s -pos=%(line+1):1 | %FileCheck -check-prefix VOID-HANDLER %s
194+
func voidCompletion(completion: (Void) -> Void) {}
195+
// VOID-HANDLER: func voidCompletion() async {}
196+
197+
// RUN: %refactor -add-async-alternative -dump-text -source-filename %s -pos=%(line+1):1 | %FileCheck -check-prefix VOID-AND-ERROR-HANDLER %s
198+
func voidAndErrorCompletion(completion: (Void?, Error?) -> Void) {}
199+
// VOID-AND-ERROR-HANDLER: func voidAndErrorCompletion() async throws {}
200+
201+
// RUN: %refactor -add-async-alternative -dump-text -source-filename %s -pos=%(line+1):1 | %FileCheck -check-prefix TOO-MUCH-VOID-AND-ERROR-HANDLER %s
202+
func tooMuchVoidAndErrorCompletion(completion: (Void?, Void?, Error?) -> Void) {}
203+
// TOO-MUCH-VOID-AND-ERROR-HANDLER: func tooMuchVoidAndErrorCompletion() async throws {}
204+
193205
// 2. Check that the various ways to call a function (and the positions the
194206
// refactoring is called from) are handled correctly
195207

@@ -348,5 +360,26 @@ func testCalls() {
348360
}
349361
// C-CONVENTION-CALL: await cConvention(){{$}}
350362
// C-CONVENTION-CALL-NEXT: {{^}}print("cConvention")
363+
364+
// RUN: %refactor -convert-call-to-async-alternative -dump-text -source-filename %s -pos=%(line+1):3 | %FileCheck -check-prefix=VOID-AND-ERROR-CALL %s
365+
voidAndErrorCompletion { v, err in
366+
print("void and error completion \(v)")
367+
}
368+
// VOID-AND-ERROR-CALL: {{^}}try await voidAndErrorCompletion(){{$}}
369+
// VOID-AND-ERROR-CALL: {{^}}print("void and error completion \(<#v#>)"){{$}}
370+
371+
// RUN: %refactor -convert-call-to-async-alternative -dump-text -source-filename %s -pos=%(line+1):3 | %FileCheck -check-prefix=VOID-AND-ERROR-CALL2 %s
372+
voidAndErrorCompletion { _, err in
373+
print("void and error completion 2")
374+
}
375+
// VOID-AND-ERROR-CALL2: {{^}}try await voidAndErrorCompletion(){{$}}
376+
// VOID-AND-ERROR-CALL2: {{^}}print("void and error completion 2"){{$}}
377+
378+
// RUN: %refactor -convert-call-to-async-alternative -dump-text -source-filename %s -pos=%(line+1):3 | %FileCheck -check-prefix=VOID-AND-ERROR-CALL3 %s
379+
tooMuchVoidAndErrorCompletion { v, v1, err in
380+
print("void and error completion 3")
381+
}
382+
// VOID-AND-ERROR-CALL3: {{^}}try await tooMuchVoidAndErrorCompletion(){{$}}
383+
// VOID-AND-ERROR-CALL3: {{^}}print("void and error completion 3"){{$}}
351384
}
352385
// CONVERT-FUNC: {{^}}}

‎test/refactoring/ConvertAsync/convert_function.swift

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -148,3 +148,51 @@ func asyncUnhandledCompletion(_ completion: (String) -> Void) {
148148
// ASYNC-UNHANDLED-NEXT: return "bad"
149149
// ASYNC-UNHANDLED-NEXT: }
150150
// ASYNC-UNHANDLED-NEXT: }
151+
152+
// RUN: %refactor -add-async-alternative -dump-text -source-filename %s -pos=%(line+1):1 | %FileCheck -check-prefix VOID-AND-ERROR-HANDLER %s
153+
func voidAndErrorCompletion(completion: (Void?, Error?) -> Void) {
154+
if .random() {
155+
completion((), nil) // Make sure we drop the ()
156+
} else {
157+
completion(nil, CustomError.Bad)
158+
}
159+
}
160+
// VOID-AND-ERROR-HANDLER: func voidAndErrorCompletion() async throws {
161+
// VOID-AND-ERROR-HANDLER-NEXT: if .random() {
162+
// VOID-AND-ERROR-HANDLER-NEXT: return // Make sure we drop the ()
163+
// VOID-AND-ERROR-HANDLER-NEXT: } else {
164+
// VOID-AND-ERROR-HANDLER-NEXT: throw CustomError.Bad
165+
// VOID-AND-ERROR-HANDLER-NEXT: }
166+
// VOID-AND-ERROR-HANDLER-NEXT: }
167+
168+
// RUN: %refactor -add-async-alternative -dump-text -source-filename %s -pos=%(line+1):1 | %FileCheck -check-prefix TOO-MUCH-VOID-AND-ERROR-HANDLER %s
169+
func tooMuchVoidAndErrorCompletion(completion: (Void?, Void?, Error?) -> Void) {
170+
if .random() {
171+
completion((), (), nil) // Make sure we drop the ()s
172+
} else {
173+
completion(nil, nil, CustomError.Bad)
174+
}
175+
}
176+
// TOO-MUCH-VOID-AND-ERROR-HANDLER: func tooMuchVoidAndErrorCompletion() async throws {
177+
// TOO-MUCH-VOID-AND-ERROR-HANDLER-NEXT: if .random() {
178+
// TOO-MUCH-VOID-AND-ERROR-HANDLER-NEXT: return // Make sure we drop the ()s
179+
// TOO-MUCH-VOID-AND-ERROR-HANDLER-NEXT: } else {
180+
// TOO-MUCH-VOID-AND-ERROR-HANDLER-NEXT: throw CustomError.Bad
181+
// TOO-MUCH-VOID-AND-ERROR-HANDLER-NEXT: }
182+
// TOO-MUCH-VOID-AND-ERROR-HANDLER-NEXT: }
183+
184+
// RUN: %refactor -add-async-alternative -dump-text -source-filename %s -pos=%(line+1):1 | %FileCheck -check-prefix VOID-RESULT-HANDLER %s
185+
func voidResultCompletion(completion: (Result<Void, Error>) -> Void) {
186+
if .random() {
187+
completion(.success(())) // Make sure we drop the .success(())
188+
} else {
189+
completion(.failure(CustomError.Bad))
190+
}
191+
}
192+
// VOID-RESULT-HANDLER: func voidResultCompletion() async throws {
193+
// VOID-RESULT-HANDLER-NEXT: if .random() {
194+
// VOID-RESULT-HANDLER-NEXT: return // Make sure we drop the .success(())
195+
// VOID-RESULT-HANDLER-NEXT: } else {
196+
// VOID-RESULT-HANDLER-NEXT: throw CustomError.Bad
197+
// VOID-RESULT-HANDLER-NEXT: }
198+
// VOID-RESULT-HANDLER-NEXT: }

‎test/refactoring/ConvertAsync/convert_result.swift

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,14 @@ func simple(_ completion: (Result<String, Error>) -> Void) { }
22
func noError(_ completion: (Result<String, Never>) -> Void) { }
33
func test(_ str: String) -> Bool { return false }
44

5+
// RUN: %refactor -add-async-alternative -dump-text -source-filename %s -pos=%(line+1):1 | %FileCheck -check-prefix VOID-RESULT %s
6+
func voidResult(completion: (Result<Void, Never>) -> Void) {}
7+
// VOID-RESULT: func voidResult() async {}
8+
9+
// RUN: %refactor -add-async-alternative -dump-text -source-filename %s -pos=%(line+1):1 | %FileCheck -check-prefix VOID-AND-ERROR-RESULT %s
10+
func voidAndErrorResult(completion: (Result<Void, Error>) -> Void) {}
11+
// VOID-AND-ERROR-RESULT: func voidAndErrorResult() async throws {}
12+
513
// RUN: %refactor -convert-call-to-async-alternative -dump-text -source-filename %s -pos=%(line+1):1 | %FileCheck -check-prefix=SIMPLE %s
614
simple { res in
715
print("result \(res)")
@@ -301,3 +309,16 @@ simple { res in
301309
// NESTEDBREAK-NEXT: print("after")
302310
// NESTEDBREAK-NOT: }
303311

312+
// RUN: %refactor -convert-call-to-async-alternative -dump-text -source-filename %s -pos=%(line+1):1 | %FileCheck -check-prefix=VOID-RESULT-CALL %s
313+
voidResult { res in
314+
print(res)
315+
}
316+
// VOID-RESULT-CALL: {{^}}await voidResult()
317+
// VOID-RESULT-CALL: {{^}}print(<#res#>)
318+
319+
// RUN: %refactor -convert-call-to-async-alternative -dump-text -source-filename %s -pos=%(line+1):1 | %FileCheck -check-prefix=VOID-AND-ERROR-RESULT-CALL %s
320+
voidAndErrorResult { res in
321+
print(res)
322+
}
323+
// VOID-AND-ERROR-RESULT-CALL: {{^}}try await voidAndErrorResult()
324+
// VOID-AND-ERROR-RESULT-CALL: {{^}}print(<#res#>)

0 commit comments

Comments
 (0)