Pre-RFC: Deprecate ok_or method

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

[Pzixel]

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.

Ok, I buy it.

Maybe then we just should move from clippy lint to a rustc warning?

[scottmcm]

Maybe then we just should move from clippy lint to a rustc warning?

This kind of method-specific warning is better in clippy, IMHO.

That said, consider whether there could be some kind of intent marker that could make sense here so that authors of methods could opt-in to similar things. As a sketch, I could imagine a #[potentially_unused] parameter attribute which would trigger warnings if anything expensive-to-compute is consumed by it. That would, of course, need more design thought and research to determine whether it’s worth it, and eventually an RFC. (Or maybe it could be #[clippy::warn_if_expensive] or something. I don’t know.)

[U_007D]

While I feel it may be somewhat overstated (I’m not sure how misued ok_or(), unwrap_or(), etc. are), I think the motivation is valid–the eager evaluation design prohibits unaware users from falling into a “pit of success”. Though, I must admit I’ve triggered the Clippy warning on this issue a couple of times, even though I should know better).

With that said, just yesterday I wrote a case in which I did want unwrap_or() (the eager version), and going through the extra (admittedly, minor) gymnastics of getting a deprecated warning, and/or switching over to the lambda version would be a usability paper cut, IMHO. It was along the lines of:

let foo = a + b(arg).unwrap_or(0);

(where b() returns Option<u8>).

Instead of changing the language (I think it would be odd to special case this instance of eager evaluation and not the rest of Rust) how effective would it be to call this out in big bold type in the documentation? Along with the Clippy warning, no one who wants to be aware of this should be caught by it.

[vorner]

While I feel it may be somewhat overstated (I’m not sure how misued ok_or(), unwrap_or(), etc. are), I think the motivation is valid–the eager evaluation design prohibits unaware users from falling into a “pit of success”.

The question is not only how often they are misused, but also how many CPU cycles are wasted by it and if it’s on a hot path or not. I think most of these errors are not really bugs (it doesn’t produce invalid results, etc ‒ in theory it could, the call there could have some side effects, but I believe that will be rare) and the performance penalty might be negligible… and if it is on the hot path, some profiling should show it.

While doing this kind of mistake is stupid, the prevention against that should probably be light-weight. Maybe propagating clippy more (because it also catches bunch of other things) is the right way.

[U_007D]

While doing this kind of mistake is stupid

I wouldn’t characterize this error in this way (even though in my personal case it is probably appropriate ;)), as people may or may not have had to contend with eager vs. lazy evaluation in their pasts.

Otherwise agreed on making prevention light-weight. And highly discoverable, in the docs. Perhaps even moving the Clippy lint into rustc, so that everybody can benefit from it?