Skip to content

Improved nullable 'is' analysis#53311

Merged
RikkiGibson merged 8 commits intodotnet:features/improved-definite-assignmentfrom
RikkiGibson:ida-nullable-is
Jun 10, 2021
Merged

Improved nullable 'is' analysis#53311
RikkiGibson merged 8 commits intodotnet:features/improved-definite-assignmentfrom
RikkiGibson:ida-nullable-is

Conversation

@RikkiGibson
Copy link
Member

@RikkiGibson RikkiGibson commented May 11, 2021 •
edited
Loading

Related to #51463

Corresponding definite assignment PR: #52616

Relevant spec section: https://github.com/dotnet/csharplang/blob/main/proposals/improved-definite-assignment.md#is-operator-and-is-pattern-expressions

is operator and is pattern expressions

We introduce a new section is operator and is pattern expressions.

For an expression expr of the form E is T, where T is any type or pattern

  • The definite assignment state of v before E is the same as the definite assignment state of v before expr.
  • The definite assignment state of v after expr is determined by:
    • If E directly contains a null-conditional expression, and the state of v after the non-conditional counterpart E0 is "definitely assigned", and T is any type or a pattern that does not match a null input, then the state of v after expr is "definitely assigned when true".
    • If E directly contains a null-conditional expression, and the state of v after the non-conditional counterpart E0 is "definitely assigned", and T is a pattern that matches a null input, then the state of v after expr is "definitely assigned when false".
    • If E is of type boolean and T is a pattern which only matches a true input, then the definite assignment state of v after expr is the same as the definite assignment state of v after E.
    • If E is of type boolean and T is a pattern which only matches a false input, then the definite assignment state of v after expr is the same as the definite assignment state of v after the logical negation expression !expr.
    • Otherwise, if the definite assignment state of v after E is "definitely assigned", then the definite assignment state of v after expr is "definitely assigned".

Remarks

This section is meant to address similar scenarios as in the ==/!= section above.
This specification does not address recursive patterns, e.g. (a?.b(out x), c?.d(out y)) is (object, object). Such support may come later if time permits.

@ghost ghost added the Area-Compilers label May 11, 2021
@RikkiGibson RikkiGibson changed the base branch from main to features/improved-definite-assignment May 11, 2021 00:49
@RikkiGibson RikkiGibson mentioned this pull request May 18, 2021
Merged
@RikkiGibson RikkiGibson marked this pull request as ready for review May 21, 2021 00:23
@RikkiGibson RikkiGibson requested a review from a team as a code owner May 21, 2021 00:23
@RikkiGibson
Copy link
Member Author

RikkiGibson commented May 21, 2021

@jcouv @333fred Please take a look when you get the chance.

@jcouv jcouv self-assigned this May 21, 2021
@jcouv jcouv added this to the C# 10 milestone May 21, 2021
@RikkiGibson RikkiGibson mentioned this pull request May 24, 2021
Closed
7 tasks
@RikkiGibson
Copy link
Member Author

RikkiGibson commented May 25, 2021

@jcouv @333fred Please review

jcouv
jcouv reviewed May 26, 2021
View reviewed changes
jcouv
jcouv reviewed May 26, 2021
View reviewed changes
jcouv
jcouv reviewed May 26, 2021
View reviewed changes
jcouv
jcouv reviewed May 26, 2021
View reviewed changes
jcouv
jcouv reviewed May 26, 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 1)

@RikkiGibson
Copy link
Member Author

RikkiGibson commented Jun 7, 2021

  • 01c2432 implements the spec change in Adjust 'is' spec csharplang#4785
  • 1e7ab9b pushes the nullable 'is' analysis into LearnFromDecisionDag. This seems to simplify things and should also make some things "just work" when it comes time to do switch expressions and switch statements.

RikkiGibson
RikkiGibson commented Jun 8, 2021
View reviewed changes
src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker_Patterns.cs
var hasStateWhenNotNull = VisitPossibleConditionalAccess(node.Expression, out var conditionalStateWhenNotNull);
var expressionState = ResultType;
var labelStateMap = LearnFromDecisionDag(node.Syntax, node.DecisionDag, node.Expression, expressionState);
var labelStateMap = LearnFromDecisionDag(node.Syntax, node.DecisionDag, node.Expression, expressionState, hasStateWhenNotNull ? conditionalStateWhenNotNull : null);
Copy link
Member Author

@RikkiGibson RikkiGibson Jun 8, 2021

Choose a reason for hiding this comment

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

Note that we want to only pass in the 'stateWhenNotNull' when 'VisitPossibleConditionalAccess' returns 'true'. Otherwise, we will mess up the x != null is true scenario--the value test node will end up using the 'stateWhenNotNull' instead of using the "original" conditional state.

jcouv
jcouv reviewed Jun 9, 2021
View reviewed changes
src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker_Patterns.cs
// [1]: t1 = (bool)t0; [2]
// [2] (this node): t1 == boolConstant ? [3] : [4]
// ...(remaining states)
if (stateWhenNotNullOpt is { } stateWhenNotNull
Copy link
Member

@jcouv jcouv Jun 9, 2021

Choose a reason for hiding this comment

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

nit: We probably don't need a new local for this (stateWhenNotNullOpt is not null then use stateWhenNotNullOpt is sufficient)

jcouv
jcouv reviewed Jun 9, 2021
View reviewed changes
src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs Outdated
}
}

private static bool PatternMatchesNull(BoundPattern pattern)
Copy link
Member

@jcouv jcouv Jun 9, 2021

Choose a reason for hiding this comment

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

nit: maybe leave as local function?

Copy link
Member Author

@RikkiGibson RikkiGibson Jun 10, 2021

Choose a reason for hiding this comment

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

Fixed

jcouv
jcouv reviewed Jun 9, 2021
View reviewed changes
src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker_Patterns.cs

void learnFromNonNullTest(int inputSlot, ref LocalState state)
{
if (stateWhenNotNullOpt is { } stateWhenNotNull && inputSlot == originalInputSlot)
Copy link
Member

@jcouv jcouv Jun 9, 2021

Choose a reason for hiding this comment

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

stateWhenNotNull

nit: Here too, it feels like we shouldn't need the extra local, even to satisfy nullability checks on CloneAndUnsplit(ref ...)

Copy link
Member Author

@RikkiGibson RikkiGibson Jun 9, 2021

Choose a reason for hiding this comment

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

Problem here is we can't take a ref to stateWhenNotNullOpt.Value.

jcouv
jcouv reviewed Jun 9, 2021
View reviewed changes
src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs
{
public string? Prop { get; set; }
}";
// Ideally we would not issue diagnostic (1).
Copy link
Member

@jcouv jcouv Jun 9, 2021

Choose a reason for hiding this comment

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

I thought this scenario might just fall out of the change to learnFromNonNullTest. Can you clarify what's missing?

Copy link
Member Author

@RikkiGibson RikkiGibson Jun 10, 2021

Choose a reason for hiding this comment

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

I didn't debug in a great amount of detail, but I suspect that when we create a slot for Prop we don't understand that its containing slot is the slot for b.C. Instead we either use some dag temp slot without relating it to the original input, or we create some slot that's accidentally linked to the conditional receiver (left side of ?.C).

jcouv
jcouv approved these changes Jun 9, 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 8)

@RikkiGibson RikkiGibson requested a review from 333fred June 10, 2021 17:52
333fred
333fred approved these changes Jun 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 8)

@RikkiGibson RikkiGibson merged commit 2cca737 into dotnet:features/improved-definite-assignment Jun 10, 2021
@RikkiGibson RikkiGibson deleted the ida-nullable-is branch June 10, 2021 20:35
333fred added a commit that referenced this pull request Jun 14, 2021
@333fred
Merge remote-tracking branch 'upstream/main' into merges/main-to-feat…
520d492 
…ures/interpolated-string

* upstream/main: (95 commits)
  Update official build number in separate job
  Update Language Feature Status.md (#54015)
  Remove IRazorDocumentOptionsService inheritance interface (#54047)
  Fix comment
  Simplify
  Do not create a cache field for lambda if it depends on caller's type argument (#44939)
  Documentation
  Documentation
  Documentation
  Update test impls
  Just pass null
  Pull diagnostics should just request from the doc, not the whole project.
  Add test plan for file-scoped namespace (#54003)
  Add source build to official build
  Improved nullable 'is' analysis (#53311)
  Multi session service (#53762)
  Resolve Versions.props conflicts
  Revert "Revert "Require partial method signatures to match" (47576) (#47879)" (#53352)
  Broaden enforcement on prototype marker (#53886)
  Update Language Feature Status.md (#53926)
  ...
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