Add an equality comparer for AssemblyName in RazorAnalyzerAssemblyResolver #76752
Conversation
dotnet-issue-labeler
bot
added
the
untriaged
|
@dotnet/roslyn-compiler for review please :) |
| { | ||
| public bool Equals(AssemblyName? x, AssemblyName? y) => x?.FullName.Equals(y?.FullName) == true; | ||
|
|
||
| public int GetHashCode(AssemblyName obj) => Hash.GetFNVHashCode(obj.FullName); |
There was a problem hiding this comment.
I guess this direct call to a hash function is preferable to string.GetHashCode()?
There was a problem hiding this comment.
Yeah, so string.GetHashCode is a bit tricky, as it changes between runs. This is purely in memory so it should be fine, but on desktop it can also change between AppDomains. This should only run on core as it ends up in ServiceHub, but this assembly does cross compile, and servicehub supports .NETFX, so could conceivably be loaded on framework. I figured the number of calls to this is going to be small enough that we can live with the hit, but if we're worried about the cost we can just go with string.GetHashCode instead.
|
|
||
| private class AssemblyNameEqualityComparer : IEqualityComparer<AssemblyName> | ||
| { | ||
| public bool Equals(AssemblyName? x, AssemblyName? y) => x?.FullName.Equals(y?.FullName) == true; |
There was a problem hiding this comment.
It's not obvious to me this does the right thing when one or both AssemblyNames are null. Maybe it would be simpler to branch like x is null ? y is null : x.FullName.Equals(y?.FullName) (if we can rely on FullName always being non-null, when the containing instance is non-null.)
There was a problem hiding this comment.
On .NET core only the short name is considered for loading purposes in the majority case. This code is going to differentiate based on the full name though.
There was a problem hiding this comment.
Yeah, so I mentioned this on Aleksey's comment, but we control the parent interface which is annotated as never being null, and the comparer is only ever used here. I've updated it to ! those values to indicate we know it will never be null as opposed to making the test stronger. Happy to switch back to a stronger null check if you prefer though.
There was a problem hiding this comment.
On .NET core only the short name is considered for loading purposes in the majority case. This code is going to differentiate based on the full name though.
This was deliberate. Full name includes the version string i.e something like Microsoft.CodeAnalysis.Razor.Compiler, Version=9.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e
Whereas .Name (which I assume is what you mean by shortname?) only includes the assembly name itself (Microsoft.CodeAnalysis.Razor.Compiler). Given that this is AssemblyLoadContext code its completely valid to load two different versions of the same assembly.
If Razor specifically asks for version 9.0.0.0 I don't want to consider an 8.0.0.0 version to be equivalent.
| { | ||
| public bool Equals(AssemblyName? x, AssemblyName? y) => x?.FullName.Equals(y?.FullName) == true; | ||
|
|
||
| public int GetHashCode(AssemblyName obj) => Hash.GetFNVHashCode(obj.FullName); |
There was a problem hiding this comment.
IEqualityComparer<T>.GetHashCode is marked [DisallowNull] which I beleive the intent is that it will never be called with a null value. We also own the parent IAnalyzerAssemblyResolver interface, and this comparer is only used here, so annotations say we can never see a null value for AssemblyName even if the comparer did support calling it with a null value. I'm happy to handle null though if you'd prefer.
src/Tools/ExternalAccess/Razor/RazorAnalyzerAssemblyResolver.cs
Outdated
Show resolved
Hide resolved
|
|
||
| private class AssemblyNameEqualityComparer : IEqualityComparer<AssemblyName> | ||
| { | ||
| public bool Equals(AssemblyName? x, AssemblyName? y) => x?.FullName.Equals(y?.FullName) == true; |
There was a problem hiding this comment.
On .NET core only the short name is considered for loading purposes in the majority case. This code is going to differentiate based on the full name though.
|
@dotnet/roslyn-compiler for second round of feedback. Thanks! |
|
Closing in favor of #76808 |
AssemblyNamedoesn't implementGetHashCodeso each instance was unique, leading to duplicates in the hash set. This PR adds an equality comparer that uses theFullNameof theAssemblyNameas the comparison item for the hash set.