Add invariant around nullable conditional state #72072

jcouv · 2024-02-12T22:45:08Z

As part of reviewing PR #72062, I was surprised to notice that we don't currently encode the rule that conditional states are only allowed for expressions of type bool (we only have StateWhenTrue and StateWhenFalse).
Here's a proposed assertion, along with a couple of fixes to unsplit when we're not dealing with a bool expression.

jcouv · 2024-02-13T23:52:23Z

src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs

@@ -1607,7 +1607,8 @@ public override BoundNode VisitUtf8String(BoundUtf8String node)

        protected void SplitIfBooleanConstant(BoundExpression node)
        {
-            if (node.ConstantValueOpt is { IsBoolean: true, BooleanValue: bool booleanValue })
+            if (node.ConstantValueOpt is { IsBoolean: true, BooleanValue: bool booleanValue }
+                && node.Type.SpecialType == SpecialType.System_Boolean)


📝 EnumTests.EnumUnsupportedUnderlyingType shows how we can get here.

jcouv · 2024-02-13T23:53:38Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

+        private bool TypeAllowsConditionalState(TypeSymbol? type)
+        {
+            return type is not null
+                && (type.SpecialType == SpecialType.System_Boolean || type.IsDynamic() || type.IsErrorType());


📝 We could disallow conditional state for dynamic but that degrades a couple of tests. #Closed

📝 The error type scenario is hit with LiftedRelationalOperation_UserDefined. Presumably, we could move the fix upstream (unsplit some time during binary operator analysis). Let me know if you have a preference

I think it's better to permit error type here than to try and adjust the bound tree shape.

RikkiGibson

LGTM but let's merge after #72062 to reduce potential for conflicts/churn.

RikkiGibson · 2024-02-14T18:25:28Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

+                return FlowAnalysisAnnotations.None;
+
+            // Conditional annotations are ignored on parameters of non-boolean members.
+            if (parameter.ContainingSymbol.GetTypeOrReturnType().Type.SpecialType != SpecialType.System_Boolean)


It seems like we are just using information on the parameter symbol to make these adjustments, did you consider moving this logic into property FlowAnalysisAnnotations instead? Or possibly we wouldn't want to do these adjustment in all places this property is used? #Resolved

Yes, I considered it (for this PR and previously when the annotations were added). We had landed on keeping the symbol API straightfoward, it shows what annotations are there. That said, I don't feel too strongly either way.
@cston what do you think?

Having the adjustment here seems fine to me, unless all callers should see the adjusted values.

jcouv · 2024-02-14T21:23:29Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

@@ -8004,10 +8054,9 @@ bool tryAsMemberOfSingleType(NamedTypeSymbol singleType, [NotNullWhen(true)] out

            (BoundExpression operand, Conversion conversion) = RemoveConversion(node, includeExplicitConversions: true);
            SnapshotWalkerThroughConversionGroup(node, operand);
-            if (targetType.SpecialType == SpecialType.System_Boolean &&
-                (operand.Type?.SpecialType == SpecialType.System_Boolean || operand.Type?.IsErrorType() == true))
+            if (TypeAllowsConditionalState(targetType.Type) && TypeAllowsConditionalState(operand.Type))


FYI @RikkiGibson I integrated with the change from other PR #Resolved

This seems more permissive than the version of the check from the other PR, is there any need to test when source or target type of the conversion is dynamic, for example, and we have a split state after visiting the operand?

Added a test where a difference is observable (PatternOnTuple_IsExpression_DynamicCast). There would be a nullable warning before this change.

cston · 2024-02-15T22:29:52Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

@@ -3547,6 +3553,20 @@ private BoundNode VisitLValue(BoundNode node)
            return Visit(node, expressionIsRead: false);
        }

+        private bool TypeAllowsConditionalState(TypeSymbol? type)


static #Resolved

cston · 2024-02-15T22:32:04Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

@@ -4499,6 +4519,7 @@ private TypeSymbol VisitArrayInitialization(TypeSymbol type, BoundArrayInitializ
                        var expression = GetConversionIfApplicable(expressions[i], expressionNoConversion);
                        expressionTypes[i] = VisitConversion(expression, expressionNoConversion, conversions[i], inferredType, expressionTypes[i], checkConversion: true,
                            fromExplicitCast: false, useLegacyWarnings: false, AssignmentKind.Assignment, reportRemainingWarnings: true, reportTopLevelWarnings: false);
+                        Unsplit();


Unsplit();

Are there tests that demonstrate the effect of this change? #Resolved

NotNullWhen_UserDefinedConversion_Completeness hits the new assertion without this because VisitConversion above leaves us in a split state in that scenario, which results in a split state at the end of the array initialization.

cston · 2024-02-15T22:32:19Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

@@ -5825,6 +5846,8 @@ void makeAndAdjustReceiverSlot(BoundExpression receiver)

            resultType ??= node.Type?.SetUnknownNullabilityForReferenceTypes();

+            UnsplitIfNeeded(resultType);


UnsplitIfNeeded(resultType);

Are there tests that demonstrate the effect of this change? #Resolved

Likewise, ConditionalOperator_17 and ConditionalOperator_WithUserDefinedConversion_BoolOperand hit the new assertion without this.

cston · 2024-02-15T22:34:36Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

+                return FlowAnalysisAnnotations.None;
+
+            // Conditional annotations are ignored on parameters of non-boolean members.
+            if (parameter.ContainingSymbol.GetTypeOrReturnType().Type.SpecialType != SpecialType.System_Boolean)


if

What drove this change? #Resolved

Scenarios like NotNullWhenTrue_ReturningObject and the other related tests I added NotNullWhen***_Returning***. They would also hit the newly added assertion, leaving a split state after analyzing an invocation returning object or some other types.

cston · 2024-02-16T17:04:24Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

+            // Conditional annotations are ignored on parameters of non-boolean members.
+            if (parameter.ContainingSymbol.GetTypeOrReturnType().Type.SpecialType != SpecialType.System_Boolean)
+            {
+                var annotations = parameter.FlowAnalysisAnnotations;


var

Consider moving this declaration before the if and having a single return annotations; at the end of the method. #Resolved

jcouv self-assigned this Feb 12, 2024

dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Feb 12, 2024

Improve NotNullWhenFalse_ReturningObject test

c6117f9

jcouv force-pushed the improve-test branch from 1e6057d to c6117f9 Compare February 12, 2024 22:45

Add invariant around conditional state

22538f6

jcouv changed the title ~~Improve NotNullWhenFalse_ReturningObject test~~ Add invariant around nullable conditional state Feb 13, 2024

jcouv mentioned this pull request Feb 13, 2024

Change NullableWalker.VisitConversion to not unsplit #72062

Merged

Avoid extra logic for VisitTypeExpression

20ed9b2

jcouv added this to the 17.10 milestone Feb 13, 2024

jcouv added the Feature - Nullable Reference Types Nullable Reference Types label Feb 13, 2024

build-analysis bot mentioned this pull request Feb 13, 2024

The active test run was aborted. Reason: Test host process crashed dotnet/dnceng#451

Open

3 tasks

Fix another scenario and refactor

98240dd

jcouv commented Feb 13, 2024

View reviewed changes

jcouv marked this pull request as ready for review February 14, 2024 01:53

jcouv requested a review from a team as a code owner February 14, 2024 01:53

RikkiGibson approved these changes Feb 14, 2024

View reviewed changes

jcouv added 2 commits February 14, 2024 12:57

Merge remote-tracking branch 'dotnet/main' into improve-test

cd919c0

Merge conflicts

9d9760d

jcouv commented Feb 14, 2024

View reviewed changes

Add test with cast to dynamic

dabb994

RikkiGibson approved these changes Feb 15, 2024

View reviewed changes

cston reviewed Feb 15, 2024

View reviewed changes

jcouv requested a review from cston February 16, 2024 05:58

cston reviewed Feb 16, 2024

View reviewed changes

cston approved these changes Feb 16, 2024

View reviewed changes

jcouv added 3 commits February 16, 2024 09:47

Tweak

e6fcf5c

static

3cbd954

Merge remote-tracking branch 'dotnet/main' into improve-test

5784629

jcouv merged commit 6b40ff8 into dotnet:main Feb 20, 2024
24 checks passed

ghost modified the milestones: 17.10, Next Feb 20, 2024

jcouv deleted the improve-test branch February 20, 2024 19:49

jjonescz modified the milestones: Next, 17.10 P2 Feb 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add invariant around nullable conditional state #72072

Add invariant around nullable conditional state #72072

jcouv commented Feb 12, 2024 •

edited

Loading

jcouv Feb 13, 2024

jcouv Feb 13, 2024 •

edited

Loading

jcouv Feb 14, 2024

RikkiGibson Feb 14, 2024

RikkiGibson left a comment

RikkiGibson Feb 14, 2024 •

edited by jcouv

Loading

jcouv Feb 16, 2024

cston Feb 16, 2024

jcouv Feb 14, 2024 •

edited

Loading

RikkiGibson Feb 14, 2024 •

edited

Loading

jcouv Feb 14, 2024

cston Feb 15, 2024 •

edited by jcouv

Loading

cston Feb 15, 2024 •

edited by jcouv

Loading

jcouv Feb 16, 2024

cston Feb 15, 2024 •

edited by jcouv

Loading

jcouv Feb 16, 2024

cston Feb 15, 2024 •

edited by jcouv

Loading

jcouv Feb 16, 2024

cston Feb 16, 2024 •

edited by jcouv

Loading

		@@ -5825,6 +5846,8 @@ void makeAndAdjustReceiverSlot(BoundExpression receiver)

		resultType ??= node.Type?.SetUnknownNullabilityForReferenceTypes();

		UnsplitIfNeeded(resultType);

Add invariant around nullable conditional state #72072

Add invariant around nullable conditional state #72072

Conversation

jcouv commented Feb 12, 2024 • edited Loading

Choose a reason for hiding this comment

jcouv Feb 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson left a comment

Choose a reason for hiding this comment

RikkiGibson Feb 14, 2024 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Feb 14, 2024 • edited Loading

Choose a reason for hiding this comment

RikkiGibson Feb 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Feb 15, 2024 • edited by jcouv Loading

Choose a reason for hiding this comment

cston Feb 15, 2024 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Feb 15, 2024 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Feb 15, 2024 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Feb 16, 2024 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv commented Feb 12, 2024 •

edited

Loading

jcouv Feb 13, 2024 •

edited

Loading

RikkiGibson Feb 14, 2024 •

edited by jcouv

Loading

jcouv Feb 14, 2024 •

edited

Loading

RikkiGibson Feb 14, 2024 •

edited

Loading

cston Feb 15, 2024 •

edited by jcouv

Loading

cston Feb 15, 2024 •

edited by jcouv

Loading

cston Feb 15, 2024 •

edited by jcouv

Loading

cston Feb 15, 2024 •

edited by jcouv

Loading

cston Feb 16, 2024 •

edited by jcouv

Loading