Can we move `AttributesToAvoidReplicating` into DynamicProxy's main namespace?

[stakx]

AttributesToAvoidReplicating currently sits in the sub-namespace Castle.DynamicProxy.Generators. It's one of only two public types left in that namespace—the namespace contains mostly internals that became truly internal in #505.

My aim here is to concentrate DynamicProxy's public API in just one single namespace: Castle.DynamicProxy.

I therefore suggest that we do either of two things:

1. move AttributesToAvoidReplicating into the main namespace Castle.DynamicProxy; or
2. turn it into a collection property in ProxyGenerationOptions

I haven't thought about (2) too deeply, but (1) would be simple enough to do. Also, it's likely not a part of the public API that one uses a lot, so perhaps it's not worth investing too much time.

Any thoughts or opinions?

[jonorossi]

I think this class got used a bit more years back when DP didn't have a framework attribute built-in and the SecurityAttribute hierarchy wasn't handled (just the top level), and then all the mocking frameworks added exclusions themselves rather than logging a defect.

Copying from a 2018 comment of mine:

AttributesToAvoidReplicating was designed as a static class so for example a user of Moq could add another attribute without needing access to the ProxyGenerator instance used by Moq. I'm pretty sure it was added as a get out of jail card when you are in a pickle with some attribute causing you grief. (https://github.com/castleproject/Core/issues/334#issuecomment-360177220)

There will still be some use out there for things like ServiceContractAttribute and whatever else people have come up with, however I think if we are going to make a breaking change I'd prefer to move the 4 hardcoded attributes into AttributeUtil together with the opposite ParamArrayAttribute, and add a collection to ProxyGenerationOptions. I agree that it belongs on PGO not on the ProxyGenerator like some have suggested in GitHub issues over time.

Using PGO means that it will then be up to mocking libraries to expose their own API to exclude attributes which is a cleaner API and the way we've gone with a few other APIs over the last few years.

[stakx]

I think this class got used a bit more years back [...]

That made me curious. Is anyone using AttributesToAvoidReplicating at all these days? Obviously I won't be able to prove that it's not needed anymore, but a search across GitHub doesn't bring up a lot of relevant-looking results.

First, none of the projects that we list on our introductory DynamicProxy documentation page appear to be using AttributesToAvoidReplicating:

• Castle Windsor: search results in castleproject/Windsor
• Moq: search results in moq/moq4 (just removed in https://github.com/moq/moq4/pull/1029)
• NSubstitute: search results in NSubstitute/NSubstitute
• FakeItEasy: search results in FakeItEasy/FakeItEasy
• Rhino Mocks: search results in hibernating-rhinos/rhino-mocks
• NHibernate: irrelevant, since newer versions no longer use DynamicProxy
• Entity Framework Core: search results in dotnet/efcore

Most projects that bring up a search result across all of GitHub appear to derive from a few repositories; most projects add UIPermissionAttribute, EnvironmentPermissionAttribute, and the like. Which, today, would no longer be necessary due to SecurityAttribute being registered by default.

The only other attributes sometimes added are (like you mentioned:) WCF ServiceContractAttribute, OperationContractAttribute, etc. We are missing those in DynamicProxy.

I have absolutely no problem keeping AttributesToAvoidReplicating, and am likely going to submit a PR soon to move it into PGO... but I do wonder whether it's worth the trouble at all, or whether we might just want to remove it from the public API, and only add it back when someone needs it?

[jonorossi]

but I do wonder whether it's worth the trouble at all, or whether we might just want to remove it from the public API, and only add it back when someone needs it?

Castle has always been pretty flexible and way too open with the public API surface, and even though we are removing heaps of the public API, I think since this is a pretty trivial amount of code that we should keep the feature rather than remove but mostly because this is going to be used more by people's private projects rather than library code.

[stakx]

Perhaps true. :+1:

I've been prototyping ProxyGenerationOptions.AttributesToAvoidReplicating last night. The fact that we hardwire certain framework attributes complicates the code quite a bit; we need to model a special collection that always .Contains them even when not explicitly .Add-ed. When enumerating, you need to concatenate the hardwired attributes with those explicitly added. Alternatively, adding the hardwired attributes upfront to a regular collection is easier but more wasteful memory-wise, as each PGO instance will have its own copy of this prepopulated collection.

It occurred to me that perhaps AttributesToAvoidReplicating doesn't need to be a collection at all; what DynamicProxy really needs in the end is some bool ShouldAvoid(Type attribute) method, so why not go with a hook approach?

partial class ProxyGenerationOptions
{
    public IAttributeReplicationHook AttributeHook { get; set; } =
        StandardAttributeReplicationHook.Instance;
}

public interface IAttributeReplicationHook
{
    bool ShouldReplicateAttribute(Type attributeType, MemberInfo fromMember, Type proxiedType);
}

public class StandardAttributeReplicationHook : IAttributeReplicationHook
{
    public static readonly StandardAttributeReplicationHook Instance =
        new StandardAttributeReplicationHook();

    protected AttributeReplicationHook() { }

    public virtual bool ShouldReproduceAttribute(...)
    {
        return attributeType.IsSubclassOf(typeof(SecurityAttribute)) == false
            && attributeType != typeof(ComImportAttribute)
            && attributeType != typeof(MarshalAsAttribute)
            && attributeType != typeof(TypeIdentifierAttribute);
    }
}

Pros:

• immutability
• quite possibly much easier to implement
• might be more extensible (e.g. we could move PGO's AdditionalAttributes to the hook interface as well)

Cons:

• bigger API change than strictly necessary
• No more Contains method (though TBH I never quite saw the use case for that).

This is just an idea, and I'm perfectly happy to keep the present API mostly unchanged (except for the move to PGO)—we don't need to change everything around in v5. I thought I'd mention it anyway in case you think it's worth following up on.

[jonorossi]

I was actually thinking similar, I looked at how we could add it to IProxyGenerationHook without it being a breaking change but couldn't come up with a way.

It won't be immutable because the hook object can be mutable or even non-deterministic.

I don't think Contains was desired, the Add method should handle being a set.

Not really sure which way to go TBH.

[stakx]

We have some time to ponder this, I found that the main difficulty lies in making the proxy generation options available to the static AttributeUtil class where the replication decision is made; options has to be passed down a whole new part of the call graph. I'm busy finding a reasonably tidy way to do it.

Re: breaking change, extending IProxyGenerationHook would be a breaking change. We could do the COM approach and derive a IProxyGenerationHookEx/2 and put the attribute handling methods there, but as with COM, discoverability will suffer somewhat, because by looking at ProxyGenerationOptions.Hook you won't find out about that interface. This might be tolerable since we're not discussing a core usage scenario here. Otherwise, the only remaining options are to add a new property to PGO (next to AdditionalAttributes), or keep AttributesToAvoidReplicating static but in a different namespace than before.

I tend towards adding a new property to PGO, but I'm open to anything really.

[stakx]

@jonorossi, I'll start with the TL;DR: I suggest doing a PR that changes the namespace of the existing AttributesToAvoidReplicating class from Castle.DynamicProxy.Generators to Castle.DynamicProxy (so that the former disappears from the public surface). If you're not OK with that, I'd also be fine with closing this issue without taking any further action.

Below I'll try to summarise my thoughts that led to the above suggestion.

I tend towards adding a new property to PGO, but I'm open to anything really.

This looked like the ideal solution to me. Unfortunately, replacing a static class AttributesToAvoidReplicating with an instance property on ProxyGenerationOptions entails having to pass around either options or options.AttributesToAvoidReplicating to just about every single generator, contributor, collector, emitter, etc. so it can be forwarded to AttributeUtil where it's needed. This doesn't seem so great to me, as having that additional parameter everywhere makes it much harder to spot where it is actually needed (vs. just being propagated).

Since there is already some passing-around of other entities going on, I considered piggybacking on that by bundling all the things being frequently passed around (e.g. options, hook, logger, attributes to avoid replicaing, etc.) into a some ProxyGenerationContext. But that doesn't fundamentally solve the problem of excessive parameter passing.

Such a context object could be made globally available via a [ThreadLocal] static property. This seems too brittle to me as it would only work as long as DynamicProxy doesn't use multi-threading or the TPL internally.

My preliminary conclusion is that changing anything fundamental about AttributesToAvoidReplicating will probably be more work than it's worth. I still think it should be moved into the main Castle.DynamicProxy namespace to reduce the overall public surface of DynamicProxy by one extra namespace, but this is mostly cosmetics.

[jonorossi]

It doesn't appear it has been a problem in the past with this static class applying to the whole runtime, however a unit test project (or even a mocking library) could add an attribute which changes what the SUT actually does because this is static and the mocking libraries no longer ilmerge DP into their assembly. I'd still like to see us remove this static.

I had a look through the usage of AttributesToAvoidReplicating then to the two AttributeUtil.GetNonInheritableAttributes overloads, and found the contributors and collector using it. We've got NonInheritableAttributesContributor which handles adding extra attributes to the type, but members are handled separately. I also noticed we've got some passing around of IProxyGenerationHook which is a member of PGO, so why not just pass the PGO.

Do you still have the code where you attempted to add a new collection property to PGO? I'd much prefer we went this way. I'd hardcode the 4 runtime attributes as we already do for ParamArrayAttribute so the collection is always empty unless you add one, that way we don't add yet another non-deterministic field to PGO making caching harder.

[stakx]

@jonorossi, I didn't keep my original code but I recreated a version similar to it. See the PR referenced above this post.

[stakx]

Shall we close this issue? It's probably not worth holding back the next release just because of this.