List-patterns: factor binding logic#57318
List-patterns: factor binding logic#57318jcouv merged 33 commits intodotnet:features/list-patternsfrom
Conversation
4ab858d to
71f8d1d
Compare
7a6ea24 to
8aecf32
Compare
| 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); |
There was a problem hiding this comment.
📝 moved check to binding #Resolved
18a51cf to
3308071
Compare
3308071 to
025f398
Compare
|
@333fred @dotnet/roslyn-compiler for review. Thanks |
|
In the future, if you could make renames a separate commit, it would make review much easier. |
| ERR_MisplacedSlicePattern, | ||
| #endregion | ||
|
|
||
| #region diagnostics introduced for C# 11.0 |
There was a problem hiding this comment.
nit: newline was accidentally removed on line 1935 in this branch. #Closed
| indexerGroup.Free(); | ||
| goto done; | ||
| } | ||
| lookupResult.Clear(); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
| TypeSymbol receiverType = member.Receiver?.Type ?? inputType; | ||
| if (!receiverType.IsErrorType()) | ||
| { | ||
| bool ignoredHasErrors = false; |
There was a problem hiding this comment.
Maybe extract the third operand of ?: below to its own overload? #WontFix
|
@RikkiGibson @dotnet/roslyn-compiler for second review. Thanks |
| 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"> |
There was a problem hiding this comment.
"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".
There was a problem hiding this comment.
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)?
Isn't the "Pattern" confusing here as well? In reply to: 955046030 In reply to: 955046030 In reply to: 955046030 In reply to: 955046030 In reply to: 955046030 In reply to: 955046030 Refers to: src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml:2017 in b1112ce. [](commit_id = b1112ce, deletion_comment = False) |
Yeah, I agree it's a problem too. 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/Lowering/LocalRewriter/LocalRewriter.PatternLocalRewriter.cs
Show resolved
Hide resolved
| 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), |
| } | ||
|
|
||
| #nullable enable | ||
| internal static Symbol? GetIndexerOrImplicitIndexerSymbol(BoundExpression? e) |
| } | ||
|
|
||
| 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); |
| 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, |
| expectedFlowGraph, expectedDiagnostics, parseOptions: TestOptions.RegularWithExtendedPropertyPatterns); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Extra blank line below #Closed
src/Compilers/CSharp/Test/Semantic/Semantics/IndexAndRangeTests.cs
Outdated
Show resolved
Hide resolved
|
Done review pass (commit 29) |
| { | ||
| Debug.Assert(indexerAccess is not null); | ||
| sliceType = indexerAccess.Type; | ||
| _ = GetWellKnownTypeMember(WellKnownMember.System_Range__ctor, diagnostics, syntax: node); |
333fred
left a comment
There was a problem hiding this comment.
LGTM (commit 30) assuming CI is passing
|
Done with review pass (commit 30), it looks like there are legitimate test failures |
|
@jcouv When merging this PR, consider providing a comment that actually reflects the nature of the change. |
|
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"). |
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
CheckValueto properly deal with placeholders. This will allow us to store the Receiver separately from the IndexerOrSliceAccess and leverage existing placeholder infrastructure.FYI @alrz