-
Notifications
You must be signed in to change notification settings - Fork 10.6k
[Diagnostics] Add fix-it to @main struct without main static function.
#82989
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
[Diagnostics] Add fix-it to @main struct without main static function.
#82989
Conversation
AnthonyLatsis
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
Please add a test case with a nested @main struct to make sure we got the indents right.
|
@AnthonyLatsis Thanks for the review! All issues have been addressed based on your feedback. |
AnthonyLatsis
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more round. Are you using clang-format?
22a54ad to
285c484
Compare
@AnthonyLatsis Yes, I ran git-clang-format HEAD~2
git add .
git commit --amend --no-editAll other review feedback has been addressed as well. |
ahoppen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice. Looking forward to using this 🤩
lib/Sema/TypeCheckAttr.cpp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: Should we add two newlines here to separate the generated main function from the remaining body using a newline?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t have a clear preference. Ideally (not in this PR!), I imagine a formatCodeForInsertion function that parses a given string into a SwiftSyntax tree and formats it using SwiftBasicFormat (indentation, separation) for insertion at a given location.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way that Fix-Its and refactorings are designed to work is that the editor applies its own indentation after applying the Fix-It refactoring because it can really know about the user’s preferences (the compiler can only guess based on existing lines). But whether to add a newline is currently an opinionated decision by the compiler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the discussion.
I've looked at other fix-it instances in TypeCheckProtocol.cpp and TypeCheckDistributed.cpp, and found that they only add a single \n at the end of inserted code, without additional blank lines.
Considering this, and editors applying indentation based on user preferences, I'm inclined to stay consistent with existing practices and not insert extra blank lines.
Of course, if you have any other thoughts, I'm open to further discussion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for finding those examples. In that case let’s stick with the existing practice of only adding one newline. 👍🏽
|
@swift-ci please smoke test Linux |
Let me know if you need help running the tests locally. |
@AnthonyLatsis Thank you for the detailed test failure information. All issues you mentioned have been fixed. Additionally, all tests in test/attr/ApplicationMain/ have been run in my local before this update, addressing the previously overlooked cases. Please let me know if you spot anything else or have further suggestions. |
285c484 to
a35ff41
Compare
AnthonyLatsis
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Please tidy up your history before we go for merge.
P.S. You are welcome to add a test for if (hasAsyncSupport) { in a follow-up.
|
@swift-ci please smoke test Linux |
|
@AnthonyLatsis Hi, Just to confirm: by tidying up history, do you mean amending/combining commits? Thanks! |
In this case I mean squashing the "review feedback" commit into the original.
Try this (async is supported or can be deployed to platform versions aligned with 5.1 and onward): |
a35ff41 to
0d0dfcf
Compare
|
I've squashed the "review feedback" commit into the original as suggested. I've also added the test for Thanks! |
0d0dfcf to
061ce72
Compare
|
@swift-ci please smoke test macOS |
|
FYI if you build the project without |
|
@swift-ci please smoke test Linux |
|
@swift-ci please smoke test Windows |
Resolves #58518
It will create four different fix-its: