Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update messages for symbol comparison diagnostic and codefix #5535

Conversation

ryzngard
Copy link
Contributor

@ryzngard ryzngard commented Sep 28, 2021

Fixes #3427

New messages:

Title: Symbols should be compared for equality
Message: Use 'SymbolEqualityComparer' when comparing symbols
Description: Symbols should be compared for equality, not identity. Use an overload accepting an 'IEqualityComparer' and pass 'SymbolEqualityComparer'.

@ryzngard ryzngard requested a review from a team as a code owner September 28, 2021 00:50
@mavasani
Copy link
Contributor

One or more auto-generated documentation files were either edited manually, or not updated. Please revert changes made to the following files (if manually edited) and run msbuild /t:pack for each solution at the root of the repo to automatically update them:
D:\workspace_work\1\s\src\Microsoft.CodeAnalysis.Analyzers\Microsoft.CodeAnalysis.Analyzers.md
D:\workspace_work\1\s\src\Microsoft.CodeAnalysis.Analyzers\Microsoft.CodeAnalysis.Analyzers.sarif
D:\workspace_work\1\s\src\Microsoft.CodeAnalysis.Analyzers\RulesMissingDocumentation.md

@ryzngard You need to run msbuild /t:pack at the root of the repo and push the changes.

</data>
<data name="CompareSymbolsCorrectlyTitle" xml:space="preserve">
<value>Compare symbols correctly</value>
<value>Symbols should be compared for equality</value>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly, this is misleading. A code violating this rule is already comparing for equality. e,g symbolA.Equals(symbolB), and this is completely equivalent to symbolA.Equals(symbolB, SymbolEqualityComparer.Default).

The thing is mostly about the "intent" when there are nullable differences.

@ryzngard ryzngard merged commit e5a3099 into dotnet:release/6.0.1xx-rc2 Sep 28, 2021
@@ -341,13 +341,13 @@
<value>Symbols should be compared for equality, not identity. Use an overload accepting an 'IEqualityComparer' and pass 'SymbolEqualityComparer'.</value>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existing description (not touched in this PR) doesn't make much sense to me for the same reasoning in #5535 (comment).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Equality kind of stretches into inclusion/exclusion of nullable here. Shorthand to say "Should compare for equality" makes some sense to me, but I definitely see where the ", not identity" can be misleading.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Equality kind of stretches into inclusion/exclusion of nullable here

That's exactly it. this is what should be indicated by the diagnostic.

Probably:

  • Title: Make your symbol comparison intent explicit
  • Message: Specify whether you want to include nullability when comparing symbols
  • Description: When comparing symbols, explicitly specify a 'SymbolEqualityComparer' to indicate whether you want to include nullability in comparison

(I know my wording isn't very good, but something along those lines?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about:

  • Title: Symbol comparison should use 'SymbolEqualityComparer'
  • Message: Use a 'SymbolEqualityComparer' to specify whether to include nullability when comparing symbols
  • Description: When comparing symbols, it's good practice to specify whether symbols with different nullability should be considered equal. Use a 'SymbolEqualityComparer' to specify in this comparison whether to include nullability as part of the equality check.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That way the title tells you what we expect, the message and description both elaborate on the why

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much better!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback :) I'll see if I can get that in this afternoon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants