Improve parser recovery around nullable types in patterns

[DoctorKrolic]

Closes: #72720

[CyrusNajmabadi]

i appreciate all the new tests.

[CyrusNajmabadi]

Done with pass. Overall approve of the high level idea. Def wary of the actual code change (i don't understand it yet), and def think we need to beef up tests to ensure that things that are now legal to parse give proper diagnostics.

[CyrusNajmabadi]

this comment was great.

[CyrusNajmabadi]

Ok. Pushed. I'm also ok changing the await-handling and updating the tests accordingly. Are you ok with me doing that, or would you prefer to do that?

[DoctorKrolic]

Are you ok with me doing that, or would you prefer to do that?

Go ahead. Just add a simple comment to changed tests, so it is obvious, that it is an intentional "degradation"

[CyrusNajmabadi]

Will do.

[CyrusNajmabadi]

whenIsKeyword got renamed to inSwitchArmPattern to try to be clearer. We pass 'true' in here when parsign the pattern for a switch-statement or switch-expression pattern. And it controls how we view 'where' and a few other error recovery rules (including new ones in this pr).

[CyrusNajmabadi]

one of hte two locations where 'true' is passed in. Here we're parsing case clauses for switch-statements.

[CyrusNajmabadi]

one of hte two locations where 'true' is passed in. Here we're parsing the arms for switch-expressions.

[CyrusNajmabadi]

lgtm. @dotnet/roslyn-compiler for reviews please.

[333fred]

Done review pass. I didn't get any further than the comments that spelled out invalid assumptions for parsing behavior.

[333fred]

This assumption is not correct. The following is valid code:

        switch(i3) {
            case b ? i1 : i2: break;
        }

This comment needs to be corrected, and we should re-examine this approach to ensure that we're not breaking anything.

[DoctorKrolic]

The "why on Earth this is legal" question should have probably been asked N years ago when switch statements were designed) I was sure that conditional expressions in case labels aren't semantically legal and thus there is no difference whether this is parsed as a conditional or a nullable pattern + label statement. There is already a test in the codebase for that case and I initially wrote a comment explaining how these 2 parse strategies both lead to a compiler error thus are identical for our purposes, had to revert that change. And I'm somewhat surprised that only a very few new test cases now are back to bad state after removing this assumption, so I don't think we should do additional work to restore them

[CyrusNajmabadi]

The "why on Earth this is legal" question should have probably been asked N years ago when switch statements were designed)

So, this is legal because cases allow for patterns. And one sort of pattern is the 'constant pattern' (that's how x is 0 works for example). So these places allow for any sort of constant expression (including conditional expressions). Note: for conditional expressions to be constant, their condition itself must be constant. But that can often normally happen, esp. in generated code, or code referencing constants that are conditionally compiled in.

[333fred]

Similarly, this assumption is closer to correct, but isn't always:

        _ = i3 switch {
                (b ? i1 : i2) => 3
        };

You do need the parentheses to force parse in this manner, but I'm concerned that we'll still end up with bad behavior here.

[DoctorKrolic]

Given that this requires parentheses, it wasn't affected even with the wrong assumption. Added a test for this case

[CyrusNajmabadi]

can you have a test for switch statements as well?

[DoctorKrolic]

They are already in the codebase. See commit 3a6f3bd, I've changed one of those tests back to what it was. The parenthesized case is exactly below it

[DoctorKrolic]

Here they are in main:

roslyn/src/Compilers/CSharp/Test/Syntax/Parsing/PatternParsingTests.cs

Lines 12585 to 12690 in 93d19c2

	[Fact]
	public void ConditionalAsConstantPatternInSwitchStatement()
	{
	UsingStatement(@"switch(a) { case a?x:y: break; }",
	TestOptions.RegularWithPatternCombinators
	);
	N(SyntaxKind.SwitchStatement);
	{
	N(SyntaxKind.SwitchKeyword);
	N(SyntaxKind.OpenParenToken);
	N(SyntaxKind.IdentifierName);
	{
	N(SyntaxKind.IdentifierToken, "a");
	}
	N(SyntaxKind.CloseParenToken);
	N(SyntaxKind.OpenBraceToken);
	N(SyntaxKind.SwitchSection);
	{
	N(SyntaxKind.CaseSwitchLabel);
	{
	N(SyntaxKind.CaseKeyword);
	N(SyntaxKind.ConditionalExpression);
	{
	N(SyntaxKind.IdentifierName);
	{
	N(SyntaxKind.IdentifierToken, "a");
	}
	N(SyntaxKind.QuestionToken);
	N(SyntaxKind.IdentifierName);
	{
	N(SyntaxKind.IdentifierToken, "x");
	}
	N(SyntaxKind.ColonToken);
	N(SyntaxKind.IdentifierName);
	{
	N(SyntaxKind.IdentifierToken, "y");
	}
	}
	N(SyntaxKind.ColonToken);
	}
	N(SyntaxKind.BreakStatement);
	{
	N(SyntaxKind.BreakKeyword);
	N(SyntaxKind.SemicolonToken);
	}
	}
	N(SyntaxKind.CloseBraceToken);
	}
	EOF();
	}
	[Fact]
	public void ConditionalAsConstantPatternInSwitchStatement_Parenthesized()
	{
	UsingStatement(@"switch(a) { case (a?x:y): break; }",
	TestOptions.RegularWithPatternCombinators
	);
	N(SyntaxKind.SwitchStatement);
	{
	N(SyntaxKind.SwitchKeyword);
	N(SyntaxKind.OpenParenToken);
	N(SyntaxKind.IdentifierName);
	{
	N(SyntaxKind.IdentifierToken, "a");
	}
	N(SyntaxKind.CloseParenToken);
	N(SyntaxKind.OpenBraceToken);
	N(SyntaxKind.SwitchSection);
	{
	N(SyntaxKind.CaseSwitchLabel);
	{
	N(SyntaxKind.CaseKeyword);
	N(SyntaxKind.ParenthesizedExpression);
	{
	N(SyntaxKind.OpenParenToken);
	N(SyntaxKind.ConditionalExpression);
	{
	N(SyntaxKind.IdentifierName);
	{
	N(SyntaxKind.IdentifierToken, "a");
	}
	N(SyntaxKind.QuestionToken);
	N(SyntaxKind.IdentifierName);
	{
	N(SyntaxKind.IdentifierToken, "x");
	}
	N(SyntaxKind.ColonToken);
	N(SyntaxKind.IdentifierName);
	{
	N(SyntaxKind.IdentifierToken, "y");
	}
	}
	N(SyntaxKind.CloseParenToken);
	}
	N(SyntaxKind.ColonToken);
	}
	N(SyntaxKind.BreakStatement);
	{
	N(SyntaxKind.BreakKeyword);
	N(SyntaxKind.SemicolonToken);
	}
	}
	N(SyntaxKind.CloseBraceToken);
	}
	EOF();
	}