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
@@ -1,8 +1,10 @@
// 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.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 +18,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);
public const string SymbolEqualityComparerName = "Microsoft.CodeAnalysis.Shared.Utilities.SymbolEquivalenceComparer";

public static readonly DiagnosticDescriptor Rule = new DiagnosticDescriptor(
DiagnosticIds.CompareSymbolsCorrectlyRuleId,
Expand Down Expand Up @@ -43,11 +47,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(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 +113,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 +144,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 @@ -16,6 +16,8 @@ namespace Microsoft.CodeAnalysis.Analyzers.MetaAnalyzers.Fixers
[Shared]
public class CompareSymbolsCorrectlyFix : CodeFixProvider
{
private const string s_equalityComparerIdentifier = "Microsoft.CodeAnalysis.SymbolEqualityComparer";

public override ImmutableArray<string> FixableDiagnosticIds => ImmutableArray.Create(CompareSymbolsCorrectlyAnalyzer.Rule.Id);

public override FixAllProvider GetFixAllProvider()
Expand All @@ -41,27 +43,71 @@ private async Task<Document> ConvertToEqualsAsync(Document document, TextSpan so
var semanticModel = await document.GetSemanticModelAsync(cancellationToken).ConfigureAwait(false);
var root = await document.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false);
var expression = root.FindNode(sourceSpan, getInnermostNodeForTie: true);
if (!(semanticModel.GetOperation(expression, cancellationToken) is IBinaryOperation operation))
var rawOperation = semanticModel.GetOperation(expression, cancellationToken);

return rawOperation switch
{
IBinaryOperation binaryOperation => await ConvertToEqualsAsync(document, semanticModel, binaryOperation, cancellationToken).ConfigureAwait(false),
IMethodReferenceOperation methodReferenceOperation => await EnsureEqualsCorrectAsync(document, semanticModel, methodReferenceOperation, cancellationToken).ConfigureAwait(false),
_ => document
};
}

private static async Task<Document> EnsureEqualsCorrectAsync(Document document, SemanticModel semanticModel, IMethodReferenceOperation methodReference, CancellationToken cancellationToken)
{
if (!UseEqualityComparer(semanticModel.Compilation))
{
return document;
}

var editor = await DocumentEditor.CreateAsync(document, cancellationToken).ConfigureAwait(false);
var generator = editor.Generator;

var replacement = generator.InvocationExpression(
generator.MemberAccessExpression(
generator.TypeExpression(semanticModel.Compilation.GetSpecialType(SpecialType.System_Object)),
nameof(object.Equals)),
operation.LeftOperand.Syntax.WithoutLeadingTrivia(),
operation.RightOperand.Syntax.WithoutTrailingTrivia());
if (operation.OperatorKind == BinaryOperatorKind.NotEquals)
var replacement = generator.AddParameters(methodReference.Syntax, new[] { generator.IdentifierName(s_equalityComparerIdentifier) });
editor.ReplaceNode(methodReference.Syntax, replacement.WithTriviaFrom(methodReference.Syntax));
return editor.GetChangedDocument();
}

private static async Task<Document> ConvertToEqualsAsync(Document document, SemanticModel semanticModel, IBinaryOperation binaryOperation, CancellationToken cancellationToken)
{

var expression = binaryOperation.Syntax;
var editor = await DocumentEditor.CreateAsync(document, cancellationToken).ConfigureAwait(false);
var generator = editor.Generator;

var replacement = UseEqualityComparer(semanticModel.Compilation) switch
{
true =>
generator.InvocationExpression(
generator.MemberAccessExpression(
generator.MemberAccessExpression(
generator.DottedName(s_equalityComparerIdentifier),
"Default"),
nameof(object.Equals)),
binaryOperation.LeftOperand.Syntax.WithoutLeadingTrivia(),
binaryOperation.RightOperand.Syntax.WithoutTrailingTrivia()),

false =>
generator.InvocationExpression(
generator.MemberAccessExpression(
generator.TypeExpression(semanticModel.Compilation.GetSpecialType(SpecialType.System_Object)),
nameof(object.Equals)),
binaryOperation.LeftOperand.Syntax.WithoutLeadingTrivia(),
binaryOperation.RightOperand.Syntax.WithoutTrailingTrivia())
};

if (binaryOperation.OperatorKind == BinaryOperatorKind.NotEquals)
{
replacement = generator.LogicalNotExpression(replacement);
}

editor.ReplaceNode(expression, replacement.WithTriviaFrom(expression));
return editor.GetChangedDocument();
}

private static bool UseEqualityComparer(Compilation compilation)
{
return compilation.GetTypeByMetadataName(s_equalityComparerIdentifier) is object;
}
}
}
Expand Up @@ -9,7 +9,7 @@
</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>
<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 @@ -9,7 +9,7 @@
</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>
<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 @@ -9,7 +9,7 @@
</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>
<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 @@ -9,7 +9,7 @@
</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>
<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 @@ -9,7 +9,7 @@
</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>
<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 @@ -9,7 +9,7 @@
</trans-unit>
<trans-unit id="CompareSymbolsCorrectlyDescription">
<source>Symbols should be compared for equality, not identity.</source>
<target state="translated">シンボルは、ID ではなく等値で比較する必要があります。</target>
<target state="needs-review-translation">シンボルは、ID ではなく等値で比較する必要があります。</target>
<note />
</trans-unit>
<trans-unit id="CompareSymbolsCorrectlyMessage">
Expand Down
Expand Up @@ -9,7 +9,7 @@
</trans-unit>
<trans-unit id="CompareSymbolsCorrectlyDescription">
<source>Symbols should be compared for equality, not identity.</source>
<target state="translated">기호는 ID가 아니라 같은지 비교해야 합니다.</target>
<target state="needs-review-translation">기호는 ID가 아니라 같은지 비교해야 합니다.</target>
<note />
</trans-unit>
<trans-unit id="CompareSymbolsCorrectlyMessage">
Expand Down
Expand Up @@ -9,7 +9,7 @@
</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>
<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 @@ -9,7 +9,7 @@
</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>
<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 @@ -9,7 +9,7 @@
</trans-unit>
<trans-unit id="CompareSymbolsCorrectlyDescription">
<source>Symbols should be compared for equality, not identity.</source>
<target state="translated">Символы необходимо сравнивать на равенство, а не на тождественность.</target>
<target state="needs-review-translation">Символы необходимо сравнивать на равенство, а не на тождественность.</target>
<note />
</trans-unit>
<trans-unit id="CompareSymbolsCorrectlyMessage">
Expand Down
Expand Up @@ -9,7 +9,7 @@
</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>
<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 @@ -9,7 +9,7 @@
</trans-unit>
<trans-unit id="CompareSymbolsCorrectlyDescription">
<source>Symbols should be compared for equality, not identity.</source>
<target state="translated">符号应比较相等性,而不是标识。</target>
<target state="needs-review-translation">符号应比较相等性,而不是标识。</target>
<note />
</trans-unit>
<trans-unit id="CompareSymbolsCorrectlyMessage">
Expand Down
Expand Up @@ -9,7 +9,7 @@
</trans-unit>
<trans-unit id="CompareSymbolsCorrectlyDescription">
<source>Symbols should be compared for equality, not identity.</source>
<target state="translated">應比較符號是否相等,而非比較身分識別。</target>
<target state="needs-review-translation">應比較符號是否相等,而非比較身分識別。</target>
<note />
</trans-unit>
<trans-unit id="CompareSymbolsCorrectlyMessage">
Expand Down