Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Let_unit_value false positive when return type is type parameter #1502

Closed
dtolnay opened this issue Feb 1, 2017 · 8 comments · Fixed by #8563
Closed

Let_unit_value false positive when return type is type parameter #1502

dtolnay opened this issue Feb 1, 2017 · 8 comments · Fixed by #8563
Labels
C-bug Category: Clippy is not doing the correct thing E-hard Call for participation: This a hard problem and requires more experience or effort to work on I-false-positive Issue: The lint was triggered on code it shouldn't have T-middle Type: Probably requires verifiying types

Comments

@dtolnay
Copy link
Member

dtolnay commented Feb 1, 2017

fn main() {
    let _: () = f();
}

fn f<T: Default>() -> T {
    T::default()
}
warning: this let-binding has unit value. Consider omitting `let _ =`, #[warn(let_unit_value)] on by default
 --> src/main.rs:2:5
  |
2 |     let _: () = f();
  |     ^^^^^^^^^^^^^^^^
  |
  = help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#let_unit_value

Maybe this specific case would be clearer with f::<()>() but I consider this a false positive. Came up in serde_json unit tests where we test that serde_json::from_str can parse various representations of ().

@oli-obk oli-obk added E-medium Call for participation: Medium difficulty level problem and requires some initial experience. C-bug Category: Clippy is not doing the correct thing T-middle Type: Probably requires verifiying types labels Feb 2, 2017
@oli-obk
Copy link
Contributor

oli-obk commented Feb 2, 2017

we need to check whether the rhs contains any type inference.

@oli-obk oli-obk added E-hard Call for participation: This a hard problem and requires more experience or effort to work on and removed E-medium Call for participation: Medium difficulty level problem and requires some initial experience. labels Jan 8, 2018
@oli-obk
Copy link
Contributor

oli-obk commented Jan 8, 2018

I'm not sure how to solve this. This will need some forensics first to figure out whether rustc can tell us if it used inference.

@dtolnay
Copy link
Member Author

dtolnay commented Jan 8, 2018

I think it would work to recognize just this specific pattern: return type consists entirely of a type parameter that is not used by any of the argument types. In that case permit let_unit_value.

flip1995 added a commit to flip1995/rust-clippy that referenced this issue Apr 7, 2020
Downgrade let_unit_value to pedantic

Given that the false positive in rust-lang#1502 is marked E-hard and I don't have much hope of it getting fixed, I think it would be wise to disable this lint by default. I have had to suppress this lint in every substantial codebase (\>100k line) I have worked in. Any time this lint is being triggered, it's always the false positive case.

The motivation for this lint is documented as:

> A unit value cannot usefully be used anywhere. So binding one is kind of pointless.

with this example:

> ```rust
> let x = {
>     1;
> };
> ```

Sure, but the author would find this out via an unused_variable warning or from `x` not being the type that they need further down. If there ends up being a type error on `x`, clippy's advice isn't going to help get the code compiling because it can only run if the code already compiles.

changelog: Remove let_unit_value from default set of enabled lints
flip1995 added a commit to flip1995/rust-clippy that referenced this issue Apr 8, 2020
Downgrade let_unit_value to pedantic

Given that the false positive in rust-lang#1502 is marked E-hard and I don't have much hope of it getting fixed, I think it would be wise to disable this lint by default. I have had to suppress this lint in every substantial codebase (\>100k line) I have worked in. Any time this lint is being triggered, it's always the false positive case.

The motivation for this lint is documented as:

> A unit value cannot usefully be used anywhere. So binding one is kind of pointless.

with this example:

> ```rust
> let x = {
>     1;
> };
> ```

Sure, but the author would find this out via an unused_variable warning or from `x` not being the type that they need further down. If there ends up being a type error on `x`, clippy's advice isn't going to help get the code compiling because it can only run if the code already compiles.

changelog: Remove let_unit_value from default set of enabled lints
@hellow554
Copy link
Contributor

This is an interesting thing I stumpled upon today.

I would say that your code @dtolnay should still be linted, because you assign the unit value to a named variable (although it has an underscore, but that doesn't matter here ;) )

I would let clippy suggest doing this:

let () = f();

This would still let the compiler infer the type but telling the user, that he can't do anything with the unit value.
Or can you think of a specific use case where you need a variable name?

@dtolnay
Copy link
Member Author

dtolnay commented Jul 30, 2021

let _: () does not assign a named variable.

@hellow554
Copy link
Contributor

I somehow saw let _a: (), don't ask me why... (:

@lilyball
Copy link

This fix for this doesn't work for me. Approximate code:

let (_tx, rx) = oneshot::channel();
let handle = spawn(async {
    let () = rx.await.unwrap();
});

This triggers the lint in 1.62.0 (now that it defaults to warn). Using let _: () = … also triggers it. The let-binding here is most certainly required for type inference.

@lilyball
Copy link

It sounds like #8998 might cover my case, though I don't know how that is different from the issue described here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Clippy is not doing the correct thing E-hard Call for participation: This a hard problem and requires more experience or effort to work on I-false-positive Issue: The lint was triggered on code it shouldn't have T-middle Type: Probably requires verifiying types
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants