Ensure parameters with 'in' modifier cannot be modified

[zvirja]

Closes #378

In this PR I added tests to ensure that in parameters indeed cannot be modified. I started to depend on a new version of Castle.Core which guarantees the in parameters immutability. Also I improved our delegate generator to emit all the necessary parameter attributes, so later Castle.Core can recognize that delegate parameter is read-only.

@dtchepak @alexandrnikitin Please take a look.

[zvirja]

@stakx Here is the question to you as an author of this improvement. Currently I'm defining a dynamic interface to make a delegate proxy later. As a part of this work I need to define an internal IsReadOnly attribute type inside the dynamic assembly to later mark the in parameters with it (see DefineIsReadOnlyAttributeIfNeeded() method around). I would like to somehow guarantee that attribute type is created only once. Otherwise, code fails with the following error and it's cumbersome to write some logic on top of this API:

System.ArgumentException : Duplicate type name within an assembly.
   at System.Reflection.Emit.ModuleBuilder.CheckTypeNameConflict(String strTypeName, Type enclosingType)
   at System.Reflection.Emit.AssemblyBuilderData.CheckTypeNameConflict(String strTypeName, TypeBuilder enclosingType)
   at System.Reflection.Emit.TypeBuilder.Init(String fullname, TypeAttributes attr, Type parent, Type[] interfaces, ModuleBuilder module, PackingSize iPackingSize, Int32 iTypeSize, TypeBuilder enclosingType)
   at System.Reflection.Emit.ModuleBuilder.DefineType(String name, TypeAttributes attr, Type parent)
...

Given that you deprecated this lock, do you see any other ways to have such a guarantee?

Thanks for the help in advance! 👍

[stakx]

@zvirja, thanks for bringing this to my attention. You're right that ModuleBuilder's API now doesn't offer a good way to do thread-safe type creation now. The only possibility that I can see is ensuring in your library that your ProxyGenerator instance won't get used at all while you're generating that attribute type... i.e. having a separate lock of your own.

I've opened castleproject/Core#399 to see what can be done at DynamicProxy's end. Could you please keep an eye on that issue? If a thread-safe alternative for ModuleScope.DefineType is going to be added, it would be good to ensure that the API is suitable for downstream libraries. 😉

[zvirja]

The only possibility that I can see is ensuring in your library that your ProxyGenerator instance won't get used at all while you're generating that attribute type... i.e. having a separate lock of your own.

Unfortunately, it's very unsafe to proceed with such approach. It could happen that in future versions of Castle.Core it will also start to define the attribute. In this case there is a possibility of the race condition between my code and Castle.Core. Rather, we should use the same shared synchronization and play safe.

I've opened castleproject/Core#399 to see what can be done at DynamicProxy's end.

Thanks 👍 Let's try to proceed with that one and see how it goes.

[stakx]

It could happen that in future versions of Castle.Core it will also start to define the attribute.

Actually, if that were to happen, then DynamicProxy would itself have to check whether that type already exists before creating it itself, just assuming that the type doesn't exist would be naïve since ModuleBuilder.DefineType opens up that possibility. So I don't think you'd have a problem there.

You could also just define that attribute type way in advance & in reserve, right after newing up your ProxyGenerator instance. That way the rest of your code could assume that the attribute is already there. You wouldn't even need a Lazy<Type> as I suggested over in the Castle issue.

[zvirja]

Actually, if that were to happen, then DynamicProxy would itself have to check whether that type already exists before creating it itself, just assuming that the type doesn't exist would be naïve since ModuleBuilder.DefineType opens up that possibility. So I don't think you'd have a problem there.

Well, the problem is that we want to guarantee compatibility with the future versions of Caslte.Core. Later Castle.Core might decide to add some sync, but our library will not use it (obviously), as at the compilation time that API didn't exist. Therefore, to guarantee compatibility with future versions of Castle.Core we need to rely on strong API and contracts.

You could also just define that attribute type way in advance & in reserve, right after newing up your ProxyGenerator instance. That way the rest of your code could assume that the attribute is already there. You wouldn't even need a Lazy as I suggested over in the Castle issue.

I don't like idea to generate type beforehand, as feature might be never required 😕

---

Added the "WIP" mark to indicate that it's in progress. Let's see how castleproject/Core#399 goes.

[stakx]

I don't like idea to generate type beforehand, as feature might be never required 😕

Agreed, neither would I. I just wanted to point it out as a possibility that you could implement today without any danger of future collisions.

[dtchepak]

This code is really nicely documented. 👍 (code comments sometimes get a lot of hate, so I wanted to give good credit where it is due 😂 )