Review of FEATURE_ASSEMBLIES and ReflectionUtil

[Jevonius]

tl;dr;
I think there are some issues with how ReflectionUtil is currently behaving, having investigated #619. I'm digging into/learning the differences between Assembly handling for Framework/Standard/Core with respect to this functionality to understand it better. If anyone has experience with this area of the code please shout!

long version
After digging into #619, I've been trying to get my head around the ReflectionUtil class, where the FEATURE_ASSEMBLIES switch does things. I think there might be a few [potential] bugs here, both with some of the logic (e.g. IsAssemblyFile, IsApplicationAssembly), incorrect usage of Assembly.Load/Assembly.LoadFile and around the handling of Framework/Standard/Core.

System.Runtime.Loader is pulled in for the netstandard2.0 targeting, but it sounds like really it should only be used for projects targeting Core - the package being pulled in dates from around the release of .net Core 1.1, but dotnet/runtime#20849 (from a year later) suggests it doesn't work with .net Framework (https://docs.microsoft.com/en-us/dotnet/api/system.runtime.loader.assemblyloadcontext has only .net Core as supported). I can't see how to reconcile targeting netstandard2.0 and crossing this boundary without the risk of runtime errors (unless the nuget package shims the behaviour for Framework?). I'm going to do some more digging into the source to understand, but I'm assuming this isn't usually an issue as Framework consumers will just consume the net462 package.

For Assembly.Load/Assembly.LoadFile, the code currently uses IsAssemblyFile but Load should only be used for referenced assemblies, LoadFile should be used for others being pulled in, and IsAssemblyFile doesn't validate this - there's no checking for referenced assemblies. Annoyingly I can't now find the page I was reading about this, but in a similar area I'm going to dig into AssemblyLoadContext as I suspect Windsor could be leveraging this more for Core.

The notes on Assembly.Load have this remark:

Do not use an AssemblyName with only the CodeBase property set. The CodeBase property does not supply any elements of the assembly identity (such as name or version), so loading does not occur according to load-by-identity rules, as you would expect from the Load method. Instead, the assembly is loaded using load-from rules. For information about the disadvantages of using the load-from context, see the Assembly.LoadFrom method overload or Best Practices for Assembly Loading

GetAssemblyName(string filePath) does this at the moment, which might be intentional, but might not be.

If anyone's got any good links/background knowledge, please let me know!

[Jevonius]

Unfortunately, I haven't made any further progress with this yet, but I'm leaning towards suggesting we drop netstandard2.x as a target; I just can't see how to reconcile the System.Runtime.Loader lack-of-support for Framework issue. As ever, if anyone has experience with this area of the code and has advice, I'm all ears :)

[jeme]

@Jevonius Considering the project are targeting .NET Framework 4.6.2, .NET Standard 2.0 and .NET 6.0 I think this is how that plays out when you depend on it, its what I would expect happening anyways, I may be wrong though.

Project Framework Target	Castle Winsor Target
.NET Framework 4.6.2	.NET Framework 4.6.2
.NET Framework 4.7.0	.NET Framework 4.6.2
.NET Framework 4.7.1	.NET Framework 4.6.2
.NET Framework 4.7.2	.NET Framework 4.6.2
.NET Framework 4.8.0	.NET Framework 4.6.2
.NET Core 2.0	.NET Standard 2.0
.NET Core 2.1	.NET Standard 2.0
.NET Core 3.0	.NET Standard 2.0
.NET Core 3.1	.NET Standard 2.0
.NET 5.0	.NET Standard 2.0
.NET 6.0	.NET 6.0
.NET 7.0	.NET 6.0
.NET Standard 2.0*	.NET Standard 2.0

So I am not sure that ""but it sounds like really it should only be used for projects targeting Core"" is a problem here, unless your using .net 4.6.1, but that is asking for trouble anyways as there is a caveat around that being treated as a framework that implements .NET Standard 2.0. Besides that is EOL as i recall.

*Only relevant in light of other libraries that depend on this one.

[jeme]

As for the Assembly.Load/Assembly.LoadFile that sort of looks strange to me, I only have experience with these kinds of things from the .NET Framework perspective, so I am not sure what change there may be. So why the code block in IsAssemblyFile doesn't match the logic on the .NET Framework understandably raises some questions.

[jonorossi]

I don't really understand the problems here, but your comment from #621:

Currently concerned we might need to consider dropping targeting netstandard2.x to avoid the risk of a run-time error.

Personally I don't see the need for .NET Standard 2.x anymore. .NET Core 3.1 is only supported for a very short time. I'd be fine with .NET 6 and .NET Framework 4.x.x.

[Kuling]

Regarding dropping support for netstandard2.x.
How about large solutions which didn't fully migrated to .NET Core.

In such case netstandard2.x is often used for projects lower in dependency hierarchy.
I may be wrong but I can imagine that people use Castle.Windsor in those projects.

I've struggled with migration to .NET Core for large solution and support for netstandard2.0 was a life saver. It is kind of intermediate step/"base camp" before you move further with migration.

[jonorossi]

@Kuling Good point. I've had some success doing that in the past, but recently tried doing the same and found other dependencies had already dropped .NET Standard 2.0, so had to go straight from .NET Framework to .NET 6. We need to understand exactly why .NET Standard 2.0 can no longer be targeted.

[jeme]

@Kuling Good point, totally didn't think about that target in the list. Obviously you would still end up with one of the implementations ones you get to something that executes the code. But I will add it to that list with a *.

@jonorossi Could you clarify what you mean by ""We need to understand exactly why .NET Standard 2.0 can no longer be targeted."" a bit?

.NET Standard 2.0 can ofc. be targeted, from a library point of view.
There is no implementation in that version so you would ofc. end up with one of the implementations behind it. Hell Mono could even be one of those but did not include that here in the list. Maybe I should have? o.O...

On the Lucene.Net project which I also pinch in a bit here and there, they still target .NET Standard 2.0 as well.

I think it's a matter of considering the effort in doing so, right now Castle.Windsor compiles and all the tests passes so I se no reason what so ever to remove it right this moment. Instead if the burden of maintaining it as a target gets to a point where it outweighs any benefits, then is should probably be removed.

At the end of the day, if a library Author chooses to support .NET 462 and .NET 6.0 but remove .NET Standard 2.0, I think it's most of the time because it's believed that there is no reason to support the standard, simply because those reasons are unknown to the Author, I have honestly been in that situation my self and made the mistake of thinking that there was no reason at all, however as @Kuling outlines, there can actually be reasons as he outlines one I didn't even think of.

[jonorossi]

@jeme I definitely knew there is a reason for supporting and agree with what you've said regarding supporting it, that's why we went to the effort to support it rather than using the netcoreapp TFMs. My comment was explicitly referencing a comment made by @Jevonius that said we may not be able to continue supporting it for things to work correctly, I don't have any more information than that which is why I said we need to know the reason and weigh up the pros/cons. This task is about FEATURE_ASSEMBLIES and ReflectionUtil, rather than TFMs generally.

Also know that .NET Standard is a leaky abstraction that worked well in general but has a lot of holes like we discovered with Castle Core, see castleproject/Core#374. Plenty of runtimes throw PlatformNotSupportedExceptions for various things too.

Happy to follow others. I've tried to maintain .NET Framework and .NET Standard support with Castle libraries for as long as practical and think we should continuing trying.

[Jevonius]

Hi all, apologies for the absence, have been busy elsewhere; didn't realise quite how long it had been! Just getting my head back into this. I'm still working through the assembly handling to understand it fully (and how System.Runtime.Loader package fits in). I agree that dropping netstandard2.0 wouldn't be great for those dealing with mixed environments, although at some point the time will come; be nice to keep it around for a while longer though.

[jonorossi]

Removing v6.0.0 milestone