Allow a message on #[must_use] & mark iterator adaptor structs with it

[huonw]

Similar to the stability attributes, a type annotated with #[must_use = "informative snippet"] will print the normal warning message along with
"informative snippet". This allows the type author to provide some
guidance about why the type should be used.

---

It can be a little unintuitive that something like v.iter().map(|x| println!("{}", x)); does nothing: the majority of the iterator adaptors
are lazy and do not execute anything until something calls next, e.g.
a for loop, collect, fold, etc.

The majority of such errors can be seen by someone writing something
like the above, i.e. just calling an iterator adaptor and doing nothing
with it (and doing this is certainly useless), so we can co-opt the
must_use lint, using the message functionality to give a hint to the
reason why.

Fixes #14666.

[lilyball]

👍 though it seems a shame that the same message must be repeated for every single iterator adaptor.

[huonw]

Long term, it may be better to make it specific to each one "the map iterator adaptor is lazy and does nothing unless consumed" etc, so the repetition may disappear. (I can do this now, if people think it would be an improvement.)

[lilyball]

Any chance we could put #[must_use = "iterators are lazy and do nothing unless consumed"] on trait Iterator and have the lint check those as well? (although checking attributes on all implemented traits is of course more expensive, though I don't know if it's enough to make a noticeable difference). That would prevent the duplication on all iterator adaptors, as well as make any future iterator adaptors transparently work with this lint without any effort.

[huonw]

It's not guaranteed that every Iterator needs to be consumed, i.e. the function returning it may do a pile of extra work, and return a secondary iterator just in case someone wants to use it. (This may be such an edge case that it doesn't matter.)

However, even just implementing it would require somehow searching all the traits that a type implements (theoretically including ones that aren't in scope). I don't actually know if this is possible atm.

[lilyball]

True, not every Iterator is necessarily supposed to be used, although I don't think I've ever run into a case like you describe where that's actually true in practice. And it's trivial enough to explicitly ignore the warning anyway. It wouldn't surprise me if it's more common to ignore a Result value than to ignore an Iterator.

Regarding the implementation, I'm not particularly familiar with that part of the compiler, so I can only make guesses. I would assume the type info for a type contains a listing of all implemented traits, not merely the ones in scope, but it's certainly possible for that to be wrong.

[huonw]

AFAIK, there's a mapping trait -> implementors, but not vice versa.

[lilyball]

You could perhaps do a single pass through all traits to collect the ones that have a must_use attribute, and use that to build a list all types that implement the must_use traits.

This assumes, of course, that supporting must_use on a trait is indeed the right approach. I think it's got a number of benefits though.

[huonw]

IMO, that can be done later. The #[must_use]s here are still a strict incremental improvement and address all the instances of iterators being misused that I can remember seeing (well, really, I've only ever seen .map called without being consumed, so I could add it to only that if the repetition is particularly hateful).

[lilyball]

True. And no, I think if we're just going to start with this approach, you should do all the adaptors. After all, if you only do map(), and someone slaps a .take(5) on it, you'd still expect the warning.

[lilyball]

Actually, that suggests that perhaps type parameters should be inspected for must_use types. Right now an Option<Result<T,U>> does not get the must_use warning. If you inspected all type parameters, that would let you then just annotate Map (and perhaps the other adaptors that take closures) and it would work even when subsequently adapted by e.g. .take(5).

Of course, this too involves looking up attributes on more types, and I wonder how expensive this ends up being. Although it shouldn't be hard to create a cache so you don't have to look up a type twice if it does end up being significant.

[alexcrichton]

Out of curiosity, do you see us expanding the #[must_use] annotation to all Iterator types, even those outside of the std::iter module?

[lilyball]

@alexcrichton I think it's reasonable to expand it to all Iterator types, though it's not something we necessarily need to do. One benefit of doing that is third-party iterator adaptors would implicitly gain the must_use annotation, which is something that would be beneficial. It's nicer if users can simply know that the compiler will catch all unused iterators instead of just the subset of them defined as adaptors.

[huonw]

@alexcrichton I thought about it, but think it's more obvious that vec.iter() doesn't do anything (i.e. most other iterator types are just "get me something to iterate", while the iterator adaptors could be doing real work, and in languages like Python and Ruby, the equivalent functions (historically) did do real work immediately, i.e. immediately consumed their argument and returned a list.).

[lilyball]

@huonw What about Unfold::new(st, |st| do_something())? Yeah you probably should realize that doesn't do anything unless iterated either, but there's still some potential for confusion.

Although I think potential third-party adaptors are the most compelling reason to add must_use to all Iterators.

[alexcrichton]

I'm going to r+ due to its containment to the std::iter module, I do not want to start spraying #[must_use] all over the place just yet. I think @huonw has a very good point that foo.iter() pretty obviously doesn't do anything.

[lilyball]

Quite reasonable. I'm trying to promote the idea that we should come up with a solution that allows us to annotate Iterator itself with #[must_use], but until such a universal solution is implemented, I agree that limiting the replication of #[must_use] annotations to the iter module (and more specifically, the adaptors) is a good idea.