Reduce public API surface

[stakx]

As noted in #193 (comment):

Castle library in general expose way too much publicly. Back in the old days of Castle especially with MonoRail everything was public by default and you'd be able to poke or extend things wherever you chose, pros and cons obviously.

I do however think we should start closing these things that are public [...].

The goal of this issue is to let us keep track of which types and type members should be removed from Core's publicly visible API in the next major version (and be marked as [Obsolete] even before that).

I suggest that we use posts for discussion and keep an up-to-date list of all decisions here at the top. If you have a better idea or format for this issue, please share!

Should be [Obsolete] in vNextMinor / removed from vNextMajor's public API:

(A check mark means that a member is already marked [Obsolete] in the master branch.)

• Castle.Core.Internal.Lock (class)
• Castle.Core.Internal.ILockHolder (interface)
• Castle.Core.Internal.IUpgradeableLockHolder (interface)
• Castle.DynamicProxy.Generators.BaseProxyGenerator.AddToCache (method)
• Castle.DynamicProxy.Generators.BaseProxyGenerator.GetFromCache (method)
• Castle.DynamicProxy.Generators.CacheKey (class)
• Castle.DynamicProxy.Generators.DelegateProxyGenerationHook (type)
• Castle.DynamicProxy.Generators.DelegateProxyGenerator (type)
• Castle.DynamicProxy.Generators.Emitters.ArgumentsUtil.IsAnyByRef(ParameterInfo[])
• Castle.DynamicProxy.IChangeProxyTarget.ChangeProxyTarget(object)
• Castle.DynamicProxy.Internal.InternalsUtil
• Castle.DynamicProxy.Internal.InternalsUtil.IsAccessible(this MethodBase)
• Castle.DynamicProxy.Internal.InternalsUtil.IsInternal(this MethodBase)
• Castle.DynamicProxy.Internal.InternalsUtil.IsInternalToDynamicProxy(this Assembly)
• Castle.DynamicProxy.ModuleScope.DefineType (method)
• Castle.DynamicProxy.ModuleScope.GetFromCache (method)
• Castle.DynamicProxy.ModuleScope.Lock (property)
• Castle.DynamicProxy.ModuleScope.RegisterInCache (method)
• Castle.DynamicProxy.Serialization.CacheMappingsAttribute.GetDeserializedMappings (method)
• Castle.DynamicProxy.Serialization.CacheMappingsAttribute.ApplyTo (method)

[TimLovellSmith]

I'm in the mood for brainstorming.

How about, everything that is a conditional 'feature' of 'Core' be considered whether it is a candidate for moving out to a separate library. The list I'm thinking of is in the readme. But I cut out a few that aren't interesting... Adding some actual thoughts per interesting sounding 'feature':

-FEATURE_ASSEMBLYBUILDER_SAVE (only if you separated all of DynamicProxy into its own library?)

-FEATURE_BINDINGLIST (dictionary adapter cruft)
-FEATURE_LISTSORT (dictionary adapter cruft)
-FEATURE_DICTIONARYADAPTER_XML (dictionary adapter cruft)
-FEATURE_IDATAERRORINFO (dictionary adapter cruft)
-FEATURE_ISUPPORTINITIALIZE (dictionary adapter cruft)

-FEATURE_EVENTLOG (sounds like a natural separate standalone library)

-FEATURE_LEGACY_REFLECTION_API (sounds still required for supporting .net 4.0 and older)

-FEATURE_SECURITY_PERMISSIONS (I am not sure what this enables - but I never hear people talking about CAS and .net partial trust any more, and it sounds very hard to do properly, is it really being supported properly?)

-FEATURE_REMOTING (not practical to remove, breaking)
-FEATURE_SERIALIZATION (not practical to remove, breaking)

-FEATURE_SYSTEM_CONFIGURATION (all the config stuff could maybe be a separate library? I don't actually know if the rest of castle core depends on it etc...)

[TimLovellSmith]

Castle.DynamicProxy.Generators.CacheKey seems like it doesn't really need to be public...

Except that it is exposed in some protected APIs etc.

[stakx]

Candidates from #391: Added to list above.

[jonorossi]

We've got 10s of classes in Castle.DynamicProxy.Generators.* that shouldn't be public, we make breaking changes in those namespaces often over the last few years without really realising because this is the implementation detail of DP, so has no need to be in the public API.

We've also got a few *.Internal namespaces. Microsoft teams have been referring to this type of API as "pubternal", public API that weren't meant for public use but for use by another component, in Castle's case many of these internal namespaces were for Windsor. We need to remove pubternal APIs, either migrate utility code into Windsor or add a publicly supported API in a proper namespace. We obviously have CompositionInvocation and InheritanceInvocation in an Internal namespace which must stay public, so we'll need to add some XML docs to them to make clear what they are for.

[stakx]

Another thing we might want to consider removing from the public API is ModuleScope. (We've established previously that client code has no business mucking around with DP's dynamic assemblies.) This currently cannot be done because not all SaveAssembly method overloads are available via PersistentProxyBuilder. We might need to complete the SaveAssembly method group there.

[jonorossi]

Definitely something to consider. Regarding SaveAssembly, I think we need to think about the API including LoadAssemblyIntoCache to determine what we are trying to support, right now loading an assembly might work but not all features are supported so making some breaking changes to properly support pre-generated proxies is likely worthwhile.

I think we are both in agreement that it needs to be crystal clear to users what APIs we want them using, including INamingScope which was probably initially meant for initial use but I suspect there would be a few users out there working around some specific problem.

DP has too many entry points, this is one of the biggest reasons for me just making the required changes for bugs/CLR changes and nothing too drastic, but if we want this library to perform better (which I'd like to do more of) we are going to need to make a lot of breaking changes (and funnily most of these public APIs have no one using them).

[stakx]

Just as a heads-up, I'll likely work on this next. I'll focus just on DynamicProxy's API for now. I'll try to verify all public API changes against Windsor to ensure we don't need too many changes or rewrites over there; all arguments below are made under the condition that there would be a relatively easy way to keep Windsor going.

Regarding SaveAssembly, I think we need to think about the API including LoadAssemblyIntoCache to determine what we are trying to support, right now loading an assembly might work but not all features are supported so making some breaking changes to properly support pre-generated proxies is likely worthwhile.

I remember reading in some GitHub issue (cannot remember where exactly, I think it was in the CoreCLR repo) that loading a .NET assembly that was generated on a different platform/target isn't actually a supported scenario; I think the meaning of this was that if one saves a dynamic assembly on .NET 4.x, one shouldn't expect to be able to load it on .NET Core x or some other platform. (This could have to do with the plethora of "system" assemblies that are now out there, i.e. mscorlib vs. System.Private.CoreLib vs. System.Runtime vs. netstandard, which could get mixed up—but I'm not sure whether that's the main reason.) If we followed that argument, we should consider simply giving up LoadAssemblyIntoCache.

If we want to keep LoadAssemblyIntoCache (and I suppose we do), I'm leaning towards lifting it into DefaultProxyBuilder (IProxyBuilder, actually) and making ModuleScope completely internal. One argument for this is that we've already lifted its counterpart, SaveAssembly, into PersistentProxyBuilder, so both "save" and "load" method would then sit at the same level.

[...] including INamingScope which was probably initially meant for [internal] use but I suspect there would be a few users out there working around some specific problem.

I think INamingScope should become internal (if Windsor isn't making use of it). AFAIK replacing the default NamingScope logic can easily break things. If people use this to work around issues, wouldn't it be better to have those issues reported so we know what they are?

[stakx]

With regard to DynamicProxy, we're mostly done here. The last remaining bit would have been making ModuleScope internal; I've now closed that PR due to a lack of feedback – I realize that making ModuleScope internal and the changes that this entails may be a matter of taste. I'm happy to leave things as is.