Skip to content

Commit

Permalink
PR feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
@alrz
alrz committed Oct 28, 2021
1 parent e6d7842 commit 64f4343
Show file tree
Hide file tree
Showing 3 changed files with 110 additions and 61 deletions.
124 changes: 66 additions & 58 deletions src/Compilers/CSharp/Portable/Binder/DecisionDagBuilder.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1110,6 +1110,7 @@ private static (BoundDagTemp? lengthTemp, int offset) TryGetTopLevelLengthTemp(B
BoundDagTemp? lengthTemp = null;
while (input.Source is BoundDagSliceEvaluation slice)
{
Debug.Assert(input.Index == 0);
offset += slice.StartIndex - slice.EndIndex;
lengthTemp = slice.LengthTemp;
input = slice.Input;
Expand All @@ -1124,6 +1125,7 @@ private static (BoundDagTemp input, BoundDagTemp lengthTemp, int index) GetCanon
BoundDagTemp lengthTemp = e.LengthTemp;
while (input.Source is BoundDagSliceEvaluation slice)
{
Debug.Assert(input.Index == 0);
index = index < 0 ? index - slice.EndIndex : index + slice.StartIndex;
lengthTemp = slice.LengthTemp;
input = slice.Input;
Expand Down Expand Up @@ -1184,17 +1186,6 @@ private void CheckConsistentDecision(
trueTestImpliesTrueOther = false;
falseTestImpliesTrueOther = false;

// For null tests or type tests we just need to make sure we are looking at the same instance,
// for other tests the projected type should also match
if (test is not (BoundDagNonNullTest or BoundDagExplicitNullTest) &&
other is not (BoundDagNonNullTest or BoundDagExplicitNullTest) &&
(test is not BoundDagTypeTest || other is not BoundDagTypeTest) &&
!test.Input.Type.Equals(other.Input.Type, TypeCompareKind.AllIgnoreOptions))
{
Debug.Assert(!test.Input.Equals(other.Input));
return;
}

switch (test)
{
case BoundDagNonNullTest _:
Expand Down Expand Up @@ -1363,68 +1354,85 @@ private static bool CheckInputRelation(
return true;
}

// For null tests or type tests we just need to make sure we are looking at the same instance,
// for other tests the projected type should also match
if (test is not (BoundDagNonNullTest or BoundDagExplicitNullTest) &&
other is not (BoundDagNonNullTest or BoundDagExplicitNullTest) &&
(test is not BoundDagTypeTest || other is not BoundDagTypeTest) &&
!test.Input.Type.Equals(other.Input.Type, TypeCompareKind.AllIgnoreOptions))
{
return false;
}

var conditions = ArrayBuilder<Tests>.GetInstance();
// Loop through the input chain for both tests at the same time and check if there's
// any pair of indexers in the path that could relate depending on the length value.
do
{
switch (s1Input.Source, s2Input.Source)
if (s1Input.IsSameValue(s2Input))
{
// Even though the two tests appear unrelated (with different inputs),
// it is possible that they are in fact related under certain conditions.
// For instance, the inputs [0] and [^1] point to the same element when length is 1.
case (BoundDagIndexerEvaluation s1, BoundDagIndexerEvaluation s2):
// Take the top-level input and normalize indices to account for indexer accesses inside a slice.
// For instance [0] in nested list pattern [ 0, ..[$$], 2 ] refers to [1] in the containing list.
(s1Input, BoundDagTemp s1LengthTemp, int s1Index) = GetCanonicalInput(s1);
(s2Input, BoundDagTemp s2LengthTemp, int s2Index) = GetCanonicalInput(s2);
Debug.Assert(s1LengthTemp.Syntax is ListPatternSyntax);
Debug.Assert(s2LengthTemp.Syntax is ListPatternSyntax);
// Ignore input source as it will be matched in the subsequent iterations.
if (s1Input.IsEquivalentTo(s2Input) &&
// We don't want to pair two indices within the same pattern.
s1LengthTemp.Syntax != s2LengthTemp.Syntax)
{
Debug.Assert(s1LengthTemp.IsEquivalentTo(s2LengthTemp));
Debug.Assert(s1LengthTemp.Equals(s2LengthTemp) == s1Input.Equals(s2Input));
if (s1Index == s2Index)
{
continue;
}

if (s1Index < 0 != s2Index < 0)
switch (s1Input.Source, s2Input.Source)
{
// We should've skipped all type evaluations at this point.
case (BoundDagTypeEvaluation, _):
case (_, BoundDagTypeEvaluation):
throw ExceptionUtilities.Unreachable;

// Even though the two tests appear unrelated (with different inputs),
// it is possible that they are in fact related under certain conditions.
// For instance, the inputs [0] and [^1] point to the same element when length is 1.
case (BoundDagIndexerEvaluation s1, BoundDagIndexerEvaluation s2):
// Take the top-level input and normalize indices to account for indexer accesses inside a slice.
// For instance [0] in nested list pattern [ 0, ..[$$], 2 ] refers to [1] in the containing list.
(s1Input, BoundDagTemp s1LengthTemp, int s1Index) = GetCanonicalInput(s1);
(s2Input, BoundDagTemp s2LengthTemp, int s2Index) = GetCanonicalInput(s2);
Debug.Assert(s1LengthTemp.Syntax is ListPatternSyntax);
Debug.Assert(s2LengthTemp.Syntax is ListPatternSyntax);
// Ignore input source as it will be matched in the subsequent iterations.
if (s1Input.IsSameValue(s2Input) &&
// We don't want to pair two indices within the same pattern.
s1LengthTemp.Syntax != s2LengthTemp.Syntax)
{
Debug.Assert(state.RemainingValues.ContainsKey(s1LengthTemp));
var lengthValues = (IValueSet<int>)state.RemainingValues[s1LengthTemp];
// We do not expect an empty set here because an indexer evaluation is always preceded by
// a length test of which an impossible match would have made the rest of the tests unreachable.
Debug.Assert(!lengthValues.IsEmpty);

// Compute the length value that would make these two indices point to the same element.
int lengthValue = s1Index < 0 ? s2Index - s1Index : s1Index - s2Index;
if (lengthValues.All(BinaryOperatorKind.Equal, lengthValue))
Debug.Assert(s1LengthTemp.IsEquivalentTo(s2LengthTemp));
if (s1Index == s2Index)
{
// If the length is known to be exact, the two are considered to point to the same element.
continue;
}

if (lengthValues.Any(BinaryOperatorKind.Equal, lengthValue))
if (s1Index < 0 != s2Index < 0)
{
// Otherwise, we add a test to make the result conditional on the length value.
conditions.Add(new Tests.One(new BoundDagValueTest(syntax, ConstantValue.Create(lengthValue), s1LengthTemp)));
continue;
Debug.Assert(state.RemainingValues.ContainsKey(s1LengthTemp));
var lengthValues = (IValueSet<int>)state.RemainingValues[s1LengthTemp];
// We do not expect an empty set here because an indexer evaluation is always preceded by
// a length test of which an impossible match would have made the rest of the tests unreachable.
Debug.Assert(!lengthValues.IsEmpty);

// Compute the length value that would make these two indices point to the same element.
int lengthValue = s1Index < 0 ? s2Index - s1Index : s1Index - s2Index;
if (lengthValues.All(BinaryOperatorKind.Equal, lengthValue))
{
// If the length is known to be exact, the two are considered to point to the same element.
continue;
}

if (lengthValues.Any(BinaryOperatorKind.Equal, lengthValue))
{
// Otherwise, we add a test to make the result conditional on the length value.
conditions.Add(new Tests.One(new BoundDagValueTest(syntax, ConstantValue.Create(lengthValue), s1LengthTemp)));
continue;
}
}
}
}
break;
break;

// If the sources are equivalent (ignoring their input), it's still possible to find a pair of indexers that could relate.
// For example, the subpatterns in `[.., { E: subpat }] or [{ E: subpat }]` are being applied to the same element in the list.
// To account for this scenario, we walk up all the inputs as long as we see equivalent evaluation nodes in the path.
case (BoundDagEvaluation s1, BoundDagEvaluation s2) when s1.IsEquivalentTo(s2) && s1Input.IsEquivalentTo(s2Input):
s1Input = OriginalInput(s1.Input);
s2Input = OriginalInput(s2.Input);
continue;
// If the sources are equivalent (ignoring their input), it's still possible to find a pair of indexers that could relate.
// For example, the subpatterns in `[.., { E: subpat }] or [{ E: subpat }]` are being applied to the same element in the list.
// To account for this scenario, we walk up all the inputs as long as we see equivalent evaluation nodes in the path.
case (BoundDagEvaluation s1, BoundDagEvaluation s2) when s1.IsEquivalentTo(s2):
s1Input = OriginalInput(s1.Input);
s2Input = OriginalInput(s2.Input);
continue;
}
}

// tests are unrelated
Expand Down
10 changes: 10 additions & 0 deletions src/Compilers/CSharp/Portable/BoundTree/BoundDagTemp.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,16 @@ public bool IsEquivalentTo(BoundDagTemp other)
this.Index == other.Index;
}

/// <summary>
/// Determine if two <see cref="BoundDagTemp"/>s represent the same value for the purpose of a pattern evaluation.
/// </summary>
public bool IsSameValue(BoundDagTemp other)
{
Debug.Assert(this.Source is not BoundDagTypeEvaluation);
Debug.Assert(other.Source is not BoundDagTypeEvaluation);
return (object)this == other || this.Index == other.Index;
}

public override int GetHashCode()
{
return Hash.Combine(this.Type.GetHashCode(), Hash.Combine(this.Source?.GetHashCode() ?? 0, this.Index));
Expand Down
37 changes: 34 additions & 3 deletions src/Compilers/CSharp/Test/Semantic/Semantics/PatternMatchingTests_ListPatterns.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5172,13 +5172,38 @@ public static void Main()
case [I2 and Base]: // 12
break;
}
object obj = null;
switch (obj)
{
case I1 and Base and [.., I1 and Base]:
break;
case Derived and [Derived s]: // 13
break;
}
switch (obj)
{
case Base and I1 and [.., Base and I1]:
break;
case Derived and [Derived s]: // OK, we're calling into different indexers
break;
}
switch (obj)
{
case Base and not null and [.., Base and not null]:
break;
case Derived and [Derived s]: // 14
break;
}
}
}
interface I1 {}
interface I1 : System.Collections.IList {}
interface I2 {}
class Base : I1
class Base : System.Collections.ArrayList, I1
{
public int F1 = 0;
public int F2 = 0;
Expand Down Expand Up @@ -5230,7 +5255,13 @@ class Derived : Base, I2
Diagnostic(ErrorCode.ERR_SwitchCaseSubsumed, "[Base and I2]").WithLocation(103, 18),
// (111,18): error CS8120: The switch case is unreachable. It has already been handled by a previous case or it is impossible to match.
// case [I2 and Base]: // 12
Diagnostic(ErrorCode.ERR_SwitchCaseSubsumed, "[I2 and Base]").WithLocation(111, 18)
Diagnostic(ErrorCode.ERR_SwitchCaseSubsumed, "[I2 and Base]").WithLocation(111, 18),
// (120,18): error CS8120: The switch case is unreachable. It has already been handled by a previous case or it is impossible to match.
// case Derived and [Derived s]: // 13
Diagnostic(ErrorCode.ERR_SwitchCaseSubsumed, "Derived and [Derived s]").WithLocation(120, 18),
// (136,18): error CS8120: The switch case is unreachable. It has already been handled by a previous case or it is impossible to match.
// case Derived and [Derived s]: // 14
Diagnostic(ErrorCode.ERR_SwitchCaseSubsumed, "Derived and [Derived s]").WithLocation(136, 18)
);
}

Expand Down

0 comments on commit 64f4343

Please sign in to comment.