Skip to content

Add Result.product and Result.Syntax#13696

Merged
nojb merged 3 commits intoocaml:trunkfrom
dbuenzli:result-improvements
Mar 31, 2025
Merged

Add Result.product and Result.Syntax#13696
nojb merged 3 commits intoocaml:trunkfrom
dbuenzli:result-improvements

Conversation

@dbuenzli
Copy link
Copy Markdown
Contributor

@dbuenzli dbuenzli commented Dec 23, 2024

This PR adds Result.Syntax with the binding operators for result values.

Before it also added

  1. Result.{get_ok',error_to_failure}, now living in Add Result.{get_ok',error_to_failure} #13720
  2. Result.retract, now living in Add Result.retract #13721

Binding operators

This is a partial implementation of what has been proposed by @lpw25 in #2170 six years ago. However the Syntax module proposed here provides only binding operators, there is no return. I find it clearer to use the dedicated constructor Ok or Result.ok directly and I think it's better not to pollute our crowded scopes with unqualified identifiers.

It is something that I use in almost all of my source files and programs.

Some people would like the named binding operator of #13325 to settle down before deciding this (see this discussion). However no one ever seemed to address this question and more than one person points out that they end up using at most one monad most of time in a given context (which hints at an answer to the question).

However when the latter is true named binding operators become both extremely noisy and a waste of your horizontal space. Personally I'm extremely unlikely to use them for my pervasive use of the error monad. Hence the proposal whose usage will, I suspect, outlive any named binding operator introduction, if it ever happens.

@dbuenzli dbuenzli force-pushed the result-improvements branch 2 times, most recently from cf385aa to 774ada7 Compare December 23, 2024 16:07
@dbuenzli
Copy link
Copy Markdown
Contributor Author

It seems that the additions changed something and made some lambda code tests fail. I blindly promoted them in e7f3914. I didn't fully understand the tests but I guess some Result field references got shifted (?).

@wikku
Copy link
Copy Markdown
Contributor

wikku commented Jan 1, 2025

get_ok' and error_to_failure are two special cases for ('a, string) result, the only difference being one uses invalid_arg and one uses failwith. I'm not a fan because they are special cases (what if you want more than string) and the names don't convey the differences and similarities.

I'd much prefer

let get_or f = function Ok v -> v | Error e -> f e;;
val get_or : ('a -> 'b) -> ('b, 'a) result -> 'b = <fun>

so that get_ok' = get_or invalid_arg, error_to_failure = get_or failwith, retract = get_or Fun.id.

(I'd also add an alias get = get_ok, for consistency with Option.get, Result.map, Result.iter and also so no one wonders why it's not get_ok_or.)

@dbuenzli
Copy link
Copy Markdown
Contributor Author

dbuenzli commented Jan 1, 2025

@wikku I don't think there's much value in making that more general. The proposed functions just make it convenient to interoperate with other idioms that are part of the stdlib. They are easy to read, easy to write and consistent with what already exists.

@wikku
Copy link
Copy Markdown
Contributor

wikku commented Jan 1, 2025

I disagree that get_ok' is easy to read. The prime can be easy to miss, its meaning is nonobvious at first, and it occurs in no other function in the stdlib.

('a, string) result doesn't occur once in the stdlib. The Failure and Invalid_exception exceptions are mostly used for programming errors, sometimes used internally for a nicer error message or converting to option. So I don't see what existing idioms there are to interoperate with.

I am not opposed to introducing functions that help with the pattern of using exceptions internally and results at the interface, but to me this looks a bit too specific.

@dbuenzli
Copy link
Copy Markdown
Contributor Author

dbuenzli commented Jan 1, 2025

I disagree that get_ok' is easy to read. The prime can be easy to miss, its meaning is nonobvious at first, and it occurs in no other function in the stdlib.

The prime is precisely unimportant.

The idiom M.get[_*] is used in OCaml to get something when you know it's here by invariant and raises the programming error Invalid_argument if it is not.

We have Result.get_ok which does so, albeit it only reports there was an error Error _ instead of Ok _. Result.get_ok' is the same function but provides more information when the error is a string message. There's no need to introduce another complicated name for that, hence the prime. When you read it, it doesn't matter if you miss the prime: you are just interested in spotting what raised Invalid_argument and both function do. Once you spotted Result.get_ok[']), you found your problem whether you saw the prime or not.

I have in the past used various other names for that (e.g. the ill named Result.to_invalid_arg) but in the end I would always use the natural get_ok and miss error messages, so I think it is the right name.

I am not opposed to introducing functions that help with the pattern of using exceptions internally and results at the interface, but to me this looks a bit too specific.

I don't think these additions preclude adding more general things if people want to.

However translating ('a, string) result to Invalid_argument and Failure is natural, these two functions just provide this.

I'd just like to add that what I'm adding here is based on quite a lot of design rounds on this including in Rresult and in B0_std.Result which over time had quite a few of fancy stuff which would never get used. What remains is what is proposed here.

@gasche
Copy link
Copy Markdown
Member

gasche commented Jan 3, 2025

For the record, I rather like get_or as proposed by @wikku. Except I would write the type annotation with the variable names flipped, so that it is ('a, 'b) result; in fact I would write ('a, 'e) result, which is common in the rest of the .mli.

Thinking out loud: having this function would possibly suggest get_or_invalid_arg and get_or_failwith as alternative names for get_ok' and error_to_failure proposed by @dbuenzli. Personally I'm not opposed to having them around as special cases, and don't have strong opinions on how they should be named.

Regarding the other questions/suggestions of @dbuenzli:

  • I'm indeed interested in making progress on named binding operators, and find it difficult to make decisions on Syntax submodules in this context.
  • The feature of retract seems fine to me, but I'm not sure about the name. (I wondered if just get would work, but I agree it is a bit confusing. get_either works. All my other ideas were worse than retract.) I wouldn't mind picking something that also work for Either and also implementing it there; retract does work in this respect, and my other suggestions as well.

@dbuenzli
Copy link
Copy Markdown
Contributor Author

dbuenzli commented Jan 3, 2025

For the record, I rather like get_or as proposed by @wikku.

Personally I'm not sure I would be using it, so I won't be the one adding it now. I would however quite disagree with the name itself. I'd really prefer if we keep get[_*] functions as strong signifiers for Invalid_argument. So if added I think it should rather be a variant of the existing Result.value, that is Result.value_or.

Thinking out loud: having this function would possibly suggest get_or_invalid_arg and get_or_failwith as alternative names for get_ok' and error_to_failure proposed by @dbuenzli.

I'd rather not :–)

  1. On get_or_invalid_arg. As mentioned get and Invalid_argument are synoymous to me, no need to stutter. Besides Result.get_ok''s terseness is part of the feature. It makes it easier to fit things on one line when you want to simply ignore a result's error with the pipe operator, exactly like get_ok does – again it's just a variant of the latter, I'm not sure why people keep on insisting to have a different name. This allows you to relegate the unpacking buraucracy at the end of the line which makes it easier for the brain to ignore that noise and focus on what you are binding m to:

    let m = M.of_string "m literal" |> Result.get_ok' in
    …
    
  2. On get_or_failwith. Here again I prefer if we keep get_* as strong signifier for Invalid_argument exceptions. In my opinion, if anything else it should rather be Result.value_or_failwith. But I really prefer Result.error_to_failure, it reads better in my opinion:

    let m = M.of_string "literal" |> Result.value_or_failwith in
    let m = M.of_string "literal" |> Result.error_to_failure in
    

    On reading I like the emphasis of what happens on Error _ which is the thing worth highlighting here: at that point an Error is turned into a Failure exception. I find value_or_failwith less obvious on the intent and mechanics (perhaps because of the confusion on reading that there can be between a "result value" and the "result's value").

  • The feature of retract seems fine to me, but I'm not sure about the name. (I wondered if just get would work, but I agree it is a bit confusing. get_either works. All my other ideas were worse than retract.)

Result.get would be confusing in my opinion (people would likely read this as Result.get_ok). Result.retract was suggested a long time ago to me by @pqwy. I remember spending some time trying to figure out something else but in the end didn't find anything better. The idea is that you retract the datatype. But yes it's a new name to learn. However it is not unheard of in Haskell.

  • I'm indeed interested in making progress on named binding operators, and find it difficult to make decisions on Syntax submodules in this context.

What are the difficulties ? Six years later it seems to be a rather low risk addition to me. As I mentioned, the current bindings operators and their use for the error monad are not going to go. If you want to add a module that introduces the hypothethical named binding operators in the future you could add them in Result.Syntax.Named – quite a few of us are not interested in mixing monads for the obvious lack of compositionality and the extra noise they entail and would not use them anyways.

@dbuenzli dbuenzli mentioned this pull request Jan 6, 2025
@brendanzab
Copy link
Copy Markdown

Would you consider renaming Result.product to Result.both? I’ve seen it used in Base.Applicative.S and Stdune.Applicative, and thought it was a nice and friendly name, and have been using it in my own projects.

@gasche
Copy link
Copy Markdown
Member

gasche commented Jan 8, 2025

@dbuenzli maybe you should consider splitting the PR in three, because I think that it's easier (at least for me) to make progress on some points than others.

@dbuenzli dbuenzli force-pushed the result-improvements branch from e7f3914 to 7b47b72 Compare January 8, 2025 22:36
@dbuenzli dbuenzli changed the title Result improvements Add Result.product and Result.Syntax Jan 8, 2025
@dbuenzli
Copy link
Copy Markdown
Contributor Author

dbuenzli commented Jan 8, 2025

The split has been done. Please continue the discussion about:

  1. Result.retract in Add Result.retract #13721
  2. Result.{get_ok',error_to_failure} in Add Result.{get_ok',error_to_failure} #13720
  3. And Result.product and Result.Syntax in this PR.

Would you consider renaming Result.product to Result.both?

I don't know, both feels weird to me. As was discussed when let bindings operators where introduced you want the monoidal product here. Now I see that what @lpw25 was using prod there, so if anything maybe change to that. In any case we should have what we would like to see in a hypothetic APPLICATIVE signature and I don't think I'd like to see both for such a signature there.

@xvw
Copy link
Copy Markdown
Contributor

xvw commented Jan 10, 2025

I don't want bikesheeding because I think the name product is perfectly fine, but it seems to me that sometimes product is called zip.

@dbuenzli
Copy link
Copy Markdown
Contributor Author

Well if no agreement can be found I'm also willing to simply not expose it, what I want in is Result.Syntax :–)

@xvw
Copy link
Copy Markdown
Contributor

xvw commented Jan 10, 2025

For what it's worth, I think the name product is uncontroversial enough to keep the function exposed, under the name product :)

@johnyob
Copy link
Copy Markdown
Contributor

johnyob commented Jan 10, 2025

Would you consider renaming Result.product to Result.both? I’ve seen it used in Base.Applicative.S and Stdune.Applicative, and thought it was a nice and friendly name, and have been using it in my own projects.

It is also used by CCResult. Additionally, not everyone may associate Result.product with the concept of a monoidal product, which could cause confusion. In contrast, Result.both clearly conveys that the function extracts both values from two results and combines them into a single result.

@dbuenzli
Copy link
Copy Markdown
Contributor Author

Additionally, not everyone may associate Result.product with the concept of a monoidal product, which could cause confusion.

What kind of confusion ?

Personally I'd find List.both equally confusing, it could mean many things. So I'm not sure using both for defining the applicative signature is very enlightening, at least product refers to the literature1. In each case you need to learn something.

Again the added term here is to satisfy an APPLICATIVE interface and make it clear what is used with each binding operator according to the convention. In fact while being a heavy user of the error monad I don't think I ever Result.product nominally in my life and use of Result.both on sherlocode is rather anedoctical – I think these 9 occurences could be replaced by Result.product and not be terribly confusing to read.

But if people prefer that I remove it, so be it.

Footnotes

  1. In fact I'm not very fond of product either, I prefer the fmap formulation but the product formulation is what binding operators require, as was discussed in the aforelinked PR.

@johnyob
Copy link
Copy Markdown
Contributor

johnyob commented Jan 12, 2025

What kind of confusion ?

My point pertains to the fact that not everybody would make the connection between the name Result.product and the fact that it is indeed a monoidal product. This could be a source of confusion around the name.

Personally I'd find List.both equally confusing, it could mean many things. So I'm not sure using both for defining the applicative signature is very enlightening, at least product refers to the literature1.

The suggestion of List.both here feels like a strawman argument. I'm not advocating for List.both as a readable name. In any case, I'd expect List.both to simply be an alias of List.zip.

In each case you need to learn something.

The standard library should minimize the need for developers to "learn something new". Functional programming has long faced criticism for its reliance on terminology that references category theory / abstract algebra, and we should avoid reinforcing this perception where possible.

Again the added term here is to satisfy an APPLICATIVE interface

The APPLICATIVE interface is not part of the stdlib (yet), so I also don't see how this constraint justifies the choice of product. A name like both would serve the same purpose.

Overall, in my opinion, Result.both would be more approachable for developers and stick to existing conventions in the community. Adopting the name here would reduce friction between standard libraries.

@dbuenzli
Copy link
Copy Markdown
Contributor Author

In any case, I'd expect List.both to simply be an alias of List.zip.

Personally I'd rather expect List.zip to be an alias of List.product.

The APPLICATIVE interface is not part of the stdlib (yet), so I also don't see how this constraint justifies the choice of product.

Because having the right name allows people outside from the standard library to define an APPLICATIVE over Result. That's the nice thing about a structural system. So I'd rather already have this name right for people who want to do that.

If people want to follow up with Result.both as an alias to Result.product that's fine. I'm just not going to add it myself because as was highlighted in my previous message, use of this function seems to be scarce enough so as not to justify its existence in my eyes.

@nojb
Copy link
Copy Markdown
Contributor

nojb commented Jan 12, 2025

OK, at this point I feel the discussion is diverging.

Discussions about which name is best are inherently subjective and as such there can be no objective "argument" that will win the day. Personally I have stopped worrying long ago about such things and just try to find names which enjoy some rough consensus and do not aspire to more than that. (And I believe this attitude has saved me quite a lot of time that could have been wasted in pointless discussions.)

With this in mind, I would suggest to stop discussing the naming of Result.product/Result.both and instead focus on the meat of the patch which is the introduction of Result.Syntax.

On this question, @gasche wondered about the interactions with the named binding operators of #13325 (see also #12015). Personally I see the relation between named binding operator (let/bind) and ordinary binding operators (let*) as being complementary. This is similar in my mind to the relation between named infix operators (mod) and symbolic infix operators (+). No-one would replace + by a "named" infix operator, but named infix operators are useful with other, less common, operations. So if I had to make a prediction, I would say that we will continue to use let* for the "bind" operation, and will use named binding operators for other, less common, operations. Given that, personally I feel confortable introducing the module suggested here into the standard library.

@dbuenzli
Copy link
Copy Markdown
Contributor Author

dbuenzli commented Jan 12, 2025

Given that, personally I feel confortable introducing the module suggested here into the standard library.

We are in agreement but I still think we should be forward looking here and think about a scheme were both named binding operators and unnamed ones can cohabit ergonomically if they become a reality. I don't think having both defined in the M.Syntax module would be very wise.

My proposal which was perhaps a bit lost above is to generally have the unnamed ones in:

open M.Syntax

and the named ones in:

open M.Syntax.Named

Perhaps this can make people who believe in named binding operators worry a little bit less about what is being introduced here.

@dbuenzli dbuenzli force-pushed the result-improvements branch from 7b47b72 to d600006 Compare March 15, 2025 08:55
@dbuenzli
Copy link
Copy Markdown
Contributor Author

This is also part of the plan of getting rid of my tiny and annoying to carry Result module overlay after 5.4 :-)

It has been two months now and I think a decision should be taken on that one, either we finally get that six years old proposal in or I'll simply close this.

Personally I can confidently say that the Result.Syntax module as proposed by this PR works very well in code bases (and that in context open Result.Syntax works better, as as a signifier, than let (let*) = Result.bind). Besides, as mentioned in the original message I'm also very confident that many of us will use this syntax whether or not named bindings operators are introduced and thus that the addition is justified.

For appeasing the believers in named binding operators, a suggestion was made for a convention to introduce them if they end up being added to language and this PR is accepted.

@hirrolot
Copy link
Copy Markdown
Contributor

+1, I'm very looking forward to seeing this sugar in the upcoming OCaml release.

@nojb
Copy link
Copy Markdown
Contributor

nojb commented Mar 27, 2025

xref #2170

I will add the question of adding Syntax submodules to the standard library to the agenda of next week's developer meeting and will report back here once the discussion has taken place.

This was discussed in today's dev meeting.

There was consensus around adding a submodule with the usual monadic operators let+, and+, let* and and* to the usual modules (Option, Result, List, Seq, ...). Time ran out before consensus could be reached around the exact name of the submodule unfortunately (Syntax, Notations, Op and O were all mentioned).

A PR consisting exclusively of the addition of these modules would be welcome, but expect some bikeshedding around the choice of the name of the submodule.

Thanks!

@dbuenzli
Copy link
Copy Markdown
Contributor Author

Personally I'm not interested in adding modules for List, Option and Seq, I never use these monads and find them rather useless. In the spirit of keeping the stdlib lean I do not add things I do not use.

The work is done here, can't we have the discussion about the name here and make this the PR for Result ?

I don't think we need a big bang PR. People can gradually add monads to the other modules later if they find it useful.

@dbuenzli
Copy link
Copy Markdown
Contributor Author

dbuenzli commented Mar 27, 2025

So, to challenge Syntax against the proposed names:

  • Notations is really weird to me. That's terminology nowhere to be found in the manual and I don't feel like it's necessary to invent new terminology here.
  • Infix, Op, Ops, Operators and O. If I'm defining an eDSL I may introduce much more than operators in scope via this module, I'm defining the Syntax of a embedded language. These other things could be bare functions (constructors), bare GADT constructors, and/or things like rebinding the list constructor syntax.

Hence Syntax. Syntax encompasses all cases of rebindable syntax and conveys well that we are defining the Syntax of an eDSL in there. Thus if you spot an open M.Syntax in a code base it clearly says:

Pay attention! Below, some of the usual operators (e.g. arithmetic), or usual bits of syntax let*, [], :: are possibly being redefined (see module definition for the details of course).

@nojb
Copy link
Copy Markdown
Contributor

nojb commented Mar 27, 2025

The work is done here, can't we have the discussion about the name here and make this the PR for Result ?

Sure, we can do that. Do you mind removing product from the interface in order to keep the discussion focused on the Syntax submodule? Having one name per PR to decide is enough as it is :)

To recapitulate, the question is: what should we call the submodule of Result containing the four standard monadic operators let+, and+, let* and and*?

The current proposal for the name is Result.Syntax (personally, am OK with it).

Other potential names for this submodule: Result.Infix, Result.Op, Result.Notations, Result.Op, Result.O, ...

cc some core devs who may have an opinion @gasche @chambart @lthls @yallop

@dbuenzli dbuenzli force-pushed the result-improvements branch from d600006 to 18ea176 Compare March 27, 2025 21:10
@dbuenzli dbuenzli changed the title Add Result.product and Result.Syntax Add Result.Syntax Mar 27, 2025
@dbuenzli
Copy link
Copy Markdown
Contributor Author

Done.

Other potential names for this submodule: Result.Infix, Result.Op, Result.Notations, Result.Op, Result.O, ...

A counter to these names can be found here.

@dbuenzli dbuenzli force-pushed the result-improvements branch from 18ea176 to 3a090e4 Compare March 27, 2025 21:34
@gasche
Copy link
Copy Markdown
Member

gasche commented Mar 27, 2025

In the maintainer meeting, we approved in principle the idea of Syntax submodules for binding operators (only) in the standard library, typically for List, Option, Result and Seq.

@nojb
Copy link
Copy Markdown
Contributor

nojb commented Mar 28, 2025

@gasche: according to my notes, there was some indecision about the name, but do I take it from your comment that Syntax was generally accepted? If that's the case, we can move to merge...

@johnyob
Copy link
Copy Markdown
Contributor

johnyob commented Mar 28, 2025

Other potential names for this submodule: Result.Infix, Result.Op, Result.Notations, Result.Op, Result.O

I think Syntax is perfectly good name for this module, but if there is scope for discussion, then what about Let_syntax? Its already adopted by Jane Street's base/core libraries as a convention and conveys that the syntax defined by the module is an eDSL using the standard monadic let operators let{+,*}/and{+,*}.

Another alternative could be Let_op (or Let_ops) -- which fits the existing naming of let operators and is less generic that Op.

@nojb
Copy link
Copy Markdown
Contributor

nojb commented Mar 28, 2025

I think Syntax is perfectly good name for this module,

That's good to hear. At this point I am willing to go with the choice of the author, unless there's a clear argument or wide consensus against it.

@dbuenzli
Copy link
Copy Markdown
Contributor Author

I think Syntax is perfectly good name for this module, but if there is scope for discussion, then what about Let_syntax?

My arguments against are similar to those exposed there against Ops and co.

Moreover, it would then become a reader's expectation that (let*) operators are introduced by Let_syntax modules. However as mentioned in that comment an eDSL may introduce much more. See for example this Rel_query.Syntax module which exposes a (let*) operator, overloaded arithmetic and boolean operators and overloaded standard library modules names.

Personally I don't think having fine grained names for syntax rebinding is useful. It feels needlessly bureaucratic to me, I rather have a good, all round, generic term for that.

@nojb
Copy link
Copy Markdown
Contributor

nojb commented Mar 28, 2025

For information, I will wait until end of day on Monday to give time for people to participate. If no substative arguments have come up by then, I am planning to merge the PR.

@dbuenzli dbuenzli force-pushed the result-improvements branch from 3a090e4 to 5e84b23 Compare March 28, 2025 14:08
Copy link
Copy Markdown
Member

@gasche gasche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is an approval in principle, but I'll let @nojb make more final decisions.

@gasche
Copy link
Copy Markdown
Member

gasche commented Mar 28, 2025

I wanted to ask if there is a pre-established justification for using Result.product rather than Result.pair. I see that in fact we already have Seq.product, so it makes sense to use the same name here.

@nojb
Copy link
Copy Markdown
Contributor

nojb commented Mar 28, 2025

Here is an approval in principle, but I'll let @nojb make more final decisions.

Thanks @gasche!

@dbuenzli
Copy link
Copy Markdown
Contributor Author

Just so that we go to the bottom of everything :-) I think that both Result.pair or Result.both which has been suggested above would be slightly misleading: the operation performed by product is more complex than these names suggest. Notably it leans on the left on Error _s

@nojb nojb merged commit 60b30b9 into ocaml:trunk Mar 31, 2025
22 checks passed
@nojb
Copy link
Copy Markdown
Contributor

nojb commented Mar 31, 2025

Merged. Thank you everyone for your participation.

@hirrolot
Copy link
Copy Markdown
Contributor

Wow...

@nojb nojb changed the title Add Result.Syntax Add Result.product and Result.Syntax Mar 31, 2025
@dbuenzli dbuenzli deleted the result-improvements branch March 31, 2025 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants