Add allow-by-default lint for unit bindings

[jieyouxu]

Example

#![warn(unit_bindings)]

macro_rules! owo {
    () => {
        let whats_this = ();
    }
}

fn main() {
    // No warning if user explicitly wrote `()` on either side.
    let expr = ();
    let () = expr;
    let _ = ();

    let _ = expr; //~ WARN binding has unit type
    let pat = expr; //~ WARN binding has unit type
    let _pat = expr; //~ WARN binding has unit type

    // No warning for let bindings with unit type in macro expansions.
    owo!();

    // No warning if user explicitly annotates the unit type on the binding.
    let pat: () = expr;
}

outputs

warning: binding has unit type `()`
  --> $DIR/unit-bindings.rs:17:5
   |
LL |     let _ = expr;
   |     ^^^^-^^^^^^^^
   |         |
   |         this pattern is inferred to be the unit type `()`
   |
note: the lint level is defined here
  --> $DIR/unit-bindings.rs:3:9
   |
LL | #![warn(unit_bindings)]
   |         ^^^^^^^^^^^^^

warning: binding has unit type `()`
  --> $DIR/unit-bindings.rs:18:5
   |
LL |     let pat = expr;
   |     ^^^^---^^^^^^^^
   |         |
   |         this pattern is inferred to be the unit type `()`

warning: binding has unit type `()`
  --> $DIR/unit-bindings.rs:19:5
   |
LL |     let _pat = expr;
   |     ^^^^----^^^^^^^^
   |         |
   |         this pattern is inferred to be the unit type `()`

warning: 3 warnings emitted

This lint is not triggered if any of the following conditions are met:

• The user explicitly annotates the binding with the () type.
• The binding is from a macro expansion.
• The user explicitly wrote let () = init;
• The user explicitly wrote let pat = ();. This is allowed for local lifetimes.

Known Issue

It is known that this lint can trigger on some proc-macro generated code whose span returns false for Span::from_expansion because e.g. the proc-macro simply forwards user code spans, and otherwise don't have distinguishing syntax context compared to non-macro-generated code. For those kind of proc-macros, I believe the correct way to fix them is to instead emit identifers with span like Span::mixed_site().located_at(user_span).

Closes #71432.

[rustbot]

r? @compiler-errors

(rustbot has picked a reviewer for you, use r? to override)

[est31]

It's more a lint for bindings of unit type, right? Maybe it should be renamed to unit_bindings? Or are there hypothetically other instances where the lint could warn?

I'm wondering because clippy has a let and return lint, something which I am hitting often because I like to make the pattern and don't really like the lint. Still, it would be good to instantly know the difference between the lint this PR is adding and clippy::let_and_return.

[jieyouxu]

It's more a lint for bindings of unit type, right? Maybe it should be renamed to unit_bindings? Or are there hypothetically other instances where the lint could warn?

Yep, I can rename it to unit_binding, it's called useless_binding now because I couldn't come up with a better name for it. I would also appreciate any feedback on the lint wording / note wording, because I don't think they're very useful (heh) right now.

[WaffleLapkin]

The implementation looks good.

[WaffleLapkin]

T-lang requested a crater run, can you add a temporary commit that makes this lint deny-by-default, so that we can make a crater run and see how much crates have triggered this?

[jieyouxu]

T-lang requested a crater run, can you add a temporary commit that makes this lint deny-by-default, so that we can make a crater run and see how much crates have triggered this?

I will make a temporary commit, but I think crater supports -D unit_bindings (cf. https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp/topic/Allowing.20a.20lint.20on.20diesel.20for.20bors.20try/near/328933796). Something like #107880 (comment).

EDIT: there are several UI tests I will need to fix.

[danielhenrymantilla]

Related to this change, the following limitation:

• issue: let_unit_value should be allowed when I explicitly write "let () = ..."
• PR in progress: Don't lint let_unit_value when () is explicit

As in, while I agree that implicit bindings of () have a rather high likelihood of being an oversight, and thus, a mistake/logic bug (which makes the lint, and this PR, very useful!), there are more rare but nonetheless legitimate cases where a () value is bound, but in a properly annotated way to acknowledge this fact (either by using the () pattern, or a : () type annotation). In such cases, this lint firing is a false-positive, and thus, an annoyance. So this is something to be aware of ⚠️

I think it will just be a matter of waiting for rust-lang/rust-clippy#10844 to be merged, and we will be good to go! 🙂

[jieyouxu]

Opened #112549 to adjust multiple UI tests to help reduce the amount of test failures.

[jieyouxu]

I updated the lint to not fire in the following cases:

• The user explicitly wrote let () = init;.
• The user explicitly wrote let pat = ();.

[jieyouxu]

Maybe the unit_bindings lint should also not fire when the binding is let _?

[WaffleLapkin]

@jieyouxu I think it makes sense to lint for let _, unsure if there are any cases where you'd want to ignore the unit like that (i.e. without asserting the type like let () = or let _: () = do)

[WaffleLapkin]

I will make a temporary commit, but I think crater supports -D unit_bindings

Oh, in this case I think you can drop the commit and we can start a perf run with the flag

[jieyouxu]

Oh, in this case I think you can drop the commit and we can start a perf run with the flag

IIUC this is going to still have to wait on #112549 and a few more UI test adjustments, since I think it won't successfully build due to the UI tests.

[WaffleLapkin]

@bors r+

[bors]

📌 Commit f0520fa has been approved by WaffleLapkin

It is now in the queue for this repository.

[bors]

⌛ Testing commit f0520fa with merge 16ee2f7...

[bors]

💔 Test failed - checks-actions

[est31]

the build failure is legit, likely needs a rebase plus x fmt (rustfmt now formats let chains). @rustbot author

[WaffleLapkin]

@bors r+

[bors]

📌 Commit 8da09ae has been approved by WaffleLapkin

It is now in the queue for this repository.

[bors]

⌛ Testing commit 8da09ae with merge 73bc121...

[bors]

☀️ Test successful - checks-actions
Approved by: WaffleLapkin
Pushing 73bc121 to master...

[rust-timer]

Finished benchmarking commit (73bc121): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.0%	[-1.0%, -1.0%]	3
All ❌✅ (primary)	-	-	0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	2.3%	[2.0%, 2.5%]	3
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.3%	[2.0%, 2.5%]	3

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 676.864s -> 675.017s (-0.27%)
Artifact size: 313.80 MiB -> 313.79 MiB (-0.00%)