-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -917,6 +917,12 @@ public override BoundNode VisitIsPatternExpression(BoundIsPatternExpression node | |
{ | ||
Debug.Assert(!IsConditionalState); | ||
|
||
// Local functions below need to handle new kinds of patterns | ||
Debug.Assert(node.Pattern is | ||
BoundTypePattern or BoundRecursivePattern or BoundITuplePattern or BoundRelationalPattern or | ||
BoundDeclarationPattern or BoundConstantPattern or BoundNegatedPattern or BoundBinaryPattern or | ||
BoundDeclarationPattern or BoundDiscardPattern or BoundListPattern or BoundSlicePattern); | ||
|
||
bool negated = node.Pattern.IsNegated(out var pattern); | ||
Debug.Assert(negated == node.IsNegated); | ||
|
||
|
@@ -976,6 +982,8 @@ static bool patternMatchesNull(BoundPattern pattern) | |
case BoundRelationalPattern: | ||
case BoundDeclarationPattern { IsVar: false }: | ||
case BoundConstantPattern { ConstantValue: { IsNull: false } }: | ||
case BoundListPattern: | ||
case BoundSlicePattern: // Only occurs in error cases | ||
return false; | ||
case BoundConstantPattern { ConstantValue: { IsNull: true } }: | ||
return true; | ||
|
@@ -1035,6 +1043,8 @@ 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 commentThe 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 commentThe 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. |
||
case BoundSlicePattern: // Only occurs in error cases | ||
return null; | ||
default: | ||
throw ExceptionUtilities.UnexpectedValue(pattern.Kind); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8121,4 +8121,81 @@ public void ListPattern_NullTestOnSlice() | |
Diagnostic(ErrorCode.ERR_SwitchCaseSubsumed, "[1,2,3]").WithLocation(15, 10) | ||
); | ||
} | ||
|
||
[Fact, WorkItem(58738, "https://github.com/dotnet/roslyn/issues/58738")] | ||
public void ListPattern_AbstractFlowPass_isBoolTest() | ||
{ | ||
var source = @" | ||
var a = new[] { 1 }; | ||
|
||
if (a is [var x] and x is [1]) | ||
{ | ||
} | ||
|
||
if ((a is [var y] and y) is .. 1) | ||
{ | ||
} | ||
|
||
var b = new[] { true }; | ||
if ((b is [var z] and z) is [true]) | ||
{ | ||
} | ||
"; | ||
var comp = CreateCompilationWithIndexAndRangeAndSpan(new[] { source, TestSources.GetSubArray }); | ||
comp.VerifyEmitDiagnostics( | ||
// (4,22): error CS0029: Cannot implicitly convert type 'int' to 'int[]' | ||
// if (a is [var x] and x is [1]) | ||
Diagnostic(ErrorCode.ERR_NoImplicitConv, "x").WithArguments("int", "int[]").WithLocation(4, 22), | ||
// (4,27): error CS0021: Cannot apply indexing with [] to an expression of type 'bool' | ||
// if (a is [var x] and x is [1]) | ||
Diagnostic(ErrorCode.ERR_BadIndexLHS, "[1]").WithArguments("bool").WithLocation(4, 27), | ||
// (8,23): error CS0029: Cannot implicitly convert type 'int' to 'int[]' | ||
// 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 commentThe 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 commentThe 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. |
||
Diagnostic(ErrorCode.ERR_BadIndexLHS, ".. 1").WithArguments("bool").WithLocation(8, 29), | ||
// (13,23): error CS0029: Cannot implicitly convert type 'bool' to 'bool[]' | ||
// if ((b is [var z] and z) is [true]) | ||
Diagnostic(ErrorCode.ERR_NoImplicitConv, "z").WithArguments("bool", "bool[]").WithLocation(13, 23), | ||
// (13,29): error CS0021: Cannot apply indexing with [] to an expression of type 'bool' | ||
// if ((b is [var z] and z) is [true]) | ||
Diagnostic(ErrorCode.ERR_BadIndexLHS, "[true]").WithArguments("bool").WithLocation(13, 29) | ||
); | ||
} | ||
|
||
[Fact, WorkItem(58738, "https://github.com/dotnet/roslyn/issues/58738")] | ||
public void ListPattern_AbstractFlowPass_patternMatchesNull() | ||
{ | ||
var source = @" | ||
var a = new[] { 1 }; | ||
|
||
if (a?.M(out var i) is [1]) | ||
i.ToString(); | ||
else | ||
i.ToString(); // 1 | ||
|
||
if (a?.M(out var j) is .. 1) // 2, 3 | ||
j.ToString(); | ||
else | ||
j.ToString(); | ||
|
||
public static class Extension | ||
{ | ||
public static T M<T>(this T t, out int i) => throw null; | ||
} | ||
"; | ||
var comp = CreateCompilationWithIndexAndRangeAndSpan(new[] { source, TestSources.GetSubArray }); | ||
comp.VerifyEmitDiagnostics( | ||
// (7,5): error CS0165: Use of unassigned local variable 'i' | ||
// i.ToString(); // 1 | ||
Diagnostic(ErrorCode.ERR_UseDefViolation, "i").WithArguments("i").WithLocation(7, 5), | ||
// (9,24): error CS8980: Slice patterns may only be used once and directly inside a list pattern. | ||
// if (a?.M(out var j) is .. 1) // 2, 3 | ||
Diagnostic(ErrorCode.ERR_MisplacedSlicePattern, ".. 1").WithLocation(9, 24), | ||
// (9,27): error CS0029: Cannot implicitly convert type 'int' to 'int[]' | ||
// if (a?.M(out var j) is .. 1) // 2, 3 | ||
Diagnostic(ErrorCode.ERR_NoImplicitConv, "1").WithArguments("int", "int[]").WithLocation(9, 27) | ||
); | ||
} | ||
} |
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.
"all patterns that come through here"? #Closed