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

Warn using last and count to exhaust an iterator #78225

Closed
wants to merge 4 commits into from

Conversation

Kestrer
Copy link
Contributor

@Kestrer Kestrer commented Oct 22, 2020

This adds the same #[must_use] attribute as used in collect to count and last, as it also does not make sense to not use the result there.

@rust-highfive
Copy link
Collaborator

r? @shepmaster

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 22, 2020
@m-ou-se
Copy link
Member

m-ou-se commented Oct 22, 2020

There's some discussion in #48926 about when to use #[must_use] in the standard library. The functions changed in this PR (count and last) don't seem to fall into the category of functions where ignoring the result definitely indicates a bug (like s.to_uppercase();) or there is a more efficient/better version if you don't need the result (like mem::replace). For now, applying #[must_use] is done conservatively in the standard library, until the discussion on #48926 comes to a conclusion.

Marking this as blocked on #48926.

@m-ou-se m-ou-se added A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. S-blocked Status: Blocked on something else such as an RFC or other implementation work. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 22, 2020
@Kestrer
Copy link
Contributor Author

Kestrer commented Oct 22, 2020

Wouldn't count/last be less efficient, as the operation performed needs to store state, or is it so trivial that we can rely on the compiler to optimize it out?

@m-ou-se
Copy link
Member

m-ou-se commented Oct 22, 2020

Wouldn't count/last be less efficient, as the operation performed needs to store state, or is it so trivial that we can rely on the compiler to optimize it out?

My guess is that yes, it'd get optimized out. The #[must_use] for mem::replace is not just there because of the resulting efficiency, but also because *a = 1 is less verbose and conceptually much simpler than mem::replace(a, 1). One could argue that .for_each(drop) is conceptually simpler than .count(), but the difference is much smaller. So I'll leave this discussion to the linked issue for now. Feel free to add your thoughts about #[must_use] to that issue. I'm not rejecting your PR (I personally think it'd be a good addition!), but it'd be better to discuss it in the bigger context of #[must_use] in std. (See also #52201 and #50462.)

By the way: the CI is failing because the expected test output still needs to be updated. You can do that by running ./x.py tests src/test/ui --bless (and --stage=1 --keep-stage=0 is probably useful to make it fast).

@Kestrer
Copy link
Contributor Author

Kestrer commented Oct 22, 2020

Should I close this or leave it open as blocked?

@jyn514 jyn514 added the A-iterators Area: Iterators label Oct 22, 2020
@jyn514
Copy link
Member

jyn514 commented Oct 22, 2020

It might make sense to make this a clippy lint instead.

@scottmcm
Copy link
Member

scottmcm commented Nov 8, 2020

I have a fuzzy memory that @withoutboats may have had thoughts on one of these specifically...

@bors
Copy link
Contributor

bors commented Jan 23, 2021

☔ The latest upstream changes (presumably #76391) made this pull request unmergeable. Please resolve the merge conflicts.

@shepmaster
Copy link
Member

r? @m-ou-se

@rust-highfive rust-highfive assigned m-ou-se and unassigned shepmaster Mar 8, 2021
@dtolnay dtolnay added I-needs-decision Issue: In need of a decision. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Oct 3, 2021
@dtolnay
Copy link
Member

dtolnay commented Oct 3, 2021

@rustbot label -S-blocked +S-waiting-on-team

Marking waiting-on-team because the discussion in #48926 appears to have reached a stopping point a while ago.

@m-ou-se
Copy link
Member

m-ou-se commented Oct 27, 2021

This was briefly discussed in the libs api meeting recently. See rust-lang/libs-team#35 for the guideline we're using now for #[must_use]. The additions in this PR fall outside of the criteria listed there, so I'm closing this.

Feel free to continue the discussion on #48926.

@m-ou-se m-ou-se closed this Oct 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-iterators Area: Iterators A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. I-needs-decision Issue: In need of a decision. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants