Skip to content

Commit

Permalink
Migrate S4158: Passing as argument removes constraint (#7433)
Browse files Browse the repository at this point in the history
  • Loading branch information
@Tim-Pohlmann
Tim-Pohlmann authored Jun 16, 2023
1 parent 897430c commit 6dceab4
Show file tree
Hide file tree
Showing 11 changed files with 94 additions and 123 deletions.
30 changes: 0 additions & 30 deletions analyzers/its/expected/Automapper/AutoMapper--net461-S4158.json
View file

This file was deleted.

30 changes: 0 additions & 30 deletions analyzers/its/expected/Automapper/AutoMapper--netstandard2.0-S4158.json
View file

This file was deleted.

3 changes: 3 additions & 0 deletions ...zers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/Extensions/IOperationExtensions.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@ static bool IsTryDownCast(IConversionOperationWrapper conversion) =>
conversion.IsTryCast && !conversion.Operand.Type.DerivesOrImplements(conversion.Type);
}

internal static IArgumentOperationWrapper? AsArgument(this IOperation operation) =>
operation.As(OperationKindEx.Argument, IArgumentOperationWrapper.FromOperation);

internal static IAssignmentOperationWrapper? AsAssignment(this IOperation operation) =>
operation.As(OperationKindEx.SimpleAssignment, IAssignmentOperationWrapper.FromOperation);

Expand Down
6 changes: 6 additions & 0 deletions ...r.Common/SymbolicExecution/Roslyn/RuleChecks/EmptyCollectionsShouldNotBeEnumeratedBase.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,12 @@ protected override ProgramState PreProcessSimple(SymbolicContext context)
{
return context.State.SetOperationConstraint(operation, constraint);
}
else if (operation.AsArgument() is { } argument
&& context.State.ResolveCaptureAndUnwrapConversion(argument.Value).TrackedSymbol() is { } symbol
&& context.State[symbol] is { } symbolValue)
{
return context.State.SetSymbolValue(symbol, symbolValue.WithoutConstraint<CollectionConstraint>());
}
else
{
return context.State;
Expand Down
2 changes: 1 addition & 1 deletion analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/SymbolicValue.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ private string SerializeConstraints() =>
private SymbolicValue RemoveConstraint(Type type) =>
Constraints.Count switch
{
1 => Empty,
1 => null,
2 => OtherSingle(type),
3 => OtherPair(type),
_ => this with { Constraints = Constraints.Remove(type) },
Expand Down
9 changes: 8 additions & 1 deletion analyzers/tests/SonarAnalyzer.UnitTest/SymbolicExecution/Roslyn/ProgramStateTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -360,7 +360,14 @@ private static void ResetFieldConstraintTests(SymbolicConstraint constraint, boo
symbolValue.HasConstraint(constraint).Should().BeTrue();
sut = sut.ResetFieldConstraints();
symbolValue = sut[field];
symbolValue.HasConstraint(constraint).Should().Be(expectIsPreserved);
if (expectIsPreserved)
{
symbolValue.HasConstraint(constraint).Should().BeTrue();
}
else
{
symbolValue.Should().BeNull();
}
}

private static ISymbol[] CreateSymbols()
Expand Down
50 changes: 25 additions & 25 deletions ...SonarAnalyzer.UnitTest/SymbolicExecution/Roslyn/RoslynSymbolicExecutionTest.Invocation.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -191,10 +191,10 @@ public static void Tag(string name, object arg) {{ }}
}}";
var validator = new SETestContext(code, AnalyzerLanguage.CSharp, Array.Empty<SymbolicCheck>()).Validator;
validator.ValidateContainsOperation(OperationKind.Invocation);
validator.ValidateTag("BeforeField", x => x.HasConstraint(ObjectConstraint.Null).Should().BeTrue());
validator.ValidateTag("BeforeStaticField", x => x.HasConstraint(ObjectConstraint.Null).Should().BeTrue());
validator.ValidateTag("AfterField", x => x.Constraint<ObjectConstraint>().Should().BeNull());
validator.ValidateTag("AfterStaticField", x => x.Constraint<ObjectConstraint>().Should().BeNull());
validator.TagValue("BeforeField").HasConstraint(ObjectConstraint.Null).Should().BeTrue();
validator.TagValue("BeforeStaticField").HasConstraint(ObjectConstraint.Null).Should().BeTrue();
validator.TagValue("AfterField").Should().BeNull();
validator.TagValue("AfterStaticField").Should().BeNull();
}

[DataTestMethod]
Expand Down Expand Up @@ -277,8 +277,8 @@ public static void Tag(string name) {{ }}
x => // Branch for "this"
{
x[condition].Should().BeNull(); // Should have BoolConstraint.True
x[field].AllConstraints.Should().BeEmpty();
x[staticField].AllConstraints.Should().BeEmpty();
x[field].Should().BeNull();
x[staticField].Should().BeNull();
},
x => // Branch for "otherInstance"
{
Expand Down Expand Up @@ -387,7 +387,7 @@ public void Instance_InstanceMethodCall_ClearsField()
var validator = SETestContext.CreateCS(code).Validator;
validator.TagValues("After").Should().Equal(
SymbolicValue.Empty.WithConstraint(ObjectConstraint.NotNull),
SymbolicValue.Empty);
null);
}

[TestMethod]
Expand All @@ -398,7 +398,7 @@ public void Instance_InstanceMethodCall_ClearsFieldWithBranchInArgument()
this.InstanceMethod(boolParameter ? 1 : 0);
Tag(""After"", this.ObjectField);";
var validator = SETestContext.CreateCS(code).Validator;
validator.ValidateTag("After", x => x.AllConstraints.Should().BeEmpty());
validator.TagValue("After").Should().BeNull();
}

[TestMethod]
Expand Down Expand Up @@ -465,28 +465,28 @@ public class Other
public static void OtherMethod() { }
}";
var validator = new SETestContext(code, AnalyzerLanguage.CSharp, Array.Empty<SymbolicCheck>()).Validator;
validator.ValidateTag("Start_Base_BaseField", x => x.HasConstraint(ObjectConstraint.Null).Should().BeTrue());
validator.ValidateTag("Start_Other_OtherField", x => x.HasConstraint(ObjectConstraint.Null).Should().BeTrue());
validator.ValidateTag("Start_Sample_SampleField1", x => x.HasConstraint(ObjectConstraint.Null).Should().BeTrue());
validator.ValidateTag("Start_Sample_SampleField2", x => x.HasConstraint(ObjectConstraint.Null).Should().BeTrue());
validator.TagValue("Start_Base_BaseField").HasConstraint(ObjectConstraint.Null).Should().BeTrue();
validator.TagValue("Start_Other_OtherField").HasConstraint(ObjectConstraint.Null).Should().BeTrue();
validator.TagValue("Start_Sample_SampleField1").HasConstraint(ObjectConstraint.Null).Should().BeTrue();
validator.TagValue("Start_Sample_SampleField2").HasConstraint(ObjectConstraint.Null).Should().BeTrue();

// SampleMethod() resets own field and base class fields, but not other class fields
validator.ValidateTag("SampleMethod_Base_BaseField", x => x.HasConstraint(ObjectConstraint.Null).Should().BeFalse());
validator.ValidateTag("SampleMethod_Other_OtherField", x => x.HasConstraint(ObjectConstraint.Null).Should().BeTrue());
validator.ValidateTag("SampleMethod_Sample_SampleField1", x => x.HasConstraint(ObjectConstraint.Null).Should().BeFalse());
validator.ValidateTag("SampleMethod_Sample_SampleField2", x => x.HasConstraint(ObjectConstraint.Null).Should().BeFalse());
validator.TagValue("SampleMethod_Base_BaseField").Should().BeNull();
validator.TagValue("SampleMethod_Other_OtherField").HasConstraint(ObjectConstraint.Null).Should().BeTrue();
validator.TagValue("SampleMethod_Sample_SampleField1").Should().BeNull();
validator.TagValue("SampleMethod_Sample_SampleField2").Should().BeNull();

// OtherMethod() resets only its own constraints
validator.ValidateTag("OtherMethod_Base_BaseField", x => x.HasConstraint(ObjectConstraint.Null).Should().BeTrue());
validator.ValidateTag("OtherMethod_Other_OtherField", x => x.HasConstraint(ObjectConstraint.Null).Should().BeFalse());
validator.ValidateTag("OtherMethod_Sample_SampleField1", x => x.HasConstraint(ObjectConstraint.Null).Should().BeTrue());
validator.ValidateTag("OtherMethod_Sample_SampleField2", x => x.HasConstraint(ObjectConstraint.Null).Should().BeTrue());
validator.TagValue("OtherMethod_Base_BaseField").HasConstraint(ObjectConstraint.Null).Should().BeTrue();
validator.TagValue("OtherMethod_Other_OtherField").Should().BeNull();
validator.TagValue("OtherMethod_Sample_SampleField1").HasConstraint(ObjectConstraint.Null).Should().BeTrue();
validator.TagValue("OtherMethod_Sample_SampleField2").HasConstraint(ObjectConstraint.Null).Should().BeTrue();

// BaseMethod() called from Sample only resets Base field
validator.ValidateTag("BaseMethod_Base_BaseField", x => x.HasConstraint(ObjectConstraint.Null).Should().BeFalse());
validator.ValidateTag("BaseMethod_Other_OtherField", x => x.HasConstraint(ObjectConstraint.Null).Should().BeTrue());
validator.ValidateTag("BaseMethod_Sample_SampleField1", x => x.HasConstraint(ObjectConstraint.Null).Should().BeTrue());
validator.ValidateTag("BaseMethod_Sample_SampleField2", x => x.HasConstraint(ObjectConstraint.Null).Should().BeTrue());
validator.TagValue("BaseMethod_Base_BaseField").Should().BeNull();
validator.TagValue("BaseMethod_Other_OtherField").HasConstraint(ObjectConstraint.Null).Should().BeTrue();
validator.TagValue("BaseMethod_Sample_SampleField1").HasConstraint(ObjectConstraint.Null).Should().BeTrue();
validator.TagValue("BaseMethod_Sample_SampleField2").HasConstraint(ObjectConstraint.Null).Should().BeTrue();
}

[DataTestMethod]
Expand All @@ -507,7 +507,7 @@ public void Invocation_StaticMethodCall_DoesNotClearInstanceFields(string invoca
validator.ValidateTag("BeforeField", x => x.HasConstraint(ObjectConstraint.Null).Should().BeTrue());
validator.ValidateTag("BeforeStaticField", x => x.HasConstraint(ObjectConstraint.Null).Should().BeTrue());
validator.ValidateTag("AfterField", x => x.HasConstraint(ObjectConstraint.Null).Should().BeTrue());
validator.ValidateTag("AfterStaticField", x => x.HasConstraint(ObjectConstraint.Null).Should().BeFalse());
validator.ValidateTag("AfterStaticField", x => x.Should().BeNull());
}

[TestMethod]
Expand Down
38 changes: 19 additions & 19 deletions ...yzer.UnitTest/SymbolicExecution/Roslyn/RoslynSymbolicExecutionTest.InvocationAttribute.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -185,32 +185,32 @@ public void Invocation_NotNullWhen_Unknown()
[TestMethod]
public void Invocation_NotNullWhen_Unknown_InstanceMethodResetsFieldConstraints()
{
const string code = @"
private object ObjectField;
public void Test()
{
this.ObjectField = null;
string byteString = Unknown<string>();
var success = TryParse(byteString, out var result);
Tag(""End"", null);
}
public bool TryParse([System.Diagnostics.CodeAnalysis.NotNullWhenAttribute(true)] string s, out object o) { o = null; return true; }";
const string code = """
private object ObjectField;
public void Test()
{
this.ObjectField = null;
string byteString = Unknown<string>();
var success = TryParse(byteString, out var result);
Tag("End", null);
}
public bool TryParse([System.Diagnostics.CodeAnalysis.NotNullWhenAttribute(true)] string s, out object o) { o = null; return true; }
""";
var validator = SETestContext.CreateCSMethod(code).Validator;
validator.TagStates("End").Should().SatisfyRespectively(
x =>
{
x[validator.Symbol("ObjectField")].HasConstraint<ObjectConstraint>().Should().BeFalse();
x[validator.Symbol("byteString")].HasConstraint(ObjectConstraint.NotNull).Should().BeTrue();
x[validator.Symbol("success")].HasConstraint(BoolConstraint.True).Should().BeTrue();
x[validator.Symbol("result")].Should().BeNull();
x[validator.Symbol("ObjectField")].Should().HaveNoConstraints();
x[validator.Symbol("byteString")].Should().HaveOnlyConstraint(ObjectConstraint.NotNull);
x[validator.Symbol("success")].Should().HaveOnlyConstraints(ObjectConstraint.NotNull, BoolConstraint.True);
x[validator.Symbol("result")].Should().HaveNoConstraints();
},
x =>
{
x[validator.Symbol("ObjectField")].HasConstraint<ObjectConstraint>().Should().BeFalse();
x[validator.Symbol("byteString")].Should().BeNull();
x[validator.Symbol("success")].HasConstraint(BoolConstraint.False).Should().BeTrue();
x[validator.Symbol("result")].Should().BeNull();
x[validator.Symbol("ObjectField")].Should().HaveNoConstraints();
x[validator.Symbol("byteString")].Should().HaveNoConstraints();
x[validator.Symbol("success")].Should().HaveOnlyConstraints(ObjectConstraint.NotNull, BoolConstraint.False);
x[validator.Symbol("result")].Should().HaveNoConstraints();
});
}

Expand Down
6 changes: 3 additions & 3 deletions analyzers/tests/SonarAnalyzer.UnitTest/SymbolicExecution/Roslyn/SymbolicValueTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ public void WithoutConstraint_RemovesOnlyTheSame()
.WithoutConstraint(TestConstraint.Second); // Do nothing
sut.HasConstraint(TestConstraint.First).Should().BeTrue();
sut = sut.WithoutConstraint(TestConstraint.First);
sut.HasConstraint(TestConstraint.First).Should().BeFalse();
sut.Should().BeNull();
}

[TestMethod]
Expand Down Expand Up @@ -242,14 +242,14 @@ public void TripletCache_ReplaceExistingConstraintType()
public void SingleCache_RemoveLastEntry_Kind()
{
var sut = SymbolicValue.Null;
sut.WithoutConstraint(ObjectConstraint.Null).Should().BeSameAs(SymbolicValue.Empty);
sut.WithoutConstraint(ObjectConstraint.Null).Should().BeNull();
}

[TestMethod]
public void SingleCache_RemoveLastEntry_Type()
{
var sut = SymbolicValue.Null;
sut.WithoutConstraint<ObjectConstraint>().Should().BeSameAs(SymbolicValue.Empty);
sut.WithoutConstraint<ObjectConstraint>().Should().BeNull();
}

[TestMethod]
Expand Down
34 changes: 23 additions & 11 deletions ...yzer.UnitTest/TestCases/SymbolicExecution/Roslyn/EmptyCollectionsShouldNotBeEnumerated.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -105,15 +105,19 @@ public void ConstructorWithEnumerableWithConstraint(bool condition)
var set = new HashSet<int>(baseCollection);
set.Clear(); // Noncompliant

baseCollection = new List<int>();
set = new HashSet<int>(baseCollection, EqualityComparer<int>.Default);
set.Clear(); // Noncompliant

baseCollection = new List<int>();
set = new HashSet<int>(comparer: EqualityComparer<int>.Default, collection: baseCollection);
set.Clear(); // Noncompliant

baseCollection = new List<int>();
set = new HashSet<int>(condition ? baseCollection : baseCollection);
set.Clear(); // Noncompliant

baseCollection = new List<int>();
baseCollection.Add(1);
set = new HashSet<int>(baseCollection);
set.Clear(); // Compliant
Expand Down Expand Up @@ -438,7 +442,7 @@ public void UnknownExtensionMethods()
{
var list = new List<int>();
list.CustomExtensionMethod(); // Compliant
list.Clear(); // Noncompliant FP
list.Clear(); // Compliant
}

public void WellKnownExtensionMethods()
Expand Down Expand Up @@ -489,14 +493,22 @@ public void WellKnownExtensionMethods()
list.Where(x => true); // FN
list.Zip(list, (x, y) => x); // FN
Enumerable.Reverse(list); // FN
list.Clear(); // Noncompliant
list.Clear(); // FN, should raise, because the methods above should not reset the state
}

public void PassingAsArgument_Removes_Constraints()
public void PassingAsArgument_Removes_Constraints(bool condition)
{
var list = new List<int>();
Foo(list);
list.Clear(); // Noncompliant FP
list.Clear(); // Compliant

list = new List<int>();
Foo(condition ? list : null);
list.Clear(); // Compliant

list = new List<int>();
Foo((condition ? list : null) as List<int>);
list.Clear(); // Compliant
}

public void HigherRank_And_Jagged_Array()
Expand Down Expand Up @@ -558,7 +570,7 @@ public void LearnConditions_Size(bool condition, List<int> arg)

if (empty.Count() == 0)
{
empty.Clear(); // Noncompliant
empty.Clear(); // FN
}
else
{
Expand All @@ -567,7 +579,7 @@ public void LearnConditions_Size(bool condition, List<int> arg)

if (empty.Count(x => condition) == 0)
{
empty.Clear(); // Noncompliant
empty.Clear(); // FN
}
else
{
Expand All @@ -576,16 +588,16 @@ public void LearnConditions_Size(bool condition, List<int> arg)

if (notEmpty.Count(x => condition) == 0)
{
empty.Clear(); // Noncompliant
empty.Clear(); // FN
}
else
{
empty.Clear(); // Noncompliant
empty.Clear(); // FN
}

if (Enumerable.Count(empty) == 0)
{
empty.Clear(); // Noncompliant
empty.Clear(); // FN
}
else
{
Expand Down Expand Up @@ -621,7 +633,7 @@ public void LearnConditions_Size_Array(bool condition)

if (empty.Count() == 0)
{
empty.Clone(); // Noncompliant
empty.Clone(); // FN
}
else
{
Expand All @@ -631,7 +643,7 @@ public void LearnConditions_Size_Array(bool condition)
notEmpty.Clone(); // Compliant, prevents LVA from throwing notEmpty away during reference capture
}

void Foo(IEnumerable<int> items) { }
void Foo(List<int> items) { }
}

static class CustomExtensions
Expand Down
Loading

0 comments on commit 6dceab4

Please sign in to comment.