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

Deprecate `ok_or` method #51292

Closed
Pzixel opened this Issue Jun 2, 2018 · 3 comments

Comments

Projects
None yet
3 participants
@Pzixel

Pzixel commented Jun 2, 2018

There is plenty of issues when people unintendely use the eager version of mapping result method just because it saves some typing. It results in confusion among the community as well as performance issues in compiler.

Rust is going into the new epoch, so it's a good time to resolve this mistake.

The proposal is:

  1. Deprecate ok_or method on Result type. We already have a lint in clippy that suggest to replace it with ok_or_else if any computations are done there. It's a good signal that it shall be done. On the other hand, or_or_else with constant value lambda would be always inlined and thus have same performance as before.
  2. (Optional) Remove ok_or. Replace it with ok_or_else implementation. ok_or_else could be a deprecated alias for ok_or with lambda as parameter.

The phase 2 looks a bit confusing, but as said it would save some typing. Or it could be done in one big step. It would break the existing code, however it's an ok breakage as it's easy to fix it automatically (via just grep replace). We are already going to break some code with dyn Trait so it's not new for us.

This probably requires an RFC, I didn't do one yet, but I think we may collect some feedback here. Do you feel that compiler is smart enough to inline constant functions and we can remove some confusion and possible bottlenecks? Do you accept to type a bit more (else(|| part) or it's too high cost (among others like deprecation and code breakage)?

@oli-obk

This comment has been minimized.

Contributor

oli-obk commented Jun 2, 2018

There are good use cases for ok_or. Most notably when the value is constant. But also when the value is already computed, because you'd need it for something else anyway. In my opinion this should stay a clippy lint.

@Pzixel

This comment has been minimized.

Pzixel commented Jun 2, 2018

In my expirience you almost never want to return just a const. All errors in common libraries follow this pattern

let mapped_path = map_path(my_path).ok_or(MyError::InvalidPath(my_path))?;

Creating MyError for happy paths doesn't seems to be a good thing. But you have to do it if you want to provide some context, in this example - you want to add path which caused an error. When you pass a constant then you just say something bad happened. It's ok sometimes, but that's not what people could expect.

@Mark-Simulacrum

This comment has been minimized.

Member

Mark-Simulacrum commented Jun 2, 2018

Closing: this discussion should be moved to internals as a precursor to an RFC; we aren't likely to change anything with a stable method without an RFC.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment