Pre-RFC: Deprecate ok_or method

[Pzixel]

Motivation

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. It happens even with the compiler team itself.

The most popular using of this method is something like following:

let path = try_get_path(my_str).ok_or(MyError::InvalidPath(my_str))?;

The problem here is that MyError struct is created not only if error actually happened, but on happy path too.

In most cases, people really want to write

let path = try_get_path(my_str).ok_or_else(|| MyError::InvalidPath(my_str))?;

But they don’t use it, mostly because it requires a bit more typing as well as they might not notice the difference. Of course, if they find a significant slowdown, they could benchmark code, spot these places and fix them, but in most cases it would remain unnoticed (although it affects performance drastically).

Rust is going into the new epoch, so I believe it’s a good time to consider the change.

The proposal is:

1. Deprecate ok_or method on Option 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) Replace or_or method via or_or_else signature and implementation, remove the latter (or make it just the proxy to the ok_or).

I believe that we mostly want a lazy evaluation rather than eager one. Error value is never a constant, except when it’s just “something bad happened” static string. So its recalculation on happy paths seems to be a undesirable behavior. In the rare cases when it’s required compiler could always figure out that constant function may be inlined and replaced by value.

This change serves two goals: remove a possible pitfall and make standard library more consistend so user don’t have to choose between two similar methods, just pick one, and if it may be optimized compiler will do so.

[steveklabnik]

We can only deprecate, but we cannot remove or replace it. Epochs can’t make this kind of change.

[sfackler]

Why is this specifically talking about only ok_or and not other similar functions?

I don’t agree that we need to remove a method because people have accidentally misused it. The fact that arguments are evaluated before a call to a function or method is pretty fundamental to ~all imperative languages.

[leodasvacas]

The most popular using of this method

I’ll need more data to be convinced that most uses of ok_or dynamically allocate a string. It is very common for errors to simply be constants. Also that method is part of an API convention, we also have map_or and unwrap_or. You make valid points for when we should prefer ok_or_else, but in many cases ok_or is more readable and arguably more efficient, so I see nothing intrinsically wrong with it.

This does give me the idea of having an .ok_or!(foo), when we get macros in method position, that desugars into an inlined ok_or, gaining us the best of both worlds. That would be cool.

[H2CO3]

If the issue is frequent misuse (which I’m not convinced about) of the eager methods when the lazy ones should be used instead, that is more of a cultural/educational issue, not a technical one. I think that docs are already pretty clear, but perhaps we could educate people more clearly? It’s also possible to write a Clippy lint that checks for the misuse of ok_or.

[birkenfeld]

It’s also possible to write a Clippy lint that checks for the misuse of ok_or .

There already is

[H2CO3]

(Thanks, I tried to check that before commenting, but I couldn’t find it…)

[Pzixel]

It’s written in the proposal itself, if you read it

I don’t agree that we need to remove a method because people have accidentally misused it. The fact that arguments are evaluated before a call to a function or method is pretty fundamental to ~all imperative languages.

If people misuse it on a day to day basis then it should be receive more attention, I do believe. The fact that there is a lint in clippy is already a signal. Probably, it’s enough, probably it’s not. This is the reason why I have written this.

[H2CO3]

people misuse it on a day to day basis […] The fact that there is a lint in clippy is already a signal

But… they don’t? Clippy has plenty of lints against accidental mistakes which are rare in practice, just as an extra precautionary measure. Clippy doesn’t only include lints because they are catastrophic or because everyone makes those mistakes with every second keystroke. The purpose of a linter is to squeeze out even the last bit of code quality if it can. By this logic we could argue for the removal of match expressions, the clone method or references, because there are several lints against those too.

As you probably noticed, I am very much against anything in Rust (especially anything additive) that has a realistic chance of resulting in generally poorer, buggier code. The ok_or method is just not an issue in practice, though.

[Pzixel]

But… they don’t? Clippy has plenty of lints against accidental mistakes which are rare in practice, just as an extra precautionary measure.

Well, I think all these lints should be considered as possible language fix. You see lint, you check that everybody makes this mistake, and you have two options: “Hmm, we must follow the principle of least astonishment and fix it in the language” or “it’s a rare case and docs are clear about this behavior”. I just think that it’s the former case, so I have written this proposal. If I’m alone thinking it that it’s ok, we’re just discussing.

I see that it’s coherent with “map_or”, “unwrap_or” and others, but probably we could reexamine if they are really required. Maybe not. It’s discutable.

By this logic we could argue for the removal of match expressions, the clone method or references, because there are several lints against those too.

No, it’s not implications of this logic, it’s ad absurdum argument. ∃ redundancy != ∀x x == redundancy

[Ixrec]

you have two options: “Hmm, we must follow the principle of least astonishment and fix it in the language” or “it’s a rare case and docs are clear about this behavior”.

This statement assumes that it’s always both possible and unproblematic to “fix it in the language”. In this case, we’re talking about detecting whether the (arbitrary) expression passed to ok_or will perform some computationally expensive task that would be better off done lazily. That’s obviously not something we can ever hope to solve for in the general case (the closest we’ve seen might be some of the “effect system” proposals to track whether a function dynamically allocates, but so far those are all very heavyweight and dubious proposals and that’s still only a subset of the problem). What we clearly can do is detect some very specific special cases of it that happen to be commonly written and are usually better off as ok_or_else, which is precisely what makes a lint appropriate even if those special cases are extremely common in practice.

[Pzixel]

This statement assumes that it’s always both possible and unproblematic to “fix it in the language”.

I don’t understand where you get it. This statement clearly says that you have an option to embrace a lint on language level, or not. That’s it. It doesn’t say anything about “unproblematic” of anything.

In this case, we’re talking about detecting whether the (arbitrary) expression passed to ok_or will perform some computationally expensive task that would be better off done lazily. That’s obviously not something we can ever hope to solve for in the general case (the closest we’ve seen might be some of the “effect system” proposals to track whether a function dynamically allocates, but so far those are all very heavyweight and dubious proposals and that’s still only a subset of the problem).

No, it’s easy: just make everything lambda. If it’s a simple constant expression then it would get inlined by optimizer. If it’s not, then it shouldn’t be computed eagerly. Nothing about any specific case. Optimizer already knows what’s constant expression is.

[kornel]

I’m not keen on this. It is convenient when used properly, and there’s a Clippy lint for misuse.

[MajorBreakfast]

I think this should be deprecated. I’m not so keen on footguns in std.

[scottmcm]

Things like .unwrap_or(0) are perfectly reasonable. Forcing that to be .unwrap_or_else(|| 0) every time is way overkill.

Having both optimistic (lint if contains unknown function calls, could be warn-by-default) and pessimistic (lint on anything but trivial things like literals, probably allow-by-default) clippy lints, however, does seem reasonable.

[vorner]

I think that if it was such a common problem, then the first step could be not deprecating the method outright, but promoting the warning from clippy lint to one in rustc. But only on the ones that contain a function call. Furthermore, rustc is probably in a better position than clippy (well, I just guess here) to know if the thing inside contains some computation or if it is just inlined as a constant (eg. Some(42) is probably not a call).

I would go for more extreme ways if that doesn’t help. I personally find a lot of legitimate uses of unwrap_or or ok_or.

However, I think we should recommend clippy more to beginners. It is useful.

[xfix]

let path = try_get_path(my_str).ok_or(MyError::InvalidPath(my_str))?;

The problem here is that MyError struct is created not only if error actually happened, but on happy path too.

If InvalidPath is an enum variant (not a regular function call), then there isn’t really a cost in doing so, as the optimizer should be able to realize there are no side-effects.

[Pzixel]

Things like .unwrap_or(0) are perfectly reasonable. Forcing that to be .unwrap_or_else(|| 0) every time is way overkill.

Nobody proposes to keep or_else variants and deprecate all their counterparts. The question was do we really need ok_or_else to be just coherent with others or_elses?

I think that if it was such a common problem, then the first step could be not deprecating the method outright, but promoting the warning from clippy lint to one in rustc. But only on the ones that contain a function call. Furthermore, rustc is probably in a better position than clippy (well, I just guess here) to know if the thing inside contains some computation or if it is just inlined as a constant (eg. Some(42) is probably not a call).

That’s a valid option too. And it look more consistent to me. I really didn’t like the idea of deprecating such a basic thing, but it’s really a footgun in most cases. Lint like deny(heavy_or) would also help with other or_elses, however, they tend to use constants indeed, unlike ok_or which almost always calls some methods.

[kornel]

unlike ok_or which almost always calls some methods.

That doesn’t match what I’ve seen, which is almost exclusively use of enum variants:

thing_returning_option().ok_or(MyErrorEnum::ThingDidntWork)?;

[Pzixel]

That doesn’t match what I’ve seen, which is almost exclusively use of enum variants:

Do you really think so? Because I mostly see something like:

fn path_to_cstring<P: AsRef<Path>>(path: P) -> Result<CString, failure::Error> {
    let path = path.as_ref();
    let x = path.to_str().ok_or(CvError::InvalidPath(path.into()))?;
    let result = CString::new(x)?;
    Ok(result)
}

(I took this example from my own code)

So I have mistaken just as described in my post. So I don’t think it’s really this rare as you describe it.