Skip to content

tuples containing #[must_use] types should be #[must_use] #61061

New issue

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

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

Already on GitHub? # to your account

Closed
cramertj opened this issue May 23, 2019 · 7 comments · Fixed by #61100
Closed

tuples containing #[must_use] types should be #[must_use] #61061

cramertj opened this issue May 23, 2019 · 7 comments · Fixed by #61100
Labels
A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@cramertj
Copy link
Member

fn main() {
    Ok::<(), ()>(());
}

gives a warning, but

fn main() {
    (Ok::<(), ()>(()),);
}

does not.

@Centril Centril added T-lang Relevant to the language team, which will review and decide on the PR/issue. A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. I-nominated labels May 23, 2019
@Centril
Copy link
Contributor

Centril commented May 23, 2019

@joshtriplett suggested that we test this out with crater to see if this would generate much substantial noise and how common it would be.

@cramertj
Copy link
Member Author

I'd expect it'd be a pretty substantial source of warnings, but that's exactly why I think we should introduce it. People compiling with #![deny(warnings)] are asking for these sorts of bugs to be caught, and we should serve them appropriately ;)

In any case, running crater would require having an implementation to do a crater run on, so let's do it!

@scottmcm
Copy link
Member

scottmcm commented May 23, 2019

Let's do it. (I'd personally even like an attribute to allow people to opt-in to this for Option<T>, for example, when T is itself must_use. That way if I make a Vec<Result<..>>, I'll get a warning on pop that I normally wouldn't want but plausibly do now that I'm getting Results in the Options.)

@varkor
Copy link
Member

varkor commented May 23, 2019

I've opened a pull request to try this out: #61100.

@Centril
Copy link
Contributor

Centril commented May 23, 2019

We discussed this on the language team meeting; folks felt generally positive, but we'd like to do a crater run to see what the impact would be like and if there would be many false positives.

@Centril Centril added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. labels May 23, 2019
bors added a commit that referenced this issue May 24, 2019
Apply #[must_use] lint to components of tuples

Fixes #61061.
bors added a commit that referenced this issue Jun 3, 2019
Apply #[must_use] lint to components of tuples

Fixes #61061.
@pickfire
Copy link
Contributor

pickfire commented May 2, 2021

I tried this using #[must_use] but it does not seemed to complain when the part of #[must_use] is not used, is the the correct behavior?

struct A;

#[must_use]
struct B;

fn t() -> (A, B) {
    (A, B)
}

fn main() {
    let (a, _b) = t();
}

https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=7034a76d4d9d38154798fa3aec48f972

I hit this when doing https://docs.rs/hyper/0.14.7/hyper/client/conn/struct.Builder.html#method.handshake where I didn't use connection part of the tuple which cause it to not work, which I did not know is required to be used.

@scottmcm
Copy link
Member

scottmcm commented May 2, 2021

@pickfire putting it into a binding, even an unused one, is considered a use, like how let _ = something_must_use(); doesn't lint. With just calling t() it lints: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=c2dc6ffbd470b5f098e4b1bdf653d57f

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants