-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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.
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
Relax conversion requirements for pattern-matching involving type parameters. #18784
Changes from 9 commits
9d0ab48
47a9d7f
8e19970
0570548
98287a8
1ad87bf
8615a76
3257f88
e2a1eb1
089227f
1eb6954
fd2109e
d9bff8f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -130,7 +130,7 @@ private DecisionTree AddByValue(DecisionTree.Guarded guarded, BoundConstantPatte | |
|
||
private DecisionTree AddByValue(DecisionTree.ByValue byValue, BoundConstantPattern value, DecisionMaker makeDecision) | ||
{ | ||
Debug.Assert(value.Value.Type == byValue.Type); | ||
Debug.Assert(value.Value.Type.Equals(byValue.Type, TypeCompareKind.IgnoreDynamicAndTupleNames)); | ||
if (byValue.Default != null) | ||
{ | ||
return AddByValue(byValue.Default, value, makeDecision); | ||
|
@@ -221,7 +221,7 @@ private DecisionTree AddByValue(DecisionTree.ByType byType, BoundConstantPattern | |
var kvp = byType.TypeAndDecision[i]; | ||
var matchedType = kvp.Key; | ||
var decision = kvp.Value; | ||
if (matchedType.TupleUnderlyingTypeOrSelf() == value.Value.Type.TupleUnderlyingTypeOrSelf()) | ||
if (matchedType.Equals(value.Value.Type, TypeCompareKind.IgnoreDynamicAndTupleNames)) | ||
{ | ||
forType = decision; | ||
break; | ||
|
@@ -260,21 +260,30 @@ private DecisionTree AddByType(DecisionTree decision, TypeSymbol type, DecisionM | |
case DecisionTree.DecisionKind.ByValue: | ||
{ | ||
var byValue = (DecisionTree.ByValue)decision; | ||
DecisionTree result; | ||
if (byValue.Default == null) | ||
{ | ||
byValue.Default = makeDecision(byValue.Expression, byValue.Type); | ||
if (byValue.Default.MatchIsComplete) | ||
if (byValue.Type.Equals(type, TypeCompareKind.IgnoreDynamicAndTupleNames)) | ||
{ | ||
byValue.MatchIsComplete = true; | ||
result = byValue.Default = makeDecision(byValue.Expression, byValue.Type); | ||
} | ||
else | ||
{ | ||
byValue.Default = new DecisionTree.ByType(byValue.Expression, byValue.Type, null); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a bug fix to insert a needed conversion. |
||
result = AddByType(byValue.Default, type, makeDecision); | ||
} | ||
|
||
return byValue.Default; | ||
} | ||
else | ||
{ | ||
Debug.Assert(byValue.Default.Type == type); | ||
return Add(byValue.Default, makeDecision); | ||
result = AddByType(byValue.Default, type, makeDecision); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a bug fix to insert a needed conversion. |
||
} | ||
|
||
if (byValue.Default.MatchIsComplete) | ||
{ | ||
byValue.MatchIsComplete = true; | ||
} | ||
|
||
return result; | ||
} | ||
case DecisionTree.DecisionKind.Guarded: | ||
return AddByType((DecisionTree.Guarded)decision, type, makeDecision); | ||
|
@@ -321,7 +330,7 @@ private DecisionTree AddByType(DecisionTree.ByType byType, TypeSymbol type, Deci | |
if (byType.TypeAndDecision.Count != 0) | ||
{ | ||
var lastTypeAndDecision = byType.TypeAndDecision.Last(); | ||
if (lastTypeAndDecision.Key.TupleUnderlyingTypeOrSelf() == type.TupleUnderlyingTypeOrSelf()) | ||
if (lastTypeAndDecision.Key.Equals(type, TypeCompareKind.IgnoreDynamicAndTupleNames)) | ||
{ | ||
result = Add(lastTypeAndDecision.Value, makeDecision); | ||
} | ||
|
@@ -533,7 +542,7 @@ private DecisionTree Add(DecisionTree.ByType byType, DecisionMaker makeDecision) | |
TypeSymbol patternType, | ||
ref HashSet<DiagnosticInfo> useSiteDiagnostics) | ||
{ | ||
if (expressionType == patternType) | ||
if ((object)expressionType == (object)patternType) | ||
{ | ||
return true; | ||
} | ||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,7 @@ | |
using Microsoft.CodeAnalysis.CSharp.Symbols; | ||
using Roslyn.Utilities; | ||
using System.Collections.Immutable; | ||
using System.Collections.Generic; | ||
|
||
namespace Microsoft.CodeAnalysis.CSharp | ||
{ | ||
|
@@ -80,7 +81,7 @@ private BoundExpression MakeIsDeclarationPattern(BoundDeclarationPattern lowered | |
return result; | ||
} | ||
|
||
Debug.Assert((object)loweredPattern.Variable != null && loweredInput.Type == loweredPattern.Variable.GetTypeOrReturnType()); | ||
Debug.Assert((object)loweredPattern.Variable != null && loweredInput.Type.Equals(loweredPattern.Variable.GetTypeOrReturnType(), TypeCompareKind.IgnoreDynamicAndTupleNames)); | ||
|
||
var assignment = _factory.AssignmentExpression(loweredPattern.VariableAccess, loweredInput); | ||
return _factory.MakeSequence(assignment, result); | ||
|
@@ -93,10 +94,10 @@ private BoundExpression MakeIsDeclarationPattern(BoundDeclarationPattern lowered | |
|
||
return _factory.Sequence(ImmutableArray.Create(temp), | ||
sideEffects: ImmutableArray<BoundExpression>.Empty, | ||
result: MakeIsDeclarationPattern(loweredPattern.Syntax, loweredInput, discard, requiresNullTest: true)); | ||
result: MakeIsDeclarationPattern(loweredPattern.Syntax, loweredInput, discard, requiresNullTest: loweredInput.Type.CanContainNull())); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a bug fix to remove an unneeded null check. |
||
} | ||
|
||
return MakeIsDeclarationPattern(loweredPattern.Syntax, loweredInput, loweredPattern.VariableAccess, requiresNullTest: true); | ||
return MakeIsDeclarationPattern(loweredPattern.Syntax, loweredInput, loweredPattern.VariableAccess, requiresNullTest: loweredInput.Type.CanContainNull()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a bug fix to remove an unneeded null check. |
||
} | ||
|
||
/// <summary> | ||
|
@@ -141,13 +142,36 @@ private BoundExpression CompareWithConstant(BoundExpression input, BoundExpressi | |
); | ||
} | ||
|
||
private bool MatchIsIrrefutable(TypeSymbol sourceType, TypeSymbol targetType, bool requiredNullTest) | ||
{ | ||
// use site diagnostics will already have been reported during binding. | ||
HashSet<DiagnosticInfo> ignoredDiagnostics = null; | ||
switch (_compilation.Conversions.ClassifyBuiltInConversion(sourceType, targetType, ref ignoredDiagnostics).Kind) | ||
{ | ||
case ConversionKind.Boxing: | ||
case ConversionKind.ImplicitReference: | ||
case ConversionKind.Identity: | ||
return true; | ||
default: | ||
return false; | ||
} | ||
} | ||
|
||
BoundExpression MakeIsDeclarationPattern(SyntaxNode syntax, BoundExpression loweredInput, BoundExpression loweredTarget, bool requiresNullTest) | ||
{ | ||
var type = loweredTarget.Type; | ||
requiresNullTest = requiresNullTest && !loweredInput.Type.CanContainNull(); | ||
|
||
// The type here is not a Nullable<T> instance type, as that would have led to the semantic error: | ||
// ERR_PatternNullableType: It is not legal to use nullable type '{0}' in a pattern; use the underlying type '{1}' instead. | ||
Debug.Assert(!type.IsNullableType()); | ||
// It is possible that the input value is already of the correct type, in which case the pattern | ||
// is irrefutable, and we can just do the assignment and return true (or perform the null test). | ||
if (MatchIsIrrefutable(loweredInput.Type, loweredTarget.Type, requiresNullTest)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a bug fix to remove an unneeded type test. |
||
{ | ||
var convertedInput = _factory.Convert(loweredTarget.Type, loweredInput); | ||
var assignment = _factory.AssignmentExpression(loweredTarget, convertedInput); | ||
return requiresNullTest | ||
? _factory.ObjectNotEqual(assignment, _factory.Null(type)) | ||
: _factory.MakeSequence(assignment, _factory.Literal(true)); | ||
} | ||
|
||
// a pattern match of the form "expression is Type identifier" is equivalent to | ||
// an invocation of one of these helpers: | ||
|
@@ -158,31 +182,15 @@ BoundExpression MakeIsDeclarationPattern(SyntaxNode syntax, BoundExpression lowe | |
// t = e as T; | ||
// return t != null; | ||
// } | ||
if (loweredInput.Type == type) | ||
{ | ||
// CONSIDER: this can be done whenever input.Type is a subtype of type for improved code | ||
var assignment = _factory.AssignmentExpression(loweredTarget, loweredInput); | ||
return requiresNullTest | ||
? _factory.ObjectNotEqual(assignment, _factory.Null(type)) | ||
: _factory.MakeSequence(assignment, _factory.Literal(true)); | ||
} | ||
else | ||
{ | ||
return _factory.ObjectNotEqual( | ||
_factory.AssignmentExpression(loweredTarget, _factory.As(loweredInput, type)), | ||
_factory.Null(type)); | ||
} | ||
return _factory.ObjectNotEqual( | ||
_factory.AssignmentExpression(loweredTarget, _factory.As(loweredInput, type)), | ||
_factory.Null(type)); | ||
} | ||
else if (type.IsValueType) | ||
{ | ||
// It is possible that the input value is already of the correct type, in which case the pattern | ||
// is irrefutable, and we can just do the assignment and return true. | ||
if (loweredInput.Type == type) | ||
{ | ||
return _factory.MakeSequence( | ||
_factory.AssignmentExpression(loweredTarget, loweredInput), | ||
_factory.Literal(true)); | ||
} | ||
// The type here is not a Nullable<T> instance type, as that would have led to the semantic error: | ||
// ERR_PatternNullableType: It is not legal to use nullable type '{0}' in a pattern; use the underlying type '{1}' instead. | ||
Debug.Assert(!type.IsNullableType()); | ||
|
||
// It may be possible to improve this code by only assigning t when returning | ||
// true (avoid returning a new default value) | ||
|
@@ -207,18 +215,26 @@ BoundExpression MakeIsDeclarationPattern(SyntaxNode syntax, BoundExpression lowe | |
Debug.Assert(type.IsTypeParameter()); | ||
// bool Is<T>(this object i, out T o) | ||
// { | ||
// // inefficient because it performs the type test twice. | ||
// // inefficient because it performs the type test twice, and also because it boxes the input. | ||
// bool s = i is T; | ||
// if (s) o = (T)i; | ||
// o = s ? (T)i : default(T); | ||
// return s; | ||
// } | ||
return _factory.Conditional(_factory.Is(loweredInput, type), | ||
_factory.MakeSequence(_factory.AssignmentExpression( | ||
loweredTarget, | ||
_factory.Convert(type, loweredInput)), | ||
_factory.Literal(true)), | ||
_factory.Literal(false), | ||
_factory.SpecialType(SpecialType.System_Boolean)); | ||
|
||
// Because a cast involving a type parameter is not necessarily a valid conversion (or, if it is, it might not | ||
// be of a kind appropriate for pattern-matching), we use `object` as an intermediate type for the input expression. | ||
var tmpType = _factory.SpecialType(SpecialType.System_Object); | ||
var s = _factory.SynthesizedLocal(_factory.SpecialType(SpecialType.System_Boolean), syntax); | ||
var i = _factory.SynthesizedLocal(tmpType, syntax); // we copy the input to avoid double evaluation | ||
return _factory.Sequence( | ||
ImmutableArray.Create(s, i), | ||
ImmutableArray.Create<BoundExpression>( | ||
_factory.AssignmentExpression(_factory.Local(i), _factory.Convert(tmpType, loweredInput)), | ||
_factory.AssignmentExpression(_factory.Local(s), _factory.Is(_factory.Local(i), type)), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should have some tests that validate IL, just to be sure how this is emitted. |
||
_factory.AssignmentExpression(loweredTarget, _factory.Conditional(_factory.Local(s), _factory.Convert(type, _factory.Local(i)), _factory.Default(type), type)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Can we inline the assignment to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. But it made no difference in any generated code. |
||
), | ||
_factory.Local(s) | ||
); | ||
} | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add tests that ignore
dynamic
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't think of a way to make that happen. The is no constant pattern whose type is generic. This could probably be simplified to
(matchedType == value.Value.Type)
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I know a way to make it happen. An enum type nested in a generic class.