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

rust/src/librustc_lint/unused.rs

Lines 37 to 111 in 18c275b

	fn check_stmt(&mut self, cx: &LateContext<'_, '_>, s: &hir::Stmt<'_>) {
	let expr = match s.kind {
	hir::StmtKind::Semi(ref expr) => &**expr,
	_ => return,
	};
	if let hir::ExprKind::Ret(..) = expr.kind {
	return;
	}
	let ty = cx.tables.expr_ty(&expr);
	let type_permits_lack_of_use = check_must_use_ty(cx, ty, &expr, s.span, "", "", 1);
	let mut fn_warned = false;
	let mut op_warned = false;
	let maybe_def_id = match expr.kind {
	hir::ExprKind::Call(ref callee, _) => {
	match callee.kind {
	hir::ExprKind::Path(ref qpath) => {
	match cx.tables.qpath_res(qpath, callee.hir_id) {
	Res::Def(DefKind::Fn, def_id) | Res::Def(DefKind::Method, def_id) => {
	Some(def_id)
	}
	// `Res::Local` if it was a closure, for which we
	// do not currently support must-use linting
	_ => None,
	}
	}
	_ => None,
	}
	}
	hir::ExprKind::MethodCall(..) => cx.tables.type_dependent_def_id(expr.hir_id),
	_ => None,
	};
	if let Some(def_id) = maybe_def_id {
	fn_warned = check_must_use_def(cx, def_id, s.span, "return value of ", "");
	} else if type_permits_lack_of_use {
	// We don't warn about unused unit or uninhabited types.
	// (See https://github.com/rust-lang/rust/issues/43806 for details.)
	return;
	}
	let must_use_op = match expr.kind {
	// Hardcoding operators here seemed more expedient than the
	// refactoring that would be needed to look up the `#[must_use]`
	// attribute which does exist on the comparison trait methods
	hir::ExprKind::Binary(bin_op, ..) => match bin_op.node {
	hir::BinOpKind::Eq
	| hir::BinOpKind::Lt
	| hir::BinOpKind::Le
	| hir::BinOpKind::Ne
	| hir::BinOpKind::Ge
	| hir::BinOpKind::Gt => Some("comparison"),
	hir::BinOpKind::Add
	| hir::BinOpKind::Sub
	| hir::BinOpKind::Div
	| hir::BinOpKind::Mul
	| hir::BinOpKind::Rem => Some("arithmetic operation"),
	hir::BinOpKind::And | hir::BinOpKind::Or => Some("logical operation"),
	hir::BinOpKind::BitXor
	| hir::BinOpKind::BitAnd
	| hir::BinOpKind::BitOr
	| hir::BinOpKind::Shl
	| hir::BinOpKind::Shr => Some("bitwise operation"),
	},
	hir::ExprKind::Unary(..) => Some("unary operation"),
	_ => None,
	};
	if let Some(must_use_op) = must_use_op {
	cx.struct_span_lint(UNUSED_MUST_USE, expr.span, |lint| {
	lint.build(&format!("unused {} that must be used", must_use_op)).emit()
	});
	op_warned = true;
	}

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 :)

[lolbinarycat]

triage: still an issue on latest nightly