add must_use to extract_if methods #148995
Conversation
rustbot
added
S-waiting-on-review
| /// assert_eq!(high.keys().copied().collect::<Vec<_>>(), [4, 5, 6, 7]); | ||
| /// ``` | ||
| #[stable(feature = "btree_extract_if", since = "1.91.0")] | ||
| #[must_use = "use `extract_if().for_each(drop)` to remove all extracted elements"] |
There was a problem hiding this comment.
The wording we have on most iterators is simply
#[must_use = "iterators are lazy and do nothing unless consumed"]
It's possible that the user meant to use the iterator results but forgot to somehow.
for_each(drop) is only recommended by an unused collect()
There was a problem hiding this comment.
The drain iterator is also lazy and still does something when not consumed... so if the user knows drain, which is likely, then just talking about the laziness of iterators in general is not enough for this specific case IMO. I think it makes sense to tell people how to use this as a drain-like API.
There was a problem hiding this comment.
Drain is an exception and we deliberately renamed it from DrainFilter to ExtractIf to avoid that association.
I guess you could add the general "iterators are lazy" statement and follow up with a "use retain or for_each(drop)" after that.
Generally we don't want to encourage for_each(drop) too much because it can indicate misuse such as iter.inspect(side_effect).for_each(drop) instead of iter.for_each(side_effect). But in this case it's a legitimate use, due to lack of better alternatives.
When we have a better alternative we can remove the for_each part of the hint.
There was a problem hiding this comment.
I have updated the wording:
#[must_use = "this iterator is lazy and does nothing unless consumed; \
use `retain` or `extract_if().for_each(drop)` to remove and discard elements"]
I went for "this iterator" to make it more clear that this applies to extract_if despite potentially conflicting expectations raised by drain. (I also considered something like "the iterator returned by extract_if" but found that too wordy.)
RalfJung
force-pushed
the
extract_if
branch
3 times, most recently
from
59d5167 to
0604669
Compare
| /// ``` | ||
| #[stable(feature = "btree_extract_if", since = "1.91.0")] | ||
| #[must_use = "this iterator is lazy and does nothing unless consumed; \ | ||
| use `retain` or `extract_if().for_each(drop)` to remove and discard elements"] |
There was a problem hiding this comment.
Should we remind people that retain needs the predicate to be negated?
Alternatively we could only mention the for_each variant here. We do still mention retain in the docs for people that have a closer look.
There was a problem hiding this comment.
must_use hints seem to be terse in general. I assume this is to not bloat diagnostics, so let's keep it this way.
| #[must_use = "this iterator is lazy and does nothing unless consumed; \ | ||
| use `extract_if().for_each(drop)` to remove and discard elements"] | ||
| pub fn extract_if<F>(&mut self, pred: F) -> ExtractIf<'_, K, V, F> |
There was a problem hiding this comment.
The functions without range arguments should only point to retain.
There was a problem hiding this comment.
Ah right, I forgot the Hash* collections have those
There was a problem hiding this comment.
FWIW personally I would probably still use extract_if in situations where the negation enforced by retain is awkward... in most of these cases, I want to characterize the elements to be removed, and then extract_if(...).for_each(drop) might beat retain(with-negation).
|
The |
Good point! I did that. |
4 participants
Also, mention the
.for_each(drop)pattern in the documentation. One can't always useretainwith a negated predicate as that does not have a range argument.r? @the8472