Improved nullable '??' analysis #52646
Conversation
RikkiGibson
changed the base branch from
main
to
features/improved-definite-assignment
RikkiGibson
force-pushed
the
ida-nullable-coalesce
branch
from
4473bea
to
fa4e198
Compare
|
|
||
| if (access is object) | ||
| { | ||
| EnterRegionIfNeeded(access); |
There was a problem hiding this comment.
Do the Enter/LeaveRegionIfNeeded calls matter?
There was a problem hiding this comment.
When doing the definite assignment side of this feature, I had to add these calls in the analogous position over there, because the usage of VisitConditionalAccess(node, out stateWhenNotNull) caused us to side-step the regular machinery for managing region analysis in AbstractFlowPass.VisitAlways. When this was missing in definite assignment, "extract method" and similar feature tests were broken.
I do not actually know what dependent features could be broken by the absence of this in NullableWalker, or how to test it in a more direct manner, but it seems like something that we should have anyway.
There was a problem hiding this comment.
Other nullable analysis methods don't have this, except that we set a value once in Scan (not exactly sure what that achieves either). I think we should remove it unless a scenario warrants it.
In reply to: 620688053 [](ancestors = 620688053)
src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs
Show resolved
Hide resolved
|
Done review pass (commit 1) |
| static void M2(bool? b) | ||
| { | ||
| var obj = new object(); | ||
| _ = b?.M0(obj = null)?.M0(obj = new object()) ?? false |
|
I believe I've addressed all your feedback @333fred, please let me know if you have any more. |
|
|
||
| void M1(C? c1, object? x) | ||
| { | ||
| S s = (S?)c1?.M2(x = 0) ?? c1!.Value.M3(x = 0); // 1 |
There was a problem hiding this comment.
Consider using parens to make it clear where the cast is applying.
| { | ||
| static void M1(C c1, object? x) | ||
| { | ||
| B b = (B?)c1?.M1(x = 0) ?? c1!.M2(x = 0); // 1 |
There was a problem hiding this comment.
This one feels a bit wonky to me. My initial reaction is that it should warn for this case. #Closed
There was a problem hiding this comment.
We do warn for this case--just on the conversion instead of on usage of the updated variables. :)
The principle behind the behavior you're seeing is that we can propagate out "state when not null" from a user-defined conversion unless it can return non-null output for a null input. The rule I implemented based on this is that we can propagate out "state when not null" under the following conditions:
- either the parameter is non-nullable, or
- the return is nullable and has a
[return: NotNullIfNotNull]referencing the parameter.
The first rule means in practice that the user-defined conversion needs to have a non-nullable value type parameter and be lifted. Otherwise we either gave a warning on the conversion or there was a suppression on the operand.
Alternatively, we could adjust the rule to say we can propagate out "state when not null" when:
- the parameter is of a non-nullable value type, or
- the return is nullable and has a
[return: NotNullIfNotNull]referencing the parameter.
I'm not extremely attached to it going either way.
|
I decided to use the same rules for "acceptable conversions" in both definite assignment and nullable for the time being. The more I thought about it, the more niche/contrived it felt to use a user-defined conversion on something and then null test it. It's easier to "loosen up" and remove warnings down the line, so I think it's OK to remain strict for now. |
| } | ||
| "; | ||
| CreateNullableCompilation(new[] { source, NotNullIfNotNullAttributeDefinition }).VerifyDiagnostics( | ||
| // (13,9): warning CS8602: Dereference of a possibly null reference. |
There was a problem hiding this comment.
I think this a perfect example of the compromise of stricter methods here, and if we want to solve it for user-defined conversions, the equivalent static method should be solved as well :)
There was a problem hiding this comment.
Related to #36164
Test plan: #51463
Implements "improved" nullable analysis of null coalescing '??' operators. It may be useful to reference #51567 when reviewing this.