Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Using setl and view for the same optic in one function gives error "The type Data.Identity does not match the type Data.Const" #344

Open
cmeeren opened this issue Aug 28, 2020 · 23 comments

Comments

@cmeeren
Copy link
Contributor

cmeeren commented Aug 28, 2020

I'm trying to use optics to set a record field together with the record's UpdatedAt: DateTimeOffset field only if the field value has changed.

I have created the following generic function, which doesn't compile:

let inline private setField valueOptic value updatedAtOptic now source =
  let oldVal = view valueOptic source
  if value = oldVal then source
  else
    setl valueOptic value source
    |> setl updatedAtOptic now

image

What am I doing wrong?

@cmeeren cmeeren changed the title How to combine setl and view for the same optic in one function How to combine setl and view for the same optic in one function? Error: "The type Data.Identity does not match the type Data.Const" Aug 28, 2020
@cmeeren cmeeren changed the title How to combine setl and view for the same optic in one function? Error: "The type Data.Identity does not match the type Data.Const" Using setl and view for the same optic in one function gives error "The type Data.Identity does not match the type Data.Const" Aug 28, 2020
@cmeeren
Copy link
Contributor Author

cmeeren commented Aug 28, 2020

There is of course a workaround by passing the optic twice as two separate arguments to the function and using one with setl and the other with view, but that defeats some of the purpose of lenses (combining get and set).

Is it at all possible to use setl and view with a single optic in a single function? Is there perhaps a better way to accomplish what I am attempting above?

@wallymathieu
Copy link
Member

This might be a bug, though I'm unsure.

@wallymathieu
Copy link
Member

Looks like this is how it should be (the compiler error). I'll see if I can find how this should be solved.

@cmeeren
Copy link
Contributor Author

cmeeren commented Aug 29, 2020

Great, thanks a lot! Looking forward to it. I've used FSharpPlus a while and just started experimenting with lenses and I like the concept, but this bug is throwing a wrench in the works 😛

@wallymathieu
Copy link
Member

wallymathieu commented Aug 29, 2020

When I tried to do the equivalent in Haskell:

setField valueOptic value updatedAtOptic now source =
  let oldVal = view valueOptic source in
    if value == oldVal then source
    else
        set valueOptic value source 

I got a similar compilation error:

Couldn't match type ‘Identity s’ with ‘Const b s’
  Expected type: Getting b s b
    Actual type: ASetter s s b b

From what I gathered, the solution to how to solve it in Haskell would be to use lens signature.

Since the lens signature is using Rank-2 types in Haskell, I'm unsure if that's possible to express in F# (why that solution might be a dud). @gusty usually knows more about these things, so might have a better answer.

@wallymathieu
Copy link
Member

What is it that you are trying to do @cmeeren ? Would it be solved by using less generic data lenses as seen in F#x?

@cmeeren
Copy link
Contributor Author

cmeeren commented Aug 29, 2020

As I said at the top:

I'm trying to use optics to set a record field together with the record's UpdatedAt: DateTimeOffset field only if the field value has changed.

In other words: I want to update a field, and if the field has changed (if the new value is different than the old), also update another field. (My use-case is slightly more complex than this; I am also setting yet another field with data that contains the old and new values of the first field.)

If this explanation is insufficient, please let me know where you're confused and I'll explain in more detail.

Please also let me know if there is another way (than what I have shown at the top) to achieve this using the lens features of FSharpPlus.

I have not tried FSharpx. My needs are not complex, and I think that at the moment, I can get away with not only something less generic, but without lens composition at all. I may therefore be able to just use ad-hoc get/set functions. Or perhaps I can try Aether. I'd like to keep external dependencies to a minimum, though, and since FSharpPlus already has excellent support for lenses (barring this problem) using fairly standardized concepts, it would be great to just use FSharpPlus for this to avoid more mental overhead.

@wallymathieu
Copy link
Member

Aether style lenses are equivalent to the lenses that can be found in FSharpX. Note the similarity of the type signatures of the two. They encode the same type of abstraction.

Since I've needed to use that style of lenses in a library intended to be used by C#.

@gusty
Copy link
Member

gusty commented Aug 29, 2020

Technically speaking there is a problem with rank2 types as the lens is applied for both reading and writing which means that a generic function passed as parameter is being instantiated twice but with different types. Of course a traditional solution is to pass it twice but it's not always a good compromise.

Now, on the philosophical field, I think the use case is not fully aligned with way lens are expected to be used.
The idea is that you can compose them and always delay the operation (read, write or update) but here you are creating an operation accepting a lens. If you write a bit more of code I might be able to suggest a different way.

Regarding using a Data.Lens implementation (F#x or Aether) I don't think it would solve the issue. Note that that implementation is less generic than this one. You can do the same they do here by using a record of operations and the helper function lens that create a lens from those operations.

@cmeeren
Copy link
Contributor Author

cmeeren commented Aug 29, 2020

I think the use case is not fully aligned with way lens are expected to be used. … If you write a bit more of code I might be able to suggest a different way.

You may be right. This may very well be my having discovered a new shiny screwdriver and attempting to hammer nails with it. I appreciate your wanting to take a look at a better solution.

The following example demonstrates the principle of what I want to achieve: Create setters (the two public functions at the bottom) that, if and only if the new value is actually different from the old value, sets the new value, sets the record's UpdatedAt field, and adds the old value to the corresponding field with the history of all values of that field. (This last part may sound a bit artificial; in reality the use-case is a bit different, but the code is similar.)

Note that I pass the optic twice here to work around the current issue.

module Lens

open System
open FSharpPlus
open FSharpPlus.Lens


[<AutoOpen>]
module GenericStuff =

  let inline setFieldWithUpdatedAtAndHistory optic1 optic2 value updatedAtOptic now historyOptic source =
    let oldValue = view optic1 source
    if value = oldValue then source
    else
      setl optic2 value source
      |> setl updatedAtOptic now
      |> fun updatedSource ->
          updatedSource |> over historyOptic (flip List.append [oldValue])


type Person = {
  Name: string
  Age: int
  UpdatedAt: DateTimeOffset
  NameHistory: string list
  AgeHistory: int list
}


module Person =


  let inline private _name f p =
    map (fun v -> { p with Name = v }) (f p.Name)

  let inline private _age f p =
    map (fun v -> { p with Age = v }) (f p.Age)

  let inline private _updatedAt f p =
    map (fun v -> { p with UpdatedAt = v }) (f p.UpdatedAt)

  let inline private _nameHistory f p =
    map (fun v -> { p with NameHistory = v }) (f p.NameHistory)

  let inline private _ageHistory f p =
    map (fun v -> { p with AgeHistory = v }) (f p.AgeHistory)


  let setName now value person =
    setFieldWithUpdatedAtAndHistory
      _name _name value _updatedAt now _nameHistory person


  let setAge now value person =
    setFieldWithUpdatedAtAndHistory
      _age _age value _updatedAt now _ageHistory person

@wallymathieu
Copy link
Member

wallymathieu commented Aug 29, 2020

Even though F# does not have rankN, F# does have at least side effects:

let inline setField valueOptic value updatedAtOptic now source =
    let mutable eq1=false
    let v1= over valueOptic (fun x -> 
                                      eq1 <- x = value
                                      value
                            ) source
    if eq1 then v1
    else
        v1 |> setl updatedAtOptic now

type Person2 = {
    Name: string
    DateOfBirth: DateTime
    UpdatedAt: DateTime
}

module Person2=
    let inline _name f { Name = a; DateOfBirth = b; UpdatedAt=c } = f a <&> fun a' -> { Name = a'; DateOfBirth = b; UpdatedAt=c }
    let inline _updatedAt f { Name = a; DateOfBirth = b; UpdatedAt=c } = f c <&> fun c' -> { Name = a; DateOfBirth = b; UpdatedAt=c' }

let p = { Name="test"; DateOfBirth= DateTime (1914, 8, 26); UpdatedAt=DateTime (1914, 8, 26) }

let v1=setField Person2._name "new name" Person2._updatedAt (DateTime.Now) p
let v2=setField Person2._name "test" Person2._updatedAt (DateTime.Now) p

I find my code kind of ugly, but perhaps it can be of help.

@gusty
Copy link
Member

gusty commented Aug 29, 2020

I think there must be a way to compose the operation conditionally but probably it will result in combinatory code which is hard to read.

@wallymathieu solution reads well and surely will be more efficient than that.

@cmeeren
Copy link
Contributor Author

cmeeren commented Aug 29, 2020

@wallymathieu Thanks for the mutable tip. Still, it certainly feels like a workaround, not a proper solution. In any case, I have adopted it for now as my chosen workaround.

Also, again, if I'm trying to hammer a nail with a screwdriver by using optics in this way, please let me know.

@nikolamilekic
Copy link

The problem is that setl and view expect different functors and that the lens gets bound by the compiler to the first one you use in the setField function. The lens signature thus does not match for the second call. In OOP we solve similar problems by setting the signature to the base class, but the Identity and Const functors don't have a base class that also conforms to the lenses' expectations (static member Map). So how about you create one?

[<AbstractClass>]
type Functor<'a>() =
    abstract member Select<'b> : ('a -> 'b) -> Functor<'b>
    static member Map(x : Functor<'a>, f : 'a -> 'b) : Functor<'b> = x.Select(f)

type IdentityFunctor<'a>(value : 'a) =
    inherit Functor<'a>()
    member __.Run = value
    override __.Select<'b> (f : 'a -> 'b) = IdentityFunctor(f value) :> Functor<'b>

type ConstFunctor<'p, 'a>(value : 'p) =
    inherit Functor<'a>()
    member __.Run = value
    override __.Select<'b> (f : 'a -> 'b) = ConstFunctor(value) :> Functor<'b>

let setl' optic value (source :'s) : 't =
    let (x : Functor<'t>) =
        optic (fun _ -> IdentityFunctor value :> Functor<'v>) source
    (x :?> IdentityFunctor<'t>).Run

let view' optic (source :'s) : 'a =
    let (x : Functor<'t>) =
        optic (fun x -> ConstFunctor x :> Functor<'b>) source
    (x :?> ConstFunctor<'a, 't>).Run

let setField valueOptic value updatedAtOptic now source =
    let oldVal = view' valueOptic source
    if value = oldVal then source
    else
        setl' valueOptic value source
        |> setl' updatedAtOptic now

Actual lens definitions don't need to change because Functor has static member Map.

@cmeeren
Copy link
Contributor Author

cmeeren commented Aug 30, 2020

@nikolamilekic That was a tip for @wallymathieu / @gusty for a change in FSharpPlus, right? Not in user code?

@nikolamilekic
Copy link

I actually meant it as a solution to your specific problem, so code you'd place in your own library. It's how I would solve it.

I'm an F#+ newbie, so I'm not comfortable making any general design suggestions, especially ones that would mean a significant departure from the current coding style (I think all other actual types are implemented as discriminated unions).

@cmeeren
Copy link
Contributor Author

cmeeren commented Aug 30, 2020

Thanks. It seems heavyweight enough that I'd rather just use the mutable workaround previously suggested, and I'd rather avoid adding my own types and functions mirroring those of FSharpPlus.

Would be great it FSharpPlus could solve this natively, though. (Or, again, illuminate whether lenses is the wrong tool for this kind of job.)

@gusty
Copy link
Member

gusty commented Aug 30, 2020

That was a tip for @wallymathieu / @gusty for a change in FSharpPlus, right? Not in user code?

We can consider using those functors for lensing but of course, we'll need to make sure all other optics don't break.
Testing should be added although there is some lens coverage already, but we'll need to add test for stuff that was previously not allowed, like your use case. Feel free to open PR or a draft PR to play with it.

Would be great it FSharpPlus could solve this natively,

We can't fix type system limitations (like rank-n types) but we can certainly explore tricks like the suggested one that makes a better user experience.

or illuminate whether lenses is the wrong tool for this kind of job

TBH I'm not an expert in lenses, it is an abstraction that it's not specific to this library or to F# so I would try to find guidance in other places as well, for instance I asked your question in the FP slack, in the #Lens channel as I'm also curious to see if/how lens would help in that situation.

My feeling is that we need a lens that encapsulate that behavior, we can certainly create it like this:

let inline _nameWithUpdate now f p =
    map (fun v -> if v = p.Name then { p with Name = v } else { p with Name = v; UpdatedAt = now; NameHistory = p.Name::p.NameHistory }) (f p.Name)

but I understand that you want to derive it in a general way, and I think that would involve using other functors in anycase. So if you're happy with the mutable solution stick to it. I personally like it because yes it does a mutation but it's encapsulated inside the function, it doesn't leak outside, as opposed to the solution of passing twice the lens.

@cmeeren
Copy link
Contributor Author

cmeeren commented Aug 31, 2020

Thanks. Indeed, a lens could encapsulate the entire behavior. I'll experiment a bit, but as you say, I think I'm after a more general/combinatorial solution – if I coded this into every lens, I could just as well not use lenses and instead encode the desired behavior in every top-level setter.

@cmeeren
Copy link
Contributor Author

cmeeren commented Aug 31, 2020

Another problem with your suggestion is that it is impossible to get the lens in the first place without having a DateTimeOffset for the now parameter, which shouldn't be needed when using view. So perhaps coding that kind of behavior directly into the lens breaks the lens abstraction (i.e., it is no longer just a get/set abstraction).

@gusty
Copy link
Member

gusty commented Aug 31, 2020

This wasn't a suggestion, in the sense that I don't suggest doing like that, it was rather a thought exercise to define what we would get as a lens, if we manage to compose it in a generic way. I still don't know how, so I did it by-hand.

But it's true, you would have to supply the now parameter all the time. Actually that lens definition includes the now parameter, I mean the now parameter it's partially applied and needs to be supplied in order to get the lens. So yes, it would be pointless for reading.

@cmeeren
Copy link
Contributor Author

cmeeren commented Aug 31, 2020

Indeed. Just to be clear, I wasn't criticizing; the intention was just to share my experience after experimenting a bit. Thanks for all the help so far.

@gusty
Copy link
Member

gusty commented Aug 31, 2020

A similar technical problem was reported by @reinux on our gitter 2 weeks ago:

Hey, hopefully quick question: is there a type alias I can use for lenses? I'm trying to pass a lens as an argument, but it results in a type error when I try to use view and setl together. Something like this:

let f lens x =
  printfn "%s" (view lens x)
  setl lens "abc" x

The fix suggested by @nikolamilekic should apply here as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants