Detect underscored variables with no side effects

[F3real]

Fixes #7545

changelog: Lint on underscored variables with no side effects in [no_effect]

[rust-highfive]

r? @xFrednet

(rust-highfive has picked a reviewer for you, use r? to override)

[flip1995]

This lint should be allowed in all test cases, that don't explicitly test for this lint. Otherwise all the stderr files are just messy.

Please also add a test case, where the assigned value implements Drop, which shouldn't lint, e.g. let _x = vec![1];. This allocates memory and keeps it allocated until the end of the scope, so definitely has an effect.

I'm also not so sure if this should be an extension of the no_effect lint, since it seems pretty noisy. E.g. what happens if the _x variable is used afterwards? A leading _ is no guarantee that the variable is unused. A possible real world example:

let _x = 42; // only required if *one of* f1 or f2 is enabled, 
             // but has to be declared with `_` to avoid warning if none of those features are enabled
let mut y = 0;

// If feature 1 is enabled call foo with `_x - 1`
#[cfg(feature = "f1")]
{ y += foo(_x - 1); }

// If feature 2 is enabled call foo with `_x + 1`
#[cfg(feature = "f2")]
{ y += foo(_x + 1); }

y += foo(0);

[F3real]

@flip1995 I had issues with cargo dev bless not being able to update number of tests.

What would be potential issue with Drop? Is this pattern used to prevent dropping value until end of the block. In any case, I will add a test case for it.

Functionality wise, it makes sense to make this part of the no_effect (it is mostly based on existing functionality). But since it does produce large number of reports, maybe we could define new lint in this file to handle it.

I think this makes most sense if used with existing used_underscore_binding lint, since this doesn't detect if binding is used.

[flip1995]

Is this pattern used to prevent dropping value until end of the block

Yes exactly: let _ = vec![]; will drop the vec at the end of the statement. let _x = vec![] will drop the vec at the end of the scope. This is a pretty common pattern, when drop order matters.

Functionality wise, it makes sense to make this part of the no_effect (it is mostly based on existing functionality). But since it does produce large number of reports, maybe we could define new lint in this file to handle it.

Agreed. It's easy to just define another lint and pass it to the span_lint_* function instead of NO_EFFECT. But I'd like input from others, before introducing a new lint for this.

I think this makes most sense if used with existing used_underscore_binding lint, since this doesn't detect if binding is used.

There are two problems with this though.

1. used_underscore_binding is allow-by-default, while no_effect is warn-by-default
2. My example from above is not being linted by used_underscore_binding if no feature is enabled. And IIRC if one of the features is being enabled, the lint produces a FP, IIRC: Playground

[xFrednet]

I'm also in favor of having this in a new lint, as discussed above, and maybe having it allow-by-default. The logic still looks good, just two NITs and there are still some .stderr with lint triggers where no_effect or the new lint should be allowed in 🙃

[xFrednet]

Having a new lint for values starting with underscores seems good to me. Thank you for the refactoring 🙃

[xFrednet]

The implementation part looks good to me. This should be ready for merge once the test output has been added. I also used lintcheck and everything is looking good. You can also squash some commits if you want 🙃

[xFrednet]

LGTM! Thank you for the work and refactoring the code several times! 🙃

[xFrednet]

@bors r+

[bors]

📌 Commit 6b22bba has been approved by xFrednet

[bors]

⌛ Testing commit 6b22bba with merge 06722c0...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: xFrednet
Pushing 06722c0 to master...

[F3real]

@xFrednet np, thanks for reviews 😄