Skip to content

Add lift2 Invokable#302

Merged
gusty merged 17 commits into
masterfrom
gusty-lift2
Mar 27, 2020
Merged

Add lift2 Invokable#302
gusty merged 17 commits into
masterfrom
gusty-lift2

Conversation

@gusty

@gusty gusty commented Mar 22, 2020

Copy link
Copy Markdown
Member

At the moment lift2 is defined in terms of apply and map (or apply and result).

This PR use that definition as default (fallback) and allows to define it more efficiently for each instance.

With the future addition of applicative CEs and the recommendation of using mapN syntax instead of <!> .. <*> the need for specialized lift2 implementation to improve performance of internal applicative code, converges with the need to have efficient lift2 implementations ready to use in applicative CEs to take advantage of the new syntax.

Here we also add two methods to CEs which will be used in F#5 for applicative CEs.

@wallymathieu

Copy link
Copy Markdown
Member

So then the question is if this is something that should be part of a potential 1.1 release?

@gusty

gusty commented Mar 24, 2020

Copy link
Copy Markdown
Member Author

We could add the Lift2 invokable to a safe 1.1 release, but it won't integrate as well as here.
See, for instance I'm taking advantage of Lift2 for traversables, but the fallback to <!> .. <*> is not straight-forward and I don't think we could make it work with the 1.0 model of Invokables.

@wallymathieu

Copy link
Copy Markdown
Member

I'm thinking that we have the new style of invokables and a lot of hidden overloads that do not get called in 1.1, but provide backwards compatibility for 1.0

@gusty

gusty commented Mar 27, 2020

Copy link
Copy Markdown
Member Author

This (Travis only) failure is interesting:

A total of 1 test files matched the specified pattern.
  X choosed [9ms]
  Error Message:
   System.InvalidProgramException : Invalid IL code in FSharpPlus.Tests.Lens/choosed@58-11:Invoke (Microsoft.FSharp.Collections.FSharpList`1<int>): IL_0171: stloc.2   

CC: @dsyme

@gusty

gusty commented Mar 27, 2020

Copy link
Copy Markdown
Member Author

Trying to get the fallback from Lift2 to <!> and <*> seems to complicate a lot the constraints.

I give up, I will not use Lift2 in the (bi) generic traverse implementation.

Also there are some problems with LeftZero predicate, which also generate more constraints. Maybe we can do a run-time check instead.

@wallymathieu wallymathieu left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would be nice with a test of lift2 for Array

Comment thread src/FSharpPlus/Extensions/Array.fs
@wallymathieu

Copy link
Copy Markdown
Member

I think it looks fine 👍

@wallymathieu wallymathieu left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍

@gusty
gusty merged commit 4adf0f3 into master Mar 27, 2020
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.

2 participants