Replace NullReferenceException thrown when interceptors swallow exceptions and cause a null value type to be returned

[jonorossi]

See #85.

[stakx]

I haven't done a detailed code review but run a few quick tests against this PR. It appears to be doing what it should. 👍

I found one minor issue regarding ref parameters. Say you have this:

using Castle.DynamicProxy;
using System;

class Program
{
    static void Main()
    {
		var generator = new ProxyGenerator();

		var proxy = generator.CreateInterfaceProxyWithoutTarget<IFoo>(new Interceptor());

		int x = 0;
		proxy.GetValue(ref x);
    }
}

class Interceptor : IInterceptor
{
	public void Intercept(IInvocation invocation)
	{
		invocation.Arguments[0] = null;
	}
}

public interface IFoo
{
	void GetValue(ref int x);
}

Then you get an error mentioning x as being an out parameter:

Unhandled Exception: System.InvalidOperationException: Interceptors either trashed or failed to set a value for out parameter 'x'
   at Castle.Proxies.IFooProxy.GetValue(Int32& x)
   at Program.Main()

In general, I am not sure if the additional check is needed at all for ref (or in) parameters, since an interceptor would have to maliciously reset an argument to null to provoke this error.

[jonorossi]

In general, I am not sure if the additional check is needed at all for ref (or in) parameters, since an interceptor would have to maliciously reset an argument to null to provoke this error.

I was wondering that too, it is probably unlikely someone would set an argument to null and wonder why it doesn't work. So remove the parameter check, and keep the return value check?

[jonorossi]

@stakx do you think the exception message is clear enough, or could be worded better/differently?

[stakx]

So remove the parameter check, and keep the return value check?

The argument check is probably a good idea for out parameters. So I'd say, only remove the parameter check for by-ref, but non-out parameters.

[jonorossi]

The argument check is probably a good idea for out parameters. So I'd say, only remove the parameter check for by-ref, but non-out parameters.

If you run the code you provided above with master you'll get a NullReferenceException because of the way all reference parameters work, they aren't actually "ref" because they can't be so their value is copied.

I can fix the exception to mention "ref" rather than "out", that was my mistake.

[stakx]

Interceptors either trashed or failed to set a return value; or swallowed the thrown exception

do you think the exception message is clear enough, or could be worded better/differently?

Generally, as a hint at the possible error source, it's definitely clear enough. The following points are just minor fine-tuning suggestions:

• As a non-native English speaker, I did initially stumble over the word "trashed"... what kind of error situation does this cover? I'm not sure if I'm missing something here, but the error message could possibly be simplified to just Interceptors failed to set a return value; .... (Why the interceptors failed to do that is probably not too important.)

• Perhaps add a brief explanation where the swallowed exception originated: ...; or swallowed the exception thrown by the target. (Do such swallowed exceptions in fact always originate from targets invoked by invocation.Proceed()? Or are there scenarios where this problem occurs when you have a chain of several interceptors?)

[stakx]

So remove the parameter check, and keep the return value check?

The argument check is probably a good idea for out parameters. So I'd say, only remove the parameter check for by-ref, but non-out parameters.

I need to take this back, I didn't realise that out parameter appear to be zero-initialized anyway, so the error would only occur when the interceptor makes an effort to set it to null explicitly.

So yes, perhaps the check could be removed completely for all by-ref parameters (even out ones).

[jonorossi]

Yep, your suggestions for the exception message make sense, done. And removed the parameter check.

[stakx]

(Seems a bit silly to me that I dare "approve" the project owner's own PR, but well... it's GitHub's wording. :)

Looks good to me! 👍