fsprojects / FSharpPlus

Extensions for F#
https://fsprojects.github.io/FSharpPlus
Apache License 2.0
849 stars 95 forks source link

Currying error on the wrong side #245

Open gusty opened 4 years ago

gusty commented 4 years ago

This story tracks back to the first release of F# where the function Async.Catch was added, using the type on the left for the "right" value.

This in a type system with HKT support is the wrong decision, as we're interested in currying types, then we want to have the good value on the rightmost position.

Now, in F# land we don't have real HKTs so that's not a technical restriction, just theorotical at the moment.

So, people started adopting it, then the new Result type landed in F# and guess what? They did it again. Left type for the right type.

Now, people is asking for some Result functions added to F# core. This is when the problem becomes a real problem, because here we're not talking about types, we're talking about actual values.

For values, it's fair to think that's also convenient to curry on the good one. See this comment from @kspeakman https://github.com/fsharp/fslang-suggestions/issues/526#issuecomment-393539604

Now the problem is we still don't know what would be the standard for F# it's probably a bit early. Here in F#+ we did the either function with the error on the right, we can change it in V2 but now we're about to ship bifoldables, so the question is do we want to swap the functions? If the answer is yes, we can still do it for v1.1 but not later.

gusty commented 4 years ago

I just had a look at other libraries. It doesn't seem to be a clear direction, FsToolkit seems to mix both approaches for instance.

My vote goes for fixing bifoldable for now, so that the right paramenter correspond to the left type, and so on.

adz commented 4 years ago

It was easy for me to follow either signature as it matched type Result<'T,'TError> = of F#.

Feels a little opposite for functions to work in reverse order, even though I can see the argument.

In the end, I would go with consistency, and not too concerned as usually compiler helps you out with which if 'a and which is 'b.

gusty commented 4 years ago

as usually compiler helps you out with which if 'a and which is 'b.

Yes. But I would say this problem becomes important when writing generic code.

Let's try to draft some consistency rules:

We're a bit late now, either we:

  1. Do reverse Bifoldable, it would be out of sync with other abstractions that will be corrected in v2.0
  2. Leave Bifoldable consistent with the existing abstractions. Eventually for v2.0 all will be swapped.
  3. We don't add Bifoldable until v2.0 to avoid conflict.
  4. We do add Bifoldable but not the instances for Either and Choice which are the conflicting ones.

Thoughts?

wallymathieu commented 4 years ago

It's a bit of a pain since as you say Choice doesn't map well to Either

wallymathieu commented 4 years ago

Sounds like a solid plan (2).

wallymathieu commented 4 years ago

Though, question is what kind of preferences are shown in the f# community around order for bimap?

gusty commented 4 years ago

It looks like the situation is worst than what I thought initially.

Bifunctor instances for Result and Choice are definitely wrong, and I don't mean "wrong", like using the wrong convention.

I mean this:

> let x : Result<_,string> = Ok 0;;
[<Struct>]
val x : Result<int,string> = Ok 0

> first ((+) 10) x ;;
[<Struct>]
val it : Result<int,string> = Ok 10

> second ((+) 10) x ;;
[<Struct>]
val it : Result<int,string> = Ok 10

Also, the fact that nobody noticed makes me think that nobody is using it, at least extensively.

So ... we have to fix it. It would be a breaking change? Maybe, but I would rather say it's a bug fix.

Then we can also fix Bifoldable.

For F#+ v2.0 the question is. Should we keep default mapping on last (and pretending Choice is reversed) or should we radically change the approach and default mapping on first.

For this question, tuples have to be considered. Specially if we're going to introduce more generic code for tuples. We already have mapFirst and mapSecond for n-tuples). So right now (including this coming fix):

map = rmap = second = "mapLast"

If we switch to map on first

map = lmap = first = mapFirst

Sounds better, but it could be really a mess considering reading material from other languages and also when currying values.

wallymathieu commented 4 years ago

It behaves as I would expect. So it could be that it's defined along the intuition that would make sense from an f# perspective looking at type signatures.

wallymathieu commented 4 years ago

The Choice structure has a definite semantic order. Result does not.

gusty commented 4 years ago

The Choice structure has a definite semantic order. Result does not.

That's why I think that Result is not a big issue. Apart from the type params order there's not other big confusion, but for Choice things are worst as there is an idea of first and second already.

It behaves as I would expect.

Do you really expect the function first to be the same as second? And only for Choice and Result ?

wallymathieu commented 4 years ago

No, I would expect the second to be an Error for Result. I read the code a bit quickly, and only noticed that first returned Ok

gusty commented 4 years ago

That's the problem.

Then once fixed, you'll have to change your expectation of getting an Ok with first.

Unless we want to change the whole criteria to "default map on first" instead of "last". Which would definitely be a big breaking change. Neverthless we could do it for v2.

wallymathieu commented 4 years ago

My feeling is that Choice should follow the semantic meaning (I'm unsure about the value of having Choice as an alternate to Result when you can do MyChoiceConvention.toResult).

gusty commented 4 years ago

(I'm unsure about the value of having Choice as an alternate to Result

I'm convinced that it's the wrong type to do error stuff and I would love to remove all instances for Choice.

The problem is that since the infamous Async.Catch a lot of F# code sat on that.

wallymathieu commented 4 years ago

Sounds like the logical choice then is to document the weirdness and live with it. It's a bit of an ugly remnant, but what to do.

kspeakman commented 4 years ago

So the two sides presented so far are fix it or not fix it. But there is a 3rd option. Do both, but namespace the fixed version differently. That way nobody is broken, but users can take advantage of the "correct" syntax. TaskBuilder did something like this where it has a different namespace (V2 IIRC).

gusty commented 4 years ago

@kspeakman thanks for sharing your thoughts.

What we did so far is to swap the bifunctor functions for Choice and Result:

Before:

It's a bit tricky to do the changes under a different namespace here as we would have to come up with an alternative for FSharpPlus.Operators, make it the default module from now on and almost duplicate it, then create an overload trick to make the generic function behave one way or another, based on the entry call.

Still, that's certainly an option.

On the other side, the current behavior is obviously buggy. The fact that nobody complained so far makes me think that nobody is using bimap for choices and results, and I wouldn't be surprised, I think it's more common for tuples. I personally am not using it at all.

However I'm still open to different opinions as I'm not releasing RC2 today (but maybe tomorrow).

kspeakman commented 4 years ago

I should have clarified that I'm not suggesting that you should introduce a new namespace. But that it is an option to evaluate. :)

gusty commented 4 years ago

Definitely, I agree. And it's doable if we spent enough time with it.