SE - Nullable: Copy constraints to `Value` property #6841

pavel-mikula-sonarsource · 2023-03-02T08:36:27Z

Part of #6812

Copy known constraints from nullable symbol to .Value property.

bool? nullable = true;          // nullable symbol is known to have True constraint after this
var result = nullable.Value;  // True constraint from symbol nullable should get propagated to its Value property so it can be assigned to the result symbol later

sonarcloud · 2023-03-02T08:51:41Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

sonarcloud · 2023-03-02T08:54:46Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

100.0% Coverage
0.0% Duplication

martin-strecker-sonarsource

LGTM. I left some questions to make sure I got the overall plan right.

martin-strecker-sonarsource · 2023-03-02T09:42:34Z

...ests/SonarAnalyzer.UnitTest/SymbolicExecution/Roslyn/RoslynSymbolicExecutionTest.Nullable.cs

+        var setter = new PreProcessTestCheck(OperationKind.Literal, x => x.Operation.Instance.ConstantValue.Value is false ? x.SetOperationConstraint(TestConstraint.First) : x.State);
+        var validator = SETestContext.CreateCS(code, ", bool? arg", setter).Validator;
+        validator.ValidateTag("Unknown", x => x.Should().BeNull());
+        validator.ValidateTag("True", x => x.HasConstraint(BoolConstraint.True).Should().BeTrue());


Suggested change

validator.ValidateTag("True", x => x.HasConstraint(BoolConstraint.True).Should().BeTrue());

validator.ValidateTag("True", x => x.HasConstraint(BoolConstraint.True).Should().BeTrue());

validator.ValidateTag("True", x => x.HasConstraint(TestConstraint.First).Should().BeFalse());

This would assert that scaffolding works. We don't need that. The point of this is that all constraints are propagated.

martin-strecker-sonarsource · 2023-03-02T09:47:46Z

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

+            return propertyReference.Property.Name == "Value" && propertyReference.Instance.Type.IsNullableValueType() && context.State[symbol] is { } value
+                ? state.SetOperationValue(context.Operation, value)
+                : state;


Nice. This should work.
Just to make sure, I understand the plan correctly:
I suppose the idea is to later also learn

ObjectConstraint.NotNull for symbol whenever HasValue gets the BoolConstraint.True

ObjectConstraint.IsNull for symbol whenever HasValue gets the BoolConstraint.False

if (nullable.HasValue) { // nullable has ObjectConstraint.NotNull } else { // nullable has ObjectConstraint.Null }

If so we need to learn

ObjectConstraint.NotNull for symbol here as well as it is known to be NotNull from here on.

I suppose these are the next steps, right?

Yes, that's exactly the plan. See 2nd step on #6812

martin-strecker-sonarsource · 2023-03-02T10:00:28Z

...ests/SonarAnalyzer.UnitTest/SymbolicExecution/Roslyn/RoslynSymbolicExecutionTest.Nullable.cs

+    {
+        const string code = """
+            var value = arg.Value;
+            Tag("Unknown", value);


I suppose this will change to Tag("NoNull", value); in the future.

Yes, that will most likely happen on your PR.

Bingo: https://dev.azure.com/sonarsource/DotNetTeam%20Project/_build/results?buildId=63594&view=ms.vss-test-web.build-test-results-tab&runId=1244981&resultId=100793&paneView=debug

antonioaversa

Educational questions, for my and other people's benefit.

antonioaversa · 2023-03-02T09:52:40Z

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

+        if (propertyReference.Instance.TrackedSymbol() is { } symbol)
+        {
+            var state = context.State.SetSymbolConstraint(symbol, ObjectConstraint.NotNull);
+            return propertyReference.Property.Name == "Value" && propertyReference.Instance.Type.IsNullableValueType() && context.State[symbol] is { } value


Educational, to clarify levels of abstractions: Is the following statement correct?

"Value" here is the name of the property at SE level, directly coming from the symbol at Semantic level and not the syntactical token. So, if the code being symbolic-analyzed is VB.NET and the user types .value, the Name will still be "Value". That's why you don't use nameof(Nullable.Value), which would return the name at syntactical level.

Extension properties (as defined here) would not break this, for the same reason.

The "Value" statement is correct. It could have used nameof if that would compile (but it doesn't).

antonioaversa · 2023-03-02T10:06:52Z

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

-        propertyReference.Instance.TrackedSymbol() is { } symbol
-            ? context.SetSymbolConstraint(symbol, ObjectConstraint.NotNull)
-            : context.State;
+    protected override ProgramState Process(SymbolicContext context, IPropertyReferenceOperationWrapper propertyReference)


Educational: explicit cast, while calling .Value, is not supported by this because the cast is a method call at CFG level (is it?) and SE is not cross-level. So you will need to learn the same you learn here (i.e. Value as a non-null value) in a "IInvocationOperationWrapper". Correct?

All casts are represented as ConversionOperation (implicit or explicit). That will be handled later.

InvocationOperation is not relevant here.

antonioaversa · 2023-03-02T10:34:06Z

...ests/SonarAnalyzer.UnitTest/SymbolicExecution/Roslyn/RoslynSymbolicExecutionTest.Nullable.cs

+    {
+        const string code = """
+            bool? value = null;
+            Tag("Null", value);


Educational. The method built by CreateCS is empty. Why "Null" learns to be null and "True" learns to be true? The two parameters of Tag seem not correlated to each other.

The method built by CreateCS is not empty. It has this body.

The Tag method is a special empty method in the SETestContext that we use to tag a capture a specific state. It works like a probe.

The first argument is a string and servers as a key. Because we can tag more things in a single run.
As the constraints evolves over time, we're probing it twice, asserting different state on each place of code.

antonioaversa · 2023-03-02T10:37:07Z

...ests/SonarAnalyzer.UnitTest/SymbolicExecution/Roslyn/RoslynSymbolicExecutionTest.Nullable.cs

+            """;
+        var setter = new PreProcessTestCheck(OperationKind.Literal, x => x.Operation.Instance.ConstantValue.Value is false ? x.SetOperationConstraint(TestConstraint.First) : x.State);
+        var validator = SETestContext.CreateCS(code, ", bool? arg", setter).Validator;
+        validator.ValidateTag("Unknown", x => x.Should().BeNull());


Educational. Shouldn't this be unknown, because arg is unknown in code? Am I missing something?

arg is a parameter of the method. So the method goes like

public void Main(bool boolParameter, bool? arg) { var value = arg.Value; Tag("Unknown", value); // ... }

and as arg is a parameter, we cannot infer anything about its initial value. So the value is unknown to us, it has no constraints.
Then we change it, and after that, we can assume some constraints.

For C#, you can see everything that is available (and that we can use in the snippets) here:

sonar-dotnet/analyzers/tests/SonarAnalyzer.UnitTest/TestFramework/SymbolicExecution/SETestContext.cs

Lines 106 to 165 in dffe8ff

private static string ClassCodeCS(string methodBody, string additionalParameters) =>

$@"

using System;

using System.Collections.Generic;

using System.Diagnostics;

using System.Linq;

using static Tagger;

public unsafe class Sample

{{

public object ObjectField;

public static int StaticField;

public static object StaticObjectField;

public static int StaticProperty {{ get; set; }}

public static event EventHandler StaticEvent;

public event EventHandler Event;

public int Property {{ get; set; }}

public Sample SampleProperty {{ get; set; }}

public NotImplementedException PropertyException {{ get; set; }}

public int this[int index] {{get => 42; set {{ }} }}

private int field;

private NotImplementedException fieldException;

private bool Condition => Environment.ProcessorCount == 42; // Something that cannot have constraint

public Sample(){{ }}

public Sample(int i){{ }}

public void Main(bool boolParameter{additionalParameters})

{{

{methodBody}

}}

public NotImplementedException CreateException() => new NotImplementedException();

public void InstanceMethod(object parameter = null) {{ }}

public static void StaticMethod() {{ }}

}}

public class Person : PersonBase

{{

public static string StaticProperty {{ get; set; }}

public string Field;

public event EventHandler Event;

public string Method() => null;

public static void StaticMethod() {{ }}

}}

public class PersonBase

{{

}}

public static class Tagger

{{

public static void Tag(string name, object arg = null) {{ }}

public static void Tag(this object o, string name) {{ }}

public static T Unknown<T>() => default;

}}

";

}

pavel-mikula-sonarsource added 2 commits March 2, 2023 09:34

Prepare UTs

70f115a

Implement

3ff0778

pavel-mikula-sonarsource added the Sprint: SE label Mar 2, 2023

github-actions bot assigned pavel-mikula-sonarsource Mar 2, 2023

pavel-mikula-sonarsource changed the title ~~Pavel/se/value~~ SE - Nullable: Copy constraints to Value property Mar 2, 2023

pavel-mikula-sonarsource requested a review from martin-strecker-sonarsource March 2, 2023 09:26

github-actions bot assigned martin-strecker-sonarsource and unassigned pavel-mikula-sonarsource Mar 2, 2023

martin-strecker-sonarsource approved these changes Mar 2, 2023

View reviewed changes

github-actions bot assigned pavel-mikula-sonarsource and unassigned martin-strecker-sonarsource Mar 2, 2023

pavel-mikula-sonarsource merged commit 567b56b into feature/SE Mar 2, 2023

pavel-mikula-sonarsource deleted the Pavel/SE/Value branch March 2, 2023 10:09

antonioaversa reviewed Mar 2, 2023

View reviewed changes

martin-strecker-sonarsource changed the title ~~SE - Nullable: Copy constraints to Value property~~ SE - Non-nullable value type: Copy constraints to Value property Mar 2, 2023

martin-strecker-sonarsource changed the title ~~SE - Non-nullable value type: Copy constraints to Value property~~ SE - Nullable: Copy constraints to Value property Mar 2, 2023

pavel-mikula-sonarsource added a commit that referenced this pull request Mar 14, 2023

SE - Nullable: Copy constraints to Value property (#6841)

22fb9f9

pavel-mikula-sonarsource added a commit that referenced this pull request Mar 20, 2023

SE - Nullable: Copy constraints to Value property (#6841)

d3fd7de

pavel-mikula-sonarsource added a commit that referenced this pull request Mar 28, 2023

SE - Nullable: Copy constraints to Value property (#6841)

ec16b3b

pavel-mikula-sonarsource added a commit that referenced this pull request Mar 31, 2023

SE - Nullable: Copy constraints to Value property (#6841)

1b0e653

pavel-mikula-sonarsource added a commit that referenced this pull request Apr 4, 2023

SE - Nullable: Copy constraints to Value property (#6841)

7218641

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SE - Nullable: Copy constraints to `Value` property #6841

SE - Nullable: Copy constraints to `Value` property #6841

pavel-mikula-sonarsource commented Mar 2, 2023 •

edited

Loading

sonarcloud bot commented Mar 2, 2023

sonarcloud bot commented Mar 2, 2023

martin-strecker-sonarsource left a comment

martin-strecker-sonarsource Mar 2, 2023

pavel-mikula-sonarsource Mar 2, 2023

martin-strecker-sonarsource Mar 2, 2023

pavel-mikula-sonarsource Mar 2, 2023

martin-strecker-sonarsource Mar 2, 2023

pavel-mikula-sonarsource Mar 2, 2023

martin-strecker-sonarsource Mar 2, 2023

pavel-mikula-sonarsource Mar 2, 2023

antonioaversa left a comment

antonioaversa Mar 2, 2023

pavel-mikula-sonarsource Mar 2, 2023

antonioaversa Mar 2, 2023

pavel-mikula-sonarsource Mar 2, 2023

antonioaversa Mar 2, 2023

pavel-mikula-sonarsource Mar 2, 2023

antonioaversa Mar 2, 2023

pavel-mikula-sonarsource Mar 2, 2023

pavel-mikula-sonarsource Mar 2, 2023

	validator.ValidateTag("True", x => x.HasConstraint(BoolConstraint.True).Should().BeTrue());
	validator.ValidateTag("True", x => x.HasConstraint(BoolConstraint.True).Should().BeTrue());
	validator.ValidateTag("True", x => x.HasConstraint(TestConstraint.First).Should().BeFalse());

	private static string ClassCodeCS(string methodBody, string additionalParameters) =>
	$@"
	using System;
	using System.Collections.Generic;
	using System.Diagnostics;
	using System.Linq;

	using static Tagger;

	public unsafe class Sample
	{{
	public object ObjectField;
	public static int StaticField;
	public static object StaticObjectField;
	public static int StaticProperty {{ get; set; }}
	public static event EventHandler StaticEvent;
	public event EventHandler Event;
	public int Property {{ get; set; }}
	public Sample SampleProperty {{ get; set; }}
	public NotImplementedException PropertyException {{ get; set; }}
	public int this[int index] {{get => 42; set {{ }} }}
	private int field;
	private NotImplementedException fieldException;

	private bool Condition => Environment.ProcessorCount == 42; // Something that cannot have constraint

	public Sample(){{ }}
	public Sample(int i){{ }}

	public void Main(bool boolParameter{additionalParameters})
	{{
	{methodBody}
	}}

	public NotImplementedException CreateException() => new NotImplementedException();
	public void InstanceMethod(object parameter = null) {{ }}
	public static void StaticMethod() {{ }}
	}}

	public class Person : PersonBase
	{{
	public static string StaticProperty {{ get; set; }}
	public string Field;
	public event EventHandler Event;
	public string Method() => null;
	public static void StaticMethod() {{ }}
	}}

	public class PersonBase
	{{
	}}

	public static class Tagger
	{{
	public static void Tag(string name, object arg = null) {{ }}
	public static void Tag(this object o, string name) {{ }}
	public static T Unknown<T>() => default;
	}}
	";
	}

SE - Nullable: Copy constraints to Value property #6841

SE - Nullable: Copy constraints to Value property #6841

Conversation

pavel-mikula-sonarsource commented Mar 2, 2023 • edited Loading

sonarcloud bot commented Mar 2, 2023

sonarcloud bot commented Mar 2, 2023

martin-strecker-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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonioaversa 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SE - Nullable: Copy constraints to `Value` property #6841

SE - Nullable: Copy constraints to `Value` property #6841

pavel-mikula-sonarsource commented Mar 2, 2023 •

edited

Loading