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

Investigate [<Optional>] attribute #20

Open
Freymaurer opened this issue Jun 5, 2024 · 6 comments
Open

Investigate [<Optional>] attribute #20

Freymaurer opened this issue Jun 5, 2024 · 6 comments
Assignees
Labels
c# f# help wanted Extra attention is needed

Comments

@Freymaurer
Copy link
Owner

Freymaurer commented Jun 5, 2024

Should improve C# accessibility, this would solve one of the two issues why a c# access layer is required!

A suggestion by Vladimir Shchur.

For the API compatibility issues, two points
1) I'd avoid exposing tuples in API at all, they are less self-descriptive than records
2) For optional parameters I'd rather used conditional compilation with [<Optional>] for C#, so C# users will never see FsharpOption<T> type

— Vladimir Shchur (@Lanayx) June 5, 2024

Seems to run for Fable.

REPL

@Freymaurer Freymaurer self-assigned this Jun 5, 2024
@Freymaurer
Copy link
Owner Author

Freymaurer commented Jun 6, 2024

I tried it out and it works fine for c# with simple types such as integer and an existing default value.

Next i tried using Discrimate Union as parameter as replacement for f# option:

open Fable.Core

open System.Runtime.InteropServices

type ThatDiscriminatedUnion =
    | A of int
    | B of string

[<AttachMembersAttribute>]
type MyClass =
    static member Foo(req: int, [<Optional; DefaultParameterValue(null)>] opt1: ThatDiscriminatedUnion, [<Optional; DefaultParameterValue(null)>] opt2: ThatDiscriminatedUnion) =
        printfn $"{req} {opt1} {opt2}"
    static member Bar(args: struct(bool*string)[]) =
        for i in args do
            let struct (i1,i2) = i
            printfn $"{i1} {i2}"

module Test =

    MyClass.Foo(16, opt2=ThatDiscriminatedUnion.A) // does not compile
    MyClass.Foo(20)  // does not compile

    MyClass.Bar([| (true,"a"); (false,"b") |])

Plus fsharp intellisense looks anything but native:

image

@Freymaurer Freymaurer added help wanted Extra attention is needed c# f# labels Jun 6, 2024
@Lanayx
Copy link

Lanayx commented Jun 7, 2024

@Freymaurer as for second issue, I recommended to get rid of tuples in API like here. If you use records, there (like { label: string; value: float }, this can both eliminate compatibility issue and make API more clear.

@kMutagene
Copy link
Collaborator

kMutagene commented Jun 10, 2024

For reference, this is why we introduced Optional<T> for the native C# layer in Plotly.NET.CSharp

Using that type for optional values in the C# API makes C# API only slightly worse in intellisense (e.g. Optional<string> instead of string):

image

But you can remove all attributes from the F# function parameters, making them nice and legible again. This also supports both reference and value types therefore fixing both of your issues i think.

Note that you can just put the normal values in the C# call, no need to explicitly create Optional<T>:

image

@Freymaurer
Copy link
Owner Author

Ok, i did more investigation and i must say, optional types can most likely be replaced by using only [<Optional>], without DefaultParameter.

image

This is the base setup for Foo. Intellisense for using looks fine, just hovering over the function is not perfectly native for f#, but okay:

image

C# on the other hand looks really good, as it does not require any more setup and works beautifully (as far as i tested it):

image

What do you think @kMutagene ?

@kMutagene
Copy link
Collaborator

kMutagene commented Jun 10, 2024

IIRC the problems start with generics and type constraints. E.g. If you try an arg with seq<#IConvertible> on the F# side and an equal type constraint in the C# wrapper method.

See the comments in the code i linked https://github.com/plotly/Plotly.NET/blob/dev/src%2FPlotly.NET.CSharp%2FOptional.cs#L10-L18

If you do not have these cases you should be fine with the standard optional param interop

@Freymaurer
Copy link
Owner Author

I tried again using [<Optional>] #seq<int> and here C# shows incompatibility, which makes using the api annoying:

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c# f# help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants