Roslyn-analyzers: 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

[KrisVandermotten]

I understand that for some ISymbol instances, there are now two notions of equality, because of NRT. Specifically, I understand that to be the case for ITypeSymbol and derived interfaces.

But what about symbols that are statically known not to be types?

For example, consider this code:

    private void M(SyntaxNodeAnalysisContext context, ISymbol symbol)
    {
        if (symbol.ContainingAssembly.Equals(context.Compilation.Assembly))
        {
            // ...
        }
    }

This code currently reports the diagnostic. But should it? Do assemblies exist in both a nullable and non-nullable version? I don't think so...

[sharwell]

@KrisVandermotten this issue concerns the messaging for the diagnostic. Your scenario sounds like a new case to handle; can you file a separate issue for it?

[KrisVandermotten]

@sharwell See issue #4410

[sharwell]

@mavasani What's the plan for documentation pages for Microsoft.CodeAnalysis.Analyzers? I would expect the description of the RS1024 scenarios to eventually live there.

[raffaeler]

@sharwell what is wrong with this? (still getting RS1024 from the analyzer)

symbol.AllInterfaces.Contains(baseType, SymbolEqualityComparer.Default)

[sharwell]

@raffaeler That looks like #4412

[raffaeler]

@sharwell yes, exactly

[weltkante]

Just came across this and wanted to mention that _no code fix is provided_ (contrary to what is claimed here) and the error message is still very confusing. Had to read this whole discussion before I knew what the error was complaining about.

[sharwell]

no code fix is provided

A code fix should be provided. Can you file a separate specific bug for the case you observed?

[Denis535]

@sharwell

What's wrong here?

var symbols = new HashSet<ISymbol>( SymbolEqualityComparer.Default );

Also how does it work? Isn't NRT compile time only? Or is it used only by analyzers?

[KrisVandermotten]

@Denis535

What's wrong here?

Nothing. That's a known bug, see #4568.

[vvuk]

I think part of the issue here is that the message -- Compare symbols correctly -- is both aggressive (e.g. read it with an implicit "... you idiot" at the end of it) and unhelpful. Perhaps:

Symbol comparison should explicitly use SymbolEqualityComparer.Default or SymbolEqualityComparer.IncludeNullability to indicate proper intent

[ChadNedzlek]

The unhelpfulness is the biggest problem for me. Requiring some having an IDE available to dechiper the correct action to take to fix an error message is rough. Sometimes I'm developing with VIM over SSH... code fixes are a great and very helpful feature, but they can't be required to compile a program without errors, at least, not for me. I've started just disabling analyzers for this reason.

Especially when @vvuk suggestion seems like an easy fix to make the error message complete and useful with minimal effort.

[CyrusNajmabadi]

Especially when @vvuk suggestion seems like an easy fix to make the error message complete and useful with minimal effort.

PRs always welcome :-)