Merge pull request #72522 from CyrusNajmabadi/redundantOperators

CyrusNajmabadi · web-flow · commit 291992806168 · 2024-03-13T08:09:43.000-07:00
diff --git a/src/Analyzers/CSharp/Tests/RemoveRedundantEquality/RemoveRedundantEqualityTests.cs b/src/Analyzers/CSharp/Tests/RemoveRedundantEquality/RemoveRedundantEqualityTests.cs
@@ -44,16 +44,23 @@ public bool M1(bool x)
         [Fact]
         public async Task TestSimpleCaseForEqualsFalse_NoDiagnostics()
         {
-            var code = """
+            await VerifyCS.VerifyCodeFixAsync("""
                 public class C
                 {
                     public bool M1(bool x)
                     {
-                        return x == false;
+                        return x [|==|] false;
                     }
                 }
-                """;
-            await VerifyCS.VerifyAnalyzerAsync(code);
+                """, """
+                public class C
+                {
+                    public bool M1(bool x)
+                    {
+                        return !x;
+                    }
+                }
+                """);
         }
 
         [Fact]
@@ -83,16 +90,23 @@ public bool M1(bool x)
         [Fact]
         public async Task TestSimpleCaseForNotEqualsTrue_NoDiagnostics()
         {
-            var code = """
+            await VerifyCS.VerifyCodeFixAsync("""
                 public class C
                 {
                     public bool M1(bool x)
                     {
-                        return x != true;
+                        return x [|!=|] true;
                     }
                 }
-                """;
-            await VerifyCS.VerifyAnalyzerAsync(code);
+                """, """
+                public class C
+                {
+                    public bool M1(bool x)
+                    {
+                        return !x;
+                    }
+                }
+                """);
         }
 
         [Fact]
diff --git a/src/Analyzers/Core/Analyzers/RemoveRedundantEquality/AbstractRemoveRedundantEqualityDiagnosticAnalyzer.cs b/src/Analyzers/Core/Analyzers/RemoveRedundantEquality/AbstractRemoveRedundantEqualityDiagnosticAnalyzer.cs
@@ -10,20 +10,13 @@
 
 namespace Microsoft.CodeAnalysis.RemoveRedundantEquality;
 
-internal abstract class AbstractRemoveRedundantEqualityDiagnosticAnalyzer
-    : AbstractBuiltInCodeStyleDiagnosticAnalyzer
+internal abstract class AbstractRemoveRedundantEqualityDiagnosticAnalyzer(ISyntaxFacts syntaxFacts)
+    : AbstractBuiltInCodeStyleDiagnosticAnalyzer(
+        IDEDiagnosticIds.RemoveRedundantEqualityDiagnosticId,
+        EnforceOnBuildValues.RemoveRedundantEquality,
+        option: null,
+        new LocalizableResourceString(nameof(AnalyzersResources.Remove_redundant_equality), AnalyzersResources.ResourceManager, typeof(AnalyzersResources)))
 {
-    private readonly ISyntaxFacts _syntaxFacts;
-
-    protected AbstractRemoveRedundantEqualityDiagnosticAnalyzer(ISyntaxFacts syntaxFacts)
-        : base(IDEDiagnosticIds.RemoveRedundantEqualityDiagnosticId,
-               EnforceOnBuildValues.RemoveRedundantEquality,
-               option: null,
-               new LocalizableResourceString(nameof(AnalyzersResources.Remove_redundant_equality), AnalyzersResources.ResourceManager, typeof(AnalyzersResources)))
-    {
-        _syntaxFacts = syntaxFacts;
-    }
-
     public override DiagnosticAnalyzerCategory GetAnalyzerCategory()
         => DiagnosticAnalyzerCategory.SemanticSpanAnalysis;
 
@@ -35,19 +28,15 @@ private void AnalyzeBinaryOperator(OperationAnalysisContext context)
         if (ShouldSkipAnalysis(context, notification: null))
             return;
 
+        // We shouldn't report diagnostic on overloaded operator as the behavior can change.
         var operation = (IBinaryOperation)context.Operation;
         if (operation.OperatorMethod is not null)
-        {
-            // We shouldn't report diagnostic on overloaded operator as the behavior can change.
             return;
-        }
 
         if (operation.OperatorKind is not (BinaryOperatorKind.Equals or BinaryOperatorKind.NotEquals))
-        {
             return;
-        }
 
-        if (!_syntaxFacts.IsBinaryExpression(operation.Syntax))
+        if (!syntaxFacts.IsBinaryExpression(operation.Syntax))
         {
             return;
         }
@@ -56,9 +45,7 @@ private void AnalyzeBinaryOperator(OperationAnalysisContext context)
         var leftOperand = operation.LeftOperand;
 
         if (rightOperand.Type is null || leftOperand.Type is null)
-        {
             return;
-        }
 
         if (rightOperand.Type.SpecialType != SpecialType.System_Boolean ||
             leftOperand.Type.SpecialType != SpecialType.System_Boolean)
@@ -67,34 +54,43 @@ private void AnalyzeBinaryOperator(OperationAnalysisContext context)
         }
 
         var isOperatorEquals = operation.OperatorKind == BinaryOperatorKind.Equals;
-        _syntaxFacts.GetPartsOfBinaryExpression(operation.Syntax, out _, out var operatorToken, out _);
+        syntaxFacts.GetPartsOfBinaryExpression(operation.Syntax, out _, out var operatorToken, out _);
+
         var properties = ImmutableDictionary.CreateBuilder<string, string?>();
-        if (TryGetLiteralValue(rightOperand) == isOperatorEquals)
+        if (TryGetLiteralValue(rightOperand) is bool rightBool)
         {
             properties.Add(RedundantEqualityConstants.RedundantSide, RedundantEqualityConstants.Right);
+            if (rightBool != isOperatorEquals)
+                properties.Add(RedundantEqualityConstants.Negate, RedundantEqualityConstants.Negate);
         }
-        else if (TryGetLiteralValue(leftOperand) == isOperatorEquals)
+        else if (TryGetLiteralValue(leftOperand) is bool leftBool)
         {
             properties.Add(RedundantEqualityConstants.RedundantSide, RedundantEqualityConstants.Left);
+            if (leftBool != isOperatorEquals)
+                properties.Add(RedundantEqualityConstants.Negate, RedundantEqualityConstants.Negate);
         }
-
-        if (properties.Count == 1)
+        else
         {
-            context.ReportDiagnostic(Diagnostic.Create(Descriptor,
-                operatorToken.GetLocation(),
-                additionalLocations: [operation.Syntax.GetLocation()],
-                properties: properties.ToImmutable()));
+            return;
         }
 
+        context.ReportDiagnostic(Diagnostic.Create(Descriptor,
+            operatorToken.GetLocation(),
+            additionalLocations: [operation.Syntax.GetLocation()],
+            properties: properties.ToImmutable()));
+
         return;
 
         static bool? TryGetLiteralValue(IOperation operand)
         {
             // Make sure we only simplify literals to avoid changing
             // something like the following example:
             // const bool Activated = true; ... if (state == Activated)
-            if (operand.ConstantValue.HasValue && operand.Kind == OperationKind.Literal &&
-                operand.ConstantValue.Value is bool constValue)
+            if (operand is
+                {
+                    Kind: OperationKind.Literal,
+                    ConstantValue: { HasValue: true, Value: bool constValue }
+                })
             {
                 return constValue;
             }
diff --git a/src/Analyzers/Core/Analyzers/RemoveRedundantEquality/RedundantEqualityConstants.cs b/src/Analyzers/Core/Analyzers/RemoveRedundantEquality/RedundantEqualityConstants.cs
@@ -9,4 +9,5 @@ internal static class RedundantEqualityConstants
     public const string RedundantSide = nameof(RedundantSide);
     public const string Left = nameof(Left);
     public const string Right = nameof(Right);
+    public const string Negate = nameof(Negate);
 }
diff --git a/src/Analyzers/Core/CodeFixes/RemoveRedundantEquality/RemoveRedundantEqualityCodeFixProvider.cs b/src/Analyzers/Core/CodeFixes/RemoveRedundantEquality/RemoveRedundantEqualityCodeFixProvider.cs
@@ -21,54 +21,49 @@ namespace Microsoft.CodeAnalysis.RemoveRedundantEquality;
 [ExportCodeFixProvider(LanguageNames.CSharp, LanguageNames.VisualBasic, Name = PredefinedCodeFixProviderNames.RemoveRedundantEquality), Shared]
 [method: ImportingConstructor]
 [method: SuppressMessage("RoslynDiagnosticsReliability", "RS0033:Importing constructor should be [Obsolete]", Justification = "Used in test code: https://github.com/dotnet/roslyn/issues/42814")]
-internal sealed class RemoveRedundantEqualityCodeFixProvider() : SyntaxEditorBasedCodeFixProvider
+internal sealed class RemoveRedundantEqualityCodeFixProvider() : ForkingSyntaxEditorBasedCodeFixProvider<SyntaxNode>
 {
     public override ImmutableArray<string> FixableDiagnosticIds => [IDEDiagnosticIds.RemoveRedundantEqualityDiagnosticId];
 
-    public override Task RegisterCodeFixesAsync(CodeFixContext context)
-    {
-        foreach (var diagnostic in context.Diagnostics)
-        {
-            RegisterCodeFix(context, AnalyzersResources.Remove_redundant_equality, nameof(AnalyzersResources.Remove_redundant_equality), diagnostic);
-        }
+    protected override (string title, string equivalenceKey) GetTitleAndEquivalenceKey(CodeFixContext context)
+        => (AnalyzersResources.Remove_redundant_equality, nameof(AnalyzersResources.Remove_redundant_equality));
 
-        return Task.CompletedTask;
-    }
-
-    protected override async Task FixAllAsync(Document document, ImmutableArray<Diagnostic> diagnostics, SyntaxEditor editor, CodeActionOptionsProvider fallbackOptions, CancellationToken cancellationToken)
+    protected override async Task FixAsync(
+        Document document,
+        SyntaxEditor editor,
+        CodeActionOptionsProvider fallbackOptions,
+        SyntaxNode node,
+        ImmutableDictionary<string, string?> properties,
+        CancellationToken cancellationToken)
     {
         var root = await document.GetRequiredSyntaxRootAsync(cancellationToken).ConfigureAwait(false);
+        var generator = editor.Generator;
+        var generatorInternal = document.GetRequiredLanguageService<SyntaxGeneratorInternal>();
+
         var syntaxFacts = document.GetRequiredLanguageService<ISyntaxFactsService>();
-        var semanticModel = await document.GetSemanticModelAsync(cancellationToken).ConfigureAwait(false);
+        var semanticModel = await document.GetRequiredSemanticModelAsync(cancellationToken).ConfigureAwait(false);
 
-        foreach (var diagnostic in diagnostics)
+        editor.ReplaceNode(node, WithElasticTrailingTrivia(RewriteNode()));
+
+        return;
+
+        SyntaxNode RewriteNode()
         {
-            var node = root.FindNode(diagnostic.AdditionalLocations[0].SourceSpan, getInnermostNodeForTie: true);
+            // This should happen only in error cases.
+            if (!syntaxFacts.IsBinaryExpression(node))
+                return node;
 
-            editor.ReplaceNode(node, (n, _) =>
-            {
-                if (!syntaxFacts.IsBinaryExpression(n))
-                {
-                    // This should happen only in error cases.
-                    return n;
-                }
+            syntaxFacts.GetPartsOfBinaryExpression(node, out var left, out var right);
+            var rewritten =
+                properties[RedundantEqualityConstants.RedundantSide] == RedundantEqualityConstants.Right ? left :
+                properties[RedundantEqualityConstants.RedundantSide] == RedundantEqualityConstants.Left ? right : node;
 
-                syntaxFacts.GetPartsOfBinaryExpression(n, out var left, out var right);
-                if (diagnostic.Properties[RedundantEqualityConstants.RedundantSide] == RedundantEqualityConstants.Right)
-                {
-                    return WithElasticTrailingTrivia(left);
-                }
-                else if (diagnostic.Properties[RedundantEqualityConstants.RedundantSide] == RedundantEqualityConstants.Left)
-                {
-                    return WithElasticTrailingTrivia(right);
-                }
+            if (properties.ContainsKey(RedundantEqualityConstants.Negate))
+                rewritten = generator.Negate(generatorInternal, rewritten, semanticModel, cancellationToken);
 
-                return n;
-            });
+            return rewritten;
         }
 
-        return;
-
         static SyntaxNode WithElasticTrailingTrivia(SyntaxNode node)
         {
             return node.WithTrailingTrivia(node.GetTrailingTrivia().Select(SyntaxTriviaExtensions.AsElastic));
diff --git a/src/Analyzers/VisualBasic/Tests/RemoveRedundantEquality/RemoveRedundantEqualityTests.vb b/src/Analyzers/VisualBasic/Tests/RemoveRedundantEquality/RemoveRedundantEqualityTests.vb
@@ -29,14 +29,19 @@ End Module
 
         <Fact>
         Public Async Function TestSimpleCaseForEqualsFalse_NoDiagnostics() As Task
-            Dim code = "
+            Await VerifyVB.VerifyCodeFixAsync("
 Public Module Module1
     Public Function M1(x As Boolean) As Boolean
-        Return x = False
+        Return x [|=|] False
     End Function
 End Module
-"
-            Await VerifyVB.VerifyAnalyzerAsync(code)
+", "
+Public Module Module1
+    Public Function M1(x As Boolean) As Boolean
+        Return Not x
+    End Function
+End Module
+")
         End Function
 
         <Fact>
@@ -60,14 +65,19 @@ End Module
 
         <Fact>
         Public Async Function TestSimpleCaseForNotEqualsTrue_NoDiagnostics() As Task
-            Dim code = "
+            Await VerifyVB.VerifyCodeFixAsync("
 Public Module Module1
     Public Function M1(x As Boolean) As Boolean
-        Return x <> True
+        Return x [|<>|] True
     End Function
 End Module
-"
-            Await VerifyVB.VerifyAnalyzerAsync(code)
+", "
+Public Module Module1
+    Public Function M1(x As Boolean) As Boolean
+        Return Not x
+    End Function
+End Module
+")
         End Function
 
         <Fact>

Original file line number	Diff line number	Diff line change
`@@ -44,16 +44,23 @@ public bool M1(bool x)`
`44`	`44`	`[Fact]`
`45`	`45`	`public async Task TestSimpleCaseForEqualsFalse_NoDiagnostics()`
`46`	`46`	`{`
`47`		`- var code = """`
	`47`	`+ await VerifyCS.VerifyCodeFixAsync("""`
`48`	`48`	`public class C`
`49`	`49`	`{`
`50`	`50`	`public bool M1(bool x)`
`51`	`51`	`{`
`52`		`- return x == false;`
	`52`	`+ return x [\|==\|] false;`
`53`	`53`	`}`
`54`	`54`	`}`
`55`		`- """;`
`56`		`- await VerifyCS.VerifyAnalyzerAsync(code);`
	`55`	`+ """, """`
	`56`	`+ public class C`
	`57`	`+ {`
	`58`	`+ public bool M1(bool x)`
	`59`	`+ {`
	`60`	`+ return !x;`
	`61`	`+ }`
	`62`	`+ }`
	`63`	`+ """);`
`57`	`64`	`}`
`58`	`65`
`59`	`66`	`[Fact]`
`@@ -83,16 +90,23 @@ public bool M1(bool x)`
`83`	`90`	`[Fact]`
`84`	`91`	`public async Task TestSimpleCaseForNotEqualsTrue_NoDiagnostics()`
`85`	`92`	`{`
`86`		`- var code = """`
	`93`	`+ await VerifyCS.VerifyCodeFixAsync("""`
`87`	`94`	`public class C`
`88`	`95`	`{`
`89`	`96`	`public bool M1(bool x)`
`90`	`97`	`{`
`91`		`- return x != true;`
	`98`	`+ return x [\|!=\|] true;`
`92`	`99`	`}`
`93`	`100`	`}`
`94`		`- """;`
`95`		`- await VerifyCS.VerifyAnalyzerAsync(code);`
	`101`	`+ """, """`
	`102`	`+ public class C`
	`103`	`+ {`
	`104`	`+ public bool M1(bool x)`
	`105`	`+ {`
	`106`	`+ return !x;`
	`107`	`+ }`
	`108`	`+ }`
	`109`	`+ """);`
`96`	`110`	`}`
`97`	`111`
`98`	`112`	`[Fact]`
Original file line number	Diff line number	Diff line change
`@@ -9,4 +9,5 @@ internal static class RedundantEqualityConstants`
`9`	`9`	`public const string RedundantSide = nameof(RedundantSide);`
`10`	`10`	`public const string Left = nameof(Left);`
`11`	`11`	`public const string Right = nameof(Right);`
	`12`	`+ public const string Negate = nameof(Negate);`
`12`	`13`	`}`