The name value_or lacks parallelism. value_or feels like it takes a noun (another value), while get_or feels like it takes a verb (function). If we're going for consistency with value, shouldn't it have a named parameter as well?

The compound value_or_failwith is clearer, but I think in that case the value_or should stay hypothetical and it wouldn't be ideal if someone wanted to add the more general form later.

The name value_or lacks parallelism. value_or feels like it takes a noun (another value), while get_or feels like it takes a verb (function). If we're going for consistency with value, shouldn't it have a named parameter as well?

value_or is not under discussion in this PR. It was just mentioned as a data point in the discussion of what is being proposed in this PR.

Perhaps value_or_failwith (which would be coherent with the hypothetical value_or)?

I mentioned in point 2. here why I think this is not a good name in context¹.

More generally I'm not very fond of these "hardcoded boolean expression in identifier" schemes when you can avoid them. For reading, I do not want my mind to have to evaluate a boolean expression in an identifier. I want the function to mention the important case it operates on, Error in this case, and what it does with it.

With that constraint, once you factor in the Result.error prefix there's not much more choices. E.g. Result.failwith_on_error doesn't read well, Result.error_to_failwith is weird (but I could live with it), so I finally went with Result.error_to_failure.

I can see the point you are making about value conversion but again, the context just shows you that this function can't possibly produce a Failure value. So I think one quickly learns that this is about the mechanism of failure rather than literally converting to a Failure value. And once that is learned I think this name is eventually much better at identifying on a glance the places where Error cases are being turned into Failure exceptions² than these "boolean expression in identifier" schemes.

I'm happy with get_ok'. I've been trying to persuade myself of error_to_failure, but I'm not particularly convinced get_or_failwith is so problematic (agreed that the "default" error behaviour of a get-like function should be invalid_arg) and I'm struggling to persuade myself why error_to_failure needs explicit reference to what it does on error and no reference to the fact it's retrieving when put next to the two get_* functions:

let get_ok = function Ok v -> v | Error _ -> invalid_arg "result is Error _"
let get_ok' = function Ok v -> v | Error e -> invalid_arg e
let error_to_failure = function Ok v -> v | Error e -> failwith e

(it's not a veto, I'm just struggling to put an approval on it)

needs explicit reference to what it does on error and no reference to the fact it's retrieving when put next to the two get_* functions:

Well in the Result.get_ok cases the error is not supposed to trigger except on programming errors so I don't think it's worth mentioning.

In the conversion to Failure _, Error is expected to happen, so that's why I like it mentioned. Besides since I usually use this function postfixed in let bindings and what you get gets on the left I prefer not to have get on the right there, it reads a bit in the wrong order (not to mention that I prefer if get generally means Invalid_argument).

let x = parse_integer s |> Result.get_or_failwith in

let x = parse_integer s |> Result.error_to_failure in

Literally I read the second let as "bind the result of parse_integer to x and by the way if there's an error just raise a Failure exception". I think the first let is more difficult to read.

Literally I read the second let as "bind the result of parse_integer to x and by the way if there's an error just raise a Failure exception". I think the first let is more difficult to read.

Also perhaps I should add that the reason why I use this function postfixed is for the similarity with (let*). If I decide to use the result monad then my code is:

let* x = parse_integer s in 

Now if suddendly I'm annoyed by it (e.g. in loops) I simply switch to

let x = parse_integer s |> Result.error_to_failure in 

And somehow to me it reads the same, except for what happens on error.

I understand @dbuenzli's argument that identifiers do always occur in some context, but Result.error_to_failure nonetheless seems too weird to me, because it reads like it maps an error value (which indeed is a value) to a failure value (which is not a value, it's raising an exception of that value). In other words, the name itself doesn't try to differentiate that the error is a value while the failure is not.

My subjective take: when I'm struggling like this with naming, it's usually a sign that the function itself is probably wrong. The suggested Result.value_or function could reproduce the behaviour of Result.error_to_failure simply as Result.value_or r failwith ¹. Looks not so bad, right? Compare the two:

Result.value_or r failwith
Result.error_to_failure r

One character saved in the second case.

Looks not so bad, right? Compare the two:

I think trying to read what you propose speaks by itself. It doesn't read in the right order and doesn't even have the right structure on how it is being used in practice. Once more… this PR proposes to add a combinator to convert Error string cases to raise Failure exceptions directly, it's ok if you don't like the name being proposed but please propose a better name for that. And no, the function itself is not wrong as you suggest, it's something you naturally often do in practice when you dance with result values and loops.

A few other names have already been proposed Result.{value,get}_or_failwith which I find for reasons already discussed in depth inferior to what is being proposed in this PR. Maybe Result.error_to_failure has to be learned but I don't think its hard to remember if you are used to work with the standard library.

So far for me the only other contender is Result.error_to_failwith (which can be read as "give the error to failwith"). I'm ok to switch to that if people prefer¹ since that name retains all the essential properties (see discussion) I'm seeking from the function name. This name could perhaps reassure people who fear that Result.error_to_failure isn't easily learned as "convert the error the mechanism used for failure" rather than "convert to a Failure exception value"² so I'm fine with it.

It doesn't read in the right order and doesn't even have the right structure on #13720 (comment) in practice.

Converting your linked example to the one using a hypothetical Result.value_or:

let x = parse_integer s |> Result.value_or failwith in

Personally, I can't see any problem reading this line of code, and I can't see why this approach is inferior to the proposed Result.error_to_failure. From your comment, you seem to be fine with the name Result.error_to_failwith, but this name encodes the partial application of failwith to Result.value_or in itself. This is why I find the function Result.error_to_failure itself too specific.

Personally, I can't see any problem reading this line of code,

But that's not the order you proposed in your message. I see many, they have already been stated.

and I can't see why this approach is inferior to the proposed Result.error_to_failure

Well for that you will have to read the whole discussion at which point you will also likely read this comment. I don't feel like wasting everyone's time by rehearsing the same arguments over and over.

Two months later. I'd really like to get rid of my extremely tiny and thus extremely annoying to carry, Result module overlay after 5.4 and this is part of the plan.

Here are a few possibilities:

1. Another dev follows @nojb
2. Another dev follows @nojb but requests Result.error_to_failure to be renamed to Result.error_to_failwith (why not but honestly I rather think that failwith should have been called failure).
3. I close this, raise Failure in my plan and continue to carry my annoying Result overlay in all my projects.

Note I'm not interested at all about the generalizations or the weak names with embedded boolean logic that have been proposed. More on the position here and of course in the whole discussion where the name has been thoroughly justified (describe what happens on error since that's when the function does something to your stack, how it reads in binding contexts with |>, yadada, yadada). I want a direct function that reads correctly with how I propose to use it in order to deal with and switch between error control flow idioms present in the stdlib and user code.

I agree we have reached a point where it is up for the core devs to make a call. In a comment in #13696 @gasche did not seem opposed to the current design (even if not particularly enthusiastic either). @gasche: are you willing to make a call here?

FWIW, I'd really appreciate if this is merged and part of the next OCaml release.

Ping. It's getting cold, and when it gets cold everyone rushes.

No need for this to be part of the rush, these are trivial, low maintenance, highly useful additions, that have been painstakingly justified and first proposed on the 23rd of december 2024.

I'm not sure why taking a decision on something as trivial and explicit as:

Result.error_to_failure, transforms string Error cases to Failure _ exceptions.

seems so difficult for upstream.

This is where the PR stands.

I cannot convince myself that I like these functions.

The premise is that people write code like

match foo with
| x::xs -> ...
| [] -> Error "The list must be non-empty"

and then we need to fight over the name of ('a, string) result -> 'a functions to turn these strings into exceptions.

I think that string is a rather poor representation for the programmatic payload of an exception; it's okay at the boundaries of the program, but a lot of code would be better off using more structured error explanations.

We already have a type that allows for structured exceptions: exn. So I think that people should write their code to return a ('a, exn) result instead, this is more informative to read in the types and a better design. Concretely they would have to write

match foo with
| x::xs -> ...
| [] -> Error (Invalid_argument "The list must be non-empty")

and then we only need to squabble over the naming of one function of type ('a, exn) result -> 'a, whose behavior is obvious from the type.

We already have a type that allows for structured exceptions: exn. So I think that people should write their code to return a ('a, exn) result instead, this is more informative to read in the types and a better design.

Since I'm the one who has been justifying things over and over for hours to get these trivialities in. Could you perhaps justify more thoroughly as to why this "a better design" ? That's just words you wrote.

I think that string is a rather poor representation for the programmatic payload of an exception; it's okay at the boundaries of the program, but a lot of code would be better off using more structured error explanations.

Could you perhaps explain us how better off the code would be ? Because no, in general that's not what happens. Most of the time you convert to strings much before because:

1. You need to be able to unify multiple error explanations (multiple error monads).
2. You need to abstract away an error explanation (the fact that you used libary Zorglub to read zorglub files and don't want the rest of your code to depend on the Zorglub error definitions)
3. You need to be able to add more information about the context which may not be part of the error payload (e.g. a human description of a high-level operation you were performing in which a sub operation errored).

Doing so with polymorphic variants is painful and most of the time not useful (been there, done that) and I can't imagine doing so with exn: you may well end up at a point in your code where you don't even know how to render the exn you are getting, I'm sorry but your "better design" seems a recipe for disasters.

Using string instead of exn is much more malleable and honest. Mind you in most of my libraries I provide two results one structured, usually primed, (operation') and another one in the string error monad (operation). I'll let you guess which one I use most.

Ok so @gasche won't approve because he has his own (rather dubious I must say) ideas on how errors should be handled in practice.

Is there perhaps any other maintainer that shares my view on this and that is willing to approve this ?

I agree that exn is not a good type to use to return error explanation fragments that are meant to be processed/enriched in some caller-aware context. I have doubts that string is a better choice for this, but I don't have your practical experience with this design pattern (maybe sometimes it's nice for types to be less precise but more uniform). I don't buy the idea that it makes sense to make it easy to call failwith or invalid_arg on those explanation fragments. If either of the two ('a, string) result -> 'a helpers you suggest is beneficial in a given situation (and their use is not a conceptual bug encouraged by lack of type information), then presumably at that point the error information is complete and could have been packaged into an exn.

I agree that exn is not a good type to use to return error explanation fragments that are meant to be processed/enriched in some caller-aware context.

Which is the very basis of good error reporting.

then presumably at that point the error information is complete and could have been packaged into an exn.

I don't get what you say here. If the error information is complete then you rather don't want to package it as an exn value, which is the most opaque thing you may have to handle in OCaml, you can just raise Failure msg or use Error msg.

In general at API boundaries I dont want any of this:

1. Having exceptions thrown (exceptions to this rule do exist though).
2. ('a, exn) result values.

Calling your hypothetical Result.error_to_exn on ('a, exn) result values returned by APIs would be fraught with perils and likely enable bad practices like catch all exception handlers. I don't know where this idea of ('a, exn) result came from or if people do that in practice but to me it looks like bundling the worse of every error mecanism at our disposition.

Now the string error monad is a very fine error monad to work with for building error messages for cli tools and application/server level logging. And Result.error_to_failure is simply a useful control flow construct for the string error monad.

For example when you process a sequence of elements in a failstop manner in a recursive subfunction, rather than painfully thread the error monad accross the recursive function calls just use Result.error_to_failure, catch Failure msg at the toplevel of your function and return Error msg. They are also useful when your primitives are in the string error monad and you get to process things with the iter functions of data structures.

Also rereading your message I have to stress that this completely misses the mark as to what Result.get_ok' should be used for:

Concretely they would have to write

match foo with
| x::xs -> ...
| [] -> Error (Invalid_argument "The list must be non-empty")

and then we only need to squabble over the naming of one function of type ('a, exn) result -> 'a, whose behavior is obvious from the type.

The idea of Result.get_ok' is not to handle Invalid_argument errors, those should never be handled. It's aim, like the existing Result.get_ok, is to turn regular errors into Invalid_argument exceptions when you have an invariant that tells you it's going to be Ok.

The only difference with Result.get_ok is that if your error happens to be in the string error monad you get a good error message rather than an unhelpful "result is Error _".

Merged. Thank you everyone for participating in the discussion!

Thanks! I think I'm going to take a little stdlib vacation after that one :–)

But I have to say, I'm sure 5.4 will become a natural lower bound in the future, it's an excellent vintage stdlib wise. Thanks for all the additions.

"Ah yes, 5.4! A rainy winter but we avoided most of the PR bitrot. And then a nice warm spring which let those stdlib functions grow nicely before the release branched."