-
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 2 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
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 |
---|---|---|
|
@@ -145,10 +145,6 @@ BoundExpression MakeIsDeclarationPattern(SyntaxNode syntax, BoundExpression lowe | |
{ | ||
var type = loweredTarget.Type; | ||
|
||
// 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()); | ||
|
||
// a pattern match of the form "expression is Type identifier" is equivalent to | ||
// an invocation of one of these helpers: | ||
if (type.IsReferenceType) | ||
|
@@ -175,8 +171,13 @@ BoundExpression MakeIsDeclarationPattern(SyntaxNode syntax, BoundExpression lowe | |
} | ||
else if (type.IsValueType) | ||
{ | ||
// 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. | ||
// is irrefutable (there is no way for a non-nullable value type to be null), and we can just do | ||
// the assignment and return true. | ||
if (loweredInput.Type == type) | ||
{ | ||
return _factory.MakeSequence( | ||
|
@@ -207,18 +208,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) | ||
); | ||
} | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5674,5 +5674,91 @@ public static void Main(string[] args) | |
var compilation = CreateCompilationWithMscorlib45(source, references: new MetadataReference[] { CSharpRef, SystemCoreRef }, options: TestOptions.ReleaseExe); | ||
var comp = CompileAndVerify(compilation, expectedOutput: "roslyn"); | ||
} | ||
|
||
[Fact, WorkItem(16195, "https://github.com/dotnet/roslyn/issues/16195")] | ||
public void OpenTypeMatch_01() | ||
{ | ||
var source = | ||
@"using System; | ||
public class Base { } | ||
public class Derived : Base { } | ||
public class Program | ||
{ | ||
public static void Main(string[] args) | ||
{ | ||
M(new Derived()); | ||
M(new Base()); | ||
} | ||
public static void M<T>(T x) where T: Base | ||
{ | ||
Console.Write(x is Derived b0); | ||
switch (x) | ||
{ | ||
case Derived b1: | ||
Console.Write(1); | ||
break; | ||
default: | ||
Console.Write(0); | ||
break; | ||
} | ||
} | ||
} | ||
"; | ||
var compilation = CreateCompilationWithMscorlib45(source, references: new MetadataReference[] { CSharpRef, SystemCoreRef }, options: TestOptions.ReleaseExe, parseOptions: TestOptions.Regular); | ||
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.
Consider explicitly using C# 7 version, otherwise this test will break once we bump the default version. |
||
compilation.VerifyDiagnostics( | ||
// (13,28): error CS9003: An expression of type 'T' cannot be handled by a pattern of type 'Derived' in C# 7. Please use language version 7.1 or greater. | ||
// Console.Write(x is Derived b0); | ||
Diagnostic(ErrorCode.ERR_PatternWrongGenericTypeInVersion, "Derived").WithArguments("T", "Derived", "7", "7.1").WithLocation(13, 28), | ||
// (16,18): error CS9003: An expression of type 'T' cannot be handled by a pattern of type 'Derived' in C# 7. Please use language version 7.1 or greater. | ||
// case Derived b1: | ||
Diagnostic(ErrorCode.ERR_PatternWrongGenericTypeInVersion, "Derived").WithArguments("T", "Derived", "7", "7.1").WithLocation(16, 18) | ||
); | ||
compilation = CreateCompilationWithMscorlib45(source, references: new MetadataReference[] { CSharpRef, SystemCoreRef }, options: TestOptions.ReleaseExe, parseOptions: TestOptions.Regular7_1); | ||
compilation.VerifyDiagnostics(); | ||
CompileAndVerify(compilation, expectedOutput: "True1False0"); | ||
} | ||
|
||
[Fact, WorkItem(16195, "https://github.com/dotnet/roslyn/issues/16195")] | ||
public void OpenTypeMatch_02() | ||
{ | ||
var source = | ||
@"using System; | ||
public class Base { } | ||
public class Derived : Base { } | ||
public class Program | ||
{ | ||
public static void Main(string[] args) | ||
{ | ||
M<Derived>(new Derived()); | ||
M<Derived>(new Base()); | ||
} | ||
public static void M<T>(Base x) | ||
{ | ||
Console.Write(x is T b0); | ||
switch (x) | ||
{ | ||
case T b1: | ||
Console.Write(1); | ||
break; | ||
default: | ||
Console.Write(0); | ||
break; | ||
} | ||
} | ||
} | ||
"; | ||
var compilation = CreateCompilationWithMscorlib45(source, references: new MetadataReference[] { CSharpRef, SystemCoreRef }, options: TestOptions.ReleaseExe, parseOptions: TestOptions.Regular); | ||
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.
Consider explicitly using C# 7 version, otherwise this test will break once we bump the default version. |
||
compilation.VerifyDiagnostics( | ||
// (13,28): error CS9003: An expression of type 'Base' cannot be handled by a pattern of type 'T' in C# 7. Please use language version 7.1 or greater. | ||
// Console.Write(x is T b0); | ||
Diagnostic(ErrorCode.ERR_PatternWrongGenericTypeInVersion, "T").WithArguments("Base", "T", "7", "7.1").WithLocation(13, 28), | ||
// (16,18): error CS9003: An expression of type 'Base' cannot be handled by a pattern of type 'T' in C# 7. Please use language version 7.1 or greater. | ||
// case T b1: | ||
Diagnostic(ErrorCode.ERR_PatternWrongGenericTypeInVersion, "T").WithArguments("Base", "T", "7", "7.1") | ||
); | ||
compilation = CreateCompilationWithMscorlib45(source, references: new MetadataReference[] { CSharpRef, SystemCoreRef }, options: TestOptions.ReleaseExe, parseOptions: TestOptions.Regular7_1); | ||
compilation.VerifyDiagnostics(); | ||
CompileAndVerify(compilation, expectedOutput: "True1False0"); | ||
} | ||
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. Consider testing types that are not type parameters but where 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.
Yes, please add several tests for scenarios like that. |
||
} | ||
} |
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.
This type comparison should ignore dynamic/tuple/modopt differences
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.
Essentially if there is an identity conversion, then match is irrefutable.
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.
That may be true (the suggested change would be a code improvement), but it is not relevant to the change in the PR.