FSharpPlus icon indicating copy to clipboard operation
FSharpPlus copied to clipboard

Currying error on the wrong side

Open gusty opened this issue 5 years ago • 19 comments

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 avatar Jan 21 '20 13:01 gusty

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.

gusty avatar Jan 21 '20 13:01 gusty

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.

adz avatar Jan 21 '20 14:01 adz

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 assume Choice and Either types to have their types reversed.
  • When we create a fake HKT for an abstraction we put the arguments in the correct order.
  • Therefore, a Bifoldable<'T1,'T2> correspond to Choice<'T2,'T1> and Result<'T2,'T1>.
  • Also, Bifunctor should follow the same rules, but we're a bit late as that would be a breaking change. Indeed this reveals that our bimap for Result is reversed, normally the first function maps on the Error.
  • Bifunctor and Bifoldable should be consistent as there are some rules involving both.
  • ArrowChoice is also affected, it looks like it is already reversed but I would say it needs to be reviewed.
  • All these changes would impact Lens.

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?

gusty avatar Jan 21 '20 18:01 gusty

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

wallymathieu avatar Jan 21 '20 18:01 wallymathieu

Sounds like a solid plan (2).

wallymathieu avatar Jan 21 '20 18:01 wallymathieu

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

wallymathieu avatar Jan 21 '20 19:01 wallymathieu

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.

gusty avatar Jan 22 '20 22:01 gusty

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 avatar Jan 23 '20 06:01 wallymathieu

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

wallymathieu avatar Jan 23 '20 06:01 wallymathieu

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 ?

gusty avatar Jan 23 '20 08:01 gusty

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

wallymathieu avatar Jan 23 '20 08:01 wallymathieu

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.

gusty avatar Jan 23 '20 08:01 gusty

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

wallymathieu avatar Jan 23 '20 08:01 wallymathieu

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

gusty avatar Jan 23 '20 08:01 gusty

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.

wallymathieu avatar Jan 23 '20 08:01 wallymathieu

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

kspeakman avatar Jan 24 '20 17:01 kspeakman

@kspeakman thanks for sharing your thoughts.

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

Before:

  • first was mapping on the "value" side.

  • second was mapping on the "value" side as well.

  • bimap was mapping the first argument on the "value" side, the second on the "error". Now:

  • first is mapping on the "error" side.

  • second is still mapping on the "value" side (No changes here)

  • bimap is mapping the second argument on the "value" side, the first on the "error".

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

gusty avatar Jan 24 '20 17:01 gusty

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

kspeakman avatar Jan 24 '20 18:01 kspeakman

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

gusty avatar Jan 24 '20 19:01 gusty