Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SE - Nullable: Copy constraints to Value property #6841

Merged
merged 2 commits into from
Mar 2, 2023
Merged

SE - Nullable: Copy constraints to Value property #6841

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
70f115a
Prepare UTs
pavel-mikula-sonarsource Mar 2, 2023
3ff0778
Implement
pavel-mikula-sonarsource Mar 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 14 additions & 4 deletions ...yzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/OperationProcessors/References.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,10 +71,20 @@ internal sealed class PropertyReference : SimpleProcessor<IPropertyReferenceOper
protected override IPropertyReferenceOperationWrapper Convert(IOperation operation) =>
IPropertyReferenceOperationWrapper.FromOperation(operation);

protected override ProgramState Process(SymbolicContext context, IPropertyReferenceOperationWrapper propertyReference) =>
propertyReference.Instance.TrackedSymbol() is { } symbol
? context.SetSymbolConstraint(symbol, ObjectConstraint.NotNull)
: context.State;
protected override ProgramState Process(SymbolicContext context, IPropertyReferenceOperationWrapper propertyReference)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

InvocationOperation is not relevant here.

{
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

? state.SetOperationValue(context.Operation, value)
: state;
Comment on lines +79 to +81
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
else
{
return context.State;
}
}
}

internal sealed class ArrayElementReference : SimpleProcessor<IArrayElementReferenceOperationWrapper>
Expand Down
62 changes: 62 additions & 0 deletions ...s/SonarAnalyzer.UnitTest/SymbolicExecution/Roslyn/RoslynSymbolicExecutionTest.Nullable.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
/*
* SonarAnalyzer for .NET
* Copyright (C) 2015-2023 SonarSource SA
* mailto: contact AT sonarsource DOT com
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
* License as published by the Free Software Foundation; either
* version 3 of the License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public License
* along with this program; if not, write to the Free Software Foundation,
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/

using SonarAnalyzer.SymbolicExecution.Constraints;
using SonarAnalyzer.UnitTest.TestFramework.SymbolicExecution;

namespace SonarAnalyzer.UnitTest.SymbolicExecution.Roslyn;

public partial class RoslynSymbolicExecutionTest
{
[TestMethod]
public void Nullable_Assignment_PropagatesConstrainsToValue()
{
const string code = """
bool? value = null;
Tag("Null", value);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

value = true;
Tag("True", value);
""";
var validator = SETestContext.CreateCS(code).Validator;
validator.ValidateTag("Null", x => x.HasConstraint(ObjectConstraint.Null).Should().BeTrue());
validator.ValidateTag("True", x => x.HasConstraint(BoolConstraint.True).Should().BeTrue());
}

[TestMethod]
public void Nullable_Value_ReadsConstraintsFromInstance()
{
const string code = """
var value = arg.Value;
Tag("Unknown", value);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that will most likely happen on your PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💥

arg = true;
value = arg.Value;
Tag("True", value);
arg = false; // This will set additional constraint TestConstraint.First
value = arg.Value;
Tag("FalseFirst", value);
""";
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());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
}}
";
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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());

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

validator.ValidateTag("FalseFirst", x => x.HasConstraint(BoolConstraint.False).Should().BeTrue());
validator.ValidateTag("FalseFirst", x => x.HasConstraint(TestConstraint.First).Should().BeTrue());
}
}