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

Mark Result::is_ok as #[must_use] #59610

Closed
alex opened this issue Apr 1, 2019 · 5 comments
Closed

Mark Result::is_ok as #[must_use] #59610

alex opened this issue Apr 1, 2019 · 5 comments
Labels
A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-enhancement Category: An issue proposing an enhancement or a PR with one. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@alex
Copy link
Member

alex commented Apr 1, 2019

I just ran into a project where Result::is_ok was accidentally being called without using the result -- I believe it had been confused with an assertion method like expect(). Had is_ok been marked #[must_use] this mistake would have been prevented.

@jonas-schievink jonas-schievink added C-enhancement Category: An issue proposing an enhancement or a PR with one. A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Apr 1, 2019
@shepmaster
Copy link
Member

Duplicate of #48926

@varkor
Copy link
Member

varkor commented Apr 1, 2019

@alex: feel free to open a pull request adding #[must_use] to is_ok.

@shepmaster
Copy link
Member

shepmaster commented Apr 2, 2019

@varkor doesn't that rather ignore the intent of the linked issue where the libs team is attempting to decide by what rationale functions should and should not have the annotation? Arbitrarily picking functions seems like a poor avenue to go down. For example:

  • why not also is_err?
  • What makes those two methods special compared to Result::ok?
  • Should it be every method in Result?
  • What about Option?
  • etc.

@varkor
Copy link
Member

varkor commented Apr 2, 2019

In the past, pull requests for individual methods have been accepted (e.g. #50177, #58145, #49533). The purpose of the tracking issue seems to me to decide on an overarching set of rules for #[must_use], but it seems clear that if a method is effectless and can be confused with an effectful variant, marking as #[must_use] is definitely what we want to do and waiting on a general guideline now just leaves the behaviour subpar (especially as the tracking issue has not seen much progress recently). Specifically in this situation, we have a concrete instance of a particular method being misused and causing practical problems.

I do think if a pull request was opened, it should include is_err. I agree broader questions should be left to the tracking issue. Maybe the best option would be to open a pull request and r? a libs team member, if there's any controversy.

@shepmaster
Copy link
Member

it seems clear that if a method is effectless and can be confused with an effectful variant, marking as #[must_use] is definitely what we want to do

It's probably worth attempting to say something to that effect on the tracking issue.

I do agree that it's a shame that it's moving so slow; you can even see some comments from me with issues I've seen in the wild.

Centril added a commit to Centril/rust that referenced this issue Apr 15, 2019
Add must_use annotations to Result::is_ok and is_err

Discussed in rust-lang#59610
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-enhancement Category: An issue proposing an enhancement or a PR with one. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants