fable-compiler / Fable

F# to JavaScript, TypeScript, Python, Rust and Dart Compiler
http://fable.io/
MIT License
2.93k stars 301 forks source link

Generate a unique name for anonymous record types #3005

Open MangelMaxime opened 2 years ago

MangelMaxime commented 2 years ago

Description

This issue has been reported in https://github.com/thoth-org/Thoth.Json/issues/144

Repro code

open Fable.Core.JsInterop
open FSharp.Reflection
open Fable.Core.DynamicExtensions
open Fable.Core

let inline log x = JS.console.log x

let anonymousValue =
    {|
        A = {| B = "content" |}
    |}

let anonymousType = anonymousValue.GetType()

type RecordB =
    {
        B : string
    }

type RecordA =
    {
        A : RecordB
    }

let realValue =
    {
        A = { B = "content" }
    }

let realType = realValue.GetType()

log (FSharpType.IsRecord(anonymousType, allowAccessToPrivateRepresentation =  true))

let test (typ : System.Type)=
    FSharpType.GetRecordFields(typ, allowAccessToPrivateRepresentation=true)
    |> Array.map (fun fi ->
        let targetKey = fi.Name
        log fi.PropertyType
    )
    |> ignore

test anonymousType // TypeInfo { fullname : '', fields: () => fields }
test realType // TypeInfo { fullname : 'Test.RecordB', fields: () => [[ "B", string_type ]] }

See, how the test anonymousType doesn't have the "B" field information reported.

REPL link

Expected and actual results

We should have access to the nested anonymous record information.

Related information

alfonsogarciacaro commented 2 years ago

I need to check how it works in Thoth.Json. In the example above, the reason just seems to be GetRecordFields is not called in the nested record. If I change the last lines I do get the information for the nested record.

let rec printFields (indent: string) (typ : System.Type) =
    for fi in FSharpType.GetRecordFields(typ) do
        let fiType = fi.PropertyType
        printfn $"{indent}{fi.Name}: {fiType.FullName}"
        if FSharpType.IsRecord(fiType) then
            printFields (indent + "    ") fiType

printFields "" anonymousType
printFields "" realType

// OUTPUT:
// A: 
//     B: System.String
// A: Test.RecordB
//     B: System.String
alfonsogarciacaro commented 2 years ago

Ok, I realized the problem is Fable gives an empty name to anonymous records. Although this is an inconsistency with dotnet and maybe we should fix it, for now I sent a PR with a workaround to Thoth.Json: https://github.com/thoth-org/Thoth.Json/pull/145

MangelMaxime commented 2 years ago

@alfonsogarciacaro

Would be nice to have Fable have the same behavior as in .NET, because I have a plan to rewrite Thoth.Json to support both Fable and .NET runtime from the same library.

So if things works differently between both runtime, it can lead to subtle errors. 🙂

Can we please re-open this issue to keep track of this problem?

alfonsogarciacaro commented 2 years ago

The proposed fix will also work in .NET :)

It probably makes sense to give a name to anonymous records with a hash based on field names and types. But please note it won't be exactly the same as .NET (same as type FullNames don't include assembly info in Fable) because .NET F# creates an actual class for anonymous records while Fable just use Pojos.

MangelMaxime commented 2 years ago

The proposed fix will also work in .NET :)

Yes, but there would still be a difference in term of behavior. On Fable, it will not be able to cache the decoder/encoder while on .Net it would cache it because it has a fullname information.

It probably makes sense to give a name to anonymous records with a hash based on field names and types. But please note it won't be exactly the same as .NET (same as type FullNames don't include assembly info in Fable) because .NET F# creates an actual class for anonymous records while Fable just use Pojos.

Not having the same name is totally fine to me :) :+1:

alfonsogarciacaro commented 2 years ago

I think we can fix this in Fable 4. This is because I want to use the hash function we already have for overloads, and in Fable 4 we made a change so you can call it with Fable types. We want to release Fable 4 for JS next month so it's better to wait a bit instead of maintaining a different solution for Fable 3.