From 3d305bdf18076606c2da01c33022bace31acd39b Mon Sep 17 00:00:00 2001 From: Tim Pohlmann Date: Tue, 22 Aug 2023 10:41:17 +0200 Subject: [PATCH 1/5] Learn BoolConstraint from IsPattern with constant number --- .../Constraints/NumberConstraint.cs | 3 ++ .../Roslyn/OperationProcessors/IsPattern.cs | 23 ++++++++++----- .../ConditionEvaluatesToConstant.CSharp8.cs | 29 ++++++++++--------- 3 files changed, 34 insertions(+), 21 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Constraints/NumberConstraint.cs b/analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Constraints/NumberConstraint.cs index 770b6823a56..b02e26d99be 100644 --- a/analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Constraints/NumberConstraint.cs +++ b/analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Constraints/NumberConstraint.cs @@ -87,6 +87,9 @@ public static NumberConstraint From(BigInteger? min, BigInteger? max) public bool CanContain(BigInteger value) => !(value < Min || Max < value); + public bool Overlaps(NumberConstraint other) => + Min <= other.Max && Max >= other.Min; + public override bool Equals(object obj) => obj is NumberConstraint other && other.Min == Min && other.Max == Max; diff --git a/analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/OperationProcessors/IsPattern.cs b/analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/OperationProcessors/IsPattern.cs index a075c42726d..e3fc2f6a3da 100644 --- a/analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/OperationProcessors/IsPattern.cs +++ b/analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/OperationProcessors/IsPattern.cs @@ -28,7 +28,7 @@ protected override IIsPatternOperationWrapper Convert(IOperation operation) => IIsPatternOperationWrapper.FromOperation(operation); protected override SymbolicConstraint BoolConstraintFromOperation(ProgramState state, IIsPatternOperationWrapper operation, bool isLoopCondition, int visitCount) => - BoolContraintFromConstant(state, operation) ?? BoolConstraintFromPattern(state, operation); + BoolConstraintFromConstant(state, operation) ?? BoolConstraintFromPattern(state, operation); protected override ProgramState LearnBranchingConstraint(ProgramState state, IIsPatternOperationWrapper operation, bool isLoopCondition, int visitCount, bool falseBranch) => operation.Value.TrackedSymbol(state) is { } testedSymbol @@ -100,19 +100,28 @@ private static SymbolicConstraint ConstraintFromDeclarationPattern(IDeclarationP } } - private static BoolConstraint BoolContraintFromConstant(ProgramState state, IIsPatternOperationWrapper isPattern) + private static BoolConstraint BoolConstraintFromConstant(ProgramState state, IIsPatternOperationWrapper isPattern) { if (state[isPattern.Value] is { } value && isPattern.Pattern.WrappedOperation.Kind == OperationKindEx.ConstantPattern - && IConstantPatternOperationWrapper.FromOperation(isPattern.Pattern.WrappedOperation).Value.ConstantValue.Value is bool boolConstant) + && IConstantPatternOperationWrapper.FromOperation(isPattern.Pattern.WrappedOperation).Value is var constantPattern + && constantPattern.ConstantValue.Value is { } constant) { - if (value.Constraint() is { } valueConstraint) + if (constant is bool boolConstant) { - return BoolConstraint.From(valueConstraint == BoolConstraint.From(boolConstant)); + if (value.Constraint() is { } valueBool) + { + return BoolConstraint.From(valueBool == BoolConstraint.From(boolConstant)); + } + else if (value.HasConstraint(ObjectConstraint.Null)) + { + return BoolConstraint.False; + } } - else if (value.HasConstraint(ObjectConstraint.Null)) + else if (state.Constraint(constantPattern) is { } constantNumber + && value.Constraint() is { } valueNumber) { - return BoolConstraint.False; + return BoolConstraint.From(valueNumber.Overlaps(constantNumber)); } } return null; // We cannot take conclusive decision diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/SymbolicExecution/Roslyn/ConditionEvaluatesToConstant.CSharp8.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/SymbolicExecution/Roslyn/ConditionEvaluatesToConstant.CSharp8.cs index 7ef0f3d2aa1..0c779520c17 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/SymbolicExecution/Roslyn/ConditionEvaluatesToConstant.CSharp8.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/SymbolicExecution/Roslyn/ConditionEvaluatesToConstant.CSharp8.cs @@ -49,43 +49,44 @@ int SwitchExpression_Results() a = false switch { _ => false }; - if (a) // Noncompliant + if (a) // Noncompliant { - return 42; // Secondary + return 42; // Secondary } a = false switch { - false => false, // Noncompliant - _ => false // Secondary + false => false, // Noncompliant + _ => false // Secondary }; - if (a) // Noncompliant + if (a) // Noncompliant { - return 42; // Secondary + return 42; // Secondary } a = false switch { - true => true, // Noncompliant - // Secondary@-1 + true => true, // Noncompliant + // Secondary@-1 _ => false }; - if (a) // Noncompliant + if (a) // Noncompliant { - return 42; // Secondary + return 42; // Secondary } a = 0 switch { - 1 => true, // FN - _ => false // Compliant, we don't raise in default statement + 1 => true, // Noncompliant + // Secondary@-1 + _ => false // Compliant, we don't raise in default statement }; - if (a) + if (a) // Noncompliant { - return 42; + return 42; // Secondary } return 0; } From cb8d8700ea354188b0fc68b96c500dd840c7bd60 Mon Sep 17 00:00:00 2001 From: Tim Pohlmann Date: Tue, 22 Aug 2023 10:47:46 +0200 Subject: [PATCH 2/5] Update ITs --- .../its/expected/Net5/Net5--net5.0-S2583.json | 24 +++++++++++++++++++ .../its/expected/Net5/Net5--net5.0-S2589.json | 13 ++++++++++ 2 files changed, 37 insertions(+) diff --git a/analyzers/its/expected/Net5/Net5--net5.0-S2583.json b/analyzers/its/expected/Net5/Net5--net5.0-S2583.json index aa9576b547d..32af9d3a856 100644 --- a/analyzers/its/expected/Net5/Net5--net5.0-S2583.json +++ b/analyzers/its/expected/Net5/Net5--net5.0-S2583.json @@ -149,6 +149,30 @@ "message": "Change this condition so that it does not always evaluate to 'False'. Some code paths are unreachable.", "location": [ { +"uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/sources/Net5/Net5/S3440.cs#L10", +"region": { +"startLine": 10, +"startColumn": 17, +"endLine": 10, +"endColumn": 18 +} +}, +{ +"uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/sources/Net5/Net5/S3440.cs#L10", +"region": { +"startLine": 10, +"startColumn": 22, +"endLine": 10, +"endColumn": 23 +} +} +] +}, +{ +"id": "S2583", +"message": "Change this condition so that it does not always evaluate to 'False'. Some code paths are unreachable.", +"location": [ +{ "uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/sources/Net5/Net5/S4201.cs#L37", "region": { "startLine": 37, diff --git a/analyzers/its/expected/Net5/Net5--net5.0-S2589.json b/analyzers/its/expected/Net5/Net5--net5.0-S2589.json index 38b1fb234e2..49176110200 100644 --- a/analyzers/its/expected/Net5/Net5--net5.0-S2589.json +++ b/analyzers/its/expected/Net5/Net5--net5.0-S2589.json @@ -51,6 +51,19 @@ "endColumn": 26 } } +}, +{ +"id": "S2589", +"message": "Change this condition so that it does not always evaluate to 'True'.", +"location": { +"uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/sources/Net5/Net5/S3440.cs#L11", +"region": { +"startLine": 11, +"startColumn": 17, +"endLine": 11, +"endColumn": 18 +} +} } ] } From 2a54698254fcb8199ef32a5fdc30095251f99c9c Mon Sep 17 00:00:00 2001 From: Tim Pohlmann Date: Tue, 22 Aug 2023 12:29:56 +0200 Subject: [PATCH 3/5] Fix Overlaps --- .../Constraints/NumberConstraint.cs | 3 +- .../Constraints/NumberConstraintTest.cs | 35 +++++++++++++++++++ 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Constraints/NumberConstraint.cs b/analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Constraints/NumberConstraint.cs index b02e26d99be..894848862a0 100644 --- a/analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Constraints/NumberConstraint.cs +++ b/analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Constraints/NumberConstraint.cs @@ -88,7 +88,8 @@ public bool CanContain(BigInteger value) => !(value < Min || Max < value); public bool Overlaps(NumberConstraint other) => - Min <= other.Max && Max >= other.Min; + (Min is null || other.Max is null || Min <= other.Max) + && (Max is null || other.Min is null || Max >= other.Min); public override bool Equals(object obj) => obj is NumberConstraint other && other.Min == Min && other.Max == Max; diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/SymbolicExecution/Constraints/NumberConstraintTest.cs b/analyzers/tests/SonarAnalyzer.UnitTest/SymbolicExecution/Constraints/NumberConstraintTest.cs index 9d6435d3cc0..4649ebb0188 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/SymbolicExecution/Constraints/NumberConstraintTest.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/SymbolicExecution/Constraints/NumberConstraintTest.cs @@ -223,4 +223,39 @@ public void CanContain_True(int? min, int? max, int value) => [DataRow(42, null, 41)] public void CanContain_False(int? min, int? max, int value) => NumberConstraint.From(min, max).CanContain(value).Should().BeFalse(); + + [DataTestMethod] + [DataRow(null, 42, null, 42)] + [DataRow(null, 42, null, 0)] + [DataRow(null, 42, 0, 0)] + [DataRow(null, 42, 0, 100)] + [DataRow(null, 42, 0, null)] + [DataRow(0, null, 0, null)] + [DataRow(0, null, 0, 42)] + [DataRow(0, null, 10, 42)] + [DataRow(0, null, -10, 42)] + [DataRow(0, null, null, 42)] + [DataRow(0, 42, 0, 42)] + [DataRow(0, 42, -10, 42)] + [DataRow(0, 42, null, 42)] + [DataRow(0, 42, -10, 10)] + [DataRow(0, 42, 10, 20)] + [DataRow(0, 42, 10, 100)] + [DataRow(0, 42, 10, null)] + [DataRow(0, 42, 0, 100)] + [DataRow(0, 42, 0, null)] + public void Overlaps_True(int? min, int? max, int? otherMin, int? otherMax) => + NumberConstraint.From(min, max).Overlaps(NumberConstraint.From(otherMin, otherMax)).Should().BeTrue(); + + [DataTestMethod] + [DataRow(42, null, null, 0)] + [DataRow(42, null, -10, 0)] + [DataRow(42, 100, null, 0)] + [DataRow(42, 100, -10, 0)] + [DataRow(null, 0, 42, null)] + [DataRow(null, 0, 42, 100)] + [DataRow(-10, 0, 42, null)] + [DataRow(-10, 0, 42, 100)] + public void Overlaps_False(int? min, int? max, int? otherMin, int? otherMax) => + NumberConstraint.From(min, max).Overlaps(NumberConstraint.From(otherMin, otherMax)).Should().BeFalse(); } From b50c36012a220e53a8f3d39c9459388318ceabb0 Mon Sep 17 00:00:00 2001 From: Tim Pohlmann Date: Thu, 24 Aug 2023 17:23:57 +0200 Subject: [PATCH 4/5] Make Overlaps null-safe --- .../SymbolicExecution/Constraints/NumberConstraint.cs | 5 +++-- .../SymbolicExecution/Constraints/NumberConstraintTest.cs | 3 +++ 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Constraints/NumberConstraint.cs b/analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Constraints/NumberConstraint.cs index 894848862a0..55724ee5599 100644 --- a/analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Constraints/NumberConstraint.cs +++ b/analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Constraints/NumberConstraint.cs @@ -88,8 +88,9 @@ public bool CanContain(BigInteger value) => !(value < Min || Max < value); public bool Overlaps(NumberConstraint other) => - (Min is null || other.Max is null || Min <= other.Max) - && (Max is null || other.Min is null || Max >= other.Min); + other is null // NumberConstraint.From(null, null) returns null and should be considered an unlimited range that overlaps with every other range + || ((Min is null || other.Max is null || Min <= other.Max) + && (Max is null || other.Min is null || Max >= other.Min)); public override bool Equals(object obj) => obj is NumberConstraint other && other.Min == Min && other.Max == Max; diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/SymbolicExecution/Constraints/NumberConstraintTest.cs b/analyzers/tests/SonarAnalyzer.UnitTest/SymbolicExecution/Constraints/NumberConstraintTest.cs index 4649ebb0188..73e1a351649 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/SymbolicExecution/Constraints/NumberConstraintTest.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/SymbolicExecution/Constraints/NumberConstraintTest.cs @@ -230,11 +230,13 @@ public void CanContain_False(int? min, int? max, int value) => [DataRow(null, 42, 0, 0)] [DataRow(null, 42, 0, 100)] [DataRow(null, 42, 0, null)] + [DataRow(null, 42, null, null)] [DataRow(0, null, 0, null)] [DataRow(0, null, 0, 42)] [DataRow(0, null, 10, 42)] [DataRow(0, null, -10, 42)] [DataRow(0, null, null, 42)] + [DataRow(0, null, null, null)] [DataRow(0, 42, 0, 42)] [DataRow(0, 42, -10, 42)] [DataRow(0, 42, null, 42)] @@ -244,6 +246,7 @@ public void CanContain_False(int? min, int? max, int value) => [DataRow(0, 42, 10, null)] [DataRow(0, 42, 0, 100)] [DataRow(0, 42, 0, null)] + [DataRow(0, 42, null, null)] public void Overlaps_True(int? min, int? max, int? otherMin, int? otherMax) => NumberConstraint.From(min, max).Overlaps(NumberConstraint.From(otherMin, otherMax)).Should().BeTrue(); From c0cfa6088f021a60a928db5d90dfb8e4d93b1b4d Mon Sep 17 00:00:00 2001 From: Tim Pohlmann Date: Mon, 28 Aug 2023 14:28:40 +0200 Subject: [PATCH 5/5] Review --- .../Roslyn/OperationProcessors/IsPattern.cs | 12 ++++++------ ...ynSymbolicExecutionTest.PatternMatching.cs | 19 +++++++++++++++++++ 2 files changed, 25 insertions(+), 6 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/OperationProcessors/IsPattern.cs b/analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/OperationProcessors/IsPattern.cs index e3fc2f6a3da..598043cac01 100644 --- a/analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/OperationProcessors/IsPattern.cs +++ b/analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/OperationProcessors/IsPattern.cs @@ -107,21 +107,21 @@ private static BoolConstraint BoolConstraintFromConstant(ProgramState state, IIs && IConstantPatternOperationWrapper.FromOperation(isPattern.Pattern.WrappedOperation).Value is var constantPattern && constantPattern.ConstantValue.Value is { } constant) { - if (constant is bool boolConstant) + if (constant is bool boolConstantConstraint) { - if (value.Constraint() is { } valueBool) + if (value.Constraint() is { } valueBoolConstraint) { - return BoolConstraint.From(valueBool == BoolConstraint.From(boolConstant)); + return BoolConstraint.From(valueBoolConstraint == BoolConstraint.From(boolConstantConstraint)); } else if (value.HasConstraint(ObjectConstraint.Null)) { return BoolConstraint.False; } } - else if (state.Constraint(constantPattern) is { } constantNumber - && value.Constraint() is { } valueNumber) + else if (state.Constraint(constantPattern) is { } constantNumberConstraint + && value.Constraint() is { } valueNumberConstraint) { - return BoolConstraint.From(valueNumber.Overlaps(constantNumber)); + return BoolConstraint.From(valueNumberConstraint.Overlaps(constantNumberConstraint)); } } return null; // We cannot take conclusive decision diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/SymbolicExecution/Roslyn/RoslynSymbolicExecutionTest.PatternMatching.cs b/analyzers/tests/SonarAnalyzer.UnitTest/SymbolicExecution/Roslyn/RoslynSymbolicExecutionTest.PatternMatching.cs index 1a30fb5459e..30317d1ad3d 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/SymbolicExecution/Roslyn/RoslynSymbolicExecutionTest.PatternMatching.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/SymbolicExecution/Roslyn/RoslynSymbolicExecutionTest.PatternMatching.cs @@ -325,6 +325,25 @@ public void ConstantPatternSetBoolConstraint_SingleState(string isPattern, bool? public void ConstantPatternSetBoolConstraint_TwoStates(string testedSymbol, string isPattern, ConstraintKind[] expectedForTrue, ConstraintKind[] expectedForFalse) => ValidateSetBoolConstraint_TwoStates(testedSymbol, isPattern, OperationKindEx.ConstantPattern, expectedForTrue, expectedForFalse); + [DataTestMethod] + [DataRow("i == 42", 42, true)] + [DataRow("i == 16", 42, false)] + [DataRow("i >= 42", 42, true)] + [DataRow("i <= 42", 42, true)] + [DataRow("i > 42", 42, false)] + [DataRow("i < 42", 42, false)] + public void ConstantNumberPatternSetBoolConstraint(string rangeCondition, int constant, bool expectedBoolConstraint) + { + var code = $$""" + if ({{rangeCondition}}) + { + var result = i is {{constant}}; + Tag("Result", result); + } + """; + SETestContext.CreateCS(code, "int i").Validator.TagValue("Result").Should().HaveOnlyConstraints(BoolConstraint.From(expectedBoolConstraint), ObjectConstraint.NotNull); + } + [DataTestMethod] [DataRow("objectNotNull is { }", true)] [DataRow("objectNotNull is object { }", true)]