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.SymbolEqualityComparer";

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

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

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(in context, symbolType);
}
else if (context.Operation is IInvocationOperation)
{
HandleInvocationOperation(in 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 +112,32 @@ 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 HandleInvocationOperation(in OperationAnalysisContext context, INamedTypeSymbol symbolType)
{
if (operation.Type is object)
var invocationOperation = (IInvocationOperation)context.Operation;
var method = invocationOperation.TargetMethod;
if (method.Name != s_symbolEqualsName)
{
if (operation.Type.Equals(symbolType))
{
return true;
}
return;
}

if (operation.Type.AllInterfaces.Contains(symbolType))
{
return true;
}
if (invocationOperation.Instance != null && !IsSymbolType(invocationOperation.Instance, symbolType))
{
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

if (parameters.All(p => IsSymbolType(p.Value, symbolType)))
{
context.ReportDiagnostic(invocationOperation.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 +148,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 @@ -146,5 +201,8 @@ private static bool IsExplicitCastToObject(IOperation operation)

return conversion.Type?.SpecialType == SpecialType.System_Object;
}

public static bool UseSymbolEqualityComparer(Compilation compilation)
=> compilation.GetTypeByMetadataName(SymbolEqualityComparerName) is object;
}
}
Expand Up @@ -2,6 +2,7 @@

using System.Collections.Immutable;
using System.Composition;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis.CodeActions;
Expand Down Expand Up @@ -41,27 +42,82 @@ 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),
IInvocationOperation invocationOperation => await EnsureEqualsCorrectAsync(document, semanticModel, invocationOperation, cancellationToken).ConfigureAwait(false),
_ => document
};
}

private static async Task<Document> EnsureEqualsCorrectAsync(Document document, SemanticModel semanticModel, IInvocationOperation invocationOperation, CancellationToken cancellationToken)
{
if (!CompareSymbolsCorrectlyAnalyzer.UseSymbolEqualityComparer(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)
if (invocationOperation.Instance is null)
{
var replacement = generator.InvocationExpression(
GetEqualityComparerDefaultEquals(generator),
invocationOperation.Arguments.Select(argument => argument.Syntax).ToImmutableArray());

editor.ReplaceNode(invocationOperation.Syntax, replacement.WithTriviaFrom(invocationOperation.Syntax));
}
else
{
var replacement = generator.AddParameters(invocationOperation.Syntax, new[] { GetEqualityComparerDefault(generator) });
editor.ReplaceNode(invocationOperation.Syntax, replacement.WithTriviaFrom(invocationOperation.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 = CompareSymbolsCorrectlyAnalyzer.UseSymbolEqualityComparer(semanticModel.Compilation) switch
{
true =>
generator.InvocationExpression(
GetEqualityComparerDefaultEquals(generator),
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 SyntaxNode GetEqualityComparerDefaultEquals(SyntaxGenerator generator)
=> generator.MemberAccessExpression(
GetEqualityComparerDefault(generator),
nameof(object.Equals));

private static SyntaxNode GetEqualityComparerDefault(SyntaxGenerator generator)
=> generator.MemberAccessExpression(generator.DottedName(CompareSymbolsCorrectlyAnalyzer.SymbolEqualityComparerName), "Default");
}
}
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