Fix parsing of parenthesized type in switch arm #81863
Conversation
| } | ||
|
|
||
| private bool ScanCast(bool forPattern = false) | ||
| private bool ScanCast(bool forPattern, bool inSwitchArmPattern) |
There was a problem hiding this comment.
Made existing parameter non-optional. There are only two callers. I think it's much clearer for each caller to be explicit. Added new parameter, to pass along contextual value we already pass through pattern parsing and use for the same purpose elsewhere.
| } | ||
|
|
||
| private bool IsValidPatternDesignation(bool whenIsKeyword) | ||
| private bool IsValidPatternDesignation(bool inSwitchArmPattern) |
There was a problem hiding this comment.
intentionally renamed to keep this consistent with the value passed along through all of pattern parsing. I think it's clearer that the pattern says what context we're in, insstead of what impact it has. This then makes it clear at callsites what is supposed to be passed in. The impact of this context is hten documented below.
| UsingExpression(""" | ||
| reqs switch | ||
| { | ||
| (CustomEnum.EnumTwo) when => "ok", |
There was a problem hiding this comment.
note: this is technically a breaking change. so we may need to have a deeper discussion/buy-off on this. Previously, this would parse as a cast, which technically could be legal. That said, it would be a pathological case of needing a constant called 'when' casted to some integral/enum value. Given that 'when' was treated as a switch-arm-when-clause in all other circumstances before, this doesn't seem reasonable as something someone would actually have a dependency on.
or, if they did, it also seems like something completely fine to still treat as a bug needing fixing, where the break is acceptable.
There was a problem hiding this comment.
I'd like to understand what the spec expects this to parse as for context.
There was a problem hiding this comment.
Will get chapter/verse. However, my expectation is that as per spec that would be parsed as a cast expression.
However. In a similar, vein, we do not allow: CustomEnum.EnumTwo when => which would also be a similar case of a TypeName designator due to unilaterally treating the when in the latter as the start of a when clause.
Put another way, this space has deep syntactic ambiguities. But we have resolved them mostly uniformly (preferring the parse as if when is the keyword), except for this particular case.
We have a few options:
- accept current behavior. You can work around things.
- Change parsing to be more consistent (with a small breaking change).
- Attempt to accept these constructs in the parser where the lang has ambiguity.
My preference is '2'.
There was a problem hiding this comment.
Based on the spec: we have simply:
constant_pattern
: constant_expression
;
...
constant_expression
: expression
;
So, effectively, any expression is allowed in a pattern location. This is obviously deeply ambiguous. We have a similar issue given that we have:
declaration_pattern
: type simple_designation
;
Where we clearly don't follow that either, esp. with dealing with confusing contextual keywords.
:)
My take would be that the parser is actually doing a good thing in agressively viewing the when as a keyword in a switch arm (we do it in all other cases throught he recursive descent into pattern parsing). We just missed it in the part when we're checking if we have a cast or a parenthesized constant pattern.
|
@dotnet/roslyn-compiler this is ready for review. |
There was a problem hiding this comment.
Let's have some versions with (CustomEnum.EnumTwo when).
|
@333fred @dotnet/roslyn-compiler ptal. |
Co-authored-by: Jan Jones <jan.jones.cz@gmail.com>
Co-authored-by: Jan Jones <jan.jones.cz@gmail.com>
Co-authored-by: Jan Jones <jan.jones.cz@gmail.com>
4 participants
Fixes #81837
Core issue was that parser was seeing
(A.B)whenas looking like a cast-expression in all cases. Instead of recognizing that in a switch-arm that thewhenshould instead be the start of a when clause (which is how we treat 'when' unilaterally elsewhere when parsing out a switch-arm).