Fix unexpected conditional state in nullable analysis of conditional access #78439
Conversation
ghost
added
Area-Compilers
untriaged
|
|
||
| // Per LDM 2019-02-13 decision, the result of a conditional access "may be null" even if | ||
| // both the receiver and right-hand-side are believed not to be null. | ||
| UnsplitIfNeeded(resultType); |
There was a problem hiding this comment.
Do we understand why we are left in a conditional state?
We analyze the call ""?.M1(x) and since M1 has post-condition on the argument, it leaves us in conditional state.
Could you explain why do you think this is the right place for the fix?
VisitConditionalAccess needs to set a result type to maybe-null. The assert in SetResultType says we can call it in conditional state only for bools (which makes sense, other types cannot be conditional). So calling UnsplitIfNeeded beforehand ensures we are not in conditional state when the type is not bool.
|
|
||
| // Per LDM 2019-02-13 decision, the result of a conditional access "may be null" even if | ||
| // both the receiver and right-hand-side are believed not to be null. | ||
| UnsplitIfNeeded(resultType); |
There was a problem hiding this comment.
I thought if we are analyzing "a"?.M1(x), and M1 returns bool, then resultType is bool? here. So why would we ever keep in a split state at this point? Would we achieve the correct behavior more simply if we just always called Unsplit() after the VisitPossibleConditionalAccess on L5928 (the non-null-const receiver case)?
There was a problem hiding this comment.
Sounds good, let me try that. Thanks.
There was a problem hiding this comment.
Turns out it is not easy to move UnsplitIfNeeded into the if above - we don't know the resultType there (the bool?) we only know LvalueResultType but that's bool. We would need to compute the resultType sooner, and in two places. Which seems unnecessary - we can just leave the UnsplitIfNeeded here where it is.
There was a problem hiding this comment.
I don’t think we should need or use the resultType with the normal Unsplit. But, if the suggestion didn’t work out, I’m ok with the current implementation. Thanks!
There was a problem hiding this comment.
Right, sorry, I misread your original suggestion, it makes sense, let me try that.
* upstream/main: (415 commits) Use lazy initialization for members in CodeFixService (dotnet#78484) Targeted perf changes to CommandLineParser (dotnet#78446) Exclude VS.ExternalAPIs.Roslyn.Package from source-build Add syntax highlighting of ignored directives (dotnet#78458) Fix unexpected conditional state in nullable analysis of conditional access (dotnet#78439) Update RoslynAnalyzers to use Roslyns version Fix source-build by renaming Assets folder to assets Unlist M.CA.AnalyzerUtilities as an expected DevDivInsertion dependency Use a project reference for M.CA.AnalyzerUtilities Rectify status of dictionary expressions (dotnet#78497) Remove unused field Remove unused field Remove using Fix check Update eng/config/PublishData.json Make internal Remove now unused methods from IWorkspaceProjectContext (dotnet#78479) Reduce allocations during SourceGeneration (dotnet#78403) Update Roslyn.sln Merge EditorFeatures.Wpf entirely into EditorFeatures ...
3 participants
Fixes #78386.