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

Jump to bottom

Fix S4158 FN: Learn Empty on Clear #7861

Merged
merged 4 commits into from
Aug 28, 2023
Merged

Fix S4158 FN: Learn Empty on Clear #7861

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4b6a3c7
Learn Empty on Clear
Tim-Pohlmann Aug 23, 2023
0f23ced
Refactor
Tim-Pohlmann Aug 23, 2023
844143f
Review 1
Tim-Pohlmann Aug 24, 2023
30badf2
Review 2
Tim-Pohlmann Aug 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
45 changes: 23 additions & 22 deletions ...r.Common/SymbolicExecution/Roslyn/RuleChecks/EmptyCollectionsShouldNotBeEnumeratedBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,8 @@ private ProgramState ProcessInvocation(SymbolicContext context, IInvocationOpera
}
}
return ProcessAddMethod(context.State, invocation.TargetMethod, instance)
?? ProcessRemoveMethod(context.State, invocation.TargetMethod, instance);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick:
Maybe you can retrieve once the invocation.TargetMethod.Name and pass it to all the calls as an argument.

?? ProcessRemoveMethod(context.State, invocation.TargetMethod, instance)
?? ProcessClearMethod(context.State, invocation.TargetMethod, instance);
}
else
{
Expand All @@ -206,30 +207,30 @@ private ProgramState ProcessInvocation(SymbolicContext context, IInvocationOpera
invocation.Arguments.Any(x => x.ToArgument().Parameter.Type.Is(KnownType.System_Func_T_TResult));
}

private static ProgramState ProcessAddMethod(ProgramState state, IMethodSymbol method, IOperation instance)
{
if (AddMethods.Contains(method.Name))
{
state = state.SetOperationConstraint(instance, CollectionConstraint.NotEmpty);
if (instance.TrackedSymbol(state) is { } symbol)
{
state = state.SetSymbolConstraint(symbol, CollectionConstraint.NotEmpty);
}
return state;
}
return null;
}
private static ProgramState ProcessAddMethod(ProgramState state, IMethodSymbol method, IOperation instance) =>
AddMethods.Contains(method.Name)
? SetOperationAndSymbolConstraint(state, instance, CollectionConstraint.NotEmpty)
: null;

private static ProgramState ProcessRemoveMethod(ProgramState state, IMethodSymbol method, IOperation instance)
private static ProgramState ProcessRemoveMethod(ProgramState state, IMethodSymbol method, IOperation instance) =>
RemoveMethods.Contains(method.Name)
? SetOperationAndSymbolValue(state, instance, (state[instance] ?? SymbolicValue.Empty).WithoutConstraint(CollectionConstraint.NotEmpty))
: null;

private static ProgramState ProcessClearMethod(ProgramState state, IMethodSymbol method, IOperation instance) =>
method.Name == nameof(ICollection<int>.Clear)
? SetOperationAndSymbolConstraint(state, instance, CollectionConstraint.Empty)
: state;

private static ProgramState SetOperationAndSymbolConstraint(ProgramState state, IOperation instance, SymbolicConstraint constraint) =>
SetOperationAndSymbolValue(state, instance, (state[instance] ?? SymbolicValue.Empty).WithConstraint(constraint));

private static ProgramState SetOperationAndSymbolValue(ProgramState state, IOperation instance, SymbolicValue value)
{
if (RemoveMethods.Contains(method.Name))
state = state.SetOperationValue(instance, value);
if (instance.TrackedSymbol(state) is { } symbol)
{
var value = (state[instance] ?? SymbolicValue.Empty).WithoutConstraint(CollectionConstraint.NotEmpty);
state = state.SetOperationValue(instance, value);
if (instance.TrackedSymbol(state) is { } symbol)
{
state = state.SetSymbolValue(symbol, value);
}
state = state.SetSymbolValue(symbol, value);
}
return state;
}
Expand Down
42 changes: 31 additions & 11 deletions ...yzer.UnitTest/TestCases/SymbolicExecution/Roslyn/EmptyCollectionsShouldNotBeEnumerated.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -412,27 +412,45 @@ public void Methods_Set_NotEmpty(bool condition)
public void Method_Set_Empty(List<int> list, HashSet<int> set, Queue<int> queue, Stack<int> stack, ObservableCollection<int> obs, Dictionary<int, int> dict)
{
list.Clear(); // Compliant
list.Clear(); // FN
list.Clear(); // Noncompliant

set.Clear(); // Compliant
set.Clear(); // FN
set.Clear(); // Noncompliant

queue.Clear(); // Compliant
queue.Clear(); // FN
queue.Clear(); // Noncompliant

stack.Clear(); // Compliant
stack.Clear(); // FN
stack.Clear(); // Noncompliant

obs.Clear(); // Compliant
obs.Clear(); // FN
obs.Clear(); // Noncompliant

dict.Clear(); // Compliant
dict.Clear(); // FN
dict.Clear(); // Noncompliant

var empty = new List<int>();
list.Add(5);
list.Intersect(empty); // Compliant
list.RemoveAll(x => true); // Compliant
list.Clear(); // FN
mary-georgiou-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
list.Add(5);
list.RemoveAll(x => x == 1); // Compliant
list.Clear(); // Compliant

set.Add(5);
set.RemoveWhere(x => true); // Compliant
set.Clear(); // FN
set.Add(5);
set.RemoveWhere(x => x == 1); // Compliant
set.Clear(); // Compliant

var empty = new List<int>();
set.Add(5);
set.IntersectWith(empty); // Compliant
set.Clear(); // FN
var notEmpty = new List<int> { 1 };
set.Add(5);
set.IntersectWith(notEmpty); // Compliant
set.Clear(); // Compliant
}
}

Expand Down Expand Up @@ -531,6 +549,8 @@ public void LearnConditions_Size(bool condition, List<int> arg)
var empty = new List<int>();
var notEmpty = new List<int>() { 1, 2, 3 };

// the tests below are messy for as long as we unlearn CollectionConstraints on empty.Count()
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand this comment :/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

empty.Count() will (wrongly) unlearn CollectionConstraint due to it being an extension method. The UTs here do not take this into consideration and thus provide unexpected results. They should be fixed when Count() does not unlearn anymore.


if ((isNull ?? empty).Count == 0)
{
empty.Clear(); // Noncompliant
Expand Down Expand Up @@ -588,11 +608,11 @@ public void LearnConditions_Size(bool condition, List<int> arg)

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

if (Enumerable.Count(empty) == 0)
Expand Down Expand Up @@ -736,7 +756,7 @@ public void AddPassedAsParameter()
Action<int> add = list.Add;
list.Clear(); // FN
add(5);
list.Clear(); // Compliant, but will break when we learn Empty from Clear()
list.Clear(); // Noncompliant FP

list = new List<int>();
Action clear = list.Clear; // We don't raise here to avoid FPs
Expand Down
2 changes: 1 addition & 1 deletion ...yzer.UnitTest/TestCases/SymbolicExecution/Roslyn/EmptyCollectionsShouldNotBeEnumerated.vb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ Public Class CollectionTests

Public Sub Method_Set_Empty(List As List(Of Integer))
List.Clear() ' Compliant
List.Clear() ' FN
List.Clear() ' Noncompliant

Dim Empty As New List(Of Integer)
List.Add(5)
Expand Down