Skip to content

Fix nullable analysis for reassignment to a union value - #85879

Open
AlekseyTs wants to merge 1 commit into
dotnet:mainfrom
AlekseyTs:Issue85852
Open

AlekseyTs wants to merge 1 commit into
dotnet:mainfrom
AlekseyTs:Issue85852

Conversation

@AlekseyTs

@AlekseyTs AlekseyTs commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #85852

Microsoft Reviewers: Open in CodeFlow

Copilot AI balanced review requested due to automatic review settings October 1, 2026 20:48
@AlekseyTs
AlekseyTs requested a review from a team as a code owner October 1, 2026 20:48
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).
1 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

The change affects core nullable flow analysis, and CI is still running.

Review effort: Balanced
Findings: None

What changed in this PR

Fixes union nullability state restoration after reassignment.

Changes:

  • Applies union-specific Value defaults during member-state inheritance.
  • Adds regression coverage for standard and member-provider unions.
File Description
NullableWalker.cs Corrects reassignment flow state.
UnionsTests.cs Covers reported and related scenarios.

@AlekseyTs

Copy link
Copy Markdown
Contributor Author

@RikkiGibson, @jjonescz, @dotnet/roslyn-compiler Please review

{
Result<string, int> r = Make();
#line 100
System.Console.WriteLine(r switch { string s => s, int f => $""{f}"" });

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we add some versions of this where null is an explicit case in the switch, even though it's still Result<string, int> without string??

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I guess we can.

_variables[containingSlot].Symbol.GetTypeOrReturnType().Type is NamedTypeSymbol { IsUnionType: true, UnionCaseTypesNoUseSiteDiagnostics: not [] } unionType &&
Binder.IsUnionTypeValueProperty(unionType, property))
{
return unionType.UnionValueDeclaredNullableFlowState;

@RikkiGibson RikkiGibson Oct 2, 2026 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am wondering if this adjustment should be moved even "deeper" into the stack. For example, within GetTypeOrReturnTypeWithAnnotations(), or ApplyUnconditionalAnnotations(), or a new helper which is called from similar places as those methods.

The reason I bring this up is, it looks like there are still places where the flow state of the Value doesn't match what we expect. For example, in the following test, a warning is reported in M1().Value.ToString(), but not on u.Value.ToString().

[Fact]
public void M()
{
    var source = """
        #nullable enable
        class Program
        {
            U M1() => 42;

            void M2()
            {
                M1().Value.ToString();
            }

            void M3()
            {
                U u = M1();
                u.Value.ToString();
            }
        }

        union U(string, int);
        """;

    CreateCompilation([source, UnionAttributeSource, IUnionSource]).VerifyEmitDiagnostics();
}

@RikkiGibson RikkiGibson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Done review pass

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unions: spurious CS8655 on a switch over a union variable reassigned from a method call

4 participants