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] on traits, e.g. on Future #55506

Closed
emilk opened this issue Oct 30, 2018 · 19 comments · Fixed by #62228
Closed

#[must_use] on traits, e.g. on Future #55506

emilk opened this issue Oct 30, 2018 · 19 comments · Fixed by #62228
Assignees
Labels
A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion.

Comments

@emilk
Copy link

emilk commented Oct 30, 2018

Putting #[must_use] on a trait is currently a no-op (it doesn't seem to do anything, but it is allowed). See example on playground.

One important use-case for this would be to have #[must_use] on std::Future. When returning impl Future<…> or Box<dyn Future<…>> from a function, it should lead to a warning if that future is not used, as that is almost certainly a bug.

@Centril Centril added the A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. label Oct 30, 2018
@Centril
Copy link
Contributor

Centril commented Oct 30, 2018

cc @varkor

@zackmdavis
Copy link
Member

(Also see previous discussion on #51560.)

bors added a commit that referenced this issue Nov 20, 2018
Allow #[must_use] on traits

Addresses #55506, but we'll probably want to add it to some library traits like `Iterator` before the issue is considered fixed. Fixes #51560.

`#[must_use]` is already permitted on traits, with no effect, so this seems like a bug fix, but I might be overlooking something. This currently warns for `impl Trait` or `dyn Trait` when the `Trait` is `#[must_use]` (although I don't think the latter is currently possible, so it's simply future-proofed).
@varkor
Copy link
Member

varkor commented Nov 20, 2018

This is now possible. It should be easy to make a pull request adding #[must_use] to Future (with a test).

We probably want to also consider what other traits would benefit from being #[must_use]. Iterator is a fairly obvious choice, for one.

@varkor varkor added E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. labels Nov 20, 2018
@F001
Copy link
Contributor

F001 commented Nov 22, 2018

Fn FnMut FnOnce are good candidates as well.

@Centril
Copy link
Contributor

Centril commented Nov 22, 2018

@F001 Can you elaborate? If Output = () then I probably don't want the warning...?

@F001
Copy link
Contributor

F001 commented Nov 22, 2018

@Centril Well, I think this warning means this:

let c : impl Fn(i32) = ... ;
c(); // `c` must be used. If `c` is discarded, it probably indicates something wrong.

let r = c(); // I think this attribute does not care `r` is used or not

@Centril
Copy link
Contributor

Centril commented Nov 22, 2018

@F001 Yeah that makes sense.

bors added a commit that referenced this issue Dec 10, 2018
#[must_use] on traits in stdlib

Based on #55506.

Adds `#[must_use]` attribute to traits in the stdlib:
- `Iterator`
- `Future`
- `FnOnce`
- `Fn`
- `FnMut`

There may be other traits that should have the attribute, but I couldn't find/think of any.
pietroalbini added a commit to pietroalbini/rust that referenced this issue Dec 12, 2018
#[must_use] on traits in stdlib

Based on rust-lang#55506.

Adds `#[must_use]` attribute to traits in the stdlib:
- `Iterator`
- `Future`
- `FnOnce`
- `Fn`
- `FnMut`

There may be other traits that should have the attribute, but I couldn't find/think of any.
pietroalbini added a commit to pietroalbini/rust that referenced this issue Dec 13, 2018
#[must_use] on traits in stdlib

Based on rust-lang#55506.

Adds `#[must_use]` attribute to traits in the stdlib:
- `Iterator`
- `Future`
- `FnOnce`
- `Fn`
- `FnMut`

There may be other traits that should have the attribute, but I couldn't find/think of any.
kennytm added a commit to kennytm/rust that referenced this issue Dec 13, 2018
#[must_use] on traits in stdlib

Based on rust-lang#55506.

Adds `#[must_use]` attribute to traits in the stdlib:
- `Iterator`
- `Future`
- `FnOnce`
- `Fn`
- `FnMut`

There may be other traits that should have the attribute, but I couldn't find/think of any.
kennytm added a commit to kennytm/rust that referenced this issue Dec 14, 2018
#[must_use] on traits in stdlib

Based on rust-lang#55506.

Adds `#[must_use]` attribute to traits in the stdlib:
- `Iterator`
- `Future`
- `FnOnce`
- `Fn`
- `FnMut`

There may be other traits that should have the attribute, but I couldn't find/think of any.
kennytm added a commit to kennytm/rust that referenced this issue Dec 14, 2018
#[must_use] on traits in stdlib

Based on rust-lang#55506.

Adds `#[must_use]` attribute to traits in the stdlib:
- `Iterator`
- `Future`
- `FnOnce`
- `Fn`
- `FnMut`

There may be other traits that should have the attribute, but I couldn't find/think of any.
pietroalbini added a commit to pietroalbini/rust that referenced this issue Dec 14, 2018
#[must_use] on traits in stdlib

Based on rust-lang#55506.

Adds `#[must_use]` attribute to traits in the stdlib:
- `Iterator`
- `Future`
- `FnOnce`
- `Fn`
- `FnMut`

There may be other traits that should have the attribute, but I couldn't find/think of any.
pietroalbini added a commit to pietroalbini/rust that referenced this issue Dec 15, 2018
#[must_use] on traits in stdlib

Based on rust-lang#55506.

Adds `#[must_use]` attribute to traits in the stdlib:
- `Iterator`
- `Future`
- `FnOnce`
- `Fn`
- `FnMut`

There may be other traits that should have the attribute, but I couldn't find/think of any.
@FrankSD
Copy link

FrankSD commented Dec 18, 2018

Has this issue been taken?

@varkor
Copy link
Member

varkor commented Dec 18, 2018

@FrankSD: this appears to have been completed in #56677. However, at least one of the traits could do with a more informative #[must_use] message as highlighted in https://github.com/rust-lang/rust/pull/56677/files#r241236020 (and perhaps the others could be improved too). So you're welcome to open a PR with that improvement!


I'm going to close this issue, though, as I think it's been completed satisfactorily.

@Nemo157
Copy link
Member

Nemo157 commented Jun 29, 2019

The OP mentions both impl Future and Box<dyn Future>. It appears the former has been implemented, but the latter doesn't warn still.

#[must_use]
trait Foo {}

impl Foo for () {}

fn foo() -> impl Foo {
    ()
}

fn bar() -> Box<dyn Foo> {
    Box::new(())
}


fn main() {
    foo();
    bar();
}

only gives (playground)

   Compiling playground v0.0.1 (/playground)
warning: unused implementer of `Foo` that must be used
  --> src/main.rs:16:5
   |
16 |     foo();
   |     ^^^^^^
   |
   = note: #[warn(unused_must_use)] on by default

    Finished dev [unoptimized + debuginfo] target(s) in 0.68s
     Running `target/debug/playground`

@varkor varkor self-assigned this Jun 29, 2019
@varkor
Copy link
Member

varkor commented Jun 29, 2019

This was an oversight on my part; I'll open up a PR soon to fix this.

@Centril
Copy link
Contributor

Centril commented Jun 29, 2019

It does apply to let x: dyn Future.

As for Box<dyn Trait> and #[must_use] it doesn't seem trivial to me unless you make it a special case for Box. We should think of a general solution for type constructors.

@varkor
Copy link
Member

varkor commented Jun 29, 2019

This is related to (or is maybe just a subissue of) #39524. It seems reasonable to special-case Box as a stop-gap and try to fix the whole issue at some point.

@Centril
Copy link
Contributor

Centril commented Jun 29, 2019

@varkor If you're going for a hack, an attribute based hack seems better, e.g.

pub struct Box<#[rustc_propagate_must_use] T: ?Sized>(Unique<T>);

@varkor
Copy link
Member

varkor commented Jun 29, 2019

@Centril: I'm not really going for a hack — Box is already a special-cased type and I'm simply going to add explicit handling for it into unused.rs. I think we'd want to special case it even if we had general support for unused components of structs, to give better error messages.

Centril added a commit to Centril/rust that referenced this issue Jun 29, 2019
…tril

Extend the #[must_use] lint to boxed types

Fixes rust-lang#55506 (comment) (cc @Nemo157).

This should have been included as part of rust-lang#55663, but was overlooked.
Centril added a commit to Centril/rust that referenced this issue Jun 30, 2019
…tril

Extend the #[must_use] lint to boxed types

Fixes rust-lang#55506 (comment) (cc @Nemo157).

This should have been included as part of rust-lang#55663, but was overlooked.
@andll
Copy link

andll commented Jul 1, 2019

@varkor is this going to propagate #[must_use] for Pin<Box<Future>>? Or just Box<Future>?

@varkor
Copy link
Member

varkor commented Jul 1, 2019

@andll: I don't think this will propagate for Pin<Box<Future>>, but I imagine #62262 would do.

@shepmaster
Copy link
Member

I'd like to bring this back up a bit.

I've been doing some introductory async work, and I've been bitten by the lack of hints to use a boxed future (futures::future::BoxFuture<'a, T> / Pin<Box<dyn Future<Output = T> + 'a + Send>>) multiple times. This is exacerbated because we cannot use async fn in traits, so boxing is a common workaround — the compiler error messages even suggest using a crate that automatically performs this rewriting!

@Nemo157
Copy link
Member

Nemo157 commented Jan 2, 2020

@shepmaster new issue for that appears to be #67387

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. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants