Pre-RFC: Deprecate ok_or method

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

[sekhat]

as people may or may not have had to contend with eager vs. lazy evaluation in their pasts.

I’d have to say, while I agree it’s not stupid, it’s fundamentally how rust’s function calling works. Just like in many other languages. While some people may use it expecting it to be lazy, I think it’s a lesson to learn, and a simple one.

So I’m going to cast a vote in making sure this is clear in the docs.

I don’t agree it should be a warning in rustc though, that just feels like a too serious level of flagging, and I certainly do not agree with the original premise of removing the function.

[drXor]

Something like this is interesting, actually. I think this eager/lazy pattern is common enough that an attribute like the following could be useful:

impl<T> Option<T> {
    #[lazy_alt = "ok_or_else"]
    fn ok_or<E>(self, err: E) -> Result<T, E> { .. }
}

If expr in opt.ok_or(expr) can’t be proven to be const, a warning + a suggestion to use opt.ok_or_else(|| expr) is emitted.

One could also imagine a dual #[eager_alt] attribute, that warns when it sees a closure with a constant expression inside (though the compiler should be smart enough to inline something like opt.ok_or_else(|| 0) anyway?). Also, what should we require of the alternatives? Should they be in the same module? Should something pointed to by #[lazy_alt] have an #[eager_alt] pointing back?

[vorner]

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.

I didn’t mean to say the people who made it were stupid, of course. More like a mild annoyance with the existence of the error ‒ like you could say „Having to type &*something is stupid“. You probably could replace „stupid“ with „unfortunate“ or „irritating“.

[drXor]

I decided to take a look closer into this to gauge how useful it would be. Looking just at Option<T>, we have the following eager/lazy pairs:

• unwrap_or/unwrap_or_else
• map_or/map_or_else
• ok_or/ok_or_else
• and/and_then
• or/or_else
• get_or_insert/get_or_insert_with

Of special note is the map_or pair, whose signatures are (approximately)

fn map_or<U>(
    self, 
    default: U, 
    f: impl FnOnce(T) -> U
) -> Option<U>

fn map_or_else<U>(
    self, 
    default: impl FnOnce() -> U, 
    f: impl FnOnce(T) -> U
) -> Option<U>

The “lazy” paramter is the middle one; thus, #[lazy_alt] should have to look like

#[lazy_alt(alt = "map_or_else", arg = "default")]
fn map_or<U>(
    self, 
    default: U, 
    f: impl FnOnce(T) -> U
) -> Option<U>

I think it’s also worth hammering out what #[lazy_alt] (and dually #[eager_alt], it it’s even necessary) should look like:

• Takes two parameters, the name of another function and the name of one of the attributee’s parameters.
• The alt must be in the same module, with the same visibility, as the attributee. If the attributee is inherent, then alt is an inherent function of the same type with the same self type. If the attributee is a trait function, alt must be in the same trait with the same self type.
• alt must have the same signature as the attributee, except for arg, which must be of type impl FnOnce() -> T, where T is the “eager” type of arg.

This should be enough information for the compiler to raise a lint warning if it sees the eager alternative given something that isn’t const.

Unresolved question: do we want to be able to give a function multiple lazy alternatives, for different parameters? (My thought is not because of combinatorial explosion, but I want to hear what other people think.)

[system]

This topic was automatically closed 90 days after the last reply. New replies are no longer allowed.