NullReferenceException when reading a virtual member with a *value type* return type in the c'tor

[ulrichb]

public class CtorWithVirtualCalls
{
  public CtorWithVirtualCalls ()
  {
    // this works and prints "NULL":
    Console.WriteLine (VirtualString ?? "NULL"); 

    // this throws a NullReferenceException within the proxy, namely
    // Castle.Proxies.CtorWithVirtualCallsProxy.get_VirtualInt():
    Console.WriteLine ("VirtualInt: " + VirtualInt); 
  }

  public virtual string VirtualString { get; set; }
  public virtual int VirtualInt { get; set; }
}

[Test]
public void Test ()
{
  A.Fake<CtorWithVirtualCalls>();
}

BTW: Yes of course, the virtual calls in the c'tor are an anti-pattern.

I saved the generated proxy assembly and decompiled get_VirtualInt where the exception is thrown, and mentioned that the exception is caused by the cast to int in the return statement return (int) t_s.ReturnValue;. (Unboxing null results in a NullReferenceException.)

Why has ReturnValue a value of null? Because during the c'tor execution, the FakeManager hasn't attached to the proxy (=> ProxyInterceptor.CallWasIntercepted is still null). And therefore the ReturnValue won't be set and stays at the value of null. Normally (outside of the c'tor) this cannot happen, because the DefaultReturnValueRule fallback would return default(T) for value types. BTW, this is one part of the problem of #365, because during the c'tor the interceptor cannot delegate to the FakeManager and therefore the c'tor cannot use an already configured mock.

I have 2 suggestions to solve this issue:

• Extend the ProxyInterceptor and ensure, that we never return null for value types (escape to default(T))
• Attach the FakeManager earlier (e.g. at the time when the interceptor is being called the first time) instead of after the c'tor has been executed, so that all virtual calls during the c'tor behave like outside of the c'tor (=> VirtualInt would return 0 in my example and VirtualString would return the empty string instead of null).

I would prefer the second option because a) I think it's nice that the virtual members behave the same way inside/outside of the c'tor and b) this would solve one part of #365, as mentioned above.

Note that the issue described above is the root cause for the FakeCreationException mentioned in #367 (I created a new one because the discussion in #367 is now just about the missing inner exception message; I recommend changing the title).
It was also mentioned in #365 (comment) (the wrapped NullReferenceException > TargetInvocationException > FakeCreationException).

[blairconrad]

Very detailed analysis, @ulrichb. Thanks. And thanks for the note about #367, which I hadn't meant to change completely into a discussion about the exception message, but am still happy about having an issue about the exception messages when constructors fail. As you suggest, I will update the title.

[ulrichb]

@blairconrad Many thanks!

[blairconrad]

For what it's worth, I also like the idea of attaching the FakeManager earlier, for many of the same reasons. I'm not sure this is going to be an easy thing, though.

[blairconrad]

To quote @ulrichb in a heretofore private conversation:

[I've] spiked the following: creating the FakaManager before creating the proxy and moving the FakeManager.Intercept() call directly into DelegateProxyGenerator resp. CastleDynamicProxyGenerator. … and it works!
I had to comment out the corresponding unit tests, just to make the whole thing buildable and then all tests, including the specs and integr. tests are greeen. :)
Additionally I wrote a spec for my virtual calls in the c'tor and everything works as expected (the virtual members behave the same way as outside of the c'tor)

That's very encouraging. @ulrichb, may we consider the issue taken by you?

[adamralph]

Isn't this just one aspect of the more general #371?

[blairconrad]

It is, but predates it. I believe @ulrich had started to work on this, but had realized that the way to fix it was to fix the more general issue.
This issue could have been renamed, but as it is, it could be fodder for the search engines, in case someone is using a slightly older (than the version that gets the fix) version of FakeItEasy and starts searching for their symptoms.
Now that you bring it up, closing this issue as a duplicate of #371 would accomplish the same thing as keeping it open and having the #372 close both.

[ulrichb]

@adamralph Yes, exactly (I also mentioned it in the description of #371). I created 2 separate issues because a) I think this is a bug and #371 is a new feature that goes far beyond solving just this NullReferenceException and b) as mentioned above this issue could also be solved simpler by just escaping unhandled value type results in the Castle interceptor.

[adamralph]

Shall we re-label this as a bug?

[blairconrad]

I'm not fussy. As always, it's in the eye of the beholder.
Argument for: "bad things happen".
Argument against: "we never really tried to support intercepting anything before the proxy was constructed".

Whatever you want to do, I'm happy.

[adamralph]

Interesting... OK, I'll ask a different question: Does adding this feature/fixing this bug provide any value? I.e. if the exception is not thrown, is the fake still usable?

[blairconrad]

Merging #373 would cause the exception not to be thrown, and calls to virtual members from the faked class's constructor would act as if they were unconfigured methods that were called after proxy construction.
Basically this means that return values would be Dummies instead of nulls. Depending on the class being faked, it would be usable. I could contrive examples of classes that would be usable or would not be, if needed. 😉

Of course, if the fake's private members were initialized with the return values of the virtual methods, the client may not be able to further configure their actions (unless there's an accessor, I suppose). @ulrichb has a further PR (the bits that we sliced off #373) in the wings to provide that functionality.

As I understand this issue and the PR that we're reviewing for it, there is value. I would be happy to see the functionality [fixed|added], and would not object to releasing a package with it, although there's more benefit if we take the second PR as well.

Does that have the information you were hoping for?

[ulrichb]

For my feeling it's a bug because when we have such a class (read access in c'tor to virtual member with value type return value), we aren't able to instantiate A.Fake<T>() (instead we get a null reference first chance exception), although the result of the virtual member may be irrelevant. We maybe aren't interested in this member at all and want to change the behavior of other virtual members of the fake.

[blairconrad]

Fixed by #373.

[adamralph]

I've relabelled as a bug, as per earlier discussion.

[ulrichb]

@blairconrad, @adamralph many thanks!

[blairconrad]

@adamralph:

relabelled as a bug

Ah, yes! Thanks.

[blairconrad]

@ulrichb, thanks very much for your work on this issue. Look for your name in the release notes. 🏆

This issue has been fixed in release 1.25.0.

https://www.nuget.org/packages/FakeItEasy/1.25.0
https://github.com/FakeItEasy/FakeItEasy/releases/tag/1.25.0