Skip to content

Commit

Permalink
Exhaustiveness checking tracks only non-null decision paths
Browse files Browse the repository at this point in the history
Addresses part of dotnet#30597
Later, in a separate PR, the null paths will be checked in the nullable walker
  • Loading branch information
gafter committed Nov 10, 2018
1 parent 69c39ed commit f548000
Show file tree
Hide file tree
Showing 7 changed files with 150 additions and 18 deletions.
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
*.userprefs
*.sln.docstates
.vs/
.dotnet/

# Build results
[Bb]inaries/
Expand Down
45 changes: 35 additions & 10 deletions src/Compilers/CSharp/Portable/Binder/SwitchExpressionBinder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -36,18 +36,17 @@ internal override BoundExpression BindSwitchExpressionCore(SwitchExpressionSynta
ImmutableArray<BoundSwitchExpressionArm> switchArms = BindSwitchExpressionArms(node, originalBinder, diagnostics);
TypeSymbol resultType = InferResultType(switchArms, diagnostics);
switchArms = AddConversionsToArms(switchArms, resultType, diagnostics);
bool hasErrors = CheckSwitchExpressionExhaustive(node, boundInputExpression, switchArms, out BoundDecisionDag decisionDag, out LabelSymbol defaultLabel, diagnostics);
bool reportedNonexhaustive = CheckSwitchExpressionExhaustive(node, boundInputExpression, switchArms, out BoundDecisionDag decisionDag, out LabelSymbol defaultLabel, diagnostics);

// When the input is constant, we use that to reshape the decision dag that is returned
// so that flow analysis will see that some of the cases may be unreachable.
decisionDag = decisionDag.SimplifyDecisionDagIfConstantInput(boundInputExpression);

return new BoundSwitchExpression(node, boundInputExpression, switchArms, decisionDag, defaultLabel, resultType, hasErrors);
return new BoundSwitchExpression(node, boundInputExpression, switchArms, decisionDag, defaultLabel, reportedNonexhaustive, resultType);
}

/// <summary>
/// Build the decision dag, giving an error if some cases are subsumed and an error if the switch expression is not exhaustive.
/// Returns true if there were errors.
/// Returns true if there was a non-exhaustive warning reported.
/// </summary>
/// <param name="node"></param>
/// <param name="boundInputExpression"></param>
Expand All @@ -74,19 +73,45 @@ private bool CheckSwitchExpressionExhaustive(
}
}

bool exhaustive = !reachableLabels.Contains(defaultLabel);
if (exhaustive)
if (!reachableLabels.Contains(defaultLabel))
{
// switch expression is exhaustive; no default label needed.
defaultLabel = null;
return false;
}
else

// We only report exhaustive warnings when the default label is reachable through some series of
// tests that do not include a test in which the value is know to be null. Handling paths with
// nulls is the job of the nullable walker.
foreach (var n in TopologicalSort.IterativeSort<BoundDecisionDagNode>(new[] { decisionDag.RootNode }, nonNullSuccessors))
{
// warning: switch expression is not exhaustive
diagnostics.Add(ErrorCode.WRN_SwitchExpressionNotExhaustive, node.SwitchKeyword.GetLocation());
if (n is BoundLeafDecisionDagNode leaf && leaf.Label == defaultLabel)
{
diagnostics.Add(ErrorCode.WRN_SwitchExpressionNotExhaustive, node.SwitchKeyword.GetLocation());
return true;
}
}

return !exhaustive;
return false;

ImmutableArray<BoundDecisionDagNode> nonNullSuccessors(BoundDecisionDagNode n)
{
switch (n)
{
case BoundTestDecisionDagNode p:
switch (p.Test)
{
case BoundDagNonNullTest t: // checks that the input is not null
return ImmutableArray.Create(p.WhenTrue);
case BoundDagNullTest t: // checks that the input is null
return ImmutableArray.Create(p.WhenFalse);
default:
return BoundDecisionDag.Successors(n);
}
default:
return BoundDecisionDag.Successors(n);
}
}
}

/// <summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ internal partial class BoundDecisionDag
private ImmutableHashSet<LabelSymbol> _reachableLabels;
private ImmutableArray<BoundDecisionDagNode> _topologicallySortedNodes;

private static ImmutableArray<BoundDecisionDagNode> Successors(BoundDecisionDagNode node)
internal static ImmutableArray<BoundDecisionDagNode> Successors(BoundDecisionDagNode node)
{
switch (node)
{
Expand Down
1 change: 1 addition & 0 deletions src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml
Original file line number Diff line number Diff line change
Expand Up @@ -1149,6 +1149,7 @@
<Field Name="SwitchArms" Type="ImmutableArray&lt;BoundSwitchExpressionArm&gt;"/>
<Field Name="DecisionDag" Type="BoundDecisionDag" Null="disallow" SkipInVisitor="true"/>
<Field Name="DefaultLabel" Type="LabelSymbol" Null="allow"/>
<Field Name="ReportedNotExhaustive" Type="bool"/>
</Node>
<Node Name="BoundSwitchExpressionArm" Base="BoundNode">
<Field Name="Locals" Type="ImmutableArray&lt;LocalSymbol&gt;"/>
Expand Down
2 changes: 0 additions & 2 deletions src/Compilers/CSharp/Portable/Errors/ErrorCode.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1662,7 +1662,5 @@ internal enum ErrorCode
#endregion diagnostics introduced for C# 8.0

// Note: you will need to re-generate compiler code after adding warnings (build\scripts\generate-compiler-code.cmd)

// Note: you will need to re-generate compiler code after adding warnings (build\scripts\generate-compiler-code.cmd)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -4102,7 +4102,7 @@ public BoundConditionalGoto Update(BoundExpression condition, bool jumpIfTrue, L

internal sealed partial class BoundSwitchExpression : BoundExpression
{
public BoundSwitchExpression(SyntaxNode syntax, BoundExpression expression, ImmutableArray<BoundSwitchExpressionArm> switchArms, BoundDecisionDag decisionDag, LabelSymbol defaultLabel, TypeSymbol type, bool hasErrors = false)
public BoundSwitchExpression(SyntaxNode syntax, BoundExpression expression, ImmutableArray<BoundSwitchExpressionArm> switchArms, BoundDecisionDag decisionDag, LabelSymbol defaultLabel, bool reportedNotExhaustive, TypeSymbol type, bool hasErrors = false)
: base(BoundKind.SwitchExpression, syntax, type, hasErrors || expression.HasErrors() || switchArms.HasErrors() || decisionDag.HasErrors())
{

Expand All @@ -4114,6 +4114,7 @@ public BoundSwitchExpression(SyntaxNode syntax, BoundExpression expression, Immu
this.SwitchArms = switchArms;
this.DecisionDag = decisionDag;
this.DefaultLabel = defaultLabel;
this.ReportedNotExhaustive = reportedNotExhaustive;
}


Expand All @@ -4125,16 +4126,18 @@ public BoundSwitchExpression(SyntaxNode syntax, BoundExpression expression, Immu

public LabelSymbol DefaultLabel { get; }

public bool ReportedNotExhaustive { get; }

public override BoundNode Accept(BoundTreeVisitor visitor)
{
return visitor.VisitSwitchExpression(this);
}

public BoundSwitchExpression Update(BoundExpression expression, ImmutableArray<BoundSwitchExpressionArm> switchArms, BoundDecisionDag decisionDag, LabelSymbol defaultLabel, TypeSymbol type)
public BoundSwitchExpression Update(BoundExpression expression, ImmutableArray<BoundSwitchExpressionArm> switchArms, BoundDecisionDag decisionDag, LabelSymbol defaultLabel, bool reportedNotExhaustive, TypeSymbol type)
{
if (expression != this.Expression || switchArms != this.SwitchArms || decisionDag != this.DecisionDag || defaultLabel != this.DefaultLabel || type != this.Type)
if (expression != this.Expression || switchArms != this.SwitchArms || decisionDag != this.DecisionDag || defaultLabel != this.DefaultLabel || reportedNotExhaustive != this.ReportedNotExhaustive || type != this.Type)
{
var result = new BoundSwitchExpression(this.Syntax, expression, switchArms, decisionDag, defaultLabel, type, this.HasErrors);
var result = new BoundSwitchExpression(this.Syntax, expression, switchArms, decisionDag, defaultLabel, reportedNotExhaustive, type, this.HasErrors);
result.WasCompilerGenerated = this.WasCompilerGenerated;
return result;
}
Expand Down Expand Up @@ -10684,7 +10687,7 @@ public override BoundNode VisitSwitchExpression(BoundSwitchExpression node)
ImmutableArray<BoundSwitchExpressionArm> switchArms = (ImmutableArray<BoundSwitchExpressionArm>)this.VisitList(node.SwitchArms);
BoundDecisionDag decisionDag = node.DecisionDag;
TypeSymbol type = this.VisitType(node.Type);
return node.Update(expression, switchArms, decisionDag, node.DefaultLabel, type);
return node.Update(expression, switchArms, decisionDag, node.DefaultLabel, node.ReportedNotExhaustive, type);
}
public override BoundNode VisitSwitchExpressionArm(BoundSwitchExpressionArm node)
{
Expand Down Expand Up @@ -12186,6 +12189,7 @@ public override TreeDumperNode VisitSwitchExpression(BoundSwitchExpression node,
new TreeDumperNode("switchArms", null, from x in node.SwitchArms select Visit(x, null)),
new TreeDumperNode("decisionDag", null, new TreeDumperNode[] { Visit(node.DecisionDag, null) }),
new TreeDumperNode("defaultLabel", node.DefaultLabel, null),
new TreeDumperNode("reportedNotExhaustive", node.ReportedNotExhaustive, null),
new TreeDumperNode("type", node.Type, null)
}
);
Expand Down
103 changes: 103 additions & 0 deletions src/Compilers/CSharp/Test/Semantic/Semantics/PatternMatchingTests4.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1384,5 +1384,108 @@ class _
);
CompileAndVerify(compilation, expectedOutput: "8");
}

[Fact]
public void IgnoreNullInExhaustiveness_01()
{
var source =
@"class Program
{
static void Main() {}
static int M1(bool? b1, bool? b2)
{
return (b1, b2) switch {
(false, false) => 1,
(false, true) => 2,
// (true, false) => 3,
(true, true) => 4
};
}
}
";
var compilation = CreatePatternCompilation(source);
compilation.VerifyDiagnostics(
// (6,25): warning CS8509: The switch expression does not handle all possible inputs (it is not exhaustive).
// return (b1, b2) switch {
Diagnostic(ErrorCode.WRN_SwitchExpressionNotExhaustive, "switch").WithLocation(6, 25)
);
}

[Fact]
public void IgnoreNullInExhaustiveness_02()
{
var source =
@"class Program
{
static void Main() {}
static int M1(bool? b1, bool? b2)
{
return (b1, b2) switch {
(false, false) => 1,
(false, true) => 2,
(true, false) => 3,
(true, true) => 4
};
}
}
";
var compilation = CreatePatternCompilation(source);
compilation.VerifyDiagnostics(
);
}

[Fact]
public void IgnoreNullInExhaustiveness_03()
{
var source =
@"class Program
{
static void Main() {}
static int M1(bool? b1, bool? b2)
{
(bool? b1, bool? b2)? cond = (b1, b2);
return cond switch {
(false, false) => 1,
(false, true) => 2,
(true, false) => 3,
(true, true) => 4,
(null, true) => 5
};
}
}
";
var compilation = CreatePatternCompilation(source);
compilation.VerifyDiagnostics(
);
}

[Fact]
public void IgnoreNullInExhaustiveness_04()
{
var source =
@"class Program
{
static void Main() {}
static int M1(bool? b1, bool? b2)
{
(bool? b1, bool? b2)? cond = (b1, b2);
return cond switch {
(false, false) => 1,
(false, true) => 2,
(true, false) => 3,
(true, true) => 4,
_ => 5,
(null, true) => 6
};
}
}
";
var compilation = CreatePatternCompilation(source);
compilation.VerifyDiagnostics(
// (13,13): error CS8510: The pattern has already been handled by a previous arm of the switch expression.
// (null, true) => 6
Diagnostic(ErrorCode.ERR_SwitchArmSubsumed, "(null, true)").WithLocation(13, 13)
);
}
}
}

0 comments on commit f548000

Please sign in to comment.