Skip to content

Improved nullable conditional operator analysis#52783

Merged
RikkiGibson merged 5 commits intodotnet:features/improved-definite-assignmentfrom
RikkiGibson:ida-nullable-cond
May 10, 2021
Merged

Improved nullable conditional operator analysis#52783
RikkiGibson merged 5 commits intodotnet:features/improved-definite-assignmentfrom
RikkiGibson:ida-nullable-cond

Conversation

@RikkiGibson
Copy link
Member

@RikkiGibson RikkiGibson commented Apr 20, 2021 •
edited
Loading

Resolves #36096

Test plan: #51463

Implements changes to conditional operator ?: analysis. It may be helpful to review the corresponding definite assignment PR #51498.

Specifically, this PR allows conditional state to propagate out of the arms of a conditional operator in the nullable walker, enabling scenarios like the following:

void M(bool b, object? x)
{
    if (b ? x != null : false)
    {
        x.ToString(); // ok
    }
}

@ghost ghost added the Area-Compilers label Apr 20, 2021
@RikkiGibson RikkiGibson force-pushed the ida-nullable-cond branch from 7e07ab1 to 704af28 Compare May 4, 2021 01:22
@RikkiGibson RikkiGibson marked this pull request as ready for review May 4, 2021 01:22
@RikkiGibson RikkiGibson requested a review from a team as a code owner May 4, 2021 01:22
@RikkiGibson
Copy link
Member Author

RikkiGibson commented May 4, 2021

@333fred @jcouv this PR is ready for review. Please take a look when you have the chance.

@333fred
Copy link
Member

333fred commented May 5, 2021 •
edited
Loading

Done review pass (commit 1). Some test suggestions.

In reply to: 833079375

In reply to: 833079375

333fred
333fred reviewed May 6, 2021
View reviewed changes
src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs Outdated

{
if (!(b ? " + @false + @" : x == null))
y = x; // 3
Copy link
Member

@333fred 333fred May 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

// 3

I'm confused how we get here when the type of x is object: I think it's because the previous if set the state to MaybeNull for all cases. I think we need to add another parameter to assign to x at the beginning of each block to reset it's state to the expected default. #Closed

Copy link
Member Author

@RikkiGibson RikkiGibson May 6, 2021

Choose a reason for hiding this comment

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

Looking into this I realize I'm not quite satisfied with the coding pattern used in this test, so I rewrote the test while ensuring that the scenarios you've called out are covered.

@333fred
Copy link
Member

333fred commented May 6, 2021 •
edited
Loading

Done review pass (commit 2).

In reply to: 833686766

jcouv
jcouv reviewed May 7, 2021
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 (iteration 3)

@jcouv jcouv self-assigned this May 7, 2021
@jcouv jcouv added this to the C# 10 milestone May 7, 2021
jcouv
jcouv reviewed May 7, 2021
View reviewed changes
src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs Outdated
return state;
}

private void SetConditionalState(in PossiblyConditionalState conditionalState)
Copy link
Member

@jcouv jcouv May 7, 2021 •
edited by 333fred
Loading

Choose a reason for hiding this comment

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

SetConditionalState

nit: SetPossiblyConditionalState #Closed

jcouv
jcouv approved these changes May 7, 2021
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 (iteration 4)

@RikkiGibson
Copy link
Member Author

RikkiGibson commented May 7, 2021 •
edited by 333fred
Loading

I believe I've addressed all your comments @333fred. Please let me know if you have any others.

In reply to: 834857966

@RikkiGibson
Copy link
Member Author

RikkiGibson commented May 10, 2021

/azp run

@azure-pipelines
Copy link

azure-pipelines bot commented May 10, 2021

Azure Pipelines successfully started running 3 pipeline(s).

333fred
333fred approved these changes May 10, 2021
View reviewed changes
Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

LGTM (commit 5)

@RikkiGibson RikkiGibson merged commit 182dd83 into dotnet:features/improved-definite-assignment May 10, 2021
@runfoapp runfoapp bot mentioned this pull request May 12, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@333fred 333fred 333fred approved these changes

@jcouv jcouv jcouv approved these changes

Assignees

@jcouv jcouv

Labels

Area-Compilers

Projects

None yet

Milestone

C# 10

Development

Successfully merging this pull request may close these issues.

3 participants

@RikkiGibson @333fred @jcouv