Rust: False warning: "unused logical operation that must be used"

[spacekookie]

In a loop, in some cases, rustc seems to not understand that a boolean expression was used for sideeffects, and warns about unused logical operation. The following code can quickly replicate the problem.

use std::sync::{atomic::{AtomicBool, Ordering}, Arc};


fn main() {
    let running = Arc::new(AtomicBool::from(true));

    {
        let running = Arc::clone(&running);
        std::thread::spawn(move || {
            running.swap(false, Ordering::Relaxed);
        });
    }

    loop {
        // ...
        println!("Running....");

        !running.load(Ordering::Relaxed) && break;
    }

    println!("Exhausted >.>");
}

The expected result is that rustc doesn't output a warning in this case.

Meta

• rustc version: 1.41.0

[RustyYato]

I don't think this is the intended use of logical operators, you should use an if, when side effects matter. This lint should help people stay away from mis-using operators in this way.

[nagisa]

While it is a bad style to do something like this, it is also perfectly valid code. As rustc’s lints are expected to have no false-positives, this is a genuine bug.

The stylistic preference should be linted by clippy, not rustc.

[Centril]

For the record, I disagree that this is bad style, or it's at least debatable. It seems like it's taking advantage of the sugar that && brings over if, and Rust being a sideeffectful language, this should be expected.

As for whether this should be linted against or not, it seems to me it should not, but cc @rust-lang/lang if there are objections.

[eddyb]

I agree with @nagisa and @Centril, && and || should not be linted, they're control-flow operators, not value operators. Looks like operators like & and | also warn, so we can keep just those.

[estebank]

Pointers:

The code that needs to be modified is

https://github.com/rust-lang/rust/blob/18c275b423f9f13c0e404ae3804967d2ab66337c/src/librustc_lint/unused.rs#L37-L111

I believe you will have to explore the ExprKind::Binary(_, _, rhs) recursively to see if the leaf rhs is of type !, and if that is the case, return early.

The cases where the lhs is ! should already be handled by the unreachable_code lint:

warning: unreachable expression
  --> src/main.rs:18:16
   |
18 |         break && break;
   |                ^^-----
   |                | |
   |                | any code following this expression is unreachable
   |                unreachable expression
   |
   = note: `#[warn(unreachable_code)]` on by default

[eddyb]

I believe you will have to explore the ExprKind::Binary(_, _, rhs) recursively to see if the leaf rhs is of type !, and if that is the case, return early.

I would just replace Some(...) with None on this line:

hir::BinOpKind::And | hir::BinOpKind::Or => Some("logical operation"),

As to lint correctly you'd probably also have to look at whether there any side-effects in the RHS.

[spacekookie]

@estebank thanks! I'll try to have a look at this and post if I have any questions :)