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

why havn't implemented Error trait for std::option::NoneError ? #46871

Closed
ZhangHanDong opened this issue Dec 20, 2017 · 20 comments · Fixed by #85482
Closed

why havn't implemented Error trait for std::option::NoneError ? #46871

ZhangHanDong opened this issue Dec 20, 2017 · 20 comments · Fixed by #85482
Labels
A-error-handling Area: Error handling 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

@ZhangHanDong
Copy link

Forgot it? Still for other reasons.

@ZhangHanDong ZhangHanDong changed the title why have not std::option::NoneError implemented Error trait? why havn't std::option::NoneError implemented Error trait? Dec 20, 2017
@ZhangHanDong ZhangHanDong changed the title why havn't std::option::NoneError implemented Error trait? why havn't implemented Error trait for std::option::NoneError ? Dec 20, 2017
@kennytm kennytm 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 Dec 20, 2017
@kennytm
Copy link
Member

kennytm commented Dec 20, 2017

See also: https://internals.rust-lang.org/t/should-noneerror-implement-the-error-trait/6312

@ZhangHanDong
Copy link
Author

@kennytm thanks

@dlight
Copy link

dlight commented Dec 13, 2018

It seems to me that if NoneError should exist at all, then it should implement Error, otherwise Box<Error> doesn't work.

@aminroosta
Copy link

Facing the same issue in 2019 :/

@Coder-256
Copy link
Contributor

@kennytm After looking through the discussion on #42526, I’m getting the impression that it is indeed an oversight that there is no way to convert NoneError to Box<Error> so that you can use optional? on a function that returns Result<T, Box<Error>>.

@skade
Copy link
Contributor

skade commented Sep 20, 2019

This becomes a prevalent problem in the trainings I give, when people learn about: Box<dyn std::error::Error>, because they also try to coerce optionals.

@bbqsrc
Copy link

bbqsrc commented Oct 18, 2019

Can this be prioritised? It would be upsetting if this were to be forgotten, particularly since std::error::Error is quite useful now.

@Mark-Simulacrum
Copy link
Member

I don't believe this has been forgotten or de-prioritized -- at the very least, based on me re-read of #42327, I get the impression that the general feeling is that the Try trait design is not ready and Option being convertible to Result via ? is not something we are sure we want.

In particular, these comments are relevant:

@Coder-256
Copy link
Contributor

I actually changed my mind on this one. Now that I think about it, it only really makes sense to use ? on an Option if the enclosing function returns an Option. Otherwise if you want to return a Result you can use .ok_or("message") to indicate what went wrong. The error message could be much better though.

@scottmcm
Copy link
Member

scottmcm commented Oct 18, 2019

I agree about the error message. I think one thing we missed in the Try trait design discussion was what the errors for mixing should look like -- right now you get the note about NoneError, but with the other possible design you would have explicitly gotten something about Result: !Try<Option>.

@bbqsrc
Copy link

bbqsrc commented Oct 18, 2019

I'll note that I think the existence of a NoneError is a bit strange and a bit of a code smell, and another design should be considered. But if it isn't considered, make sure std::error::Error is implemented at least, haha.

@nixpulvis
Copy link

nixpulvis commented Feb 25, 2020

If NoneError is to exist, then it should implement Error, otherwise a better name should be chosen for the ? try trait result.

But generally, I think I'd like to be able to use ? in functions returning Result on either Result or Option types (without explicit conversion), while only allowing use on Option types in functions that return Option. Does this make sense?

@jonas-schievink jonas-schievink added the A-error-handling Area: Error handling label Apr 5, 2020
@ZhangHanDong
Copy link
Author

I don't think should implement Try for Option<T>.

None is not an error, it is just a state that may cause errors.

If Option does not implement Error, then try block does not need Ok-wrapping.

If you don't have any ? inside, then with ok-wrapping there is no real information about what the "wrapper type" should be (i.e., should try { 3 } have the type Option or Result<i32, _>, and what should _ be?)
from niko

@ZhangHanDong
Copy link
Author

Should NoneError implement the Error trait?

Niko

Just a note

@skade
Copy link
Contributor

skade commented Apr 22, 2020

I see the issue that Try for Error is already on stable.

@scottmcm
Copy link
Member

scottmcm commented Apr 23, 2020

Note that as of right now, it's absolutely by design that NoneError doesn't implement Error, because it's by design that you can't ? on Option in something returning Result<_, Box<dyn Error>>.

It's also unstable for a reason. We have a meeting planned about redesigning the current Try approach, which will likely remove NoneError from being involved in ?-on-Option.

I strongly discourage anyone from trying to hack around this restriction. Just use .ok_or(GoodReason) if you want to early-return on None in a Result-returning function.

@Halkcyon
Copy link

Halkcyon commented Dec 1, 2020

@scottmcm Make any progress this year on the issue? You've mentioned "we have a meeting planned", just following up here.

@scottmcm
Copy link
Member

scottmcm commented Apr 26, 2021

@Halkcyon Sorry for replying super-late here.

The meeting was https://blog.rust-lang.org/inside-rust/2020/04/10/lang-team-design-meetings.html#try-blocks-try-traits-functions-that-try-oh-my, though I'm not able to find minutes from there.

But rust-lang/rfcs#3058 merged last week, which explicitly blocks the ?-on-Option-in-Result-functions possibility, and (as it obviates RFC 1859) will likely result in NoneError being deleted from nightly.


Edit a while later: the PR I'm working on for RFC 3058 currently gives this error:

error[E0277]: the `?` operator can only be used on `Result`s, not `Option`s, in a function that returns `Result`
  --> $DIR/bad-interconversion.rs:11:12
   |
LL | / fn option_to_result() -> Result<u64, String> {
LL | |     Some(3)?;
   | |            ^ use `.ok_or(...)?` to provide an error compatible with `Result<u64, String>`
LL | |
LL | |     Ok(10)
LL | | }
   | |_- this function returns a `Result`
   |
   = help: the trait `FromResidual<Option<Infallible>>` is not implemented for `Result<u64, String>`
   = note: required by `from_residual`

Hopefully that will point people in the right direction, and avoid them ever hearing of NoneError.

@nhooey
Copy link

nhooey commented Jun 2, 2021

What was the resolution for this?

If it was resolved, what version of Rust contains the fix and how do we use it in our projects?

Is there a convenient workaround that works before the fix?

Forgive my questions, I'm really new to Rust and just trying to get it working.

@scottmcm
Copy link
Member

scottmcm commented Jun 2, 2021

The resolution of this is that it's expected that mixing them does not compile. NoneError has been deleted.

As such, the fix is to call ok_or_else or ok_or to provide a value implementing Error that includes the information about what went wrong, or to change the context to return Option instead of Result.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-error-handling Area: Error handling 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

Successfully merging a pull request may close this issue.