Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Verify that in parameter modifier is supported #625

Merged
merged 2 commits into from
Jun 9, 2018

Conversation

stakx
Copy link
Contributor

@stakx stakx commented Jun 9, 2018

This adds a few tests, including those mentioned in castleproject/Core#339 (comment) and castleproject/Core#339 (comment), to document the state of in parameter modifier support.

As it turns out, the previous update of Castle.Core to 4.3.0 makes in parameter modifiers work on .NET Core even though Moq doesn't specifically target .NET Standard 1.5 or above... it suffices that Castle.Core does that.

(.NET Standard 1.3 vs 1.5 is important because the latter includes the System.Reflection.Emit facilities required for mocking in; more specifically, the ability to emit custom modifiers.)

This closes #605.

@stakx stakx merged commit 396a9e5 into devlooped:master Jun 9, 2018
@stakx stakx deleted the in-parameter-modifier branch June 9, 2018 15:27
@zvirja
Copy link

zvirja commented Jun 9, 2018

@stakx And what about delegates with in modifiers? Do you also support them? We have the same situation in NSubstitute and I'm guessing that'll make us to target 1.5 directly..

@zvirja
Copy link

zvirja commented Jun 9, 2018

@stakx Oh, disregard - you have a test for that. That's really interesting - will investigate later how that works, given that we emit own interface...

@stakx
Copy link
Contributor Author

stakx commented Jun 9, 2018

@zvirja - In Moq, think the important bits occur here:

https://github.com/moq/moq4/blob/396a9e5f2196ee20a7df0a74b0495db2835af943/Source/ProxyFactories/CastleProxyFactory.cs#L143-L149

Note that in the call to DefineMethod, the original delegate's ParameterInfos are supplied. ParameterInfo carries with it the means to query custom attributes as well as custom modifiers, so (I guess) System.Reflection.Emit is smart enough to copy those.

https://github.com/moq/moq4/blob/396a9e5f2196ee20a7df0a74b0495db2835af943/Source/ProxyFactories/CastleProxyFactory.cs#L151-L154

I'm not actually sure the above loop is needed at all—quite possibly it does little more than copy the parameter names (and perhaps even that has already happened).

@zvirja
Copy link

zvirja commented Jun 9, 2018

Note that in the call to DefineMethod, the original delegate's ParameterInfos are supplied.

I dont see that. You pass parameter types, but they don't carry modifiers..

I suspect that it happens because runtime allows delegate and method to differ by in modifiers for parameters, so it doesn't matter that generated method is without it. But it's just a guess.

@stakx
Copy link
Contributor Author

stakx commented Jun 9, 2018

@zvirja - Oh boy, you're right. Now I feel like a fool. 🦆😆 The tests I've already got there might be inadequate then. I've done some more REPL-style testing, Moq indeed doesn't reproduce the delegate method's signature accurately, yet invoking the mocked delegate still works. If, during your own investigations, you find a way of making mocked delegate invocation fail, I'd love to hear what your tests look like!

@stakx
Copy link
Contributor Author

stakx commented Jun 9, 2018

@zvirja - I found out why delegate mocking works even when custom modifiers aren't replicated. It seems that for delegates, the CLR performs a signature check that's less strict than usual:

public delegate void WithIn(in AttributeTargets arg);

public interface WithoutIn
{
	void Invoke(ref AttributeTargets arg);
}

public class Impl : WithoutIn
{
	public void Invoke(ref AttributeTargets arg)
	{
		Console.WriteLine($"Called with {arg}.");
	}
}

class Program
{
	static void Main(string[] args)
	{
		// The following will print 1 and 0, meaning that the metho signatures differ!
		Console.WriteLine(typeof(WithIn).GetMethod("Invoke").GetParameters()[0].GetRequiredCustomModifiers().Length);
		Console.WriteLine(typeof(WithoutIn).GetMethod("Invoke").GetParameters()[0].GetRequiredCustomModifiers().Length);

		// Despite the difference in signature, the methods are compatible when
		// going through a delegate invocation:
		var interfaceMethod = typeof(WithoutIn).GetMethod("Invoke");
		var deleg = (WithIn)interfaceMethod.CreateDelegate(typeof(WithIn), new Impl());
		deleg.Invoke(AttributeTargets.All);
	}
}

I might raise an issue over at dotnet/coreclr later to ask whether that's accidental or intentional, but for now I'll add custom modifier replication code to Moq. Thanks for double-checking this PR! 👍

@zvirja
Copy link

zvirja commented Jun 11, 2018

@stakx Thanks for raising the investigation. It's indeed a critical issue, as you can now rewrite the in value, which supposed to be read-only.

Let's see what guys think about this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

C# 7.2 in ValueType parameters are not supported with .NET Standard / Core
2 participants