ParameterBuilder.SetConstant fails when using a default value of null #35

Closed
jonasro opened this Issue Oct 28, 2013 · 11 comments

Projects

None yet

3 participants

@jonasro
jonasro commented Oct 28, 2013

Castle.DynamicProxy.Generators.Emitters.DefineParameters line 162 fails if one of the parameters passed in is a nullable type with a default value of null.

To reproduce change the default value in ClassWithMethodWithParameterWithDefaultValue.Method to null.

public class ClassWithMethodWithParameterWithDefaultValue
{
    public virtual void Method(int? value = null)
    {  }
}

Run test MethodParameterWithDefaultValue_DefaultValueIsSetOnProxiedMethodAsWell.

    [Test]
    public void MethodParameterWithDefaultValue_DefaultValueIsSetOnProxiedMethodAsWell()
    {
        var proxiedType = generator.CreateClassProxy<ClassWithMethodWithParameterWithDefaultValue>().GetType();

        var parameter = proxiedType.GetMethod("Method").GetParameters().Single(paramInfo => paramInfo.Name == "value");

        Assert.True(parameter.HasDefaultValue);
        Assert.AreEqual(3, parameter.DefaultValue);
    }
@kkozmic
Member
kkozmic commented Oct 28, 2013

3.2 or 3.2.1?

@jonasro
jonasro commented Oct 28, 2013

3.2.1. Reproduced it on master,

@kkozmic
Member
kkozmic commented Oct 28, 2013

ok, do you want to have a stab at pull-requesting it?

i reckon it shouldn't be all that hard to fix code-wise

@jonasro
jonasro commented Oct 29, 2013

Sure. I could do that, but I am not sure this is fixable since doing SetConstant does not seem to support setting default value null.
So unless you have another idea the fix would be not setting the value if default value is null.

@kkozmic
Member
kkozmic commented Oct 29, 2013

Oh interesting.
I didn't realize that.

Krzysztof,
sent from my phone
On 29/10/2013 6:27 PM, "jonasro" notifications@github.com wrote:

Sure. I could do that, but I am not sure this is fixable since doing
SetConstant does not seem to support setting default value null.
So unless you have another idea the fix would be not setting the value if
default value is null.


Reply to this email directly or view it on GitHubhttps://github.com/castleproject/Core/issues/35#issuecomment-27285182
.

@pawelpabich

@kkozmic what do you think about @jonasro idea? Would it work?

@kkozmic
Member
kkozmic commented Nov 11, 2013

@pawelpabich I don't know.

@jonasro sent a pull request #36 but I haven't tried integrating it yet.
@jonasro - does it then handle the scenario?

Now that I think about it - knowing how the default value works, being at compile time, with DP working at runtime, if the value is not there, it should still work.

I'll give it a go and see later this week.

@pawelpabich

Great

@jonasro jonasro added a commit to jonasro/Core that referenced this issue Nov 11, 2013
@jonasro jonasro Added test for issue #35 5ab532c
@kkozmic kkozmic added a commit that referenced this issue Dec 1, 2013
@kkozmic kkozmic Added test for issue #35
(cherry picked from commit 5ab532c)
8163b42
@kkozmic kkozmic added a commit that closed this issue Dec 1, 2013
@kkozmic kkozmic updated changes.txt
fixed #35
closes #36
9b06bb4
@kkozmic kkozmic closed this in 9b06bb4 Dec 1, 2013
@pawelpabich

Thanks a lot. How often do you release new versions on nuget ?

@kkozmic
Member
kkozmic commented Dec 1, 2013

Not terribly often but since this is a somewhat critical fix to an issue introduced in the last release I guess we'll do a release either today or next week

@pawelpabich

Awesome :)

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