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

Jump to bottom

List patterns: counterexample for non-exhaustive switch expressions #55327

Merged
merged 14 commits into from
Oct 27, 2021
Merged

List patterns: counterexample for non-exhaustive switch expressions #55327

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
cc00129
WIP
alrz Oct 18, 2021
e9eb399
WIP
alrz Oct 18, 2021
7dc6ac4
Cherry-pick tests from #57210
alrz Oct 19, 2021
4b29226
Update baseline
alrz Oct 19, 2021
abd7676
Skip discarded slice pattern
alrz Oct 19, 2021
3012db0
Bail if there's a slice but only one length value is remained
alrz Oct 20, 2021
b5b117e
Reorder
alrz Oct 20, 2021
2bf9fc4
Remove comments
alrz Oct 20, 2021
ddbc0c6
Add test
alrz Oct 22, 2021
7121a44
Bound check
alrz Oct 22, 2021
7dc82d4
Reword
alrz Oct 22, 2021
b3db969
Bail if the sample value is less than the required minimum length by …
alrz Oct 22, 2021
dcaac93
Add comments
alrz Oct 22, 2021
01ea646
PR feedback
alrz Oct 22, 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
159 changes: 127 additions & 32 deletions src/Compilers/CSharp/Portable/Binder/PatternExplainer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,7 @@ private static string SamplePatternForTemp(
tryHandleTuplePattern(ref unnamedEnumValue) ??
tryHandleNumericLimits(ref unnamedEnumValue) ??
tryHandleRecursivePattern(ref unnamedEnumValue) ??
tryHandleListPattern(ref unnamedEnumValue) ??
produceFallbackPattern();

static ImmutableArray<T> getArray<T>(Dictionary<BoundDagTemp, ArrayBuilder<T>> map, BoundDagTemp temp)
Expand Down Expand Up @@ -236,6 +237,100 @@ constraints[0] is (BoundDagNonNullTest _, true) &&
return null;
}

// Handle the special case of a list pattern
string tryHandleListPattern(ref bool unnamedEnumValue)
{
if (constraints.IsEmpty && evaluations.IsEmpty)
return null;

if (!constraints.All(c => c switch
{
Copy link
Member

Choose a reason for hiding this comment

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

Can we indent this? The current indentation level hurts my head.

// not-null tests are implicitly incorporated into a list pattern
(test: BoundDagNonNullTest _, sense: true) => true,
(test: BoundDagExplicitNullTest _, sense: false) => true,
_ => false,
}))
{
return null;
}

if (evaluations[0] is BoundDagPropertyEvaluation { IsLengthOrCount: true } lengthOrCount)
{
BoundDagSliceEvaluation slice = null;
for (int i = 1; i < evaluations.Length; i++)
{
switch (evaluations[i])
{
case BoundDagIndexerEvaluation e:
continue;
// A list pattern can only support a single slice within.
// We won't try to generate a list pattern if there's more.
case BoundDagSliceEvaluation e when slice == null:
Copy link
Member

@jcouv jcouv Oct 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

nit: We can remove the when clause and use an assertion instead (Debug.Assert(slice is null);) #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

See comment above.

Copy link
Member Author

@alrz alrz Oct 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Would it help to add a Assert.Fail in code paths that we don't have a test scenario for?

Copy link
Member

@jcouv jcouv Oct 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I'd even throw UnreachableException, since we won't get here with multiple slices.
Never mind

slice = e;
continue;
default:
return null;
}
}

var lengthTemp = new BoundDagTemp(lengthOrCount.Syntax, lengthOrCount.Property.Type, lengthOrCount);
var lengthValues = (IValueSet<int>)computeRemainingValues(ValueSetFactory.ForLength, getArray(constraintMap, lengthTemp));
int lengthValue = lengthValues.Sample.Int32Value;
if (slice != null && lengthValues.All(BinaryOperatorKind.Equal, lengthValue))
{
// Bail if there's a slice but only one length value is remained.
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we can support some of these cases, as this could be useful. Consider: null or { Length: 4 } => 0, [_, .. var middle, _] => 0 (that'd be a good test to add).
Drawing the distinction between cases we can correctly explain and those we can't may be tricky.
In draft #57210 I'd probably gone a bit too far in trying to explain more slice scenarios, but it may give you some idea.

We can also punt for now and file a follow-up issue to refine.

Copy link
Member Author

@alrz alrz Oct 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I added a test but it didn't hit this check. I think it's actually unlikely to get here without a nested length test.

Copy link
Member

Choose a reason for hiding this comment

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

It may be good to say why we bail (both cases).

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I see there was a typo in my example. Should have said not 4

Copy link
Member Author

@alrz alrz Oct 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Then it would be exhaustive. (no error)

return null;
}

var subpatterns = new ArrayBuilder<string>(lengthValue);
subpatterns.AddMany("_", lengthValue);
for (int i = 1; i < evaluations.Length; i++)
{
switch (evaluations[i])
{
case BoundDagIndexerEvaluation e:
var indexerTemp = new BoundDagTemp(e.Syntax, e.IndexerType, e);
int index = e.Index;
if (index > lengthValue - 1)
Copy link
Member

@jcouv jcouv Oct 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Do we need this check? #Closed

Copy link
Member Author

@alrz alrz Oct 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

It would hit if we didn't check slice/lengthValues above. But I think it's safer to just check all this upfront to avoid a crash. The explainer is generally defensive on out-of-bound errors and such. See tryHandleTuplePattern for instance.
Should check the other end as well. (added)

return null;
var effectiveIndex = index < 0 ? ^-index : index;
var oldPattern = subpatterns[effectiveIndex];
var newPattern = SamplePatternForTemp(indexerTemp, constraintMap, evaluationMap, requireExactType: false, ref unnamedEnumValue);
subpatterns[effectiveIndex] = makeConjunct(oldPattern, newPattern);
continue;
case BoundDagSliceEvaluation e:
Debug.Assert(e == slice);
continue;
case var v:
throw ExceptionUtilities.UnexpectedValue(v);
}
}

if (slice != null)
{
var sliceTemp = new BoundDagTemp(slice.Syntax, slice.SliceType, slice);
var slicePattern = SamplePatternForTemp(sliceTemp, constraintMap, evaluationMap, requireExactType: false, ref unnamedEnumValue);
if (slicePattern != "_")
{
// If the slice is not matched against any pattern, the slice pattern would
// have no effect on the output given the provided sample length value.
subpatterns.Insert(slice.StartIndex, $".. {slicePattern}");
}
}

return "[" + string.Join(", ", subpatterns) + "]";
}

return null;
}

static string makeConjunct(string oldPattern, string newPattern) => (oldPattern, newPattern) switch
{
("_", var x) => x,
(var x, "_") => x,
(var x, var y) => x + " and " + y
alrz marked this conversation as resolved.
Show resolved Hide resolved
};

// Handle the special case of a tuple pattern
string tryHandleTuplePattern(ref bool unnamedEnumValue)
{
Expand All @@ -262,13 +357,6 @@ string tryHandleTuplePattern(ref bool unnamedEnumValue)
}

return null;

static string makeConjunct(string oldPattern, string newPattern) => (oldPattern, newPattern) switch
{
("_", var x) => x,
(var x, "_") => x,
(var x, var y) => x + " and " + y
};
}

// Handle the special case of numeric limits
Expand All @@ -283,33 +371,10 @@ string tryHandleNumericLimits(ref bool unnamedEnumValue)
(BoundDagNonNullTest _, true) => true,
_ => false
}) &&
ValueSetFactory.ForType(input.Type) is { } fac)
ValueSetFactory.ForInput(input) is { } fac)
{
// All we have are numeric constraints. Process them to compute a value not covered.
var remainingValues = fac.AllValues;
foreach (var constraint in constraints)
{
var (test, sense) = constraint;
switch (test)
{
case BoundDagValueTest v:
addRelation(BinaryOperatorKind.Equal, v.Value);
break;
case BoundDagRelationalTest r:
addRelation(r.Relation, r.Value);
break;
}
void addRelation(BinaryOperatorKind relation, ConstantValue value)
{
if (value.IsBad)
return;
var filtered = fac.Related(relation, value);
if (!sense)
filtered = filtered.Complement();
remainingValues = remainingValues.Intersect(filtered);
}
}

IValueSet remainingValues = computeRemainingValues(fac, constraints);
if (remainingValues.Complement().IsEmpty)
return "_";

Expand Down Expand Up @@ -399,6 +464,36 @@ string produceFallbackPattern()
{
return requireExactType ? input.Type.ToDisplayString() : "_";
}

IValueSet computeRemainingValues(IValueSetFactory fac, ImmutableArray<(BoundDagTest test, bool sense)> constraints)
{
var remainingValues = fac.AllValues;
foreach (var constraint in constraints)
{
var (test, sense) = constraint;
switch (test)
{
case BoundDagValueTest v:
addRelation(BinaryOperatorKind.Equal, v.Value);
break;
case BoundDagRelationalTest r:
addRelation(r.Relation, r.Value);
break;
}

void addRelation(BinaryOperatorKind relation, ConstantValue value)
{
if (value.IsBad)
return;
var filtered = fac.Related(relation, value);
if (!sense)
filtered = filtered.Complement();
remainingValues = remainingValues.Intersect(filtered);
}
}

return remainingValues;
}
}

private static string SampleValueString(IValueSet remainingValues, TypeSymbol type, bool requireExactType, ref bool unnamedEnumValue)
Expand Down
Loading