Fix region analysis of == containing ?. #53687
Conversation
|
|
||
| // This function is only for the specific scenario where the LHS of a binary '=='/'!=' operator is a conditional access | ||
| // and the RHS is either a constant value or is of a non-nullable value type. | ||
| void visitRightOperandWithAnyTransferFunction(BoundExpression rightOperand, ref TLocalState leftStateWhenNotNull) |
There was a problem hiding this comment.
This implementation is the same shape as VisitCatchBlockWithAnyTransferFunction.
The theory here is that in some dataflow analyses such as DataFlowsOut, we may "flip" the behavior of definite assignment within the /*bind*/ region, so that assigning a variable really behaves as if you "unassigned" the variable. Inside this function, we can take any "unassignments" which occurred in the RHS and propagate them to both this.State and stateWhenNotNull.
We do this by taking the "bottom" state in which all variables are assigned, and keeping it "on the side" in NonMonotonicState when we visit the right operand. After the visit, the only "unassigned" variables in the NonMonotonicState were those unassigned by the right operand, and we can simply Join those unassignments into the stateWhenNotNull.
See CondAccess_Equals_DataFlowsOut_02 for a test case which is impacted by this change.
| public static void M(C? c) | ||
| { | ||
| int x = 0; | ||
| if (" + leftOperand + @" == /*<bind>*/c!.M0(x = 0)/*</bind>*/) |
There was a problem hiding this comment.
This is the test case where behavior is affected by the implementation change. Without the change, we don't include x in the DataFlowsOut set in this scenario.
| Assert.Equal("c", GetSymbolNamesJoined(dataFlowAnalysis.ReadInside)); | ||
| Assert.Equal("c, x", GetSymbolNamesJoined(dataFlowAnalysis.ReadOutside)); | ||
| Assert.Equal("x", GetSymbolNamesJoined(dataFlowAnalysis.WrittenInside)); | ||
| Assert.Equal("c, x", GetSymbolNamesJoined(dataFlowAnalysis.WrittenOutside)); |
There was a problem hiding this comment.
It appears that this is how parameters are handled in general. For example, the following test will pass in the main branch:
[Fact]
public void Test1()
{
var results = CompileAndAnalyzeDataFlowExpression(@"
class C
{
static void M(int i)
{
System.Console.Write(/*<bind>*/i/*</bind>*/);
}
}
");
Assert.Equal("i", GetSymbolNamesJoined(results.WrittenOutside));
}My speculation is that it might be useful to model a parameter as having been "written" at the beginning of a method.
| @@ -2452,6 +2451,35 @@ bool learnFromOperator(BoundBinaryOperator binary) | |||
|
|
|||
| return false; | |||
| } | |||
|
|
|||
| // This function is only for the specific scenario where the LHS of a binary '=='/'!=' operator is a conditional access | |||
| // and the RHS is either a constant value or is of a non-nullable value type. | |||
There was a problem hiding this comment.
Consider asserting these conditions. #Closed
There was a problem hiding this comment.
I found it prohibitively cumbersome to assert on this meaningfully, so I ended up deciding to just inline the local function at its call site.
|
Done review pass (commit 1). Just a couple of small questions/comments. |
|
@333fred for second review |
1 similar comment
|
@333fred for second review |
Related to #51463
This PR fixes region analysis of scenarios like DataFlowsOut for
a?.b(out x) == /*<bind>M(out x)</bind>*/by imitating the prior art for try/finally.Tagging @jcouv @333fred @dotnet/roslyn-compiler