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

Add required parentheses around method receiver #12851

Merged
merged 2 commits into from
Jun 7, 2024

Conversation

samueltardieu
Copy link
Contributor

Fix #12846

changelog: [needless_bool]: Add missing parentheses around method receiver

@rustbot
Copy link
Collaborator

rustbot commented May 25, 2024

r? @y21

rustbot has assigned @y21.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label May 25, 2024
Comment on lines 164 to 161
if condition_needs_parentheses(cond) && is_parent_stmt(cx, e.hir_id) {
if (condition_needs_parentheses(cond) && is_parent_stmt(cx, e.hir_id))
|| is_receiver_of_method_call(cx, e)
{
snip = snip.maybe_par();
Copy link
Member

Choose a reason for hiding this comment

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

Most lints just unconditionally call maybe_par since that can already figure out on its own if it needs parentheses, I wonder why we aren't just doing this here? Would this otherwise add too many unnecessary parens?

FWIW, method call receivers are not the only thing where we'd need parentheses. && has very low precedence so they are needed for almost any parent expression (e.g. any binary expression other than ||).

let (a, b) = Default::default();

_ = if a && b { true } else { false } as u8; // should also suggest `(a && b) as u8`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most lints just unconditionally call maybe_par since that can already figure out on its own if it needs parentheses, I wonder why we aren't just doing this here? Would this otherwise add too many unnecessary parens?

Yup.

FWIW, method call receivers are not the only thing where we'd need parentheses. && has very low precedence so they are needed for almost any parent expression (e.g. any binary expression other than ||).

let (a, b) = Default::default();

_ = if a && b { true } else { false } as u8; // should also suggest `(a && b) as u8`

I'll check this one as well, I'll put the PR in WIP mode in the meantime.

@bors
Copy link
Collaborator

bors commented May 27, 2024

☔ The latest upstream changes (presumably #12842) made this pull request unmergeable. Please resolve the merge conflicts.

@samueltardieu samueltardieu marked this pull request as draft June 1, 2024 14:29
@samueltardieu samueltardieu marked this pull request as ready for review June 6, 2024 18:54
@samueltardieu
Copy link
Contributor Author

@y21 I've handled the as case as well. It would be best of course to be able to have something that inserts parentheses automatically by checking the precedence of an expression vs. a context where this expression will be inserted. It would however require writing/reusing a parser.

@y21
Copy link
Member

y21 commented Jun 7, 2024

Ok, fair, thanks! @bors r+

@bors
Copy link
Collaborator

bors commented Jun 7, 2024

📌 Commit 35b2aa9 has been approved by y21

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Jun 7, 2024

⌛ Testing commit 35b2aa9 with merge d553ebe...

@bors
Copy link
Collaborator

bors commented Jun 7, 2024

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: y21
Pushing d553ebe to master...

@bors bors merged commit d553ebe into rust-lang:master Jun 7, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[needless_bool] suggestion cause error when the if expression is a method caller
4 participants