Nullable parameters on controller methods

[worthy7]

I have explained the issue quite well here: aspnetboilerplate/aspnetboilerplate#2355 (comment)

Basically, if a method uses nullable parameters (int?) then this causes Castle.Windsor to error because it doesn't seem to want to use null, and instead replaces it with Missing.

[jonorossi]

@Worthy7 just in case you weren't aware this is the Castle Core issue tracker not the Castle Windsor one. I see you logged https://github.com/castleproject/Windsor/issues/306 which you then closed, however here is likely the right place since it sounds Castle DynamicProxy related.

Can you provide a reproduction (i.e. sample code) of how Castle DynamicProxy is failing/working incorrectly using just Castle Core without ASP.NET Boilerplate or ASP.NET MVC. System.Reflection.Missing.Value is a valid value for performing reflection calls with optional parameters, this may be a Castle DynamicProxy defect in this case or something ASP.NET MVC isn't handling.

[worthy7]

I see it was actually reported a long long time ago by Abp owner himself, castleproject/Core#45

Just for reference, we are now using Castle 4.0.0, so it is relatively up-to-date, but still has this issue. His code I assume is still a valid example

[jonorossi]

The example in #45 is still reproducible thanks, you'll see why that issue didn't go anywhere since @hikalkan closed it. #35 is an earlier one on the same problem, however I think the fix in 17c1596b1e7f547eaa15479ada0b793a306ab991 (back in 2013) might not have actually fixed the problem and resulted in the default value being left as System.Reflection.Missing (I've not confirmed the ParameterBuilder behaviour).

DynamicProxy used to set the default value on the generated type prior to 4.0, but that was causing all sorts of problems because ParameterBuilder.SetConstant is buggy, see #141 and you'll see a web of linked issues. We decided for 4.0 that optional parameters are 99% of the time just a compile time syntactic sugar feature and doesn't really affect DynamicProxy at runtime, however the default value obviously does go along with the metadata and developers can access the metadata of the proxy types using reflection.

I don't think this will be an easy bug to fix, but if someone would like to work through all the scenarios with ParameterBuilder and is able to find something we can actually do then we could re-instate this type of code. Be aware that ParameterBuilder.SetConstant never used to accept null as a value but looking at the reference source it appears this might have been fixed in a later .NET Framework version (can't find versions older than 4.6.2), we could conditionally compile for this version and above.

class Program
{
    static void Main(string[] args)
    {
        Console.WriteLine(typeof(MyClass).GetMethod("MyMethod1").GetParameters()[0].DefaultValue); // null
        Console.WriteLine(typeof(MyClass).GetMethod("MyMethod2").GetParameters()[0].DefaultValue); // "0"
        Console.WriteLine(typeof(MyClass).GetMethod("MyMethod3").GetParameters()[0].DefaultValue); // null

        var gen = new ProxyGenerator();
        var proxy = gen.CreateClassProxy<MyClass>();

        Console.WriteLine(proxy.GetType().GetMethod("MyMethod1").GetParameters()[0].DefaultValue); // System.Reflection.Missing
        Console.WriteLine(proxy.GetType().GetMethod("MyMethod2").GetParameters()[0].DefaultValue); // System.Reflection.Missing
        Console.WriteLine(proxy.GetType().GetMethod("MyMethod3").GetParameters()[0].DefaultValue); // null
    }
}

public class MyClass
{
    public virtual void MyMethod1(string str = null)
    {
        Console.WriteLine(str);
    }

    public virtual void MyMethod2(int i = 0)
    {
        Console.WriteLine(i);
    }

    public virtual void MyMethod3(string str)
    {
        Console.WriteLine(str);
    }
}

[stakx]

I've been looking into this out of curiosity, and—admittedly without testing on different versions of the CLR and Mono yet—I think I have found a way, at least for recent versions of the CLR, to correctly reproduce default values for most optional parameters; see https://github.com/castleproject/Core/compare/master...stakx:optional-parameters.

The previous implementation for copying default values of optional parameters appears to have been based on checking ParameterInfo.DefaultValue and then duplicating it with ParameterBuilder.SetConstant(object defaultValue). This is too simplistic.

What appears to work better is first checking whether the parameter is optional—but instead of querying ParameterInfo.HasDefaultValue, query ParameterInfo.IsOptional. Then we need to be very careful when to call ParameterBuilder.SetConstant. That method can only be used for compile-time constants that can be successfully placed in the Constant metadata table. (Other compile-time constants such as those for decimal and System.DateTime are encoded using custom attributes instead.)

I've also found a very few edge cases that simply don't work:

• The C# compiler doesn't properly encode void Method(DateTime arg = default(DateTime)). I've reported this as a bug at https://github.com/dotnet/roslyn/issues/24004. This doesn't directly affect DynamicProxy, but it means we can't have a unit test for such an optional parameter.

• Apart from that, setting default values for System.DateTime and decimal as well as their nullable counterparts should be feasible. It's done by replicating the [DateTimeConstant] and [DecimalConstant] attributes.

• System.Reflection.Emit doesn't support setting a default value for all other value types, e. g. void Method(SomeValueType arg = default(SomeValueType)). The best we can currently do is to not replicate the default value at all, so it'll get reported as Missing.Value. That's quite ugly. I'm thinking about filing a bug for this over at the microsoft/dotnet repo.

What's left to be done additional to the above branch in my fork is this:

• Test on Mono and apply corrections. Reproducing default values will possibly have to be disabled on Mono, if not for everything, then probably for nullable parameter types.

• Possibly put the whole default value reproduction logic inside a try..catch block swallowing any errors that might occur. While I'm fairly certain that I've got the logic basically right, there might be more edge cases I haven't caught yet. Default values are nice to have on the proxy, but non-essential, so this feature shouldn't be able to cause DynamicProxy failures.

Given that default values can only be reproduced faithfully 95% or so of the time, would you prefer to just not support this at all, or would you be interested in a PR, @jonorossi?

[jonorossi]

@stakx thanks for the detailed description.

One thing I never really looked into is HasDefaultValue vs IsOptional, what is the difference there?

I was aware of DecimalConstantAttribute, but hadn't seen DateTimeConstantAttribute which I now see C# has never implemented, and I guess no one uses System.Runtime.CompilerServices.CustomConstantAttribute which the CLR supports.

I think with the limitations I'd be more comfortable this being an opt-in feature where a developer has to consciously accept the limitations, otherwise I suspect we might get back to where we were when we get bug reports for the edge cases which we can't do much about, well until the runtime gets fixed. This would mean default values don't get replicated at all which most people don't care about, unless you do because you are using a library reflecting over your proxy types. I think the only thing that sort of works like this today would be AttributesToAvoidReplicating. Do you agree, I want your honest opinion.

[stakx]

I think the only thing that sort of works like this today would be AttributesToAvoidReplicating. Do you agree, I want your honest opinion.

I am divided on this matter, but at the end of the day, I agree with you that it's probably better to not reproduce default values than to have this feature in a semi-buggy state. Not sure how it could be made into an opt-in feature, though. (AttributesToAvoidReplicating won't suffice to make this opt-in/opt-out because not all default values are encoded as custom attributes; more on that further down.)

I fully understand and appreciate your position. Above, I was suggesting a try..catch safeguard in case some unexpected scenario shows up. That by itself is a good indication that it's probably indeed too early to implement default value reproduction in proxy methods.

At the same time, I think it'd be awesome if DynamicProxy were eventually able to faithfully reproduce default values.

Perhaps the best and safest approach would be trying to get the framework and/or C# compiler fixed first. Once that has happened, we can merge a PR that implements default value reproduction. I will keep the optional-parameters branch in my fork, just in case we ever get that far.

In the meantime, it might be nice to add DateTimeConstantAttribute and DecimalConstantAttribute to AttributesToAvoidReplicating and make MethodEmitter honor the latter (if it doesn't already), so that reflecting over proxy method parameters consistently reports Missing.Value for all parameters. (Right now, some default values are reported correctly, but most are not, and this inconsistency probably raises questions unnecessarily.)

I'm happy to submit a PR doing something AttributesToAvoidReplicating-related to make DynamicProxy more consistent... or any other solution you'd prefer, for that matter.

One thing I never really looked into is HasDefaultValue vs IsOptional, what is the difference there?

In IL, marking a parameter as optional and supplying a default value for it are distinct:

• In ILASM, you mark a parameter as optional using the [opt] modifier. This is what ParameterInfo.IsOptional looks for via ParameterInfo.Attributes. For example:

  .method private hidebysig static void SomeMethod (
             [opt] valuetype [mscorlib]System.DateTime dateTime
         ) cil managed 
  {
     ...
  }

  AFAIK the runtime doesn't do anything with the [opt] flag. It's merely a piece of metadata for compilers etc. to reflect over, so not providing a default value doesn't have any negative consequences. The framework library will just make up some fallback default value (e. g. Missing.Value or null).

• Supplying a default value for a parameter is done through the .param directive. This is what ParameterInfo.HasDefaultValue looks for:

  .method private hidebysig static void SomeMethod (
             [opt] valuetype [mscorlib]System.DateTime dateTime
         ) cil managed 
  {
     .param [1] = nullref // puts the supplied value in the Constant metadata table,
                          // thus this approach is limited to primitive types
     ...
  }
  
  .method private hidebysig static void SomeOtherMethod (
         [opt] valuetype [mscorlib]System.Decimal 'decimal'
     ) cil managed 
  {
      .param [1]
      .custom instance void [mscorlib]System.Runtime.CompilerServices.DecimalConstantAttribute::.ctor(uint8,  uint8,  uint32,  uint32,  uint32) = ( 01 00 02 00 00 00 00 00 00 00 00 00 39 30 00 00 00 00 )
      // ^ non-primitive default values can be encoded using custom attributes.
  }

Obviously, the framework cannot know whether an attribute defines a default value, and how to decode it, since there is no ConstantAttribute base class. It'll recognise only the pre-defined attribute types from System.Runtime.CompilerServices. So if you place [Optional, MyCustomConstant] attributes on a parameter, the compiler would turn the former into the [opt] metadata flag and emit the latter as an attribute, but reflection will not recognize that attribute. The end result would be that IsOptional returns true but HasDefaultValue would report false (and DefaultValue would report Missing.Value).

[jonorossi]

Not sure how it could be made into an opt-in feature, though. (AttributesToAvoidReplicating won't suffice to make this opt-in/opt-out because not all default values are encoded as custom attributes; more on that further down.)

I should have been clearer what I meant here, I wasn't talking about using AttributesToAvoidReplicating just that opting into this feature would need something static rather than being on ProxyGenerator so when a library like a mocking framework creates the ProxyGenerator inside you need to still talk to DP, AttributesToAvoidReplicating is our only thing that does that today, I just meant we'd need to do something similar.

I was suggesting a try..catch safeguard in case some unexpected scenario shows up. That by itself is a good indication that it's probably indeed too early to implement default value reproduction in proxy methods.

It seemed bad on my first read but we actually do this quite often in the DP code to handle strange bugs in the .NET Framework, some that have been fixed in later versions and some that are hard to reproduce. There isn't really a lot of point waiting until the runtime gets fixed because we'll be supporting older runtimes for quite a long time.

In the meantime, it might be nice to add DateTimeConstantAttribute and DecimalConstantAttribute to AttributesToAvoidReplicating and make MethodEmitter honor the latter (if it doesn't already), so that reflecting over proxy method parameters consistently reports Missing.Value for all parameters. (Right now, some default values are reported correctly, but most are not, and this inconsistency probably raises questions unnecessarily.)

True. We have to make a change, so possibly a breaking change either way.

Perhaps the best and safest approach would be trying to get the framework and/or C# compiler fixed first. Once that has happened, we can merge a PR that implements default value reproduction. I will keep the optional-parameters branch in my fork, just in case we ever get that far.

I think you should submit a PR for the actual change and we'll get it in, no need for opt-in we want to find any defects quickly and I don't really want different modes to run DP with. Looking at your diff again I think this time we know the limitations of the runtime much better and can add some documentation that lists the limitations with links to defects logged in the .NET issue trackers.

[stakx]

@jonorossi - Thanks for your detailed reply. I'll submit a PR based on my earlier work. It might take me a little while, as I'd like to make it as bullet-proof as possible, and document the edge cases a little better. I also anticipate some initial test failures on Mono, those might take some more work to iron out.

I think this time we know the limitations of the runtime much better and can add some documentation that lists the limitations with links to defects logged in the .NET issue trackers.

Are you thinking of a user documentation article under docs/, or just code comments?

[jonorossi]

@stakx sounds great. Code documentation is always best because it is the living document, but some user documentation in docs/ was what I was thinking. I think it really depends on what info you end up with, go with what feels natural, we can always have a brief user documentation article that points at the code for more detail.

[worthy7]

@stakx @jonorossi Shall we can close this if it's outdated/unneeded now?

[stakx]

@Worthy7: Sorry, it's taken me way longer than I thought as I got really busy at work soon after I posted the above two comments.

I'm still interested and willing to give this one a go if you can bear with me a while longer, so I'd suggest to keep this open. But of course it is your and @jonorossi's call.

[worthy7]

Honestly, I hit this problem because I was trying to do a crazy callback into a controller from a Razor view, which means the controller function was not getting called in the normal way (from a web request) and it was my lack of experience which led me there. A year on and I understand that it was the wrong was to build Razor views (I should have used an Orchestrator instead). I think it's probably not worth the work @stakx. PR separately if you want to do something about it if you don't mind.