Pre-RFC: Deprecate ok_or method

[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.

[inejge]

I extracted every usage of ok_or() from the corpus of first 1000 published moderately active (>= 5 releases) crates since the beginning of 2017, which amounts to ~ 800 repos. There were 553 instances of ok_or(). After eliminating all constant or pre-calculated expressions, 85 remained. That’s about 15%, so not quite rare, but far from prevalent.

[bestouff]

Typos: s/or_or/ok_or/g

[scottmcm]

Nobody proposes to keep or_else variants and deprecate all their counterparts.

I can’t imagine any argument for deprecating foo().ok_or(0) that wouldn’t equally imply deprecating bar().unwrap_or(0). Result and Option just aren’t that different.

unlike ok_or which almost always calls some methods.

Proof?

[Pzixel]

I can’t imagine any argument for deprecating foo().ok_or(0) that wouldn’t equally imply deprecating bar().unwrap_or(0) . Result and Option just aren’t that different.

unwrap_or is mostly used with string or numeric constant. ok_or is never used with string or numeric constant.

Proof?

Well, I don’t have an actual research, however @inejge provided one. As i read it, from 553 instances of ok_or 85 was actual constants and remained as they was, while the rest was method misuse that had to be replaced with ok_or_else.

[repax]

If this is a source of bugs then I think it should be taken seriously. I care about correctness.

Is it possible to require that the expression is const / pure?

[rkruppe]

unwrap_or is mostly used with string or numeric constant. ok_or is never used with string or numeric constant.

It is, however, often used with other trivial expressions (variants of an error enum). Such as the example at the start of this thread, MyError::InvalidPath(my_str) (which, contrary to your assertion, is really cheap and optimizable, so it really should not have any performance overhead at all, let alone a measurable one.)

Well, I don’t have an actual research, however @inejge provided one. As i read it, from 553 instances of ok_or 85 was actual constants and remained as they was, while the rest was method misuse that had to be replaced with ok_or_else .

Check again, I read the exact opposite. “After eliminating all constant or pre-calculated expressions, 85 remained” – i.e., 85 of 553 uses weren’t constants or pre-calculated expressions.