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::{ok,err} as #[must_use] #66116

Open
upsuper opened this issue Nov 5, 2019 · 5 comments
Open

Mark Result::{ok,err} as #[must_use] #66116

upsuper opened this issue Nov 5, 2019 · 5 comments
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@upsuper
Copy link
Contributor

upsuper commented Nov 5, 2019

In a discussion in our Rust group, someone incorrectly uses ok() without checking its return value, causing confusing issue. As Result::{is_ok, is_err} are already marked #[must_use] in #59610, I don't see any reason Result::{ok, err} shouldn't be.

cc #48926

@Mark-Simulacrum
Copy link
Member

Result::ok is an accepted style for "ignoring" results -- I'm not sure that's a good thing, but it's the reality today, so I don't think marking it as must_use is viable. Marking err but not ok would be quite odd, I expect, so we'd probably not want to do that either.

Personally I think until we come up with a proper pattern for "ignoring" must_use values and document that we shouldn't change things here. I've personally not found a solution -- let _ = result I'm not a fan of; result.ok(); I'm also not a big fan of personally, both look somewhat unnatural. Though maybe that's not a bad thing.

@Mark-Simulacrum Mark-Simulacrum added C-feature-request Category: A feature request, i.e: not implemented / a PR. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Nov 5, 2019
@upsuper
Copy link
Contributor Author

upsuper commented Nov 5, 2019

How is it "accepted"? What's the logic behind having it such style? How widely used it is?

I personally think let _ = is the general style for ignoring anything which otherwise would be warned for not being used. If you insist that there should be a method, adding something like .ignore() is probably an option as well.

@Mark-Simulacrum
Copy link
Member

I'm not saying that there should be a method, but I have definitely heard and probably read that .ok() is how some people work around the must_use lint today.

To be clear I'm not really opposed to this marking, I just am uncertain we should do it, as these methods are sometimes used in a way where the return type isn't "must use." Maybe that doesn't make much sense :)

@kornelski
Copy link
Contributor

I've used .ok() in a few places for silencing must_use, but I'd be happy to change that code.

.ok() by itself is not entirely clear. Did I mean to assert that the result is always OK? Did I mean to use the Option and forgot to bind it?

.ignore_result() or .ignore_err() would be clearer. If the language gets #[must_bind] to make let _ = safer, that'd be better too.

@edward-shen
Copy link
Contributor

I'm a big fan of .ignore(). Thinking about it, an implementation would basically just be

fn ignore(self) {}

which is reminds me of drop. Maybe drop should really be the conventional way to ignore a Result?

I think between an .ignore() and drop I would still prefer a convenience method (especially since it's postfix!) on Result (and if it's not clear, to also throw #[must_use] on .ok()).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR. 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