Private protected methods are not intercepted

[CrispyDrone]

Hello,

it seems that private protected methods are not intercepted, even though I would expect them to be:

using System.Runtime.CompilerServices;
using Castle.Core.Internal;
using System;
using NUnit.Framework;
[assembly: InternalsVisibleTo(InternalsVisible.ToDynamicProxyGenAssembly2)]

namespace Castle.DynamicProxy.Tests
{
	[TestFixture]
	public class BasicClassProxyTestCase : BasePEVerifyTestCase
	{
		private protected virtual void PrivateProtectedThrowException() => throw new Exception("I was called");
		protected virtual void ProtectedThrowException() => throw new Exception("I was called");
		internal virtual void InternalThrowException() => throw new Exception("I was called");

		[Test]
		public void PrivateProtectedVirtualMethodsAreInterceptedCorrectly()
		{
			var proxy = generator.CreateClassProxy<BasicClassProxyTestCase>(
				new DoNothingAndReturnDefaultInterceptor()
			);

			Assert.DoesNotThrow(() => proxy.InternalThrowException());
			Assert.DoesNotThrow(() => proxy.ProtectedThrowException());
			Assert.DoesNotThrow(() => proxy.PrivateProtectedThrowException());
		}

		public class DoNothingAndReturnDefaultInterceptor : StandardInterceptor
		{
			protected override void PerformProceed(IInvocation invocation)
			{
				object returnValue = invocation.ReturnValue;

				if (returnValue != null)
				{
					var returnValueType = returnValue.GetType();

					if (returnValueType.IsValueType)
					{
						invocation.ReturnValue = Activator.CreateInstance(returnValueType);
					}
					else
					{
						invocation.ReturnValue = null;
					}
				}
			}
		}
	}
}

This fails on the private protected call, but not on the internal, or protected one.

Is this expected behavior? If not, there is an existing issue in NSubstitute regarding this, and I think the root cause might then be with dynamicproxy.

[jonorossi]

I assume since your unit test is using BasePEVerifyTestCase that you've written this code against master, and so you've got the changes in #491 even though those changes don't look like they'd affect your unit test.

Since you've got this far, do you want to confirm that ProxyUtil.IsAccessibleMethod is returning true.

[CrispyDrone]

I can confirm that this passes:

var method = proxy
	.GetType()
	.GetMethod(
		nameof(PrivateProtectedThrowException),
		System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance
	);

Assert.IsTrue(
	ProxyUtil.IsAccessibleMethod(method)
);

[jonorossi]

My guess is that MembersCollector.AcceptMethod is the problem:

Core/src/Castle.Core/DynamicProxy/Contributors/MembersCollector.cs

Lines 225 to 229 in a7454e5

	//can only proxy methods that are public or protected (or internals that have already been checked above)
	if ((method.IsPublic || method.IsFamily || method.IsAssembly || method.IsFamilyOrAssembly) == false)
	{
	return false;
	}

@CrispyDrone could you add IsFamilyAndAssembly to that condition and see if that fixes your unit test. We probably don't need this check anyway since ProxyUtil.IsAccessibleMethod will handle this and is called first, but this is likely the quickest lowest risk fix.

[CrispyDrone]

@jonorossi

I can confirm that the unit test passes.

However something that might be interesting is that for the proxy, ProxyUtil.IsAccessibleMethod now returns false. The generated method is marked as internal I think; it only has the attribute Assembly, and no longer FamANDAssem.

I assume that ProxyUtil.IsAccessibleMethod should return false as the dynamic assembly in which the proxy is generated won't contain an InternalsVisibleTo attribute for the test assembly, but I think it's returning false for the wrong reasons now i.e. being a method marked as internal?

I might be wrong though, as I am only just dipping my toes into this codebase and I'm quite tired.

In any case I hope this helps!

[stakx]

@CrispyDrone, thanks for running these tests!

The generated method is marked as internal I think; it only has the attribute Assembly, and no longer FamANDAssem.

I suspect this by itself is unproblematic, ECMA-335 §II.10.33 specifically allows widening of accessibility in overrides. I vaguely remember having seen instances of such widening somewhere else (possibly in reference assemblies, as they only represent the public-facing parts). There is also a special case for narrowing. I haven't thought about this in detail but I thought I'd post a pointer here. At the surface it doesn't seem like a problem.

[stakx]

@CrispyDrone, I can confirm that @jonorossi's suggestion fixes this issue, see master...stakx:private-protected-methods.

Since it was you who did much of the work (describing the problem, and providing repro code 👍), I think it would be great if you could submit a bug fix PR. Please feel free to steal my commits if you find them useful.

Otherwise—if I don't hear back from you during the next two weeks or so—I'll go ahead and submit the above commits as a PR so we get this fixed.

[CrispyDrone]

@stakx

Hi again. Sorry for the late response, I have been preoccupied with some other things. I'm going to try to submit the PR this weekend. If I don't, feel free to continue without me.

[CrispyDrone]

@stakx

Actually, I'm fine with you submitting your fix as a PR. I would not feel comfortable with just taking your commits; I hope you understand. An acknowledgement in the changelog is more than enough for me at this time 😄

I'm sure I'll be able to make a real code contribution myself in the future!