List patterns: add more tests and fix nullability analysis #53822

jcouv · 2021-06-01T23:37:33Z

Test plan: #51289

Note: the nullability analysis in this PR does not handle exhaustiveness analysis with aliasing of start/end or nested indexes

alrz · 2021-06-05T05:31:06Z

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

@@ -1867,5 +1868,1811 @@ void verify(PropertyPatternClauseSyntax clause, string syntax, string type)
 Assert.Equal(type, typeInfo.ConvertedType.ToDisplayString());
 }
 }
+
+ [Fact]
+ public void SlicePattern_Exhaustiveness()


This is not testing slice. Did you mean LengthPattern_Exhaustiveness #Resolved

alrz · 2021-06-05T05:35:54Z

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

+ Diagnostic(ErrorCode.ERR_NoTypeDef, "c[1..^1]").WithArguments("Missing", "missing, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null").WithLocation(9, 21)
+ );
+
+


new line #Resolved

alrz · 2021-06-05T05:41:18Z

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

+ public int this[int i] => throw null;
+ public int Property => throw null;
+}";
+ // PROTOTYPE bad explanation


📝 I'll have a PR out for this next (I'd probably hold that off until after LDM) #Resolved

alrz · 2021-06-05T05:46:55Z

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

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


Maybe also test for new C<Index>() is { 0 } with class C<T> { this[T] } (should work) #Resolved

Good idea. It looks like there may be a bug with Index/Range, and the same one for list patterns. See ListPattern_GenericIndexingParameter below.

alrz · 2021-06-05T05:48:23Z

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

+ {
+ if (new C() is [var length])
+ length.ToString();
+ _ = new C<int>()[^1];


I don't think this is intended to be here. #Resolved

alrz · 2021-06-05T06:12:33Z

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

+ public int Count => throw null;
+ public int this[int i] => throw null;
+}";
+ // PROTOTYPE should not warn, because we've covered all possibilities for first position in single item list and we don't care about lists of other lengths.


Slice could return an arbitrary type - as far as the analysis goes, the input type of the subpattern is "unrelated" to the list. #Resolved

Right. I think there's still a question of whether we have some reasonable expectations when a slice returns an indexable/rangeable type. Do we just give up on analysis or do we have some notion of what is "well-behaved"?

We only do this for tests when the input is exactly the same. In #53891 I had to explicitly add a special case when there's two different but "matching" indexers as the input which is a little unorthodox.

As for slice subpatterns, it wouldn't be that simple. We should match nested subpattern with the outer list. I didn't go through it but I'd expect it's going to be a lot of work which is arguably unlikely to be useful in real code.

I'd expect it's going to be a lot of work which is arguably unlikely to be useful in real code.

I agree. I'd be comfortable dropping the slice analysis. Just need to confirm with LDM.

alrz · 2021-06-05T06:14:42Z

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

+
+class C
+{
+ public nint Count => throw null!;


📝 We possibly can support this if we relax range indexers too. #Resolved

alrz · 2021-06-05T06:20:02Z

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

+ public string? Property => throw null!;
+}";
+ // PROTOTYPE bad explanations on 1 and 2
+ // Note: it's a bit annoying that we don't assume a positive Count here, or allow a `uint` Count


Assuming positive count in the codegen or for subsumption? e.g. Is the match e is [-1] impossible? of course we don't need to worry about this if we drop length patterns.. #Resolved

LDM agreed to make such assumptions for the length/list pattern. But not generally for property patterns such as Count: <= 0 above.
Not having a good syntax for empty list means causing more pain if I fall back to Count property pattern.

alrz · 2021-06-05T06:21:14Z

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

+ public int Count => throw null;
+ public int this[int i] => throw null;
+}";
+ // PROTOTYPE should not warn, because we've covered all possibilities for first position in single item list and we don't care about lists of other lengths.


📝 Will be addressed by #53891 #Resolved

Very cool. Thanks!
Ping when you want some eyes on that PR.

alrz · 2021-06-05T06:34:28Z

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

+public class C
+{
+ public int Length => 2;
+ public string this[in System.Index i] => ""item value"";


I think we should also test for [ImplicitlyConvertibleFromIndex i] (should work) #Resolved

Good idea. Added ListPattern_ImplicitlyConvertibleFromIndexAndRange which hit an assertion.

alrz · 2021-06-05T17:14:21Z

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

+ }
+}
+";
+ // PROTOTYPE errors 1 and 2 are unexpected


This is expected. we require exactly this[int] as the member for implicit support (vs. only a viable lookup for Index). #Resolved

jcouv · 2021-07-13T19:26:05Z

Rebased the change on top of syntax change PR which is now merged.

…t-tests

jcouv · 2021-10-04T16:55:00Z

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

333fred

Done review pass (commit 8)

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker_Patterns.cs

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

…t-tests

333fred

LGTM (commit 9)

jcouv · 2021-10-06T21:05:14Z

@dotnet/roslyn-compiler for second review. Thanks

RikkiGibson · 2021-10-08T23:42:20Z

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

 tests.Add(new Tests.One(indexEvaluation));
- var indexTemp = new BoundDagTemp(syntax, subpattern.InputType, indexEvaluation);
+ var indexTemp = new BoundDagTemp(subpattern.Syntax, subpattern.InputType, indexEvaluation);


Perhaps we should open a tracking issue to make an analogous change for the scenarios @alrz is referring to.

RikkiGibson · 2021-10-09T00:01:13Z

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

+ verify(declarations[0], "item", "System.Int32");
+ verify(declarations[1], "item2", "System.Int32?");
+ verify(declarations[2], "item3", "System.String?");
+ verify(declarations[3], "item4", "System.String?");


nit: assert on declarations.Length? (same comment in the other similar new tests) #Resolved

RikkiGibson · 2021-10-09T00:03:15Z

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

+ }
+}
+";
+ // PROTOTYPE: incorrect exhaustiveness examples from explainer


nit: prefer PROTOTYPE(list-patterns) (also for other prototype comments in the file.)

FWIW, I don't see shortcomings in the pattern explainer as blocking.

Since we disallow the "PROTOTYPE" keyword in main branch, I figured the longer format doesn't help much (and it's more trouble to type). I'll stick with that if that's okay.

jcouv added Area-Compilers PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. New Feature - List Patterns labels Jun 1, 2021

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

jcouv self-assigned this Jun 1, 2021

jcouv force-pushed the list-tests branch from 465870b to 4667a92 Compare June 3, 2021 15:03

jcouv changed the base branch from features/extended-property-patterns to features/list-patterns June 3, 2021 15:14

jcouv force-pushed the list-tests branch 2 times, most recently from 0e1f442 to 232d033 Compare June 5, 2021 02:57

jcouv marked this pull request as ready for review June 5, 2021 03:00

jcouv requested a review from a team as a code owner June 5, 2021 03:00

jcouv requested a review from alrz June 5, 2021 03:00

jcouv removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Jun 5, 2021

alrz reviewed Jun 5, 2021

View reviewed changes

jcouv requested a review from alrz June 5, 2021 16:23

alrz reviewed Jun 5, 2021

View reviewed changes

jcouv marked this pull request as draft June 24, 2021 00:55

jcouv added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Jun 24, 2021

jcouv mentioned this pull request Jun 25, 2021

List patterns: change syntax to use square brackets #54335

Merged

jcouv force-pushed the list-tests branch from 88f347e to 765f937 Compare June 28, 2021 23:25

jaredpar modified the milestones: C# 10, 17.0 Jul 13, 2021

List patterns: add more tests and fix nullability analysis

2d59c69

jcouv force-pushed the list-tests branch from 765f937 to 2d59c69 Compare July 13, 2021 19:06

jcouv removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Jul 13, 2021

jcouv marked this pull request as ready for review July 13, 2021 19:25

jcouv marked this pull request as draft July 13, 2021 20:11

tweak

87455e0

jcouv marked this pull request as ready for review July 14, 2021 15:53

jcouv mentioned this pull request Aug 6, 2021

List patterns: subsumption checking #53891

Merged

jcouv added 2 commits September 16, 2021 23:25

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

2168d8d

…t-tests

tweak

918da0f

jcouv requested a review from 333fred September 17, 2021 06:31

jcouv modified the milestones: 17.0, 17.1 Sep 17, 2021

jcouv added 2 commits September 17, 2021 09:09

tweaks

dc204ab

nit

1761bed

jcouv mentioned this pull request Sep 18, 2021

Test Plan for "List Patterns" #51289

Closed

91 tasks

333fred reviewed Oct 5, 2021

View reviewed changes

jcouv added 3 commits October 5, 2021 21:26

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

d0819f4

…t-tests

fix formatting in tests

dbfc0ed

Address feedback

3b28d40

jcouv requested a review from 333fred October 6, 2021 06:08

333fred approved these changes Oct 6, 2021

View reviewed changes

RikkiGibson approved these changes Oct 9, 2021

View reviewed changes

Assertions

866346a

jcouv enabled auto-merge (squash) October 11, 2021 05:39

jcouv merged commit d7c97c6 into dotnet:features/list-patterns Oct 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

List patterns: add more tests and fix nullability analysis #53822

List patterns: add more tests and fix nullability analysis #53822

jcouv commented Jun 1, 2021 •

edited

Loading

alrz Jun 5, 2021 •

edited by jcouv

Loading

alrz Jun 5, 2021 •

edited by jcouv

Loading

alrz Jun 5, 2021 •

edited by jcouv

Loading

alrz Jun 5, 2021 •

edited by jcouv

Loading

jcouv Jun 5, 2021

alrz Jun 5, 2021 •

edited by jcouv

Loading

alrz Jun 5, 2021 •

edited by jcouv

Loading

jcouv Jun 5, 2021

alrz Jun 5, 2021

jcouv Jun 5, 2021

alrz Jun 5, 2021 •

edited by jcouv

Loading

alrz Jun 5, 2021 •

edited by jcouv

Loading

jcouv Jun 5, 2021

alrz Jun 5, 2021 •

edited by jcouv

Loading

jcouv Jun 5, 2021

alrz Jun 5, 2021 •

edited by jcouv

Loading

jcouv Jun 5, 2021

alrz Jun 5, 2021 •

edited by jcouv

Loading

jcouv commented Jul 13, 2021

jcouv commented Oct 4, 2021

333fred left a comment

333fred left a comment

jcouv commented Oct 6, 2021

RikkiGibson Oct 8, 2021

RikkiGibson Oct 9, 2021 •

edited by jcouv

Loading

RikkiGibson Oct 9, 2021

jcouv Oct 11, 2021

		Diagnostic(ErrorCode.ERR_NoTypeDef, "c[1..^1]").WithArguments("Missing", "missing, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null").WithLocation(9, 21)
		);

List patterns: add more tests and fix nullability analysis #53822

List patterns: add more tests and fix nullability analysis #53822

Conversation

jcouv commented Jun 1, 2021 • edited Loading

alrz Jun 5, 2021 • edited by jcouv Loading

Choose a reason for hiding this comment

alrz Jun 5, 2021 • edited by jcouv Loading

Choose a reason for hiding this comment

alrz Jun 5, 2021 • edited by jcouv Loading

Choose a reason for hiding this comment

alrz Jun 5, 2021 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alrz Jun 5, 2021 • edited by jcouv Loading

Choose a reason for hiding this comment

alrz Jun 5, 2021 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alrz Jun 5, 2021 • edited by jcouv Loading

Choose a reason for hiding this comment

alrz Jun 5, 2021 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alrz Jun 5, 2021 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alrz Jun 5, 2021 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alrz Jun 5, 2021 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv commented Jul 13, 2021

jcouv commented Oct 4, 2021

333fred left a comment

Choose a reason for hiding this comment

333fred left a comment

Choose a reason for hiding this comment

jcouv commented Oct 6, 2021

Choose a reason for hiding this comment

RikkiGibson Oct 9, 2021 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv commented Jun 1, 2021 •

edited

Loading

alrz Jun 5, 2021 •

edited by jcouv

Loading

alrz Jun 5, 2021 •

edited by jcouv

Loading

alrz Jun 5, 2021 •

edited by jcouv

Loading

alrz Jun 5, 2021 •

edited by jcouv

Loading

alrz Jun 5, 2021 •

edited by jcouv

Loading

alrz Jun 5, 2021 •

edited by jcouv

Loading

alrz Jun 5, 2021 •

edited by jcouv

Loading

alrz Jun 5, 2021 •

edited by jcouv

Loading

alrz Jun 5, 2021 •

edited by jcouv

Loading

alrz Jun 5, 2021 •

edited by jcouv

Loading

alrz Jun 5, 2021 •

edited by jcouv

Loading

RikkiGibson Oct 9, 2021 •

edited by jcouv

Loading