Rust: Warn on useless bindings like `let v2 = v1.sort();`

[mbrubeck]

I've seen a lot of beginning Rust programmers make a mistake similar to the program below, which compiles and runs without warning:

fn main() {
    let mut x = [0,1,2];
    x[0] = 5;
    let y = x.sort();
    println!("{:?}", y);
}

The bug in this program is hard for new Rust programmers to spot. The compiler should help them, by warning that x.sort() does not return a value.

This could be done with a new attribute #[must_not_use], applied to functions like sort. This would be the inverse of must_use: It would warn only if the result of the function is used. Like must_use, it would take an optional argument that provides an additional diagnostic message.

(Bikeshed: must_not_use may be a confusing name. Other suggestions welcome.)

Or perhaps any assignment let foo = expr; where expr has type () should cause a warning by default. This would be a more general solution, catching more possible errors but also potentially causing more disruption. The warnings in this case might be less helpful, because they wouldn't include suggestions tailored to specific functions.

[Mark-Simulacrum]

It seems like we at least want the let y = { expr evaluating to () }; warning, since that's entirely useless AFAICT.

[comex]

It could possibly occur legitimately due to a macro, though. For example, this silly macro doesn't care whether the expression returns () or a real value:

macro_rules! say_hello_after_evaluating_expr {
    ($expr:expr) => { {
        let ret = $expr;
        println!("hello!");
        ret
    } }
}
fn main() {
    let forty_two = say_hello_after_evaluating_expr!(42);
    say_hello_after_evaluating_expr!(println!("this returns ()!"));
}

[Nokel81]

Well, in that situation the macro writer could just do a local allow.

[cuviper]

How about nested () -- would you also warn on this?

fn foo() -> io::Result<()> { ... }
fn main() {
    if let Ok(x) = foo() { ... }
}

IOW, is this just about the RHS being (), or about binding a name to () at all?

[Mark-Simulacrum]

I think this is about binding a name to (), there's not really a good reason to do so IMO.

Maybe the lint would then be less about the "this function returns unit" (though that could be a special case to provide better messaging on), but more so that any binding with a type of () is linted on.

I'm trying to think of what exactly makes () special -- e.g. it seems like ((), ()) should also be linted against -- but not really coming up with anything concrete. Certainly ZST + Copy isn't right.

[mbrubeck]

I think it would be reasonable to warn on any binding to (). If the program never reads from the binding, then it already triggers an unused_variables warning. If it does read from it, this is probably a bug and would benefit from a warning.

Though any solution that captures simple cases like my original example is probably sufficient to provide most of the benefit here.

I'm trying to think of what exactly makes () special -- e.g. it seems like ((), ()) should also be linted against -- but not really coming up with anything concrete.

() may not be uniquely special, but because it is the default return type for Rust functions and blocks, and is supposed to be used for all expressions that exist only for their side effects, it's probably the only such type that matters in practice for 99% of programs. I see little practical benefit from trying to catch every possible case where a return value is useless. Types like ((), ()) show up mainly in generic or macro code where this lint wouldn't be useful, anyway.

[Mark-Simulacrum]

I definitely agree that catching just unit is probably enough. I was mostly trying to come up with some cases where I also want this for non-unit types -- but I was unable to do so.

I'm going to nominate this issue for lang to hopefully gauge whether we want an FCP here or if adding a lint like this (naming to be bikeshedded) needs a full RFC. I personally lean towards maybe an FCP, maybe even just "no one in the room has objections" approval (we'd have some time to roll it back if necessary, of course).

[scottmcm]

Clippy: https://rust-lang.github.io/rust-clippy/master/#let_unit_value

[cuviper]

_Of course_ clippy has this covered. There's probably even an xkcd for that.

It was only recently downgraded to pedantic (allowed): https://github.com/rust-lang/rust-clippy/pull/5409

[Mark-Simulacrum]

I agree with @dtolnay that the bug there (with inference) seems like a hard blocker.

But it seems plausible that we'd perhaps want it in rustc regardless as allow by default to start. Maybe if we throw compiler team at it someone will come up with an idea how to do it (seems related to @nikomatsakis work on the ! type inference).

[cuviper]

let _: () = f();

_ is a special binding -- it should be exempt from this. That binding can also be avoided while still affecting type inference: let () = f();. However, clippy continues to lint this. I haven't looked at the code, but it seems like clippy is only looking at the expression, not the pattern bindings.

[moxian]

To bikeshed this further, i would argue that let x: () = ... should not trigger the lint (even with non-_ binding, and even when no inference is involved), on the premise that the author is already aware of the type, and this must be deliberate.

[estebank]

I think that this belongs in rustc (I have argued as much in the past) and the rules should be:

• Ignore let assignments with a user provided type
• Ignore macros
• Ignore _

[joshtriplett]

To bikeshed this further, i would argue that let x: () = ... should not trigger the lint (even with non-_ binding, and even when no inference is involved), on the premise that the author is already aware of the type, and this must be deliberate.

Agreed. The lint should cover the case where the user doesn't explicitly write the type ().

[joshtriplett]

We discussed this further in today's language team meeting, and came to the following consensus:

• We should lint on let x = expr; if we infer x as type ().
• We should suppress that lint if you write the type () explicitly: let x: () = expr;
• We should do a crater run to see how noisy this will be, before merging it.