Runtime: From corefx's perspective, how should an optional DateTime parameter's default value be encoded in IL?

[stakx]

This is a follow-up question from https://github.com/dotnet/roslyn/issues/24004 intended to determine whether there is a bug in the C# compiler or the framework. I'd appreciate an answer from corefx's perspective that I can take back to the discussion in the referenced issue.

The following C# program will throw a runtime exception:

class Program
{
    public void Method(System.DateTime arg = default(System.DateTime)) { }

    static void Main()
    {
        var defaultValue = typeof(Program).GetMethod("Method").GetParameters()[0].DefaultValue;
    }
}

Unhandled Exception: System.FormatException: Encountered an invalid type for a default value.
   at System.Reflection.MdConstant.GetValue(MetadataImport scope, Int32 token, RuntimeTypeHandle fieldTypeHandle, Boolean raw)
   at System.Reflection.RuntimeParameterInfo.GetDefaultValueInternal(Boolean raw)
   at System.Reflection.RuntimeParameterInfo.GetDefaultValue(Boolean raw)
   at Program.Main() in ...\Program.cs:line 7

An equivalent program written in VB.NET would work just fine. Unlike the VB.NET compiler, the C# compiler does not emit a [System.Runtime.CompilerServices.DateTimeConstant] custom attribute on the optional parameter; instead, it assigns a default value equalling nullref to the parameter:

IL produced by the C# compiler:

// public void Method(System.DateTime arg = default(System.DateTime))
.method public hidebysig instance void Method([opt] valuetype [mscorlib]System.DateTime arg)
  cil managed 
{
  .param [1] = nullref
  // ...
}

IL produced by the VB.NET compiler:

// Public Shared Sub Method(Optional arg As System.DateTime = Nothing)
.method public instance void Method([opt] valuetype [mscorlib]System.DateTime arg) cil managed 
{
  .param [1]
  .custom instance void [mscorlib]System.Runtime.CompilerServices.DateTimeConstantAttribute::.ctor(int64) = ( 01 00 00 00 00 00 00 00 00 00 00 )
  // ...
}

In https://github.com/dotnet/roslyn/issues/24004#issuecomment-354922688, @HaloFour said that the exception appears to be a bug in the BCL's reflection code.

So I am left wondering, who is to blame for the runtime exception?

• the C# compiler, because it does not emit the [System.Runtime.CompilerServices.DateTimeConstant] attribute that the BCL reflection code apparently expects for optional DateTime parameters; or
• the implementation of ParameterInfo.DefaultValue in the BCL, which should allow .param [n] = nullref and interpret it as default(DateTime), i. e. a DateTime with .Ticks == 0?

[JonHanna]

Unlike the VB.NET compiler, the C# compiler does not emit a [System.Runtime.CompilerServices.DateTimeConstant] custom attribute on the optional parameter.

Unlike VB.NET, the only possible compile-time constant value for DateTime is default(DateTime) and that is represented just as validly by DateTimeConstantAttribute or by OptionalAttribute alone. In the latter case DefaultValue would be null, but it shouldn't throw (compare with the same code using another struct as the type of the parameter).

This would seem to be a bug in the implementation of Default, perhaps a legacy of the days when C# didn't have optional parameters but VB.NET did and so any default DateTime parameter likely in real code would be done the VB.NET way.

[HaloFour]

I've confirmed that it seems to work with any struct that I've thrown at it with the sole exception of System.DateTime. I tried this with .NET 4.7.

[stakx]

@HaloFour - It might also be interesting to throw nullable structs at it... have you tried this? I'm off for today, but might try this tomorrow.

Either way, I'm asking specifically about DateTime because as noted above, it seems to be the only case that doesn't work from C#. (decimal, another potential special case, works fine because the compiler knows to emit [DecimalConstantAttribute]. If it didn't, we might have another buggy case.)

[JonHanna]

It might also be interesting to throw nullable structs at it.

They work fine, including DateTime?

[JonHanna]

The flaw is in https://github.com/dotnet/coreclr/blob/d2f2e81294a80a1b463ea185decd07e49e67f976/src/mscorlib/src/System/Reflection/MdConstant.cs#L97-L98 where the case of a default value for a DateTime that is optional but has no metadata about the value itself included (should be CorElementType.Class I think) and return the relevant value.

A question is just what is that relevant value? There's an argument to be made for default(DateTime) in terms of convenience, but null would be more consistent with other struct types.

[stakx]

@JonHanna:

A question is just what is that relevant value? There's an argument to be made for default(DateTime) in terms of convenience, but null would be more consistent with other struct types.

I believe the best solution (compromise, actually) would be...

1. for ParameterInfo.DefaultValue to return null (like for all value types) in the absence of [DateTimeConstant]; and
2. for the C# compiler to emit a [DateTimeConstant] attribute so it actually gets back default(DateTime) via reflection.

The second point is probably optional, but it seems important to me because otherwise decimal becomes a new sole exception: Reflection would return null for all value-typed default values except with decimal, for which the C# compiler emits a [DecimalConstant] and therefore gets something other than null from reflection. If the C# compiler also emitted a [DateTimeConstant] for DateTime, we'd have two exceptions—but at least these "exceptions" would now match up with, and therefore be "documented" and explained by the presence of, these two attribute types in System.Runtime.CompilerServices.

[JonHanna]

Decimal is a sole exception in C#, because it's the sole struct that C# can have constant values for that aren't default, so the status quo seems sound to me as far as the C# compilation goes. Still, that's a matter for the clang and roslyn repos; for corefx null for such existing cases does seem to be the thing to do.

[stakx]

Decimal is a sole exception in C#, because it's the sole struct that C# can have constant values for that aren't default [...]

@JonHanna - that's true. I meant that in a slightly different manner. Currently, decimal being a sole exception is not really noticeable when doing reflection, because ParameterInfo.DefaultValue acts so peculiarly: Sometimes it returns null, sometimes Missing.Value, sometimes it returns an actual struct value, sometimes it throws an exception... if we did what I suggest above, things would seem a little bit more sane and explicable:

• You'd get null for reference types.
• You'd also get null for value types that do not have a dedicated [...Constant] attribute in System.Runtime.CompilerServices. (default(TStruct) would make more sense than null, but that ship has probably sailed and things can't be changed now due to backwards compatibility concerns.)
• You'd get an actual value for those value types that have a dedicated [...Constant] attribute.

(It would probably still report Missing.Value in some cases, but likely not for code that was produced by the C# or VB.NET compilers.)

Still, that's a matter for the clang and roslyn repos.

True. I'll probably follow up on this with them.

for corefx null for such existing cases does seem to be the thing to do.

Agreed. I'd be totally happy with that. :+1:

[stakx]

@JonHanna - two more things regarding how to proceed:

1. Can this issue act as a bug report, or should I submit a new one with only the result of this discussion, as well as a summary of the expected and actual behavior?
2. If I'd also like to see this fixed in the full .NET Framework, do I need to file a separate bug report at Microsoft/dotnet, or would a bugfix in .NET Core automatically trickle down into the .NET Framework?

[HaloFour]

I opened https://github.com/dotnet/csharplang/issues/1236 to inquire as to whether the C# spec would allow that change.

I personally don't think that the C# compiler should start emitting that attribute now. It would silently change the semantics of existing code, even if that's not in a way that would likely have any negative consequences. But it's worth asking the question.

C# has had optional parameters for almost a decade now. Has this bug been sitting there that long? If so it doesn't seem to be such an issue to require trying to address it in multiple places.

[stakx]

@HaloFour, thanks a lot for opening that other issue!

It would silently change the semantics of existing code

How that? (Actually, perhaps we'd better discuss that over at dotnet/csharplang.)

I personally don't think that the C# compiler should start emitting that attribute now.

I could also live with that. If you know that a ParameterInfo.DefaultValue == null for a value-typed parameter really means default(TStruct), you can account for that in user code. This should however be properly documented.

C# has had optional parameters for almost a decade now. Has this bug been sitting there that long?

I originally got involved with all this while looking into https://github.com/castleproject/Core/issues/291. There's a trail of GitHub issues related to this going back to at least early 2016. DynamicProxy used to have a different issue tracker in the past. I'm pretty sure this problem has been around for much longer than that; it's just not very noticeable because it's probably not a very common use case.

[JonHanna]

If I'd also like to see this fixed in the full .NET Framework, do I need to file a separate bug report at Microsoft/dotnet, or would a bugfix in .NET Core automatically trickle down into the .NET Framework?

It's probably worth reporting on connect or visual studio, though there is also a "netfx-port-consider" label that gets used to indicate "hey, maybe we should do this for the .NET Framework too", but I don't have any insight into that beyond seeing the label being attached.

[stakx]

The PR linked above just got merged, so this can be closed. Reflection on the CoreCLR is going to report a DefaultValue of null for an optional parameter of the form DateTime arg = default.