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

Add unstable #[may_ignore] attribute to cancel #[must_use] #79572

Closed
wants to merge 2 commits into from

Conversation

m-ou-se
Copy link
Member

@m-ou-se m-ou-se commented Nov 30, 2020

This adds an unstable #[may_ignore] attribute, that can cancel the effect of a #[must_use] attribute. This is useful when the result of a function can be safely ignored, but it returns a #[must_use] type such as a Result.


Edit: Example usage of this: #78822

@rust-highfive
Copy link
Collaborator

r? @lcnr

(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 Nov 30, 2020
@@ -620,6 +620,9 @@ declare_features! (
/// Allows capturing disjoint fields in a closure/generator (RFC 2229).
(active, capture_disjoint_fields, "1.49.0", Some(53488), None),

/// Allow `#[may_ignore]` attribute.
(active, may_ignore, "1.50.0", Some(99999999), None),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// TODO: This still needs a real tracking issue.

@egilburg
Copy link

Soon to be followed by #[cannot_ignore] to cancel out the may_ignore attribute, which cancels out the must_use attribute? 😆 Is there a better approach than arms race between the function author and user? What is the actual motivation for must_use to be a semantic requirement rather than a suggestion?

@m-ou-se
Copy link
Member Author

m-ou-se commented Nov 30, 2020

This is just to be able to make the must_use warning a bit smarter. Rust warns about all ignored Results by default, but in some cases it's fine to ignore. This allows turning the warning off for specific functions.

Would you have any use case for #[cannot_ignore]?

@egilburg
Copy link

egilburg commented Nov 30, 2020

Not really, just a humorous reply. Perhaps it's actually valuable, in which case go ahead. It just reminds me of the cautionary tale of the "arms race" (anti) patterns in other languages, where code exist to suppress other code, instead of working through why the original code was set up that way to begin with. E.g. !important tag in CSS.

@Mark-Simulacrum
Copy link
Member

@m-ou-se One thing I'd like to clarify -- does this have transitive effects?

#[may_ignore]
fn bar() -> Result<usize, usize> { todo!() }

// is this must_use or may_ignore?
fn foo() -> Result<usize, usize> { bar() }

I am not sure which behavior is preferable myself, I think either could be reasonable..

@m-ou-se
Copy link
Member Author

m-ou-se commented Nov 30, 2020

@Mark-Simulacrum Same as the other way around, so not transitive:

#[must_use]
fn bar() -> usize { todo!() }

fn foo() -> usize { bar() } // not must_use
#[may_ignore]
fn bar() -> Result<usize, usize> { todo!() }

fn foo() -> Result<usize, usize> { bar() } // not may_ignore (so, must_use)

@jyn514 jyn514 added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Nov 30, 2020
Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you forbid may_ignore when applied to invalid items similar to what #79073 is doing?

We also aren't checking this for must_use :/
Would be nice to fix this in a different PR as that probably requires crater.

Not sure if someone needs to sign this off before merging, cc @rust-lang/lang

@Mark-Simulacrum Mark-Simulacrum added I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. and removed T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Nov 30, 2020
@the8472
Copy link
Member

the8472 commented Nov 30, 2020

Soon to be followed by #[cannot_ignore] to cancel out the may_ignore attribute, which cancels out the must_use attribute? 😆

Don't forget warnings about useless may_ignores applied to values that aren't must_use!

@lukaslueg
Copy link
Contributor

Soon to be followed by #[cannot_ignore] to cancel out the may_ignore attribute, which cancels out the must_use attribute? 😆

Don't forget warnings about useless may_ignores applied to values that aren't must_use!

I always compile with #[allow(probably_deny_may_ignore_allow_must_use_maybe)])

@scottmcm
Copy link
Member

scottmcm commented Dec 8, 2020

Some rationale here that came up in the triage meeting which I wanted to write down in the PR: This can't be solved by looking for uninhabited error types (like it could for u64::try_from(0_u32)) because this is desired mostly for cases where the method must (either because of a trait or backward compatibility) use a common error type that's always inhabited.

@joshtriplett
Copy link
Member

Bikeshedding about names aside, this seems reasonable to me.

There was some discussion in the meeting about whether it'd be feasible to apply this attribute to a specific impl of a trait, such that if you're in a non-generic context and using that specific impl, the compiler could avoid emitting the warning. That seems reasonable as well, assuming that it's implementable in the compiler.

@bors
Copy link
Contributor

bors commented Dec 10, 2020

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@nikomatsakis
Copy link
Contributor

Update from the lang meeting today:

We would like to see some examples of where this ought to be used. One of them that we came up with was when you have a trait that returns result, but you know the impl only returns Ok, but the PR needs an update to support that.

Are there other examples, @m-ou-se?

@nikomatsakis
Copy link
Contributor

Some concrete examples of this pattern:

  • Implementations of std::io::Write for things like String or what have you.

@m-ou-se m-ou-se added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed I-nominated S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 5, 2021
@camelid camelid added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 22, 2021
@JohnCSimon JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 14, 2021
@Dylan-DPC-zz
Copy link

@m-ou-se any updates?

@crlf0710 crlf0710 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 5, 2021
@JohnCSimon JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 22, 2021
@Dylan-DPC-zz
Copy link

closing this due to inactivity.

@Dylan-DPC-zz Dylan-DPC-zz added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.