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.

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: subsumption checking #53891

Merged
merged 72 commits into from
Sep 16, 2021
Merged
Show file tree
Hide file tree
Changes from 67 commits
Commits
Show all changes
72 commits
Select commit Hold shift + click to select a range
8ddbf28
WIP
alrz Jul 21, 2021
3a3f72a
Revert
alrz Jul 21, 2021
a24334d
Tweaks
alrz Jul 21, 2021
52e6be5
Move
alrz Jul 21, 2021
e3ce4fb
Remove unused usings
alrz Jul 21, 2021
3625d2f
Simplify
alrz Jul 21, 2021
5a359d0
Merge remote-tracking branch 'origin/features/list-patterns' into lis…
alrz Jul 26, 2021
de4f14a
WIP
alrz Jul 27, 2021
2beb033
WIP
alrz Jul 29, 2021
0dfd167
WIP
alrz Jul 29, 2021
ae7b003
Inline
alrz Jul 29, 2021
787b93d
Revert
alrz Jul 29, 2021
fa8a663
Extract method
alrz Jul 29, 2021
81d0e31
Add test
alrz Jul 30, 2021
91dbb3a
WIP
alrz Aug 1, 2021
f0adefa
Revert
alrz Aug 1, 2021
e671bd9
Cleanup
alrz Aug 3, 2021
0697233
Mark as unreachable
alrz Aug 4, 2021
0bb97b1
Renamings
alrz Aug 5, 2021
e5f1d86
Tweaks
alrz Aug 6, 2021
cdcd434
Add comments
alrz Aug 6, 2021
242690f
PR feedback
alrz Aug 7, 2021
ebd45ea
Add test
alrz Aug 7, 2021
8ed4ca8
Simplify
alrz Aug 7, 2021
33ca394
More tests
alrz Aug 7, 2021
eb44915
Test bad constants
alrz Aug 7, 2021
11ffabd
Merge values for matching indices
alrz Aug 8, 2021
d79cb4c
Revert
alrz Aug 12, 2021
7ca1ed7
WIP
alrz Aug 13, 2021
ee90c56
WIP
alrz Aug 13, 2021
dfa6b17
Tests
alrz Aug 13, 2021
389b9d0
Fixup
alrz Aug 13, 2021
ce9e8c4
Add skipped test
alrz Aug 13, 2021
c82209c
Do not drop the next node after a skipped evaluation
alrz Aug 14, 2021
b1d12cd
There cannot be any implication for an evaluation node
alrz Aug 14, 2021
635080d
Reorder
alrz Aug 15, 2021
67dc48b
PR feedback
alrz Aug 19, 2021
79c8fff
Swap
alrz Aug 19, 2021
50f4b1d
Run fuzz test
alrz Aug 19, 2021
9f38068
Typo
alrz Aug 19, 2021
840812a
Add PROTOTYPE comments
alrz Aug 19, 2021
565a972
Revert
alrz Aug 20, 2021
a20b47a
Add assertions
alrz Aug 21, 2021
18cdab3
Revert unrelated changes
alrz Sep 2, 2021
8d56fa8
Renamings
alrz Sep 2, 2021
a52aba1
Clarify comment
alrz Sep 2, 2021
24dcece
Add comments
alrz Sep 2, 2021
04d2de7
Reword
alrz Sep 2, 2021
5c2625b
New lines
alrz Sep 2, 2021
c86e44f
Add braces
alrz Sep 2, 2021
c5092df
Use statement-bodied methods across the file
alrz Sep 2, 2021
bebe7b0
New line
alrz Sep 2, 2021
8b1cead
Add comment on the test scenario
alrz Sep 2, 2021
2ca2b0c
Fixup and verify
alrz Sep 2, 2021
24f607d
Inheritdoc
alrz Sep 2, 2021
3307706
Whitespace
alrz Sep 2, 2021
cf73eab
Check for known result before shifting values
alrz Sep 2, 2021
7c63939
Whitespace
alrz Sep 2, 2021
6e109de
Revert new line
alrz Sep 2, 2021
ee27875
Reflect renamings in comment
alrz Sep 2, 2021
105f74f
Fixup
alrz Sep 2, 2021
ab253f7
Strengthen exhaustiveness tests
alrz Sep 2, 2021
aa99ba2
Revert
alrz Sep 2, 2021
bfeecd6
PR feedback
alrz Sep 5, 2021
418e644
Merge remote-tracking branch 'origin/features/list-patterns' into lis…
alrz Sep 5, 2021
f75e52a
PR feedback
alrz Sep 9, 2021
2df2a20
Use helpers
alrz Sep 9, 2021
18fcbdb
PR feedback
alrz Sep 10, 2021
7c13399
Extract debug-only helper
alrz Sep 10, 2021
b1d3a08
Extract method
alrz Sep 14, 2021
0e7737b
Rename
alrz Sep 14, 2021
68a9ab6
Revert code
alrz Sep 16, 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
18 changes: 15 additions & 3 deletions src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System;
using System.Collections.Immutable;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
Expand Down Expand Up @@ -341,11 +342,9 @@ private bool TryPerformPatternIndexerLookup(
BindingDiagnosticBag diagnostics)
{
Debug.Assert(!receiverType.IsErrorType());

indexerAccess = null;
patternSymbol = null;
lengthProperty = null;

bool found;
CompoundUseSiteInfo<AssemblySymbol> useSiteInfo = GetNewCompoundUseSiteInfo(diagnostics);
var bindingDiagnostics = BindingDiagnosticBag.GetInstance(diagnostics);
Expand Down Expand Up @@ -1437,6 +1436,7 @@ private ImmutableArray<BoundPropertySubpattern> BindPropertyPatternClause(
PatternSyntax pattern = p.Pattern;
BoundPropertySubpatternMember? member;
TypeSymbol memberType;
bool isLengthOrCount = false;
if (expr == null)
{
if (!hasErrors)
Expand All @@ -1450,10 +1450,22 @@ private ImmutableArray<BoundPropertySubpattern> BindPropertyPatternClause(
{
member = LookupMembersForPropertyPattern(inputType, expr, diagnostics, ref hasErrors);
memberType = member.Type;
// If we're dealing with the member that makes the type countable, and the type is also indexable, then it will be assumed to always return a non-negative value
if (memberType.SpecialType == SpecialType.System_Int32 &&
member.Symbol is { Name: WellKnownMemberNames.LengthPropertyName or WellKnownMemberNames.CountPropertyName, Kind: SymbolKind.Property } memberSymbol)
{
TypeSymbol receiverType = member.Receiver?.Type ?? inputType;
if (!receiverType.IsErrorType())
{
isLengthOrCount = receiverType.IsSZArray()
? ReferenceEquals(memberSymbol, Compilation.GetSpecialTypeMember(SpecialMember.System_Array__Length))
: TryPerformPatternIndexerLookup(node, receiverType, argIsIndex: true, indexerAccess: out _, patternSymbol: out _, lengthProperty: out _, BindingDiagnosticBag.Discarded);
}
}
}

BoundPattern boundPattern = BindPattern(pattern, memberType, GetValEscape(memberType, inputValEscape), permitDesignations, hasErrors, diagnostics);
builder.Add(new BoundPropertySubpattern(p, member, boundPattern));
builder.Add(new BoundPropertySubpattern(p, member, isLengthOrCount, boundPattern));
}

return builder.ToImmutableAndFree();
Expand Down
388 changes: 338 additions & 50 deletions src/Compilers/CSharp/Portable/Binder/DecisionDagBuilder.cs

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -32,15 +32,11 @@ private Tests MakeTestsAndBindingsForListPattern(BoundDagTemp input, BoundListPa
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.
alrz marked this conversation as resolved.
Show resolved Hide resolved
}
else
{
Debug.Assert(list.LengthProperty is not null);

var lengthEvaluation = new BoundDagPropertyEvaluation(syntax, list.LengthProperty, input);
var lengthEvaluation = new BoundDagPropertyEvaluation(syntax, list.LengthProperty, isLengthOrCount: true, input);
tests.Add(new Tests.One(lengthEvaluation));
var lengthTemp = new BoundDagTemp(syntax, _compilation.GetSpecialType(SpecialType.System_Int32), lengthEvaluation);
tests.Add(new Tests.One(list.HasSlice
Expand Down
49 changes: 32 additions & 17 deletions src/Compilers/CSharp/Portable/BoundTree/BoundDagEvaluation.cs
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,22 @@ namespace Microsoft.CodeAnalysis.CSharp
{
partial class BoundDagEvaluation
{
public override bool Equals([NotNullWhen(true)] object? obj) => obj is BoundDagEvaluation other && this.Equals(other);
public virtual bool Equals(BoundDagEvaluation other)
public sealed override bool Equals([NotNullWhen(true)] object? obj) => obj is BoundDagEvaluation other && this.Equals(other);
public bool Equals(BoundDagEvaluation other)
{
return this == other ||
this.Kind == other.Kind &&
this.Input.Equals(other.Input) &&
Symbol.Equals(this.Symbol, other.Symbol, TypeCompareKind.AllIgnoreOptions);
this.IsEquivalentTo(other) &&
333fred marked this conversation as resolved.
Show resolved Hide resolved
this.Input.Equals(other.Input);
}

/// <summary>
/// Check if this is equivalent to the <paramref name="other"/> node, ignoring the input.
/// </summary>
public virtual bool IsEquivalentTo(BoundDagEvaluation other)
{
return this == other ||
this.Kind == other.Kind &&
Symbol.Equals(this.Symbol, other.Symbol, TypeCompareKind.AllIgnoreOptions);
}

private Symbol? Symbol
Expand All @@ -32,6 +41,7 @@ private Symbol? Symbol
BoundDagIndexEvaluation e => e.Property,
BoundDagSliceEvaluation e => (Symbol?)e.SliceMethod ?? e.IndexerAccess?.Indexer,
BoundDagIndexerEvaluation e => e.IndexerSymbol ?? e.IndexerAccess?.Indexer,
BoundDagAssignmentEvaluation => null,
_ => throw ExceptionUtilities.UnexpectedValue(this.Kind)
};
}
Expand Down Expand Up @@ -79,37 +89,42 @@ internal string GetOutputTempDebuggerDisplay()
partial class BoundDagIndexEvaluation
{
public override int GetHashCode() => base.GetHashCode() ^ this.Index;
public override bool Equals(BoundDagEvaluation obj)
public override bool IsEquivalentTo(BoundDagEvaluation obj)
{
return this == obj ||
Copy link
Contributor

Choose a reason for hiding this comment

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

return this == obj ||

Here and below it looks like this change removed the reference equality optimization.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's just moved to the base; Equals does its own and then calls into the overrides.
For direct IsEquivalent usages it's not important because it's rather unlikely to have identical nodes where we use it.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's just moved to the base; Equals does its own and then calls into the overrides.

Because of this change we no longer bypass the following checks when the same instance is compared.

Copy link
Member Author

Choose a reason for hiding this comment

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

        public bool Equals(BoundDagEvaluation other)
        {
            return this == other ||
                this.IsEquivalentTo(other) &&
                this.Input.Equals(other.Input);
        }

I think this implementation does just that if I'm not mistaken.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this implementation does just that if I'm not mistaken.

I am not sure what implementation you are looking at, I am commenting on this implementation:

        public override bool IsEquivalentTo(BoundDagEvaluation obj)
        {
            return base.IsEquivalentTo(obj) &&
                // base.IsEquivalentTo checks the kind field, so the following cast is safe
                this.Index == ((BoundDagIndexEvaluation)obj).Index;
        }

I think, the checks after base.IsEquivalentTo(obj) are performed for the same instance. Whereas before the change we would bypass all checks.

Copy link
Member Author

@alrz alrz Oct 25, 2021

Choose a reason for hiding this comment

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

I'm talking about BoundDagEvaluation.Equals which is an existing code path with reference check intact.

A I said earlier, for direct IsEquivalentTo usages (where we indeed don't have a reference check) most of the time we've already determined that the two are not the same instance, e.g. Equals just returned false.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can just revert the check if you feel it's going to make a visible difference. I have no objection on that.

Copy link
Contributor

@AlekseyTs AlekseyTs Oct 25, 2021

Choose a reason for hiding this comment

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

We can just revert the check if you feel it's going to make a visible difference. I have no objection on that.

I prefer that we don't remove optimazation just because we assume that APIs are going to be called in certain order. That doesn't feel like the right approoach in the long term.

base.Equals(obj) &&
// base.Equals checks the kind field, so the following cast is safe
return base.IsEquivalentTo(obj) &&
// base.IsEquivalentTo checks the kind field, so the following cast is safe
this.Index == ((BoundDagIndexEvaluation)obj).Index;
}
}

partial class BoundDagIndexerEvaluation
{
public override int GetHashCode() => base.GetHashCode() ^ this.Index;
public override bool Equals(BoundDagEvaluation obj)
public override bool IsEquivalentTo(BoundDagEvaluation obj)
{
return this == obj ||
base.Equals(obj) &&
// base.Equals checks the kind field, so the following cast is safe
return base.IsEquivalentTo(obj) &&
this.Index == ((BoundDagIndexerEvaluation)obj).Index;
}
}

partial class BoundDagSliceEvaluation
{
public override int GetHashCode() => base.GetHashCode() ^ this.StartIndex ^ this.EndIndex;
public override bool Equals(BoundDagEvaluation obj)
public override bool IsEquivalentTo(BoundDagEvaluation obj)
{
return this == obj ||
base.Equals(obj) &&
// base.Equals checks the kind field, so the following cast is safe
return base.IsEquivalentTo(obj) &&
(BoundDagSliceEvaluation)obj is var e &&
this.StartIndex == e.StartIndex && this.EndIndex == e.EndIndex;
}
}

partial class BoundDagAssignmentEvaluation
{
public override int GetHashCode() => Hash.Combine(base.GetHashCode(), this.Target.GetHashCode());
public override bool IsEquivalentTo(BoundDagEvaluation obj)
{
return base.IsEquivalentTo(obj) &&
this.Target.Equals(((BoundDagAssignmentEvaluation)obj).Target);
}
}
}
17 changes: 14 additions & 3 deletions src/Compilers/CSharp/Portable/BoundTree/BoundDagTemp.cs
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,26 @@ partial class BoundDagTemp
/// </summary>
public bool IsOriginalInput => this.Source is null;

public static BoundDagTemp ForOriginalInput(SyntaxNode syntax, TypeSymbol type) => new BoundDagTemp(syntax, type, null, 0);
public static BoundDagTemp ForOriginalInput(SyntaxNode syntax, TypeSymbol type) => new BoundDagTemp(syntax, type, source: null, 0);

public override bool Equals(object? obj) => obj is BoundDagTemp other && this.Equals(other);

public bool Equals(BoundDagTemp other)
{
return other is { } &&
return
this.Type.Equals(other.Type, TypeCompareKind.AllIgnoreOptions) &&
object.Equals(this.Source, other.Source) && this.Index == other.Index;
object.Equals(this.Source, other.Source) &&
this.Index == other.Index;
}

/// <summary>
/// Check if this is equivalent to the <paramref name="other"/> node, ignoring the source.
/// </summary>
public bool IsEquivalentTo(BoundDagTemp other)
{
return
this.Type.Equals(other.Type, TypeCompareKind.AllIgnoreOptions) &&
this.Index == other.Index;
}

public override int GetHashCode()
Expand Down
4 changes: 4 additions & 0 deletions src/Compilers/CSharp/Portable/BoundTree/BoundDagTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ private bool Equals(BoundDagTest? other)
return false;
if (this == other)
return true;
if (!this.Input.Equals(other.Input))
return false;
alrz marked this conversation as resolved.
Show resolved Hide resolved

switch (this, other)
{
Expand Down Expand Up @@ -71,6 +73,8 @@ public override int GetHashCode()
return result;
case BoundDagIndexEvaluation i:
return $"{i.GetOutputTempDebuggerDisplay()} = {i.Input.GetDebuggerDisplay()}[{i.Index}]";
case BoundDagIndexerEvaluation i:
return $"{i.GetOutputTempDebuggerDisplay()} = {i.Input.GetDebuggerDisplay()}[{i.Index}]";
case BoundDagEvaluation e:
return $"{e.GetOutputTempDebuggerDisplay()} = {e.Kind}({e.Input.GetDebuggerDisplay()})";
case BoundDagTypeTest b:
Expand Down
5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml
Original file line number Diff line number Diff line change
Expand Up @@ -1429,6 +1429,7 @@
</Node>
<Node Name="BoundDagPropertyEvaluation" Base="BoundDagEvaluation">
<Field Name="Property" Type="PropertySymbol" Null="disallow"/>
<Field Name="IsLengthOrCount" Type="bool"/>
alrz marked this conversation as resolved.
Show resolved Hide resolved
</Node>
<Node Name="BoundDagIndexEvaluation" Base="BoundDagEvaluation">
<Field Name="Property" Type="PropertySymbol" Null="disallow"/>
Expand All @@ -1450,6 +1451,9 @@
<Field Name="IndexerAccess" Type="BoundIndexerAccess?"/>
<Field Name="SliceMethod" Type="MethodSymbol?"/>
</Node>
<Node Name="BoundDagAssignmentEvaluation" Base="BoundDagEvaluation">
<Field Name="Target" Type="BoundDagTemp"/>
</Node>

<Node Name="BoundSwitchSection" Base="BoundStatementList">
<Field Name="Locals" Type="ImmutableArray&lt;LocalSymbol&gt;"/>
Expand Down Expand Up @@ -2170,6 +2174,7 @@
<Node Name="BoundPropertySubpattern" Base="BoundSubpattern">
<!-- The property or field access in a property pattern. -->
<Field Name="Member" Type="BoundPropertySubpatternMember?"/>
<Field Name="IsLengthOrCount" Type="bool"/>
</Node>
<Node Name="BoundPropertySubpatternMember" Base="BoundNode">
<Field Name="Receiver" Type="BoundPropertySubpatternMember?"/>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -503,6 +503,8 @@ public PossiblyConditionalState Clone()
case BoundDagSliceEvaluation e:
addTemp(e, e.SliceType);
break;
case BoundDagAssignmentEvaluation e:
break;
default:
throw ExceptionUtilities.UnexpectedValue(p.Evaluation.Kind);
}
Expand Down
Loading