Learn from binary operators on non-null constant values #33929
Conversation
jcouv
added
Area-Compilers
New Language Feature - Nullable Reference Types
jcouv
force-pushed
the
learn-comparisons
branch
3 times, most recently
from
3a11af2
to
b748e6b
Compare
|
@dotnet/roslyn-compiler FYI, I just rebased to resolve conflicts. This PR is ready for review. Thanks #Resolved |
| operandComparedToNonNull = SkipReferenceConversions(operandComparedToNonNull); | ||
| splitAndLearnFromNonNullTest(operandComparedToNonNull, caseToLearn: true); | ||
| return; | ||
| default: |
There was a problem hiding this comment.
We should learn from a != test as well. In if (o != 1) we know o is not null in the else branch. #Resolved
There was a problem hiding this comment.
Please either make a change or file an issue (your choice)
In reply to: 263641782 [](ancestors = 263641782)
| } | ||
|
|
||
| static BoundExpression skipImplicitNullableConversions(BoundExpression possiblyConversion) | ||
| { |
There was a problem hiding this comment.
while (possiblyConversion.Kind == BoundKind.Conversion && possiblyConversion is BoundConversion { ConversionKind: ConversionKind.ImplicitNullable, Operand: var operand })
{
possiblyConversion = operand;
}
return possiblyConversion;
#Resolved
| operandComparedToNonNull = binary.Left; | ||
| } | ||
|
|
||
| if (operandComparedToNonNull != null) |
There was a problem hiding this comment.
What happens in the case if (null == null)?
There was a problem hiding this comment.
Nothing will happen because there is no slot to update for either null.
There is however a scenario that is a bit weird:
const string? nullable = null;
const string? nonNullable = "";
if (nullable == nonNullable)We could say that we don't update constants, or maybe we need to distinguish null from other null-valued constants...
| return possiblyConversion; | ||
| } | ||
|
|
||
| // If caseToLearn is true, we'll learn in the when-true branch, otherwise in the when-false branch. |
There was a problem hiding this comment.
caseToLearn [](start = 18, length = 11)
Perhaps name whenTrue. #Resolved
|
|
||
| // learn from non-null constant | ||
| BoundExpression operandComparedToNonNull = null; | ||
| if (skipImplicitNullableConversions(binary.Left).ConstantValue?.IsNull == false) |
There was a problem hiding this comment.
skipImplicitNullableConversions(binary.Left).ConstantValue?.IsNull == false [](start = 16, length = 75)
Consider extracting an isNullConstant() local function. #Resolved
| @@ -63105,25 +63424,16 @@ class Program | |||
| static void F1(object[]? x1, object[]? y1) | |||
| { | |||
| foreach (var x in x1) { } // 1 | |||
| } | |||
| static void F2(object[]? x1, object[]? y1) | |||
| { | |||
| foreach (var x in x1) { } | |||
| } | |||
| static void F3(object[]? x1, object[]? y1) | |||
There was a problem hiding this comment.
F3 [](start = 16, length = 2)
Consider renumbering these methods. #Resolved
* Track inferred state changes in a finally block Fixes dotnet#33446 * Add a test so we don't regress when merging with dotnet#33929 * Add some comments and tests * Use `LearnFromNullTest` in recently integrated code in `NullableWalker` * Remove Unused Parameter
Two changes in this PR:
!=) to a non-null constant is a non-null testnull(x == null,x != null,(Type)x == null, ...) is a pure null test and therefore affects both branchesRelated issues:
?.expression compared with non-null #31906 (Infer nullability of receiver from?.expression compared with non-null)