PathAssemblyResolver System.Reflection.MetadataLoadContext should be sealed or allow overrides

[DevDrake]

_fileToPaths is marked private with seals all possibilities of extending the class over MetadataAssemblyResolver.
If the intention is to seal the class it should be clear, however i dont think it is appropriate. Instead I think the _fileToPaths should be made protected.

Another issue worth considering is making Resolve code to fail/return fast e.g.
if (!_fileToPaths.TryGetValue(assemblyName.Name, out List<string>? paths)) return null; and that should be checked after the Assert.

API Proposal

Make the field protected

public class PathAssemblyResolver : MetadataAssemblyResolver
{
-    private readonly Dictionary<string, List<string>> _fileToPaths;
+    protected readonly Dictionary<string, List<string>> fileToPaths;
}

Alternative Designs

Add a property

public class PathAssemblyResolver : MetadataAssemblyResolver
{
    private readonly Dictionary<string, List<string>> _fileToPaths;
+    protected Dictionary<string, List<string>> FileToPaths => _fileToPaths;
}

[Dotnet-GitSync-Bot]

I couldn't figure out the best area label to add to this issue. Please help me learn by adding exactly one area label.

[DevDrake]

area-System.Reflection

[DevDrake]

PR: #36756

[steveharter]

Due to 5.0 schedule constraints, this is being moved to Future.

Normally fields are always private or internal, not public or protected, so to address this scenario we may need to add a property.

A workaround is to copy the logic from PathAssemblyResolver into a new class and make necessary changes.

[bartonjs]

Video

For the primary proposal: we (generally) don't expose fields as public or protected.

For the alternative proposal: we feel that simply exposing the field via a property is still too little abstraction and too limiting on the type. Instead, determine what the use cases are for the extensibility and add protected members to provide that functionality without limiting the implementation details (for example, and add method / remove method / indexer)

[MarkPflug]

Without knowing the author's exact use-case, I wonder if they could create a "Composite Resolver" that derives from MetadataAssemblyResolver to combine multiple PathAssemblyResolvers. I created such a thing for a tool I was working on, and it worked for my needs.

	sealed class CompositeResolver : MetadataAssemblyResolver
	{
		readonly MetadataAssemblyResolver[] resolvers;
		public CompositeResolver(params MetadataAssemblyResolver[] resolvers)
		{
			this.resolvers = resolvers;
		}

		public override Assembly Resolve(MetadataLoadContext context, AssemblyName assemblyName)
		{
			foreach (var r in this.resolvers)
			{
				var asm = r.Resolve(context, assemblyName);
				if (asm != null)
					return asm;
			}
			return null;
		}
	}

PathAssemblyResolver a, b; // two immutable resolvers
var resolver = new CompositeResolver(a, b);

Sealing PathAssemblyResolver would makes sense if it wasn't a breaking change. Too bad sealed isn't the default in C#.

[steveharter]

It seems like PathAssemblyResolver should just have been sealed. However, it is too late for that.

I don't yet see a compelling ask for exposing the underlying dictionary for purposes of extending the existing logic. Until we have that, I suggest closing this issue.

[steveharter]

Closing; please re-open if there is a read-world extensibility need for exposing the underlying dictionary in PathAssemblyResolver.