SE: Learn bool constraint from Equals when parameters are numerical values #7906

mary-georgiou-sonarsource · 2023-08-28T11:55:42Z

mary-georgiou-sonarsource · 2023-08-28T13:23:35Z

analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/OperationProcessors/Invocation.cs

@@ -186,15 +186,30 @@ private static ProgramState ProcessAssertedBoolSymbol(ProgramState state, IOpera
        invocation switch
        {
            { Arguments.Length: 2, TargetMethod.IsStatic: true } => ProcessEquals(context, invocation.Arguments[0].ToArgument().Value, invocation.Arguments[1].ToArgument().Value),
-            { Arguments.Length: 1 } when invocation.TargetMethod.ContainingType.IsNullableValueType() => ProcessEquals(context, invocation.Instance, invocation.Arguments[0].ToArgument().Value),
+            { Arguments.Length: 1 } when invocation.TargetMethod.ContainingType.IsNullableValueType() || invocation.TargetMethod.ContainingType.IsStruct() =>


I'm not super confident about this.
This is so we can process also invocations for 1.Equals(2) and false.Equals(true).
I limited it only to structs as for classes it might be overridden and create weird side efffects.

I'm surprised this was limited to nullables in the first place. Seems like a reasonable improvement to me.

Maybe I can be very specific here again and ask that the containing type is bool or int?

You would need to include all the integral types (long, byte etc.). Keep it as is.

mary-georgiou-sonarsource · 2023-08-28T13:24:06Z

...ts/SonarAnalyzer.UnitTest/SymbolicExecution/Roslyn/RoslynSymbolicExecutionTest.Invocation.cs

-    [DataRow("42", "42")]
-    [DataRow("42", "0")]
-    [DataRow("0", "42")]


All of these are now detected as equal.

Tim-Pohlmann

Looks good for the most part. Just some improvements.

Tim-Pohlmann · 2023-08-28T13:52:27Z

analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/OperationProcessors/Invocation.cs

@@ -186,15 +186,30 @@ private static ProgramState ProcessAssertedBoolSymbol(ProgramState state, IOpera
        invocation switch
        {
            { Arguments.Length: 2, TargetMethod.IsStatic: true } => ProcessEquals(context, invocation.Arguments[0].ToArgument().Value, invocation.Arguments[1].ToArgument().Value),
-            { Arguments.Length: 1 } when invocation.TargetMethod.ContainingType.IsNullableValueType() => ProcessEquals(context, invocation.Instance, invocation.Arguments[0].ToArgument().Value),
+            { Arguments.Length: 1 } when invocation.TargetMethod.ContainingType.IsNullableValueType() || invocation.TargetMethod.ContainingType.IsStruct() =>


I'm surprised this was limited to nullables in the first place. Seems like a reasonable improvement to me.

Tim-Pohlmann · 2023-08-28T14:23:13Z

analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/OperationProcessors/Invocation.cs

-        && context.State.Constraint<BoolConstraint>(rightOperation) is { } leftBool
-            ? context.SetOperationConstraint(BoolConstraint.From(leftBool == rightBool)).ToArray()
-            : ProcessEqualsObject(context, leftOperation, rightOperation);
+    private static ProgramState[] ProcessEquals(SymbolicContext context, IOperation leftOperation, IOperation rightOperation)


If you extract all three options into methods you can do something like this:

ProcessEqualsBool(context, leftOperation, rightOperation) ?? ProcessEqualsNumber(context, leftOperation, rightOperation) ?? ProcessEqualsObject(context, leftOperation, rightOperation);

I reverted - it does not look natural to return null when the return type is an array of ProgramState.

@pavel-mikula-sonarsource any better way to split ProcessEquals

It's fine as it is.

Why does it feel unnatural? This is a pattern used quite commonly in the code-base.
I'm not super opposed to the if-else tree, but I find the alternative more readable.

Tim-Pohlmann · 2023-08-28T14:32:39Z

analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/OperationProcessors/Invocation.cs

+        else if (context.State.Constraint<NumberConstraint>(leftOperation) is { } leftNumber
+            && context.State.Constraint<NumberConstraint>(rightOperation) is { } rightNumber
+            && leftNumber.IsSingleValue
+            && rightNumber.IsSingleValue)
+        {
+            return context.SetOperationConstraint(BoolConstraint.From(leftNumber.Equals(rightNumber))).ToArray();
+        }


It would be really good if we would also learn false for non-SingleValues ranges that do not Overlap. It should be very easy to add here. But feel free to not include it in this PR.

That sounds like a good idea :D
I'll see how "easy" it is.

EDIT: Done

Tim-Pohlmann · 2023-08-28T14:56:45Z

...ts/SonarAnalyzer.UnitTest/SymbolicExecution/Roslyn/RoslynSymbolicExecutionTest.Invocation.cs

+        var code = $"""
+                var result = {left}.Equals({right});
+                Tag("Result", result);
+
+                var resultStaticCall = object.Equals({left}, {right});
+                Tag("ResultStaticCall", resultStaticCall);
+                """;


Suggested change

var code = $"""

var result = {left}.Equals({right});

Tag("Result", result);

var resultStaticCall = object.Equals({left}, {right});

Tag("ResultStaticCall", resultStaticCall);

""";

var code = $"""

var result = {left}.Equals({right});

Tag("Result", result);

var resultStaticCall = object.Equals({left}, {right});

Tag("ResultStaticCall", resultStaticCall);

""";

I think this is correct - there's no need to move it.

There is no reason for the extra indention and I cannot find anything about string literals in our code conventions. Why do you think this is correct?

Just followed the same pattern of the other examples in the file - which I now noticed that it's not consistent. :/
I'll update everything to 3 * 4 spaces.

Tim-Pohlmann · 2023-08-28T14:57:10Z

...ts/SonarAnalyzer.UnitTest/SymbolicExecution/Roslyn/RoslynSymbolicExecutionTest.Invocation.cs

+        var code = """
+                if (i>1 && i<10)
+                {
+                    var result = object.Equals(i, 0);
+                    Tag("Result", result);
+                }
+                """;


Suggested change

var code = """

if (i>1 && i<10)

{

var result = object.Equals(i, 0);

Tag("Result", result);

}

""";

var code = """

if (i>1 && i<10)

{

var result = object.Equals(i, 0);

Tag("Result", result);

}

""";

Tim-Pohlmann · 2023-08-28T15:04:48Z

...ts/SonarAnalyzer.UnitTest/SymbolicExecution/Roslyn/RoslynSymbolicExecutionTest.Invocation.cs

+    public void Invocation_NumberEquals_DoesNotLearn()
+    {
+        var code = """
+                if (i>1 && i<10)


Suggested change

if (i>1 && i<10)

if (i > 1 && i < 10)

Tim-Pohlmann · 2023-08-28T15:06:09Z

...ts/SonarAnalyzer.UnitTest/SymbolicExecution/Roslyn/RoslynSymbolicExecutionTest.Invocation.cs

+                }
+                """;
+        var validator = SETestContext.CreateCS(code, "int i").Validator;
+        validator.TagValue("Result").Should().HaveOnlyConstraints(ObjectConstraint.NotNull);


Add a comment to document the not-implemented behavior.

Suggested change

validator.TagValue("Result").Should().HaveOnlyConstraints(ObjectConstraint.NotNull);

validator.TagValue("Result").Should().HaveOnlyConstraints(ObjectConstraint.NotNull); // Should learn false

pavel-mikula-sonarsource

LGTM with some nitpicks

pavel-mikula-sonarsource · 2023-08-30T09:08:18Z

...ts/SonarAnalyzer.UnitTest/SymbolicExecution/Roslyn/RoslynSymbolicExecutionTest.Invocation.cs

+                }
+                """;
+        var validator = SETestContext.CreateCS(code, "int i").Validator;
+        validator.TagValue("Result").Should().HaveOnlyConstraints(ObjectConstraint.NotNull); // Should learn 'false'


It is in range, so it could be true or false

Suggested change

validator.TagValue("Result").Should().HaveOnlyConstraints(ObjectConstraint.NotNull); // Should learn 'false'

validator.TagValue("Result").Should().HaveOnlyConstraints(ObjectConstraint.NotNull);

pavel-mikula-sonarsource · 2023-08-30T09:09:16Z

...ts/SonarAnalyzer.UnitTest/SymbolicExecution/Roslyn/RoslynSymbolicExecutionTest.Invocation.cs

+    public void Invocation_NumberEquals_RangesDoNotOverlap_LearnsResult()
+    {
+        var code = $$"""
+                if (i > 0 && j < 0)


Same thing, easier to imagine (negative first, on the left of the axis)

Suggested change

if (i > 0 && j < 0)

if (i < 0 && j > 0)

pavel-mikula-sonarsource · 2023-08-30T09:09:51Z

...ts/SonarAnalyzer.UnitTest/SymbolicExecution/Roslyn/RoslynSymbolicExecutionTest.Invocation.cs

+    }
+
+    [TestMethod]
+    public void Invocation_NumberEquals_DoesNotLearn()


Needs to explain how is it different from the others

Suggested change

public void Invocation_NumberEquals_DoesNotLearn()

public void Invocation_NumberEquals_ValueInRange_DoesNotLearn()

pavel-mikula-sonarsource · 2023-08-30T09:12:33Z

analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/OperationProcessors/Invocation.cs

+        if (context.State.Constraint<BoolConstraint>(leftOperation) is { } rightBool
+            && context.State.Constraint<BoolConstraint>(rightOperation) is { } leftBool)


Swapped names

Suggested change

if (context.State.Constraint<BoolConstraint>(leftOperation) is { } rightBool

&& context.State.Constraint<BoolConstraint>(rightOperation) is { } leftBool)

if (context.State.Constraint<BoolConstraint>(leftOperation) is { } leftBool

&& context.State.Constraint<BoolConstraint>(rightOperation) is { } rightBool)

pavel-mikula-sonarsource · 2023-08-30T09:14:59Z

analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/OperationProcessors/Invocation.cs

+            else if (!left.Overlaps(right))
+            {
+                return context.SetOperationConstraint(BoolConstraint.False);
+            }
+            else
+            {
+                return context.State;
+            }


I would make the condition positive

If I make it positive then I don't know the outcome.
If the number constrains don't overlap I know that the condition is false - if they do overlap then I know nothing.

Tim-Pohlmann

I'll approve this besides some open discussions, since I think they should not be blocking.
Proceed on based on your own judgment.

Tim-Pohlmann · 2023-08-31T10:05:08Z

analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/OperationProcessors/Invocation.cs

-        && context.State.Constraint<BoolConstraint>(rightOperation) is { } leftBool
-            ? context.SetOperationConstraint(BoolConstraint.From(leftBool == rightBool)).ToArray()
-            : ProcessEqualsObject(context, leftOperation, rightOperation);
+    private static ProgramState[] ProcessEquals(SymbolicContext context, IOperation leftOperation, IOperation rightOperation)


Why does it feel unnatural? This is a pattern used quite commonly in the code-base.
I'm not super opposed to the if-else tree, but I find the alternative more readable.

Tim-Pohlmann · 2023-08-31T10:06:19Z

...ts/SonarAnalyzer.UnitTest/SymbolicExecution/Roslyn/RoslynSymbolicExecutionTest.Invocation.cs

+        var code = $"""
+                var result = {left}.Equals({right});
+                Tag("Result", result);
+
+                var resultStaticCall = object.Equals({left}, {right});
+                Tag("ResultStaticCall", resultStaticCall);
+                """;


There is no reason for the extra indention and I cannot find anything about string literals in our code conventions. Why do you think this is correct?

sonarcloud · 2023-08-31T12:06:58Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

sonarcloud · 2023-08-31T12:11:08Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

100.0% Coverage
0.0% Duplication

mary-georgiou-sonarsource force-pushed the mary/equals-numerical branch 2 times, most recently from 393cdf2 to 1097ca6 Compare August 28, 2023 13:21

mary-georgiou-sonarsource requested a review from Tim-Pohlmann August 28, 2023 13:22

mary-georgiou-sonarsource commented Aug 28, 2023

View reviewed changes

Tim-Pohlmann requested changes Aug 28, 2023

View reviewed changes

mary-georgiou-sonarsource added 3 commits August 29, 2023 09:30

Learn equals from number constraints

aa3ae2a

fix UTs

de47e33

update test with infinite loop

c13155f

mary-georgiou-sonarsource force-pushed the mary/equals-numerical branch 3 times, most recently from 8efbbe9 to ae69dca Compare August 29, 2023 09:23

mary-georgiou-sonarsource requested a review from pavel-mikula-sonarsource August 29, 2023 09:23

mary-georgiou-sonarsource force-pushed the mary/equals-numerical branch from f52bd19 to cf326a1 Compare August 29, 2023 13:06

learn equals reasult from number constrain with ranges

376fefd

mary-georgiou-sonarsource force-pushed the mary/equals-numerical branch from cf326a1 to 376fefd Compare August 29, 2023 13:07

pavel-mikula-sonarsource approved these changes Aug 30, 2023

View reviewed changes

apply comments

4f9d452

Tim-Pohlmann approved these changes Aug 31, 2023

View reviewed changes

update identation

8d9a44c

mary-georgiou-sonarsource merged commit 9e6cc0d into master Aug 31, 2023
22 checks passed

mary-georgiou-sonarsource deleted the mary/equals-numerical branch August 31, 2023 12:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SE: Learn bool constraint from Equals when parameters are numerical values #7906

SE: Learn bool constraint from Equals when parameters are numerical values #7906

mary-georgiou-sonarsource commented Aug 28, 2023

mary-georgiou-sonarsource Aug 28, 2023

Tim-Pohlmann Aug 28, 2023

mary-georgiou-sonarsource Aug 28, 2023

Tim-Pohlmann Aug 28, 2023

mary-georgiou-sonarsource Aug 28, 2023

Tim-Pohlmann left a comment

Tim-Pohlmann Aug 28, 2023

Tim-Pohlmann Aug 28, 2023

mary-georgiou-sonarsource Aug 29, 2023

mary-georgiou-sonarsource Aug 29, 2023

pavel-mikula-sonarsource Aug 30, 2023

Tim-Pohlmann Aug 31, 2023

Tim-Pohlmann Aug 28, 2023

mary-georgiou-sonarsource Aug 29, 2023 •

edited

Tim-Pohlmann Aug 28, 2023

mary-georgiou-sonarsource Aug 29, 2023

Tim-Pohlmann Aug 31, 2023

mary-georgiou-sonarsource Aug 31, 2023

Tim-Pohlmann Aug 28, 2023

mary-georgiou-sonarsource Aug 29, 2023

Tim-Pohlmann Aug 28, 2023

Tim-Pohlmann Aug 28, 2023

pavel-mikula-sonarsource left a comment

pavel-mikula-sonarsource Aug 30, 2023

pavel-mikula-sonarsource Aug 30, 2023

pavel-mikula-sonarsource Aug 30, 2023

pavel-mikula-sonarsource Aug 30, 2023

pavel-mikula-sonarsource Aug 30, 2023

mary-georgiou-sonarsource Aug 31, 2023 •

edited

Tim-Pohlmann left a comment

Tim-Pohlmann Aug 31, 2023

Tim-Pohlmann Aug 31, 2023

sonarcloud bot commented Aug 31, 2023

sonarcloud bot commented Aug 31, 2023

	validator.TagValue("Result").Should().HaveOnlyConstraints(ObjectConstraint.NotNull);
	validator.TagValue("Result").Should().HaveOnlyConstraints(ObjectConstraint.NotNull); // Should learn false

	public void Invocation_NumberEquals_DoesNotLearn()
	public void Invocation_NumberEquals_ValueInRange_DoesNotLearn()

		if (context.State.Constraint<BoolConstraint>(leftOperation) is { } rightBool
		&& context.State.Constraint<BoolConstraint>(rightOperation) is { } leftBool)

SE: Learn bool constraint from Equals when parameters are numerical values #7906

SE: Learn bool constraint from Equals when parameters are numerical values #7906

Conversation

mary-georgiou-sonarsource commented Aug 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tim-Pohlmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mary-georgiou-sonarsource Aug 29, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavel-mikula-sonarsource left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mary-georgiou-sonarsource Aug 31, 2023 • edited

Choose a reason for hiding this comment

Tim-Pohlmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Aug 31, 2023

sonarcloud bot commented Aug 31, 2023

mary-georgiou-sonarsource Aug 29, 2023 •

edited

mary-georgiou-sonarsource Aug 31, 2023 •

edited