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

Erased types cannot implement abstract members #2295

Closed
Titaye opened this issue Nov 25, 2020 · 6 comments
Closed

Erased types cannot implement abstract members #2295

Titaye opened this issue Nov 25, 2020 · 6 comments

Comments

@Titaye
Copy link
Contributor

Titaye commented Nov 25, 2020

Description

A project referencing Fable.SignalR is not compiling with fable 3 with the error "Erased types cannot implement abstract members"

Repro code

type LogLevel = string

[<Erase>]
type NullLogger =
    interface ILogger with
        member this.log logLevel message = this.log logLevel message

    [<Emit("$0.log($1, $2)")>]
    member _.log (logLevel: LogLevel) (message: string) : unit = jsNative

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.

The code could be rewritten has this in the library with Fable 3 but is it a regression or an expected behavior ?

[<Erase>]
type NullLogger =
    interface ILogger with
        [<Emit("$0.log($1, $2)")>]
        member _.log (logLevel: LogLevel) (message: string) : unit = jsNative

Expected and actual results

Code should compile

Related information

  • Fable version: Fable 3
  • Operating system : Windows 10
@alfonsogarciacaro
Copy link
Member

Interesting, I didn't know you could declare a non-static type without a constructor 👀

Not sure if it's a regression. The error message is more or less correct, but it may happen it was not displayed in Fable 2. However, I think Erase only has an effect on union types, but here you're using a class that cannot be instantiated, so I'm not sure. What's the purpose of this class and why do you need the Erase attribute?

@Titaye
Copy link
Contributor Author

Titaye commented Nov 25, 2020

I'm just using the Fable.SignalR library, I did not write it. I raised the issue here as it was compiling with Fable 2. Maybe I should raise the issue to the owner repository.

@alfonsogarciacaro
Copy link
Member

Ah, ok! Maybe @Shmew can help here? If it's not possible to update the library, we can try to remove the error message in this case.

@Shmew
Copy link
Contributor

Shmew commented Nov 26, 2020

Interesting, I didn't know you could declare a non-static type without a constructor 👀

Yeah I like this a lot when I import them from JS over interfaces as I can directly implement my own overloads on that type via inlining.

[<Erase>] is used because I either emit or inline every method so there's no bundle size from the type itself. I use this pattern frequently in my libraries, and can confirm it would be a regression if this doesn't work in Fable 3. You can see that pattern everywhere in Fable.Extras. Fable won't actually respect the code in your interface, so it's kind of hacky.

I suspect this would regress because interfaces mangle by default now? Or was it decided they need a mangle attribute, don't remember.

@alfonsogarciacaro
Copy link
Member

I see, interesting. Well, it's kind of a regression because doesn't allow this "hack" although the error message is correct. For example this code doesn't fail in Fable 2 but produces "undefined" in runtime unless you remove the Erase attribute:

open Fable.Core

type IFoo =
    abstract Foo: string

[<Erase>]
type Foo =
    | Foo of string
    interface IFoo with
        member this.Foo =
            match this with Foo s -> "foo:" + s

let test (x: IFoo) =
    printfn "%s" x.Foo

Foo "bar" |> test

I think you shouldn't need to use Erase because tree shaking should remove the type declaration if not used. But I understand if you already have it all over place in published libraries it can be difficult to remove it. We could "fix" it for now by only making the check for unions and records and ignore classes. In the case of erased classes, it will fail at runtime anyways if you try to instantiate them (without a workaround like Emit in the constructor). Erased records would also fail right now, but we're considering turning them into JS arrays, see #2279.

@Shmew
Copy link
Contributor

Shmew commented Nov 26, 2020

I think you shouldn't need to use Erase because tree shaking should remove the type declaration if not used.

That makes sense, this was mostly because if I can not rely on outside tooling to tree-shake to reduce bundle size I'd rather do that.

Yeah, unions won't work because there's no way to "replace" all the functionality of the type via emit/inlining, so the error makes sense in that circumstance.

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

No branches or pull requests

3 participants