Prevent interceptors from being able to modify in parameters

[stakx]

This should fix #369.

[zvirja]

Good work, thanks! 👍
Left a few minor comments to make the code even better 😉

[zvirja]

Create a local method for this - it will allow compiler to cache the value better (only once, rather than per each invocation).

[stakx]

I've run a few experiments to see the compiler automatically cache instantiated delegates, but I haven't been able to observe such behavior (neither in a debug nor release builds)...?

The compiler will quite literally do what it's told, so the important thing is to take the hidden delegate instantiation out of the loop... it's a quick and easy win. Writing the lambda in-place instead of in a local function or static method produces less line noise. IMO we can suffer that one delegate instantiation per invocation—if not, we'd possibly have to refactor a lot of other code in DynamicProxy!

[zvirja]

Playing with it more I found this issue: dotnet/roslyn#19431. That's the reason whey are not currently cached. If you pass anonymous non-capturing lambda and target e.g. net462 you'll see the caching in action:

foreach (var parameter in parameters)
{
  if (parameter.attributes.Any(t => t.FullName == "EEEE"))
  {
    // Do smth
  }
}

Compiled:

foreach (System.ValueTuple<string, Type[]> parameter in parameters)
{
    if (!parameter.Item2.Any(<>c.<>9__0_0 ?? (<>c.<>9__0_0 = <>c.<>9.<CycleWithLocalMethod>b__0_0)))
        continue;
}

Later they might enable the similar optimization for the local functions - that's why I suggested it.
Also it looks more natural with local methods. But as this a matter of tastes - it's up to you 😉

[zvirja]

As to me, this method started to have too many responsibilities. Better create a standalone method like ArgumentsUtil.IsReadOnlyByRefArg() or similar and put all the detection logic there.

Also it might make sense to create ArgumentsUtil.IsAnyWritableByRef or similar and use it instead of ArgumentsUtil.IsAnyByRef, so we don't generate e.g. redundant try/catch when we are not going to write something.

[stakx]

Given that the method is called CopyOutAndRefParameters, I think the additional 6 lines of code are still in scope.

That said, I do agree that it could be re-written to make better use of small helper methods to raise the level of abstraction.

As you already noted, AttributeUtil might be a good place to have additional helpers similar to IsAnyByRef. But once we start adding methods there, we also have to think about accessibility for consistency's sake. AttributeUtil.IsAnyByRef is public which I think was a mistake. So either we expand Core's public API further by adding new public methods, or we add non-public methods and are therefore inconsistent (albeit for good, practical reasons), or we add method(s) in a different place, which can also be seen as inconsistent.

This is a design problem I'd rather not solve today. May I leave it up to you to submit a PR that cleans this method up a little?

[zvirja]

Well, if you don't want to produce too much noise, then I'd suggest to simply extract all the IsReadOnlyRef logic to a static method in the same class. Each method should have single responsibility only, while currently CopyOutAndRefParameters method orchestrates parameters handling and knows the particularity of how in modifier is implemented.

[zvirja]

We are mixing facade (or integration) tests here and our expectations about compiler. I'd remove all the tests about expectations, as:

• it's unclear what to do when they start to fail;
• facade tests will start to fail anyway, so these are redundant.

[stakx]

facade tests will start to fail anyway, so these are redundant.

Their added value lies in giving us an additional, very direct hint at why the unit tests might be failing. I've done that kind of testing in a few other places recently.

@jonorossi has previously been in favor of these kinds of tests, so I'd like to leave it up to him whether to remove these tests or not.

it's unclear what to do when they start to fail

Isn't that true for most tests?

It would be magical if unit tests instructed us precisely how to fix them once they break (for whatever reason). I'd be interested to learn how to write such tests, can you point me to any resources?

[jonorossi]

I'm definitely in favour of keeping these additional tests that are sort of like proofs, if it happens the implementation of the runtime changes or we misunderstood how it works we'll know straight away. They have heaps of value for DynamicProxy where they wouldn't in most libraries because we are so low in the stack and make so many assumptions about implementation details that a lot of the time aren't documented in the ECMA specifications.

The only thing that would be good is a "heading-like" comment like you've done in the others to separate them from the actual unit tests.

[stakx]

Sure, I'll add such a comment! Done.

[stakx]

@zvirja, thanks for your review. You know what, I think you were right. I've rewritten the method to make use of local functions, and I've marked ArgumentsUtil.IsAnyByRef as [Obsolete]. Things look a little cleaner now (but @jonorossi, I can always revert if you'd prefer the previous closer-to-the-bare-metal approach).

[jonorossi]

I thought these classes were public classes, I assume they aren't visible in .NET Standard 1.3? Read below, wow! 🥇

[jonorossi]

This class should probably sit alongside Castle.DynamicProxy.Tests.Interceptors.SetReturnValueInterceptor, and also accept an index.

[stakx]

Done.

[jonorossi]

Way too much of the DP API is marked public including this class. Probably worthwhile writing Castle.DynamicProxy.Generators.Emitters.ArgumentsUtil.IsAnyByRef otherwise someone might be confused with one of the classes people might actually use.

[stakx]

Done.

[jonorossi]

Maybe it is just me but 3 levels of nested methods makes CopyOutAndRefParameters appear quite complicated when in actual fact it isn't?

[stakx]

@jonorossi: 😆, I almost thought that might be your reaction!

The idea here is to make the scope of helper functions as local as possible, so that it's clear where they are actually needed. Contrast this with a "global" helper function like ArgumentsUtil.IsAnyByRef which, as it turned out, was only used in one single location—but you couldn't tell by taking a casual glance at its source.

Options:

1. Leave it as it is now (nested local functions).
2. Bring IsIsReadOnlyAttribute to the same level as the two other local functions to reduce nesting.
3. Turn the three local functions into private static methods in the same class (GeneratorUtil).
4. Revert to how things were before (i.e. don't have helper functions/methods at all).

What's your preference?

[jonorossi]

Let's just leave it as is, I didn't really have a preference other than being a little surprised. I think if CopyOutAndRefParameters was non-void it would look strange because you'd have to try to match up returns.

[jonorossi]

Some minor comments, looks great.

[jonorossi]

@stakx All good. I'll let you do the honours of merging it, please squish it either manually or via the GitHub UI.

[stakx]

@jonorossi - thanks for entrusting me with the reigns at this point. ❤️ Unfortunately, Core's master branch is currently read-only to me. (Ironically, I do have write access on other Castle projects, where I would hardly ever make use of it. 🤔)

[jonorossi]

Unfortunately, Core's master branch is currently read-only to me

Fixed. Castle Core and Windsor have "protected" master branches from when there were other people working on a single .NET Core branch that weren't committers.

[zvirja]

Hi @stakx,

I'm trying to consume this change in NSubstitute and have complications with the way we perform the check.
Everything works fine for the interface proxy. However, to support delegate proxy NSubstitute uses an approach similar to Moq:

1. Generate dynamic interface with a single Invoke method, matching the delegate's signature.
2. Obtain proxy for that interface from Castle.
3. Create delegate from proxy's Invoke method.

The complication is that for .NET Framework the compiler generates the IsReadOnly attribute as an internal type in the module where the delegate is defined. Therefore, when I'm generating an interface (point 1 from above), I cannot simply re-use the existing IsReadOnly attribute - it's internal and is not visible by the dynamic assembly. As result, the interface's Invoke method doesn't contain the IsReadOnly attribute and later (at point 2) Castle doesn't make the method read-only.

You'll see the same issue with Moq if you decide to guarantee that mock for delegate with in modifier is indeed read-only.

I see a way to fix that (generate IsReadOnly attribute in dynamic assembly), however it looks overcomplicated. Therefore, I wanted to clarify your initial motivation for having the check like this:

Core/src/Castle.Core/DynamicProxy/Generators/GeneratorUtil.cs

Lines 77 to 86 in 1ba6c89

	if ((parameter.Attributes & (ParameterAttributes.In | ParameterAttributes.Out)) == ParameterAttributes.In)
	{
	// Here we perform the actual check. We don't rely on cmods because support
	// for them is at current too unreliable in general, and because we wouldn't
	// be saving much time anyway.
	if (parameter.GetCustomAttributes(false).Any(IsIsReadOnlyAttribute))
	{
	return true;
	}
	}

Why can't we simply check for the In required modifier here? It's fully supported by .NET Standard 1.5+, so it looks like a reliable way to do that. According to this description:

In addition, if the method is abstract or virtual, then the signature of such parameters (and only such parameters) must have modreq[System.Runtime.CompilerServices.IsReadOnlyAttribute].

modifier is always available for the virtual and abstract methods which we are dealing with only. (BTW, it looks like a typo in spec, as they use System.Runtime.InteropServices.InAttribute modifier type).

I believe we shouldn't worry about .NET Standard 1.3 as in modifiers are not supported there neither. This way it will be much simpler to make everything work, as In modifier type is already a part of .NET Standard.

Thanks. Sorry for not testing this before the release - I was almost confident that everything must work fine... 😖

P.S. Just tested the check like this:

#if FEATURE_EMIT_CUSTOMMODIFIERS
if (parameter.GetRequiredCustomModifiers().Any(mod => mod == typeof(InAttribute)))
{
    return true;
}
#endif

Works like a charm for all .NET Framework and .NET Core versions.

[stakx]

@zvirja:

Thank you for following up on this.

I should have realised in time that the compiler-generated System.Runtime.CompilerServices.IsReadOnlyAttribute shim is non-public and thus often non-copyable by DynamicProxy. So we have the situation that DynamicProxy recognizes C# in parameters by the presence of that custom attribute, but may generate proxy types that fail the very same test.

What a pity I didn't realise this earlier. To be extra-super-correct, DynamicProxy would have to follow the compilers' lead and emit the IsReadOnlyAttribute into the dynamic assembly on the fly if it doesn't already exist in an accessible location. Alas, let's wait and see if anyone else actually needs / runs into this. Right now, I'm not too keen on implementing this to be honest. (Sorry, I had the wrong use case in mind. Ignore.)

Everything works fine for the interface proxy.

I take it that this is because you're using a in parameter detection logic based on custom modifiers (cmods) instead of the custom attribute? (As above.)

Why can't we simply check for the In required modifier here?

Core/src/Castle.Core/DynamicProxy/Generators/GeneratorUtil.cs

Lines 70 to 73 in 1ba6c89

	// * A required custom modifier `System.Runtime.InteropServices.InAttribute`
	// is always present in those cases relevant for DynamicProxy (proxyable methods),
	// but not all targeted platforms support reading custom modifiers. Also,
	// support for cmods is generally flaky (at this time of writing, mid-2018).

Like you're saying, we could do this. However, I've spent quite a bit of time with cmods in the past few months, and it wasn't super enjoyable. None of the major runtimes (Mono, .NET Framework, .NET Core) support cmods very well when it comes to Reflection and Reflection.Emit. I think it entirely possible that yet more cmod-related bugs in these facilities will come to the fore as C# continues to rely on them more and more. Hence I figured it would be best to avoid them if possible, and decided against relying on cmods for the detection logic.

Add to this the fact that the primary characteristic of C# in parameters is the "duck-typed" custom attribute anyway, so it was a fairly easy decision to rely on that instead.

I guess we could have a detection logic that tests for either cmods and custom attributes (or both). I considered this, but decided against it as a trade-off because checking two things takes more time than one thing. Given that we have a fast-guard check for ParameterAttributes.In, we could probably afford to do an additional check for the cmod.

But for the time being at least, you'll have to emit a IsReadOnlyAttribute yourself (it needs to be visible to DynamicProxy) and add it to the Invoke parameters that have the InAttribute cmod.

BTW, it looks like a typo in spec, as they use System.Runtime.InteropServices.InAttribute modifier type

(Agree! I've taken the liberty to ask over at dotnet/csharplang whether this is a typo or not. Let's see what they say.)

[zvirja]

None of the major runtimes (Mono, .NET Framework, .NET Core) support cmods very well when it comes to Reflection and Reflection.Emit.

Could you please be more specific here? What exact issues did you see? I'm just curios, as I tested a few versions of both .NET Framework and .NET Core and it worked well there. Probably, not the generics (as you already reported), but apart from that it worked fine.

Given that we have a fast-guard check for ParameterAttributes.In, we could probably afford to do an additional check for the cmod.

Looks like a great trade-off! If method has in modifier, we could test for either custom modifier or custom attribute. If any of them is available - we consider parameter as a read-only.

It will add an additional robustness to the detection logic and support non-perfect code-gen scenarios. And would also simplify the NSubstitute code as it will be enough to emit custom modifier only 😊 I could raise a PR for that if you wish.

(Agree! I've taken the liberty to ask over at dotnet/csharplang whether this is a typo or not.

Thanks for registering that issue 👍 Likely I should be that proactive to do it by myself... 😖

[stakx]

None of the major runtimes [...] support cmods very well [...]

Could you please be more specific here? [...] Probably, not the generics (as you already reported) [...]

Fair question. I should be more specific, but in truth, I cannot. I guess it comes down to being a trust issue for me. Consider:

Custom modifiers have historically been used very little outside C++/CLI, whose community was relatively small (compared to e.g. that of C#). Add to this that the typical use case for C++/CLI was to be a fast and easy interop bridge between managed and unmanaged/native code, not to do heavy-duty reflection stuff like DynamicProxy does. So if there are in fact cmod-related bugs in .NET's managed Reflection API, that would likely have gone unnoticed by most people for the longest time. Not even Microsoft's compiler team might have noticed since the compilers were native programs using the CLR's native APIs.

C# has started to use cmods for its own purposes only quite recently, and almost right away we walk into bugs/limitations with Reflection. (You can look up at some of the work I did in DynamicProxy, or some of the issues I raised over at the Mono, CoreCLR, and CoreFX repos. The CoreCLR issues are also representative for the CLR.) That's not exactly reassuring. Who knows what else we might discover.

On the other hand, custom attributes have long been used by the C# community, so they are much more "battle-tested". This is why I trust in them a lot more than I trust in Reflection's ability to handle cmods properly.

It doesn't help that one of the code owners of Reflection / Reflection.Emit admits (here) that Reflection has fundamental limitations (even though they might not matter much in practice) when it comes to cmods:

This is a long standing defect in how Reflection thinks about custom modifiers and it's not likely you could address this [...]

That all being said, you're of course right that the very scenario we're looking at doesn't appear to be affected.

I'll let you judge for yourself whether it's right to be cautious about cmods or not.

Just tested the check like this: [...]

Have you checked on Mono, as well?

Looks like a great trade-off! If method has in modifier [sic], we could test for either custom modifier or custom attribute. If any of them is available - we consider parameter as a read-only.

(I guess you meant "if the parameter has the ParameterAttributes.In flag set, we could...")

Yes, precisely.

I could raise a PR for that if you wish.

Sounds great! I feel a little saturated with cmod-related work right now, so I'd be totally happy to defer to your initiative!

One suggestion, though: It would be good to make sure that the faster of the two checks is made first. I'm suspecting that the modreq check will generally outperform a custom attributes-based check (but it would be awesome if you could actually benchmark this using a variety of different scenarios):

• Parameters are generally likely to have less cmods than custom attributes, so parameter.GetRequiredCustomModifiers() should return an array that tends to be smaller (giving us less things to search through) than parameter.GetCustomAttributes(false).

• parameter.GetCustomAttributes(false) requires us to look for an object of a type which we compare by name. parameter.GetRequiredCustomModifiers() OTOH returns Type[], and assuming that Roslyn actually encodes the right type as modreq (we need to wait for confirmation of this!), it's one that's always available in the BCL, so we can compare using simple .Equals instead of by name.

[zvirja]

@stakx Thanks for the verbose reply! 😉

This is why I trust in them a lot more than I trust in Reflection's ability to handle cmods properly.

I see your point now. Likely you are right that cmods support is quite limited and indeed your possibilities are quite limited. However, probably, for our case it doesn't matter.

In any case if we tests for both modifiers and attributes, than I suppose we play safely 😉

Sounds great! I feel a little saturated with cmod-related work right now, so I'd be totally happy to defer to your initiative!

Sure. Will do after this one is closed.

One suggestion, though: It would be good to make sure that the faster of the two checks is made first.

Sure :)

---

In the meanwhile I've adjusted the NSubstitute to generate the IsReadOnly attribute in runtime if needed. If you could take a look at nsubstitute/NSubstitute#420 it would be awesome (code is not very specific to NSubstitute there) as you might want to do the same in Moq.