Skip to content
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.

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

Implement conditional expression changes in definite assignment #51498

Conversation

RikkiGibson
Copy link
Contributor

@RikkiGibson RikkiGibson commented Feb 25, 2021

Note that ed38577 simply deletes and regenerates the baseline in order to make comparison easier in the subsequent commit.

I decided to do this bit first, as the precise method of doing it had been in my head. There are corresponding nullable analysis changes I'd like to make, but nullable does more complex things with conditional expressions. Therefore I'd like to do those changes in a subsequent PR.

/cc @jcouv

Implementation of part of dotnet/csharplang#4472. The relevant spec sections implemented in this PR are:

Boolean constant expressions

We introduce a new section "Boolean constant expressions":

For an expression expr where expr is a constant expression with a bool value:

  • The definite assignment state of v after expr is determined by:
    • If expr is a constant expression with value true, and the state of v before expr is "not definitely assigned", then the state of v after expr is "definitely assigned when false".
    • If expr is a constant expression with value false, and the state of v before expr is "not definitely assigned", then the state of v after expr is "definitely assigned when true".

Remarks

We assume that if an expression has a constant value bool false, for example, it's impossible to reach any branch that requires the expression to return true. Therefore variables are assumed to be definitely assigned in such branches. This ends up combining nicely with the spec changes for expressions like ?? and ?: and enabling a lot of useful scenarios.

It's also worth noting that we never expect to be in a conditional state before visiting a constant expression. That's why we do not account for scenarios such as "expr is a constant expression with value true, and the state of v before expr is "definitely assigned when true".

?: (conditional) expressions

We augment the section ?: (conditional) expressions as follows:

For an expression expr of the form expr_cond ? expr_true : expr_false:

  • ...
  • The definite assignment state of v after expr is determined by:
    • ...
    • If the state of v after expr_true is "definitely assigned when true", and the state of v after expr_false is "definitely assigned when true", then the state of v after expr is "definitely assigned when true".
    • If the state of v after expr_true is "definitely assigned when false", and the state of v after expr_false is "definitely assigned when false", then the state of v after expr is "definitely assigned when false".

Remarks

This makes it so when both arms of a conditional expression result in a conditional state, we join the corresponding conditional states and propagate it out instead of unsplitting the state and allowing the final state to be non-conditional. This enables scenarios like the following:

bool b = true;
object x = null;
int y;
if (b ? x != null && Set(out y) : x != null && Set(out y))
{
  y.ToString();
}

bool Set(out int x) { x = 0; return true; }

This is an admittedly niche scenario, that compiles without error in the native compiler, but was broken in Roslyn in order to match the specification at the time. See internal issue.

Relates to test plan #51463

@RikkiGibson
Copy link
Contributor Author

@dotnet/roslyn-compiler please review.

@jcouv jcouv self-assigned this Mar 1, 2021
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 5)

@jcouv jcouv added this to the C# 10 milestone Mar 2, 2021
@RikkiGibson RikkiGibson requested review from 333fred and a team March 2, 2021 17:32
@333fred
Copy link
Member

333fred commented Mar 2, 2021

/* DAT?NDA:NDA-->NDA */ { int a; if ((x && G(out a)) ? y : z) b = a; else d = c; } // Error

I think it would be worth adding some variations of these tests (where the condition is DAT/F) and the branch is a constant value. ie, something like { int a; if ((x && G(out a)) ? true : false) b = a; else d = c; } // OK


Refers to: src/Compilers/CSharp/Test/Semantic/FlowAnalysis/FlowTests.cs:1752 in f126ebe. [](commit_id = f126ebe, deletion_comment = False)

@333fred
Copy link
Member

333fred commented Mar 2, 2021

/* DAT?NDA:NDA-->NDA */ { int a; if ((x && G(out a)) ? y : z) b = a; else d = c; } // Error

Also, is ! support already implemented, or will it be in a future PR? We'll want to have variations that involve that.


In reply to: 789277456 [](ancestors = 789277456)


Refers to: src/Compilers/CSharp/Test/Semantic/FlowAnalysis/FlowTests.cs:1752 in f126ebe. [](commit_id = f126ebe, deletion_comment = False)

@333fred
Copy link
Member

333fred commented Mar 2, 2021

Done review pass (commit 5)

@RikkiGibson
Copy link
Contributor Author

Also, is ! support already implemented, or will it be in a future PR? We'll want to have variations that involve that.

Does this refer to unary not or to nullable suppression?

@333fred
Copy link
Member

333fred commented Mar 5, 2021

Does this refer to unary not or to nullable suppression?

Unary not.

@RikkiGibson
Copy link
Contributor Author

I have added a related test case in IfConditionalConstant. No implementation change needed. The existing handling of the ! operator knows what to do when the operand has conditional state, and we just introduced a new way that expressions can have conditional state after visiting.

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

@RikkiGibson RikkiGibson merged commit e1bee98 into dotnet:features/improved-definite-assignment Mar 5, 2021
@RikkiGibson RikkiGibson deleted the ida-ternary branch March 5, 2021 18:13
@RikkiGibson RikkiGibson restored the ida-ternary branch March 5, 2021 18:21
@RikkiGibson RikkiGibson deleted the ida-ternary branch March 5, 2021 19:54
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.

3 participants