From dd51e9638113313afd4252a68d5b019ec9e85c58 Mon Sep 17 00:00:00 2001 From: Samuel Tardieu Date: Fri, 31 Jan 2025 23:25:26 +0100 Subject: [PATCH] Fix `obfuscated_if_else` suggestion on left side of a binary expr MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An `if … { … } else { … }` used as the left operand of a binary expression requires parentheses to be parsed as an expression. --- .../src/methods/obfuscated_if_else.rs | 12 +++++ tests/ui/obfuscated_if_else.fixed | 17 +++++++ tests/ui/obfuscated_if_else.rs | 17 +++++++ tests/ui/obfuscated_if_else.stderr | 50 ++++++++++++++++++- 4 files changed, 95 insertions(+), 1 deletion(-) diff --git a/clippy_lints/src/methods/obfuscated_if_else.rs b/clippy_lints/src/methods/obfuscated_if_else.rs index b71f79f84824..2272e03ef26e 100644 --- a/clippy_lints/src/methods/obfuscated_if_else.rs +++ b/clippy_lints/src/methods/obfuscated_if_else.rs @@ -1,6 +1,7 @@ use super::OBFUSCATED_IF_ELSE; use clippy_utils::diagnostics::span_lint_and_sugg; use clippy_utils::eager_or_lazy::switch_to_eager_eval; +use clippy_utils::get_parent_expr; use clippy_utils::source::snippet_with_applicability; use clippy_utils::sugg::Sugg; use rustc_errors::Applicability; @@ -41,6 +42,17 @@ pub(super) fn check<'tcx>( snippet_with_applicability(cx, unwrap_arg.span, "..", &mut applicability) ); + // To be parsed as an expression, the `if { … } else { … }` as the left operand of a binary operator + // requires parentheses. + let sugg = if let Some(parent_expr) = get_parent_expr(cx, expr) + && let ExprKind::Binary(_, left, _) = parent_expr.kind + && left.hir_id == expr.hir_id + { + format!("({sugg})") + } else { + sugg + }; + span_lint_and_sugg( cx, OBFUSCATED_IF_ELSE, diff --git a/tests/ui/obfuscated_if_else.fixed b/tests/ui/obfuscated_if_else.fixed index bfe1c5e10cf8..65874bfdcc35 100644 --- a/tests/ui/obfuscated_if_else.fixed +++ b/tests/ui/obfuscated_if_else.fixed @@ -16,3 +16,20 @@ fn main() { if true { a += 1 } else { () }; if true { () } else { a += 2 }; } + +fn issue11141() { + // Parentheses are required around the left side of a binary expression + let _ = (if true { 40 } else { 17 }) | 2; + // Parentheses are required only for the leftmost expression + let _ = (if true { 30 } else { 17 }) | if true { 2 } else { 3 } | if true { 10 } else { 1 }; + + // Parentheses are not required around the right side of a binary expression + let _ = 2 | if true { 40 } else { 17 }; + + // Parentheses are not required for a cast + let _ = if true { 42 } else { 17 } as u8; + // Parentheses are not required for a deref + let _ = *if true { &42 } else { &17 }; + // Parentheses are not required for a deref followed by a cast + let _ = *if true { &42 } else { &17 } as u8; +} diff --git a/tests/ui/obfuscated_if_else.rs b/tests/ui/obfuscated_if_else.rs index 0ded2a2ceedf..c9784329a125 100644 --- a/tests/ui/obfuscated_if_else.rs +++ b/tests/ui/obfuscated_if_else.rs @@ -16,3 +16,20 @@ fn main() { true.then_some(a += 1).unwrap_or(()); true.then_some(()).unwrap_or(a += 2); } + +fn issue11141() { + // Parentheses are required around the left side of a binary expression + let _ = true.then_some(40).unwrap_or(17) | 2; + // Parentheses are required only for the leftmost expression + let _ = true.then_some(30).unwrap_or(17) | true.then_some(2).unwrap_or(3) | true.then_some(10).unwrap_or(1); + + // Parentheses are not required around the right side of a binary expression + let _ = 2 | true.then_some(40).unwrap_or(17); + + // Parentheses are not required for a cast + let _ = true.then_some(42).unwrap_or(17) as u8; + // Parentheses are not required for a deref + let _ = *true.then_some(&42).unwrap_or(&17); + // Parentheses are not required for a deref followed by a cast + let _ = *true.then_some(&42).unwrap_or(&17) as u8; +} diff --git a/tests/ui/obfuscated_if_else.stderr b/tests/ui/obfuscated_if_else.stderr index 9ce1f475c480..1ca63a3f04f9 100644 --- a/tests/ui/obfuscated_if_else.stderr +++ b/tests/ui/obfuscated_if_else.stderr @@ -37,5 +37,53 @@ error: this method chain can be written more clearly with `if .. else ..` LL | true.then_some(()).unwrap_or(a += 2); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `if true { () } else { a += 2 }` -error: aborting due to 6 previous errors +error: this method chain can be written more clearly with `if .. else ..` + --> tests/ui/obfuscated_if_else.rs:22:13 + | +LL | let _ = true.then_some(40).unwrap_or(17) | 2; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `(if true { 40 } else { 17 })` + +error: this method chain can be written more clearly with `if .. else ..` + --> tests/ui/obfuscated_if_else.rs:24:13 + | +LL | let _ = true.then_some(30).unwrap_or(17) | true.then_some(2).unwrap_or(3) | true.then_some(10).unwrap_or(1); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `(if true { 30 } else { 17 })` + +error: this method chain can be written more clearly with `if .. else ..` + --> tests/ui/obfuscated_if_else.rs:24:48 + | +LL | let _ = true.then_some(30).unwrap_or(17) | true.then_some(2).unwrap_or(3) | true.then_some(10).unwrap_or(1); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `if true { 2 } else { 3 }` + +error: this method chain can be written more clearly with `if .. else ..` + --> tests/ui/obfuscated_if_else.rs:24:81 + | +LL | let _ = true.then_some(30).unwrap_or(17) | true.then_some(2).unwrap_or(3) | true.then_some(10).unwrap_or(1); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `if true { 10 } else { 1 }` + +error: this method chain can be written more clearly with `if .. else ..` + --> tests/ui/obfuscated_if_else.rs:27:17 + | +LL | let _ = 2 | true.then_some(40).unwrap_or(17); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `if true { 40 } else { 17 }` + +error: this method chain can be written more clearly with `if .. else ..` + --> tests/ui/obfuscated_if_else.rs:30:13 + | +LL | let _ = true.then_some(42).unwrap_or(17) as u8; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `if true { 42 } else { 17 }` + +error: this method chain can be written more clearly with `if .. else ..` + --> tests/ui/obfuscated_if_else.rs:32:14 + | +LL | let _ = *true.then_some(&42).unwrap_or(&17); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `if true { &42 } else { &17 }` + +error: this method chain can be written more clearly with `if .. else ..` + --> tests/ui/obfuscated_if_else.rs:34:14 + | +LL | let _ = *true.then_some(&42).unwrap_or(&17) as u8; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `if true { &42 } else { &17 }` + +error: aborting due to 14 previous errors