-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
AbstractFlowPass.VisitIsPatternExpression - add handling for list-pattern specific patterns #59285
Conversation
@@ -1035,6 +1035,7 @@ static bool patternMatchesNull(BoundPattern pattern) | |||
case BoundITuplePattern: | |||
case BoundRelationalPattern: | |||
case BoundDeclarationPattern: | |||
case BoundListPattern: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's another switch with a missing case in patternMatchesNull above.
It's not clear how an exhaustive switch is supposed to help here since not every pattern node is going through this check as demonstrated in this bug. Unless there's a compile-time diagnostic, you'd need to scan the code to find each instance or else, wait for a crash later. Would it make sense to have a safe default with Assert(false) instead of throwing right away? (or make it so that every pattern is visited here regardless of the source tree) #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Fixed that other case, plus two more (for slices).
I added an assertion that should help catch this problem with future patterns.
@333fred @dotnet/roslyn-compiler for review. Thanks |
@@ -917,6 +917,12 @@ public override BoundNode VisitIsPatternExpression(BoundIsPatternExpression node | |||
{ | |||
Debug.Assert(!IsConditionalState); | |||
|
|||
// Local functions below need to handle new kinds of patterns |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jcouv Consider using more descriptive title, especially when merging. Something like: "AbstractFlowPass.VisitIsPatternExpression - add handling for list-pattern specific patterns." |
// if ((a is [var y] and y) is .. 1) | ||
Diagnostic(ErrorCode.ERR_NoImplicitConv, "y").WithArguments("int", "int[]").WithLocation(8, 23), | ||
// (8,29): error CS0021: Cannot apply indexing with [] to an expression of type 'bool' | ||
// if ((a is [var y] and y) is .. 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it's only valid for a slice-pattern to be in a list-pattern (once). But from a parsing point of view, the slice-pattern is a primary pattern.
But the error for bad usage of slice-pattern ("Slice patterns may only be used once and directly inside a list pattern.") isn't produced when the operand is an error. That's why that error doesn't appear in this test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (commit 3)
Fixes #58738
Relates to test plan #51289