Prototype implementation approach for negated-if-statements

[CyrusNajmabadi]

See dotnet/csharplang#882 for Language Change Proposal.

The purpose of my PR is to demonstrate a very low-friction implementation strategy for adding the 'negated-if'/'guard statement' to the Roslyn compiler and IDE. The impl satisfies an updated grammar production of:

if_statement
    : 'if' '!'? '(' boolean_expression ')' embedded_statement
    | 'if' '!' ?'(' boolean_expression ')' embedded_statement 'else' embedded_statement
    ;

However, it does so with a potentially unexpected implementation strategy. Specifically, i looked into a few ways to implement this in the C# compiler.

The first was simply to add an optional ! token to IfStatementSyntax. This had the pro of being an exceptionally simple grammar.xml/syntax change. However, it had a lot of unpleasant fallout.

Specifically, every single piece of code that analyzed semantics of expressions in the compiler and IDE now likely needed to be updated to understand this form. i.e. when analyzing the 'Condition' of a If-Statement, you needed to pass a boolean along saying if it was negated, and that then had to flow into the code that deals with LogicalNotExpressions. In a similar vein, every IDE feature had to be updated to understand this form. For example, in order to goto-def on an overloaded ! operator, you needed special code to analyze an IfStatement and get the right operator called, since htat was not available on the actual condition operator. This had widespread consequences and turned out to be highly unpalatable.

The second implementation strategy was similar to the first. Except this involved a common base type that there would be two 'IfStatements' deriving from. The existing IfStatement, and a new GuardIfStatement. This was also unpalatable because it meant every single piece of code that examined a SyntaxKind.IfStatement something highly suspect and in need of auditing.

The approach i settled on here was to instead change the Roslyn syntactic model to allow the parentheses of an IfStatement to be optional. However, there is a compiler/parser enforced invariant, that you can only elide those parentheses if the conditional is of the form !(...). i.e. the condition must be a logical-not surrounding a parenthesized-expression.

The actual LanguageParser impl initially operates exactly as the updated grammar would expect. It looks for an optional !, and then parses the rest of the if statement normally. Then, if it saw !, it takes it, the parens and the condition and then manually just constructs the !(cond) expression and passes that along as hte condition, with the parens no longer being passed explicitly.

This approach also ensures we don't have any sort of weird precedence problems. For example, we explicitly do not call ParseExpression on the ( as that could consume up through the ) as many different types of expressions, including something that would then try to consume the code of the subsequent true-statement of the if.

This enforcement means we only parse exactly the set of input strings that the above grammar rules allow. However, it also means that the entire roslyn stack now gets a standard IfStatementSyntax with a normal 'Condition' expression. For nearly all analysis/compilation/semantics, this means no changes necessary whatsoever, as almost none of those codepaths care about the presence or absence of outer parens on the IfStatement. Instead, the burden is placed on a far smaller set of consumers. Specifically, those customers for whom specific decisions have to be made based on these tokens being there are not. In other words, a minimal set of IDE features that have specialized logic involving the Open/CloseParent tokens of an IfStatement. Fortunately, it is not hard to determine which features these are. It suffices to just audit usages of those two properties (of which there are just a tiny set in the entire codebase).

While this approach feels a little different than how we might normally do things, i think the pros are significant. IfStatement conditions always remain expressions, and those expressions behave exactly how they always have. There's just a new facility for leaving off the parens in a very specific case, and only features that specifically care about these parens need to care.

--

Note: i have only done this work for IfStatements. I think it would be appropriate to extend this to while and do statements as well. However, i don't want to move forward unless this approach is felt to be appropriate.

--

Todos:

• Gate this behind a language version check. Have currently gated this behind C#8. Will have to move to 8.1 if this would not fit in the 8.0 release.
• Adequate testing at the compiler layer. This includes, but is not limited to:
• Parsing
• Semantic analysis
• Analyzer callbacks
• Emit
• Support 'while' or 'do' statements if the LDM wants this.
• IDE test pass of all potentially impacted features.
• IDE design work for anything new. For example, do we need a formatting option to say if you prefer if !(...) vs. if! (...)? Have to discuss things with team to see what is felt to be appropriate.
• Potential codestyle/feature to allow users to say if they prefer this form, and to update their code accordingly. Work for that is ongoing in: Update 'remove unnecessary parens' to support removing from 'negated if statements'. #30954. I'd like to keep new feature-work separate from just the language-change/fallout PR.
  -[ ] For LDM, we need a PR with a proposal in a separate file next to the template https://github.com/dotnet/csharplang/blob/master/proposals/proposal-template.md.
• In the feature branch, we need to add a feature document under https://github.com/dotnet/roslyn/tree/features/NegatedConditionStatements/docs/features, see for examples for other features in this folder.
• SemanticModel/IOperation/ControlFlowGraph test, etc.
  Also, we need an issue with a Test Plan for the feature, see Test plan for ExpressionVariables #25073, Test plan for enhanced using and foreach #28588, etc. for example. See https://github.com/dotnet/roslyn/blob/master/docs/contributing/Compiler%20Test%20Plan.md for inspiration.

[CyrusNajmabadi]

IMO, this is a hacky solution/syntax for a negated if statement. while using other keywords would make it hard to parse, I think making ! a part of if syntax would make more sense here. Just my 2c.

This is a WIP. It's an attempt to learn more about this approach.

Note: i've looked into other mechanism. Making ! part of the syntax has other problems associated with it. For example:

You no longer have an expression. So all APIs that want to ask questions about expressions cannot work with this.

You need to update anything that is doing any sort of semantic operation to have knowledge of this. F or example, when binding the condition of an if, you have to pass this information down. Then expression binding needs to act as if it had a unary not expression there.

How do you represent overloaded operator calls? ! can be overloaded. How do you get that information from the tree? How does a feature like goto-def work here? How does find-refs work? Every single one of them needs ot be updated to be aware of the new syntactic form, instead of htis all falling out since !(expr) is just a normal expression like it always was.

How do you manipulate code? If i want to 'invert an if condition' i now need to know what sort of 'if' this is and pass this information downwards.

Effectively if! now needs to be recognized and the ! information has to pass downward into all analysis code which now needs to go "ok, how would i treat that as if the ! applied to the expression?"

To me, it's completely reasonable to say:

In the case of an if-statement whose condition is a negated-parenthesized expression, the outer parens are optional.

This has the virtue of any and all semantic analysis code is completely untouched. It has the downside of any syntactically sensitive feature (like formatting) needing to know about this. However those same features would still have to know about any sort of if! construct as well.

So this has the virtue of eliminating an entire category of awareness.

--

Finally, i don't see anything hacky here given that syntax already fully supports the concept of optional child nodes/tokens. So this doesn't feel strange to me at all, and is vastly less work (so far) than other approaches.

[CyrusNajmabadi]

@jaredpar @jasonmalinowski Do you know what's up with the testing system. I'm seeing it say things like:

[CyrusNajmabadi]

Tagging @gafter who is championed on dotnet/csharplang#882

[gafter]

This approach feels similar to how we allow "tuple switches" in the recursive pattern-matching feature branch (by making the switch statement's own parens optional rather than adding a comma and further expressions inside them)

switch (a, b)
{
    case (1, 2):
        etc
}

[CyrusNajmabadi]

This approach feels similar to how we allow "tuple switches" in the recursive pattern-matching feature branch (by making the parens optional)

@gafter Thanks for letting me know that. Having other areas also investigating similar implementation strategies gives me more confidence this is a reasonable approach going forward.

[AlekseyTs]

this.CurrentToken.Kind == SyntaxKind.ExclamationToken [](start = 35, length = 53)

Consider changing parsing logic only if the ExclamationToken is followed by the OpenParenToken to keep the diagnostics and the parse tree as they were before this feature.

[CyrusNajmabadi]

Hi @AlekseyTs . I'll look into that. I think it will depend on which produces more sensible errors in the case where you write something like if !non_paren_expr (which people are likely to try). Thanks!

[AlekseyTs]

I think it will depend on which produces more sensible errors in the case where you write something like if !non_paren_expr (which people are likely to try).

I think, if user is targeting C# 7.3, we better not suggest (with diagnostics) to add parentheses where they wouldn't be allowed anyway for that language version. Strictly speaking they would be allowed there, but they are actually recured in a different place and placing them where suggetsed wouldn't fix the problem.

---

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

[AlekseyTs]

@CyrusNajmabadi Going forward, I think it would be more manageable to review PRs submitted into the the feature branch.

[CyrusNajmabadi]

@CyrusNajmabadi Going forward, I think it would be more manageable to review PRs submitted into the the feature branch.

Makes sense to me. How do you guys generally do that? Should i retarget thsi to roslyn:features/guard-statements and have you submit there? I don't think i can make a roslyn branch. So i'd need you to make me one. We could then commit this there and then followup with further work. Thanks!

[AlekseyTs]

I'll create a feature branch and then we should be able to retarget the PR

[AlekseyTs]

Overall, the compiler implementation changes in iteration 32 look good to me. We do need tests. Those that already were mentioned in #30945 (review), plus SemanticModel/IOperation/ControlFlowGraph test, etc.
Also, we need an issue with a Test Plan for the feature, see #25073, #28588, etc. for example. See https://github.com/dotnet/roslyn/blob/master/docs/contributing/Compiler%20Test%20Plan.md for inspiration.

[CyrusNajmabadi]

@AlekseyTs Great. Thanks for letting me know. Is there any time pressure on any of this? If not, i'll try to fill this out over the holidays as i get time. Let me know when the feature branch is ready as well and i can start retargetting things. Thanks!

[AlekseyTs]

The feature branch https://github.com/dotnet/roslyn/tree/features/NegatedConditionStatements has been created from master and CI should be enabled for it.

For LDM, we need a PR with a proposal in a separate file next to the template https://github.com/dotnet/csharplang/blob/master/proposals/proposal-template.md.

In the feature branch, we need to add a feature document under https://github.com/dotnet/roslyn/tree/features/NegatedConditionStatements/docs/features, see for examples for other features in this folder.

[CyrusNajmabadi]

@AlekseyTs Thanks so much! I will add these to the checklist of things to do at the top of this PR.

[CyrusNajmabadi]

closing

[CyrusNajmabadi]

Opening!

[gafter]

At LDM today we noted that the proposed addition of a negated pattern might make this feature less compelling. For example, you'd be able to write

    if (o is not null) ...

[jaredpar]

Sorry this PR was closed accidentally. I was doing a stale branch cleanup in this repository because our branch count had gotten a bit out of control. This was impacting our clone time in CI and causing a non-trivial number of build failures. Hence I did a cleanup which essentially deleted every branch where HEAD was already merged into master and it was not a known VS release branch. That caught features/NegatedConditinoalStatements in the query and it was deleted. Deleting that branch in turn apparently closed your PR.

I've restored the branch so this PR should be able to be re-opened now. About to attempt that.

[jaredpar]

Hmm ... apparently we can't re-open the PR. Probably an effect of deleting the target branch when the PR is opened. Can you create a new PR for this change? Sorry again for doing this. I hadn't considered this particular case when I was doing the branch clean up work.

[CyrusNajmabadi]

Sure. I'm happy to reopen in the future. However, @gafter @jaredpar can you get a read if this is something the LDM is interested in pursuing, or would they prefer to go the route of if (o is not null)? Thanks!

[RikkiGibson]

I feel that there would be value in this feature even if o is not null is added to the language and that the LDM should consider that when evaluating this feature.

[gafter]

I have doubts over whether or not we will want this feature, but having the PR will help us decide more on the merits of the language features and less on the effort.

[CyrusNajmabadi]

SGTM. I will reopen. I do still like the feature, but i also agree it's less compelling in a not pattern world.

[AlekseyTs]

@CyrusNajmabadi

I will reopen. I do still like the feature, but i also agree it's less compelling in a not pattern world.

For LDM, we need a proposal in a separate file based on the https://github.com/dotnet/csharplang/blob/master/proposals/proposal-template.md. Could you please submit one?

[AlekseyTs]

@CyrusNajmabadi

We discussed this feature at recent LDM - https://github.com/dotnet/csharplang/blob/master/meetings/2019/LDM-2019-09-11.md#negated-condition-if-statement. The conclusion was to reevaluate if we still want the feature after C# 9: "consider after "is not" has shipped and see if there are significant use cases that are not addressed by the "is not" pattern."

[CyrusNajmabadi]

@AlekseyTs Thanks for the update. Given that, i'll likely table this for now. This was just a PoC to validate this approach could work well for compiler/IDE. It could certainly be resurrected in the future if LDM thinks there's value in the guard statement even if we have is not pattern.

Thanks!