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.

Sign up for GitHub

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

Jump to bottom

obfuscated_if_else doesn't check for side effects in subexpressions #14034

Open
samueltardieu opened this issue Jan 19, 2025 · 1 comment · May be fixed by #14061
Open

obfuscated_if_else doesn't check for side effects in subexpressions #14034

samueltardieu opened this issue Jan 19, 2025 · 1 comment · May be fixed by #14061
Labels
C-bug Category: Clippy is not doing the correct thing I-suggestion-causes-error Issue: The suggestions provided by this Lint cause an ICE/error when applied

Comments

@samueltardieu
Copy link
Contributor

Summary

The lint suggests changing b.then_some(x).unwrap_or(y) into if b { x } else { y } without checking that the evaluation of x and y (whose evaluation is short-circuited in the suggested change) doesn't have any side effect.

Reproducer

I tried this code:

#![allow(clippy::all)]
#![warn(clippy::obfuscated_if_else)]

fn allocate(counter: &mut u32) -> u32 {
    *counter += 1;
    *counter
}

fn main() {
    let mut counter = 0;
    let a = 3;
    let c = (a == 1)
        .then_some(allocate(&mut counter))
        .unwrap_or(allocate(&mut counter));
    assert_eq!(c, 2);
    assert_eq!(counter, 2);
}

which runs without error.

Clippy proposed to transform it into:

#![allow(clippy::all)]
#![warn(clippy::obfuscated_if_else)]

fn allocate(counter: &mut u32) -> u32 {
    *counter += 1;
    *counter
}

fn main() {
    let mut counter = 0;
    let a = 3;
    let c = if (a == 1) { allocate(&mut counter) } else { allocate(&mut counter) };
    assert_eq!(c, 2);
    assert_eq!(counter, 2);
}

which fails the test assert_eq!(c, 2) because only one call to allocate now took place.

At minimum, the applicability should be downgraded from MachineApplicable to MaybeIncorrect.

Version

rustc 1.86.0-nightly (049355708 2025-01-18)
binary: rustc
commit-hash: 049355708383ab1b9a1046559b9d4230bdb3a5bc
commit-date: 2025-01-18
host: x86_64-unknown-linux-gnu
release: 1.86.0-nightly
LLVM version: 19.1.7

Additional Labels

@rustbot label +I-suggestion-causes-error
@samueltardieu samueltardieu added the C-bug Category: Clippy is not doing the correct thing label Jan 19, 2025
@rustbot rustbot added the I-suggestion-causes-error Issue: The suggestions provided by this Lint cause an ICE/error when applied label Jan 19, 2025
@samueltardieu
Copy link
Contributor Author

(pinging @lapla-cogito since we talked about this)

This was referenced Jan 19, 2025
Closed
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
C-bug Category: Clippy is not doing the correct thing I-suggestion-causes-error Issue: The suggestions provided by this Lint cause an ICE/error when applied
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

proper applicability for obfuscated_if_else lapla-cogito/rust-clippy
2 participants
@samueltardieu @rustbot