Skip to content

[Refactoring] A couple of fixes for async return handling #37411

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
May 14, 2021
Merged

[Refactoring] A couple of fixes for async return handling #37411

merged 5 commits into from
May 14, 2021

Conversation

hamishknight
Copy link
Contributor

  • Fix placeholder handling so we don't crash while re-writing a return statement with a return expression
  • Avoid duplicating return statements when converting a completion handler call into a return statement
  • Don't drop return statements at the end of a block if they have an expression

rdar://77789360

@hamishknight hamishknight requested a review from bnbarham May 13, 2021 20:21
bnbarham
bnbarham approved these changes May 13, 2021
View reviewed changes
@hamishknight
[Refactoring] Tweak handling of return and break placeholders
d7d58cb 
- Add a missing return to the break statement
placeholder handling.

- Only turn the `return` token into a placeholder,
as we still want to apply the transformation to
the sub expression.

This stops us from crashing by attempting to walk
into the return sub-expression.

rdar://77789360
@hamishknight
[IDE] Allow SourceEntityWalker to keep track of its ASTWalker
854a8bc 
This will allow it to be queried for things like
the parent ASTNode in the current traversal.
@hamishknight
[Refactoring] Avoid duplicating return statements
aa189f2 
It's possible the user has already written an
explicit return for the call to the completion
handler. In that case, avoid adding another return.

rdar://77789360
@hamishknight
[Refactoring] Don't drop returns with expressions
7ab5915 
Previously we were unconditionally dropping a
return statement if it was the last node, which
could cause us to inadvertently drop the result
expression as well. Instead, only drop bare
'return' statements.

rdar://77789360
@hamishknight
[test] Add test for async completion handler call in parens
87703a0 
We don't currently handle this, but arguably we
should be able to.
@hamishknight
Copy link
Contributor Author

@swift-ci please smoke test

@hamishknight hamishknight mentioned this pull request May 14, 2021
Merged
@bnbarham
Copy link
Contributor

Thanks for the extra test :)

@hamishknight hamishknight merged commit 5b2f40a into swiftlang:main May 14, 2021
@hamishknight hamishknight deleted the returning-so-soon branch May 14, 2021 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bnbarham bnbarham bnbarham approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hamishknight @bnbarham