The failures are coming from the simple cases here:

            yield return new object[] { "Some.Fake.Assembly.FakeType, Some.Fake.Assembly", "Some.Fake.Assembly" }; // simple
            yield return new object[] { "Some.Fake.Assembly.FakeType, Some.Fake.Assembly ", "Some.Fake.Assembly" }; // simple w/ whitespace

CoreCLR reports Some.Assembly, Culture=neutral, PublicKeyToken=null here. Is that desirable?

Edit: never mind, misread the logs, lots of other errors

Assembly name parsing trims whitespaces, and this one is no exception. For example. new AssemblyName("A ") will ignore white spaces too.

Oh, I see why these tests are failing now; this behavior differs significantly between core and framework. It seems on framework only the specified components are printed, whereas on core the Culture and PublicKeyToken are always printed even if defaults and the Version is only printed when specified. Do you know by chance where this changed and if it was intentional?

For this program:

using System;
                    
public class Program
{
    public static void Main()
    {
        ResolveEventHandler handler = (sender, arg) =>
        {
            Console.WriteLine(arg.Name);
            return null;
        };

        AppDomain.CurrentDomain.AssemblyResolve += handler;

        var t = Type.GetType("Some.Fake.Assembly.FakeType, Some.Fake.Assembly2");
    }
}

Framework: Some.Fake.Assembly2
Core: Some.Fake.Assembly2, Culture=neutral, PublicKeyToken=null

I guess for Mono, I'll need to also add some tests to our mono/mono test suite and make the behavior different between the two versions.

I do not know when this changed.

This also seems different from when instantiating an AssemblyName directly? For example, from AssemblyNameTests.cs:

        [Fact]
        public static void FullName_WithPublicKey()
        {
            AssemblyName assemblyName = new AssemblyName("MyAssemblyName, Version=1.0.0.0");
            assemblyName.SetPublicKey(TheKey);
            Assert.Equal("MyAssemblyName, Version=1.0.0.0, PublicKeyToken=b03f5f7f11d50a3a", assemblyName.FullName);
        }

        [Fact]
        public static void Name_WithNullPublicKey()
        {
            AssemblyName assemblyName = new AssemblyName("noname,PublicKeyToken=null");
            Assert.Equal(0, assemblyName.GetPublicKeyToken().Length);
            Assert.Equal("noname, PublicKeyToken=null", assemblyName.FullName);
        }

@vitek-karas do you know when this behavior changed? Is it documented anywhere?

The assembly name loader is very different between .NET Framework and .NET Core. Also, we had several different implementations of assembly name parsing in .NET Framework. The implementation to use was selected based on compatibility quirks. Some of the behavior changes likely go 10+ years ago the assembly loader was refactored for Silverlight.

I think this boils down to differences in how Assembly.Load works and parses assembly names. For example:

class Program
{
    static void Main(string[] args)
    {
        ResolveEventHandler handler = (sender, arg) =>
        {
            Console.WriteLine(arg.Name);
            return null;
        };

        AppDomain.CurrentDomain.AssemblyResolve += handler;

        try { Assembly.Load("Some.Fake.Assembly.FakeType, abc"); } catch (Exception e) { Console.WriteLine(e); }
    }
}

.NET Core:
The given assembly name or codebase was invalid.

.NET Framework:
Some.Fake.Assembly.FakeType\, xxxxxxxxxxx

.NET Framework + quirks (set set COMPlus_useLegacyIdentityFormat=1):
Some.Fake.Assembly.FakeType

The best way to achieve identical assembly name parsing behavior between the runtimes would be to rewrite it in C# and share the implementation.

Hmm, I see.

I agree with you on the assembly name parsing and stringification; it would be very nice to have a shared managed implementation to serve as a source of truth. I'll open an issue about that separately, it's a nice longer-term goal. I would love to get rid of our current managed implementation, especially since it has accidentally deviated from the native one multiple times now.

In the short term, I think it's not too difficult for me to change the behavior of the specific cases I've included in this test for our parsers if we can decide on the desired behavior. Is the current CoreCLR behavior intentional and ideal? Is there any possibility of changing it? I want to add tests so that we at least can be sure we agree on the desirable behavior here, though probably the ones I've added here are not even sufficient.

This PR was prompted by a community contributor adding a bunch of the runtime work to mono/mono for framework compatibility, and I'd rather not leave them out to dry, but I don't want to merge a bunch of runtime changes that make our dotnet/runtime behavior even worse than now.

I am not aware of any user filled issue complaining about the current behavior of assembly name parsing, so I do not do see a good reason for changing it.

I'm happy to defer to @jkotas on this - he knows a lot more about the history of this code than I do. I do agree on not making changes to this - I haven't seen any complaints about this behavior yet. So while I find it somewhat random at places, it's better to leave as is if we don't have a strong reason to change.

Ideally we would:

• Add a strong suite of tests to "Describe" the current behavior
• Document it somewhere (maybe not docs, but at least internally in the repo)
• Implement a managed version of the code - can be used by mono first
• Move both runtimes to use the managed version of the code

That rough plan sounds good to me, with the actual managed version being a longer-term goal. The test suite, however, seems much more immediately valuable, so it would be nice to have someone refine and extend this PR (which only covers some weird stringification examples from framework) to better describe the netcore parsing and stringification behavior.

I'll open an issue for the rest of the work, and hopefully we can experiment with the managed parser down the line, it would be really nice to have.

@steveisok, is the test suite something your team could reasonably take on? It seems most appropriate there, or if not with someone on the CoreCLR side.

cc: @SamMonoRT

Okay, gave this a couple weeks to stew around in my head and want to document my current thoughts. From my perspective, there are four issues that have come up.

1. AssemblyName stringification inside ResolveEventHandler is different from AssemblyName.FullName/AssemblyName.ToString()

This is most easily illustrated with a sample:

using System;
using System.Reflection;

namespace HelloWorld
{
    internal class Program
    {
        private static void Main(string[] args)
        {
            string test_type = "Some.Assembly.FakeType";
            string test_assembly = "Some.Assembly, Version=4.0.0.0";

            string event_name = "";
            ResolveEventHandler handler = (sender, args) =>
            {
                event_name = args.Name;
                return null;
            };

            AppDomain.CurrentDomain.AssemblyResolve += handler;

            var t = Type.GetType(test_type + ", " + test_assembly);
            var asm_name = new AssemblyName(test_assembly);

            Console.WriteLine(event_name);
            Console.WriteLine(asm_name);
        }
    }
}

Output under CoreCLR:

Some.Assembly, Version=4.0.0.0, Culture=neutral, PublicKeyToken=null
Some.Assembly, Version=4.0.0.0

I really think this is just a bug we should fix to make behavior consistent throughout the framework. This appears to be a stringification difference rather than a parsing difference, and indeed Jan's example above seems to parse identically under .NET 5 in both the Assembly.Load and AssemblyName cases. If we want to leave the stringification for events as-is we really need to get test coverage there, because it is substantially different from both framework and AssemblyName.FullName. We have significant documentation around the AssemblyName behavior, and again a bunch of it no longer applies here and we should note that somewhere.

As far as I can tell the reason ResolveEventHandler and associated events were reintroduced in identical form to framework was to ease migrations, and more modern APIs like the AssemblyLoadContext.Resolving event just pass the AssemblyName directly. It's unfortunate that for AppDomain.AssemblyResolve we can't just pass the AssemblyName instead, and obviously when doing so there is no concern about stringification behavior, so this is just a problem with the legacy events. Having this API reintroduced but having the stringification differ from both framework and AssemblyName.FullName seems like it helps no one.

2. The runtime handles the AssemblyName stringification for these events

I think we should be using a managed formatter here instead of having the runtime do the stringification. If we match the behavior of AssemblyName.FullName, we could just have the runtime pass up the AssemblyName and then use the existing shared formatter (AssemblyNameFormatter), but otherwise we would have to write a second one to match the different behavior. From the Mono side, I would definitely love to get rid of calls to mono_stringify_assembly_name outside of error paths.

3. Both runtimes use a native parser for AssemblyName

Mono has the ParseAssemblyName icall and CoreCLR has its own equivalent. At least in our case, we'd love to use CoreRT's AssemblyNameParser instead, parse in managed where possible, and pass an AssemblyName down to the runtime in cases like Assembly.Load. Our parser is fragile and buggy, and switching over to the CoreRT parser seems like a clear win.

4. We should also consider sharing a Type name parser

Mono currently has a native parser and two managed parsers (TypeNameParser and TypeSpec) for... reasons. Ideally we would only have a single managed implementation, use it as much as possible, and share it with CoreCLR. This is not strictly related to the previous issues, but I was reminded of it due to the previous point.

I'll open separate issues for these in a bit.