It is too easy to lose #[must_use] annotations when using impl Trait, leading to footguns

If I return a direct type that is must_use, I get a helpful compiler warning:

use std::iter::{self, Empty, Skip};

fn x() -> Skip<Empty<i32>> {
    iter::empty().skip(1)
}

fn main() {
    x();
}

warning: unused `std::iter::Skip` which must be used
 --> src/main.rs:8:5
  |
8 |     x();
  |     ^^^^
  |
  = note: #[warn(unused_must_use)] on by default
  = note: iterator adaptors are lazy and do nothing unless consumed

The equivalent (and I believe preferred) code using impl Trait gives no such useful feedback:

use std::iter;

fn x() -> impl Iterator<Item = i32> {
    iter::empty().skip(1)
}

fn main() {
    x();
}

Amusingly, this isn't a new problem, as returning a boxed trait object also has the same problem. It's arguably worse there because you've performed an allocation for no good reason, but it's also less likely because people are more reluctant to introduce unneeded allocations.

Since Rust 1.27 stabilizes #[must_use] (on functions), one possible solution would be a lint that checks the compiler-known type underneath the impl Trait and the function returning it. If the type has the annotation but the function does not, it could suggest adding the attribute to the function.

I think it would make sense for @rust-lang/lang to quickly review and agree on how we should handle this.

A lint seems plausible. Inside the same module, where we know the concrete underlying type, it would definitely make sense to pass through the #[must_use]. I'd love to know if it makes sense to automatically pass through the #[must_use] on the concrete type when used from outside the module, so that this can work automatically without needing a lint and a manual additional #[must_use] on the function.

Why is the #[must_use] not on, say, Iterator?

@eddyb

I agree with @eddyb that #[must_use] traits seem like the better solution here than trying to "leak" must-used-ness.

In that case, is it fine that returning an impl Debug of real type Result will drop the must-used-ness?

Maybe we should suggest to the author of the function to add the attribute to the function if the real type is #[must_use]

@oli-obk

@oli-obk Right, there's nothing meaningfully tying such a function to Result, if Result is hidden.

Since Rust 1.27 stabilizes #[must_use]

The new thing being stabilized in 1.27 is specifically #[must_use] on functions and methods (RFC 1940). #[must_use] on types, including the iterator-adapter structs like Skip, are much older.

We may need to add an arm for TyAnon in this match?

rust/src/librustc_lint/unused.rs

Lines 61 to 72 in 5205ae8

    
           let ty_warned = match t.sty { 
        
               ty::TyTuple(ref tys) if tys.is_empty() => return, 
        
               ty::TyNever => return, 
        
               ty::TyAdt(def, _) => { 
        
                   if def.variants.is_empty() { 
        
                       return; 
        
                   } else { 
        
                       check_must_use(cx, def.did, s.span, "") 
        
                   } 
        
               }, 
        
               _ => false, 
        
           };

So, I'd absolutely support adding #[must_use] for traits, which would have the same effect on a function returning -> impl Trait as #[must_use] on a type T would have for a function returning -> T.

However, I also think we ought to handle this case somehow, preferably in a way that doesn't require the user to manually propagate #[must_use] outward on every -> impl Trait function that returns a #[must_use] type.

#[must_use]
struct S { ... }

impl Trait for S { ... }

fn foo() -> impl Trait {
}

@joshtriplett

@joshtriplett I don't think we should. The type, including its must-use'dness, is not a part of the public API in that context. If you want the function return to be treated as must-use, the solution in that case is to tag the function as must-use.

@withoutboats

@withoutboats Then in that case, would it make sense to have a lint suggesting propagation of #[must_use] from the type to the function that returns that concrete type?

@joshtriplett

@joshtriplett I don't think so. The point of returning impl Trait is to hide the return type and the facts of its API, the fact that the type is must-use seems no different from other facts about the return type.

As alluded to above, I argue that this is a bug in the must-use lint due to its antedating impl-Trait; I don't see sufficient cause here to complicate the language with another lint or feature. A pull request is forthcoming.

As alluded to above, I argue that this is a bug in the must-use lint due to its antedating impl-Trait

Hm, perhaps this was too confidently worded: see description of the new pull request #51571.

So let me summarize to see if I understand correctly what you're planning for must_use for traits:

#[must_use]
struct Foo {}

#[must_use]
trait MustUseTrait {}
impl MustUseTrait for Foo {}

trait CanUseTrait {}
impl CanUseTrait for Foo {}

fn get_foo() -> Foo { Foo {} }
fn get_must_use_trait() -> impl MustUseTrait { Foo {} }
fn get_can_use_trait() -> impl CanUseTrait { Foo {} }

fn main() {
    get_foo(); // Warning (because of must_use on Foo and MustUseTrait)
    get_must_use_trait(); // Warning (because of must_use on MustUseTrait)
    get_can_use_trait(); // No warning (must_use on Foo does not matter because the type is anonymous)
}

@zackmdavis

I think expanding the applicability of must_use should probably be introduced using an RFC. It seems to me that there are two different approaches proposed and an RFC could clarify why we chose a certain approach:

@zackmdavis proposes to evaluate the lint using the concrete type and provided a PR to this effect.
This is incompatible with @withoutboats statement with which I agree with: "The type, including its must-use'dness, is not a part of the public API in that context [i.e. return impl Trait]". I also agree with @cramertj that must_use for traits is something we should investigate and pursue. This would be extremely helpful for futures because forgetting to poll needs to be obvious.

@withoutboats

@withoutboats Then in that case, would it make sense to have a lint suggesting propagation of #[must_use] from the type to the function that returns that concrete type?

@joshtriplett I don't think so. The point of returning impl Trait is to hide the return type and the facts of its API, the fact that the type is must-use seems no different from other facts about the return type.

Can you provide an example of a case where it is the correct thing to return a concrete type that is tagged as #[must_use] through impl Trait and not use the result of the function?

My original premise is that this is an unintentional mistake in 95-99% of the cases, which feels like the ideal case for a lint. In the (rare) case where the author of the impl Trait-returning function deliberately wants to allow forgetting to use the return value, they can allow the lint.

I don't know exactly which "other facts" you mean, but I can think of two primary things:

"sub" traits, which are not automagically propagated:

fn x() -> impl Iterator { [42].iter() }
fn go_backwards<T: DoubleEndedIterator>(_: T) {}
go_backwards(x());

Auto traits, which are automagically propagated:

fn x() -> impl std::fmt::Debug { 42 }
fn send_it<T: Send>(_: T) {}
send_it(x());

I think that the must_use case is different from the "sub trait" case because you will rather quickly run into "well, my code doesn't compile" errors when you forget to return the "sub trait". You then have to decide if you meant to expose that interface.

I know the auto trait aspect was much discussed, so I don't wish to re-litigate it. My point here is that we made the decision to automatically pass them for ergonomics:

Ergonomics: Trait objects already have the issue of explicitly needing to declare Send/Sync-ability, and not extending this problem to abstract return types is desirable. In practice, most uses of this feature would have to add explicit bounds for OIBITS if they wanted to be maximally usable.

I think there's a case to be made that #[must_use] is already an ergonomics-based feature as it helps prevent the user from writing code they didn't mean to.

As a concrete side note, I opened this because I did it myself: I wrote some code that returned impl Future<...> and was confused for a minute because the compiler didn't have any warnings. I'm used to those warnings and they are part of my development flow. Having them "suddenly go missing" just because I wanted to use impl Trait felt like trying to step on the last stair that isn't there.

For me, it's right up there with #36375 in that my "normal" flow of programming that I've established since ~Rust 0.12 doesn't quite work the same with impl Trait.

@shepmaster

@shepmaster IMO the more conservative approach would be to not leak must_use on the concrete type. If that turns out to be the wrong call, it can always be added later. I think defining must_use on traits (e.g. on Future and Iterator) will already be enough to catch all the bugs we're trying to prevent.

The big advantage of defining must_use on the trait is that this system can also work for unnameable types like the return type of an async function. Additionally it also avoids all the boilerplate code that defines each concrete iterator or future type as much_use.

@shepmaster

@shepmaster I think the current approach of "stick #[must_use] on the dozens of types implementing Iterator and Future" just doesn't scale and this is an example where it goes wrong.
Also consider type parameters or associated types that are known implement those traits.
IMO they should get the same treatment by #[must_use], without knowing the concrete type.

To be clear, I'm totally happy with implementing #[must_use] for traits, but that particular option has been available "forever" but doesn't seem to be implemented for whichever reason. On the flip side, @zackmdavis was able to get something in the ballpark of a lint fairly quickly.

I really just don't want to see this particular feature held up by a months (years?) long discussion of the ideal Platonic implementation of #[must_use] for traits.

the more conservative approach would be to not leak must_use on the concrete type

Isn't that what the current implementation does? Am I missing something?

it can always be added later.

I believe that's what I'm proposing... 😕

this system can also work for unnameable types like the return type of an async function

avoids all the boilerplate code

the current approach [...] just doesn't scale

consider type parameters or associated types

Yes, I 💯 believe that that's a better option, and I'll add one more: presumably it will also work for trait objects. I just don't want to lose what existing ergonomics we have until someone implements #[must_use] for traits.

These don't even have to be mutually exclusive options! The lint can catch cases that a #[must_use] trait wouldn't — such as when a given instance of a trait should be must use but not the trait itself.

The lint could even be removed once the better solution is created, if we felt it didn't carry its weight anymore.

I also want to suggest a more aggressive approach: existential impl Trait is always #[must_use] no matter of its underlying type. If a function is returning an impl Trait thing, you were probably meant to use it in some way.

existential impl Trait is always #[must_use]

😮 I wonder how hard it would be to turn such a thing on to see the impact. 😨

@shepmaster

@shepmaster Thanks for the compelling argument! I agree with what I think is the implication of your post: this question boils down to "Is #[must_use] more like an auto trait, or a regular API fact?" I'm not sure of the answer.

@oli-obk What is the distinction with impl Trait vs any other return type? Isn't it plausible that something that returns impl Trait also has a useful side effect, and the return is not relevant all the time?

Navigation Menu

Repository navigation

It is too easy to lose `#[must_use]` annotations when using impl Trait, leading to footguns #51560

1 remaining item

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	let ty_warned = match t.sty {
	ty::TyTuple(ref tys) if tys.is_empty() => return,
	ty::TyNever => return,
	ty::TyAdt(def, _) => {
	if def.variants.is_empty() {
	return;
	} else {
	check_must_use(cx, def.did, s.span, "")
	}
	},
	_ => false,
	};

It is too easy to lose #[must_use] annotations when using impl Trait, leading to footguns #51560

Description

Activity

shepmaster commented on Jun 14, 2018

joshtriplett commented on Jun 14, 2018

eddyb commented on Jun 14, 2018

cramertj commented on Jun 14, 2018

oli-obk commented on Jun 14, 2018

eddyb commented on Jun 14, 2018

zackmdavis commented on Jun 14, 2018

zackmdavis commented on Jun 14, 2018

joshtriplett commented on Jun 14, 2018

withoutboats commented on Jun 14, 2018

joshtriplett commented on Jun 14, 2018

withoutboats commented on Jun 14, 2018

zackmdavis commented on Jun 15, 2018

1 remaining item

zackmdavis commented on Jun 15, 2018

MajorBreakfast commented on Jun 15, 2018

MajorBreakfast commented on Jun 15, 2018

shepmaster commented on Jun 15, 2018

shepmaster commented on Jun 15, 2018

MajorBreakfast commented on Jun 15, 2018

eddyb commented on Jun 15, 2018

shepmaster commented on Jun 15, 2018

oli-obk commented on Jun 15, 2018

shepmaster commented on Jun 15, 2018

withoutboats commented on Jun 15, 2018

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

It is too easy to lose `#[must_use]` annotations when using impl Trait, leading to footguns #51560