List pattern syntax by alrz · Pull Request #51299 · dotnet/roslyn

alrz · 2021-02-18T05:22:40Z

Spec: https://github.com/dotnet/csharplang/blob/main/proposals/list-patterns.md
Test plan: #51289

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

.editorconfig

src/Compilers/CSharp/Portable/Syntax/Syntax.xml

jcouv · 2021-03-01T22:39:48Z

I couldn't find the csharplang spec/proposal. Was it uploaded? #Closed

alrz · 2021-03-02T04:52:29Z

I couldn't find the csharplang spec/proposal. Was it uploaded?

Not yet. The proposal is updated at dotnet/csharplang#3245 #Closed

…t-patterns-syntax

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

jcouv · 2021-03-24T04:59:23Z

...tures/CSharp/Portable/Completion/CompletionProviders/PropertySubPatternCompletionProvider.cs

            }

-            return token.Parent.IsKind(SyntaxKind.PropertyPatternClause) ? token : default;
+            return token.Parent.IsKind(SyntaxKind.PropertyPatternClause, SyntaxKind.ListPatternClause) ? token : default;


Is there a test for this change?

Lots of failing tests for this completion provider.

{ $$ } always see a list pattern clause.

jcouv · 2021-03-24T04:59:38Z

...s/SharedUtilitiesAndExtensions/Compiler/CSharp/Formatting/Rules/IndentBlockFormattingRule.cs


            // for lambda, set alignment around braces so that users can put brace wherever they want
-            if (node.IsLambdaBodyBlock() || node.IsAnonymousMethodBlock() || node.IsKind(SyntaxKind.PropertyPatternClause) || node.IsKind(SyntaxKind.SwitchExpression))
+            if (node.IsLambdaBodyBlock() || node.IsAnonymousMethodBlock() || node.IsKind(SyntaxKind.PropertyPatternClause, SyntaxKind.ListPatternClause) || node.IsKind(SyntaxKind.SwitchExpression))


Do we have a corresponding test?

Just one, failing in the previous test runs.

For cases with no failing test I added a comment to cover later.

jcouv

Compiler changes LGTM Thanks (iteration 23)

jcouv · 2021-03-24T05:03:36Z

The correctness leg reports a diagnostic that probably needs to be suppressed:
error RS0027: Symbol 'PropertyPatternClause' violates the backcompat requirement: 'Public API with optional parameter(s) should have the most parameters amongst its public overloads'. See 'https://github.com/dotnet/roslyn/blob/master/docs/Adding%20Optional%20Parameters%20in%20Public%20API.md' for details. #Closed

jcouv · 2021-03-31T21:00:52Z

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

jasonmalinowski · 2021-03-31T21:21:59Z

src/Compilers/CSharp/Portable/Syntax/Syntax.xml

+    <Field Name="DotDotToken" Type="SyntaxToken">
+      <Kind Name="DotDotToken"/>
+    </Field>
+    <Field Name="Pattern" Type="PatternSyntax" Optional="true"/>


Is there an example for what this field is being used for? The proposal implied that nothing can be listed after the .. in the list or that there was still an open question there.

The proposal implied that nothing can be listed after the .. in the list

That's true only for enumerable and multi-dimensional arrays.

For compatible types this optionally matches the returned slice. See https://github.com/dotnet/csharplang/blob/main/proposals/list-patterns.md#lowering-on-countableindexeablesliceable-type

src/Compilers/CSharp/Test/Syntax/Parsing/PatternParsingTests_ListPatterns.cs

333fred · 2021-04-05T23:34:34Z

src/Compilers/CSharp/Test/Syntax/Parsing/PatternParsingTests_ListPatterns.cs

+        }
+
+        [Fact]
+        public void SlicePattern_10()


It would be good to have variations on these slice tests that have the .. after the other pattern component. Also, some pattern combinations with positional and length patterns would be good.

Added a few tests. Let me know if you have any specific scenario that is not covered.

I'd expect to see some of the previous variations you had here. For example, c is { {}.. } from SlicePattern_05. I'd go through your existing tests and add flipped versions of these.

In reply to: 608770979 [](ancestors = 608770979)

333fred · 2021-04-05T23:35:44Z

Done review pass (commit 24). Code looks good, but I'd like a few more tests before signing off. #Closed

333fred · 2021-04-07T20:00:44Z

Done review pass (commit 25)

…t-patterns-syntax # Conflicts: # src/Compilers/CSharp/Portable/CSharpResources.resx # src/Compilers/CSharp/Portable/xlf/CSharpResources.cs.xlf # src/Compilers/CSharp/Portable/xlf/CSharpResources.de.xlf # src/Compilers/CSharp/Portable/xlf/CSharpResources.es.xlf # src/Compilers/CSharp/Portable/xlf/CSharpResources.fr.xlf # src/Compilers/CSharp/Portable/xlf/CSharpResources.it.xlf # src/Compilers/CSharp/Portable/xlf/CSharpResources.ja.xlf # src/Compilers/CSharp/Portable/xlf/CSharpResources.ko.xlf # src/Compilers/CSharp/Portable/xlf/CSharpResources.pl.xlf # src/Compilers/CSharp/Portable/xlf/CSharpResources.pt-BR.xlf # src/Compilers/CSharp/Portable/xlf/CSharpResources.ru.xlf # src/Compilers/CSharp/Portable/xlf/CSharpResources.tr.xlf # src/Compilers/CSharp/Portable/xlf/CSharpResources.zh-Hans.xlf # src/Compilers/CSharp/Portable/xlf/CSharpResources.zh-Hant.xlf

jcouv · 2021-04-19T16:52:01Z

@333fred Please take another look

333fred

LGTM (commit 28). @jcouv, do you want to take another look?

jcouv

Still LGTM Thanks (iteration 28)

jcouv · 2021-04-20T06:05:22Z

Merged/squashed. Thanks!

alrz · 2021-04-20T12:09:14Z

FYI, this is pending a few open questions especially around binding which I'm going to do next.
(while we're awaiting LDM I'm open to move forward with the other PR if there's enough bandwidth)

jcouv · 2021-05-06T06:16:05Z

@alrz I reached out on discord. I didn't realize there were blocking questions for LDM.
Do you mean the question about multi-dimensional arrays described here? Anything else?

alrz · 2021-05-06T07:40:58Z

@jcouv That's not all. Look for "open question" in the document. A few of those are critical to move forward with the first iteration of list patterns.

jcouv · 2021-05-07T04:12:00Z

@alrz Collected the open issues from the list-pattern spec:

Open question: Should we accept a general pattern following .. in a slice_pattern? Answer: let's say yes for now. Allowing some nested patterns versus allowing all isn't a deep change. We can add that before shipping.
Open question: Should we support all these combinations? Answer: LDM already confirmed, yes.
Open question: We should define the exact binding rules for any of these members and decide if we want to diverge from the range spec. Answer: we should stick to the same behavior as ranges. In short, we bind to indexers if available, otherwise special Slice method.
Open question: Should extension methods play a role in sliceability? Answer: we may change that later (for slicing and slice pattern, thus transforming an error scenario into a working scenario) but should assume "no" for now (keep the existing rules from ranges).
Open question: The pattern {..} tests for expr.Length >= 0. Should we omit such test (assuming Length is always non-negative)? Answer: this doesn't need to be spec'ed, it is an implementation detail. We are free to change/optimize what we emit for this case. I'd start without the optimization. We can add if we have time.
Question on multi-dimensional arrays: I didn't quite understand this one...

Tagging @dotnet/roslyn-compiler as FYI

alrz · 2021-05-07T07:25:51Z

@jcouv Ok, that is not very distant from my draft except for "explicit range indexer support" that needs to be added. I will tidy it up and finish the array match lowering for review. Would you prefer a single PR or should I divide it up?

Question on multi-dimensional arrays: I didn't quite understand this one...

That's only a question on the lowering strategy. Given array is {{1}} which one of the following is expected:

array.GetLength(0) == 1 && array.GetLength(1) == 1 && array[0, 0] is 1

array.GetLength(0) == 1 && array.GetLength(1) == 1 &&
array.GetLowerBound(0) is var b0 && array.GetLowerBound(1) is var b1 &&
array[b0 + 0, b1 + 0] is 1

or something else? I think the latter is actually the correct one, but needed a confirmation.

EDIT: I will do md arrays separately, I think that makes it a lot more manageable.

alrz · 2021-05-20T10:46:44Z

A few questions on md arrays in addition to the lowering issue above:

Should we allow length patterns with md arrays (probably no, unless we want to change the syntax to accept [0,0] etc)
Should we allow length patterns in nested lists? (probably no, there's no point to give a length when all of them must match)
Should we allow slice patterns (without subpatterns) in a nested list? (this is possible unless we decide against it)
Should we treat {} as an empty list in a nested pattern? (in that case new int[,] {{},{}} is {{},{}} returns true)

alrz added 3 commits February 18, 2021 08:41

List pattern syntax

f669d73

Update resources

2340d9a

Revert code

589dbd1

alrz requested a review from a team as a code owner February 18, 2021 05:22

ghost added the Area-Compilers label Feb 18, 2021

alrz marked this pull request as draft February 18, 2021 05:23

Revert code

4708bea

alrz commented Feb 18, 2021

View reviewed changes

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs Outdated Show resolved Hide resolved

Fix typo

6de5e2d

jcouv added the Feature - Pattern Matching Pattern Matching label Feb 18, 2021

alrz added 4 commits February 27, 2021 20:20

Adjust parsing to the confirmed syntax

5fa18bb

Simplify

39f8a6f

Revert whitespace

d6be7be

Disable PublicApi diagnostics

fdc50a0

This comment has been minimized.

Sign in to view

alrz commented Feb 27, 2021

View reviewed changes

.editorconfig Outdated Show resolved Hide resolved

alrz marked this pull request as ready for review February 27, 2021 18:11

alrz commented Feb 28, 2021

View reviewed changes

src/Compilers/CSharp/Portable/Syntax/Syntax.xml Show resolved Hide resolved

jcouv added this to the C# 10 milestone Mar 1, 2021

jcouv self-assigned this Mar 9, 2021

This comment has been minimized.

Sign in to view

jcouv mentioned this pull request Mar 20, 2021

Diagnostic suppressions from editorconfig are ignored in source-generated syntax trees #52014

Closed

Attempt to disable RS0016

0b2c4c4

alrz marked this pull request as draft March 20, 2021 08:41

alrz added 4 commits March 20, 2021 12:18

Add tests

b2471d5

Merge remote-tracking branch 'origin/features/list-patterns' into lis…

2eba839

…t-patterns-syntax

Suppress RS0027

dd000b9

Fixup tests

12a5641

jcouv reviewed Mar 24, 2021

View reviewed changes

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs Show resolved Hide resolved

jcouv reviewed Mar 24, 2021

View reviewed changes

jcouv approved these changes Mar 24, 2021

View reviewed changes

Attempt to suppress RS0027

2a8e5b3

alrz force-pushed the list-patterns-syntax branch from 07f1138 to 2a8e5b3 Compare March 24, 2021 06:10

jasonmalinowski reviewed Mar 31, 2021

View reviewed changes

333fred reviewed Apr 5, 2021

View reviewed changes

src/Compilers/CSharp/Test/Syntax/Parsing/PatternParsingTests_ListPatterns.cs Show resolved Hide resolved

333fred reviewed Apr 5, 2021

View reviewed changes

Add more tests

19b7320

jcouv requested a review from 333fred April 7, 2021 19:26

alrz added 3 commits April 13, 2021 20:20

Update resources

fe596e9

Add more tests

f5b20c4

333fred approved these changes Apr 20, 2021

View reviewed changes

jcouv approved these changes Apr 20, 2021

View reviewed changes

jcouv merged commit 65fce0c into dotnet:features/list-patterns Apr 20, 2021

alrz mentioned this pull request May 5, 2021

Implement extended property patterns #52139

Merged

Conversation

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

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

This comment has been minimized.

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!

Choose a reason for hiding this comment

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

jcouv commented Mar 31, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

333fred commented Apr 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

333fred commented Apr 7, 2021

Uh oh!

jcouv commented Apr 19, 2021

Uh oh!

333fred left a comment

Choose a reason for hiding this comment

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

jcouv commented Apr 20, 2021

Uh oh!

alrz commented Apr 20, 2021

Uh oh!

jcouv commented May 6, 2021

Uh oh!

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

Uh oh!

Uh oh!

jcouv commented May 7, 2021

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

alrz commented Feb 18, 2021 •

edited

Loading

jcouv commented Mar 1, 2021 •

edited

Loading

alrz commented Mar 2, 2021 •

edited by jcouv

Loading

jcouv commented Mar 24, 2021 •

edited

Loading

333fred commented Apr 5, 2021 •

edited

Loading

alrz commented May 6, 2021 •

edited

Loading

alrz commented May 7, 2021 •

edited

Loading

alrz commented May 20, 2021 •

edited

Loading