Skip to content

Commit

Permalink
Fix S3900 FP: After coalesce with assignment. (#7023)
Browse files Browse the repository at this point in the history
  • Loading branch information
@pavel-mikula-sonarsource
pavel-mikula-sonarsource authored Apr 5, 2023
1 parent 8b872cf commit 474a363
Show file tree
Hide file tree
Showing 9 changed files with 173 additions and 157 deletions.
26 changes: 0 additions & 26 deletions analyzers/its/expected/akka.net/Akka.Persistence.Query.Sql--netstandard2.0-S3900.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,32 +12,6 @@
"endColumn": 38
}
}
},
{
"id": "S3900",
"message": "Refactor this method to add validation of parameter 'offset' before using it.",
"location": {
"uri": "sources\akka.net\src\contrib\persistence\Akka.Persistence.Query.Sql\SqlReadJournal.cs",
"region": {
"startLine": 211,
"startColumn": 84,
"endLine": 211,
"endColumn": 90
}
}
},
{
"id": "S3900",
"message": "Refactor this method to add validation of parameter 'offset' before using it.",
"location": {
"uri": "sources\akka.net\src\contrib\persistence\Akka.Persistence.Query.Sql\SqlReadJournal.cs",
"region": {
"startLine": 232,
"startColumn": 84,
"endLine": 232,
"endColumn": 90
}
}
}
]
}
17 changes: 17 additions & 0 deletions analyzers/its/expected/akka.net/Akka.Persistence.TestKit--netstandard2.0-S2259.json
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
{
"issues": [
{
"id": "S2259",
"message": "'(IEnumerable<IPersistentRepresentation>)w.Payload' is null on at least one execution path.",
"location": {
"uri": "sources\akka.net\src\core\Akka.Persistence.TestKit\Journal\TestJournal.cs",
"region": {
"startLine": 52,
"startColumn": 39,
"endLine": 52,
"endColumn": 88
}
}
}
]
}
78 changes: 0 additions & 78 deletions analyzers/its/expected/akka.net/Akka.TestKit--netstandard2.0-S3900.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,19 +28,6 @@
},
{
"id": "S3900",
"message": "Refactor this method to add validation of parameter 'matchedEventHandler' before using it.",
"location": {
"uri": "sources\akka.net\src\core\Akka.TestKit\EventFilter\Internal\EventFilterApplier.cs",
"region": {
"startLine": 287,
"startColumn": 44,
"endLine": 287,
"endColumn": 63
}
}
},
{
"id": "S3900",
"message": "Refactor this method to add validation of parameter 'func' before using it.",
"location": {
"uri": "sources\akka.net\src\core\Akka.TestKit\EventFilter\Internal\EventFilterApplier.cs",
Expand All @@ -54,32 +41,6 @@
},
{
"id": "S3900",
"message": "Refactor this method to add validation of parameter 'matchedEventHandler' before using it.",
"location": {
"uri": "sources\akka.net\src\core\Akka.TestKit\EventFilter\Internal\EventFilterApplier.cs",
"region": {
"startLine": 293,
"startColumn": 48,
"endLine": 293,
"endColumn": 67
}
}
},
{
"id": "S3900",
"message": "Refactor this method to add validation of parameter 'matchedEventHandler' before using it.",
"location": {
"uri": "sources\akka.net\src\core\Akka.TestKit\EventFilter\Internal\EventFilterApplier.cs",
"region": {
"startLine": 320,
"startColumn": 44,
"endLine": 320,
"endColumn": 63
}
}
},
{
"id": "S3900",
"message": "Refactor this method to add validation of parameter 'system' before using it.",
"location": {
"uri": "sources\akka.net\src\core\Akka.TestKit\EventFilter\Internal\EventFilterApplier.cs",
Expand All @@ -93,19 +54,6 @@
},
{
"id": "S3900",
"message": "Refactor this method to add validation of parameter 'matchedEventHandler' before using it.",
"location": {
"uri": "sources\akka.net\src\core\Akka.TestKit\EventFilter\Internal\EventFilterApplier.cs",
"region": {
"startLine": 350,
"startColumn": 44,
"endLine": 350,
"endColumn": 63
}
}
},
{
"id": "S3900",
"message": "Refactor this method to add validation of parameter 'func' before using it.",
"location": {
"uri": "sources\akka.net\src\core\Akka.TestKit\EventFilter\Internal\EventFilterApplier.cs",
Expand All @@ -123,32 +71,6 @@
"location": {
"uri": "sources\akka.net\src\core\Akka.TestKit\EventFilter\Internal\EventFilterApplier.cs",
"region": {
"startLine": 356,
"startColumn": 48,
"endLine": 356,
"endColumn": 67
}
}
},
{
"id": "S3900",
"message": "Refactor this method to add validation of parameter 'matchedEventHandler' before using it.",
"location": {
"uri": "sources\akka.net\src\core\Akka.TestKit\EventFilter\Internal\EventFilterApplier.cs",
"region": {
"startLine": 383,
"startColumn": 44,
"endLine": 383,
"endColumn": 63
}
}
},
{
"id": "S3900",
"message": "Refactor this method to add validation of parameter 'matchedEventHandler' before using it.",
"location": {
"uri": "sources\akka.net\src\core\Akka.TestKit\EventFilter\Internal\EventFilterApplier.cs",
"region": {
"startLine": 404,
"startColumn": 28,
"endLine": 404,
Expand Down
10 changes: 7 additions & 3 deletions analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/OperationProcessors/IsNull.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,13 @@ protected override SymbolicConstraint BoolConstraintFromOperation(ProgramState s
? BoolConstraint.From(value.HasConstraint(ObjectConstraint.Null))
: null;

protected override ProgramState LearnBranchingConstraint(ProgramState state, IIsNullOperationWrapper operation, bool falseBranch) =>
state.ResolveCapture(operation.Operand).TrackedSymbol() is { } testedSymbol
protected override ProgramState LearnBranchingConstraint(ProgramState state, IIsNullOperationWrapper operation, bool falseBranch)
{
var constraint = falseBranch ? ObjectConstraint.NotNull : ObjectConstraint.Null;
state = state.SetOperationConstraint(operation.Operand, constraint);
return state.ResolveCapture(operation.Operand).TrackedSymbol() is { } testedSymbol
// Can't use ObjectConstraint.ApplyOpposite() because here, we are sure that it is either Null or NotNull
? state.SetSymbolConstraint(testedSymbol, falseBranch ? ObjectConstraint.NotNull : ObjectConstraint.Null)
? state.SetSymbolConstraint(testedSymbol, constraint)
: state;
}
}
10 changes: 4 additions & 6 deletions ...lyzer.UnitTest/SymbolicExecution/Roslyn/RoslynSymbolicExecutionTest.ExceptionCandidate.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -373,18 +373,16 @@ public void ExceptionCandidate_ForEachLoop()
}
tag = ""AfterCatch"";";
var validator = SETestContext.CreateCS(code, ", IEnumerable<string> collection").Validator;
validator.ValidateTagOrder("BeforeTry", "InTry", "InCatch", "AfterCatch", "InCatch", "InCatch", "AfterCatch");

validator.ValidateTagOrder("BeforeTry", "InTry", "InCatch", "AfterCatch", "InCatch", "InCatch", "InCatch", "AfterCatch", "AfterCatch", "InCatch");
// IForEachLoopOperation is not generated. It doesn't seem to be used.

// In the case of foreach, there are implicit method calls that in the current implementation can throw:
// - IEnumerable<>.GetEnumerator()
// - System.Collections.IEnumerator.MoveNext()
// - System.IDisposable.Dispose()
validator.TagStates("InCatch").Should().HaveCount(3)
validator.TagStates("InCatch").Should().HaveCount(5)
.And.Contain(x => HasExceptionOfType(x, "NullReferenceException"))
.And.Subject.Where(HasUnknownException).Should().HaveCount(2);
validator.ExitStates.Should().HaveCount(2).And.OnlyContain(x => HasNoException(x));
.And.Subject.Where(HasUnknownException).Should().HaveCount(3);
validator.ExitStates.Should().HaveCount(3).And.OnlyContain(x => HasNoException(x));
}

[TestMethod]
Expand Down
147 changes: 109 additions & 38 deletions ...sts/SonarAnalyzer.UnitTest/SymbolicExecution/Roslyn/RoslynSymbolicExecutionTest.IsNull.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,40 +45,87 @@ public void IsNull_Coalesce_SetsObjectConstraint()
Tag(""NotNullToUnknown"", notNullToUnknown);";
var validator = SETestContext.CreateCS(code, ", object arg").Validator;
validator.ValidateContainsOperation(OperationKind.IsNull);
validator.ValidateTag("NullToNull", x => x.HasConstraint(ObjectConstraint.Null).Should().BeTrue());
validator.ValidateTag("NullToNotNull", x => x.HasConstraint(ObjectConstraint.NotNull).Should().BeTrue());
validator.ValidateTag("NullToUnknown", x => x.Should().BeNull());
validator.ValidateTag("NotNullToNull", x => x.HasConstraint(ObjectConstraint.NotNull).Should().BeTrue());
validator.ValidateTag("NotNullToNotNull", x => x.HasConstraint(ObjectConstraint.NotNull).Should().BeTrue());
validator.ValidateTag("NotNullToUnknown", x => x.HasConstraint(ObjectConstraint.NotNull).Should().BeTrue());
validator.ValidateTag("NullToNull", x => x.Should().HaveOnlyConstraint(ObjectConstraint.Null));
validator.ValidateTag("NullToNotNull", x => x.Should().HaveOnlyConstraint(ObjectConstraint.NotNull));
validator.ValidateTag("NullToUnknown", x => x.Should().HaveNoConstraints());
validator.ValidateTag("NotNullToNull", x => x.Should().HaveOnlyConstraint(ObjectConstraint.NotNull));
validator.ValidateTag("NotNullToNotNull", x => x.Should().HaveOnlyConstraint(ObjectConstraint.NotNull));
validator.ValidateTag("NotNullToUnknown", x => x.Should().HaveOnlyConstraint(ObjectConstraint.NotNull));
}

[TestMethod]
public void IsNull_Coalesce_UnknownToNull()
{
const string code = @"
object nullValue = null;
var unknownToNull = arg ?? nullValue;
Tag(""UnknownToNull"", unknownToNull);";
var validator = SETestContext.CreateCS(code, ", object arg").Validator;
const string code = """
object nullValue = null;
var result = arg ?? nullValue;
Tag("End");
""";
var validator = SETestContext.CreateCS(code, ", object arg", new PreserveTestCheck("arg", "result")).Validator;
var arg = validator.Symbol("arg");
var result = validator.Symbol("result");
validator.ValidateContainsOperation(OperationKind.IsNull);
validator.TagValues("UnknownToNull").Should().HaveCount(2)
.And.ContainSingle(x => x == null)
.And.ContainSingle(x => x != null && x.HasConstraint(ObjectConstraint.Null));
validator.TagStates("End").Should().SatisfyRespectively(
x =>
{
x[arg].Should().HaveOnlyConstraint(ObjectConstraint.Null);
x[result].Should().HaveOnlyConstraint(ObjectConstraint.Null); // It's from nullValue
},
x =>
{
x[arg].Should().HaveOnlyConstraint(ObjectConstraint.NotNull);
x[result].Should().HaveOnlyConstraint(ObjectConstraint.NotNull);
});
}

[TestMethod]
public void IsNull_Coalesce_UnknownToNotNull()
{
const string code = @"
object notNullValue = new object();
var unknownToNotNull = arg ?? notNullValue;
Tag(""UnknownToNotNull"", unknownToNotNull);";
var validator = SETestContext.CreateCS(code, ", object arg").Validator;
const string code = """
object notNullValue = new object();
var result = arg ?? notNullValue;
Tag("End");
""";
var validator = SETestContext.CreateCS(code, ", object arg", new PreserveTestCheck("arg", "result")).Validator;
var arg = validator.Symbol("arg");
var result = validator.Symbol("result");
validator.ValidateContainsOperation(OperationKind.IsNull);
validator.TagValues("UnknownToNotNull").Should().HaveCount(2)
.And.ContainSingle(x => x == null)
.And.ContainSingle(x => x != null && x.HasConstraint(ObjectConstraint.NotNull));
validator.TagStates("End").Should().SatisfyRespectively(
x =>
{
x[arg].Should().HaveOnlyConstraint(ObjectConstraint.Null);
x[result].Should().HaveOnlyConstraint(ObjectConstraint.NotNull);
},
x =>
{
x[arg].Should().HaveOnlyConstraint(ObjectConstraint.NotNull);
x[result].Should().HaveOnlyConstraint(ObjectConstraint.NotNull);
});
}

[TestMethod]
public void IsNull_Coalesce_UnknownToNotNull_WithConversion()
{
const string code = """
var notNullValue = new ArgumentException();
var result = arg ?? notNullValue;
Tag("End");
""";
var validator = SETestContext.CreateCS(code, ", Exception arg", new PreserveTestCheck("arg", "result")).Validator;
var arg = validator.Symbol("arg");
var result = validator.Symbol("result");
validator.ValidateContainsOperation(OperationKind.IsNull);
validator.TagStates("End").Should().SatisfyRespectively(
x =>
{
x[arg].Should().HaveOnlyConstraint(ObjectConstraint.NotNull);
x[result].Should().HaveOnlyConstraint(ObjectConstraint.NotNull);
},
x =>
{
x[arg].Should().HaveOnlyConstraint(ObjectConstraint.Null);
x[result].Should().HaveOnlyConstraint(ObjectConstraint.NotNull);
});
}

[TestMethod]
Expand All @@ -89,8 +136,31 @@ public void IsNull_Coalesce_UnknownToUnknown()
Tag(""UnknownToUnknown"", unknownToUnknown);";
var validator = SETestContext.CreateCS(code, ", object arg1, object arg2").Validator;
validator.ValidateContainsOperation(OperationKind.IsNull);
validator.TagValues("UnknownToUnknown").Should().HaveCount(1)
.And.ContainSingle(x => x == null);
validator.TagValues("UnknownToUnknown").Should().SatisfyRespectively(
x => x.Should().HaveNoConstraints(),
x => x.Should().HaveOnlyConstraint(ObjectConstraint.NotNull));
}

[TestMethod]
public void IsNull_Coalesce_AssignedToVariable()
{
const string code = """
var value = arg ?? "N/A";
Tag("Value", value);
""";
var validator = SETestContext.CreateCS(code, ", string arg").Validator;
validator.ValidateTag("Value", x => x.Should().HaveOnlyConstraint(ObjectConstraint.NotNull));
}

[TestMethod]
public void IsNull_Coalesce_AssignedToSelf()
{
const string code = """
arg = arg ?? "N/A";
Tag("Arg", arg);
""";
var validator = SETestContext.CreateCS(code, ", string arg").Validator;
validator.ValidateTag("Arg", x => x.Should().HaveOnlyConstraint(ObjectConstraint.NotNull));
}

[TestMethod]
Expand Down Expand Up @@ -120,12 +190,12 @@ public void IsNull_CoalesceAssignment_SetsObjectConstraint()
";
var validator = SETestContext.CreateCS(code, ", object arg").Validator;
validator.ValidateContainsOperation(OperationKind.IsNull);
validator.ValidateTag("NullToNull", x => x.HasConstraint(ObjectConstraint.Null).Should().BeTrue());
validator.ValidateTag("NullToNotNull", x => x.HasConstraint(ObjectConstraint.NotNull).Should().BeTrue());
validator.ValidateTag("NullToUnknown", x => x.Should().BeNull());
validator.ValidateTag("NotNullToNull", x => x.HasConstraint(ObjectConstraint.NotNull).Should().BeTrue());
validator.ValidateTag("NotNullToNotNull", x => x.HasConstraint(ObjectConstraint.NotNull).Should().BeTrue());
validator.ValidateTag("NotNullToUnknown", x => x.HasConstraint(ObjectConstraint.NotNull).Should().BeTrue());
validator.ValidateTag("NullToNull", x => x.Should().HaveOnlyConstraint(ObjectConstraint.Null));
validator.ValidateTag("NullToNotNull", x => x.Should().HaveOnlyConstraint(ObjectConstraint.NotNull));
validator.ValidateTag("NullToUnknown", x => x.Should().HaveNoConstraints());
validator.ValidateTag("NotNullToNull", x => x.Should().HaveOnlyConstraint(ObjectConstraint.NotNull));
validator.ValidateTag("NotNullToNotNull", x => x.Should().HaveOnlyConstraint(ObjectConstraint.NotNull));
validator.ValidateTag("NotNullToUnknown", x => x.Should().HaveOnlyConstraint(ObjectConstraint.NotNull));
}

[TestMethod]
Expand All @@ -137,9 +207,9 @@ public void IsNull_CoalesceAssignment_UnknownToNull()
Tag(""Arg"", arg);";
var validator = SETestContext.CreateCS(code, ", object arg").Validator;
validator.ValidateContainsOperation(OperationKind.IsNull);
validator.TagValues("Arg").Should().HaveCount(2)
.And.ContainSingle(x => x.HasConstraint(ObjectConstraint.NotNull))
.And.ContainSingle(x => x.HasConstraint(ObjectConstraint.Null));
validator.TagValues("Arg").Should().SatisfyRespectively(
x => x.Should().HaveOnlyConstraint(ObjectConstraint.NotNull),
x => x.Should().HaveOnlyConstraint(ObjectConstraint.Null));
}

[TestMethod]
Expand All @@ -151,8 +221,9 @@ public void IsNull_CoalesceAssignment_UnknownToNotNull()
Tag(""Arg"", arg);";
var validator = SETestContext.CreateCS(code, ", object arg").Validator;
validator.ValidateContainsOperation(OperationKind.IsNull);
validator.TagValues("Arg").Should().HaveCount(2)
.And.OnlyContain(x => x.HasConstraint(ObjectConstraint.NotNull));
validator.TagValues("Arg").Should().SatisfyRespectively(
x => x.Should().HaveOnlyConstraint(ObjectConstraint.NotNull), // This should be here just once, one state persists notNullValue without LVA removing it
x => x.Should().HaveOnlyConstraint(ObjectConstraint.NotNull));
}

[TestMethod]
Expand All @@ -163,9 +234,9 @@ public void IsNull_CoalesceAssignment_UnknownToUnknown()
Tag(""Arg"", arg1);";
var validator = SETestContext.CreateCS(code, ", object arg1, object arg2").Validator;
validator.ValidateContainsOperation(OperationKind.IsNull);
validator.TagValues("Arg").Should().HaveCount(2)
.And.ContainSingle(x => x == null)
.And.ContainSingle(x => x != null && x.HasConstraint(ObjectConstraint.NotNull));
validator.TagValues("Arg").Should().SatisfyRespectively(
x => x.Should().HaveOnlyConstraint(ObjectConstraint.NotNull),
x => x.Should().HaveNoConstraints());
}

[TestMethod]
Expand Down
Loading

0 comments on commit 474a363

Please sign in to comment.