-
Notifications
You must be signed in to change notification settings - Fork 488
Add RemoveRedundantParentheses refactoring #3232
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
base: main
Are you sure you want to change the base?
Add RemoveRedundantParentheses refactoring #3232
Conversation
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.
Thanks for implementing this. This is a great start, the question is how far you want to take this? Should this build on top of SwiftOperators to eg. simplify 1 + (2 * 3) to 1 + 2 * 3? And could we also remove unnecessary parentheses in let x = (1 + 2)?
| private static func getSingleUnlabeledElement(from tuple: TupleExprSyntax) -> LabeledExprSyntax? { | ||
| guard tuple.elements.count == 1, | ||
| let element = tuple.elements.first, | ||
| element.label == nil | ||
| else { | ||
| return nil | ||
| } | ||
| return element | ||
| } |
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.
We should already have this as tuple.elemetns.singleUnlabledExpression or tuple.isParentheses in SwiftIfConfig. Maybe it makes sense to pull this up to Convenience.swift in the SwiftSyntax module, for now at the package access level.
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 have made it public for now since bazel build breaks due to some reason when i make it package level.(I'll probably have to read up on how bazel and build config works to figure out why it breaks.)
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.
Oh, seems like we don’t have any package access level declarations right now. Let’s use @_spi(RawSyntax) public for now. I don’t want to add this as part of the public API just yet in an effort to keep the API surface small.
@keith Do we need to set the package name in the Bazel rules so we can use the package access modifier? If so, could you add it?
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.
yes we just have to set package_name for w/e targets to w/e string and that will be passed through to -package-name. don't block on bazel to merge this in the preferred way and I can fix that up async
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.
Great. Thank you @keith!
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.
Looks like the CMake build doesn’t support the package access level yet either, let’s use @_spi(RawSyntax) public for now.
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.
Just gave this a little more of an in-depth review and found some more edge cases we need to handle. Given that this references swiftlang/sourcekit-lsp#2451 as the issue and that contains let x = (a + b), we should also at least handle that case, even if we don’t go into full operator-parsing just yet.
696112d to
700f876
Compare
|
I've added more through tests and a assertParenRemoval helper since the old method would have required a lot of ast parsing and was quite verbose |
844c5be to
2c62f8a
Compare
|
rebased to main and make singleUnlabeledExpression package level |
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.
A few more comments, yet again. This really is more tricky than it looks like on the surface, even without operator handling.
f7aa51d to
c8ef85d
Compare
|
yep, it does seem like there are a lot of edge cases that we can potentially run into with this pr, also a heads up I added support for removing redundant parentheses in control flow and return/throw statements(it was non trivial and felt weird not having something so basic) |
c8ef85d to
7c18041
Compare
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.
Sorry for the delayed review. I looked through this again and found some more cases that we weren’t handling correctly.
1f622e5 to
a078659
Compare
Signed-off-by: Karan <[email protected]>
Signed-off-by: Karan <[email protected]>
Signed-off-by: Karan <[email protected]>
Signed-off-by: Karan <[email protected]>
Signed-off-by: Karan <[email protected]>
Signed-off-by: Karan <[email protected]>
a078659 to
d585e82
Compare
|
getting a second look at this after some time, this is trickier than it sounds. |
Relevant issue: swiftlang/sourcekit-lsp#2451
The scope of possible edge cases here is quite large, I might be missing some but I have tried to cover as many as I could come up with.