Visit left operand conversion in a fast path of nullable analysis - #85115
Conversation
jjonescz
added
Area-Compilers
Feature - Nullable Reference Types
|
Azure Pipelines: Successfully started running 1 pipeline(s). 1 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
🔵 Needs a closer look
Pull request overview
Fixes a nullable-analysis fast path that skipped the left operand’s implicit conversion, triggering debug verification failures.
Changes:
- Visits the stripped left conversion before evaluating the right operand.
- Adds regression coverage for
default != true. - No code issues identified; CI remains in progress.
File summaries
| File | Description |
|---|---|
NullableWalker.cs |
Records the omitted conversion in nullable analysis. |
NullableReferenceTypesTests.cs |
Adds the issue regression test. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Balanced
dce3074 to
03e7af9
Compare
|
Azure Pipelines: Successfully started running 1 pipeline(s). 1 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
| if (binary.Left is BoundConversion leftConversionNode && leftConversionNode != leftOperand) | ||
| { | ||
| Debug.Assert(leftConversionNode.Type is not null); | ||
| VisitConversion( |
There was a problem hiding this comment.
Is this double-visiting the operand itself? Should we instead just be calling SetAnalyzedNullability?
There was a problem hiding this comment.
No, this calls the private VisitConversion overload which assumes the operand was already visited, it does not visit it again. I think a double visit would also fail some debug assert.
I think calling VisitConversion is correct. It's what other code paths of VisitBinaryOperatorChildren eventually call too - see line 5670.
| // The left operand was visited without its stripped conversion. | ||
| if (binary.Left is BoundConversion leftConversionNode && leftConversionNode != leftOperand) | ||
| { | ||
| Debug.Assert(leftConversionNode.Type is not null); |
There was a problem hiding this comment.
nit: It looks like the constructor of BoundConversion already asserts this
There was a problem hiding this comment.
🔵 Needs a closer look
The fix appears correct, but it modifies sensitive nullable-flow logic and CI for the latest commit remains in progress.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Fixes #84642.
In
VisitBinaryOperatorChildren, the conversion from the LHS is removed but then in the code path going throughlearnFromConditionalAccessOrBoolConstant, the removed conversion wasn't visited, failing the assert.Microsoft Reviewers: Open in CodeFlow