Allow methods with name New* on DUs as long as they have different parameters

xperiandri commented 7 years ago

Allow to define a static method on DU which name is New<DU case name> and a set of parameters is different from the set of values of the DU case with name <DU case name>.

Sometimes DU case has composite values which can be set one by one or separately. Imagine a ventilation channel which part has length and profile where profile has width and height.

type Dimension = int
type TypeSize = int
type Profile = Profile of int * int

type ElementDimensions =
    | Inherited of Length : Dimension
    | TypeSizeDimensions of TypeSize : TypeSize * Length : Dimension
    | CustomDimensions of Profile : Profile * Lenght : Dimension

So the last case can be constructed from profile and length or form width, height and length. But neither this

type ElementDimensions =
    | Inherited of Length : Dimension
    | TypeSizeDimensions of TypeSize : TypeSize * Length : Dimension
    | CustomDimensions of Profile : Profile * Lenght : Dimension
    static member NewCustomDimensions (width, height, length) =
        CustomDimensions(Profile(width, height), length)

Nor this does work

    static member CustomDimensions (width, height, length) =
        CustomDimensions(Profile(width, height), length)

Pros and Cons

As this is a construction of the same DU case it would be natural to overload this method especially for consumer from C#. However be able to use NewCustomDimensions (width, height, length) the same way as CustomDimensions (Profile, Length) would be nice in F#. Anyway DU case constructor is tupled.

Workaround

Use a different case name, e.g.

type ElementDimensions =
    | Inherited of Length : Dimension
    | TypeSizeDimensions of TypeSize : TypeSize * Length : Dimension
    | CustomDimensionsAux of Profile : Profile * Lenght : Dimension
    static member CustomDimensions (width, height, length) =  CustomDimensionsAux(Profile(width, height), length)
    static member CustomDimensions (profile, length) =  CustomDimensionsAux(profile, length)

Use a different method name not prefixing New, e.g.

type ElementDimensions =
    | Inherited of Length : Dimension
    | TypeSizeDimensions of TypeSize : TypeSize * Length : Dimension
    | CustomDimensions of Profile : Profile * Lenght : Dimension
    static member MakeCustomDimensions (width, height, length) = CustomDimensions(Profile(width, height), length)
    static member MakeCustomDimensions (profile, length) = CustomDimensions(profile, length)

Extra informtion

Estimated cost (XS, S, M, L, XL, XXL): I guess it is S, I don't have enough knowledge to estimate.

Original issue

Affadavit (must be submitted)

Please tick this by placing a cross in the box:

[X] This is not a question (e.g. like one you might ask on stackoverflow) and I have searched stackoverflow for discussions of this issue
[X] I have searched both open and closed suggestions on this site and believe this is not a duplicate
[X] 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:

[X] This is not a breaking change to the F# language design
[X] I would be willing to help implement and/or test this
[ ] I or my company would be willing to help crowdfund F# Software Foundation members to work on this

theprash commented 7 years ago

It seems common - in the F# core library at least - to have a module and a type with the same name, where the module contains functions for transforming or creating the type. Although you do need to know the magic module attribute that make this possible without causing a duplicate definition error: [<CompilationRepresentation(CompilationRepresentationFlags.ModuleSuffix)>]

This way I believe you can achieve the API that you're looking for:

type ElementDimensions =
    | Inherited of Length : Dimension
    | TypeSizeDimensions of TypeSize : TypeSize * Length : Dimension
    | CustomDimensions of Profile : Profile * Length : Dimension

[<CompilationRepresentation(CompilationRepresentationFlags.ModuleSuffix)>]
[<RequireQualifiedAccess>]
module ElementDimensions =
    let newCustomDimensions width height length = CustomDimensions(Profile(width, height), length)

ElementDimensions.newCustomDimensions 1 2 3

Note the RequireQualifiedAccess attribute, which just means the module cannot be opened and you must always prefix with the module name, as if it were a class.

An existing example of this pattern is the List type, which has a corresponding List module with constructor functions like List.empty and List.singleton.

So I believe this pattern already provides what you ask for but it has some downsides:

The extra module definition is quite verbose and unpleasant to look at with those attributes.
Module functions cannot have overloads like methods can. Of course, this also has the benefit of enforcing that they always work will with type inference when passed around as values.

An alternative language feature request might be to have a keyword to put after module that just adds those two attributes. I think it would make this pattern more discoverable and legitimise it as idiomatic F#, because right now I think it looks and feels a bit like a hack.

cartermp commented 7 years ago

@theprash Note that in F# 4.1, [<CompilationRepresentation(CompilationRepresentationFlags.ModuleSuffix)>] is now implicit.

xperiandri commented 7 years ago

It is an option for F# consumers, but for C# there are no options now.

dsyme commented 7 years ago

@xperiandri I'm torn about this suggestion. In some ways I like it. But what about symmetry with decomposition in pattern matching - would we allow active patterns and union cases to share overloaded names? That would be a considerable change, and in general F# eschews this kind of thing,

I've added a workaround in the description

xperiandri commented 7 years ago

@dsyme, if at least static methods like New<DU case> will be allowed it will not involve any other consequences that you described. And it will look consistent to C# consumer. And it looks to be quite simple.

Allowing case overload only for construction also will be nice but not as critical as New<DU case> static methods in my opinion.

dsyme commented 7 years ago

@dsyme, if at least static methods like New will be allowed it will not involve any other consequences that you described. And it will look consistent to C# consumer.

Yes I'm OK with allowing these. I'll change the title and mark that as approved

dsyme commented 7 years ago

@xperiandri If you would like to go ahead and make an RFC and/or an impementation that would be awesome. I don't think it's too hard. However we may also want to make the existing NewXYZ method available.

xperiandri commented 7 years ago

Could you confirm if I understand you right?

I have to fork /fsharp/fslang-design/
I have to create an RFC issue in /fsharp/fslang-design/issues
I need to add RFC.md from template referencing issue from 2

Then if I will be managed to build F# compiler solution I should try to make a draft implementation of the created RFC.

Right?

dsyme commented 7 years ago

@xperiandri That's right. (if you prefer you can just do a draft implementation - others can help with the RFC too)

7sharp9 commented 6 years ago

Personally for the effort involved I would just use an alternative verb:

    static member CreateCustomDimensions (width, height, length) =
            CustomDimensions(Profile(width, height), length)

    static member MakeCustomDimensions (width, height, length) =
            CustomDimensions(Profile(width, height), length)

Ive never encountered an issue with union creations, perhaps because I would always use Make or Create as my creational verb.

xperiandri commented 6 years ago

@7sharp9 your solution adds inconsistency for consumer from C#

7sharp9 commented 6 years ago

@xperiandri Im was just being pragmatic there are workarounds, unless the RFC is written, then likely this will just lie here forever.

It also feels like NewCaseName is an implementation detail thats leaked out rather than being an aspect of the defined type.

cartermp commented 6 years ago

@xperiandri I'm not sure what you mean by this:

your solution adds inconsistency for consumer from C#

How would this be inconsistent for a consumer in C#? C# has no notion of discriminated unions, so dealing with a DU is already a specialized concern.

It's also fairly common in C# and Java to have a static method called Create or CreateFoo when you want to instantiate that way instead of with a constructor.

I'm perfectly fine with this suggestion, but when writing the RFC and considering alternatives, I don't think I'd agree that this fixes an inconsistency for C# consumers.

0x53A commented 6 years ago

@cartermp Currently F# unions compile down to NewX static methods in IL.

Example:

type T = A | B

will compile down to (simplified):

public class T
{
   public static T NewA();
   public static T NewB();
}

If you were able to define other NewX overloads yourself, then from a C# perspective, you would have multiple overloads of the NewX method, one from the ctor, one from yourself.

Otherwise a C# consumer would see one NewX method and one CreateX method.

7sharp9 commented 6 years ago

This is more of a leaking implementation detain though.

On Thu, 19 Jul 2018 16:55 Lukas Rieger, notifications@github.com wrote:

@cartermp https://github.com/cartermp Currently F# unions compile down to NewX static methods in IL.

Example:

type T = A | B

will compile down to (simplified):

public class T { public static T NewA(); public static T NewB(); }

If you were able to define other NewX overloads yourself, then from a C# perspective, you would have multiple overloads of the NewX method, one from the ctor, one from yourself.

Otherwise a C# consumer would see one NewX method and one CreateX method.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/fsharp/fslang-suggestions/issues/532#issuecomment-406326510, or mute the thread https://github.com/notifications/unsubscribe-auth/AAj7yvXcl-l9IP9bm2v5zvNH70w9yLLOks5uIKvYgaJpZM4L3jjD .

0x53A commented 6 years ago

This is more of a leaking implementation detain though.

Well yes, but at this point it is less implementation detail and more implementation spec.

fsharp / fslang-suggestions