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

Make "ModuleSuffix" the default if a type has the same name even if it's generic #627

Open
alfonsogarciacaro opened this Issue Nov 26, 2017 · 6 comments

Comments

Projects
None yet
4 participants
@alfonsogarciacaro

alfonsogarciacaro commented Nov 26, 2017

This suggestion has already been implemented in F# 4.1 (see #113 and RFC https://github.com/fsharp/fslang-design/blob/master/FSharp-4.1/FS-1019-implicitly-add-the-module-suffix.md). However in the cases where the type is generic this doesn't happen. For example:

type Foo<'a> =
   { value: 'a }

module Foo =
   let add (x: Foo<'a>) (y: Foo<'a>) = { value = x.value + y.value }

This makes sense because the compiled name of the type is "Foo`1" so it doesn't conflict with the module, however it doesn't look consistent. My main problem is in Fable we don't add the number of generic parameters to the type name (this is to improve readability of the generated JS code) and because of this the sample above will do conflict. Before F# 4.1 devs were just used to add the "ModuleSuffix" attribute, but after that they are not using it anymore and when Fable complains the type and the module have the same public name they get surprised. See for example fable-compiler/Fable#1231

This problem can be solved by adding the attribute but the user experience for Fable would be much better as it wouldn't differ from standard F#.

Pros

  • Consistent behavior of adding Module suffix when there's a type and a module with the same name, no matter if the type is generic.
  • Removing the need of adding the attribute only when compiling with Fable

Cons

  • For more savvy users, adding the Module suffix in this case could actually be unexpected (as they know names don't conflict in compiled .NET code)

Extra information

Estimated cost (XS, S, M, L, XL, XXL): S, probably just removing the generic param number suffix to the current check.

Affidavit (please submit!)

Please tick this by placing a cross in the box:

  • This is not a question (e.g. like one you might ask on stackoverflow) and I have searched stackoverflow for discussions of this issue
  • I have searched both open and closed suggestions on this site and believe this is not a duplicate
  • This is not something which has obviously "already been decided" in previous versions of F#. If you're questioning a fundamental design decision that has obviously already been taken (e.g. "Make F# untyped") then please don't submit it.

Please tick all that apply:

  • This is not a breaking change to the F# language design
  • I or my company would be willing to help implement and/or test this
@ijsgaus

This comment has been minimized.

Show comment
Hide comment
@ijsgaus

ijsgaus Nov 28, 2017

Then we need alternative mechanism for don't use suffix in generic case

ijsgaus commented Nov 28, 2017

Then we need alternative mechanism for don't use suffix in generic case

@cartermp

This comment has been minimized.

Show comment
Hide comment
@cartermp

cartermp Nov 28, 2017

Member

@ijsgaus What scenario are you concerned about? The module suffix isn't necessary in F# 4.1 in cases aside from what @alfonsogarciacaro mentions above, and with his suggestion the picture is completed.

Member

cartermp commented Nov 28, 2017

@ijsgaus What scenario are you concerned about? The module suffix isn't necessary in F# 4.1 in cases aside from what @alfonsogarciacaro mentions above, and with his suggestion the picture is completed.

@0x53A

This comment has been minimized.

Show comment
Hide comment
@0x53A

0x53A Nov 28, 2017

Contributor

Doesn't this break binary compatibility?

If I compile the snippet in the op with fsc vCurrent, then it emits a type Foo. But if I then compile the exact same source with fsc vNext, it will emit a different type, and all my consumers break.

Therefore I have voted the issue down, even though I actually like the suggestion.

Contributor

0x53A commented Nov 28, 2017

Doesn't this break binary compatibility?

If I compile the snippet in the op with fsc vCurrent, then it emits a type Foo. But if I then compile the exact same source with fsc vNext, it will emit a different type, and all my consumers break.

Therefore I have voted the issue down, even though I actually like the suggestion.

@alfonsogarciacaro

This comment has been minimized.

Show comment
Hide comment
@alfonsogarciacaro

alfonsogarciacaro Nov 28, 2017

@0x53A This wouldn't change the name of the type Foo, it would just add the -Module suffix to the module Foo. Exactly the same thing as happens now if type Foo wasn't generic.

@ijsgaus Please note the -Module suffix is transparent to the user, it only affects compiled code.

alfonsogarciacaro commented Nov 28, 2017

@0x53A This wouldn't change the name of the type Foo, it would just add the -Module suffix to the module Foo. Exactly the same thing as happens now if type Foo wasn't generic.

@ijsgaus Please note the -Module suffix is transparent to the user, it only affects compiled code.

@0x53A

This comment has been minimized.

Show comment
Hide comment
@0x53A

0x53A Nov 28, 2017

Contributor

@alfonsogarciacaro Yes, I was talking about the static type that represents the module.

If the name of that type changes from Foo to FooModule, I would expect a TypeLoadException at runtime.

Contributor

0x53A commented Nov 28, 2017

@alfonsogarciacaro Yes, I was talking about the static type that represents the module.

If the name of that type changes from Foo to FooModule, I would expect a TypeLoadException at runtime.

@0x53A

This comment has been minimized.

Show comment
Hide comment
@0x53A

0x53A Nov 28, 2017

Contributor

I already really don't like the automatic feature that is already implemented for the same reason. As I understand it adding a type changes the compiled name of the module.

Contributor

0x53A commented Nov 28, 2017

I already really don't like the automatic feature that is already implemented for the same reason. As I understand it adding a type changes the compiled name of the module.

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