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

Enforce that symbol comparison should use an equality comparer #2764

Merged
merged 11 commits into from Oct 1, 2019

Conversation

ryzngard
Copy link
Contributor

With the introduction of nullable reference types, comparison of symbols has become more complex. The recommended way is to use an EqualityComparer. This adds a check for the Equals method where all parameters are ISymbol types. If the method invocation is from an instance, the instance is also checked to be ISymbol.

Also updated the text to reflect new recommended comparison and resolution.

@sharwell
Copy link
Member

❓ Have you verified that this diagnostic does not suggest using an EqualityComparer if the user is building against Roslyn 1.x or 2.x? For those releases, Equals is the expected approach.

@ryzngard
Copy link
Contributor Author

ryzngard commented Aug 14, 2019

❓ Have you verified that this diagnostic does not suggest using an EqualityComparer if the user is building against Roslyn 1.x or 2.x? For those releases, Equals is the expected approach.

I have not yet. I also may have misinterpreted the recommendation from @jasonmalinowski .

Is there a good way to version lock? Can we check for the existence of the wrapped type?

@mavasani
Copy link
Member

Is there a good way to version lock? Can we check for the existence of the wrapped type?

Is there a specific equality comparer type in Roslyn that was added in 3.x? If so, you can fetch this named type symbol at compilation start and keep your logic guarded around the presence of that type symbol.

@ryzngard
Copy link
Contributor Author

private const string s_symbolEqualityComparerName = typeof(SymbolEqualityComparer).FullName;
...

comparerType = compilation.GetTypeByMetadataName(s_symbolEqualityComparerName);

Is that the correct way?

Currently it doesn't look like the version of Roslyn being referenced by the analyzers is up to date enough to have the comparer.

@mavasani
Copy link
Member

@ryzngard Master is currently targeting 2.9.0 version of MS.CA so that the analyzers can execute on Dev15 toolset. You may want to consider retargeting your PR to 3.x branch and move the version of referenced MS.CA in that branch to your required Roslyn version. You would need to change the following:

<MicrosoftCodeAnalysisVersion>3.0.0</MicrosoftCodeAnalysisVersion>
<MicrosoftNetCompilersVersion>3.0.0</MicrosoftNetCompilersVersion>
and also update the VersionPrefix so the major and minor versions for the analyzer package match that of the consumed Roslyn version:
<VersionPrefix>3.0.0</VersionPrefix>

@mavasani
Copy link
Member

Actually, stepping back. If you don't need to invoke any new Roslyn API in 3.x version and are just looking for a type symbol for SymbolEqualityComparer, then you don't need to move to newer Roslyn bits. You can just hardcode the fully qualified type name for SymbolEqualityComparer in the call to GetTypeByMetadataName

@ryzngard
Copy link
Contributor Author

ryzngard commented Sep 3, 2019

@sharwell with the updated code I'm having trouble with tests. Is there an assembly reference I should add? I would have thought Microsoft.CodeAnalysis was already referenced

Diagnostics:
    // Test0.cs(5,29): error CS0234: The type or namespace name 'SymbolEqualityComparer' does not exist in the namespace 'Microsoft.CodeAnalysis' (are you missing an assembly reference?)

@mavasani
Copy link
Member

mavasani commented Sep 3, 2019

I would have thought Microsoft.CodeAnalysis was already referenced

I believe the tests would also reference the older (2.9.x) version of code analysis. You should extend your test code to define the required equality comparer in test source code with stub implementations, we do so in many tests. For example, see https://github.com/dotnet/roslyn-analyzers/blob/master/src/Microsoft.CodeAnalysis.Analyzers/UnitTests/UpgradeMSBuildWorkspaceAnalyzerTests.cs#L20

@ryzngard
Copy link
Contributor Author

ryzngard commented Sep 3, 2019

@mavasani ah, thanks. For some reason I thought the public comparer was older but was added at the same time. I'll update the test

return;
}

var parameters = invocationOperation.Arguments;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: rename to arguments

@ryzngard ryzngard merged commit 789704f into master Oct 1, 2019
@mavasani mavasani deleted the feature/catch_isymbol_equals_without_comparer branch November 26, 2019 15:38
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.

None yet

3 participants