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

Support optional arguments for optional parameters #1167

Open
5 tasks done
JordanMarr opened this issue Aug 9, 2022 · 10 comments
Open
5 tasks done

Support optional arguments for optional parameters #1167

JordanMarr opened this issue Aug 9, 2022 · 10 comments

Comments

@JordanMarr
Copy link

JordanMarr commented Aug 9, 2022

I propose we simplify setting optional arguments for optional method parameters by allowing them to be set when the parameter is defined (like C# Optional Arguments).

Existing approaches:

Existing Approach 1

type Checkout =         
    static member GetTotal(basePrice: decimal, ?discount: decimal) =
    basePrice - defaultArg discount 0M

Existing Approach 2 (for C# interop)

type Checkout =         
    static member GetTotal(basePrice: decimal, [<Optional; DefaultParameterValue(0.0)>] discount: double) =
        let discount = System.Convert.ToDecimal(discount)
        basePrice - discount

Desired Approach

type Checkout =         
    static member GetTotal(basePrice: decimal, discount: double = 0.0) =
        let discount = System.Convert.ToDecimal(discount)
        basePrice - discount

I would also expect (if possible) to be able to leave out the optional parameter's data type since it can be inferred from the default argument:

type Checkout =         
    static member GetTotal(basePrice, discount = 0.0) =
        let discount = System.Convert.ToDecimal(discount)
        basePrice - discount

Pros and Cons

The advantages of making this adjustment to F# are:

  • It would simplify usage of optional parameters
  • It would be more intuitive (especially for C# developers who will likely expect this to work)

The disadvantages of making this adjustment to F# are:

  • There are already two ways of doing this, so adding a third would be undesirable. Ideally, this would replace the C# interop version by deprecating it.

Extra information

Estimated cost (XS, S, M, L, XL, XXL):
I would guess this to be a S to M cost.

NOTE 1

In the Desired Approaches above, I did not include a ? before the discount parameter; however, require it may ease implementation. Another benefit in requiring ? would be to maintain consistency with the established syntax for declaring an optional parameter.

NOTE 2

I learned while making this proposal that decimal values cannot be used as literals.

In C#, trying to set a decimal default argument will result in the following error:
"CS1750: A value of type 'double' cannot be used as a default parameter because there are no standard conversions to type 'decimal'."

In the F# version above ("Existing Approach 2 (for C# interop)"), this will also fail with the following error:
"This is not a valid constant expression or custom attribute value."

I had to change the example in my proposal to use a double as the default value.

This does demonstrate a certain elegance to the native F# approach of using defaultArg which allows you to use an optional decimal and sidesteps this issue entirely.

Related suggestions: (put links to related suggestions here)

The closest I could find to this issue posted here is this one by @baronfel:
Emit Optional;DefaultParameterValue by default for interop between F# and C#

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

For Readers

If you would like to see this issue implemented, please click the 👍 emoji on this issue. These counts are used to generally order the suggestions by engagement.

@vzarytovskii
Copy link

Regarding the

NOTE: In the Desired Approach, I did not include a ? before the discount parameter; however, it may make implementation easier to include it.

I think it makes sense to have it there since it's a default representation for the optional parameters in F#, and it should already be familiar for anyone who knows F#.

@jasiozet
Copy link

jasiozet commented Aug 9, 2022

Would it be possible to change Approach 2 into the proposed approach?
Or change approach 1 into proposed approach?
Or even make it the only approach?

I find the proposed approach the most elegant of the 3 and the most C# familiar (but maybe it's just my bias).

@edgarfgp
Copy link
Member

edgarfgp commented Aug 10, 2022

Would be good to align this suggestion with #1136 ? . I like the suggested approach in your suggestion :)

@Tarmil
Copy link

Tarmil commented Aug 12, 2022

@jasiozet We definitely can't make this the only approach, because this and C#-style are less powerful than F#-style. Here, you can't know whether the caller passed the default value or didn't pass any value. In F#-style, the case where the caller didn't pass any value uses None, so it's always distinct from any explicitly passed value.

I would be fine with making this a syntax sugar for C#-style though.

@jasiozet
Copy link

@jasiozet We definitely can't make this the only approach, because this and C#-style are less powerful than F#-style. Here, you can't know whether the caller passed the default value or didn't pass any value. In F#-style, the case where the caller didn't pass any value uses None, so it's always distinct from any explicitly passed value.

I would be fine with making this a syntax sugar for C#-style though.

From my perspective this would still be an improvement. Thank you for providing the context #:+1:

@xperiandri
Copy link

xperiandri commented Aug 13, 2022

Good point.

In F#-style, the case where the caller didn't pass any value uses None, so it's always distinct from any explicitly passed value.

Good point. F# compiler can do that for us behind the scene and make it compatible with the general .NET approach too.

@cartermp
Copy link
Member

cartermp commented Oct 3, 2022

Regarding this point:

There are already two ways of doing this, so adding a third would be undesirable. Ideally, this would replace the C# interop version by deprecating it.

We would not want to deprecate the existing use of [<Optional; DefaultParameterValue(...)>]. It is still perfect valid (if suboptimal) to use these in C#, and it compiles down to the same IL as the syntax, but is also different.

The param = value syntax in C# is more strict:

public class C {
  // Compiles
  public void MAtttributes([Optional, DefaultParameterValue(1)] object i) { }

  // error CS1763: 'i' is of type 'object'. A default parameter value of a reference type other than string can only be initialized with null
  public void MSyntax(object i = 1) { }
}

And so if you need the ability to declare that i is an object with a default value of 1, you use the attributes. Niche, but there's certainly someone out there who relies on this.

I also think that should this be pursued, then the following issues should also be considered in scope: https://github.com/dotnet/fsharp/issues?q=is%3Aissue+is%3Aopen+sort%3Aupdated-desc+DefaultParameterValue

@dsyme
Copy link
Collaborator

dsyme commented Oct 28, 2022

I'm labelling this as approved - I don't see any reason not to also support these given we interop with them anyway, and there are legitimate reasons to need them in interop

@BoundedChenn31
Copy link

BoundedChenn31 commented Sep 29, 2023

Perhaps, it would be nice to have described approach with discount: double = 0.0 and also ?discount: double = 0.0 to unify syntax a little bit? The latter will be just a sugar for existing approach with option so you don't have to call defaultArg explicitly.

@Lanayx
Copy link

Lanayx commented Sep 29, 2023

I'm also for two improvements in one. Compile time optional argument like in C# via discount: double = 0.0 syntax and runtime optional through Option via ?discount: double = 0.0 syntax

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

No branches or pull requests

10 participants