Skip to content

[Refactoring] Fix invalid legacy body for Result<Void, ...> handler #38855

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Aug 12, 2021

Conversation

bnbarham
Copy link
Contributor

When adding an async alternative for a function with a (Result<Void, ...>) -> Void completion handler, the legacy body was adding a call to
the handler with .success(result). We don't assign to result for
Void handlers, so pass () instead.

Resolves rdar://81829392

@bnbarham
Copy link
Contributor Author

@swift-ci please test

Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We really should try and only use refactor-check-compiles tests, which would have caught the issue…

@bnbarham
Copy link
Contributor Author

We really should try and only use refactor-check-compiles tests, which would have caught the issue…

Good point, I didn't even notice that we weren't. I assume the majority of these were added before it existed? I'll convert these now in a separate commit (and put the unrelated _ change with it).

Copy link
Contributor

@hamishknight hamishknight left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

@bnbarham bnbarham force-pushed the async-refactoring-void-result branch 2 times, most recently from e3f99c5 to 3ab7862 Compare August 12, 2021 10:57
When adding an async alternative for a function with a
`(Result<Void, ...>) -> Void` completion handler, the legacy body was
adding a call to the handler with `.success(result)`. We don't assign to
`result` for `Void` handlers, so pass `()` instead.

Resolves rdar://81829392
@bnbarham bnbarham force-pushed the async-refactoring-void-result branch from 3ab7862 to 46a4b24 Compare August 12, 2021 11:04
@bnbarham
Copy link
Contributor Author

bnbarham commented Aug 12, 2021

Updated all the tests in convert_function.swift to compile as well. Changed them all to -convert-to-async since none are checking the legacy body.

Also noticed that we already had the Task { ... } checks in basic.swift, just none for the Result case so I added that one and removed the Task { ... } from convert_function.swift. Also already had the functionWithSomeHandler test.

Update the async refactoring tests in `convert_function.swift` to
compile the refactored code (where possible) to check for errors. This
would have prevented the issue with the refactored `Result<Void, ...`>
handler producing erroneous code.
@bnbarham bnbarham force-pushed the async-refactoring-void-result branch from 46a4b24 to 9fc758a Compare August 12, 2021 11:13
@bnbarham
Copy link
Contributor Author

@swift-ci please test

@bnbarham bnbarham merged commit 061a8f4 into main Aug 12, 2021
@bnbarham bnbarham deleted the async-refactoring-void-result branch August 12, 2021 22:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants