Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 10 commits into from
Oct 11, 2021
Merged

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

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
2d59c69
List patterns: add more tests and fix nullability analysis
jcouv May 27, 2021
87455e0
tweak
jcouv Jul 13, 2021
2168d8d
Merge remote-tracking branch 'dotnet/features/list-patterns' into lis…
jcouv Sep 17, 2021
918da0f
tweak
jcouv Sep 17, 2021
dc204ab
tweaks
jcouv Sep 17, 2021
1761bed
nit
jcouv Sep 17, 2021
d0819f4
Merge remote-tracking branch 'dotnet/features/list-patterns' into lis…
jcouv Oct 6, 2021
dbfc0ed
fix formatting in tests
jcouv Oct 6, 2021
3b28d40
Address feedback
jcouv Oct 6, 2021
866346a
Assertions
jcouv Oct 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions src/Compilers/CSharp/Portable/Binder/DecisionDagBuilder_ListPatterns.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,18 +53,18 @@ private Tests MakeTestsAndBindingsForListPattern(BoundDagTemp input, BoundListPa

if (slice.Pattern is BoundPattern slicePattern)
{
var sliceEvaluation = new BoundDagSliceEvaluation(syntax, slicePattern.InputType, lengthTemp, startIndex: startIndex, endIndex: index, slice.IndexerAccess, slice.SliceMethod, input);
var sliceEvaluation = new BoundDagSliceEvaluation(slicePattern.Syntax, slicePattern.InputType, lengthTemp, startIndex: startIndex, endIndex: index, slice.IndexerAccess, slice.SliceMethod, input);
tests.Add(new Tests.One(sliceEvaluation));
var sliceTemp = new BoundDagTemp(syntax, slicePattern.InputType, sliceEvaluation);
var sliceTemp = new BoundDagTemp(slicePattern.Syntax, slicePattern.InputType, sliceEvaluation);
tests.Add(MakeTestsAndBindings(sliceTemp, slicePattern, bindings));
}

continue;
}

var indexEvaluation = new BoundDagIndexerEvaluation(syntax, subpattern.InputType, lengthTemp, index++, list.IndexerAccess, list.IndexerSymbol, input);
var indexEvaluation = new BoundDagIndexerEvaluation(subpattern.Syntax, subpattern.InputType, lengthTemp, index++, list.IndexerAccess, list.IndexerSymbol, input);
tests.Add(new Tests.One(indexEvaluation));
var indexTemp = new BoundDagTemp(syntax, subpattern.InputType, indexEvaluation);
var indexTemp = new BoundDagTemp(subpattern.Syntax, subpattern.InputType, indexEvaluation);
Copy link
Member

@alrz alrz Jun 5, 2021 •
edited by jcouv
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this needed? We didn't do this for ITuple and others, I think. #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This much improves the debugging experience by associating temps with syntax they are related to, rather than the whole syntax.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the equivalent code for positional patterns (MakeTestsAndBindingsForRecursivePattern) it looks like we're already doing this. The BoundDagTemp creations use the narrower syntax (corresponding to individual elements rather than the whole pattern syntax).

                        BoundPattern pattern = recursive.Deconstruction[i].Pattern;
                        SyntaxNode syntax = pattern.Syntax;
                        var element = new BoundDagTemp(syntax, method.Parameters[i + extensionExtra].Type, evaluation, i);

                        BoundPattern pattern = recursive.Deconstruction[i].Pattern;
                        SyntaxNode syntax = pattern.Syntax;
...
                        var element = new BoundDagTemp(syntax, field.Type, evaluation);

tests.Add(MakeTestsAndBindings(indexTemp, subpattern, bindings));
}
}
Expand Down
15 changes: 15 additions & 0 deletions src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.DebugVerifier.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,21 @@ private void VisitBinaryOperatorChildren(BoundBinaryOperatorBase node)
return null;
}

public override BoundNode? VisitListPattern(BoundListPattern node)
{
VisitList(node.Subpatterns);
Visit(node.VariableAccess);
// Ignore indexer access (just a node to hold onto some symbols)
return null;
}

public override BoundNode? VisitSlicePattern(BoundSlicePattern node)
{
this.Visit(node.Pattern);
// Ignore indexer access (just a node to hold onto some symbols)
return null;
}

public override BoundNode? VisitSwitchExpressionArm(BoundSwitchExpressionArm node)
{
this.Visit(node.Pattern);
Expand Down
75 changes: 71 additions & 4 deletions src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker_Patterns.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -352,6 +352,7 @@ public PossiblyConditionalState Clone()
var originalInputMap = PooledDictionary<int, BoundExpression>.GetInstance();
originalInputMap.Add(originalInputSlot, expression);

// Note we customize equality in BoundDagTemp
var tempMap = PooledDictionary<BoundDagTemp, (int slot, TypeSymbol type)>.GetInstance();
Debug.Assert(isDerivedType(NominalSlotType(originalInputSlot), expressionType.Type));
tempMap.Add(rootTemp, (originalInputSlot, expressionType.Type));
Expand Down Expand Up @@ -498,11 +499,77 @@ public PossiblyConditionalState Clone()
addTemp(e, e.Property.Type);
break;
case BoundDagIndexerEvaluation e:
addTemp(e, e.IndexerType);
break;
{
Debug.Assert(inputSlot > 0);
TypeWithAnnotations type;
if (e.IndexerType.IsErrorType())
{
type = TypeWithAnnotations.Create(isNullableEnabled: true, e.IndexerType, isAnnotated: false);
}
else if (e.IndexerAccess is not null)
{
// this[Index]
Debug.Assert(e.IndexerSymbol is null && e.Input.Type is not ArrayTypeSymbol);
var indexer = AsMemberOfType(inputType, e.IndexerAccess.Indexer);
type = indexer.GetTypeOrReturnType();
}
else if (e.IndexerSymbol is not null)
{
// this[int]
Debug.Assert(e.Input.Type is not ArrayTypeSymbol);
var indexer = AsMemberOfType(inputType, e.IndexerSymbol);
type = indexer.GetTypeOrReturnType();
}
else
{
// array[int]
var arrayType = (ArrayTypeSymbol)e.Input.Type;
type = arrayType.ElementTypeWithAnnotations;
}

var output = new BoundDagTemp(e.Syntax, type.Type, e);
var outputSlot = makeDagTempSlot(type, output);
Debug.Assert(outputSlot > 0);
addToTempMap(output, outputSlot, type.Type);
break;
}
case BoundDagSliceEvaluation e:
addTemp(e, e.SliceType);
break;
{
Debug.Assert(inputSlot > 0);
TypeWithAnnotations type;

if (e.SliceType.IsErrorType())
{
type = TypeWithAnnotations.Create(isNullableEnabled: true, e.SliceType, isAnnotated: false);
}
else if (e.IndexerAccess is not null)
{
// this[Range]
Debug.Assert(e.SliceMethod is null && e.Input.Type is not ArrayTypeSymbol);
var symbol = AsMemberOfType(inputType, e.IndexerAccess.Indexer);
type = symbol.GetTypeOrReturnType();
}
else if (e.SliceMethod is not null)
{
// Slice(int, int)
Debug.Assert(e.Input.Type is not ArrayTypeSymbol);
var symbol = AsMemberOfType(inputType, e.SliceMethod);
type = symbol.GetTypeOrReturnType();
}
else
{
// RuntimeHelpers.GetSubArray(T[], Range)
var arrayType = e.Input.Type;
Debug.Assert(arrayType is ArrayTypeSymbol);
type = TypeWithAnnotations.Create(isNullableEnabled: true, arrayType, isAnnotated: false);
}

var output = new BoundDagTemp(e.Syntax, type.Type, e);
var outputSlot = makeDagTempSlot(type, output);
Debug.Assert(outputSlot > 0);
addToTempMap(output, outputSlot, type.Type);
break;
}
case BoundDagAssignmentEvaluation e:
break;
default:
Expand Down
5 changes: 0 additions & 5 deletions src/Compilers/CSharp/Test/Emit/CodeGen/IndexAndRangeTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,6 @@ public int this[int index]
_array[index] = value;
}
}
}
class C
{
Expand All @@ -260,7 +259,6 @@ static void Main(string[] args)
var s = new S(array);
s[^1] += 5;
Console.WriteLine(array[1]);
}
}
";
Expand Down Expand Up @@ -352,7 +350,6 @@ static void Main(string[] args)
var s = new S(array);
s[1..] += 5;
Console.WriteLine(array[1]);
}
}
";
Expand Down Expand Up @@ -441,7 +438,6 @@ public string this[int index]
_array[index] = value;
}
}
}
class C
{
Expand Down Expand Up @@ -3004,7 +3000,6 @@ static Index Create(int x, out int y)
}", options: TestOptions.ReleaseExe), expectedOutput: "YES");
}


private const string PrintIndexesAndRangesCode = @"
partial class Program
{
Expand Down
Loading