fsprojects / FSharp.Compiler.PortaCode

The PortaCode F# code format and corresponding interpreter. Used by Fabulous and others.
Other
42 stars 11 forks source link

Calling Resolved/Erased function fails at EvalUnionCaseTest #13

Closed tylerhartwig closed 5 years ago

tylerhartwig commented 5 years ago

Hi,

Following the guide of Fabulous Live Update and Giraffe.HotReload, I'm trying to implement HotReload for Bolero Elmish. I've been able to successfully resolve the "update" function from the MVU pipeline, but when I call the erased function, the following exception occurs:

Screen Shot 2019-04-07 at 12 13 11 PM

What's interesting is that it's resolving the actual union value, rather than the structure describing it.

The code I'm attempting to hot reload is as follows:

type Model =
    {
        value: int
    }

let initModel =
    {
        value = 0
    }

type Message =
    | Increment
    | Decrement

let update (message : Message) (model : Model) =
    match message with
    | Increment ->
        { model with value = model.value + 1 }, Cmd.none
    | Decrement ->
        { model with value = model.value - 1 }, Cmd.none

let view model dispatch =
    concat [
        button [on.click (fun _ -> dispatch Decrement)] [text "-"]
        span [] [textf " %i " model.value]
        button [on.click (fun _ -> dispatch Increment)] [text "+"]
    ]

type Updater(update) =
    member __.UniqueUpdate message model = update message model

let myHotReload = Updater(update)

And the code that is resolving the myHotReload value:

    let handleUpdate (updater : ProgramUpdater<'msg, 'model>) (files : (string * DFile)[]) =
        printfn "Created interpreter! %A" interpreter

        lock interpreter (fun () ->
            files |> Array.iter (fun (fileName, file) -> interpreter.AddDecls file.Code)
            files |> Array.iter (fun (fileName, file) -> interpreter.EvalDecls (envEmpty, file.Code))
          )

        let mem = tryFindMemberInFiles "myHotReload" files
        match mem with
        | Some (def, expr) ->
            try
                printfn "Found member!"
                let entity = interpreter.ResolveEntity(def.EnclosingEntity)
                printfn "Got entity! %A" entity

                let (def, memberValue) = interpreter.GetExprDeclResult(entity, def.Name)
                printfn "Got member value! %A" memberValue

                let value = getVal memberValue
                printfn "Got Value!: %A" value

                match value with
                | :? ObjectValue as x ->
                    let (ObjectValue v) = x
                    let updater = Map.find "update" v.Value

                    printfn "Found update %A" updater
                    let erasedUpdater = updater :?> obj -> obj -> obj * Cmd<obj>

                    printfn "Successfully cast: %A" erasedUpdater
                    let model = initModel
                    let message = Message.Increment

                    printfn "Calling update"
                    let newSet = erasedUpdater message model
                    printfn "Got new value %A" newSet
                | :? Updater as updater ->
                    printfn "Found Updater!"
                    let model = initModel
                    let message = Message.Increment

                    printfn "calling Updater!"
                    let newSet = updater.UniqueUpdate message model
                    printfn "Got new values! %A" newSet

            with e ->
                printfn "Got exception: %A" e

        | None ->
            printfn "could not find member"

More interesting details, the value here is resolving as an ObjectValue rather than as the Updater type as I expected. I've tried resolving the update function various ways as well, packaged in a type, as a method itself, however I always receive the previously mentioned exception.

I'm not sure if there is an issue in the way I'm resolving the types, or a bug in the interpreter, any advice or help would be greatly appreciated. Thanks :)

EDIT:

Update, the reason the object was resolving as ObjectValue is because of how I set up my projects, I've fixed that now, but the issue still remains.

dsyme commented 5 years ago

@tylerhartwig It's hard for me to see the bug from the above. Is there a way to create a stand-alone repro that is a PortaCode test case?

tylerhartwig commented 5 years ago

@dsyme

I have a pretty minimal repository with the setup here: https://github.com/tylerhartwig/Bolero.HotReload -- The HotReload.Listener is what I've been using to do the interpreting and debugging, with HotReload.Elmish being the code which gets reloaded.

I'll look into the tests today and see if I can come up with a proper PortaCode test for it tonight.

dsyme commented 5 years ago

So on this line do you get an Updater or a UnionValue? I think I'd expect to see a UnionValue here? (I might be wrong, hard to tell without running)

tylerhartwig commented 5 years ago

I get an Updater on that line.

The problem lies in this line

Here this line is returning the actual union instantiation (of type Message), in this case Increment or Decrement. If I'm understanding the code correctly, it should be of UnionValue type instead.

dsyme commented 5 years ago

@tylerhartwig Yes, I can't see why that wouldn't be a UnionValue. All the branches on which Message was produced should wrap in UnionValue. Odd. Suggest debugging (breakpointing) the creation/consumption of the message values

tylerhartwig commented 5 years ago

@dsyme Yes, when I debugged this I followed it down to resolving the Message value from this line.

Is this a cached value? or is the value being resolved here? I'm guessing it's a cached value and that I'll need to back-track and find where it's being resolved and added to the environment.

dsyme commented 5 years ago

Is this a cached value? or is the value being resolved here? I'm guessing it's a cached value and that I'll need to back-track and find where it's being resolved and added to the environment.

I think that's just reading the value from a local. You'd have to trace back further.

But it's just very hard to see how an unwrapped Message object could be produced - the EvalNewUnionCase wraps it, and no one unwraps it except EvalUnionCaseTest and EvalUnionCaseGet, both of which consume (discard) the unwrapped value.

tylerhartwig commented 5 years ago

Okay thanks, this helps a lot, I'll jump in again tonight and see how it's getting unwrapped and added :)

tylerhartwig commented 5 years ago

Okay, after a little research it's getting added to the Vals map on the environment here on the bind call in the dynamically made function from EvalLambda (I'm guessing this isn't too surprising).

We jump into that anonymous function after making this call (listed below as well).

    let updater = handleUpdate files
    let newSet = updater.Update (Decrement) (initModel)
    printfn "Got new set: %A" newSet

Am I calling this method wrong? do I need to wrap the arguments myself before passing it into the interpreted function? or do I need to call the function via the interpreter in some fashion?

The best I could tell from Fabulous and HotReload.Giraffe I just needed to call it normally, but I very well could have missed something.

dsyme commented 5 years ago

@tylerhartwig I'm not totally sure - am I right that updater has type ProgramUpdater<obj,obj...> and the use of obj might be obscuring something here?

Getting the "update" function field from the object here feels suspicious, it's like accessing the internals of the interpreter. https://github.com/tylerhartwig/Bolero.HotReload/blob/master/src/HotReload.Library/Library.fs#L92. If you could simplify things so that you didn't do that the issue might go away

tylerhartwig commented 5 years ago

@dsyme No, in this case it is of type Updater.

I previously was pulling the function out of the internals because I did not setup my project references correctly and the library did not have proper knowledge of the type to resolve it correctly. I should delete that from the repo now (I'll update it now).

Regardless, calling it from the internals, calling it from within an object, and resolving it directly as a function have all yielded this same exception. 😞

dsyme commented 5 years ago

BTW you might also want to look at FCSWatch eg. https://github.com/humhei/FCSWatch/

dsyme commented 5 years ago

Also since you may be able to compile-on-device with Bolero maybe you don't need PortaCode at all? Just wondering.

tylerhartwig commented 5 years ago

Interesting, I've not explored these other two ideas. FSCWatch seems intriguing and may be a great boon for development with Bolero.

Part of the attractive part with PortaCode is I could likely maintain the current Elmish model when swapping out the View and Update layers. Since I'm unfamiliar with the other methods I'm not sure if they could do that as well. Additionally following Giraffe.HotReload seemed very straightforward.

tylerhartwig commented 5 years ago

@dsyme

I was reviewing this again just now, I think I could potentially fix this bug (if it's a bug).

Is it right for EvalLambda to receive unwrapped union case values? Or should it always deal with wrapped values?

In this case it seems that UnionCaseTest should be able to support comparing against unwrapped union case values (I believe this is used to interpret pattern matches on unions).

If not, then whatever is calling the method being created in EvalLambda should be wrapping a value and is not. Going to try and write a fix tonight, and want to make sure I take the correct approach.

dsyme commented 5 years ago

Is it right for EvalLambda to receive unwrapped union case values? Or should it always deal with wrapped values?

In this case it seems that UnionCaseTest should be able to support comparing against unwrapped union case values (I believe this is used to interpret pattern matches on unions).

I think we should never see unwrapped union values (i.e. without UnionCase) for union types defined in the interpreted code.

tylerhartwig commented 5 years ago
  • No for union values defined in the file being interpreted. They should always be UnionValue

I believe this is where the problems lies in my case, Message is a type in a file being interpreted. It should be the job of the object or dynamically created instance method to wrap this in a UnionValue, and I'm guessing that's where the solution would need to happen.

dsyme commented 5 years ago

I believe this is where the problems lies in my case, Message is a type in a file being interpreted. It should be the job of the object or dynamically created instance method to wrap this in a UnionValue, and I'm guessing that's where the solution would need to happen.

I'm still not understanding the origin of this value, sorry. The only place in the interpreter (NewUnionCase) where we create a union value we do wrap in UnionValue.

So am wondering - where else is this value being created?

tylerhartwig commented 5 years ago

Sorry, I probably didn't explain it very clearly.

  1. I resolve the value I want to work here
  2. I call this method, passing in the hard, boxed types (of type Message and Model) here
  3. The resolved, interpreted object call bind here This is the point at which the unwrapped, Message and Model values end up in the env
  4. EvalUnionCaseTest retrieves the unwrapped Message and Model values here

I really appreciate all the help :)

dsyme commented 5 years ago

Is this file also being interpreted?

tylerhartwig commented 5 years ago

No, this is the one utilizing the Hot Reload functionality.

dsyme commented 5 years ago

OK - well, this isn't really supported. You're passing in a value based on a "compiled" version of the type into interpreted code.

When invoking the interpreted code, you should not pass in any values related to the interpreted code unless those values were created by the interpreter.

tylerhartwig commented 5 years ago

Interesting, I must be mis-understanding how Giraffe.HotReload and Fabulous swap out their runners then. Nonetheless it should be easy enough to derive the correct types to pass the update function using the interpreter.

tylerhartwig commented 5 years ago

Closing this as I believe it's been resolved, but I'll summarize my understanding of this problem.

Any functions, classes, or code that is interpreted, will expect interpreted values and return interpreted values/types, if those types are included in the files being interpreted.

However, HotReload.Giraffe uses the interpretation to produce and use a Giraffe-specific type that can be used with compiled code. I assume this is because this type is resolved differently because the Giraffe library is not being interpreted in this case.