fable-compiler / Fable

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

Invalid TypeScript generics when implementing interfaces #3586

Open leononame opened 1 year ago

leononame commented 1 year ago

Description

I'm evaluating Fable for a toy project and was trying out TypeScript compilation. However, using Thoth.Json results in invalid code (with fable 4.5.0) when compiling Thoth.Json (the JS code is perfectly fine).

Invalid generics when implementing interfaces

The first error I encounter is when implementing an interface:

Repro code

namespace Thoth.MyJson

type Decoder<'T> = obj -> 'T

type IDecoder =
    abstract member Decode: Decoder<'b> -> 'b

module Decode =
    let ForValue<'T> (v: obj) =
        { new IDecoder with
            member _.Decode(decoder) = decoder v }

In the REPL, it returns invalid TypeScript code (also reproducible in latest version 4.5.0), because the generic parameter <b> in TypeScript should probably be names <$a> (line 9).

TypeScript result:

export interface IDecoder {
    Decode<b>(arg0: ((arg0: any) => b)): b
}

export function Decode_ForValue<T>(v: any): IDecoder {
    return {
        Decode<b>(decoder: ((arg0: any) => $a)): $a {
            return decoder(v);
        },
    };
}

Please provide the F# code to reproduce the problem or a link to the REPL. Ideally, it should be possible to easily turn this code into a unit test.

Reflection in fable-library

Repro code

There is another error when calling Reflection isUnion:

module Tour.Functions
open FSharp.Reflection
module BasicFunctions =
    let myIsUnion t = FSharpType.IsUnion(t, allowAccessToPrivateRepresentation = true)

REPL

this compiles to

import { isUnion } from "fable-library/Reflection.js";

export function myIsUnion(t: any): boolean {
    return isUnion(t, true);
}

However, the fable-library's version of isUnion does not take any additional parameter apart from t (https://github.com/fable-compiler/Fable/blob/585a73d1796294496afc1918aeb8f757f975a851/src/fable-library/Reflection.ts#L392)

MangelMaxime commented 1 year ago

For the FSharpType.IsUnion(t, allowAccessToPrivateRepresentation = true) I believe the fix is for Thoth.Json to not provide the allowAccessToPrivateRepresentation because the second argument is not supported. And perhaps, also make Fable report an error if user try to use a version of the method which is not supported.

https://github.com/fable-compiler/Fable/blob/585a73d1796294496afc1918aeb8f757f975a851/src/Fable.Transforms/Replacements.fs#L2839-L2840

Right now Fable, seems to only check for the method names and forward the arguments as is.

@ncave @dbrattli Do you think this is the correct path to take here? I can work on the fix if that's the case.

ncave commented 1 year ago

@leononame Perhaps being a bit more explicit with the generic types can help, something like this:

type Decoder<'T> = obj -> 'T

type IDecoder<'T> =
    abstract member Decode: Decoder<'T> -> 'T

module Decode =
    let ForValue (v: obj) =
        { new IDecoder<'T> with
            member _.Decode(decoder) = decoder v }
leononame commented 1 year ago

@ncave I could try to make a PR to Thoth.Json if I'll manage to find all the spots. However, I still kind of believe this seems to be an issue with Fable, in .NET F# this doesn't seem to cause any issues

ncave commented 1 year ago

@leononame Yes, it works for .NET, but perhaps it's a bit different there, as .NET is specializing generic types from usage, not code-generating generic types. I'm not sure how easy it would be to fix that in Fable.

Anyway, IMO having the generics as part of the generic interface type is much cleaner, but if we want to have each interface method have its own different generics, then we can try matching the generic interface method declaration verbatim in the implementation:

type Decoder<'T> = obj -> 'T

type IDecoder =
    abstract member Decode: Decoder<'b> -> 'b

module Decode =
    let ForValue (v: obj) =
        { new IDecoder with
            member _.Decode<'b>(decoder: Decoder<'b>) = decoder v }

Admittedly it's somewhat brittle, as you need to match the implementation generic types exactly to the interface declaration.

If possible, I would still have the generics in the interface type (e.g. IDecoder<'T>, see previous comment), as it's less brittle and much cleaner.

I understand both of these are just work-arounds, so we can keep this issue open until fixed.

leononame commented 1 year ago

@ncave I've had a look at how thoth.Json handles this and I don't think making IDecoder generic is very doable here because my example was very dumbed down. e.g., looking at IRequiredGetter:

type IRequiredGetter =
        abstract Field: string -> Decoder<'a> -> 'a
        abstract At: List<string> -> Decoder<'a> -> 'a
        abstract Raw: Decoder<'a> -> 'a

You can see that Field returns a method where a decoder is applied, and the generics get applied afterwards? I'm not sure if I'm correct, I'm not really familiar with F# at all.

However, I've found several other things while trying to clean up, e.g.:

MangelMaxime commented 1 year ago

@leononame

  • I can help pinpoint it down if you guys are interested, but I don't know how to reduce the sample to something palatable because I don't fully understand Thoth.Json's code

If you can point any invalid code generation or things that don't work we are always interested in it. This will allows us to fix them and also add tests for them when possible to avoid regression in the future.

If you do so, I think it is best to open an issue per problem like that they can be easily tracked and the discussion / notes will be more focused.

Regarding Thoth.Json, I am currently working on a major rewrite of it and will make sure it works against the different targets supported by Fable. So if I encounter any issues, I will make sure to list them to on my side.

If you need any explanations regarding Thoth.Json or the code from it, please open an issue on Thoth.Json repo so I can help you.