List patterns: binding and lowering#53299
List patterns: binding and lowering#53299jcouv merged 43 commits intodotnet:features/list-patternsfrom
Conversation
This comment has been minimized.
This comment has been minimized.
src/Compilers/CSharp/Test/Semantic/Semantics/PatternMatchingTests_ListPatterns.cs
Show resolved
Hide resolved
src/Compilers/CSharp/Test/Semantic/Semantics/PatternMatchingTests_ListPatterns.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Test/Semantic/Semantics/PatternMatchingTests_ListPatterns.cs
Outdated
Show resolved
Hide resolved
jcouv
left a comment
There was a problem hiding this comment.
Done with review pass (iteration 41)
| if (clause.HasSlice && | ||
| subpatterns.Length == 1 && | ||
| subpatterns[0] is BoundSlicePattern { Pattern: null }) | ||
| { | ||
| // If `..` is the only pattern in the list, bail. This is a no-op and we don't need to match anything further. | ||
| // If there's a subpattern, we're going to need the length value to extract a slice for the input, | ||
| // in which case we will test the length even if there is no other patterns in the list. | ||
| // i.e. the length is required to be non-negative for the match to be correct. | ||
| return; | ||
| } |
There was a problem hiding this comment.
With this change, there's an observable difference between .. and .._. In either case Slice is not emitted but we require a non-negative length for the latter. This is the same as doing {_} or (_, _) with ITuple, any discard added affects the required length but the indexer call is not emitted for either of subpatterns.
Thoughts? #Resolved
There was a problem hiding this comment.
@333fred may have a different opinion. We could add null or Discard above to have both behave the same but at the moment we never treat discards as if they don't exist. Leaving this up to you. Thanks.
|
|
||
| if (clause.HasSlice && | ||
| subpatterns.Length == 1 && | ||
| subpatterns[0] is BoundSlicePattern { Pattern: null }) |
There was a problem hiding this comment.
What about a slice with no subpattern in the middle or tail of a list pattern? Do we need to skip emitting a test for those too?
Example: { 1, .., 2 }, { 1, .. } #Closed
There was a problem hiding this comment.
That does not require a Slice and therefore nothing to emit.
As for the length, it's already handled below as usual.
jcouv
left a comment
There was a problem hiding this comment.
Done with review pass (iteration 42).
FYI Monday is a holiday so there may be delay for second review.
| { | ||
| patternSymbol = null; | ||
| var lookupResult = LookupResult.GetInstance(); | ||
| CompoundUseSiteInfo<AssemblySymbol> useSiteInfo = GetNewCompoundUseSiteInfo(diagnostics); |
There was a problem hiding this comment.
I think there's still a problem with use-site errors. See tests below. Can be PROTOTYPE'd.
[Fact]
public void ListPattern_UseSiteErrorOnIndexerAndSlice()
{
var missing_cs = @"
public class Missing
{
}
";
var missingRef = CreateCompilation(missing_cs, assemblyName: "missing")
.EmitToImageReference();
var lib2_cs = @"
public class C
{
public int Length => 0;
public Missing this[int i] => throw null;
public Missing Slice(int i, int j) => throw null;
}
";
var lib2Ref = CreateCompilation(lib2_cs, references: new[] { missingRef })
.EmitToImageReference();
var source = @"
class D
{
void M(C c)
{
_ = c is { var item };
_ = c is { ..var rest };
var index = c[^1];
var range = c[1..^1];
}
}
";
var compilation = CreateCompilation(new[] { source, TestSources.Index, TestSources.Range },
references: new[] { lib2Ref }, parseOptions: TestOptions.RegularWithListPatterns);
// PROTOTYPE missing diagnostics
compilation.VerifyEmitDiagnostics(
// (8,21): error CS0012: The type 'Missing' is defined in an assembly that is not referenced. You must add a reference to assembly 'missing, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null'.
// var index = c[^1];
Diagnostic(ErrorCode.ERR_NoTypeDef, "c[^1]").WithArguments("Missing", "missing, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null").WithLocation(8, 21),
// (9,21): error CS0012: The type 'Missing' is defined in an assembly that is not referenced. You must add a reference to assembly 'missing, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null'.
// var range = c[1..^1];
Diagnostic(ErrorCode.ERR_NoTypeDef, "c[1..^1]").WithArguments("Missing", "missing, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null").WithLocation(9, 21)
);
var tree = compilation.SyntaxTrees.First();
var model = compilation.GetSemanticModel(tree, ignoreAccessibility: false);
var declarations = tree.GetRoot().DescendantNodes().OfType<VarPatternSyntax>().ToArray();
verify(declarations[0], "item", "Missing?");
verify(declarations[1], "rest", "Missing?");
void verify(VarPatternSyntax declaration, string name, string expectedType)
{
var local = (ILocalSymbol)model.GetDeclaredSymbol(declaration.Designation)!;
Assert.Equal(name, local.Name);
Assert.Equal(expectedType, local.Type.ToTestDisplayString(includeNonNullable: true));
}
}
[Fact]
public void ListPattern_UseSiteErrorOnIndexAndRangeIndexers_WithFallback()
{
var missing_cs = @"
public class Missing
{
}
";
var missingRef = CreateCompilation(missing_cs, assemblyName: "missing")
.EmitToImageReference();
var lib2_cs = @"
using System;
public class C
{
public int Length => 0;
public Missing this[Index i] => throw null;
public Missing this[Range r] => throw null;
public int this[int i] => throw null;
public int Slice(int i, int j) => throw null;
}
";
var lib2Ref = CreateCompilation(new[] { lib2_cs, TestSources.Index, TestSources.Range }, references: new[] { missingRef })
.EmitToImageReference();
var source = @"
class D
{
void M(C c)
{
_ = c is { var item };
_ = c is { ..var rest };
var index = c[^1];
var range = c[1..^1];
}
}
";
var compilation = CreateCompilation(source, references: new[] { lib2Ref }, parseOptions: TestOptions.RegularWithListPatterns);
// PROTOTYPE binding for list and slice patterns should fail too
compilation.VerifyEmitDiagnostics(
// (8,21): error CS0012: The type 'Missing' is defined in an assembly that is not referenced. You must add a reference to assembly 'missing, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null'.
// var index = c[^1];
Diagnostic(ErrorCode.ERR_NoTypeDef, "c[^1]").WithArguments("Missing", "missing, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null").WithLocation(8, 21),
// (9,21): error CS0012: The type 'Missing' is defined in an assembly that is not referenced. You must add a reference to assembly 'missing, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null'.
// var range = c[1..^1];
Diagnostic(ErrorCode.ERR_NoTypeDef, "c[1..^1]").WithArguments("Missing", "missing, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null").WithLocation(9, 21)
);
}
There was a problem hiding this comment.
Looks like the BindIndexerOrIndexedPropertyAccess I'm using also fallback to TryBindIndexOrRange, but is BoundAccess fails and we throw away any diagnostics.
For indexer access it's working almost accidentally - we report the error while TryBindIndexOrRange succeeds. As a result, we've bound the fallback (just like list patterns) but reporting errors from the previous lookup (which is thrown away for list patterns)
Maybe we should depend on BindIndexerOrIndexedPropertyAccess for the fallback lookup and extract the symbol from BoundIndexOrRangePatternIndexerAccess?
I'll leave this as a prototype comment so you can decide what is the correct fix here. Thanks.
There was a problem hiding this comment.
Will be addressed by #53784
A quickfix would be to capture diagnostics when we have a BoundIndexOrRangeIndexer, but we might as well just use it to avoid re-binding.
|
@333fred for review. thanks. note: we may want to update the feature branch before proceeding with the follow-up. |
| originalBinder: this, | ||
| diagnose: false, | ||
| useSiteInfo: ref discardedUseSiteInfo); | ||
| useSiteInfo: ref useSiteInfo); |
There was a problem hiding this comment.
We've been passing Discard here for ranges too, I followed suit. I can add a prototype comment to revise.
|
|
||
| public override BoundNode VisitSlicePattern(BoundSlicePattern node) | ||
| { | ||
| return null; |
There was a problem hiding this comment.
Why are we not visiting the nested patterns? #Resolved
There was a problem hiding this comment.
They indeed need to be visited. This will be addressed in a later PR (here's the relevant commit I'm working on): 933dfd8
With the current PR, multiple nullability scenarios will hit assertions and/or produce incorrect nullability warnings.
|
Done review pass (commit 42) |
333fred
left a comment
There was a problem hiding this comment.
LGTM (commit 42), assuming the relevant prototype comments are added for followup.
|
I forgot to push. comments are added now. |
|
Merged/squashed. Thanks @alrz! |
Test plan: #51289
Spec: https://github.com/dotnet/csharplang/blob/main/proposals/list-patterns.md