Remove redundant parentheses code action

[PhantomInTheWire]

Description

I Propose adding a code action to remove redundant parentheses in Swift expressions.

Originally discussed here: https://github.com/swiftlang/sourcekit-lsp/pull/2406#discussion_r2658894049

Although it was originally discussed as a improvement to the existing demorgan implementation. I figured this was something that could be more generalized.

The action would detect parentheses that do not affect meaning and safely remove them, improving readability while preserving operator precedence and evaluation order.

Example:

Before:

let x = (a + b)
let y = ((foo()))

After:

let x = a + b
let y = foo()

Open question: should the core detection logic live in swift-syntax so it can be reused by other tools(formatters or linters maybe), or should this be implemented locally in SourceKit-LSP?

If so i think this can be broken down into a couple PRs.

1. Implement the core upstream in swift-syntax
2. Use that in demorgan implemenation in sourcekit-lsp
3. Make a full code action for it in sourcekit-lsp

[ahoppen]

Open question: should the core detection logic live in swift-syntax so it can be reused by other tools

I think we haven’t been too good about it but refactoring actions should generally be implemented in swift-syntax’s SwiftRefactor module so other tools could also use them. And as you said, this seems like a fundamental building block for other refracting actions (or even macros), so it would be good to have it in swift-syntax.

[vanshkamra12]

Hi! I'm interested in working on this issue. I've been exploring the SourceKit-LSP codebase and plan to follow the same SyntaxCodeActionProvider pattern used in ApplyDeMorganLaw. Would it be okay if I start implementing this locally in SourceKit-LSP first (without the swift-syntax step)? Happy to open a draft PR once I have the basic cases working.

[ahoppen]

Thanks for your interest @vanshkamra12. @PhantomInTheWire has already been working on this in https://github.com/swiftlang/swift-syntax/pull/3232 and https://github.com/swiftlang/sourcekit-lsp/pull/2491.

One part that’s still open on this issue is to remove parentheses that are redundant based on operator precedence rules, eg. removing the parentheses in 1 + (2 * 3). @PhantomInTheWire would you like to tackle that next? If not, @vanshkamra12 could take a look at that part but @PhantomInTheWire, you would have precedence because you’ve already done a lot of work on this issue.

[PhantomInTheWire]

@vanshkamra12 feel free to give this a shot, and ping me here if you need any context from my side.

I’m happy to help with code review once you have something ready.

[plemarquand]

@ahoppen (foo()) is obviously redundant but 1 + (2 * 3) I find useful so I don't have to stop and think about precedence rules. Restating operator precedence may be redundant to the compiler but I think they convey some useful information to the reader.

All that to say I think there may be some nuance here where we might not want to strip every possible redundant set of parens as it may make the code harder to read.

[ahoppen]

I would argue that if you invoke the Remove redundant parentheses refactoring on an expression, you acknowledge that redundant parentheses will be removed even if you think they are valuable for readability. If you want to keep them, you shouldn’t invoke the refactoring. But happy to be convinced either way. @PhantomInTheWire do you have thoughts here? You spent the most time thinking about this refactoring so far.

[PhantomInTheWire]

I agree with @ahoppen here, why would someone call the action if they don't want their valuable parentheses stripped?

that said a more flexible approach(and likely what @plemarquand meant was) that it would be easier to let the user decide how aggressively the refactoring should behave.

For example, given (1 + (2 * 3)), someone might prefer to keep (2 * 3) for readability while removing just the outer parentheses. In that case, the result would be:

1 + (2 * 3)

if the user opts for aggressive removal based on precedence rules, the result would be:

1 + 2 * 3

Although I imagine this makes the suggestions more complicated, add a lot more nuance to how it should behave in many cases and adds some work for @vanshkamra12 (I imagine he plans to work on this part as well).

[ahoppen]

I think having two kinds of code actions to remove parentheses with different aggessivity levels would likely be very confusing to users and trying to guess which parentheses are useful for readability and which are not is subjective, so I’d prefer to not go along that path. I think I stand by my opinion that this action should really remove as many parentheses as possible and if the user doesn’t want that, they shouldn’t invoke this refactoring.

[PhantomInTheWire]

@ahoppen yeah you're right, the complexity probably isn't worth it