LinqParsing - MetadataToken is insufficient to uniquely identify an expression method.

[JonnyWideFoot]

Appologies, I'm having trouble writing a Marten test to reproduce this as I have no control over what dotnet assignes for MetadataToken, however here are a couple of screengrabs from my project running against the latest (887afe7) codebase which is just after 7.3.0...

As you can see the IsDeleted and String compare methods in two different modules, Marten and System.Linq respectively, are getting the same hash code, and therefore LinqParsing.cs is caching the wrong value in the _methodParsers hash map.

Looking at the MS docs MetadataToken is only unique per module, and therefore I think the cache key needs to be changed..
https://learn.microsoft.com/en-us/dotnet/api/system.reflection.memberinfo.metadatatoken?view=net-8.0

Here is an example from local execution of my codebase with Marten integrated, where MaybeDeletedParser is being returned for a string comparison:

public class LinqParsing: IReadOnlyLinqParsing
{
    private static readonly HashSet<string> Encoutered = new HashSet<string>();

    internal IMethodCallParser FindMethodParser(MethodCallExpression expression)
    {
        var key = ToKey(expression.Method);
...

    private static long ToKey(MethodInfo expressionMethod)
    {
        Encoutered.Add(
            $"{expressionMethod.Module.Name}_{expressionMethod.Module.MetadataToken}_{expressionMethod.MetadataToken}_{expressionMethod.DeclaringType?.Name}_{expressionMethod.Name}");

        Console.WriteLine();
        Console.WriteLine();
        Console.WriteLine("---");
        Console.WriteLine();
        foreach (var x in Encoutered)
        {
            Console.WriteLine(x);
        }
...

Output:

Marten.dll_1_100663701_LinqExtensions_IsOneOf
Marten.dll_1_100666478_MatchesSqlExtensions_MatchesSql
Marten.dll_1_100665670_SoftDeletedExtensions_MaybeDeleted   // Duplicate
System.Linq.dll_1_100663360_Enumerable_Any
System.Linq.dll_1_100663395_Enumerable_Contains
Marten.dll_1_100663704_LinqExtensions_In
System.Private.CoreLib.dll_1_100665670_String_Contains   // Duplicate

Another repro path output:

Marten.dll_1_100663701_LinqExtensions_IsOneOf
Marten.dll_1_100665672_SoftDeletedExtensions_DeletedSince
Marten.dll_1_100665671_SoftDeletedExtensions_IsDeleted
System.Linq.dll_1_100663360_Enumerable_Any
System.Linq.dll_1_100663395_Enumerable_Contains
Marten.dll_1_100663704_LinqExtensions_In
Marten.dll_1_100665670_SoftDeletedExtensions_MaybeDeleted   // Dupe
Marten.dll_1_100666478_MatchesSqlExtensions_MatchesSql
System.Private.CoreLib.dll_1_100665670_String_Contains   // Dupe

Note I see "100665670" twice for two different methods

[jeremydmiller]

@JonnyWideFoot I hate being a developer sometimes. I've been bitten by similar issues in the past where you can never trust the Equals() comparison between MethodInfo

But, the mechanism in the PR is a no go on performance. Could we change the look up so that it's a two stage thing? Keep a separate ImHashMap per module, that has an ImHashMap per int for each method within that module? I'd guess that's faster than doing the string concatenation every time.

We had perf issues in StructureMap years ago from recalculating hash codes all the time that shocked me when someone found it and did a PR.

But hey, I appreciate your recent contributions and your willingness to dive into things!

[JonnyWideFoot]

@jeremydmiller - Yeah, this isn't a "done" implementaion, and agree on the performance concern. Just wanted to get the information in with what error I am seeing on my machine.

I'll have a think on a more performant way.. (I did want to use 'Module.MetadataToken' and store that in a long, but that turned out not to be unique either.)

(For completeness, I see the same error from both the 7.3.0 nuget packages and if I build my solution against Marten's source code.)