Skip to content

Adjust nullability analysis for extension operators. #79103

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Jun 25, 2025
Merged

Adjust nullability analysis for extension operators. #79103

merged 4 commits into from
Jun 25, 2025

Conversation

@AlekseyTs
Copy link
Contributor

@AlekseyTs AlekseyTs commented Jun 23, 2025 •
edited
Loading

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

Relates to test plan #76130

@AlekseyTs AlekseyTs requested a review from a team as a code owner June 23, 2025 16:17
@AlekseyTs AlekseyTs added Area-Compilers Feature - Extension Everything The extension everything feature labels Jun 23, 2025
@RikkiGibson RikkiGibson self-assigned this Jun 23, 2025
@jcouv jcouv self-assigned this Jun 23, 2025
jcouv
jcouv reviewed Jun 23, 2025
View reviewed changes
jcouv
jcouv reviewed Jun 23, 2025
View reviewed changes
jcouv
jcouv reviewed Jun 23, 2025
View reviewed changes
jcouv
jcouv reviewed Jun 24, 2025
View reviewed changes
@jcouv

This comment was marked as resolved.

Sign in to view
@jcouv
Copy link
Member

jcouv commented Jun 24, 2025 •
edited by AlekseyTs
Loading 

        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

This comment was marked as resolved.

Sign in to view
@jcouv

This comment was marked as resolved.

Sign in to view
@jcouv

This comment was marked as resolved.

Sign in to view
jcouv
jcouv reviewed Jun 24, 2025
View reviewed changes
Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done with review pass (commit 1)

@AlekseyTs
Copy link
Contributor Author

AlekseyTs commented Jun 24, 2025

/azp run

@azure-pipelines
Copy link

azure-pipelines bot commented Jun 24, 2025

Azure Pipelines successfully started running 2 pipeline(s).

@AlekseyTs
Copy link
Contributor Author

AlekseyTs commented Jun 24, 2025 

    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
Copy link
Contributor Author

AlekseyTs commented Jun 24, 2025 

    [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)

@AlekseyTs
Copy link
Contributor Author

AlekseyTs commented Jun 24, 2025 •
edited
Loading 

    }
  • chained binary operator

Added a targeted test - Binary_128_NullableAnalysis_Logical_Chained

  • nullability suppression

Perhaps it would be good to have some tests for completeness sake, but, since we are not changing the shape of the bound tree and not introducing new syntax for consumption, I do not find these scenarios particularly interesting or high priority. Making a note for test plan

  • interesting target-typed expressions as operands (null, lambda, collection expression)

Added a targeted nullability test with a lambda. Making a note for test plan.

  • null-conditional compound assignment

Conditional nature of the assignment doesn't affect how compound assignment operation
is bound or represented in bound tree. It is (the conditional nature) is handled elsewhere. Therefore,
I do not find these scenarios particularly interesting or high priority. Making a note for test plan for
completeness.

In reply to: 2998703574

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

@jcouv
Copy link
Member

jcouv commented Jun 24, 2025 

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

Will raise the question in WG to confirm

In reply to: 3000409835

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

@AlekseyTs AlekseyTs requested a review from jcouv June 24, 2025 17:03
jcouv
jcouv approved these changes Jun 24, 2025
View reviewed changes
Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM Thanks (commit 3)

@AlekseyTs
Copy link
Contributor Author

AlekseyTs commented Jun 24, 2025

@RikkiGibson, @jjonescz, @dotnet/roslyn-compiler For a second review.

RikkiGibson
RikkiGibson reviewed Jun 24, 2025
View reviewed changes
src/Compilers/CSharp/Portable/BoundTree/NullabilityRewriter.cs

public override BoundNode? VisitCompoundAssignmentOperator(BoundCompoundAssignmentOperator node)
{
ImmutableArray<MethodSymbol> originalUserDefinedOperatorsOpt = GetUpdatedArray(node, node.OriginalUserDefinedOperatorsOpt);
Copy link
Member

@RikkiGibson RikkiGibson Jun 24, 2025 •
edited by AlekseyTs
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like originalUserDefinedOperatorsOpt is not used. Was that intentional? #Resolved

Copy link
Contributor Author

@AlekseyTs AlekseyTs Jun 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like originalUserDefinedOperatorsOpt is not used. Was that intentional?

Looks like a bug in NullabilityRewriter code generator. The function was moved from generated code and adjusted to fix up node.Operator. I'll fix this part too.

Copy link
Contributor Author

@AlekseyTs AlekseyTs Jun 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened #79132 for the generator

@AlekseyTs AlekseyTs mentioned this pull request Jun 25, 2025
Open
@AlekseyTs AlekseyTs requested a review from RikkiGibson June 25, 2025 11:18
@AlekseyTs
Copy link
Contributor Author

AlekseyTs commented Jun 25, 2025

@RikkiGibson, @jjonescz, @dotnet/roslyn-compiler For a second review.

jjonescz
jjonescz approved these changes Jun 25, 2025
View reviewed changes
src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.DebugVerifier.cs
{
if (node.LeftConversion is BoundConversion leftConversion)
if (node.LeftConversion is BoundConversion leftConversion &&
!(node.Operator.Method is { IsStatic: false } method && method.GetIsNewExtensionMember()))
Copy link
Member

@jjonescz jjonescz Jun 25, 2025 •
edited by AlekseyTs
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we skipping verification in this case? #Resolved

Copy link
Contributor Author

@AlekseyTs AlekseyTs Jun 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we skipping verification in this case?

Because we know we do not visit the node in a normal fashion. The rewrite is done for the benefit of type information from SemanticModel and it doesn't depend on LeftConversion member anyway.

@AlekseyTs AlekseyTs merged commit 1c40207 into dotnet:features/extensions Jun 25, 2025
24 checks passed
@AlekseyTs
Copy link
Contributor Author

AlekseyTs commented Jun 25, 2025

@RikkiGibson I merged the PR in order to expedite code flow since the PR met the two sign-offs requirement. If you have additional feedback, I will follow up on it separately.

@dotnet-bot dotnet-bot mentioned this pull request Jul 2, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jjonescz jjonescz jjonescz approved these changes

@jcouv jcouv jcouv approved these changes

@RikkiGibson RikkiGibson Awaiting requested review from RikkiGibson

Assignees

@RikkiGibson RikkiGibson

@jcouv jcouv

Labels

Area-Compilers Feature - Extension Everything The extension everything feature

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@AlekseyTs @jcouv @jjonescz @RikkiGibson