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
Expand Up @@ -339,7 +339,7 @@
<value>Upgrade MSBuildWorkspace</value>
</data>
<data name="CompareSymbolsCorrectlyDescription" xml:space="preserve">
<value>Symbols should be compared for equality, not identity.</value>
<value>Symbols should be compared for equality with an EqualityComparer.</value>
ryzngard marked this conversation as resolved.
Show resolved Hide resolved
</data>
<data name="CompareSymbolsCorrectlyMessage" xml:space="preserve">
<value>Compare symbols correctly</value>
Expand Down
@@ -1,8 +1,11 @@
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Linq;
using System.Collections.Immutable;
using Analyzer.Utilities;
using Analyzer.Utilities.Extensions;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.Diagnostics;
using Microsoft.CodeAnalysis.Operations;

Expand All @@ -16,6 +19,8 @@ public class CompareSymbolsCorrectlyAnalyzer : DiagnosticAnalyzer
private static readonly LocalizableString s_localizableDescription = new LocalizableResourceString(nameof(CodeAnalysisDiagnosticsResources.CompareSymbolsCorrectlyDescription), CodeAnalysisDiagnosticsResources.ResourceManager, typeof(CodeAnalysisDiagnosticsResources));

private static readonly string s_symbolTypeFullName = typeof(ISymbol).FullName;
private const string s_symbolEqualsName = nameof(ISymbol.Equals);
private const string s_symbolEqualityComparerName = "Microsoft.CodeAnalysis.Shared.Utilities.SymbolEquivalenceComparer";

public static readonly DiagnosticDescriptor Rule = new DiagnosticDescriptor(
DiagnosticIds.CompareSymbolsCorrectlyRuleId,
Expand Down Expand Up @@ -43,11 +48,30 @@ public override void Initialize(AnalysisContext context)
return;
}

context.RegisterOperationAction(context => HandleBinaryOperator(in context, symbolType), OperationKind.BinaryOperator);
// Check that the s_symbolEqualityComparerName exists and can be used, otherwise the Roslyn version
// being used it too low to need the change for method references
var symbolEqualityComparerType = context.Compilation.GetTypeByMetadataName(s_symbolEqualityComparerName);
var operatorsToHandle = symbolEqualityComparerType is null ?
new[] { OperationKind.BinaryOperator } :
new[] { OperationKind.BinaryOperator, OperationKind.MethodReference };

context.RegisterOperationAction(context => HandleOperation(in context, symbolType), operatorsToHandle);
});
}

private void HandleBinaryOperator(in OperationAnalysisContext context, INamedTypeSymbol symbolType)
private void HandleOperation(in OperationAnalysisContext context, INamedTypeSymbol symbolType)
{
if (context.Operation is IBinaryOperation)
{
HandleBinaryOperator(context, symbolType);
}
if (context.Operation is IMethodReferenceOperation)
{
HandleMethodReferenceOperation(context, symbolType);
}
}

private static void HandleBinaryOperator(in OperationAnalysisContext context, INamedTypeSymbol symbolType)
{
var binary = (IBinaryOperation)context.Operation;
if (binary.OperatorKind != BinaryOperatorKind.Equals && binary.OperatorKind != BinaryOperatorKind.NotEquals)
Expand Down Expand Up @@ -90,19 +114,27 @@ private void HandleBinaryOperator(in OperationAnalysisContext context, INamedTyp
context.ReportDiagnostic(binary.Syntax.GetLocation().CreateDiagnostic(Rule));
}

private static bool IsSymbolType(IOperation operation, INamedTypeSymbol symbolType)
private static void HandleMethodReferenceOperation(OperationAnalysisContext context, INamedTypeSymbol symbolType)
ryzngard marked this conversation as resolved.
Show resolved Hide resolved
{
if (operation.Type is object)
var methodReference = (IMethodReferenceOperation)context.Operation;

if (methodReference.Instance != null && !IsSymbolType(methodReference.Instance, symbolType))
{
if (operation.Type.Equals(symbolType))
{
return true;
}
return;
}

if (operation.Type.AllInterfaces.Contains(symbolType))
{
return true;
}
var parameters = methodReference.Method.Parameters;
if (methodReference.Method.Name == s_symbolEqualsName && parameters.All(p => IsSymbolType(p.Type, symbolType)))
ryzngard marked this conversation as resolved.
Show resolved Hide resolved
{
context.ReportDiagnostic(methodReference.Syntax.GetLocation().CreateDiagnostic(Rule));
}
}

private static bool IsSymbolType(IOperation operation, INamedTypeSymbol symbolType)
{
if (operation.Type is object && IsSymbolType(operation.Type, symbolType))
{
return true;
}

if (operation is IConversionOperation conversion)
Expand All @@ -113,6 +145,26 @@ private static bool IsSymbolType(IOperation operation, INamedTypeSymbol symbolTy
return false;
}

private static bool IsSymbolType(ITypeSymbol typeSymbol, INamedTypeSymbol symbolType)
{
if (typeSymbol == null)
{
return false;
}

if (typeSymbol.Equals(symbolType))
{
return true;
}

if (typeSymbol.AllInterfaces.Contains(symbolType))
{
return true;
}

return false;
}

private static bool IsSymbolClassType(IOperation operation)
{
if (operation.Type is object)
Expand Down
Expand Up @@ -8,8 +8,8 @@
<note />
</trans-unit>
<trans-unit id="CompareSymbolsCorrectlyDescription">
<source>Symbols should be compared for equality, not identity.</source>
<target state="translated">Symboly by se měly porovnat podle rovnosti, ne podle identity.</target>
<source>Symbols should be compared for equality with an EqualityComparer.</source>
<target state="needs-review-translation">Symboly by se měly porovnat podle rovnosti, ne podle identity.</target>
<note />
</trans-unit>
<trans-unit id="CompareSymbolsCorrectlyMessage">
Expand Down
Expand Up @@ -8,8 +8,8 @@
<note />
</trans-unit>
<trans-unit id="CompareSymbolsCorrectlyDescription">
<source>Symbols should be compared for equality, not identity.</source>
<target state="translated">Symbole sollten hinsichtlich Gleichheit verglichen werden, nicht hinsichtlich Identität.</target>
<source>Symbols should be compared for equality with an EqualityComparer.</source>
<target state="needs-review-translation">Symbole sollten hinsichtlich Gleichheit verglichen werden, nicht hinsichtlich Identität.</target>
<note />
</trans-unit>
<trans-unit id="CompareSymbolsCorrectlyMessage">
Expand Down
Expand Up @@ -8,8 +8,8 @@
<note />
</trans-unit>
<trans-unit id="CompareSymbolsCorrectlyDescription">
<source>Symbols should be compared for equality, not identity.</source>
<target state="translated">Solo se debe realizar una comparación de igualdad de los símbolos, no de identidad.</target>
<source>Symbols should be compared for equality with an EqualityComparer.</source>
<target state="needs-review-translation">Solo se debe realizar una comparación de igualdad de los símbolos, no de identidad.</target>
<note />
</trans-unit>
<trans-unit id="CompareSymbolsCorrectlyMessage">
Expand Down
Expand Up @@ -8,8 +8,8 @@
<note />
</trans-unit>
<trans-unit id="CompareSymbolsCorrectlyDescription">
<source>Symbols should be compared for equality, not identity.</source>
<target state="translated">La comparaison des symboles doit porter sur l'égalité, pas sur l'identité.</target>
<source>Symbols should be compared for equality with an EqualityComparer.</source>
<target state="needs-review-translation">La comparaison des symboles doit porter sur l'égalité, pas sur l'identité.</target>
<note />
</trans-unit>
<trans-unit id="CompareSymbolsCorrectlyMessage">
Expand Down
Expand Up @@ -8,8 +8,8 @@
<note />
</trans-unit>
<trans-unit id="CompareSymbolsCorrectlyDescription">
<source>Symbols should be compared for equality, not identity.</source>
<target state="translated">È necessario confrontare i simboli per verificarne l'uguaglianza, non l'identità.</target>
<source>Symbols should be compared for equality with an EqualityComparer.</source>
<target state="needs-review-translation">È necessario confrontare i simboli per verificarne l'uguaglianza, non l'identità.</target>
<note />
</trans-unit>
<trans-unit id="CompareSymbolsCorrectlyMessage">
Expand Down
Expand Up @@ -8,8 +8,8 @@
<note />
</trans-unit>
<trans-unit id="CompareSymbolsCorrectlyDescription">
<source>Symbols should be compared for equality, not identity.</source>
<target state="translated">シンボルは、ID ではなく等値で比較する必要があります。</target>
<source>Symbols should be compared for equality with an EqualityComparer.</source>
<target state="needs-review-translation">シンボルは、ID ではなく等値で比較する必要があります。</target>
<note />
</trans-unit>
<trans-unit id="CompareSymbolsCorrectlyMessage">
Expand Down
Expand Up @@ -8,8 +8,8 @@
<note />
</trans-unit>
<trans-unit id="CompareSymbolsCorrectlyDescription">
<source>Symbols should be compared for equality, not identity.</source>
<target state="translated">기호는 ID가 아니라 같은지 비교해야 합니다.</target>
<source>Symbols should be compared for equality with an EqualityComparer.</source>
<target state="needs-review-translation">기호는 ID가 아니라 같은지 비교해야 합니다.</target>
<note />
</trans-unit>
<trans-unit id="CompareSymbolsCorrectlyMessage">
Expand Down
Expand Up @@ -8,8 +8,8 @@
<note />
</trans-unit>
<trans-unit id="CompareSymbolsCorrectlyDescription">
<source>Symbols should be compared for equality, not identity.</source>
<target state="translated">Symbole powinny być porównywane pod kątem równości, a nie tożsamości.</target>
<source>Symbols should be compared for equality with an EqualityComparer.</source>
<target state="needs-review-translation">Symbole powinny być porównywane pod kątem równości, a nie tożsamości.</target>
<note />
</trans-unit>
<trans-unit id="CompareSymbolsCorrectlyMessage">
Expand Down
Expand Up @@ -8,8 +8,8 @@
<note />
</trans-unit>
<trans-unit id="CompareSymbolsCorrectlyDescription">
<source>Symbols should be compared for equality, not identity.</source>
<target state="translated">Os símbolos devem ser comparados quanto à igualdade, não à identidade.</target>
<source>Symbols should be compared for equality with an EqualityComparer.</source>
<target state="needs-review-translation">Os símbolos devem ser comparados quanto à igualdade, não à identidade.</target>
<note />
</trans-unit>
<trans-unit id="CompareSymbolsCorrectlyMessage">
Expand Down
Expand Up @@ -8,8 +8,8 @@
<note />
</trans-unit>
<trans-unit id="CompareSymbolsCorrectlyDescription">
<source>Symbols should be compared for equality, not identity.</source>
<target state="translated">Символы необходимо сравнивать на равенство, а не на тождественность.</target>
<source>Symbols should be compared for equality with an EqualityComparer.</source>
<target state="needs-review-translation">Символы необходимо сравнивать на равенство, а не на тождественность.</target>
<note />
</trans-unit>
<trans-unit id="CompareSymbolsCorrectlyMessage">
Expand Down
Expand Up @@ -8,8 +8,8 @@
<note />
</trans-unit>
<trans-unit id="CompareSymbolsCorrectlyDescription">
<source>Symbols should be compared for equality, not identity.</source>
<target state="translated">Semboller kimlik değil eşitlik için karşılaştırılmalıdır.</target>
<source>Symbols should be compared for equality with an EqualityComparer.</source>
<target state="needs-review-translation">Semboller kimlik değil eşitlik için karşılaştırılmalıdır.</target>
<note />
</trans-unit>
<trans-unit id="CompareSymbolsCorrectlyMessage">
Expand Down
Expand Up @@ -8,8 +8,8 @@
<note />
</trans-unit>
<trans-unit id="CompareSymbolsCorrectlyDescription">
<source>Symbols should be compared for equality, not identity.</source>
<target state="translated">符号应比较相等性,而不是标识。</target>
<source>Symbols should be compared for equality with an EqualityComparer.</source>
<target state="needs-review-translation">符号应比较相等性,而不是标识。</target>
<note />
</trans-unit>
<trans-unit id="CompareSymbolsCorrectlyMessage">
Expand Down
Expand Up @@ -8,8 +8,8 @@
<note />
</trans-unit>
<trans-unit id="CompareSymbolsCorrectlyDescription">
<source>Symbols should be compared for equality, not identity.</source>
<target state="translated">應比較符號是否相等,而非比較身分識別。</target>
<source>Symbols should be compared for equality with an EqualityComparer.</source>
<target state="needs-review-translation">應比較符號是否相等,而非比較身分識別。</target>
<note />
</trans-unit>
<trans-unit id="CompareSymbolsCorrectlyMessage">
Expand Down