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> (#62653)

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 25, 2022
1 parent 4bbd4bf commit d056317
Show file tree
Hide file tree
Showing 23 changed files with 823 additions and 23 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
70 changes: 58 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 ((constantValueOpt?.IsNumeric == true) && ShouldBlockINumberBaseConversion(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,22 @@ private static BoundPattern BindDiscardPattern(DiscardPatternSyntax node, TypeSy
}
}

private bool ShouldBlockINumberBaseConversion(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 interfaces = inputType is TypeParameterSymbol typeParam ? typeParam.EffectiveInterfacesNoUseSiteDiagnostics : inputType.AllInterfacesNoUseSiteDiagnostics;
return interfaces.Any(static (i, _) => i.IsWellKnownINumberBaseType(), 0) || inputType.IsWellKnownINumberBaseType();
}

private static ExpressionSyntax SkipParensAndNullSuppressions(ExpressionSyntax e, BindingDiagnosticBag diagnostics, ref bool hasErrors)
{
while (true)
Expand Down Expand Up @@ -465,19 +487,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 +515,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 +532,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 +568,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 +605,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 +649,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 +1617,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 +1655,13 @@ void addSubpatternsForTuple(ImmutableArray<TypeWithAnnotations> elementTypes)
constantValueOpt = ConstantValue.Bad;
}

if (!hasErrors && ShouldBlockINumberBaseConversion(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 @@ -1897,6 +1897,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 @@ -1916,7 +1919,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 @@ -7187,6 +7187,9 @@ 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>
<data name="IDS_ArrayAccess" xml:space="preserve">
<value>array access</value>
</data>
Expand Down
6 changes: 4 additions & 2 deletions src/Compilers/CSharp/Portable/Errors/ErrorCode.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2108,11 +2108,13 @@ internal enum ErrorCode
WRN_AnalyzerReferencesNewerCompiler = 9057,
ERR_FeatureNotAvailableInVersion11 = 9058,
ERR_RefFieldInNonRefStruct = 9059,
ERR_CannotMatchOnINumberBase = 9060,

#endregion

// Note: you will need to do the following after adding warnings:
// Note: you will need to do the following after adding any code:
// 1) Update ErrorFacts.IsBuildOnlyDiagnostic to handle the new error code.
// Additionally, after adding a new warning you will need to do the following:
// 1) Re-generate compiler code (eng\generate-compiler-code.cmd).
// 2) Update ErrorFacts.IsBuildOnlyDiagnostic to handle the new error code.
}
}
1 change: 1 addition & 0 deletions src/Compilers/CSharp/Portable/Errors/ErrorFacts.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2208,6 +2208,7 @@ internal static bool IsBuildOnlyDiagnostic(ErrorCode code)
case ErrorCode.ERR_FeatureNotAvailableInVersion11:
case ErrorCode.ERR_RefFieldInNonRefStruct:
case ErrorCode.WRN_AnalyzerReferencesNewerCompiler:
case ErrorCode.ERR_CannotMatchOnINumberBase:
return false;
default:
// NOTE: All error codes must be explicitly handled in this switch statement
Expand Down
26 changes: 21 additions & 5 deletions src/Compilers/CSharp/Portable/Symbols/TypeSymbolExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2060,18 +2060,34 @@ internal static bool IsCompilerServicesTopLevelType(this TypeSymbol typeSymbol)
internal static bool IsWellKnownSetsRequiredMembersAttribute(this TypeSymbol type)
=> type.Name == "SetsRequiredMembersAttribute" && type.IsWellKnownDiagnosticsCodeAnalysisTopLevelType();

internal static bool IsWellKnownINumberBaseType(this TypeSymbol type)
{
type = type.OriginalDefinition;
return type is NamedTypeSymbol { Name: "INumberBase", IsInterface: true, Arity: 1, ContainingType: null } &&
IsContainedInNamespace(type, "System", "Numerics");
}

private static bool IsWellKnownDiagnosticsCodeAnalysisTopLevelType(this TypeSymbol typeSymbol)
=> typeSymbol.ContainingType is null && IsContainedInNamespace(typeSymbol, "System", "Diagnostics", "CodeAnalysis");

private static bool IsContainedInNamespace(this TypeSymbol typeSymbol, string outerNS, string midNS, string innerNS)
private static bool IsContainedInNamespace(this TypeSymbol typeSymbol, string outerNS, string midNS, string? innerNS = null)
{
var innerNamespace = typeSymbol.ContainingNamespace;
if (innerNamespace?.Name != innerNS)
NamespaceSymbol? midNamespace;

if (innerNS != null)
{
return false;
var innerNamespace = typeSymbol.ContainingNamespace;
if (innerNamespace?.Name != innerNS)
{
return false;
}
midNamespace = innerNamespace.ContainingNamespace;
}
else
{
midNamespace = typeSymbol.ContainingNamespace;
}

var midNamespace = innerNamespace.ContainingNamespace;
if (midNamespace?.Name != midNS)
{
return false;
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.

Loading

0 comments on commit d056317

Please sign in to comment.