Remove obsolete or redundant conditional compilation symbols

[stakx]

PR #495 removed approx. half of all conditional compilation symbols, but a few are left. This issue is a follow-up to @jonorossi comment in said PR:

After this is merged we can talk through these, more definitely need to go.

As to how more of those symbols can be removed, I would need to take a closer look. Right now, I'm assuming the following:

Conditional compilation symbol	Remarks
FEATURE_SERIALIZATION	Some serialization support was reintroduced in .NET Standard 2.0, e.g. all targeted platforms now support the [Serializable] attribute... but the binary serializer can't deal with e.g. Type nor delegates, so we can't easily enable this on .NET Core). But: This symbol is closely related to FEATURE_APPDOMAIN & FEATURE_ASSEMBLYBUILDER_SAVE, perhaps we could merge those into one.
FEATURE_REMOTING	We can remove this iff we decide to give up support for remoting / IsTransparentProxy checks / MarshalByRefObject. (Is this something that's still used these days? For which types? I don't know.)
FEATURE_EVENTLOG	We can remove this iff we deprecate Castle.Core.Logging.DiagnosticsLogger, which logs to Windows' event log.
FEATURE_SYSTEM_CONFIGURATION	Tied to the facility in the Castle.Core.Resource namespace, which we could deprecate/remove.
FEATURE_TEST_PEVERIFY	Needed for now, but I'm wondering whether we could offload the location of a PEVerify binary into the build scripts / MSBuild files. It might be neat to set an environment variable PEVERIFY to the PEVerify binary's location. (Setting an environment variable in an MSBuild project file might be non-trivial, but perhaps it can be done.)
FEATURE_TEST_COM	.NET Core has some nominal COM support. It may not support functionality tied to CoCreateInstance, but AFAIK it does support COM interop and the COM attributes. So it may be possible to reduce usage of this attribute.

[jonorossi]

• FEATURE_SERIALIZATION: not sure, I'd have to look into it.
• FEATURE_REMOTING: I can't think of a good reason anyone would want do this today, there could be old code out there using MarshalByRefObject, but this is definitely one of those things that isn't moving forward with .NET 5.
• FEATURE_EVENTLOG: Need to get back to you on Future of Castle DictionaryAdapter #394.
• FEATURE_SYSTEM_CONFIGURATION: Probably need to review how Windsor uses the whole resource thing, and maybe we move those classes into Castle.Windsor.dll.
• FEATURE_TEST_PEVERIFY: runtime check rather than conditional compilation sounds like a good idea, or maybe we can solely move to ILVerify (Run ILVerify on unit tests #506).
• FEATURE_TEST_COM: Interested to see what you come up with.

[stakx]

@jonorossi,

• FEATURE_APPDOMAIN and FEATURE_REMOTING — see the two small PRs that I just submitted.

• FEATURE_TEST_COM — snip (please ignore my lightweight COM pipe dreams that I previously elaborated here) — it seems that all but a single COM test can run even on Linux. For that one test we'll need a replacement for NUnit's [Platform] attribute which isn't available when targeting anything other than netXY. I'll look into that next.

• FEATURE_SERIALIZATION — this might get messy. .NET Core supports binary serialization, but its BinaryFormatter cannot deal with all the types that the CLR version supports (see this list of serializable types on Microsoft Docs). In some cases, we can simply drop the FEATURE_SERIALIZATION guards, in others we still need it (e.g. when a type contains a Type field). I'll happily submit a PR to remove a large number of unnecessary guards, but I don't yet know what to do about the remaining cases.

[jonorossi]

WRT binary serialisation, you can serialise a proxy so it serialises the target, however is there anything else we are supporting (ignoring the serialisation code there for the save/load assembly dance). Serialising a proxy isn't something people have asked for on .NET Core likely because no one is writing new code using binary serialisation.

I'm hesitant to add the feature to the .NET Standard build just because we could sort of do it. I think we have to work out which conditional compilation could be changed to FEATURE_ASSEMBLYBUILDER_SAVE and what can go in all builds, for example there is some code that adds XmlIgnoreAttribute to the proxy's hidden fields using the FEATURE_SERIALIZATION symbol which we could probably put in all builds.

[stakx]

Let me see if I can get this straight. FEATURE_SERIALIZATION is a hodge-podge of at least 4 distinct concerns which we possibly need to untangle:

1. guards types that need to be serializable because they go in the LoadAssemblyIntoCache cache mappings blob (e.g. CacheKey, ProxyGenerationOptions, proxy generation hook types, or interceptor selector types) — i.e. types that support the load/save assembly scenario

2. guards code that's responsible for making generated proxy types serializable

3. guards code that's responsible for making generated invocation types serializable

4. guards exception types because MSFT says those should be serializable (so they can be remoted & cross app domain boundaries)

and with both (2) and (3), there's not just binary serialization to consider, but also XML serialization (and while we're at it, these days, we should at least mention DataContractSerializer and the very new System.Text.Json serializer).

With (1), I'm mostly concerned about serializability of ProxyGenerationOptions due to the potential presence of unserializable objects in the object graph—e.g. delegate mixins. Would we still mark PGO as serializable even when serializability cannot be guaranteed?

I'm not sure I fully grasp the importance of (2) and (3). I guess people might use mocking libraries to easily generate objects, then expect those to format just as nicely as the mocked/proxied base type for transmission across a wire... but would anyone want to deserialize a proxy or invocation? Not sure whether that's an important scenario but I tend towards "not important"; don't use proxies as DTOs.

(4) should be easy, at least.

[jonorossi]

Great summary, and I too am not sure how much of everything is important.

(1) We've always had problems with serialization of types involved in the pre-generated assembly process, you can always have an interceptor, interceptor selector, mixin, etc that just can't be serialized properly, but no one has reported any defects with this in the last decade I can recall, so either so few people use this feature, people only use it for simple interceptors, or people worked out their problem and fixed their code to be serializable. My guess is it is mostly number 1, this feature has extremely limited use with partial trust web hosting a non-existent thing today, security is done by the OS not the runtime, not sure we can really offer this feature reliably anyway, too high of a risk of unbounded object graphs. We could consider dropping this and adding something better back in the future that was actually reliable if/when the need arises. What would we do for unit tests though.

(2) and (3) Maybe instead of DP doing this since we obviously don't support serialization for every possible different library someone could want to use (we've never had JsonIgnoreAttribute) we remove support and add a documentation page on how people can set that up for themselves with whatever serialization library they want to use? Maybe we don't even need that since most serialization libraries only serialize public properties, and we could just add NonSerializedAttribute for binary serialization. I do agree that this all seems a bit pointless because what is the use case, I do wonder if we just drop this support as a breaking change since we are having a new major version and add it back if someone presents a useful scenario which we can then document.

(4) There is probably little harm in just following the .NET Framework coding guidelines for making all exceptions serializable, but do you know if this is still the guidelines for .NET Core? I see for example FileNotFoundException in .NET Core implements the extra constructor and GetObjectData. It looks like all our exceptions except InvalidProxyConstructorArgumentsException are right.

[stakx]

I didn't quite dare suggest it myself, but I'm also favor of removing serializability for (1), (2), and (3) and only gradually add back the bits people really need. Could Windsor possibly be a roadblock here?

When you mention unit tests, I assume you're thinking of being able to run PEVerify? I could be wrong, but that ought to still work even without serializability. SaveAssembly isn't in danger, it's LoadAssemblyIntoCache that would go away from the public API. I agree we'd want to keep SaveAssembly, refactoring and extending DP without being able to look at the generated types would be a pain.

Regarding (4), I guess framework exception types are still made [Serializable] and ISerializable so that the FCL code can be shared with Mono, which now sits in the same monorepo. I don't see any harm in following the recommended practice. We're no longer supporting remoting, but people could still be using DP inside a single app domain and expect unhandled DP exceptions to cross back into the main app domain.

[stakx]

I am going to close this issue. I've removed as many conditional compilation symbols as is currently possible without removing actual features.

The discussion about whether or not serialization support should be removed probably deserves a dedicated issue.