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.

Sign up for GitHub

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

Jump to bottom

Tracking issue for RFC 1940: allow #[must_use] on functions #43302

Closed
3 tasks done
aturon opened this issue Jul 17, 2017 · 43 comments
Closed
3 tasks done

Tracking issue for RFC 1940: allow #[must_use] on functions #43302

aturon opened this issue Jul 17, 2017 · 43 comments
Labels
B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@aturon
Copy link
Member

aturon commented Jul 17, 2017 •
edited by Centril
Loading

This is a tracking issue for the RFC "allow #[must_use] on functions" (rust-lang/rfcs#1940).

Steps:
@aturon aturon added B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Jul 17, 2017
@aturon aturon mentioned this issue Jul 17, 2017
Merged
@Mark-Simulacrum Mark-Simulacrum added the C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC label Jul 27, 2017
@zackmdavis
Copy link
Member

I think I should have an implementation shortly.

@zackmdavis zackmdavis mentioned this issue Aug 8, 2017
Merged
zackmdavis added a commit to zackmdavis/rust that referenced this issue Aug 8, 2017
@zackmdavis
#[must_use] for functions (RFC 1940)
3645b06 
The return value of a function annotated with `must_use`, must be used.

This is in the matter of rust-lang#43302.
zackmdavis added a commit to zackmdavis/rust that referenced this issue Aug 8, 2017
@zackmdavis
mark comparison trait methods as #[must_use]
f5ac228 
Note that this doesn't actually give us warnings on `a == b;` and the like, as
some observers may have hoped.

This is in the matter of rust-lang#43302.
bors added a commit that referenced this issue Aug 9, 2017
@bors
Auto merge of #43728 - zackmdavis:fnused, r=eddyb
3f977ba 
#[must_use] for functions

This implements [RFC 1940](rust-lang/rfcs#1940).

The RFC and discussion thereof seem to suggest that tagging `PartialEq::eq` and friends as `#[must_use]` would automatically lint for unused comparisons, but it doesn't work out that way (at least the way I've implemented it): unused `.eq` method calls get linted, but not `==` expressions. (The lint operates on the HIR, which sees binary operations as their own thing, even if they ultimately just call `.eq` _&c._.)

What do _you_ think??

Resolves #43302.
@crumblingstatue
Copy link
Contributor

crumblingstatue commented Aug 9, 2017 •
edited
Loading

I'm just adding a note here that the current implementation of #1940 does not lint for unused ==, which was the main motivation for it.

In fact, the RFC itself states in the motivation

By marking PartialEq #[must_use] the compiler would complain about things like:

 modem_reset_flag == false; //warning
 modem_reset_flag = false; //ok

@zackmdavis
Copy link
Member

Yeah, that's a little awkward. I can add the comparison operators in a follow-on PR.

@eddyb
Copy link
Member

eddyb commented Aug 9, 2017

Also, this issue shouldn't have been closed. Ahhh I may have not checked for a feature gate in the implementation. I'm a bad reviewer.

@eddyb eddyb reopened this Aug 9, 2017
@zackmdavis zackmdavis mentioned this issue Aug 9, 2017
Merged
@zackmdavis
Copy link
Member

I have an implementation for linting == and the other comparison operators. (It's based off the feature-gating work, so we'll let that land first.)

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Apr 1, 2018
@rfcbot
Copy link

rfcbot commented Apr 11, 2018

The final comment period is now complete.

@TimNN
Copy link
Contributor

TimNN commented Apr 17, 2018

@nikomatsakis: It looks like it was decided to stabilize this, can I assume that #48925 is now un-blocked?

@varkor varkor mentioned this issue Apr 20, 2018
Closed
@zackmdavis zackmdavis mentioned this issue Apr 22, 2018
Merged
@nikomatsakis
Copy link
Contributor

@TimNN yep

@zackmdavis
Copy link
Member

@SimonSapin @nikomatsakis

Can we make this attribute a compilation error when used in places where it would be a no-op? This leaves the option of making it work later without changing the meaning of programs that compile.

I'm confused about how exactly how literally we take our stability guarantee. #[must_use] on trait-impls (and other non-ADT locations) is already a no-op on stable; are we allowed to break programs with such attributes by making it an error?

This seems like it ought to be sheerly hypothetical—why would anyone have already written a program with such no-op attributes? But the fn_must_use feature gate ended up emitting a warning rather an error (which took some extra work) for precisely this reason, which warnings turned out to reveal at least one crate in the wild that was trying to use the #[must_use] attributes even though they were no-oping.

@SimonSapin
Copy link
Contributor

#[must_use] on trait-impls (and other non-ADT locations) is already a no-op on stable

This is the situation I was hoping to prevent, I didn’t realize it was already the case.

@Havvy
Copy link
Contributor

Havvy commented Apr 26, 2018

We can make it a hard error on the next edition then.

zackmdavis added a commit to zackmdavis/rust that referenced this issue Apr 29, 2018
@zackmdavis
stabilize #[must_use] for functions and must-use operators
3dbdccc 
This is in the matter of RFC 1940 and tracking issue rust-lang#43302.
@Havvy
Copy link
Contributor

Havvy commented May 15, 2018

@aturon The stabilization PR has already gone through. I'm not sure if all the necessary docs are updated yet, although the reference has been.

@varkor varkor mentioned this issue May 23, 2018
Closed
@Centril Centril added disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels May 24, 2018
@Centril
Copy link
Contributor

Centril commented Oct 31, 2018

@Havvy is there anything left here to do?

@Havvy
Copy link
Contributor

Havvy commented Nov 4, 2018

I don't know. It's documented in the reference and there's an issue for the API guidelines book. But I don't think it's talked about in TRPL and definitely untouched in Rust By Example, if it should even be in there.

@Centril Centril mentioned this issue Nov 10, 2018
Closed
@Centril
Copy link
Contributor

Centril commented Nov 10, 2018

Closing as completed.

@Centril Centril closed this as completed Nov 10, 2018
luojia65 added a commit to luojia65/smoltcp that referenced this issue Oct 27, 2021
@luojia65
Add lint #[must_use] for ring buffer functions.
bd1d6da 
This lint is in stable Rust now: rust-lang/rust#43302.
These changes are noted according to code comment from @whitequark.
Some test cases and functions are altered due to changes of #[must_use].
@luojia65 luojia65 mentioned this issue Oct 27, 2021
Merged
bors bot added a commit to smoltcp-rs/smoltcp that referenced this issue Nov 3, 2021
@bors @luojia65
Merge #561
24fa299 
561: Add lint `#[must_use]` for ring buffer functions r=Dirbaio a=luojia65

This pull request adds `#[must_use]` flag on several ring buffer functions that have side-effects.

This lint is in stable Rust now: rust-lang/rust#43302, these changes are noted according to code comment from `@whitequark:` f65bc8a.

Some test cases and functions are altered due to changes of `#[must_use]`.

Co-authored-by: luojia65 <me@luojia.cc>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests