Skip to content

Commit

Permalink
proper applicability for obfuscated_if_else (#14061)
Browse files Browse the repository at this point in the history
fix #14034

The currect implementation of `obfuscated_if_else` sometimes makes
incorrect suggestions when the original code have side effects (see the
example in the above issue). I think this can be fixed by changing the
applicability depending on whether it can have side effects or not.

changelog: [`obfuscated_if_else`]: change applicability when the
original code can have side effects
  • Loading branch information
Centri3 authored Jan 28, 2025
2 parents 83bde36 + 2640f65 commit 66dc8a1
Show file tree
Hide file tree
Showing 4 changed files with 30 additions and 4 deletions.
8 changes: 7 additions & 1 deletion clippy_lints/src/methods/obfuscated_if_else.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
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::source::snippet_with_applicability;
use clippy_utils::sugg::Sugg;
use rustc_errors::Applicability;
Expand All @@ -18,7 +19,12 @@ pub(super) fn check<'tcx>(
let recv_ty = cx.typeck_results().expr_ty(then_recv);

if recv_ty.is_bool() {
let mut applicability = Applicability::MachineApplicable;
let mut applicability = if switch_to_eager_eval(cx, then_arg) && switch_to_eager_eval(cx, unwrap_arg) {
Applicability::MachineApplicable
} else {
Applicability::MaybeIncorrect
};

let if_then = match then_method_name {
"then" if let ExprKind::Closure(closure) = then_arg.kind => {
let body = cx.tcx.hir().body(closure.body);
Expand Down
6 changes: 5 additions & 1 deletion tests/ui/obfuscated_if_else.fixed
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
#![warn(clippy::obfuscated_if_else)]
#![allow(clippy::unnecessary_lazy_evaluations)]
#![allow(clippy::unnecessary_lazy_evaluations, clippy::unit_arg, clippy::unused_unit)]

fn main() {
if true { "a" } else { "b" };
Expand All @@ -11,4 +11,8 @@ fn main() {

let partial = (a == 1).then_some("a");
partial.unwrap_or("b"); // not lint

let mut a = 0;
if true { a += 1 } else { () };
if true { () } else { a += 2 };
}
6 changes: 5 additions & 1 deletion tests/ui/obfuscated_if_else.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
#![warn(clippy::obfuscated_if_else)]
#![allow(clippy::unnecessary_lazy_evaluations)]
#![allow(clippy::unnecessary_lazy_evaluations, clippy::unit_arg, clippy::unused_unit)]

fn main() {
true.then_some("a").unwrap_or("b");
Expand All @@ -11,4 +11,8 @@ fn main() {

let partial = (a == 1).then_some("a");
partial.unwrap_or("b"); // not lint

let mut a = 0;
true.then_some(a += 1).unwrap_or(());
true.then_some(()).unwrap_or(a += 2);
}
14 changes: 13 additions & 1 deletion tests/ui/obfuscated_if_else.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -25,5 +25,17 @@ error: this method chain can be written more clearly with `if .. else ..`
LL | (a == 1).then(|| "a").unwrap_or("b");
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `if a == 1 { "a" } else { "b" }`

error: aborting due to 4 previous errors
error: this method chain can be written more clearly with `if .. else ..`
--> tests/ui/obfuscated_if_else.rs:16:5
|
LL | true.then_some(a += 1).unwrap_or(());
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `if true { a += 1 } else { () }`

error: this method chain can be written more clearly with `if .. else ..`
--> tests/ui/obfuscated_if_else.rs:17:5
|
LL | true.then_some(()).unwrap_or(a += 2);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `if true { () } else { a += 2 }`

error: aborting due to 6 previous errors

0 comments on commit 66dc8a1

Please sign in to comment.