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

#[must_use] does not work for nested structures #39524

Closed
sunjay opened this issue Feb 4, 2017 · 5 comments
Closed

#[must_use] does not work for nested structures #39524

sunjay opened this issue Feb 4, 2017 · 5 comments
Labels
A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-feature-request Category: A feature request, i.e: not implemented / a PR. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@sunjay
Copy link
Member

sunjay commented Feb 4, 2017

When #[must_use] is applied to a structure that is nested, the linter no longer picks up when that value is unused.

This is related to: #26291 and #26281

Looking at those issues, there seem to be a lot of cases where #[must_use] does not work. This is unfortunate because it is actually an incredibly useful annotation that definitely has applications past just the Result enum.

Accidentally relying on it too much can have you make mistakes you otherwise would have corrected had the warning been a bit smarter. I consider this a bug because #[must_use] implies that you must use that value no matter what; regardless of what form it is returned in.

I tried this code: (Rust Playground: https://is.gd/vCLZKq)

#[must_use]
#[derive(Debug)]
struct MustUse;

struct Foo {
    must: MustUse,
}

impl Foo {
    fn bar(&self) {
        println!("{:?}", self.must);
    }
}

fn must() -> Foo {
    Foo {must: MustUse}
}

fn main() {
    must().bar();
    must();
}

I expected to see this happen: A warning about the MustUse structure not being used

Instead, this happened: The code compiled and ran with no errors

Meta

rustc --version --verbose: stable, nightly, etc.

Backtrace: not applicable

@Mark-Simulacrum Mark-Simulacrum added the A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. label May 20, 2017
@mitsuhiko
Copy link
Contributor

This definitely comes up with Option<Result<T, E>>. Since .map can produce something like this easily that is not great.

@Mark-Simulacrum Mark-Simulacrum added the C-feature-request Category: A feature request, i.e: not implemented / a PR. label Jul 27, 2017
ramosbugs added a commit to ramosbugs/oauth2-rs that referenced this issue Apr 21, 2018
NOTE: This is another breaking change that will be part of the 2.0.0 release.

This diff replaces weakly typed `String`s, `Url`s, and other types with new
types generated using the `NewType` pattern. Using stronger types here
should avoid common mistakes (e.g., switching the order of the authorization
and endpoint URLs when instantiating a new `Client`).

In addition to adding a `NewType` trait, this diff adds a `NewSecretType`
trait, which implements `Debug` in a way that redacts the secret. This
behavior avoids a common source of security bugs: logging secrets,
especially when errors occur. Unlike the `NewType` trait, the
`NewSecretType` does not implement `Deref`. Instead, the secret must
be explicitly extracted by calling the `secret` method.

Finally, this PR resolves #28 by having the `authorize_url` method accept a
closure for generating a fresh CSRF token on each invocation. The token is
returned by the method as `#[must_use]`, which the caller should compare
against the response sent by the authorization server to the redirect URI.
Note that `#[must_use]` currently has no effect in this context, but it should
once rust-lang/rust#39524 is resolved.
@Centril Centril added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Jun 29, 2019
@varkor
Copy link
Member

varkor commented Jun 29, 2019

I think this should essentially be considered a straightforward extension of #61061 (which extended #[must_use] to tuples). I have a WIP branch that extends #[must_use] to nested user-defined structures like this, but it needs some cleaning up.

bors added a commit that referenced this issue Jul 22, 2019
Extend `#[must_use]` to nested structures

Extends the `#[must_use]` lint to apply when `#[must_use]` types are nested within `struct`s (or one-variant `enum`s), making the lint much more generally useful. This is in line with #61100 extending the lint to tuples.

Fixes #39524.

cc @rust-lang/lang and @rust-lang/compiler for discussion in case this is a controversial change. In particular, we might want to consider allowing annotations on fields containing `#[must_use]` types in user-defined types (e.g. `#[allow(unused_must_use)]`) to opt out of this behaviour, if there are cases where we this this is likely to have frequent false positives.

(This is based on top of #62235.)
bors added a commit that referenced this issue Jul 24, 2019
Extend `#[must_use]` to nested structures

Extends the `#[must_use]` lint to apply when `#[must_use]` types are nested within `struct`s (or one-variant `enum`s), making the lint much more generally useful. This is in line with #61100 extending the lint to tuples.

Fixes #39524.

cc @rust-lang/lang and @rust-lang/compiler for discussion in case this is a controversial change. In particular, we might want to consider allowing annotations on fields containing `#[must_use]` types in user-defined types (e.g. `#[allow(unused_must_use)]`) to opt out of this behaviour, if there are cases where we this this is likely to have frequent false positives.

(This is based on top of #62235.)
bors added a commit that referenced this issue Jul 30, 2019
Extend `#[must_use]` to nested structures

Extends the `#[must_use]` lint to apply when `#[must_use]` types are nested within `struct`s (or one-variant `enum`s), making the lint much more generally useful. This is in line with #61100 extending the lint to tuples.

Fixes #39524.

cc @rust-lang/lang and @rust-lang/compiler for discussion in case this is a controversial change. In particular, we might want to consider allowing annotations on fields containing `#[must_use]` types in user-defined types (e.g. `#[allow(unused_must_use)]`) to opt out of this behaviour, if there are cases where we this this is likely to have frequent false positives.

(This is based on top of #62235.)
bors added a commit that referenced this issue Sep 9, 2019
Extend `#[must_use]` to nested structures

Extends the `#[must_use]` lint to apply when `#[must_use]` types are nested within `struct`s (or one-variant `enum`s), making the lint much more generally useful. This is in line with #61100 extending the lint to tuples.

Fixes #39524.

cc @rust-lang/lang and @rust-lang/compiler for discussion in case this is a controversial change. In particular, we might want to consider allowing annotations on fields containing `#[must_use]` types in user-defined types (e.g. `#[allow(unused_must_use)]`) to opt out of this behaviour, if there are cases where we this this is likely to have frequent false positives.

(This is based on top of #62235.)
bors added a commit that referenced this issue Oct 8, 2019
Extend `#[must_use]` to nested structures

Extends the `#[must_use]` lint to apply when `#[must_use]` types are nested within `struct`s (or one-variant `enum`s), making the lint much more generally useful. This is in line with #61100 extending the lint to tuples.

Fixes #39524.

cc @rust-lang/lang and @rust-lang/compiler for discussion in case this is a controversial change. In particular, we might want to consider allowing annotations on fields containing `#[must_use]` types in user-defined types (e.g. `#[allow(unused_must_use)]`) to opt out of this behaviour, if there are cases where we this this is likely to have frequent false positives.

(This is based on top of #62235.)
@varkor
Copy link
Member

varkor commented Oct 25, 2019

Closed as not being desirable in #62262 (comment). We would need a more nuanced handling of #[must_use], which would require an RFC.

@varkor varkor closed this as completed Oct 25, 2019
@jebrosen
Copy link
Contributor

I see this was rejected in general, but would it be doable to special case it for Pin the way it is for Box? It is not uncommon to have a Pin<Box<dyn Future>> especially when working with something like async-trait, but Pin does not propagate #[must_use]: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=51b7e0669d9d7462cd0345da4ec9dec8. This can be problematic when refactoring, because it's easy to forget to add the .awaits to every caller when changing functions to return pinned-boxed futures.

@varkor
Copy link
Member

varkor commented Dec 17, 2019

@jebrosen: that should be possible — could you open a new issue for this? I'll try to write up some mentoring advice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-feature-request Category: A feature request, i.e: not implemented / a PR. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants