Skip to content

Commit

Permalink
Revert "Fix S1854 FN: Support &&, ||, ?? and ??= (#9536)"
Browse files Browse the repository at this point in the history
This reverts commit 2c9c49a.
  • Loading branch information
mary-georgiou-sonarsource committed Aug 2, 2024
1 parent 4f2b11b commit 95b89ac
Show file tree
Hide file tree
Showing 5 changed files with 37 additions and 73 deletions.
Original file line number Diff line number Diff line change
@@ -1,11 +1,5 @@
{
"Issues": [
{
"Id": "S1854",
"Message": "Remove this useless assignment to local variable \u0027person\u0027.",
"Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/CSharpLatest/CSharpLatest/CSharp11Features/RequiredMembers.cs#L19",
"Location": "Line 19 Position 13-40"
},
{
"Id": "S1854",
"Message": "Remove this useless assignment to local variable \u0027person\u0027.",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ protected override IEnumerable<BasicBlock> Successors(BasicBlock block) =>
protected override State ProcessBlock(BasicBlock block)
{
var ret = new RoslynState(this);
ret.ProcessBlock(Cfg, block);
ret.ProcessBlock(Cfg, block, flowCaptures);
return ret;
}

Expand Down Expand Up @@ -245,11 +245,18 @@ private sealed class RoslynState : State
public RoslynState(RoslynLiveVariableAnalysis owner) =>
this.owner = owner;

public void ProcessBlock(ControlFlowGraph cfg, BasicBlock block)
public void ProcessBlock(ControlFlowGraph cfg, BasicBlock block, Dictionary<CaptureId, List<ISymbol>> flowCaptureOperations)
{
foreach (var operation in block.OperationsAndBranchValue.ToReversedExecutionOrder().Select(x => x.Instance))
{
ProcessOperation(cfg, operation);
if (operation.AsFlowCaptureReference() is { } flowCaptureReference && flowCaptureOperations.TryGetValue(flowCaptureReference.Id, out var symbols))
{
UsedBeforeAssigned.UnionWith(symbols);
}
else
{
ProcessOperation(cfg, operation);
}
}
}

Expand All @@ -264,9 +271,6 @@ private void ProcessOperation(ControlFlowGraph cfg, IOperation operation)
case OperationKindEx.ParameterReference:
ProcessParameterOrLocalReference(IParameterReferenceOperationWrapper.FromOperation(operation));
break;
case OperationKindEx.FlowCaptureReference:
ProcessParameterOrLocalReference(IFlowCaptureReferenceOperationWrapper.FromOperation(operation));
break;
case OperationKindEx.SimpleAssignment:
ProcessSimpleAssignment(ISimpleAssignmentOperationWrapper.FromOperation(operation));
break;
Expand All @@ -290,7 +294,7 @@ private void ProcessParameterOrLocalReference(IOperationWrapper reference)
Assigned.UnionWith(symbols);
UsedBeforeAssigned.ExceptWith(symbols);
}
else if (!reference.IsAssignmentTarget() && reference.ToSonar().Parent?.Kind != OperationKindEx.FlowCapture)
else if (!reference.IsAssignmentTarget())
{
UsedBeforeAssigned.UnionWith(symbols);
}
Expand Down Expand Up @@ -355,7 +359,7 @@ private void ProcessLocalFunction(ControlFlowGraph cfg, IMethodSymbol method)
var localFunctionCfg = cfg.FindLocalFunctionCfgInScope(localFunction, owner.Cancel);
foreach (var block in localFunctionCfg.Blocks.Reverse()) // Simplified approach, ignoring branching and try/catch/finally flows
{
ProcessBlock(localFunctionCfg, block);
ProcessBlock(localFunctionCfg, block, []);
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,8 @@ public void FlowCaptrure_NullCoalescingAssignment()
var context = CreateContextCS(code, additionalParameters: "string param");
context.ValidateEntry(LiveIn("param"), LiveOut("param"));
context.Validate(context.Cfg.Blocks[1], LiveIn("param"), LiveOut("param"));
context.Validate(context.Cfg.Blocks[2], LiveIn("param"));
context.Validate(context.Cfg.Blocks[3]);
context.Validate(context.Cfg.Blocks[2], LiveIn("param"), LiveOut("param"));
context.Validate(context.Cfg.Blocks[3], LiveIn("param"));
context.ValidateExit();
}

Expand Down Expand Up @@ -214,20 +214,20 @@ public void FlowCaptrure_NullCoalescingOperator_ConsequentCalls_Assignment()
{
const string code = """var result = s1 ??= s2 = s3 ??= s4 ?? "End";""";
var context = CreateContextCS(code, additionalParameters: "string s1, string s2, string s3, string s4");
context.ValidateEntry(LiveIn("s1", "s3", "s4"), LiveOut("s1", "s3", "s4"));
context.Validate(context.Cfg.Blocks[1], LiveIn("s1", "s3", "s4"), LiveOut("s1", "s3", "s4")); // 1: #0=s1
context.Validate(context.Cfg.Blocks[2], LiveIn("s1", "s3", "s4"), LiveOut("s1", "s3", "s4")); // 2: #1=#0; if #1 is null
context.Validate(context.Cfg.Blocks[3], LiveIn("s1"), LiveOut("s1")); // 3: F: #2=#1
context.Validate(context.Cfg.Blocks[4], LiveIn("s3", "s4"), LiveOut("s3", "s4")); // 4: T: #3=s2
context.Validate(context.Cfg.Blocks[5], LiveIn("s3", "s4"), LiveOut("s3", "s4")); // 5: #4=s3
context.Validate(context.Cfg.Blocks[6], LiveIn("s3", "s4"), LiveOut("s3", "s4")); // 6: #5=#4; if #5 is null
context.Validate(context.Cfg.Blocks[7], LiveIn("s3"), LiveOut("s3")); // 7: F: #6=#5
context.Validate(context.Cfg.Blocks[8], LiveIn("s4"), LiveOut("s4")); // 8: T: #7=s4; if #7 is null
context.Validate(context.Cfg.Blocks[9], LiveIn("s4"), LiveOut("s4")); // 9: F: #8=#7
context.Validate(context.Cfg.Blocks[10], LiveIn("s4"), LiveOut("s4")); // 10: #7=null; #8="End"
context.Validate(context.Cfg.Blocks[11], LiveIn("s4"), LiveOut("s3")); // 11: #6= (#4=#8)
context.Validate(context.Cfg.Blocks[12], LiveIn("s3"), LiveOut("s1")); // 12: #2= (#0 = (#3=#6) )
context.Validate(context.Cfg.Blocks[13], LiveIn("s1")); // 13: result=#2
context.ValidateEntry(LiveIn("s1", "s2", "s3", "s4"), LiveOut("s1", "s2", "s3", "s4"));
context.Validate(context.Cfg.Blocks[1], LiveIn("s1", "s2", "s3", "s4"), LiveOut("s1", "s2", "s3", "s4")); // 1: #0=s1
context.Validate(context.Cfg.Blocks[2], LiveIn("s1", "s2", "s3", "s4"), LiveOut("s1", "s2", "s3", "s4")); // 2: #1=#0; if #1 is null
context.Validate(context.Cfg.Blocks[3], LiveIn("s1"), LiveOut("s1")); // 3: F: #2=#1
context.Validate(context.Cfg.Blocks[4], LiveIn("s1", "s2", "s3", "s4"), LiveOut("s1", "s2", "s3", "s4")); // 4: T: #3=s2
context.Validate(context.Cfg.Blocks[5], LiveIn("s1", "s2", "s3", "s4"), LiveOut("s1", "s2", "s3", "s4")); // 5: #4=s3
context.Validate(context.Cfg.Blocks[6], LiveIn("s1", "s2", "s3", "s4"), LiveOut("s1", "s2", "s3", "s4")); // 6: #5=#4; if #5 is null
context.Validate(context.Cfg.Blocks[7], LiveIn("s1", "s2", "s3"), LiveOut("s1", "s2", "s3")); // 7: F: #6=#5
context.Validate(context.Cfg.Blocks[8], LiveIn("s1", "s2", "s3", "s4"), LiveOut("s1", "s2", "s3", "s4")); // 8: T: #7=s4; if #7 is null
context.Validate(context.Cfg.Blocks[9], LiveIn("s1", "s2", "s3", "s4"), LiveOut("s1", "s2", "s3", "s4")); // 9: F: #8=#7
context.Validate(context.Cfg.Blocks[10], LiveIn("s1", "s2", "s3", "s4"), LiveOut("s1", "s2", "s3", "s4")); // 10: #7=null; #8="End"
context.Validate(context.Cfg.Blocks[11], LiveIn("s1", "s2", "s3", "s4"), LiveOut("s1", "s2", "s3")); // 11: #6= (#4=#8)
context.Validate(context.Cfg.Blocks[12], LiveIn("s1", "s2", "s3"), LiveOut("s1")); // 12: #2= (#0 = (#3=#6) )
context.Validate(context.Cfg.Blocks[13], LiveIn("s1")); // 13: result=#2
context.ValidateExit();
}

Expand Down Expand Up @@ -301,13 +301,12 @@ public void FlowCaptrure_NullCoalescingOperator_Overwrite()
*/
const string code = """s1 = (s1 = "overwrite") ?? "value";""";
var context = CreateContextCS(code, additionalParameters: "string s1");
// s1 is never read. The assignment returns its r-value, which is used for further calculation.
context.ValidateEntry();
context.Validate(context.Cfg.Blocks[1]);
context.Validate(context.Cfg.Blocks[2]);
context.Validate(context.Cfg.Blocks[3]);
context.Validate(context.Cfg.Blocks[4]);
context.Validate(context.Cfg.Blocks[5]);
context.ValidateEntry(LiveIn("s1"), LiveOut("s1"));
context.Validate(context.Cfg.Blocks[1], LiveIn("s1"));
context.Validate(context.Cfg.Blocks[2], LiveOut("s1")); // This should have LiveIn("s1") and LiveOut("s1") but #1 gets as value all the assignment operation.
context.Validate(context.Cfg.Blocks[3], LiveIn("s1"), LiveOut("s1"));
context.Validate(context.Cfg.Blocks[4], LiveIn("s1"), LiveOut("s1"));
context.Validate(context.Cfg.Blocks[5], LiveIn("s1"));
context.ValidateExit();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -233,36 +233,14 @@ public void LocalFunctionInvocation_Recursive_LiveIn()
return;
LocalFunction(10);

int LocalFunction(int cnt)
{
if (cnt == 0)
return 0;
return variable + LocalFunction(cnt - 1);
}
int LocalFunction(int cnt) => variable + (cnt == 0 ? 0 : LocalFunction(cnt - 1));
""";
var context = CreateContextCS(code);
context.ValidateEntry(LiveIn("boolParameter"), LiveOut("boolParameter"));
context.Validate("boolParameter", LiveIn("boolParameter"), LiveOut("variable"));
context.Validate("LocalFunction(10);", LiveIn("variable"));
}

[TestMethod]
public void LocalFunctionInvocation_Recursive_FlowCapture()
{
var code = """
var variable = 42;
if (boolParameter)
return;
LocalFunction(10);

int LocalFunction(int cnt) => variable + (cnt == 0 ? 0 : LocalFunction(cnt - 1));
""";
var context = CreateContextCS(code);
context.ValidateEntry(LiveIn("boolParameter"), LiveOut("boolParameter"));
context.Validate("boolParameter", LiveIn("boolParameter")); // should be LiveOut("variable") but FlowCaptures inside of local functions are not resolved
context.Validate("LocalFunction(10);"); // should be Livein("variable")
}

[TestMethod]
public void LocalFunctionInvocation_Recursive_WhenAnalyzingLocalFunctionItself_LiveIn()
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -648,22 +648,11 @@ public void Unused()
private void ConditionalEvaluation(bool b1, bool b2, object coalesce, object coalesceAssignment)
{
var x = false; // Compliant ignored value
x = true; // Noncompliant
x = b1 && b2; // Noncompliant
x = true; // Roslyn CFG FN: Consequence of inaccurate LVA state below
x = b1 && b2; // Roslyn CFG FN: Branching with FlowCaptureOperation
x = b1 || b2; // Noncompliant
coalesce = coalesce ?? "Value"; // Noncompliant
coalesceAssignment ??= "Value"; // Noncompliant

DeadStores lst;
lst = new DeadStores // Noncompliant
{
Property = 42
};
lst = new DeadStores
{
Property = 42
};
lst.ToString();
}

private void SimpleAssignment()
Expand Down

0 comments on commit 95b89ac

Please sign in to comment.