List patterns: counterexample for non-exhaustive switch expressions by alrz · Pull Request #55327 · dotnet/roslyn

alrz · 2021-08-01T16:37:46Z

Relates to test plan #51289

Co-authored-by: Julien Couvreur <jcouv@users.noreply.github.com>

src/Compilers/CSharp/Portable/Binder/PatternExplainer.cs

jcouv · 2021-10-20T21:45:41Z

src/Compilers/CSharp/Portable/Binder/PatternExplainer.cs

+                    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.


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.

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.

It may be good to say why we bail (both cases).

Sorry, I see there was a typo in my example. Should have said not 4

Then it would be exhaustive. (no error)

jcouv · 2021-10-20T21:48:39Z

src/Compilers/CSharp/Portable/Binder/PatternExplainer.cs

+                            case BoundDagIndexerEvaluation e:
+                                var indexerTemp = new BoundDagTemp(e.Syntax, e.IndexerType, e);
+                                int index = e.Index;
+                                if (index > lengthValue - 1)


Do we need this check? #Closed

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)

jcouv · 2021-10-20T21:49:45Z

src/Compilers/CSharp/Portable/Binder/PatternExplainer.cs

+                                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:


nit: We can remove the when clause and use an assertion instead (Debug.Assert(slice is null);) #Closed

See comment above.

Would it help to add a Assert.Fail in code paths that we don't have a test scenario for?

~~I'd even throw UnreachableException, since we won't get here with multiple slices.~~
Never mind

jcouv

Done with review pass (iteration 8)

…the slice.

jcouv · 2021-10-22T16:54:46Z

src/Compilers/CSharp/Portable/Binder/PatternExplainer.cs

+                        if (slice.StartIndex - slice.EndIndex > lengthValue)
+                        {
+                            // Bail if the sample value is less than the required minimum length by the slice.
+                            return null;


~~Is this reachable? If not, consider throwing UnreachableException instead.~~
Ah, saw your comment about the explainer being defensive. #Closed

jcouv · 2021-10-22T16:58:08Z

src/Compilers/CSharp/Test/Semantic/Semantics/PatternMatchingTests_ListPatterns.cs

+        var src = @"
+_ = new C() switch
+{
+    null or { Length: 4 } => 0,


Should this be not 4?

jcouv

LGTM Thanks (iteration 12) modulo one test nit

jcouv · 2021-10-22T17:00:03Z

@333fred @dotnet/roslyn-compiler for second review. Thanks

333fred · 2021-10-22T18:51:28Z

src/Compilers/CSharp/Portable/Binder/PatternExplainer.cs

+                    return null;
+
+                if (!constraints.All(c => c switch
+                {


Can we indent this? The current indentation level hurts my head.

333fred · 2021-10-22T19:13:27Z

src/Compilers/CSharp/Test/Semantic/Semantics/PatternMatchingTests_ListPatterns.cs

        };

-        _ = 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


incorrect explanation [.. null, null]

Let's remove this bit.

333fred · 2021-10-22T19:14:04Z

src/Compilers/CSharp/Test/Semantic/Semantics/PatternMatchingTests_ListPatterns.cs

+                //         _ = 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


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.

333fred · 2021-10-22T19:19:58Z

Done review pass (commit 13). Just a bit of formatting cleanup.

333fred

LGTM (commit 14)

ghost added the Area-Compilers label Aug 1, 2021

alrz changed the title ~~List patterns: generate counterexample for non-exhaustive switch expressions~~ List patterns: counterexample for non-exhaustive switch expressions Aug 2, 2021

WIP

cc00129

alrz force-pushed the list-patterns-explainer branch from bcdc483 to cc00129 Compare October 18, 2021 19:42

alrz mentioned this pull request Oct 18, 2021

Explainer for list-patterns #57210

Closed

alrz and others added 7 commits October 19, 2021 00:53

WIP

e9eb399

Cherry-pick tests from dotnet#57210

7dc6ac4

Co-authored-by: Julien Couvreur <jcouv@users.noreply.github.com>

Update baseline

4b29226

Skip discarded slice pattern

abd7676

Bail if there's a slice but only one length value is remained

3012db0

Reorder

b5b117e

Remove comments

2bf9fc4

alrz marked this pull request as ready for review October 20, 2021 19:24

alrz requested a review from a team as a code owner October 20, 2021 19:24

jcouv self-assigned this Oct 20, 2021

jcouv added the Feature - List Patterns label Oct 20, 2021

jcouv reviewed Oct 20, 2021

View reviewed changes

src/Compilers/CSharp/Portable/Binder/PatternExplainer.cs Outdated Show resolved Hide resolved

jcouv reviewed Oct 20, 2021

View reviewed changes

alrz added 4 commits October 22, 2021 18:59

Add test

ddbc0c6

Bound check

7121a44

Reword

7dc82d4

Bail if the sample value is less than the required minimum length by …

b3db969

…the slice.

alrz force-pushed the list-patterns-explainer branch from d8bccf6 to b3db969 Compare October 22, 2021 16:16

alrz requested a review from jcouv October 22, 2021 16:17

jcouv reviewed Oct 22, 2021

View reviewed changes

jcouv approved these changes Oct 22, 2021

View reviewed changes

jcouv added this to the 17.1.P1 milestone Oct 22, 2021

Add comments

dcaac93

333fred reviewed Oct 22, 2021

View reviewed changes

PR feedback

01ea646

333fred approved these changes Oct 22, 2021

View reviewed changes

runfoapp bot mentioned this pull request Oct 22, 2021

Light bulb menu showed up over Pick Members dialog #57321

Closed

jcouv mentioned this pull request Oct 22, 2021

Test Plan for "List Patterns" #51289

Closed

91 tasks

jcouv modified the milestones: 17.1.P1, 17.0, 17.1 Oct 27, 2021

jcouv merged commit 94ea14c into dotnet:features/list-patterns Oct 27, 2021

Conversation

alrz commented Aug 1, 2021 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alrz Oct 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alrz Oct 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Oct 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alrz Oct 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Oct 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alrz Oct 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Oct 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

jcouv Oct 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

jcouv commented Oct 22, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

333fred commented Oct 22, 2021

Uh oh!

333fred left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

alrz commented Aug 1, 2021 •

edited by jcouv

Loading

alrz Oct 22, 2021 •

edited

Loading

alrz Oct 22, 2021 •

edited

Loading

jcouv Oct 20, 2021 •

edited

Loading

alrz Oct 22, 2021 •

edited

Loading

jcouv Oct 20, 2021 •

edited

Loading

alrz Oct 22, 2021 •

edited

Loading

jcouv Oct 22, 2021 •

edited

Loading

jcouv Oct 22, 2021 •

edited

Loading