Messaging for RS1024 (Compare symbols correctly) is unclear

[jods4]

Version Used: 3.5.0

Steps to Reproduce:
Compare two ISymbol through their IEquatable<ISymbol> interface

ISymbol s1, s2;
// hovering indicates this is not Object.Equals(object) 
// but IEquatable<ISymbol>.Equals(ISymbol)
var eq = s1.Equals(s2); 

Expected Behavior:
No warning.

Actual Behavior:
Warning RS1024 Compare symbols correctly

[sharwell]

This is the expected behavior. When building against a version of roslyn that provides SymbolEqualityComparer, it should be used instead of Equals for comparing symbols. A code fix should be provided to make the conversion a simple process.

[jods4]

@sharwell could you explain a bit more please?
Why is SymbolEqualityComparer better? (it's certainly not simpler in the example that I gave)
Is IEquatable<Symbol> not doing the right thing?

The warning message says the symbols are not compared correctly, from my tests I think the comparison is correct. If there's another reason to refactor my code, at least the message should be changed a bit?

[sharwell]

IEquatable<ISymbol> and object.Equals both behave like SymbolEqualityComparer.Default, specifically meaning the types ISymbol and ISymbol? are considered equal. The new equality comparer and patterns force code to clearly indicate the manner in which nullable reference types are expected to impact code analysis.

[jods4]

I can mute the warning, so it's not a big issue.

As a user, I just don't understand why the IDE wants me to write
bool same = SymbolEqualityComparer.Default.Equals(s1, s2)
when I can do
bool same = s1.Equals(s2)
and it works just the same.

[CyrusNajmabadi]

when I can do
bool same = s1.Equals(s2)
and it works just the same.

Because the above could be a bug, and the user intent isn't clear in the code. i.e. you may have purposefully left out the equality comaprer, or you may have mistakingly done it.

The point of the error is to push users to be explicit so that intent is clear in the code. i.e. if you write:

s1.Equals(s2, SymbolEqualityComparer.Default) i can see exactly that you want these semantics, because you wrote that out in the code.

[jods4]

@CyrusNajmabadi I do not agree with you.

The analyzer 100% knows that I'm calling IEquatable<ISymbol>.
Both operands are statically typed as ISymbol and the result of the comparison will always be correct.
This cannot be a bug and it is guaranteed to work. If it doesn't I don't see why, so please tell me.

As it stands:

• The analyzer think I'm an idiot and I can't use IEquatable<T> properly.
• The analyzer tells me (literally) that my code is incorrect although the code is demonstrably correct.

The point of the error is to push users to be explicit so that intent is clear in the code.

The intent is very clear, I'm comparing two symbols for equality. If you find the meaning of "equality" ambiguous then you should remove operators overloading ==, virtual Equals, and IEquatable from C#.

Nothing in your argument is specific to symbols, so taking this to its logical conclusion, we should have a general analyzer that forbids all those things on all objects.

Anyway, if you think there's something special and magical about symbols; I can always silent this warning, so no biggie.

[sharwell]

@jods4 The team who created this API incorrectly compared symbols in a number of cases that led to subtle bugs. Since this is a known point of confusion for analyzer authors, it makes sense to recommend one consistent way to perform the operation so all participants in the project immediately understand the intent of any given operation. The use of IEquatable<ISymbol> is not ambiguous at runtime, but it is ambiguous to readers. It can mean either of these:

1. The analyzer was written prior to the introduction of Nullable Reference Types, and needs to be updated to account for the way the new feature impacts symbols by explicitly specifying the manner in which symbols are compared
2. The analyzer was written with Nullable Reference Types in mind, and intends to perform this comparison without consideration of nullable annotations

[jods4]

@sharwell I feel like this might just be a case of missing information.
Could you explain what has changed with the introduction of nullable reference types, when it comes to comparing symbol?
And how that could cause an actual bug if I make the wrong assumption?

[sharwell]

@mavasani do you have an example of where we need nullable-aware comparison, and an example of where we need the default comparison to ignore it?

[mavasani]

Tagging @ryzngard @jasonmalinowski - they might know of the cases where the new symbol comparer is required in nullable context.

[CyrusNajmabadi]

The analyzer 100% knows that I'm calling IEquatable.
Both operands are statically typed as ISymbol and the result of the comparison will always be correct.

Not necessarily. For example, the code might have not wanted string? and string to be equals. There's no way to know what the intent was. That's why we ask that the equality comparer be explicit. That way code can be clear about if their intent is i do want nullable annotated types to be considered equivalent to non-annotated types or i do not want that :)

[CyrusNajmabadi]

Nothing in your argument is specific to symbols, so taking this to its logical conclusion, we should have a general analyzer that forbids all those things on all objects.

This is unrelated to objects. this is about Symbols now having two distinct concepts of equality. strict-equality, where things like Foo(string?) is distinct from Foo(string), and null-oblivious equality where those two symbols would now be considered the same.

Because there are two completely valid forms of equality, we ask that people be explicit to make their code clear.

This is an issue because we shipped a prior api where there was only one form of equality. However, with the introduction of NRT we now had two. Had we had NRT from the beginning, we would not have had a 'default' concept of equality here, and we would have made it a required parameter. This analyzer is to help move older code forward to use the better pattern now that there are multiple concepts of equality.

[CyrusNajmabadi]

and an example of where we need the default comparison to ignore it?

One example. When you're implementing an interface, we need to see whihc members are already implemented. Implementations do not care if there are NRT differences. As such, we use the 'default' comparer.

However, for code that is generating casts, we'll often put in a check that says "don't bother to cast if the types are hte same". i.e. no point to cast a string to (string). However, that's not true if we're actually widening to (string?). In that case, we would consider this an appropriate cast to add.

[CyrusNajmabadi]

@jods, i recommend taking questions over to gitter as well, you'll likely be able to get responses faster and in a more conducive conversational manner to your questions.

[jods4]

@CyrusNajmabadi I see and I understand now!
I think it's really a matter of changing the warning message, then (I think you should re-open the issue for that).

Right now, as a user I just read: "your code is incorrect" and I'm like: "no it's not".

It would be much better if the warning said something along the lines of:
"Equals is ambiguous, prefer an explicit comparaison between XXX (strict equality) and YYY (null-oblivious equality)."

That is actually actionnable. On top of disagreeing with the message, I had just no idea what I should change about it, as IEquatable looked perfectly fine to me.

[CyrusNajmabadi]

I have no problem with htat. Reactivating to track an improved error message.

[Evangelink]

@jods4 Would you find these messages more helpful:

• Message: Equality is ambiguous, prefer an explicit comparison.
• Description: Equality is ambiguous for readers, do you mean strict equality or null-oblivious equality?

[jods4]

Close! If possible, it would be best if you could point what the right ways to do it are (obviously there are two options).

[sharwell]

@jods4 There is a code fix for this diagnostic which automates the transition to the new pattern, so there is a bit less need to be verbose about the steps to take.

[ChadNedzlek]

The code fix isn't always available (in my case, it crashes VS, but I'm sure that's just a bug). I just wasted quite a bit of time trying to figure out what this warning even meant. Thank goodness for this random issue, or I'd never have figured it out. The error message just should just up and say what the fix wants you to do.

[Evangelink]

@sharwell Shall we update the message or is everything's ok now and the ticket can be closed?

[Evangelink]

ping @sharwell

[mavasani]

Let's use this issue to improve the diagnostic message.

[carlossanlop]

Edit: I just saw a fix has been provided here #4571.

I'm also reproing this as well when defining a dictionary with ISymbol as the TKey:

[ReubenBond]

What am I doing wrong?

As a matter of usability, could the default equality comparer implementation be made to work 'correctly' for all ISymbol implementations?

@carlossanlop, I believe you should be passing the correct IEqualityComparer instance to the Dictionary's constructor.

[weltkante]

I'm also reproing this as well when defining a dictionary with ISymbol as the TKey:

you're probably on the wrong issue, the PR #4571 was for issue #4568 (which has been mentioned above)

What am I doing wrong?

@ReubenBond Same question was asked earlier above, this was issue #4568, you probably don't have the fix from PR #4571 yet

[mbaumanns]

Hello everybody,

in general I understand (and like) the warining. But I do not understand why I get the warning: RS1024 when creating a HashSet in the following way:

private readonly ISet<ITypeSymbol> HandledTypes = new HashSet<ITypeSymbol>(SymbolEqualityComparer.Default);

How should I store multiple ITypeSymbol objects in my rewriters? I prefer an ISet instead of an IList object because of better search capabilities.
Or should I use a List and use Linq method "Contains" to Check if the object is already in the list? I would not see the warning but I guess the performace is not that good.

Best regards,

Michael

[weltkante]

But I do not understand why I get the warning: RS1024 when creating a HashSet in the following way:

@mbaumanns That has been confirmed above as a false-positive that has been fixed in #4568

#4845 confirms a fix has been deployed so you may want to check the version you are using? I don't know if its in a non-beta build yet, but either way its a different issue than what is discussed here.

If you have to use a broken version you can selectively suppress the warning for this line of code (if you do it often you can write a helper method that does the work and suppresses the warning once)

[MarkKharitonov]

I have just got this warning, but I do not understand why - I do not compare any symbols:

Dictionary<string, IGrouping<string, IMethodSymbol>> argTypeVariants = methodSymbolInfo.CandidateSymbols
    .Cast<IMethodSymbol>()
    .GroupBy(candidateSymbol => candidateSymbol.Parameters[err.ArgNo - 1].Type.ToString())
    .ToDictionary(g => g.Key);

Here is the snapshot:

What exactly is the problem?

[raffaeler]

In order to work, the GroupBy operator uses Equatable.Default which look for the IEquatable interface ending up to an Equals method call for the comparison

[KrisVandermotten]

@raffaeler

The code shown by @MarkKharitonov is not comparing symbols, it is comparing strings.

[raffaeler]

@KrisVandermotten Agree, but my guess is that the Analyzer is not that smart to get the right type.
I guesss it just finds a groupby with the roslyn symbol in the descendants syntax tree.

[MarkKharitonov]

@raffaeler - then would you not agree it is a bug in the analyzer? Should I open another github issue for it?

[raffaeler]

I believe it is and this is the right place.
I would like to know from @sharwell if the fix (maybe the correct word is improve) is already ongoing.

[ryzngard]

In general if you find a new issue with an analyzer a new bug would be better than adding onto an existing issue.

In this case, the latest code seems to have fixed this. The analyzer now checks for known generic constraints TKey instead of checking all constraints for ISymbol

Here's the test code I used in CompareSymbolsCorrectlyTests.cs to verify

[Fact, WorkItem(3427, "https://github.com/dotnet/roslyn-analyzers/issues/3427")]
public async Task RS1024_GroupBy()
{
    await new VerifyCS.Test
    {
        TestState =
        {
            Sources =
            {
@"
using System;
using System.Collections;
using System.Collections.Generic;
using System.Linq;
using Microsoft.CodeAnalysis;

class C
{
void M(IEnumerable<IMethodSymbol> methodSymbols)
{
var dict = methodSymbols
    .GroupBy(candidateSymbol => candidateSymbol.Parameters[0].Type.ToString())
    .ToDictionary(g => g.Key);
}
}"
                , SymbolEqualityComparerStubCSharp
            },
            ReferenceAssemblies = CreateNetCoreReferenceAssemblies()
        }
    }.RunAsync();
}

[MarkKharitonov]

The fixed code has not been released yet, correct?

[ryzngard]

@MarkKharitonov I believe if you're getting them from the SDK it is in .NET 6 previews, or preview releases of the analyzers.

[jods4]

After an SDK upgrade (I suppose...) I am getting new warnings.

It's now triggered by dictionaries, such as new Dictionary<ITypeSymbol, string>.
Fair enough, I added an explicit comparer:

new Dictionary<ITypeSymbol, string>(SymbolEqualityComparer.Default)

But the warning remains and I don't know what to do.
Same on HashSet.

I note that the message is more clear now as it points out that string and string? may be compared equal vs different. 👍

It doesn't say what action we should take, for example in the dictionary case it would be helpful to say You should specify which SymbolEqualityComparer you want explicitely.

Then again, I did that and the warning is still here so I dunno?

[KrisVandermotten]

@jods4 That looks like #4568 again.

[jods4]

@KrisVandermotten yes, thanks. That's exactly it.
Looks like it's been closed since Dec 2020 and I am running VS 16.10.1; is it expected that I still get the error?

[MihailsKuzmins]

get the error even when IPropertySymbol is a value in the dictionary

[silkfire]

@ryzngard Does the .NET 6 SDK include the reworded error message as well?

[ryzngard]

@MihailsKuzmins this looks like it should be fixed already. What version of analyzers are you using?

@silkfire let me check scheduling. I haven't made changes to the text as it hasn't been high on my priority list, but if there's still time to make a change here I will. Otherwise it will go into .NET 7 SDK

[MihailsKuzmins]

@ryzngard
version of Microsoft.CodeAnalysis.Analyzers is 3.3.2 which was released a long time ago (on 4 December 2020). Have also checked prerelease NuGets - there are none for the package.

Is it possible to get a package version with the fix?

[Youssef1313]

@MarkKharitonov I believe if you're getting them from the SDK it is in .NET 6 previews, or preview releases of the analyzers.

@ryzngard I believe this is not released with the SDK at all.

[Youssef1313]

@MihailsKuzmins A pre-release is available here.

I think a stable version should be pushed to NuGet. cc @jmarolf

[ryzngard]

Yea, I always forget metadata analyzers aren't in the SDK. Also means that my string fix might be in the wrong branch? Happy to retarget port depending on where we release our NuGet from

[MihailsKuzmins]

@Youssef1313, great thank you 🙏