List-patterns: add a few tests and add HasValidate flag #57914

jcouv · 2021-11-22T07:16:54Z

Additional validation:

Built runtime repo (using instructions)

Test plan: #51289

alrz · 2021-11-22T15:17:20Z

src/Tools/Source/CompilerGeneratorTools/Source/BoundTreeGenerator/BoundNodeClassWriter.cs

+
+                        if (hasValidate)
+                        {
+                            WriteLine("private partial void Validate();");


💡 Would be nice to add something like [Conditional("DEBUG")] partial void Validate(); for all nodes

I agree, it feels like the design of partial methods is such that we shouldn't need to introduce new flags to BoundNodes.xml or branches in this logic. Just unconditionally emit the declaration part and the call, then trust the compiler to eliminate the methods and calls to methods that lack implementations.

That said, it's fine with me if the desire is to use a flag in the xml to reduce clutter in the PR.

AlekseyTs · 2021-11-23T19:24:26Z

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

@@ -238,7 +238,6 @@ private BoundExpression BindSwitchExpression(SwitchExpressionSyntax node, Bindin
                pattern = BindPattern(node.Pattern, sliceType, GetValEscape(sliceType, inputValEscape), permitDesignations, hasErrors, diagnostics);
            }

-            Debug.Assert(GetIndexerOrImplicitIndexerSymbol(indexerAccess) is var _);


GetIndexerOrImplicitIndexerSymbol

~~I think we should keep this assert, unless we are removing this method or changing how it is used.~~ I see that this line got moved to Validate method. #Closed

AlekseyTs · 2021-11-23T19:24:52Z

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

@@ -317,7 +316,6 @@ private BoundExpression BindSwitchExpression(SwitchExpressionSyntax node, Bindin
                inputValEscape, permitDesignations, typeSyntax: null, diagnostics, ref hasErrors,
                out Symbol? variableSymbol, out BoundExpression? variableAccess);

-            Debug.Assert(GetIndexerOrImplicitIndexerSymbol(indexerAccess) is var _);


GetIndexerOrImplicitIndexerSymbol

Same comment here as well. #Closed

AlekseyTs · 2021-11-23T19:27:52Z

src/Compilers/CSharp/Portable/BoundTree/BoundDagEvaluation.cs

@@ -108,6 +108,10 @@ public override bool IsEquivalentTo(BoundDagEvaluation obj)
            return base.IsEquivalentTo(obj) &&
                this.Index == ((BoundDagIndexerEvaluation)obj).Index;
        }
+        private partial void Validate()


private partial void Validate()

Consider adding some space between declarations. #Closed

AlekseyTs · 2021-11-23T19:29:51Z

src/Compilers/CSharp/Portable/BoundTree/BoundImplicitIndexerAccess.cs

+
+        private partial void Validate()
+        {
+            Debug.Assert(LengthOrCountAccess is BoundPropertyAccess or BoundLocal);


BoundPropertyAccess or BoundLocal

It looks like CheckValue can return a BoundBadExpression. We might want to be prepared to deal with that. #Closed

AlekseyTs · 2021-11-23T19:43:59Z

src/Compilers/CSharp/Portable/BoundTree/BoundImplicitIndexerAccess.cs

+        private partial void Validate()
+        {
+            Debug.Assert(LengthOrCountAccess is BoundPropertyAccess or BoundLocal);
+            Debug.Assert(IndexerOrSliceAccess is BoundIndexerAccess or BoundCall);


IndexerOrSliceAccess is BoundIndexerAccess or BoundCall

This is going to "conflict" with the array PR (#57918). Consider adding BoundArrayAccess into the list. #Closed

AlekseyTs · 2021-11-23T19:47:11Z

                Debug.Assert(indexerAccess is BoundIndexerAccess or BoundImplicitIndexerAccess or BoundArrayAccess or BoundBadExpression or BoundDynamicIndexerAccess);

I think this line should move to Validate.

In reply to: 977085556

Refers to: src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs:226 in 3ed14b9. [](commit_id = 3ed14b9, deletion_comment = False)

AlekseyTs · 2021-11-23T19:50:48Z

        Debug.Assert(indexerAccess is BoundIndexerAccess or BoundImplicitIndexerAccess or BoundArrayAccess or BoundBadExpression or BoundDynamicIndexerAccess);

I think this line should move to Validate.

In reply to: 977090678

Refers to: src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs:372 in 3ed14b9. [](commit_id = 3ed14b9, deletion_comment = False)

AlekseyTs · 2021-11-23T19:57:23Z

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

-        var csCompilation = CreateCompilation(csSource, parseOptions: TestOptions.RegularWithListPatterns, references: new[] { vbCompilation.EmitToImageReference() });
-        // PROTOTYPE(list-patterns) Unsupported because the lookup fails not that the indexer is static
+        var csCompilation = CreateCompilation(csSource, references: new[] { vbCompilation.EmitToImageReference() });
+        // Note: the VB indexer's name is "Item" but C# looks for "this[]", but we can't put Default on Shared indexer declaration


but we can't put Default on Shared indexer declaration

Can we do that in IL? Is it worth testing a property like that? #Closed

We have a test with a static indexer from IL, see SlicePattern_LengthAndIndexAndSliceAreStatic

AlekseyTs · 2021-11-23T19:59:04Z

Public Shared ReadOnly Property Item(i As System.Index) As Integer

Are we testing scenarios with an instance property like that (expected name but not Default member)?
Are we testing scenarios with Default property named differently?

In reply to: 977102228 #Closed

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/PatternMatchingTests_ListPatterns.cs:1396 in 3ed14b9. [](commit_id = 3ed14b9, deletion_comment = False)

jcouv · 2021-11-23T21:24:14Z

Public Shared ReadOnly Property Item(i As System.Index) As Integer

I don't know how to do the first suggestion (C# looks for this[] as the name)

In reply to: 977102228

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/PatternMatchingTests_ListPatterns.cs:1396 in 3ed14b9. [](commit_id = 3ed14b9, deletion_comment = False)

AlekseyTs · 2021-11-23T21:54:35Z

src/Compilers/CSharp/Portable/BoundTree/BoundImplicitIndexerAccess.cs

+        private partial void Validate()
+        {
+            Debug.Assert(LengthOrCountAccess is BoundPropertyAccess or BoundLocal or BoundBadExpression);
+            Debug.Assert(IndexerOrSliceAccess is BoundIndexerAccess or BoundCall or BoundArrayAccess or BoundBadExpression);


or BoundBadExpression

The addition of a BoundBadExpression for IndexerOrSliceAccess feels unexpected. We don't call CheckValue on it and we don't handle BoundBadExpression in various helpers. For example in GetReceiver above. #Closed

AlekseyTs · 2021-11-23T21:58:18Z

src/Compilers/CSharp/Portable/BoundTree/BoundListPattern.cs

+        private partial void Validate()
+        {
+            Debug.Assert(LengthAccess is null or BoundPropertyAccess or BoundBadExpression);
+            Debug.Assert(IndexerAccess is BoundIndexerAccess or BoundImplicitIndexerAccess or BoundArrayAccess or BoundBadExpression or BoundDynamicIndexerAccess);


IndexerAccess

Can it be null? #Closed

AlekseyTs · 2021-11-23T21:58:45Z

src/Compilers/CSharp/Portable/BoundTree/BoundSlicePattern.cs

+    {
+        private partial void Validate()
+        {
+            Debug.Assert(IndexerAccess is BoundIndexerAccess or BoundImplicitIndexerAccess or BoundArrayAccess or BoundBadExpression or BoundDynamicIndexerAccess);


Can it be null? #Closed

AlekseyTs · 2021-11-23T22:15:49Z

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

+switch (a)
+{
+    case [..[1],2,3]:
+    case [1,2,3]: // no error


case [1,2,3]: // no error

Why is there a behavior difference? #Closed

Because we inject a null test for reference types (int[] vs. Span<int>) that we get back from the slice operation. We're having an email thread with LDM on this.

AlekseyTs · 2021-11-23T22:17:18Z

Done with review pass (commit 9)

AlekseyTs

LGTM (commit 10)

…t-plan

RikkiGibson · 2022-03-02T01:22:39Z

eng/test-build-correctness.ps1

@@ -53,6 +53,14 @@ try {
    }
  }

+  # Verify no TODO2 marker left


@jcouv is this something we expect to be useful on an ongoing basis? should we do something like flip the existing TODO comments over to TODO2 and change this to prevent introduction of new TODO comments to main?

I don't think we can do much about existing TODO comments. I've been using TODO2 comments to track things within a PR and having this helps me ensure I don't let any into the main branch or feature branches.

jcouv added 3 commits November 21, 2021 00:17

Add HasValidate

e033f3b

Test plan

ea9f5d6

Enforce TODO2 marker in CI

36b4917

jcouv self-assigned this Nov 22, 2021

dotnet-issue-labeler bot added the Area-Compilers label Nov 22, 2021

runfoapp bot mentioned this pull request Nov 22, 2021

Test failure in Microsoft.CodeAnalysis.CSharp.Emit.UnitTests.dll #57560

Closed

alrz reviewed Nov 22, 2021

View reviewed changes

tweaks

9695eae

jcouv added the Feature - List Patterns label Nov 22, 2021

jcouv marked this pull request as ready for review November 22, 2021 20:17

jcouv requested review from a team as code owners November 22, 2021 20:17

tweak

5eeebe4

jcouv requested a review from AlekseyTs November 23, 2021 18:30

Add conditional attribute

3ed14b9

AlekseyTs reviewed Nov 23, 2021

View reviewed changes

runfoapp bot mentioned this pull request Nov 23, 2021

Timeout on Microsoft.VisualStudio.LanguageServices.CSharp.UnitTests.dll tests #55638

Open

Address feedback

b6d1139

Add BoundBadExpression case for safety

4398672

jcouv requested a review from AlekseyTs November 23, 2021 21:27

Test with non-default indexer named Item

9135c4a

AlekseyTs reviewed Nov 23, 2021

View reviewed changes

Remove BoundBadExpression

88e4503

AlekseyTs approved these changes Nov 23, 2021

View reviewed changes

RikkiGibson approved these changes Nov 24, 2021

View reviewed changes

jcouv added 2 commits November 23, 2021 16:22

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

ccb7dea

…t-plan

Merge with array change

92622a0

jcouv enabled auto-merge (squash) November 24, 2021 00:30

jcouv merged commit 0660d83 into dotnet:features/list-patterns Nov 24, 2021

runfoapp bot mentioned this pull request Nov 24, 2021

[Flaky Test] VisualBasic.BasicAddMissingReference.InvokeSomeFixesInVisualBasicThenVerifyReferences #57980

Closed

commit-lint bot mentioned this pull request Dec 9, 2021

[pull] main from dotnet:main Blaquez-home/roslyn#2

Open

jcouv deleted the test-plan branch February 17, 2022 05:07

RikkiGibson reviewed Mar 2, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

List-patterns: add a few tests and add HasValidate flag #57914

List-patterns: add a few tests and add HasValidate flag #57914

jcouv commented Nov 22, 2021 •

edited

Loading

alrz Nov 22, 2021 •

edited

Loading

RikkiGibson Nov 24, 2021 •

edited

Loading

AlekseyTs Nov 23, 2021 •

edited

Loading

AlekseyTs Nov 23, 2021 •

edited

Loading

AlekseyTs Nov 23, 2021 •

edited

Loading

AlekseyTs Nov 23, 2021 •

edited

Loading

AlekseyTs Nov 23, 2021 •

edited

Loading

AlekseyTs commented Nov 23, 2021 •

edited by jcouv

Loading

AlekseyTs commented Nov 23, 2021 •

edited by jcouv

Loading

AlekseyTs Nov 23, 2021 •

edited

Loading

jcouv Nov 23, 2021

AlekseyTs commented Nov 23, 2021 •

edited

Loading

jcouv commented Nov 23, 2021

AlekseyTs Nov 23, 2021 •

edited

Loading

AlekseyTs Nov 23, 2021 •

edited

Loading

AlekseyTs Nov 23, 2021 •

edited

Loading

AlekseyTs Nov 23, 2021 •

edited

Loading

jcouv Nov 23, 2021

AlekseyTs commented Nov 23, 2021

AlekseyTs left a comment

RikkiGibson Mar 2, 2022

jcouv Mar 2, 2022

List-patterns: add a few tests and add HasValidate flag #57914

List-patterns: add a few tests and add HasValidate flag #57914

Conversation

jcouv commented Nov 22, 2021 • edited Loading

alrz Nov 22, 2021 • edited Loading

Choose a reason for hiding this comment

RikkiGibson Nov 24, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 23, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 23, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 23, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 23, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 23, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Nov 23, 2021 • edited by jcouv Loading

AlekseyTs commented Nov 23, 2021 • edited by jcouv Loading

AlekseyTs Nov 23, 2021 • edited Loading

Choose a reason for hiding this comment

jcouv Nov 23, 2021

Choose a reason for hiding this comment

AlekseyTs commented Nov 23, 2021 • edited Loading

jcouv commented Nov 23, 2021

AlekseyTs Nov 23, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 23, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 23, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 23, 2021 • edited Loading

Choose a reason for hiding this comment

jcouv Nov 23, 2021

Choose a reason for hiding this comment

AlekseyTs commented Nov 23, 2021

AlekseyTs left a comment

Choose a reason for hiding this comment

RikkiGibson Mar 2, 2022

Choose a reason for hiding this comment

jcouv Mar 2, 2022

Choose a reason for hiding this comment

jcouv commented Nov 22, 2021 •

edited

Loading

alrz Nov 22, 2021 •

edited

Loading

RikkiGibson Nov 24, 2021 •

edited

Loading

AlekseyTs Nov 23, 2021 •

edited

Loading

AlekseyTs Nov 23, 2021 •

edited

Loading

AlekseyTs Nov 23, 2021 •

edited

Loading

AlekseyTs Nov 23, 2021 •

edited

Loading

AlekseyTs Nov 23, 2021 •

edited

Loading

AlekseyTs commented Nov 23, 2021 •

edited by jcouv

Loading

AlekseyTs commented Nov 23, 2021 •

edited by jcouv

Loading

AlekseyTs Nov 23, 2021 •

edited

Loading

AlekseyTs commented Nov 23, 2021 •

edited

Loading

AlekseyTs Nov 23, 2021 •

edited

Loading

AlekseyTs Nov 23, 2021 •

edited

Loading

AlekseyTs Nov 23, 2021 •

edited

Loading

AlekseyTs Nov 23, 2021 •

edited

Loading