Skip to content
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

Fix fatal error caused by switch cases without any statements (#473) #499

Merged
merged 1 commit into from
Apr 10, 2023

Conversation

stackotter
Copy link
Contributor

The issue can be reproduced on main by formatting the following snippet:

switch x {
case y:
default:
  print("hi")
}

The code isn't valid Swift of course, however it shouldn't cause a fatal error, and in my opinion it shouldn't prevent formatting of the code (because intent is clear). However, I'm not sure of the official position of this project on how invalid code should be handled.

Note that adding a comment in the y case doesn't effect whether a fatal error occurs.

I have added a test that covers formatting of empty cases and after my relatively minimal (and commented) changes it no longer crashes. The issue occurred because in the situation where a case is empty, the opening break and open token are added after the same syntax token as the closing break and close token. These two sets of tokens are added one after another, and because of the implementation of after, the two sets end up getting swapped when constructing the token stream. Thus, the closing break and close token end up preceding the opening break and open token causing the fatal error.

@stackotter
Copy link
Contributor Author

@allevato Would you be able to review this PR and give feedback if necessary? It’s a relatively small and safe PR. I’m not sure if you’re the right person to ping for a review, but you seem to be the most active maintainer (please direct me to the correct person if I’m wrong).

If there are any other steps I need to follow before getting the PR reviewed, please let me know.

Copy link
Member

@allevato allevato left a comment

Choose a reason for hiding this comment

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

Thanks for catching this; I agree that we don't want the formatter crashing under any circumstances (especially for code that parses correctly but is rejected later during compilation).

Will kick off CI on the companion PR in swift-syntax after my other requests are done processing and merge when it's ready.

@stackotter
Copy link
Contributor Author

Thanks, out of interest what do you mean by the companion pr in swift-syntax?

@allevato
Copy link
Member

allevato commented Apr 7, 2023

Thanks, out of interest what do you mean by the companion pr in swift-syntax?

We don't have swift-ci directly set up on swift-format yet, but I can kick it off indirectly by linking to this PR from this dummy PR that uses the trigger.

I don't want to dirty up swift-syntax with multiple dummy PRs though, so I need to wait for each one to finish and report the results before starting the next.

@allevato allevato merged commit b5b62af into swiftlang:main Apr 10, 2023
allevato added a commit to allevato/swift-format that referenced this pull request Jun 29, 2023
Fix fatal error caused by switch cases without any statements (swiftlang#473)
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.

None yet

2 participants