fix: identity_op suggestions use correct parenthesis
#13647
Conversation
The `identity_op` lint was suggesting code fixes that resulted in incorrect or broken code, due to missing parenthesis in the fix that changed the semantics of the code. For a binary expression, `left op right`, if the `left` was redundant, it would check if the right side needed parenthesis, but if the `right` was redundant, it would just assume that the left side did not need parenthesis. This can result in either rustfix generating broken code and failing, or code that has different behavior than before the fix. e.g. `-(x + y + 0)` would turn into `-x + y`, changing the behavior, and `1u64 + (x + y + 0i32) as u64` where `x: i32` and `y: i32` would turn into `1u64 + x + y as u64`, creating broken code where `x` cannot be added to the other values, as it was never cast to `u64`. This commit fixes both of these cases by always checking the non-redundant child of a binary expression for needed parenthesis, and makes it so if we need parenthesis, but they already exist, we don't add any redundant ones. Fixes rust-lang#13470
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Jarcho (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
rustbot
added
the
S-waiting-on-review
| @@ -234,7 +220,13 @@ fn span_ineffective_operation( | |||
| expr_snippet.into_owned() | |||
| }; | |||
| let suggestion = match parens { | |||
| Parens::Needed => format!("({expr_snippet})"), | |||
| Parens::Needed => { | |||
| if !expr_snippet.starts_with('(') && !expr_snippet.ends_with(')') { | |||
There was a problem hiding this comment.
This doesn't actually work. Something like (x) + (y) would act as though it has parenthesis.
tests/ui/identity_op.stderr
Outdated
| LL | let _ = 2 + (x + (y * z) + 0); | ||
| | ^^^^^^^^^^^^^^^^^ help: consider reducing it to: `x + (y * z)` |
There was a problem hiding this comment.
This is a very subtle behaviour change that can result in an overflow. x + (y*z) can exceed the minimum value on signed integers in cases where 2 + x + (x*z) do not.
Even without that this shouldn't be suggesting to remove the parenthesis.
It was an incorrect, and could lead to behavior changes in the suggested code
|
@Jarcho Thanks a bunch for the review, I believe I have resolved both of the issues you pointed out! |
|
Thank you. @bors r+ |
|
@Jarcho: 🔑 Insufficient privileges: Not in reviewers |
The
identity_oplint was suggesting code fixes that resulted in incorrect or broken code, due to missing parenthesis in the fix that changed the semantics of the code.For a binary expression,
left op right, if theleftwas redundant, it would check if the right side needed parenthesis, but if therightwas redundant, it would just assume that the left side did not need parenthesis.This can result in rustfix generating broken code and failing, or generating code that has different behavior than before the fix. e.g.
-(x + y + 0)would turn into-x + y, changing the behavior, and1u64 + (x + y + 0i32) as u64wherex: i32andy: i32would turn into1u64 + x + y as u64, creating an error wherexcannot be added to the other values, as it was never cast tou64.This commit fixes both of these problems by always checking the non-redundant child of a binary expression for needed parenthesis.
fixes #13470
changelog: [
identity_op]: Fix suggested code that is broken or has changed behavior