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

Warn for is-pattern using a relational pattern with a known result. #42501

Merged
merged 9 commits into from
Mar 26, 2020
Merged

Warn for is-pattern using a relational pattern with a known result. #42501

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
8e5126d
Warn for is-pattern using a relational pattern with a known result.
Mar 17, 2020
81d5459
Merge branch 'features/patterns3' of https://github.com/dotnet/roslyn…
Mar 18, 2020
c02a2c7
Merge branch 'features/patterns3' of https://github.com/dotnet/roslyn…
Mar 20, 2020
7d4d669
Fixup after merge from features/patterns3
Mar 20, 2020
702515a
Adjust subsumption diagnostics for clarity given that patterns can be…
Mar 24, 2020
b30b15a
Update xlf files.
Mar 24, 2020
21e3571
Merge branch 'features/patterns3' of https://github.com/dotnet/roslyn…
Mar 25, 2020
e381464
Expand the set of is-pattern forms that result in an always-matched w…
Mar 25, 2020
149ffce
Don't warn for is-pattern with a discard.
Mar 25, 2020
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
48 changes: 42 additions & 6 deletions src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ private BoundExpression MakeIsPatternExpression(
bool hasErrors,
DiagnosticBag diagnostics)
{
// Note that these labels are for the convenience of the compilation of patterns, and are not actually emitted into the lowered code.
// Note that these labels are for the convenience of the compilation of patterns, and are not necessarily emitted into the lowered code.
LabelSymbol whenTrueLabel = new GeneratedLabelSymbol("isPatternSuccess");
LabelSymbol whenFalseLabel = new GeneratedLabelSymbol("isPatternFailure");
BoundDecisionDag decisionDag = DecisionDagBuilder.CreateDecisionDagForIsPattern(
Expand All @@ -61,8 +61,31 @@ private BoundExpression MakeIsPatternExpression(
diagnostics.Add(ErrorCode.ERR_IsPatternImpossible, node.Location, expression.Type);
hasErrors = true;
}

if (expression.ConstantValue != null)
else if (!hasErrors && !decisionDag.ReachableLabels.Contains(whenFalseLabel))
{
switch (pattern)
{
case BoundConstantPattern _:
case BoundITuplePattern _:
// these patterns can fail in practice
throw ExceptionUtilities.Unreachable;
case BoundRelationalPattern _:
case BoundTypePattern _:
case BoundNegatedPattern _:
case BoundBinaryPattern _:
diagnostics.Add(ErrorCode.WRN_IsPatternAlways, node.Location, expression.Type);
break;
case BoundDiscardPattern _:
// we do not give a warning on this because it is an existing scenario, and it should
// have been obvious in source that it would always match.
break;
case BoundDeclarationPattern _:
case BoundRecursivePattern _:
// We do not give a warning on these because people do this to give a name to a value
break;
}
}
else if (expression.ConstantValue != null)
{
decisionDag = decisionDag.SimplifyDecisionDagIfConstantInput(expression);
if (!hasErrors)
Expand All @@ -71,9 +94,21 @@ private BoundExpression MakeIsPatternExpression(
{
diagnostics.Add(ErrorCode.WRN_GivenExpressionNeverMatchesPattern, node.Location);
}
else if (!decisionDag.ReachableLabels.Contains(whenFalseLabel) && pattern.Kind == BoundKind.ConstantPattern)
else if (!decisionDag.ReachableLabels.Contains(whenFalseLabel))
{
diagnostics.Add(ErrorCode.WRN_GivenExpressionAlwaysMatchesConstant, node.Location);
switch (pattern)
{
case BoundConstantPattern _:
diagnostics.Add(ErrorCode.WRN_GivenExpressionAlwaysMatchesConstant, node.Location);
break;
case BoundRelationalPattern _:
case BoundTypePattern _:
case BoundNegatedPattern _:
case BoundBinaryPattern _:
case BoundDiscardPattern _:
diagnostics.Add(ErrorCode.WRN_GivenExpressionAlwaysMatchesPattern, node.Location);
break;
}
}
}
}
Expand Down Expand Up @@ -189,7 +224,8 @@ private BoundExpression BindExpressionOrTypeForPattern(
}
else
{
Debug.Assert(expression.Kind == BoundKind.TypeExpression);
Debug.Assert(expression is { Kind: BoundKind.TypeExpression, Type: { } });
hasErrors |= CheckValidPatternType(patternExpression, inputType, expression.Type, patternTypeWasInSource: true, diagnostics: diagnostics);
return expression;
}
}
Expand Down
13 changes: 12 additions & 1 deletion src/Compilers/CSharp/Portable/Binder/DecisionDagBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -611,9 +611,20 @@ private Tests MakeTestsAndBindingsForRelationalPattern(
BoundRelationalPattern rel,
out BoundDagTemp output)
{
// check if the test is always true or always false
var tests = ArrayBuilder<Tests>.GetInstance(2);
output = MakeConvertToType(input, rel.Syntax, rel.Value.Type!, isExplicitTest: false, tests);
tests.Add(new Tests.One(new BoundDagRelationalTest(rel.Syntax, rel.Relation, rel.ConstantValue, output, rel.HasErrors)));
var fac = ValueSetFactory.ForSpecialType(input.Type.SpecialType);
var values = fac?.Related(rel.Relation.Operator(), rel.ConstantValue);
if (values?.IsEmpty == true)
{
tests.Add(Tests.False.Instance);
}
else if (values?.Complement().IsEmpty != true)
{
tests.Add(new Tests.One(new BoundDagRelationalTest(rel.Syntax, rel.Relation, rel.ConstantValue, output, rel.HasErrors)));
}
Copy link
Member

@cston cston Mar 20, 2020 •
edited by gafter
Loading

Choose a reason for hiding this comment

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

Consider wrapping block in if (fac is { }) { ... } to avoid multiple ?. operators and == true. #ByDesign

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried that, but ended up having to repeat line 625 in multiple else clauses.

In reply to: 395854215 [](ancestors = 395854215)


return Tests.AndSequence.Create(tests);
}

Expand Down
6 changes: 6 additions & 0 deletions src/Compilers/CSharp/Portable/BoundTree/BoundDecisionDag.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,12 @@ BoundDecisionDagNode makeReplacement(BoundDecisionDagNode dag, Func<BoundDecisio
return d.Value == inputConstant;
case BoundDagTypeTest d:
return inputConstant.IsNull ? (bool?)false : null;
case BoundDagRelationalTest d:
var f = ValueSetFactory.ForSpecialType(input.Type.SpecialType);
if (f is null) return null;
// TODO: When ValueSetFactory has a method for comparing two values, use it.
Copy link
Member

@cston cston Mar 20, 2020 •
edited by gafter
Loading

Choose a reason for hiding this comment

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

TODO [](start = 31, length = 4)

PROTOTYPE? #ByDesign

Copy link
Member Author

Choose a reason for hiding this comment

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

No. The code works correctly as is. This is a suggestion for a future optimization, which I expect to happen after merging into master.

In reply to: 395854439 [](ancestors = 395854439)

Copy link
Member

@jcouv jcouv Mar 24, 2020 •
edited by gafter
Loading

Choose a reason for hiding this comment

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

Should this be also filed as a follow-up issue? #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

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

That wouldn't be particularly helpful to me, though I don't object to it.

In reply to: 397252372 [](ancestors = 397252372)

var set = f.Related(d.Relation.Operator(), d.Value);
return set.Any(BinaryOperatorKind.Equal, inputConstant);
default:
throw ExceptionUtilities.UnexpectedValue(choice);
}
Expand Down
16 changes: 14 additions & 2 deletions src/Compilers/CSharp/Portable/CSharpResources.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5090,13 +5090,13 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<value>The switch expression must be a value; found '{0}'.</value>
</data>
<data name="ERR_SwitchCaseSubsumed" xml:space="preserve">
<value>The switch case has already been handled by a previous case.</value>
<value>The switch case is unreachable. It has already been handled by a previous case or it is impossible to match.</value>
</data>
<data name="ERR_StdInOptionProvidedButConsoleInputIsNotRedirected" xml:space="preserve">
<value>stdin argument '-' is specified, but input has not been redirected from the standard input stream.</value>
</data>
<data name="ERR_SwitchArmSubsumed" xml:space="preserve">
<value>The pattern has already been handled by a previous arm of the switch expression.</value>
<value>The pattern is unreachable. It has already been handled by a previous arm of the switch expression or it is impossible to match.</value>
</data>
<data name="ERR_PatternWrongType" xml:space="preserve">
<value>An expression of type '{0}' cannot be handled by a pattern of type '{1}'.</value>
Expand Down Expand Up @@ -5854,6 +5854,12 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<data name="ERR_IsPatternImpossible" xml:space="preserve">
<value>An expression of type '{0}' can never match the provided pattern.</value>
</data>
<data name="WRN_IsPatternAlways" xml:space="preserve">
<value>An expression of type '{0}' always matches the provided pattern.</value>
</data>
<data name="WRN_IsPatternAlways_Title" xml:space="preserve">
<value>The input always matches the provided pattern.</value>
</data>
<data name="WRN_GivenExpressionNeverMatchesPattern" xml:space="preserve">
<value>The given expression never matches the provided pattern.</value>
</data>
Expand All @@ -5866,6 +5872,12 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<data name="WRN_GivenExpressionAlwaysMatchesConstant_Title" xml:space="preserve">
<value>The given expression always matches the provided constant.</value>
</data>
<data name="WRN_GivenExpressionAlwaysMatchesPattern" xml:space="preserve">
<value>The given expression always matches the provided pattern.</value>
</data>
<data name="WRN_GivenExpressionAlwaysMatchesPattern_Title" xml:space="preserve">
<value>The given expression always matches the provided pattern.</value>
</data>
<data name="ERR_FeatureNotAvailableInVersion8_0" xml:space="preserve">
<value>Feature '{0}' is not available in C# 8.0. Please use language version {1} or greater.</value>
</data>
Expand Down
2 changes: 2 additions & 0 deletions src/Compilers/CSharp/Portable/Errors/ErrorCode.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1775,6 +1775,8 @@ internal enum ErrorCode
ERR_ExpressionTreeContainsPatternIndexOrRangeIndexer = 8790,
ERR_ExpressionTreeContainsFromEndIndexExpression = 8791,
ERR_ExpressionTreeContainsRangeExpression = 8792,
WRN_GivenExpressionAlwaysMatchesPattern = 8793,
WRN_IsPatternAlways = 8794,

#endregion diagnostics introduced for C# 9.0

Expand Down
2 changes: 2 additions & 0 deletions src/Compilers/CSharp/Portable/Errors/ErrorFacts.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -450,6 +450,8 @@ internal static int GetWarningLevel(ErrorCode code)
case ErrorCode.WRN_MemberNotNull:
case ErrorCode.WRN_MemberNotNullBadMember:
case ErrorCode.WRN_MemberNotNullWhen:
case ErrorCode.WRN_GivenExpressionAlwaysMatchesPattern:
case ErrorCode.WRN_IsPatternAlways:
return 1;
default:
return 0;
Expand Down
2 changes: 2 additions & 0 deletions src/Compilers/CSharp/Portable/Generated/ErrorFacts.Generated.cs
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

16 changes: 8 additions & 8 deletions src/Compilers/CSharp/Portable/Utilities/ValueSetFactory.NumericValueSet.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,19 +62,19 @@ static bool anyInterval(Interval interval, BinaryOperatorKind relation, T value,
case Interval.Mixed mixed:
switch (relation)
{
case LessThan when tc.Related(LessThan, value, minValue):
case LessThan when tc.Related(LessThanOrEqual, value, minValue):
return false;
case LessThan when tc.Related(LessThan, maxValue, value):
return true;
case GreaterThan when tc.Related(GreaterThan, value, maxValue):
case GreaterThan when tc.Related(GreaterThanOrEqual, value, maxValue):
return false;
case GreaterThan when tc.Related(GreaterThan, minValue, value):
return true;
case LessThanOrEqual when tc.Related(LessThanOrEqual, value, minValue):
case LessThanOrEqual when tc.Related(LessThan, value, minValue):
return false;
case LessThanOrEqual when tc.Related(LessThanOrEqual, maxValue, value):
return true;
case GreaterThanOrEqual when tc.Related(GreaterThanOrEqual, value, maxValue):
case GreaterThanOrEqual when tc.Related(GreaterThan, value, maxValue):
return false;
case GreaterThanOrEqual when tc.Related(GreaterThanOrEqual, minValue, value):
return true;
Expand Down Expand Up @@ -127,19 +127,19 @@ static bool allInterval(Interval interval, BinaryOperatorKind relation, T value,
case Interval.Mixed mixed:
switch (relation)
{
case LessThan when tc.Related(LessThan, value, minValue):
case LessThan when tc.Related(LessThanOrEqual, value, minValue):
return false;
case LessThan when tc.Related(LessThan, maxValue, value):
return true;
case GreaterThan when tc.Related(GreaterThan, value, maxValue):
case GreaterThan when tc.Related(GreaterThanOrEqual, value, maxValue):
return false;
case GreaterThan when tc.Related(GreaterThan, minValue, value):
return true;
case LessThanOrEqual when tc.Related(LessThanOrEqual, value, minValue):
case LessThanOrEqual when tc.Related(LessThan, value, minValue):
return false;
case LessThanOrEqual when tc.Related(LessThanOrEqual, maxValue, value):
return true;
case GreaterThanOrEqual when tc.Related(GreaterThanOrEqual, value, maxValue):
case GreaterThanOrEqual when tc.Related(GreaterThan, value, maxValue):
return false;
case GreaterThanOrEqual when tc.Related(GreaterThanOrEqual, minValue, value):
return true;
Expand Down
28 changes: 24 additions & 4 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.cs.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -473,13 +473,13 @@
<note />
</trans-unit>
<trans-unit id="ERR_SwitchArmSubsumed">
<source>The pattern has already been handled by a previous arm of the switch expression.</source>
<target state="translated">Tento vzor už zpracovala předchozí větev výrazu switch.</target>
<source>The pattern is unreachable. It has already been handled by a previous arm of the switch expression or it is impossible to match.</source>
<target state="needs-review-translation">Tento vzor už zpracovala předchozí větev výrazu switch.</target>
<note />
</trans-unit>
<trans-unit id="ERR_SwitchCaseSubsumed">
<source>The switch case has already been handled by a previous case.</source>
<target state="translated">Větev příkazu switch se už zpracovala předchozí větví.</target>
<source>The switch case is unreachable. It has already been handled by a previous case or it is impossible to match.</source>
<target state="needs-review-translation">Větev příkazu switch se už zpracovala předchozí větví.</target>
<note />
</trans-unit>
<trans-unit id="ERR_SwitchExpressionNoBestType">
Expand Down Expand Up @@ -1514,6 +1514,16 @@
<target state="translated">Daný výraz vždy odpovídá zadané konstantě.</target>
<note />
</trans-unit>
<trans-unit id="WRN_GivenExpressionAlwaysMatchesPattern">
<source>The given expression always matches the provided pattern.</source>
<target state="new">The given expression always matches the provided pattern.</target>
<note />
</trans-unit>
<trans-unit id="WRN_GivenExpressionAlwaysMatchesPattern_Title">
<source>The given expression always matches the provided pattern.</source>
<target state="new">The given expression always matches the provided pattern.</target>
<note />
</trans-unit>
<trans-unit id="WRN_GivenExpressionNeverMatchesPattern">
<source>The given expression never matches the provided pattern.</source>
<target state="translated">Daný výraz nikdy neodpovídá zadané konstantě.</target>
Expand All @@ -1534,6 +1544,16 @@
<target state="translated">Volání člena, který nemá modifikátor readonly, ze člena s modifikátorem readonly má za následek implicitní kopii.</target>
<note />
</trans-unit>
<trans-unit id="WRN_IsPatternAlways">
<source>An expression of type '{0}' always matches the provided pattern.</source>
<target state="new">An expression of type '{0}' always matches the provided pattern.</target>
<note />
</trans-unit>
<trans-unit id="WRN_IsPatternAlways_Title">
<source>The input always matches the provided pattern.</source>
<target state="new">The input always matches the provided pattern.</target>
<note />
</trans-unit>
<trans-unit id="WRN_IsTypeNamedUnderscore">
<source>The name '_' refers to the type '{0}', not the discard pattern. Use '@_' for the type, or 'var _' to discard.</source>
<target state="translated">Název „_“ odkazuje na typ {0}, ne vzor discard. Použijte „@_“ pro tento typ nebo „var _“ pro zahození.</target>
Expand Down
Loading