Runtime: `ParameterBuilder.SetConstant(null)` should be allowed for value-typed parameters

[stakx]

TL;DR: Could ParameterBuilder.SetConstant be made less strict so it accepts nullref for value-typed parameters, too?

(I'm requesting this for https://github.com/castleproject/Core/issues/291 and it is somewhat related to bug report dotnet/runtime#24574, which confirms that setting an optional value-typed parameter's default value to nullref in IL is common practice and not incorrect.)

Let's say I need to faithfully reproduce an existing methods' signature using System.Reflection.Emit. Take for example this method:

public static void Method(SomeStruct arg = default(SomeStruct)) /* where SomeStruct : struct */ { }

The C# compiler would emit this as:

.method public hidebysig static void Method([opt] valuetype SomeStruct arg) cil managed 
{
  .param [1] = nullref  // default(SomeStruct) is represented by the null reference
  // ...
}

Unfortunately, it is currently impossible to produce the same output with System.Reflection.Emit:

// preparation:
var assembly = AssemblyBuilder.DefineDynamicAssembly(new AssemblyName("Assembly"), AssemblyBuilderAccess.Run);
var module = assembly.DefineDynamicModule("Module");
var type = module.DefineType("Type", TypeAttributes.Public);
var method = type.DefineMethod("Method", MethodAttributes.Public | MethodAttributes.Static, typeof(void), new[] { typeof(SomeStruct) });
var parameter = method.DefineParameter(1, ParameterAttributes.Optional, "arg");

// attempting to do the equivalent of IL's `.param [1] = nullref` will throw:
parameter.SetConstant(null);

// ...

Unhandled Exception: System.ArgumentException: Null is not a valid constant value for this type.
   at System.Reflection.Emit.TypeBuilder.SetConstantValue(ModuleBuilder module, Int32 tk, Type destType, Object value)
   at ...

Calling parameter.SetConstant(default(SomeStruct)) instead wouldn't work either:

Unhandled Exception: System.ArgumentException: SomeStruct is not a supported constant type.
   at System.Reflection.Emit.TypeBuilder.SetConstantValue(ModuleBuilder module, Int32 tk, Type destType, Object value)
   at ...

So I'm left with no way of faithfully reproducing the shown C# method's signature. The only current workaround is not to call parameter.SetConstant at all, but that will result in later reflection reporting a default value of Missing.Value instead of null.

According to ECMA-335, the type of a parameter's default value (supplied via .param [n] = constant) does not have to match the parameter's actual type. The runtime doesn't do anything with the constant, it's merely there for e.g. importing compilers to reflect over. Only CLS compliancy rules require that the types match. However, I think it should be possible to emit non-CLS-compliant code with System.Reflection.Emit, too.

[JonHanna]

That seems reasonable, though I'm wondering why that degree of fidelity is necessary. The produced code would still work correctly as an actual method:

C# var assembly = AssemblyBuilder.DefineDynamicAssembly(new AssemblyName("Assembly"), AssemblyBuilderAccess.RunAndCollect); var module = assembly.DefineDynamicModule("Module"); var type = module.DefineType("Type", TypeAttributes.Public); var method = type.DefineMethod("Method", MethodAttributes.Public, typeof(TimeSpan), new[] { typeof(TimeSpan) }); var parameter = method.DefineParameter(1, ParameterAttributes.Optional, "arg"); var il = method.GetILGenerator(); il.Emit(OpCodes.Ldarg_1); il.Emit(OpCodes.Ret); var resultType = type.CreateType(); dynamic d = Activator.CreateInstance(resultType); TimeSpan ts = d.Method(); Console.WriteLine(ts == TimeSpan.Zero); // True

[stakx]

The use case here, AFAIK, is e.g. DI containers like Castle Windsor or Autofac that might perform reflection over generated dynamic proxy types. They need to know what values to inject/provide for optional parameters, and if that information isn't accurate, they'll fail. See e.g. https://github.com/castleproject/Core/issues/291 which references a couple such issues where missing default value info actually causes errors... e.g. https://github.com/aspnetboilerplate/aspnetboilerplate/issues/2355 or https://github.com/castleproject/Core/issues/45 or https://github.com/castleproject/Core/issues/141. (These issues aren't all precisely about DI nor about nullref for value types, but they show that incorrectly reproduced default parameter values does indeed cause problems.)

I'm not quite sure what kind of late binding code gets executed when you use dynamic (like you did above), but I'm fairly certain that there are use cases where you cannot or prefer not to use the DLR, but directly use reflection instead. For example, you'd see a problem appear when you create an instance of the dynamically generated type, then call .GetType() on it, and proceed to reflect over methods and their parameters.

[stakx]

@karelz, seeing that @AtsushiKan added this to the Future milestone, is there something that will have to be discussed before work on this can get started... or are you OK if I have a go at this?

Without having run any preliminary tests, at first glance it appears that the solution here would be quite simple: In dotnet/coreclr, file src/mscorlib/src/System/Reflection/Emit/TypeBuilder.cs, remove lines 401-406:

 internal static unsafe void SetConstantValue(ModuleBuilder module, int tk, Type destType, Object value)
 {
     ...
     if (value != null)
     {
          ...
     }
     else
     {
-        if (destType.IsValueType)
-        {
-            // nullable types can hold null value.
-            if (!(destType.IsGenericType && destType.GetGenericTypeDefinition() == typeof(Nullable<>)))
-                throw new ArgumentException(SR.Argument_ConstantNull);
-        }
+       // A null default value in metadata is permissible even for non-nullable value types.
+       // This is how the Roslyn compilers generally encode `default(TValueType)` default values.

         SetConstantValue(module.GetNativeHandle(), tk, (int)CorElementType.Class, null);
     }
 }

[karelz]

@AtsushiKan would you take such PR?

[ghost]

If someone wants to grab this item, it's fine by me.

[stakx]

Great, I'll take a look at this then!

@AtsushiKan - I have one question about where (and when) to add tests: System.Reflection is implemented over in dotnet/coreclr, but it might be appropriate to also add tests in dotnet/corefx (under https://github.com/dotnet/corefx/tree/master/src/System.Reflection/tests). How would I go about this? Should I first submit a coreclr PR, then wait until it gets merged into master, and only then send a PR for corefx tests?

[ghost]

Should I first submit a coreclr PR, then wait until it gets merged into master, and only then send a PR for corefx tests?

One more step: wait until CoreFX starts consuming the CoreCLR with your changes (usually takes a couple days after the merge into master)

Other than that, you got it exactly right.

[karelz]

@stakx I sent you collaborator invite. Once you accept, we will be able to assign the issue to you (GH limitation). Assigning to myself temporarily, please ping me when you accept the invite.
Pro-tip: Being collaborator will automatically subscribe you to all repo notifications (500+ day). We recommend to switch the repo to "Not Watching" which will send you notifications only on issues you are tagged, assigned, or you subscribe for explicitly.

[stakx]

@karelz: Thanks a lot for the invite. I've gladly acepted it. :)