Skip to content

Conversation

lapla-cogito
Copy link
Contributor

@lapla-cogito lapla-cogito commented Jan 29, 2025

close #12157

needless_return sometimes makes incorrect suggestions by omitting necessary enclosing parentheses. This PR resolves the issue by using clippy_utils::sugg::Sugg.

changelog: [needless_return]: now makes correct suggestions which require enclosing parentheses

@rustbot
Copy link
Collaborator

rustbot commented Jan 29, 2025

r? @Alexendoo

rustbot has assigned @Alexendoo.
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 Jan 29, 2025
@lapla-cogito
Copy link
Contributor Author

Rebased.

@lapla-cogito
Copy link
Contributor Author

Rebased.

@lapla-cogito
Copy link
Contributor Author

@Alexendoo Should I do a reassign? (I pinged you because it's been over 2 weeks)

@lapla-cogito
Copy link
Contributor Author

r? clippy

@rustbot rustbot assigned blyxyas and unassigned Alexendoo Feb 19, 2025
@lapla-cogito
Copy link
Contributor Author

r? @Manishearth

@rustbot rustbot assigned Manishearth and unassigned blyxyas Feb 23, 2025
@Manishearth
Copy link
Member

r? @samueltardieu since he started reviewing

@rustbot rustbot assigned samueltardieu and unassigned Manishearth Feb 24, 2025
@lapla-cogito
Copy link
Contributor Author

Ah, I thought Samuel could approve but not have the privilege to merge.

@Manishearth
Copy link
Member

He's clippy-contributors, I can merge once he's done.

Copy link
Member

@samueltardieu samueltardieu left a comment

Choose a reason for hiding this comment

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

I am not sure this is the right fix, as I don't think that the problem lies in returns.rs. Moreover, fixing it this way removes an explanatory message "and wrap the sequence with parentheses".

I think that you would obtain a better result by fixing clippy_utils::binary_expr_needs_parentheses(). I have the impression that you can add a condition to the match of the inner contains_block() function to cover the problematic cases:

            ExprKind::Cast(lhs, _) => contains_block(lhs, true),

Doing so while letting return.rs untouched seems to pass your new tests just fine, while keeping the explanatory wrapping message when appropriate.

WDYT?

@lapla-cogito
Copy link
Contributor Author

lapla-cogito commented Mar 5, 2025

@samueltardieu Sorry for the late reply. Certainly removing NeedsPar would provide a bad description to users, so fixed it, thanks.

@samueltardieu
Copy link
Member

I can merge once he's done.

@Manishearth This now looks good to me, feel free to merge as you please.

@lapla-cogito
Copy link
Contributor Author

@Manishearth Could you take a look at this when you have some time?

Copy link
Member

@Alexendoo Alexendoo left a comment

Choose a reason for hiding this comment

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

👍

@Alexendoo Alexendoo added this pull request to the merge queue Mar 16, 2025
Merged via the queue into rust-lang:master with commit 775a5c2 Mar 16, 2025
11 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.

Autofix for needless_return with unsafe block produces invalid code
6 participants