Hot Reload breaks MethodInfo equality

[stakx]

Description

This is very similar to #67989, but with an important difference. That other issue states that:

In some cases, MethodInfo objects that refer to the same method but are obtained in different ways [...] do not compare equal [...].

It seems that Hot Reload appears to break MethodInfo equality even when the MethodInfos are obtained in exactly the same way. See the attached repro code example.

Reproduction Steps

Here's a short console app that you'll need to run twice: once without making any code changes; and a second time by making any code change while Console.ReadLine is waiting for user input. For example, change the . in "Hot reload here." to a !, then hit Apply Code Changes (then hit enter in the app's console window).

var method1 = typeof(IInterface).GetMethod("Method")!;

Console.WriteLine("Hot reload here.");
Console.ReadLine();

var method2 = typeof(IInterface).GetMethod("Method")!;

Console.WriteLine(method1.ReflectedType == method2.ReflectedType && method1.Module == method2.Module && method1.MetadataToken == method2.MetadataToken);
Console.WriteLine(method1 == method2);
Console.WriteLine(method1.Equals(method2));

public interface IInterface
{
    void Method();
}

Expected behavior

In both cases (without & with making any code changes), the expected output of the console app should be:

True
True
True

Actual behavior

When making a code change and Hot Reload's Apply Code Changes is performed, the output will be:

True
False
False

Regression?

No response

Known Workarounds

I suppose one could do what the repro code does, and use method1.Module == method2.Module && method1.MetadataToken == method2.MetadataToken instead of method1.Equals(method2) (though not sure whether that is guaranteed to work in all cases, e.g. with generic method instantiations)... but that obviously shouldn't be necessary.

Configuration

• .NET version: 6 (SDK version 6.0.300, commit 8473146e7d) with Visual Studio 17.2.0
• OS: Windows 10 Pro (21H2)
• Architecture: x64

Other information

No response

[dotnet-issue-labeler]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[ghost]

Tagging subscribers to this area: @dotnet/area-system-reflection
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Description

This is very similar to #67989, but with an important difference. That other issue states that:

In some cases, MethodInfo objects that refer to the same method but are obtained in different ways [...] do not compare equal [...].

It seems that Hot Reload appears to break MethodInfo equality even when the MethodInfos are obtained in exactly the same way. See the attached repro code example.

Reproduction Steps

Here's a short console app that you'll need to run twice: once without making any code changes; and a second time by making any code change while Console.ReadLine is waiting for user input. For example, change the . in "Hot reload here." to a !, then hit Apply Code Changes (then hit enter in the app's console window).

var method1 = typeof(IInterface).GetMethod("Method")!;

Console.WriteLine("Hot reload here.");
Console.ReadLine();

var method2 = typeof(IInterface).GetMethod("Method")!;

Console.WriteLine(method1.ReflectedType == method2.ReflectedType && method1.Module == method2.Module && method1.MetadataToken == method2.MetadataToken);
Console.WriteLine(method1 == method2);
Console.WriteLine(method1.Equals(method2));

public interface IInterface
{
    void Method();
}

Expected behavior

In both cases (without & with making any code changes), the expected output of the console app should be:

True
True
True

Actual behavior

When making a code change and Hot Reload's Apply Code Changes is performed, the output will be:

True
False
False

Regression?

No response

Known Workarounds

I suppose one could do what the repro code does, and use method1.Module == method2.Module && method1.MetadataToken == method2.MetadataToken instead of method1.Equals(method2) (though not sure whether that is guaranteed to work in all cases, e.g. with generic method instantiations)... but that obviously shouldn't be necessary.

Configuration

• .NET version: 6 (SDK version 6.0.300, commit 8473146e7d) with Visual Studio 17.2.0
• OS: Windows 10 Pro (21H2)
• Architecture: x64

Other information

No response

Author:	stakx
Assignees:	-
Labels:	area-System.Reflection, untriaged
Milestone:	-

[steveharter]

It is not possible to change existing references held by user code during a hot reload. Hot reload in general just clears runtime caches, so any existing references in memory will still be there and thus fail comparison against newly-created ones.

If we wanted to preserve existing references, the underlying cache could use WeakReference, but that would come at a non-trivial cost to implement plus likely a performance penalty.

However, it is likely feasible to have Equals() succeed against hot reload and similar scenarios.

[stephentoub]

However, it is likely feasible to have Equals() succeed against hot reload and similar scenarios.

We'd want to make sure this didn't negatively impact the performance of equality, in particular for inequality (since reference equality could still be used for the fast path). We might look to make any additions here conditioned on the process being configured for hot reload.

[steveharter]

Closing as duplicate of #67989 assuming that we implement a proper Equals().

[jkotas]

We should treat this and #67989 as two separate bugs.

#67989 (comment)

[steveharter]

The fix here is to implement Equals() + GetHashCode() on MethodInfo at least. Consider FieldInfo, PropertyInfo, ... pending research into these other areas and implementation cost.

The implementation would likely compare Module+Token, perhaps with a ReferenceEquals() pre-check for performance although checking Module+Token should be simple\fast.

@stakx what is your real-world scenario and priority of getting this fixed? Is it just a timing issue as shown in the main description, or is MethodInfo being cached in the user code outside of reflection caches? Does the false inequality side effect cause a bug that needs to be worked around or does it just cause an inefficiency perhaps?

[stakx]

@steveharter, I've opened this issue here as a reaction to devlooped/moq#1252. In short, using Hot Reload in conjunction with Moq, a rather popular .NET mocking library, can lead to the faulty behavior described. It's not just a timing issue; it breaks the debugging experience by introducing fake errors that wouldn't be there during "normal" code execution not involving Hot Reload.

Moq caches MethodInfos (outside Reflection caches) as a performance optimization: say, when you set up an interface type and Moq dynamically creates a proxy type implementing that interface, it'll at some point have to query the interface map to find out which methods in the proxy correspond to the interface methods that user code is setting up expectations for. In order to not have to re-query the interface method map over and over again (on every single method invocation happening on the proxy), it caches the proxy methods' MethodInfos.

I can't say how urgently Moq users need this fixed (these days I only maintain the library, I don't actively use it myself); I suppose one can simply switch off Hot Reload and all is well. Except that (a) you guys probably wants Hot Reload to work, and be used by devs, and (b) it simply seems wrong IMHO to leave something as fundamental as object equality broken.

/cc @frenzibyte @peppy — maybe you can add a word on how this bug affects you in your work.

[peppy]

I think the explanation and videos I included in devlooped/moq#1252 (comment) should summarise our use case quite well.

Basically, our component testing framework pins on us being able to hot-reload and see UI changes. We want to use moq to isolate individual components for testing, and with this bug existing we are unable to easily integrate it without making abstract implementations of every moq interface we are testing.

[steveharter]

Moq caches MethodInfos (outside Reflection caches) as a performance optimization

Another option would be a public "cache iteration" counter (or event perhaps) -- if the counters are not equal on access, then the Moq caches should be cleared. E.g.

 // Moq cache access
 if (Moq.MethodInfoCache.Iteration != MethodInfo.CacheIteration) 
 {
     Moq.MethodInfoCache.Clear(); // Caches will be built-up again
     Moq.MethodInfoCache.Iteration = MethodInfo.CacheIteration;
 }

[jkotas]

The existing MetadataUpdateHandlerAttribute allows libraries to clear their caches on hot reload updates. I agree that it would be an option for Moq.

Iteration = MethodInfo.CacheIteration;

This API would introduce hot-reload performance regressions. The reflection caches are cleared only partially. We are not clearing the cache e.g. for CoreLib methods since we know that CoreLib cannot be updated via hot-reload. It means that there is no uniform CacheIteration today. Also, updating all places that care about MethodInfo equality to check the CacheIteration would be very invasive for the ecosystem.

[stakx]

If my understanding of MetadataUpdateHandlerAttribute is correct, this won't work for Moq without some changes that may possibly affect performance (though I cannot say by how much without running some benchmarks). We're not caching MethodInfos in a central, static Dictionary<,>; we cache them distributed in object instances of a certain type (here). That is, we can't reset those MethodInfo references from a static ClearCache method because we'd be missing the references to the objects caching the MethodInfos. We'd likely have to start using a single ConditionalWeakTable<,> where all MethodInfos get put in. But accessing one of those is going to be one or more magnitudes slower than accessing a single class field, I guess.

I'm willing to give this a try, but what I don't understand is, why the push towards workarounds? Why not simply fix MethodInfo equality?

[buyaa-n]

Consider FieldInfo, PropertyInfo, ... pending research into these other areas and implementation cost.

FieldInfo, PropertyInfo has same issue, should we fix those similar way too? What about other members?

[jkotas]

Yes, we should fix all reflection *Infos that have this problem.

[MichalStrehovsky]

Should we also add an analyzer that would flag uses of == on FieldInfo, MethodInfo, etc. when targeting old versions of netstandard? The operator was only added in later versions of netstandard so people who use == on netstandard 1.x are actually doing referential equality. They need to call .Equals.

People do run into this. @eerhardt just ran into it two weeks ago octokit/octokit.graphql.net#262 (comment) and it's also a pain to troubleshoot.

...unless we plan to get rid of targeting netstandard 1.x.

[jkotas]

...unless we plan to get rid of targeting netstandard 1.x.

Yes, we are slowly getting rid of netstandard 1.x targeting.

From https://devblogs.microsoft.com/dotnet/the-future-of-net-standard : We’d generally recommend against targeting .NET Standard 1.x as it’s not worth the hassle anymore.

Leading by example in #53283