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

Improve removal of parentheses around expressions #609

Open
stopdropandrew opened this issue Oct 26, 2022 · 3 comments · Fixed by #610
Open

Improve removal of parentheses around expressions #609

stopdropandrew opened this issue Oct 26, 2022 · 3 comments · Fixed by #610
Labels
enhancement New feature or request

Comments

@stopdropandrew
Copy link
Contributor

stopdropandrew commented Oct 26, 2022

Currently stylua removes many redundant parens, and in one specific case it's triggering a luau analysis warning.

Ex:

local _ = (not true) == true

Changes to:

local _ = not true == true

This triggers the linter:
ComparisonPrecedence: not X == Y is equivalent to (not X) == Y; consider using X ~= Y, or add parentheses to silence

I noticed that a number of cases where redundant parens aren't removed, maybe this use case should be added? Should Stylua add parens to clarify the operator precedence?

Here are some cases I noticed where parens aren't removed:

local _ = true and (false or true)
local _ = (true and false or true)
local _ = (true and false) or true
local _ = (not true == false)

Another example from Prettier, it adds parens to show operator precedence between and/or:

(true && false) || true

Interested to hear your thoughts!

@JohnnyMorganz
Copy link
Owner

JohnnyMorganz commented Oct 26, 2022

The first example is definitely a bug as it is changing the meaning of the code Actually no it doesn't, but it should still keep the parens (or even add it in like you mention).

it adds parens to show operator precedence between and/or:

Yup, this is something I've always wanted to do. IIRC, The main reason I didn't do it yet is because of the incredibly common and-or idiom which would make this annoying (it's superseded by if expressions but that is just Luau only)

Looks like parentheses removal could be improved in some of the cases you've shown - I'd probably say it's a separate enhancement compared to the initial bug, but I'll take a look at this, thanks!

@JohnnyMorganz
Copy link
Owner

The bug where the parentheses highlighting precedence were removed has been fixed

I do want to improve the parentheses removal, and start adding in new parentheses too.

The reason right now that parentheses are not being removed in the samples you showed is because the internal expressions are binary expressions. For simplicity, I don't touch them right now, but I do want to improve this. Will convert this issue into a feature request about this

@JohnnyMorganz JohnnyMorganz reopened this Oct 27, 2022
@JohnnyMorganz JohnnyMorganz changed the title Removing parens triggers luau linter warning Improve removal of parentheses around expressions Oct 27, 2022
@JohnnyMorganz JohnnyMorganz added enhancement New feature or request and removed bug Something isn't working labels Oct 27, 2022
@stopdropandrew
Copy link
Contributor Author

Thanks so much!

@JohnnyMorganz JohnnyMorganz added this to the 0.16 milestone Dec 7, 2022
@JohnnyMorganz JohnnyMorganz removed this from the 0.16 milestone Jan 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants