8000 Verify that `in` parameter modifier is supported by stakx · Pull Request #625 · devlooped/moq · GitHub
[go: up one dir, main page]

Skip to content

Verify that in parameter modifier is supported - #625

Merged
stakx merged 2 commits into
devlooped:masterfrom
stakx:in-parameter-modifier
Jun 9, 2018
Merged

stakx merged 2 commits into
devlooped:masterfrom
stakx:in-parameter-modifier

Conversation

@stakx
@stakx stakx commented Jun 9, 2018 •
Copy link
Copy Markdown
Contributor

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
zvirja commented Jun 9, 2018
Copy link
Copy Markdown

@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
zvirja commented Jun 9, 2018
Copy link
Copy Markdown

@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
stakx commented Jun 9, 2018
Copy link
Copy Markdown
Contributor Author

@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
zvirja commented Jun 9, 2018 •
Copy link
Copy Markdown

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
stakx commented Jun 9, 2018
Copy link
Copy Markdown
Contributor Author

@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
stakx commented Jun 9, 2018 8000
Copy link
Copy Markdown
Contributor Author

@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
zvirja commented Jun 11, 2018
Copy link
Copy Markdown

@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 subscribe to this conversation on GitHub. Already have an account? Sign in.

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

0