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

Explainer for list-patterns #57210

Closed
wants to merge 5 commits into from
Closed

Explainer for list-patterns #57210

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4ff038d
Explainer for list-patterns
jcouv Oct 18, 2021
89edb85
Use refactoring from alrz
jcouv Oct 18, 2021
4f6cc89
Simplify logic
jcouv Oct 19, 2021
3144a3d
Compute example length for list pattern in slice
jcouv Oct 19, 2021
3f133d4
tests
jcouv Oct 19, 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
90 changes: 90 additions & 0 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) ??
Comment on lines 179 to +180
Copy link
Member

Choose a reason for hiding this comment

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

Doing this after recursive patterns will make it unnecessary to worry about [] or [..] but we may choose to generate those instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that something I've been debating. Should we say { Length: 0 } or []?

produceFallbackPattern();

static ImmutableArray<T> getArray<T>(Dictionary<BoundDagTemp, ArrayBuilder<T>> map, BoundDagTemp temp)
Expand Down Expand Up @@ -319,6 +320,95 @@ void addRelation(BinaryOperatorKind relation, ConstantValue value)
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
{
// not-null tests are implicitly incorporated into a recursive pattern
(test: BoundDagNonNullTest _, sense: true) => true,
(test: BoundDagExplicitNullTest _, sense: false) => true,
_ => false,
}))
{
return null;
}

var indexes = new Dictionary<int, string>();
Copy link
Member

Choose a reason for hiding this comment

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

Did not know you were working on this. I took a different approach to handle trailing patterns. Updated #55327 just in case. We should def go with your change since this is closer to completion. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, I got a bit excited this weekend and gave it a try. Thanks
I'll probably steal some bits from your draft

int length = -1;

foreach (var eval in evaluations)
{
switch (eval)
{
case BoundDagPropertyEvaluation e when e.IsLengthOrCount:
Copy link
Member

Choose a reason for hiding this comment

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

It's unlikely to have multiple length tests in the shortest path. I suspect this doesn't need to be in the loop.

{
Debug.Assert(length == -1);
var subInput = new BoundDagTemp(e.Syntax, e.Property.Type, e);
var lengthValue = SamplePatternForTemp(subInput, constraintMap, evaluationMap, false, ref unnamedEnumValue);
if (int.TryParse(lengthValue, out var parsedLength))
{
Debug.Assert(parsedLength >= 0);
length = parsedLength;
}
}
break;
case BoundDagIndexerEvaluation e:
{
var subInput = new BoundDagTemp(e.Syntax, e.IndexerType, e);
var subPattern = SamplePatternForTemp(subInput, constraintMap, evaluationMap, false, ref unnamedEnumValue);
indexes.Add(e.Index, subPattern);
}
break;
case BoundDagSliceEvaluation e:
Copy link
Member

Choose a reason for hiding this comment

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

This may produce multiple slice patterns in the list but since we won't construct a dag for invalid patterns that probably won't happen.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, we don't get here when multiple slices. Added test

{
if (e.StartIndex < length + e.EndIndex)
{
var subInput = new BoundDagTemp(e.Syntax, e.SliceType, e);
var subPattern = SamplePatternForTemp(subInput, constraintMap, evaluationMap, false, ref unnamedEnumValue);
indexes.Add(e.StartIndex, ".. " + subPattern);
}
}
break;
default:
return null;
}
}

if (length < 0)
{
Debug.Assert(false);
return null;
}

var pooledBuilder = PooledStringBuilder.GetInstance();
var builder = pooledBuilder.Builder;

builder.Append("[");
for (int i = 0; i < length; i++)
{
string sample = (indexes.TryGetValue(i, out var sampleFromStart), indexes.TryGetValue(i - length, out var sampleFromEnd)) switch
{
(true, false) => sampleFromStart,
(false, true) => sampleFromEnd,
_ => "_"
};

if (builder.Length > 1)
{
builder.Append(", ");
}

builder.Append(sample);
}
builder.Append("]");

return pooledBuilder.ToStringAndFree();
}

// Handle the special case of a recursive pattern
string tryHandleRecursivePattern(ref bool unnamedEnumValue)
{
Expand Down
153 changes: 113 additions & 40 deletions src/Compilers/CSharp/Test/Semantic/Semantics/PatternMatchingTests_ListPatterns.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3129,21 +3129,21 @@ public void Pattern_Nullability_Exhaustiveness()
{ Length: 0 or > 1 } => 0,
};

_ = o switch // didn't test for null
_ = o switch // 1, didn't test for null
{
[null] => 0,
[not null] => 0,
{ Length: 0 or > 1 } => 0,
};

_ = o switch // didn't test for [null]
_ = o switch // 2, didn't test for [null]
{
null => 0,
[not null] => 0,
{ Length: 0 or > 1 } => 0,
};

_ = o switch // didn't test for [not null]
_ = o switch // 3, didn't test for [not null]
{
null => 0,
[null] => 0,
Expand All @@ -3158,14 +3158,14 @@ public void Pattern_Nullability_Exhaustiveness()
[.., not null] => 0,
};

_ = o switch // didn't test for [.., null]
_ = o switch // 4, didn't test for [null]
{
null => 0,
[] => 0,
[.., not null] => 0,
};

_ = o switch // didn't test for [.., not null]
_ = o switch // 5, didn't test for [not null]
{
null => 0,
[] => 0,
Expand All @@ -3179,25 +3179,65 @@ public void Pattern_Nullability_Exhaustiveness()
[not null, ..] => 0,
{ Length: 0 or > 1 } => 0,
};

_ = o switch // 6, didn't test for [_, null]
{
null => 0,
[] => 0,
[_] => 0,
[.., not null] => 0,
};

_ = o switch // 7, didn't test for [null, _]
{
null => 0,
[] => 0,
[_] => 0,
[not null, ..] => 0,
};

_ = o switch // 8, didn't test for { Length: 0 }
{
null => 0,
[_, ..] => 0,
};

_ = o switch // 9, didn't test for [null]
{
null => 0,
[] => 0,
[..var x, not null] => 0,
};
";
// PROTOTYPE: incorrect exhaustiveness examples from explainer
var compilation = CreateCompilation(source);
var compilation = CreateCompilation(new[] { source, TestSources.Index, TestSources.Range, TestSources.GetSubArray });
compilation.VerifyEmitDiagnostics(
// (13,7): warning CS8655: The switch expression does not handle some null inputs (it is not exhaustive). For example, the pattern 'null' is not covered.
// _ = o switch // didn't test for null
// _ = o switch // 1, didn't test for null
Diagnostic(ErrorCode.WRN_SwitchExpressionNotExhaustiveForNull, "switch").WithArguments("null").WithLocation(13, 7),
// (20,7): warning CS8655: The switch expression does not handle some null inputs (it is not exhaustive). For example, the pattern '_' is not covered.
// _ = o switch // didn't test for [null]
Diagnostic(ErrorCode.WRN_SwitchExpressionNotExhaustiveForNull, "switch").WithArguments("_").WithLocation(20, 7),
// (27,7): warning CS8509: The switch expression does not handle all possible values of its input type (it is not exhaustive). For example, the pattern '_' is not covered.
// _ = o switch // didn't test for [not null]
Diagnostic(ErrorCode.WRN_SwitchExpressionNotExhaustive, "switch").WithArguments("_").WithLocation(27, 7),
// (42,7): warning CS8655: The switch expression does not handle some null inputs (it is not exhaustive). For example, the pattern '_' is not covered.
// _ = o switch // didn't test for [.., null]
Diagnostic(ErrorCode.WRN_SwitchExpressionNotExhaustiveForNull, "switch").WithArguments("_").WithLocation(42, 7),
// (49,7): warning CS8509: The switch expression does not handle all possible values of its input type (it is not exhaustive). For example, the pattern '_' is not covered.
// _ = o switch // didn't test for [.., not null]
Diagnostic(ErrorCode.WRN_SwitchExpressionNotExhaustive, "switch").WithArguments("_").WithLocation(49, 7)
// (20,7): warning CS8655: The switch expression does not handle some null inputs (it is not exhaustive). For example, the pattern '[null]' is not covered.
// _ = o switch // 2, didn't test for [null]
Diagnostic(ErrorCode.WRN_SwitchExpressionNotExhaustiveForNull, "switch").WithArguments("[null]").WithLocation(20, 7),
// (27,7): warning CS8509: The switch expression does not handle all possible values of its input type (it is not exhaustive). For example, the pattern '[not null]' is not covered.
// _ = o switch // 3, didn't test for [not null]
Diagnostic(ErrorCode.WRN_SwitchExpressionNotExhaustive, "switch").WithArguments("[not null]").WithLocation(27, 7),
// (42,7): warning CS8655: The switch expression does not handle some null inputs (it is not exhaustive). For example, the pattern '[null]' is not covered.
// _ = o switch // 4, didn't test for [null]
Diagnostic(ErrorCode.WRN_SwitchExpressionNotExhaustiveForNull, "switch").WithArguments("[null]").WithLocation(42, 7),
// (49,7): warning CS8509: The switch expression does not handle all possible values of its input type (it is not exhaustive). For example, the pattern '[not null]' is not covered.
// _ = o switch // 5, didn't test for [not null]
Diagnostic(ErrorCode.WRN_SwitchExpressionNotExhaustive, "switch").WithArguments("[not null]").WithLocation(49, 7),
// (64,7): warning CS8655: The switch expression does not handle some null inputs (it is not exhaustive). For example, the pattern '[_, null]' is not covered.
// _ = o switch // 6, didn't test for [_, null]
Diagnostic(ErrorCode.WRN_SwitchExpressionNotExhaustiveForNull, "switch").WithArguments("[_, null]").WithLocation(64, 7),
// (72,7): warning CS8655: The switch expression does not handle some null inputs (it is not exhaustive). For example, the pattern '[null, _]' is not covered.
// _ = o switch // 7, didn't test for [null, _]
Diagnostic(ErrorCode.WRN_SwitchExpressionNotExhaustiveForNull, "switch").WithArguments("[null, _]").WithLocation(72, 7),
// (80,7): warning CS8509: The switch expression does not handle all possible values of its input type (it is not exhaustive). For example, the pattern '{ Length: 0 }' is not covered.
// _ = o switch // 8, didn't test for { Length: 0 }
Diagnostic(ErrorCode.WRN_SwitchExpressionNotExhaustive, "switch").WithArguments("{ Length: 0 }").WithLocation(80, 7),
// (86,7): warning CS8655: The switch expression does not handle some null inputs (it is not exhaustive). For example, the pattern '[null]' is not covered.
// _ = o switch // 9, didn't test for [null]
Diagnostic(ErrorCode.WRN_SwitchExpressionNotExhaustiveForNull, "switch").WithArguments("[null]").WithLocation(86, 7)
);
}

Expand Down Expand Up @@ -3263,15 +3303,52 @@ public void M()
}
}
";
// PROTOTYPE: incorrect exhaustiveness examples from explainer
var compilation = CreateCompilation(new[] { source, TestSources.Index, TestSources.Range });
compilation.VerifyEmitDiagnostics(
// (20,18): warning CS8655: The switch expression does not handle some null inputs (it is not exhaustive). For example, the pattern '_' is not covered.
// (20,18): warning CS8655: The switch expression does not handle some null inputs (it is not exhaustive). For example, the pattern '[.. null]' is not covered.
// _ = this switch // no tests for null slice
Diagnostic(ErrorCode.WRN_SwitchExpressionNotExhaustiveForNull, "switch").WithArguments("_").WithLocation(20, 18),
// (27,18): warning CS8509: The switch expression does not handle all possible values of its input type (it is not exhaustive). For example, the pattern '_' is not covered.
Diagnostic(ErrorCode.WRN_SwitchExpressionNotExhaustiveForNull, "switch").WithArguments("[.. null]").WithLocation(20, 18),
// (27,18): warning CS8509: The switch expression does not handle all possible values of its input type (it is not exhaustive). For example, the pattern '[.. not null]' is not covered.
// _ = this switch // no test for not null slice
Diagnostic(ErrorCode.WRN_SwitchExpressionNotExhaustive, "switch").WithArguments("_").WithLocation(27, 18)
Diagnostic(ErrorCode.WRN_SwitchExpressionNotExhaustive, "switch").WithArguments("[.. not null]").WithLocation(27, 18)
);
}

[Fact]
public void SlicePattern_Nullability_Exhaustiveness_WithNestedPropertyTest()
{
var source = @"
#nullable enable
using System;
class C
{
public int Length => throw null!;
public D? this[Index i] => throw null!;
public D? this[Range r] => throw null!;

public void M()
{
_ = this switch
{
null => 0,
[] => 0,
[_, _, ..] => 0,
[.. null] => 0,
[.. { Property: < 0 }] => 0,
};
}
}

class D
{
public int Property { get; set; }
}
";
var compilation = CreateCompilation(new[] { source, TestSources.Index, TestSources.Range });
compilation.VerifyEmitDiagnostics(
// (12,18): warning CS8509: The switch expression does not handle all possible values of its input type (it is not exhaustive). For example, the pattern '[.. { Property: 0 }]' is not covered.
// _ = this switch
Diagnostic(ErrorCode.WRN_SwitchExpressionNotExhaustive, "switch").WithArguments("[.. { Property: 0 }]").WithLocation(12, 18)
);
}

Expand Down Expand Up @@ -3316,6 +3393,7 @@ public void M()
}
}
";
// TODO2
// PROTOTYPE: unexpected exhaustiveness diagnostic
var compilation = CreateCompilation(new[] { source, TestSources.Index, TestSources.Range });
compilation.VerifyEmitDiagnostics(
Expand Down Expand Up @@ -3878,15 +3956,14 @@ class C
public int Count => throw null;
public int this[int i] => throw null;
}";
// PROTOTYPE bad explanation for 2
var comp = CreateCompilation(src);
comp.VerifyEmitDiagnostics(
// (2,13): warning CS8509: The switch expression does not handle all possible values of its input type (it is not exhaustive). For example, the pattern '{ Count: 0 }' is not covered.
// _ = new C() switch // 1
Diagnostic(ErrorCode.WRN_SwitchExpressionNotExhaustive, "switch").WithArguments("{ Count: 0 }").WithLocation(2, 13),
// (8,13): warning CS8509: The switch expression does not handle all possible values of its input type (it is not exhaustive). For example, the pattern '_' is not covered.
// (8,13): warning CS8509: The switch expression does not handle all possible values of its input type (it is not exhaustive). For example, the pattern '[0]' is not covered.
// _ = new C() switch // 2
Diagnostic(ErrorCode.WRN_SwitchExpressionNotExhaustive, "switch").WithArguments("_").WithLocation(8, 13)
Diagnostic(ErrorCode.WRN_SwitchExpressionNotExhaustive, "switch").WithArguments("[0]").WithLocation(8, 13)
);
}

Expand Down Expand Up @@ -3928,12 +4005,11 @@ class C
public int Count => throw null!;
public string? this[int i] => throw null!;
}";
// PROTOTYPE bad explanations on 1 and 2
var comp = CreateCompilation(src);
comp.VerifyEmitDiagnostics(
// (3,13): warning CS8655: The switch expression does not handle some null inputs (it is not exhaustive). For example, the pattern '_' is not covered.
// (3,13): warning CS8655: The switch expression does not handle some null inputs (it is not exhaustive). For example, the pattern '[null]' is not covered.
// _ = new C() switch // 1
Diagnostic(ErrorCode.WRN_SwitchExpressionNotExhaustiveForNull, "switch").WithArguments("_").WithLocation(3, 13),
Diagnostic(ErrorCode.WRN_SwitchExpressionNotExhaustiveForNull, "switch").WithArguments("[null]").WithLocation(3, 13),
// (18,13): warning CS8655: The switch expression does not handle some null inputs (it is not exhaustive). For example, the pattern 'null' is not covered.
// _ = new C() switch // 2
Diagnostic(ErrorCode.WRN_SwitchExpressionNotExhaustiveForNull, "switch").WithArguments("null").WithLocation(18, 13)
Expand All @@ -3956,12 +4032,11 @@ class C
public int Count => throw null;
public int this[int i] => throw null;
}";
// PROTOTYPE bad explanation
var comp = CreateCompilation(src);
comp.VerifyEmitDiagnostics(
// (2,13): warning CS8509: The switch expression does not handle all possible values of its input type (it is not exhaustive). For example, the pattern '_' is not covered.
// (2,13): warning CS8509: The switch expression does not handle all possible values of its input type (it is not exhaustive). For example, the pattern '[_, 0]' is not covered.
// _ = new C() switch // 1
Diagnostic(ErrorCode.WRN_SwitchExpressionNotExhaustive, "switch").WithArguments("_").WithLocation(2, 13)
Diagnostic(ErrorCode.WRN_SwitchExpressionNotExhaustive, "switch").WithArguments("[_, 0]").WithLocation(2, 13)
);
}

Expand All @@ -3981,12 +4056,11 @@ class C
public int Count => throw null;
public int this[int i] => throw null;
}";
// PROTOTYPE bad explanation
var comp = CreateCompilation(src);
comp.VerifyEmitDiagnostics(
// (2,13): warning CS8509: The switch expression does not handle all possible values of its input type (it is not exhaustive). For example, the pattern '_' is not covered.
// (2,13): warning CS8509: The switch expression does not handle all possible values of its input type (it is not exhaustive). For example, the pattern '[0, _]' is not covered.
// _ = new C() switch // 1
Diagnostic(ErrorCode.WRN_SwitchExpressionNotExhaustive, "switch").WithArguments("_").WithLocation(2, 13)
Diagnostic(ErrorCode.WRN_SwitchExpressionNotExhaustive, "switch").WithArguments("[0, _]").WithLocation(2, 13)
);
}

Expand All @@ -4006,12 +4080,11 @@ class C
public int Count => throw null;
public int this[int i] => throw null;
}";
// PROTOTYPE bad explanation
var comp = CreateCompilation(src);
comp.VerifyEmitDiagnostics(
// (2,13): warning CS8509: The switch expression does not handle all possible values of its input type (it is not exhaustive). For example, the pattern '_' is not covered.
// (2,13): warning CS8509: The switch expression does not handle all possible values of its input type (it is not exhaustive). For example, the pattern '[0]' is not covered.
// _ = new C() switch // 1
Diagnostic(ErrorCode.WRN_SwitchExpressionNotExhaustive, "switch").WithArguments("_").WithLocation(2, 13)
Diagnostic(ErrorCode.WRN_SwitchExpressionNotExhaustive, "switch").WithArguments("[0]").WithLocation(2, 13)
);
}

Expand Down