List-patterns: factor binding logic #57318

jcouv · 2021-10-22T03:55:26Z

Relates to test plan #51289

This changes the bound tree representation of list patterns, slice patterns and implicit indexer accesses to avoid storing property or method symbols. Instead we store BoundIndexerAccess/BoundArrayAccess/BoundCall, which be obtain from existing "Bind*" methods and that we run through CheckValue. This ensures that we're not missing on all the validation rules which those include.
We intend to do some further refactoring, allowing CheckValue to properly deal with placeholders. This will allow us to store the Receiver separately from the IndexerOrSliceAccess and leverage existing placeholder infrastructure.

FYI @alrz

jcouv · 2021-10-23T19:16:36Z

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

@@ -707,8 +707,7 @@ private PatternSyntax ParseListPattern(bool whenIsKeyword)
                SyntaxKind.CloseBracketToken,
                static p => p.ParsePattern(Precedence.Conditional));
            TryParseSimpleDesignation(out VariableDesignationSyntax designation, whenIsKeyword);
-            var result = _syntaxFactory.ListPattern(openBracket, list, closeBracket, designation);
-            return CheckFeatureAvailability(result, MessageID.IDS_FeatureListPattern);


📝 moved check to binding #Resolved

jcouv · 2021-10-26T18:01:43Z

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

333fred · 2021-10-26T18:09:35Z

In the future, if you could make renames a separate commit, it would make review much easier.

alrz · 2021-10-22T06:54:50Z

src/Compilers/CSharp/Portable/Errors/ErrorCode.cs

-        ERR_MisplacedSlicePattern,
+        #endregion
+
+        #region diagnostics introduced for C# 11.0


nit: newline was accidentally removed on line 1935 in this branch. #Closed

alrz · 2021-10-26T18:07:38Z

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

@@ -406,26 +406,55 @@ private BoundExpression BindSwitchExpression(SwitchExpressionSyntax node, Bindin
                    }
                    analyzedArguments.Free();
                    indexerGroup.Free();
-                    goto done;
                }
                lookupResult.Clear();


Given the change, this is no longer needed. However I suspect this is changing the flow. If lookupResult.IsMultiViable is false we'd want to fallback to implicit indexer support? #Closed

Ah, you're right. We'll be able to observe the change in error scenarios once we expose the chosen symbols via IOperation. I'll revert this part

alrz · 2021-10-26T18:09:10Z

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

@@ -1456,9 +1485,10 @@ void addSubpatternsForTuple(ImmutableArray<TypeWithAnnotations> elementTypes)
                        TypeSymbol receiverType = member.Receiver?.Type ?? inputType;
                        if (!receiverType.IsErrorType())
                        {
+                            bool ignoredHasErrors = false;


Maybe extract the third operand of ?: below to its own overload? #WontFix

333fred

LGTM (commit 1)

jcouv · 2021-10-27T00:13:57Z

@RikkiGibson @dotnet/roslyn-compiler for second review. Thanks

…t-prototypes

AlekseyTs · 2021-10-29T20:56:14Z

src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml

  It does survive past initial binding but will be replaced with a synthesized argument in DAG lowering.
  -->
-  <Node Name="BoundIndexOrRangeIndexerPatternValuePlaceholder" Base="BoundValuePlaceholderBase">
+  <Node Name="BoundIndexOrRangeIndexerFallbackValuePlaceholder" Base="BoundValuePlaceholderBase">


Fallback

What is the "Fallback" supposed to convey. Do we need to have it in the name? #Closed

"Fallback" is how we've been referring to such members in LDM. This is the fallback when this[Index] or this[Range] don't exist.
Looking at the ranges spec I'd also be fine with "ImplicitIndexer".

I think "ImplicitIndexer" is better. Also, the "ValuePlaceholder" feels confusing. Does the node represent an argument for the indexer or the value of the indexer (i.e. result of its evaluation)?

src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml

AlekseyTs · 2021-10-29T21:02:42Z

<Field Name="PatternSymbol" Type="Symbol" Null="disallow" />

Isn't the "Pattern" confusing here as well?

In reply to: 955046030

Refers to: src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml:2017 in b1112ce. [](commit_id = b1112ce, deletion_comment = False)

jcouv · 2021-10-29T21:13:39Z

<Field Name="PatternSymbol" Type="Symbol" Null="disallow" />

Yeah, I agree it's a problem too.
How about "BoundIndexOrRangeImplicitIndexerAccess" for the node and "UnderlyingSymbol" for the member (could be an indexer or a method)?

In reply to: 955046030

Refers to: src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml:2017 in b1112ce. [](commit_id = b1112ce, deletion_comment = False)

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

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

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter.PatternLocalRewriter.cs

src/Compilers/CSharp/Portable/Syntax/SyntaxKind.cs

AlekseyTs · 2021-11-19T14:30:46Z

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

@@ -39,8 +39,8 @@ public virtual bool IsEquivalentTo(BoundDagEvaluation other)
                    BoundDagTypeEvaluation e => e.Type,
                    BoundDagDeconstructEvaluation e => e.DeconstructMethod,
                    BoundDagIndexEvaluation e => e.Property,
-                    BoundDagSliceEvaluation e => (Symbol?)e.SliceMethod ?? e.IndexerAccess?.Indexer,
-                    BoundDagIndexerEvaluation e => e.IndexerSymbol ?? e.IndexerAccess?.Indexer,
+                    BoundDagSliceEvaluation e => Binder.GetIndexerOrImplicitIndexerSymbol(e.IndexerAccess),


Binder.GetIndexerOrImplicitIndexerSymbol(e.IndexerAccess)

It looks like these two are going to return null for a BoundArrayAccess. It doesn't look like that is going to help IsEquivalentTo implementation and similar usages. #Closed

AlekseyTs · 2021-11-19T14:34:32Z

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

@@ -1593,6 +1598,26 @@ private static PropertySymbol GetPropertySymbol(BoundExpression expr, out BoundE
            return propertySymbol;
        }

+#nullable enable
+        internal static Symbol? GetIndexerOrImplicitIndexerSymbol(BoundExpression? e)


GetIndexerOrImplicitIndexerSymbol

It doesn't look like this method handles all inputs that BoundListPattern/BoundSlicePattern can use in place of IndexerAccess. I think this is the root cause of the IOperation leg failure. #Closed

AlekseyTs · 2021-11-19T14:39:01Z

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

                }

                pattern = BindPattern(node.Pattern, sliceType, GetValEscape(sliceType, inputValEscape), permitDesignations, hasErrors, diagnostics);
            }

-            return new BoundSlicePattern(node, pattern, indexerAccess, sliceMethod, inputType: inputType, narrowedType: inputType, hasErrors);
+            return new BoundSlicePattern(node, pattern, indexerAccess, receiverPlaceholder, argumentPlaceholder, inputType: inputType, narrowedType: inputType, hasErrors);


indexerAccess

Consider asserting that GetIndexerOrImplicitIndexerSymbol can handle this as an input. #Closed

AlekseyTs · 2021-11-19T14:40:31Z

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

-                syntax: node, subpatterns: subpatterns, hasSlice: sawSlice, lengthProperty: lengthProperty,
-                indexerAccess: indexerAccess, indexerSymbol: indexerSymbol, variable: variableSymbol,
+                syntax: node, subpatterns: subpatterns, hasSlice: sawSlice, lengthAccess: lengthAccess,
+                indexerAccess: indexerAccess, receiverPlaceholder, argumentPlaceholder, variable: variableSymbol,


indexerAccess

Consider asserting that GetIndexerOrImplicitIndexerSymbol can handle this as an input. #Closed

333fred · 2021-11-19T18:38:11Z

src/Compilers/CSharp/Test/IOperation/IOperation/IOperationTests_IIsPatternExpression.cs

@@ -2450,7 +2450,9 @@ void M(int[] o)

            var expectedDiagnostics = DiagnosticDescription.None;

-            VerifyFlowGraphAndDiagnosticsForTest<BlockSyntax>(source, expectedFlowGraph, expectedDiagnostics, parseOptions: TestOptions.RegularWithExtendedPropertyPatterns);
+            VerifyFlowGraphAndDiagnosticsForTest<BlockSyntax>(new[] { source, TestSources.Index, TestSources.Range, TestSources.GetSubArray },
+                expectedFlowGraph, expectedDiagnostics, parseOptions: TestOptions.RegularWithExtendedPropertyPatterns);
        }
    }
 }


Extra blank line below #Closed

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

333fred · 2021-11-19T19:48:21Z

Done review pass (commit 29)

AlekseyTs · 2021-11-19T21:08:11Z

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

                    {
-                        Debug.Assert(indexerAccess is not null);
-                        sliceType = indexerAccess.Type;
+                        _ = GetWellKnownTypeMember(WellKnownMember.System_Range__ctor, diagnostics, syntax: node);


_ = GetWellKnownTypeMember(WellKnownMember.System_Range__ctor, diagnostics, syntax: node);

I guess it is fine to check this, however it looks like lowering handles error reporting for this.

333fred

LGTM (commit 30) assuming CI is passing

AlekseyTs · 2021-11-19T21:23:00Z

Done with review pass (commit 30), it looks like there are legitimate test failures

AlekseyTs

LGTM (commit 32)

AlekseyTs · 2021-11-20T02:56:18Z

@jcouv When merging this PR, consider providing a comment that actually reflects the nature of the change.

jcouv · 2021-11-21T06:46:24Z

Updated the PR title and the description for the squashed commit. Also made the name update we'd discussed earlier in the PR (using "ImplicitIndexer" and "IndexerOrSliceAccess").
Thanks @AlekseyTs for all the help and @333fred for the review.

jcouv added Area-Compilers Feature - List Patterns labels Oct 22, 2021

jcouv self-assigned this Oct 22, 2021

jcouv force-pushed the list-prototypes branch from 4ab858d to 71f8d1d Compare October 22, 2021 04:47

This was referenced Oct 22, 2021

Integration tests fail to wait for async light bulb #57326

Closed

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 force-pushed the list-prototypes branch 2 times, most recently from 7a6ea24 to 8aecf32 Compare October 23, 2021 19:13

jcouv commented Oct 23, 2021

View reviewed changes

jcouv force-pushed the list-prototypes branch 4 times, most recently from 18a51cf to 3308071 Compare October 24, 2021 22:03

Address some PROTOTYPE markers

025f398

jcouv force-pushed the list-prototypes branch from 3308071 to 025f398 Compare October 25, 2021 07:13

jcouv marked this pull request as ready for review October 25, 2021 07:20

jcouv requested a review from a team as a code owner October 25, 2021 07:20

runfoapp bot mentioned this pull request Oct 25, 2021

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

Open

alrz reviewed Oct 26, 2021

View reviewed changes

333fred approved these changes Oct 26, 2021

View reviewed changes

Address feedback

00e6ff2

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

b1112ce

…t-prototypes

AlekseyTs reviewed Oct 29, 2021

View reviewed changes

src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml Outdated Show resolved Hide resolved