Omitting Optional; DefaultParameterValue arguments passes the default value and works for F# to F# calls #1812

kurtschelfthout · 2016-11-22T22:14:23Z

See #96.

This code seems to be going in the right direction in that as far as I can tell it now results in the same IL as the C# compiler generates for DefaultParameterValue (and for optional arguments, which in C# is just syntactic sugar for both Optional and DefaultParameterValue).

However we are not out of the woods.

F# still does not recognise a parameter with both [<Optional; DefaultParameterValue>] as optional only if the method definition is compiled by fsc. In other words:
Optional value defined in C# can be omitted at F# call site.
Optional value defined in F# can be omitted at C# call site, and now picks up the default value defined on the F# side.
Optional value defined in F# can not be omitted at F# call site (both same assembly or two different assemblies).
So @dsyme to be pedantic, it seem like C#-style optional arguments defined in F# cannot be consumed by F# as optional #1783 is not really a duplicate of [<DefaultParameterValue>] is ignored in F#-authored code #96 in that I think this PR as is pretty much solves [<DefaultParameterValue>] is ignored in F#-authored code #96 but not C#-style optional arguments defined in F# cannot be consumed by F# as optional #1783.
Note that I filtered out DefaultParameterValue from the attributes that are actually written in the IL, as, like Optional it is supposed to be written in the IL attributes directly. As far as I can tell, nothing should have problems with this, but I thought I'd mention it.
I have no idea what GenSlotParam is for, just modified it blindly like GenParamAttribs.
I have little doubt there will be test failures, as well as new tests to add.

Meanwhile any thoughts are appreciated.

…t-param-value # Conflicts: # src/fsharp/IlxGen.fs # src/fsharp/TcGlobals.fs

dsyme · 2016-11-22T22:48:31Z

src/fsharp/IlxGen.fs

+and GenDefaultParameterValue (Attrib (_,_,exprs,_,_,_,_)) =
+    let (AttribExpr (_,defaultValueExpr)) = List.head exprs
+    match defaultValueExpr with
+    | Expr.Const ((Const.String s),_,_) -> Some (ILFieldInit.String s)


GenFieldInit?

dsyme · 2016-11-22T22:51:20Z

Re GenSlotParam: GenSlotParam generates the data used for parameters at definitions of abstract method slots such as interface methods or override methods

dsyme · 2016-11-22T22:58:36Z

This looks very good. I suppose the F#-to-F# case is covered by this todo-like comment in infos.fs

                let isOutArg = HasFSharpAttribute g g.attrib_OutAttribute argInfo.Attribs && isByrefTy g ty
                let isOptArg = HasFSharpAttribute g g.attrib_OptionalArgumentAttribute argInfo.Attribs
                // Note: can't specify caller-side default arguments in F#, by design (default is specified on the callee-side) 
                let optArgInfo = if isOptArg then CalleeSide else NotOptional

I think you want to crack the attributes at that point to produce one of these (as we do for .NET methods)

                CallerSide (Constant v)

It looks like you also have to consider null" default values along the lines of this for C# code

                CallerSide (analyze (ImportILTypeFromMetadata amap m ilScope ilTypeInst [] ilParam.Type))

You can probably share some of the code each way

kurtschelfthout · 2016-11-23T09:29:46Z

Thanks much for the pointers, I'll take a look at that.

Start digression
I was looking at that first snippet before actually, but was confused by the "caller-side" vs "callee-side" terminology. But I now understand what happens: when at a call site the parameter is omitted, the compiler reads the default value from the definition of the method (i.e. the value defined as argument to DefaultParameterValue), and then passes that value explicitly!

In other words these default values are caller side, even though they are optically defined at the callee side. Interestingly, this means that if I change the default value in an assembly, this default value would only be picked up by clients after they recompile their assemblies. I would bet at least one person has lost several hours due to that subtlety...
End digression

kurtschelfthout · 2016-11-23T23:12:52Z

With latest commits it looks like F# to F# optional args work. I left some comments in the code where I did things that may be controversial.

Very simple example works; next step: adding tests!

dsyme · 2016-11-24T13:15:52Z

@kurtschelfthout This is looking good

dsyme · 2016-11-24T21:38:14Z

src/fsharp/infos.fs

+    static member FieldInitForDefaultParameterValueAttrib (Attrib (_,_,exprs,_,_,_,_)) =
+        let (AttribExpr (_,defaultValueExpr)) = List.head exprs
+        match defaultValueExpr with
+        | Expr.Const ((Const.String s),_,_) -> Some (ILFieldInit.String s)


This is closely related to GenFieldInit in IlxGen.fs. May make sense to share the common core of the code, putting it in TastOps.fs?

Yes, good pointer, will try to reuse...tomorrow evening, time for relaxing now. :)

dsyme · 2016-11-25T11:31:12Z

...clarationElements/MemberDefinitions/OptionalDefaultParameterValueArguments/FSharpToFSharp.fs

+open System
+
+type Class() =
+    static member Method1  ([<Optional;DefaultParameterValue(42y)>]i:sbyte) = i


Thanks for being systematic :)

dsyme · 2016-11-25T22:46:25Z

@kurtschelfthout Please also check cases where C# and F# arguments have Nullable<T> type, which I understand is allowed for C#-style optional arguments

kurtschelfthout · 2016-11-25T23:15:10Z

With the latest commits, I've taken care of all review comments so far, and all issues I raised in the initial PR description above. So almost there!

The tests revealed one issue though - we should add a check whether the argument of DefaultParameterValue matches the type of the argument. Otherwise invalid IL can be generated (as is currently exposed by one of the tests I added, so I'd expect the test run to fail).

For that, I need to figure out where to add the error, and how to add compile errors :)

dsyme · 2016-11-26T13:26:26Z

...eclarationElements/MemberDefinitions/OptionalDefaultParameterValueArguments/ConsumeFromCS.fs

+    static member Method15 ([<Optional;DefaultParameterValue(new DateTime())>]i:DateTime) = i
+
+    //Check nullables. 
+    static member MethodNullable1 ([<Optional;DefaultParameterValue(null)>]i:Nullable<int>) = i


In C#, can you just declare Optional with `DefaultParameterValue(null)`` implied?

Yes, or whatever the default value of the type is. To support that we could add some type-directed analysis (a simplified version of analyze which you mentioned earlier - wouldn't need to support marshal and ref parameters) to the added code in infos.fs.

Or maybe we do need to support byref optional arguments and COM marshalling. Perhaps I'm just assuming things to make my life easier.

kurtschelfthout · 2016-11-27T15:43:24Z

I added a compiler error to deal with the unverifiable code in infos.fs. A few potential issues with that.

I'm not sure which type the compiler thinks null is, but this error is triggered if I don't write DefaultParameterValue(null:obj) or whatever the type is. So it seems you need to duplicate the type annotation there.
I wanted to show the type of the default parameter and the argument in the error message, but in infos.fs NicePrint is not yet defined...

To mitigate the first issue I'll look into allowing to omit the DefautParameterValue if you just want to specify the default value for the type.

forki · 2016-11-27T15:46:45Z

src/fsharp/FSComp.txt

 3208,tcCouldNotFindOffsetToStringData,"Could not find method System.Runtime.CompilerServices.OffsetToStringData in references when building 'fixed' expression."
 3209,chkNoByrefReturnOfLocal,"The address of the variable '%s' cannot be used at this point. A method or function may not return the address of this local value."
 3210,tcNamedActivePattern,"%s is an active pattern and cannot be treated as a discriminated union case with named fields."
+3211,DefaultParameterValueNotAppropriateForArgument,"The default value does not have the same type as the argument."


Could you please also specify the names of the types.

forki · 2016-11-27T15:48:36Z

...clarationElements/MemberDefinitions/OptionalDefaultParameterValueArguments/FSharpToFSharp.fs

+open System
+
+type Class() =
+    //check that all known supported primitive types work.


Can you please add a space between // and the text. I think we do that everywhere in the code base

forki · 2016-11-27T15:57:42Z

src/fsharp/IlxGen.fs

 // Generate object expressions as ILX "closures"
 //-------------------------------------------------------------------------- 

+// generates the data used for parameters at definitions of abstract method slots such as interface methods or override methods.


Can you please make it a triple slash comment and uppercase it?

The behavior here matches that of C# as far as I can tell, but it seems unspecified anywhere. I could not find a reference to it in the C# 5.0 spec or C# 6.0 draft spec. In particular the System.Reflection.Missing for obj is interesting.

kurtschelfthout · 2016-11-27T20:34:20Z

This is ready for final review.

I do feel like it snowballed a bit on me, so feel free to take as much time as you need.

dsyme · 2016-11-27T21:55:53Z

@kurtschelfthout Dude there's a reason why it wasn't fixed earlier. You've done an important job, snowball and all :)

dsyme

Superb job. One small change request. Also update the RFC please

Could you take a look if there are optional-argument-functions-with-default-value-attributes in the .NET Framework which we could write some tests against? Also, have you tested it against the original scenarios mentioned in original reports?

dsyme · 2016-11-27T22:01:11Z

src/fsharp/infos.fs

+                            let defaultValue = OptionalArgInfo.ValueOfDefaultParameterValueAttrib attr
+                            match defaultValue with
+                            | Some (Expr.Const (_, m, typ)) when not (typeEquiv g typ ty) -> 
+                                error(Error(FSComp.SR.DefaultParameterValueNotAppropriateForArgument(), m)); NotOptional


I think a warning would be better here

The problem is that then the compiler can generate unverifiable IL at call sites where the argument is omitted - the default value is put on the stack and the method is called, but the value on the stack will have the type of the default value, which does not correspond to the method argument's type.

So our options are:

Keep it as an error

Make it a warning, but ignore the default value (i.e. infer the default value from the type)

Make it a warning, ignore the default value and the optional argument (i.e. won't be able to omit this argument) - because inferring the default value from the type can have some surprising consequences.

Just thought of a fourth option - generate a warning at the definition site, and an error at the call site when the parameter is omitted.

Also note C# generates an error in this case:

kurtschelfthout · 2016-11-28T08:25:36Z

@dsyme other review items:

With "update the RFC" did you mean write it :) I can't find an existing RFC.... In any case I'll be happy to do that.
I couldn't find any references to DefaultParameterValue or testable references to OptionalAttribute in the reference source.
I have confirmed the original scenarios in the reports are covered by the Conformance tests added in this PR, in particular [<DefaultParameterValue>] is ignored in F#-authored code #96 are covered by the ConsumeFromCS.fs tests and the cases in C#-style optional arguments defined in F# cannot be consumed by F# as optional #1783 are covered by the FSharpToFSharp.fs tests.

dsyme · 2016-11-28T11:42:04Z

I think we do this: "Make it a warning, ignore the default value and the optionality of the argument"

With "update the RFC" did you mean write it :) I can't find an existing RFC.... In any case I'll be happy to do that.

Yes :) thanks!

kurtschelfthout · 2016-11-28T22:22:10Z

So now the error is a warning as requested, but there is a problem I'm not sure how to address. Every time the method GetParamAttribs is called the same warning gets emitted again. By the way, this is also the case for the CallerInfo related warning a few lines down (CallerMemberNameIsOverriden). I've noticed this method gets called once per definition, and three times per call. So in other words define a method and a method call to it, you'll get 4x the same warning(s).

I have tried to move the checks to PostInferenceChecks but it's awkward, because the CallerSide union type carries the ILFieldInit directly, and then I'd have to back out the type again from that in PostInferenceChecks.

Perhaps the OptionalArgInfo should be changed to carry either TType * Expr or ILFieldInit depending on where the method is from (same assembly or different assembly). We don't need to do any checks on the latter.

dsyme · 2016-11-29T17:17:27Z

Every time the method GetParamAttribs is called the same warning gets emitted again

I don't think this is such a problem in practice.

I'm happy with things as they are, I think we can now accept this?

dsyme · 2016-11-29T17:44:04Z

@kurtschelfthout Thank you so much for addressing this, it's a major improvement

kurtschelfthout · 2016-11-29T20:53:31Z

@dsyme thanks for the many pointers and support. The only reason I could even get started on this is because you put such a nice analysis on the bug report.

haf-lynx · 2016-12-01T10:02:39Z

Well done =). Can I use this commit of the compiler with Visual Studio on Windows straight away somehow?

forki · 2016-12-01T10:05:36Z

@haf-lynx sorry no. I mean yes you can build your own compiler, but it's not exactly recommended to use that one in production.

goswinr · 2018-04-18T12:45:09Z

I am still confused. Now that this is merged, What is the correct way to declare optional parameters that can also be used via C# or Ironpython?
is it [<Optional; DefaultParameterValue>] ?
for example: [<Optional; DefaultParameterValue(true)>] myParameter:bool ?

cartermp · 2018-04-18T13:50:39Z

@goswinr The latter:

type Class1() =
    static member Right([<Optional;DefaultParameterValue(12)>] i: int) = i + 1

type Class2() =
    // Compiler warning
    static member Wrong([<Optional;DefaultParameterValue("I am a string")>] i: int) = i + 1

haf · 2018-05-14T21:11:49Z

What's the default of CancellationToken in this case?

cartermp · 2018-05-15T21:52:32Z

@dsyme Any thoughts here? It appears that any unchecked value is not accepted.

open System.Runtime.InteropServices

[<Struct>]
type Egads = { Yee: int; Doot: string }

type C() =
    member __.M([<Optional; DefaultParameterValue(Unchecked.defaultof<Egads>)>] ?e: Egads) = ()

It does not appear to be covered in the RFC either.

dsyme · 2018-05-16T17:17:29Z

Yes there is an incompleteness here.

The relevant C# equivalent is this:

using System.Runtime.InteropServices;

class C {
    static void M(System.DateTime e = new System.DateTime()) { System.Console.WriteLine ("e = {0}", e); }
    static void Main() { M(); }
}

Or this

    static void M(System.DateTime e = default(System.DateTime)) ...

The IL is this:

.method private hidebysig static void  M([opt] valuetype [mscorlib]System.DateTime e) cil managed
{
  .param [1] = nullref

I somehow hadn't realised that default(DateTime) or new System.DateTime() counted as a literal constant in C#. I'm surprised this hasn't come up before, but there you go.

We think we can make it that Unchecked.defaultof<_> and/or System.DateTime() are accepted. Will take a look.

xperiandri · 2021-03-15T23:38:56Z

What about constructors? Does this PR cover them too?

kurtschelfthout added 2 commits November 21, 2016 22:32

First rough try.

a6e88c3

Write DefaultParameterValue to Constant table.

0f8f9e7

msftclas added the cla-already-signed label Nov 22, 2016

Merge remote-tracking branch 'refs/remotes/origin/master' into defaul…

b4eaaf6

…t-param-value # Conflicts: # src/fsharp/IlxGen.fs # src/fsharp/TcGlobals.fs

dsyme reviewed Nov 22, 2016

View reviewed changes

Rename GenDefaultParameterValue and make local.

e530944

dsyme closed this Nov 23, 2016

dsyme reopened this Nov 23, 2016

msftclas added the cla-already-signed label Nov 23, 2016

Optional+DefaultParamValue args F# to F# works.

3d36778

dsyme reviewed Nov 24, 2016

View reviewed changes

Add first set of tests.

88def88

dsyme reviewed Nov 25, 2016

View reviewed changes

kurtschelfthout added 2 commits November 25, 2016 21:44

Add more tests.

c1d14c8

Merge branch 'master' into default-param-value

5f060a3

kurtschelfthout added 2 commits November 25, 2016 22:59

Extract const to fieldinit code to active pattern.

39abee3

Add tests for Nullable optional parameters.

c65c1a0

dsyme reviewed Nov 26, 2016

View reviewed changes

kurtschelfthout added 2 commits November 26, 2016 21:15

Add one more test.

cf52069

Add compiler error to avoid unverifiable code.

4ba096b

forki reviewed Nov 27, 2016

View reviewed changes

kurtschelfthout added 3 commits November 27, 2016 16:57

Spaces.

13a138c

Merge branch 'master' into default-param-value

960b305

kurtschelfthout changed the title ~~[WIP] DefaultParameterValue shenanigans~~ Calling Optional and DefaultParameterValue actually pass the default value, and can be specified in F# and called from F# Nov 27, 2016

dsyme requested changes Nov 27, 2016

View reviewed changes

kurtschelfthout changed the title ~~Calling Optional and DefaultParameterValue actually pass the default value, and can be specified in F# and called from F#~~ Omitting Optional; DefaultParameterValue arguments passes the default value and works for F# to F# calls Nov 28, 2016

kurtschelfthout mentioned this pull request Nov 28, 2016

New case for FS3186: An error occurred while reading the F# metadata node #1626

Closed

kurtschelfthout added 3 commits November 28, 2016 21:42

Make error into a warning, improve text.

06ebd93

Clean up test names, more conventional.

64d00e3

Add test thats calls F# from F# in other assembly.

2e44225

dsyme approved these changes Nov 29, 2016

View reviewed changes

dsyme merged commit 9f407d5 into dotnet:master Nov 29, 2016

kurtschelfthout mentioned this pull request Nov 29, 2016

Add FS-1027 DefaultParameterValue related changes. fsharp/fslang-design#150

Merged

NinoFloris mentioned this pull request Feb 19, 2019

[Docs] Add example of DefaultParameterValue accepting the 'default' value of a valuetype #6251

Closed

Omitting Optional; DefaultParameterValue arguments passes the default value and works for F# to F# calls #1812

Omitting Optional; DefaultParameterValue arguments passes the default value and works for F# to F# calls #1812

Uh oh!

Conversation

kurtschelfthout commented Nov 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dsyme commented Nov 22, 2016

Uh oh!

dsyme commented Nov 22, 2016

Uh oh!

kurtschelfthout commented Nov 23, 2016

Uh oh!

kurtschelfthout commented Nov 23, 2016

Uh oh!

dsyme commented Nov 24, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dsyme commented Nov 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kurtschelfthout commented Nov 25, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kurtschelfthout commented Nov 27, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kurtschelfthout commented Nov 27, 2016

Uh oh!

dsyme commented Nov 27, 2016

Uh oh!

dsyme left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kurtschelfthout Nov 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kurtschelfthout commented Nov 28, 2016

Uh oh!

dsyme commented Nov 28, 2016

Uh oh!

kurtschelfthout commented Nov 28, 2016

Uh oh!

dsyme commented Nov 29, 2016

Uh oh!

dsyme commented Nov 29, 2016

Uh oh!

kurtschelfthout commented Nov 29, 2016

Uh oh!

haf-lynx commented Dec 1, 2016

Uh oh!

forki commented Dec 1, 2016

Uh oh!

goswinr commented Apr 18, 2018

Uh oh!

cartermp commented Apr 18, 2018

Uh oh!

haf commented May 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cartermp commented May 15, 2018

dsyme commented Nov 25, 2016 •

edited

Loading

kurtschelfthout Nov 28, 2016 •

edited

Loading

haf commented May 14, 2018 •

edited

Loading