Skip to content

Commit

Permalink
Block constant and relational pattern matching on types inheriting fr…
Browse files Browse the repository at this point in the history
…om or constrained to INumberBase<T>

To ensure we have a good user experience and avoid any potential breaking changes later, we block this scenario for types that don't have known conversions from patterns.

Spec change: dotnet/csharplang#6273.
  • Loading branch information
333fred committed Jul 14, 2022
1 parent 8d0ca82 commit 5b2a8d1
Show file tree
Hide file tree
Showing 23 changed files with 441 additions and 18 deletions.
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Binder/Binder_Operators.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3187,7 +3187,7 @@ private BoundExpression BindIsOperator(BinaryExpressionSyntax node, BindingDiagn
}

bool hasErrors = node.Right.HasErrors;
var convertedExpression = BindExpressionForPattern(operand.Type, node.Right, ref hasErrors, isPatternDiagnostics, out var constantValueOpt, out var wasExpression);
var convertedExpression = BindExpressionForPattern(operand.Type, node.Right, ref hasErrors, isPatternDiagnostics, out var constantValueOpt, out var wasExpression, out _);
if (wasExpression)
{
hasErrors |= constantValueOpt is null;
Expand Down
77 changes: 65 additions & 12 deletions src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs
Original file line number Diff line number Diff line change
Expand Up @@ -408,7 +408,7 @@ private static BoundPattern BindDiscardPattern(DiscardPatternSyntax node, TypeSy
BindingDiagnosticBag diagnostics)
{
ExpressionSyntax innerExpression = SkipParensAndNullSuppressions(expression, diagnostics, ref hasErrors);
var convertedExpression = BindExpressionOrTypeForPattern(inputType, innerExpression, ref hasErrors, diagnostics, out var constantValueOpt, out bool wasExpression);
var convertedExpression = BindExpressionOrTypeForPattern(inputType, innerExpression, ref hasErrors, diagnostics, out var constantValueOpt, out bool wasExpression, out Conversion patternConversion);
if (wasExpression)
{
var convertedType = convertedExpression.Type ?? inputType;
Expand All @@ -417,6 +417,12 @@ private static BoundPattern BindDiscardPattern(DiscardPatternSyntax node, TypeSy
convertedType = inputType;
}

if (HasBlockedINumberConversion(patternConversion, inputType))
{
// Cannot use a numeric constant or relational pattern on '{0}' because it inherits from or extends 'INumberBase&lt;T&gt;'. Consider using a type pattern to narrow to a specific numeric type.
diagnostics.Add(ErrorCode.ERR_CannotMatchOnINumberBase, node.Location, inputType);
}

return new BoundConstantPattern(
node, convertedExpression, constantValueOpt ?? ConstantValue.Bad, inputType, convertedType, hasErrors || constantValueOpt is null);
}
Expand All @@ -431,6 +437,29 @@ private static BoundPattern BindDiscardPattern(DiscardPatternSyntax node, TypeSy
}
}

private bool HasBlockedINumberConversion(Conversion patternConversion, TypeSymbol inputType)
{
// We want to block constant and relation patterns that have an input type constrained to or inherited from INumberBase<T>, if we don't
// know how to represent the constant being matched against in the input type. For example, `1.0 is 1` will work when written inline, but
// will fail if the input type is `INumberBase<T>`. We block this now so that we can make make it work as expected in the future without
// being a breaking change.

if (patternConversion.IsIdentity || patternConversion.IsConstantExpression || patternConversion.IsNumeric)
{
return false;
}

var iNumberBase = Compilation.GetWellKnownType(WellKnownType.System_Numerics_INumberBase_T);

if (iNumberBase.IsErrorType())
{
return false;
}

var interfaces = inputType is TypeParameterSymbol typeParam ? typeParam.EffectiveInterfacesNoUseSiteDiagnostics : inputType.AllInterfacesNoUseSiteDiagnostics;
return interfaces.Any(static (i, iNumberBase) => i.OriginalDefinition.Equals(iNumberBase), iNumberBase);
}

private static ExpressionSyntax SkipParensAndNullSuppressions(ExpressionSyntax e, BindingDiagnosticBag diagnostics, ref bool hasErrors)
{
while (true)
Expand Down Expand Up @@ -465,19 +494,21 @@ private static ExpressionSyntax SkipParensAndNullSuppressions(ExpressionSyntax e
ref bool hasErrors,
BindingDiagnosticBag diagnostics,
out ConstantValue? constantValueOpt,
out bool wasExpression)
out bool wasExpression,
out Conversion patternExpressionConversion)
{
constantValueOpt = null;
BoundExpression expression = BindTypeOrRValue(patternExpression, diagnostics);
wasExpression = expression.Kind != BoundKind.TypeExpression;
if (wasExpression)
{
return BindExpressionForPatternContinued(expression, inputType, patternExpression, ref hasErrors, diagnostics, out constantValueOpt);
return BindExpressionForPatternContinued(expression, inputType, patternExpression, ref hasErrors, diagnostics, out constantValueOpt, out patternExpressionConversion);
}
else
{
Debug.Assert(expression is { Kind: BoundKind.TypeExpression, Type: { } });
hasErrors |= CheckValidPatternType(patternExpression, inputType, expression.Type, diagnostics: diagnostics);
patternExpressionConversion = Conversion.NoConversion;
return expression;
}
}
Expand All @@ -491,13 +522,15 @@ private static ExpressionSyntax SkipParensAndNullSuppressions(ExpressionSyntax e
ref bool hasErrors,
BindingDiagnosticBag diagnostics,
out ConstantValue? constantValueOpt,
out bool wasExpression)
out bool wasExpression,
out Conversion patternExpressionConversion)
{
constantValueOpt = null;
var expression = BindExpression(patternExpression, diagnostics: diagnostics, invoked: false, indexed: false);
expression = CheckValue(expression, BindValueKind.RValue, diagnostics);
wasExpression = expression.Kind switch { BoundKind.BadExpression => false, BoundKind.TypeExpression => false, _ => true };
return wasExpression ? BindExpressionForPatternContinued(expression, inputType, patternExpression, ref hasErrors, diagnostics, out constantValueOpt) : expression;
patternExpressionConversion = Conversion.NoConversion;
return wasExpression ? BindExpressionForPatternContinued(expression, inputType, patternExpression, ref hasErrors, diagnostics, out constantValueOpt, out patternExpressionConversion) : expression;
}

private BoundExpression BindExpressionForPatternContinued(
Expand All @@ -506,10 +539,11 @@ private static ExpressionSyntax SkipParensAndNullSuppressions(ExpressionSyntax e
ExpressionSyntax patternExpression,
ref bool hasErrors,
BindingDiagnosticBag diagnostics,
out ConstantValue? constantValueOpt)
out ConstantValue? constantValueOpt,
out Conversion patternExpressionConversion)
{
BoundExpression convertedExpression = ConvertPatternExpression(
inputType, patternExpression, expression, out constantValueOpt, hasErrors, diagnostics);
inputType, patternExpression, expression, out constantValueOpt, hasErrors, diagnostics, out patternExpressionConversion);

ConstantValueUtils.CheckLangVersionForConstantValue(convertedExpression, diagnostics);

Expand Down Expand Up @@ -541,7 +575,8 @@ private static ExpressionSyntax SkipParensAndNullSuppressions(ExpressionSyntax e
BoundExpression expression,
out ConstantValue? constantValue,
bool hasErrors,
BindingDiagnosticBag diagnostics)
BindingDiagnosticBag diagnostics,
out Conversion patternExpressionConversion)
{
BoundExpression convertedExpression;

Expand Down Expand Up @@ -577,16 +612,24 @@ private static ExpressionSyntax SkipParensAndNullSuppressions(ExpressionSyntax e
if (!hasErrors)
{
var requiredVersion = MessageID.IDS_FeatureRecursivePatterns.RequiredVersion();
if (Compilation.LanguageVersion < requiredVersion &&
!this.Conversions.ClassifyConversionFromExpression(expression, inputType, isChecked: CheckOverflowAtRuntime, ref useSiteInfo).IsImplicit)
patternExpressionConversion = this.Conversions.ClassifyConversionFromExpression(expression, inputType, isChecked: CheckOverflowAtRuntime, ref useSiteInfo);
if (Compilation.LanguageVersion < requiredVersion && !patternExpressionConversion.IsImplicit)
{
diagnostics.Add(ErrorCode.ERR_ConstantPatternVsOpenType,
expression.Syntax.Location, inputType, expression.Display, new CSharpRequiredLanguageVersion(requiredVersion));
}
}
else
{
patternExpressionConversion = Conversion.NoConversion;
}

diagnostics.Add(node, useSiteInfo);
}
else
{
patternExpressionConversion = Conversion.NoConversion;
}
}
else
{
Expand All @@ -613,13 +656,16 @@ private static ExpressionSyntax SkipParensAndNullSuppressions(ExpressionSyntax e
{
diagnostics.Add(ErrorCode.ERR_PatternSpanCharCannotBeStringNull, convertedExpression.Syntax.Location, inputType);
}

patternExpressionConversion = Conversion.NoConversion;

return convertedExpression;
}

// This will allow user-defined conversions, even though they're not permitted here. This is acceptable
// because the result of a user-defined conversion does not have a ConstantValue. A constant pattern
// requires a constant value so we'll report a diagnostic to that effect later.
convertedExpression = GenerateConversionForAssignment(inputType, expression, diagnostics);
convertedExpression = GenerateConversionForAssignment(inputType, expression, diagnostics, out patternExpressionConversion);

if (convertedExpression.Kind == BoundKind.Conversion)
{
Expand Down Expand Up @@ -1578,7 +1624,7 @@ void addSubpatternsForTuple(ImmutableArray<TypeWithAnnotations> elementTypes)
bool hasErrors,
BindingDiagnosticBag diagnostics)
{
BoundExpression value = BindExpressionForPattern(inputType, node.Expression, ref hasErrors, diagnostics, out var constantValueOpt, out _);
BoundExpression value = BindExpressionForPattern(inputType, node.Expression, ref hasErrors, diagnostics, out var constantValueOpt, out _, out Conversion patternConversion);
ExpressionSyntax innerExpression = SkipParensAndNullSuppressions(node.Expression, diagnostics, ref hasErrors);
RoslynDebug.Assert(value.Type is { });
BinaryOperatorKind operation = tokenKindToBinaryOperatorKind(node.OperatorToken.Kind());
Expand Down Expand Up @@ -1616,6 +1662,13 @@ void addSubpatternsForTuple(ImmutableArray<TypeWithAnnotations> elementTypes)
constantValueOpt = ConstantValue.Bad;
}

if (!hasErrors && HasBlockedINumberConversion(patternConversion, inputType))
{
// Cannot use a numeric constant or relational pattern on '{0}' because it inherits from or extends 'INumberBase&lt;T&gt;'. Consider using a type pattern to narrow to a specific numeric type.
diagnostics.Add(ErrorCode.ERR_CannotMatchOnINumberBase, node.Location, inputType);
hasErrors = true;
}

return new BoundRelationalPattern(node, operation | opType, value, constantValueOpt, inputType, value.Type, hasErrors);

static BinaryOperatorKind tokenKindToBinaryOperatorKind(SyntaxKind kind) => kind switch
Expand Down
5 changes: 4 additions & 1 deletion src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1870,6 +1870,9 @@ internal enum ConversionForAssignmentFlags
}

internal BoundExpression GenerateConversionForAssignment(TypeSymbol targetType, BoundExpression expression, BindingDiagnosticBag diagnostics, ConversionForAssignmentFlags flags = ConversionForAssignmentFlags.None)
=> GenerateConversionForAssignment(targetType, expression, diagnostics, out _, flags);

internal BoundExpression GenerateConversionForAssignment(TypeSymbol targetType, BoundExpression expression, BindingDiagnosticBag diagnostics, out Conversion conversion, ConversionForAssignmentFlags flags = ConversionForAssignmentFlags.None)
{
Debug.Assert((object)targetType != null);
Debug.Assert(expression != null);
Expand All @@ -1889,7 +1892,7 @@ internal BoundExpression GenerateConversionForAssignment(TypeSymbol targetType,

CompoundUseSiteInfo<AssemblySymbol> useSiteInfo = GetNewCompoundUseSiteInfo(diagnostics);

var conversion = (flags & ConversionForAssignmentFlags.IncrementAssignment) == 0 ?
conversion = (flags & ConversionForAssignmentFlags.IncrementAssignment) == 0 ?
this.Conversions.ClassifyConversionFromExpression(expression, targetType, isChecked: CheckOverflowAtRuntime, ref useSiteInfo) :
this.Conversions.ClassifyConversionFromType(expression.Type, targetType, isChecked: CheckOverflowAtRuntime, ref useSiteInfo);

Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Binder/SwitchBinder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,7 @@ protected BoundExpression ConvertCaseExpression(CSharpSyntaxNode node, BoundExpr
caseExpression = CreateConversion(caseExpression, conversion, SwitchGoverningType, diagnostics);
}

return ConvertPatternExpression(SwitchGoverningType, node, caseExpression, out constantValueOpt, hasErrors, diagnostics);
return ConvertPatternExpression(SwitchGoverningType, node, caseExpression, out constantValueOpt, hasErrors, diagnostics, out _);
}

private static readonly object s_nullKey = new object();
Expand Down
3 changes: 3 additions & 0 deletions src/Compilers/CSharp/Portable/CSharpResources.resx
Original file line number Diff line number Diff line change
Expand Up @@ -7178,4 +7178,7 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<data name="IDS_FeatureFileTypes" xml:space="preserve">
<value>file types</value>
</data>
<data name="ERR_CannotMatchOnINumberBase" xml:space="preserve">
<value>Cannot use a numeric constant or relational pattern on '{0}' because it inherits from or extends 'INumberBase&lt;T&gt;'. Consider using a type pattern to narrow to a specifc numeric type.</value>
</data>
</root>
1 change: 1 addition & 0 deletions src/Compilers/CSharp/Portable/Errors/ErrorCode.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2106,6 +2106,7 @@ internal enum ErrorCode
ERR_GlobalUsingStaticFileType = 9055,
ERR_FileTypeNameDisallowed = 9056,
ERR_FeatureNotAvailableInVersion11 = 9058,
ERR_CannotMatchOnINumberBase = 9059,

#endregion

Expand Down
5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.cs.xlf

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

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.de.xlf

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

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.es.xlf

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

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.fr.xlf

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

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.it.xlf

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

Loading

0 comments on commit 5b2a8d1

Please sign in to comment.