From 162adf8c3d873157f65fff73b653bc75f26dab0b Mon Sep 17 00:00:00 2001 From: Zsolt Kolbay Date: Mon, 22 Jan 2024 12:51:07 +0100 Subject: [PATCH 1/3] Fix FP for object types --- .../Rules/BooleanLiteralUnnecessaryBase.cs | 13 ++++++++----- .../TestCases/BooleanLiteralUnnecessary.Fixed.cs | 10 ++++++++++ .../TestCases/BooleanLiteralUnnecessary.cs | 10 ++++++++++ .../TestCases/BooleanLiteralUnnecessary.vb | 14 ++++++++++++-- 4 files changed, 40 insertions(+), 7 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.Common/Rules/BooleanLiteralUnnecessaryBase.cs b/analyzers/src/SonarAnalyzer.Common/Rules/BooleanLiteralUnnecessaryBase.cs index 7c31c1dcd0c..a308f2ec370 100644 --- a/analyzers/src/SonarAnalyzer.Common/Rules/BooleanLiteralUnnecessaryBase.cs +++ b/analyzers/src/SonarAnalyzer.Common/Rules/BooleanLiteralUnnecessaryBase.cs @@ -145,7 +145,9 @@ protected bool CheckForNullabilityAndBooleanConstantsReport(SonarSyntaxNodeRepor var left = Language.Syntax.RemoveParentheses(GetLeftNode(node)); var right = Language.Syntax.RemoveParentheses(GetRightNode(node)); - if (CheckForNullability(left, right, context.SemanticModel)) + if (right is null // Avoids DeclarationPattern or RecursivePattern + || TypeShouldBeIgnored(left, context.SemanticModel) + || TypeShouldBeIgnored(right, context.SemanticModel)) { return true; } @@ -205,9 +207,10 @@ private Location CalculateExtendedLocation(SyntaxNode parent, bool isLeftSide) return isLeftSide ? parent.CreateLocation(token) : token.CreateLocation(parent); } - private static bool CheckForNullability(SyntaxNode left, SyntaxNode right, SemanticModel model) => - right is null // Avoids DeclarationPattern or RecursivePattern - || model.GetTypeInfo(left).Type.IsNullableBoolean() - || model.GetTypeInfo(right).Type.IsNullableBoolean(); + private static bool TypeShouldBeIgnored(SyntaxNode node, SemanticModel model) + { + var type = model.GetTypeInfo(node).Type; + return type.IsNullableBoolean() || type.Is(KnownType.System_Object); + } } } diff --git a/analyzers/tests/SonarAnalyzer.Test/TestCases/BooleanLiteralUnnecessary.Fixed.cs b/analyzers/tests/SonarAnalyzer.Test/TestCases/BooleanLiteralUnnecessary.Fixed.cs index b4208487713..27409b12aea 100644 --- a/analyzers/tests/SonarAnalyzer.Test/TestCases/BooleanLiteralUnnecessary.Fixed.cs +++ b/analyzers/tests/SonarAnalyzer.Test/TestCases/BooleanLiteralUnnecessary.Fixed.cs @@ -214,4 +214,14 @@ void Method(bool cond) if (cond) { } // Fixed } } + + // https://github.com/SonarSource/sonar-dotnet/issues/7792 + class ObjectIsBool + { + void Method(object obj, Exception exc) + { + if (obj is true) { } + if (exc.Data["SomeKey"] is true) { } + } + } } diff --git a/analyzers/tests/SonarAnalyzer.Test/TestCases/BooleanLiteralUnnecessary.cs b/analyzers/tests/SonarAnalyzer.Test/TestCases/BooleanLiteralUnnecessary.cs index 221c6e56ce7..1a7c3ad4f9b 100644 --- a/analyzers/tests/SonarAnalyzer.Test/TestCases/BooleanLiteralUnnecessary.cs +++ b/analyzers/tests/SonarAnalyzer.Test/TestCases/BooleanLiteralUnnecessary.cs @@ -231,4 +231,14 @@ void Method(bool cond) if (cond == false) { } // Noncompliant, TP but code fix is wrong - it should be fixed to "if (!cond)" } } + + // https://github.com/SonarSource/sonar-dotnet/issues/7792 + class ObjectIsBool + { + void Method(object obj, Exception exc) + { + if (obj is true) { } + if (exc.Data["SomeKey"] is true) { } + } + } } diff --git a/analyzers/tests/SonarAnalyzer.Test/TestCases/BooleanLiteralUnnecessary.vb b/analyzers/tests/SonarAnalyzer.Test/TestCases/BooleanLiteralUnnecessary.vb index 70ae9e3725b..57967da10ba 100644 --- a/analyzers/tests/SonarAnalyzer.Test/TestCases/BooleanLiteralUnnecessary.vb +++ b/analyzers/tests/SonarAnalyzer.Test/TestCases/BooleanLiteralUnnecessary.vb @@ -182,8 +182,8 @@ Public Class BooleanLiteralUnnecessary End Sub Function BooleanMethod() As Boolean - Return True - End Function + Return True + End Function End Class @@ -220,3 +220,13 @@ Public Class Repro Return True End Function End Class + +' https://github.com/SonarSource/sonar-dotnet/issues/7792 +Class ObjectIsBool + Sub Method(obj As Object, exc As Exception) + If obj = True Then + End If + If exc.Data("SomeKey") = True Then + End If + End Sub +End Class From c71eb60ed20bdbd94ccb9e7f48c4dbfce8f43b9a Mon Sep 17 00:00:00 2001 From: Zsolt Kolbay Date: Tue, 23 Jan 2024 01:17:16 +0100 Subject: [PATCH 2/3] Review --- .../Rules/BooleanLiteralUnnecessaryBase.cs | 10 ++++++++-- .../BooleanLiteralUnnecessary.CSharp8.Fixed.cs | 13 +++++++++++++ .../TestCases/BooleanLiteralUnnecessary.CSharp8.cs | 13 +++++++++++++ .../TestCases/BooleanLiteralUnnecessary.Fixed.cs | 10 +++++++++- .../TestCases/BooleanLiteralUnnecessary.cs | 10 +++++++++- 5 files changed, 52 insertions(+), 4 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.Common/Rules/BooleanLiteralUnnecessaryBase.cs b/analyzers/src/SonarAnalyzer.Common/Rules/BooleanLiteralUnnecessaryBase.cs index a308f2ec370..f4384fe1c33 100644 --- a/analyzers/src/SonarAnalyzer.Common/Rules/BooleanLiteralUnnecessaryBase.cs +++ b/analyzers/src/SonarAnalyzer.Common/Rules/BooleanLiteralUnnecessaryBase.cs @@ -25,6 +25,8 @@ public abstract class BooleanLiteralUnnecessaryBase : SonarDiagnost { internal const string DiagnosticId = "S1125"; + private INamedTypeSymbol[] systemBooleanInterfaces; + protected enum ErrorLocation { // The BooleanLiteral node is highlighted @@ -207,10 +209,14 @@ private Location CalculateExtendedLocation(SyntaxNode parent, bool isLeftSide) return isLeftSide ? parent.CreateLocation(token) : token.CreateLocation(parent); } - private static bool TypeShouldBeIgnored(SyntaxNode node, SemanticModel model) + private bool TypeShouldBeIgnored(SyntaxNode node, SemanticModel model) { var type = model.GetTypeInfo(node).Type; - return type.IsNullableBoolean() || type.Is(KnownType.System_Object); + systemBooleanInterfaces ??= model.Compilation.GetTypeByMetadataName(KnownType.System_Boolean).Interfaces.ToArray(); + return type.IsNullableBoolean() + || type is ITypeParameterSymbol + || type.Is(KnownType.System_Object) + || Array.Exists(systemBooleanInterfaces, x => x.Equals(type)); } } } diff --git a/analyzers/tests/SonarAnalyzer.Test/TestCases/BooleanLiteralUnnecessary.CSharp8.Fixed.cs b/analyzers/tests/SonarAnalyzer.Test/TestCases/BooleanLiteralUnnecessary.CSharp8.Fixed.cs index 99bd2e5e30d..3b36b942a77 100644 --- a/analyzers/tests/SonarAnalyzer.Test/TestCases/BooleanLiteralUnnecessary.CSharp8.Fixed.cs +++ b/analyzers/tests/SonarAnalyzer.Test/TestCases/BooleanLiteralUnnecessary.CSharp8.Fixed.cs @@ -18,4 +18,17 @@ public void Foo(string key) var x = (key is null) ? throw new ArgumentNullException(nameof(key)) : false; } } + + // https://github.com/SonarSource/sonar-dotnet/issues/7792 + class ConvertibleGenericTypes + { + void ConvertibleToBool(T1 unconstrained, T2 constrainedToStruct, T3 constrainedToBoolInterface) + where T2 : struct + where T3 : IComparable + { + if (unconstrained is true) { } + if (constrainedToStruct is true) { } + if (constrainedToBoolInterface is true) { } + } + } } diff --git a/analyzers/tests/SonarAnalyzer.Test/TestCases/BooleanLiteralUnnecessary.CSharp8.cs b/analyzers/tests/SonarAnalyzer.Test/TestCases/BooleanLiteralUnnecessary.CSharp8.cs index 6d81f970c4d..c50278b0fc6 100644 --- a/analyzers/tests/SonarAnalyzer.Test/TestCases/BooleanLiteralUnnecessary.CSharp8.cs +++ b/analyzers/tests/SonarAnalyzer.Test/TestCases/BooleanLiteralUnnecessary.CSharp8.cs @@ -18,4 +18,17 @@ public void Foo(string key) var x = (key is null) ? throw new ArgumentNullException(nameof(key)) : false; } } + + // https://github.com/SonarSource/sonar-dotnet/issues/7792 + class ConvertibleGenericTypes + { + void ConvertibleToBool(T1 unconstrained, T2 constrainedToStruct, T3 constrainedToBoolInterface) + where T2 : struct + where T3 : IComparable + { + if (unconstrained is true) { } + if (constrainedToStruct is true) { } + if (constrainedToBoolInterface is true) { } + } + } } diff --git a/analyzers/tests/SonarAnalyzer.Test/TestCases/BooleanLiteralUnnecessary.Fixed.cs b/analyzers/tests/SonarAnalyzer.Test/TestCases/BooleanLiteralUnnecessary.Fixed.cs index 27409b12aea..47f5436f08a 100644 --- a/analyzers/tests/SonarAnalyzer.Test/TestCases/BooleanLiteralUnnecessary.Fixed.cs +++ b/analyzers/tests/SonarAnalyzer.Test/TestCases/BooleanLiteralUnnecessary.Fixed.cs @@ -218,10 +218,18 @@ void Method(bool cond) // https://github.com/SonarSource/sonar-dotnet/issues/7792 class ObjectIsBool { - void Method(object obj, Exception exc) + void Object(object obj, Exception exc) { if (obj is true) { } if (exc.Data["SomeKey"] is true) { } } + + void ConvertibleToBool(IComparable comparable, IComparable comparableBool, IEquatable equatable, IConvertible convertible) + { + if (comparable is true) { } + if (comparableBool is true) { } + if (equatable is true) { } + if (convertible is true) { } + } } } diff --git a/analyzers/tests/SonarAnalyzer.Test/TestCases/BooleanLiteralUnnecessary.cs b/analyzers/tests/SonarAnalyzer.Test/TestCases/BooleanLiteralUnnecessary.cs index 1a7c3ad4f9b..2f6f2608efa 100644 --- a/analyzers/tests/SonarAnalyzer.Test/TestCases/BooleanLiteralUnnecessary.cs +++ b/analyzers/tests/SonarAnalyzer.Test/TestCases/BooleanLiteralUnnecessary.cs @@ -235,10 +235,18 @@ void Method(bool cond) // https://github.com/SonarSource/sonar-dotnet/issues/7792 class ObjectIsBool { - void Method(object obj, Exception exc) + void Object(object obj, Exception exc) { if (obj is true) { } if (exc.Data["SomeKey"] is true) { } } + + void ConvertibleToBool(IComparable comparable, IComparable comparableBool, IEquatable equatable, IConvertible convertible) + { + if (comparable is true) { } + if (comparableBool is true) { } + if (equatable is true) { } + if (convertible is true) { } + } } } From 6c3bc932fa191f2ed8f75f7450674d64b71f4e05 Mon Sep 17 00:00:00 2001 From: Zsolt Kolbay Date: Wed, 24 Jan 2024 10:40:59 +0100 Subject: [PATCH 3/3] Review 2 --- .../Rules/BooleanLiteralUnnecessary.cs | 1 + .../Rules/BooleanLiteralUnnecessaryBase.cs | 11 ++++++++--- .../Rules/BooleanLiteralUnnecessary.cs | 1 + 3 files changed, 10 insertions(+), 3 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/BooleanLiteralUnnecessary.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/BooleanLiteralUnnecessary.cs index 29b3ee25f31..4a876081461 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/BooleanLiteralUnnecessary.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/BooleanLiteralUnnecessary.cs @@ -66,6 +66,7 @@ protected override void Initialize(SonarAnalysisContext context) context.RegisterNodeAction(CheckNotEquals, SyntaxKind.NotEqualsExpression); context.RegisterNodeAction(CheckConditional, SyntaxKind.ConditionalExpression); context.RegisterNodeAction(CheckForLoopCondition, SyntaxKind.ForStatement); + base.Initialize(context); } private void CheckForLoopCondition(SonarSyntaxNodeReportingContext context) diff --git a/analyzers/src/SonarAnalyzer.Common/Rules/BooleanLiteralUnnecessaryBase.cs b/analyzers/src/SonarAnalyzer.Common/Rules/BooleanLiteralUnnecessaryBase.cs index f4384fe1c33..747ebb581bd 100644 --- a/analyzers/src/SonarAnalyzer.Common/Rules/BooleanLiteralUnnecessaryBase.cs +++ b/analyzers/src/SonarAnalyzer.Common/Rules/BooleanLiteralUnnecessaryBase.cs @@ -25,7 +25,7 @@ public abstract class BooleanLiteralUnnecessaryBase : SonarDiagnost { internal const string DiagnosticId = "S1125"; - private INamedTypeSymbol[] systemBooleanInterfaces; + private ImmutableArray systemBooleanInterfaces; protected enum ErrorLocation { @@ -48,6 +48,12 @@ protected enum ErrorLocation protected abstract bool IsTrue(SyntaxNode syntaxNode); protected abstract bool IsFalse(SyntaxNode syntaxNode); + protected override void Initialize(SonarAnalysisContext context) => + context.RegisterCompilationStartAction(x => + { + systemBooleanInterfaces = x.Compilation.GetTypeByMetadataName(KnownType.System_Boolean).Interfaces; + }); + // For C# 7 syntax protected virtual bool IsInsideTernaryWithThrowExpression(SyntaxNode syntaxNode) => false; @@ -212,11 +218,10 @@ private Location CalculateExtendedLocation(SyntaxNode parent, bool isLeftSide) private bool TypeShouldBeIgnored(SyntaxNode node, SemanticModel model) { var type = model.GetTypeInfo(node).Type; - systemBooleanInterfaces ??= model.Compilation.GetTypeByMetadataName(KnownType.System_Boolean).Interfaces.ToArray(); return type.IsNullableBoolean() || type is ITypeParameterSymbol || type.Is(KnownType.System_Object) - || Array.Exists(systemBooleanInterfaces, x => x.Equals(type)); + || systemBooleanInterfaces.Any(x => x.Equals(type)); } } } diff --git a/analyzers/src/SonarAnalyzer.VisualBasic/Rules/BooleanLiteralUnnecessary.cs b/analyzers/src/SonarAnalyzer.VisualBasic/Rules/BooleanLiteralUnnecessary.cs index 1af06936da0..52847be0a6f 100644 --- a/analyzers/src/SonarAnalyzer.VisualBasic/Rules/BooleanLiteralUnnecessary.cs +++ b/analyzers/src/SonarAnalyzer.VisualBasic/Rules/BooleanLiteralUnnecessary.cs @@ -45,6 +45,7 @@ protected override void Initialize(SonarAnalysisContext context) context.RegisterNodeAction(CheckEquals, SyntaxKind.EqualsExpression); context.RegisterNodeAction(CheckNotEquals, SyntaxKind.NotEqualsExpression); context.RegisterNodeAction(CheckConditional, SyntaxKind.TernaryConditionalExpression); + base.Initialize(context); } private void CheckLogicalNot(SonarSyntaxNodeReportingContext context)