False positive for RS1024 when using ToDictionary(...)

[craigktreasure]

Analyzer

Diagnostic ID: RS1024: Compare symbols correctly

Analyzer source

SDK: Built-in CA analyzers in .NET 6 SDK or later

Version: SDK 6.0.100

AND

NuGet Package: Microsoft.CodeAnalysis.Analyzers

Version: 3.3.3 (Latest)

Describe the bug

When using the Enumerable.ToDictionary(...) extension method on something like an IEnumerable<ITypeSymbol>, it is not possible (from what I can tell) to satisfy the analyzer.

Steps To Reproduce

Consider the following example code:

IEnumerable<ITypeSymbol> symbols = GetTypeSymbolsFromSomewhere();
IDictionary<ITypeSymbol, string> symbolNameMap = symbols.ToDictionary(s => s, s => s.ToDisplayString());
# ^ Raises warning | RS1024: Use 'SymbolEqualityComparer' when comparing symbols

However, you can't fix the warning by using SymbolEqualityComparer.Default because it causes the wrong type to be returned. In this case an IDictionary<ISymbol, string> instead of an <IDictionary<ITypeSymbol, string>.

IEnumerable<ITypeSymbol> symbols = GetTypeSymbolsFromSomewhere();
IDictionary<ITypeSymbol, string> symbolNameMap = symbols.ToDictionary(s => s, s => s.ToDisplayString(), SymbolEqualityComparer.Default);
# ^ Raises error | CS0266: Cannot implicitly convert type 
#    'System.Collections.Generic.Dictionary<Microsoft.CodeAnalysis.ISymbol, string>' to
#    'System.Collections.Generic.IDictionary<Microsoft.CodeAnalysis.ITypeSymbol, string>'.
#    An explicit conversion exists (are you missing a cast?)

So, I figured I'd create an implementation of an IEqualityComparer<ITypeSymbol?>, since one doesn't already exist, using something like the following:

internal class TypeSymbolEqualityComparer : IEqualityComparer<ITypeSymbol?>
{
    public static readonly TypeSymbolEqualityComparer Default = new(SymbolEqualityComparer.Default);

    private readonly IEqualityComparer<ITypeSymbol?> symbolComparer;

    private TypeSymbolEqualityComparer(IEqualityComparer<ITypeSymbol?> symbolComparer)
        => this.symbolComparer = symbolComparer ?? throw new ArgumentNullException(nameof(symbolComparer));

    public bool Equals(ITypeSymbol? x, ITypeSymbol? y)
        => this.symbolComparer.Equals(x, y);

    public int GetHashCode([DisallowNull] ITypeSymbol? obj)
        => this.symbolComparer.GetHashCode(obj);
}

Then, back in our code, we'd write the following:

IEnumerable<ITypeSymbol> symbols = GetTypeSymbolsFromSomewhere();
IDictionary<ITypeSymbol, string> symbolNameMap = symbols.ToDictionary(s => s, s => s.ToDisplayString(), TypeSymbolEqualityComparer.Default);
# ^ Raises warning | RS1024: Use 'SymbolEqualityComparer' when comparing symbols

But again, the analyzer isn't satisfied.

Expected behavior

I can cleanly compare symbols properly using the Enumerable.ToDictionary(...) extension without getting an RS1024 warning.

Actual behavior

I can't cleanly compare symbols without getting the RS1024 warning, so I disable the warning.

Additional information

The same appears to be true for other extension methods like Enumerable.Union(...).

[svick]

However, you can't fix the warning by using SymbolEqualityComparer.Default because it causes the wrong type to be returned.

You can, if you take advantage of contravariance of IEqualityComparer:

symbols.ToDictionary(s => s, s => s.ToDisplayString(), (IEqualityComparer<ITypeSymbol>)SymbolEqualityComparer.Default);

Though it's probably better to take advantage of C# 10 and instead explicitly specify the return type of the first lambda:

symbols.ToDictionary(ITypeSymbol (s) => s, s => s.ToDisplayString(), SymbolEqualityComparer.Default);

[Youssef1313]

IDictionary<ITypeSymbol, string> symbolNameMap = symbols.ToDictionary(s => s, s => s.ToDisplayString(), TypeSymbolEqualityComparer.Default);

But again, the analyzer isn't satisfied.

This one seems like a bug to me. A custom equality comparer shouldn't violate RS1024. Other than that, I agree with @svick

[craigktreasure]

@svick Thanks! I didn't think to cast the silly comparer. That seems to be the only solution for extensions like .Distinct(...) and .Union(...). I just updated to .NET 6, so I can use the C# 10 trick you suggested for .ToDictionary(...).

As @Youssef1313 said, I still think it's reasonable to be able to use a custom comparer.

[Flash0ver]

I believe my issue is related: I'm facing a RS1024 when passing a custom IEqualityComparer<ISymbol> to HashSet`1.

_ = {|#0:new HashSet<ISymbol>(SymbolNameComparer.Instance)|};

internal sealed class SymbolNameComparer : EqualityComparer<ISymbol>
{
    private SymbolNameComparer() { }

    internal static IEqualityComparer<ISymbol> Instance { get; } = new SymbolNameComparer();

    public override bool Equals(ISymbol? x, ISymbol? y)
    {
        Debug.Assert(x is not null, $"{nameof(x)} not expected to be null");
        Debug.Assert(y is not null, $"{nameof(y)} not expected to be null");

        return x.Name.Equals(y.Name, StringComparison.Ordinal);
    }

    public override int GetHashCode([DisallowNull] ISymbol obj)
        => obj.Name.GetHashCode();
}

Location 0: Warning | RS1024 | Use 'SymbolEqualityComparer' when comparing symbols

<!-- .csproj -->
<ItemGroup>
  <PackageReference Include="Microsoft.CodeAnalysis.Analyzers" Version="3.3.3" PrivateAssets="all" />
</ItemGroup>

See https://github.com/Flash0ver/F0.Analyzers/blob/b8dc14657185c893d81dafb1eec5c871b88f21c9/source/production/F0.Analyzers/CodeAnalysis/CodeRefactorings/ObjectInitializer.cs#L104

[Youssef1313]

@Flash0ver #5807 should fix it.

[Flash0ver]

@Youssef1313 oh, wow - super quick, thank you.
Is there perhaps a pre-release package I may try? If so, I'm afraid I'm unaware of the feed.

[Youssef1313]

Links for pre-releases are in the README.

https://dev.azure.com/dnceng/public/_packaging?_a=package&feed=dotnet7&package=Microsoft.CodeAnalysis.Analyzers&version=3.3.4-beta1.22068.2&protocolType=NuGet

You can try witg the next prerelease after the PR gets merged.

[Flash0ver]

Thanks - fixed my issue.