Adjust nullability analysis for extension operators. #79103

AlekseyTs · 2025-06-23T16:17:25Z

Related to #29605.
Related to #29961.
Related to #79011.

Relates to test plan #76130

Related to dotnet#29605. Related to dotnet#29961. Related to dotnet#79011.

jcouv · 2025-06-23T22:55:25Z

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

@@ -5313,6 +5312,58 @@
            var inferredResult = InferResultNullability(operatorKind, method, returnType, leftType, rightType);

            return inferredResult;
+        }
+
+        MethodSymbol ReInferBinaryOperator(


private #Pending

jcouv · 2025-06-23T22:57:22Z

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

+                    extension,
+                    method.OriginalDefinition.ParameterTypesWithAnnotations,
+                    method.OriginalDefinition.ParameterRefKinds,
+                    [new BoundExpressionWithNullability(leftOperand.Syntax, leftOperand, leftUnderlyingType.ToTypeWithAnnotations(compilation).NullableAnnotation, leftUnderlyingType.Type),


BoundExpressionWithNullability

In analysis of invocations (VisitCall/VisitArguments), we use GetArgumentsForMethodTypeInference to get inputs to MethodTypeInferrer.Infer. Do we need the same thing here (it has extra cases to deal with lambda, collection expressions and typeless expressions)? #Pending

GetArgumentsForMethodTypeInference operates in terms of VisitResult for arguments, this function gets TypeWithState instead. Moreover, the passed in TypeWithState is getting adjusted by the caller for lifted scenarios. Given that, it doesn't look trivial to switch from TypeWithState to VisitResult as an input. I think the current implementation is going to work for majority of scenarios, I will leave a prototype comment to follow up.

jcouv · 2025-06-23T23:33:23Z

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

+                    break;
+                }
+
+                AdjustConditionalState(binary);


Should AdjustConditionalState be applied to the left-most operand too (ie. in the first switch in this method)? #Resolved

Should AdjustConditionalState be applied to the left-most operand too (ie. in the first switch in this method)?

For case BoundBinaryOperator, VisitCondition does that and, as can be observed in AbstractFlowPass, AdjustConditionalState isn't called explicitly for the left-most operand.

For case BoundUserDefinedConditionalLogicalOperator, AfterLeftChildOfBinaryConditionalLogicalOperatorHasBeenVisited does

Unsplit(); Split();

right at the beginning, which is equivalent to AdjustConditionalState for non-Boolean case.

jcouv · 2025-06-23T23:52:22Z

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

            }
        }

-        protected override void AfterLeftChildOfBinaryLogicalOperatorHasBeenVisited(BoundExpression node, BoundExpression right, bool isAnd, bool isBool, ref LocalState leftTrue, ref LocalState leftFalse)
+        private static void GetBinaryConditionalOperatorInfo(BinaryOperatorKind kind, out bool isAnd, out bool isBool)


GetBinaryConditionalOperatorInfo

nit: consider bringing GetBinaryConditionalOperatorInfo and both AfterLeftChildOfBinaryConditionalLogicalOperatorHasBeenVisited methods into VisitBinaryLogicalOperatorChildren as local functions, since only used there #Pending

jcouv · 2025-06-23T23:55:39Z

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

+            Visit(node.Right);
+            TypeWithState rightType = ResultType;
+            SetResultType(node, InferResultNullability(node.OperatorKind, node.Method, node.Type, leftType, rightType));
+            AfterRightChildOfBinaryLogicalOperatorHasBeenVisited(node.Right, isAnd, isBool, ref leftTrue, ref leftFalse);


nit: it looks like the AfterRightChildOfBinaryLogicalOperatorHasBeenVisited signature could be simplified by removing the ref on last two parameters (no callers care for any updates this method might make) #Resolved

it looks like the AfterRightChildOfBinaryLogicalOperatorHasBeenVisited signature could be simplified by removing the ref on last two parameters (no callers care for any updates this method might make)

I am not going to mess with the signature of existing method from the base class. Passing state by ref actually makes sense when the state is a struct.

jcouv · 2025-06-24T00:57:12Z

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

-            SetResultType(node, InferResultNullabilityOfBinaryLogicalOperator(node, leftType, rightType));
+            GetBinaryConditionalOperatorInfo(binary.OperatorKind, out bool isAnd, out bool isBool);
+            Debug.Assert(!isBool);
+            SetState(isAnd ? leftTrue : leftFalse);


This is strange. leftTrue and leftFalse hold the same state here. Consider removing this line and the Split a couple of lines above.
Removing the ref from last two parameters of AfterRightChildOfBinaryLogicalOperatorHasBeenVisited as suggested earlier will also simplify this further (no need for separate leftTrue/leftFalse) #Resolved

The split is actually required to properly calculate the state after the operator. See the logic around joining states in AfterRightChildOfBinaryLogicalOperatorHasBeenVisited. It is true that at the beginning the states are the same, but the one that we set here is going to diverge as we keep visiting things.
Apart from ref the input for AfterRightChildOfBinaryLogicalOperatorHasBeenVisited indeed could be somewhat simplified if we move calculations around isAnd, leftTrue, and leftFalse``` into the callers. But I am not interested doing this due to the following reasons:

this moves complexity (calculation that could be implemented with a mistake) to every consumer.

The AfterRightChildOfBinaryLogicalOperatorHasBeenVisited is existing method, which otherwise is not getting touched.

jcouv · 2025-06-24T03:26:32Z

    public static C2 operator -(C2? x, C2? y)

It seems that for static operators we currently don't care about the nullability annotations in the extension parameter. Should we check that as a declaration warning?

public static class E
{
  extension(C?)
  {
    public static C operator -(C x, C y) { } // operator doesn't handle `C?`...
  }
}
``` #Resolved

---
Refers to: src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionOperatorsTests.cs:14916 in b279a80. [](commit_id = b279a80c6a967ec8f1b8038ad785e45668fa5746, deletion_comment = False)

jcouv · 2025-06-24T03:30:13Z

        var comp = CreateCompilation(src, options: TestOptions.DebugExe);

nit: consider removing unnecessary option (applies throughout) #WontFix

Refers to: src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionOperatorsTests.cs:14947 in b279a80. [](commit_id = b279a80, deletion_comment = False)

jcouv · 2025-06-24T03:31:59Z

    (1 - x).ToString();

Consider verifying semantic model shows generic operator with updated nullability

Refers to: src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionOperatorsTests.cs:15068 in b279a80. [](commit_id = b279a80, deletion_comment = False)

jcouv · 2025-06-24T03:48:02Z

Do we have a test for the following:

chained binary operator
nullability suppression
interesting target-typed expressions as operands (null, lambda, collection expression)
null-conditional compound assignment

Refers to: src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionOperatorsTests.cs:16074 in b279a80. [](commit_id = b279a80, deletion_comment = False)

jcouv · 2025-06-24T03:52:55Z

    [WorkItem("https://github.com/dotnet/roslyn/issues/29961")]

I didn't follow why link to this issue. The diagnostics seem as expected #Resolved

Refers to: src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionOperatorsTests.cs:8068 in b279a80. [](commit_id = b279a80, deletion_comment = False)

jcouv

Done with review pass (commit 1)

AlekseyTs · 2025-06-24T11:25:34Z

/azp run

azure-pipelines · 2025-06-24T11:25:51Z

Azure Pipelines successfully started running 2 pipeline(s).

AlekseyTs · 2025-06-24T13:04:03Z

    public static C2 operator -(C2? x, C2? y)

It seems that for static operators we currently don't care about the nullability annotations in the extension parameter. Should we check that as a declaration warning?

I think the behavior is intentional and desirable.

In reply to: 2998670074

Refers to: src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionOperatorsTests.cs:14916 in b279a80. [](commit_id = b279a80, deletion_comment = False)

AlekseyTs · 2025-06-24T13:14:35Z

    [WorkItem("https://github.com/dotnet/roslyn/issues/29961")]

Because the scenarios are related to the issue and they were not working properly before my changes. I also modified the code in the function from the issue.

In reply to: 2998710883

Refers to: src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionOperatorsTests.cs:8068 in b279a80. [](commit_id = b279a80, deletion_comment = False)

Adjust nullability analysis for extension operators.

b279a80

Related to dotnet#29605. Related to dotnet#29961. Related to dotnet#79011.

AlekseyTs requested review from jjonescz, RikkiGibson and jcouv June 23, 2025 16:17

AlekseyTs requested a review from a team as a code owner June 23, 2025 16:17

AlekseyTs added Area-Compilers Feature - Extension Everything The extension everything feature labels Jun 23, 2025

RikkiGibson self-assigned this Jun 23, 2025

jcouv self-assigned this Jun 23, 2025

jcouv reviewed Jun 23, 2025

View reviewed changes

jcouv reviewed Jun 24, 2025

View reviewed changes

Remove unused method

27177f2

Adjust nullability analysis for extension operators. #79103

Are you sure you want to change the base?

Adjust nullability analysis for extension operators. #79103

Conversation

AlekseyTs commented Jun 23, 2025 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv Jun 23, 2025 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jun 23, 2025 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jun 24, 2025

Choose a reason for hiding this comment

Uh oh!

jcouv Jun 23, 2025 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jun 24, 2025

Choose a reason for hiding this comment

Uh oh!

jcouv Jun 23, 2025 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jun 23, 2025 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jun 24, 2025

Choose a reason for hiding this comment

Uh oh!

jcouv Jun 24, 2025 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jun 24, 2025

Choose a reason for hiding this comment

Uh oh!

jcouv commented Jun 24, 2025 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv commented Jun 24, 2025 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv commented Jun 24, 2025

Uh oh!

jcouv commented Jun 24, 2025

Uh oh!

jcouv commented Jun 24, 2025 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Jun 24, 2025

Uh oh!

azure-pipelines bot commented Jun 24, 2025

Uh oh!

AlekseyTs commented Jun 24, 2025

Uh oh!

AlekseyTs commented Jun 24, 2025

Uh oh!

Uh oh!

AlekseyTs commented Jun 23, 2025 •

edited by jcouv

Loading

jcouv Jun 23, 2025 •

edited by AlekseyTs

Loading

jcouv Jun 23, 2025 •

edited by AlekseyTs

Loading

jcouv Jun 23, 2025 •

edited by AlekseyTs

Loading

jcouv Jun 23, 2025 •

edited by AlekseyTs

Loading

jcouv Jun 23, 2025 •

edited by AlekseyTs

Loading

jcouv Jun 24, 2025 •

edited by AlekseyTs

Loading

jcouv commented Jun 24, 2025 •

edited by AlekseyTs

Loading

jcouv commented Jun 24, 2025 •

edited by AlekseyTs

Loading

jcouv commented Jun 24, 2025 •

edited by AlekseyTs

Loading