-
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
List patterns: counterexample for non-exhaustive switch expressions #55327
List patterns: counterexample for non-exhaustive switch expressions #55327
Conversation
bcdc483
to
cc00129
Compare
Co-authored-by: Julien Couvreur <jcouv@users.noreply.github.com>
int lengthValue = lengthValues.Sample.Int32Value; | ||
if (slice != null && lengthValues.All(BinaryOperatorKind.Equal, lengthValue)) | ||
{ | ||
// Bail if there's a slice but only one length value is remained. |
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.
I wonder if we can support some of these cases, as this could be useful. Consider: null or { Length: 4 } => 0, [_, .. var middle, _] => 0
(that'd be a good test to add).
Drawing the distinction between cases we can correctly explain and those we can't may be tricky.
In draft #57210 I'd probably gone a bit too far in trying to explain more slice scenarios, but it may give you some idea.
We can also punt for now and file a follow-up issue to refine.
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.
I added a test but it didn't hit this check. I think it's actually unlikely to get here without a nested length 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.
It may be good to say why we bail (both cases).
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.
Sorry, I see there was a typo in my example. Should have said not 4
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.
Then it would be exhaustive. (no error)
case BoundDagIndexerEvaluation e: | ||
var indexerTemp = new BoundDagTemp(e.Syntax, e.IndexerType, e); | ||
int index = e.Index; | ||
if (index > lengthValue - 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.
Do we need this check? #Closed
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.
It would hit if we didn't check slice/lengthValues above. But I think it's safer to just check all this upfront to avoid a crash. The explainer is generally defensive on out-of-bound errors and such. See tryHandleTuplePattern
for instance.
Should check the other end as well. (added)
continue; | ||
// A list pattern can only support a single slice within. | ||
// We won't try to generate a list pattern if there's more. | ||
case BoundDagSliceEvaluation e when slice == null: |
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.
nit: We can remove the when
clause and use an assertion instead (Debug.Assert(slice is null);
) #Closed
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.
See comment above.
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.
Would it help to add a Assert.Fail in code paths that we don't have a test scenario for?
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.
I'd even throw UnreachableException, since we won't get here with multiple slices.
Never mind
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.
Done with review pass (iteration 8)
d8bccf6
to
b3db969
Compare
if (slice.StartIndex - slice.EndIndex > lengthValue) | ||
{ | ||
// Bail if the sample value is less than the required minimum length by the slice. | ||
return null; |
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.
Is this reachable? If not, consider throwing UnreachableException instead.
Ah, saw your comment about the explainer being defensive. #Closed
var src = @" | ||
_ = new C() switch | ||
{ | ||
null or { Length: 4 } => 0, |
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.
Should this be not 4
?
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 Thanks (iteration 12) modulo one test nit
@333fred @dotnet/roslyn-compiler for second review. Thanks |
return null; | ||
|
||
if (!constraints.All(c => c switch | ||
{ |
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.
Can we indent this? The current indentation level hurts my head.
}; | ||
|
||
_ = this switch // didn't test for not null first element | ||
_ = this switch // we didn't test for [.. null] but we're looking for an example with Length=1. incorrect explanation [.. null, null] // 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.
// _ = this switch // we didn't test for [.. null] but we're looking for an example with Length=1. incorrect explanation [.. null, null] // 1 | ||
Diagnostic(ErrorCode.WRN_SwitchExpressionNotExhaustiveForNull, "switch").WithArguments("_").WithLocation(20, 18), | ||
// (27,18): warning CS8509: The switch expression does not handle all possible values of its input type (it is not exhaustive). For example, the pattern '_' is not covered. | ||
// _ = this switch // didn't test for [.. [not null]] // // 2 |
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.
It general, it would be good to add an explanatory comment in this file somewhere that these are the ideal explanations, but we're not able to handle these cases.
Done review pass (commit 13). Just a bit of formatting cleanup. |
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 14)
Relates to test plan #51289