-
Notifications
You must be signed in to change notification settings - Fork 4k
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
recursive-patterns(18): Permit a constant pattern to be used with an open type as input #25995
recursive-patterns(18): Permit a constant pattern to be used with an open type as input #25995
Conversation
for a moment I thought this is about dotnet/csharplang#356, is there anything planned to support it? #Resolved |
We don't have any current plans around dotnet/csharplang#356, #Resolved |
IL_0015: call ""bool string.op_Equality(string, string)"" | ||
IL_001a: ret | ||
IL_001b: ldc.i4.0 | ||
IL_001c: ret | ||
}"); | ||
} |
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.
} [](start = 8, length = 1)
Consider testing with nested value type:
class C<T>
{
internal struct S { }
static bool Test(C<T>.S s)
{
return s == null;
}
}
``` #Resolved
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.
// Code size 10 (0xa) | ||
.maxstack 2 | ||
IL_0000: ldarg.0 | ||
IL_0001: box ""T"" |
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.
Why do we need a box? It looks like this is generated because we choose object as the type of the pattern expression. Is there a way to avoid boxing? #Resolved
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.
As far as I know, no. The CLR does not actually produce code for the box. If it is a reference type, the box goes away. If it is a value type, the whole method turns into false
.
In reply to: 180594436 [](ancestors = 180594436)
Also change to using `out` patemters where apropos.
I've changed the implementation to give an error on |
internal struct S { } | ||
static bool Test(S s) | ||
{ | ||
return s is 1; |
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.
return s is 1 [](start = 8, length = 13)
Should this result in an "expr is always false" warning? #ByDesign
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.
The short answer is no.
First, such warnings are implemented on another branch, but in any case only are given when the input is constant. This would be an error if it were determined that it can never match, but there are no rules that cause us to infer that. We used to depend on the absence of conversions, but that prevented matching a constant against a type parameter. The purpose of this change is specifically to remove the error that this cannot match.
In reply to: 180634901 [](ancestors = 180634901)
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.
Added an open issue #26100 that would result in this case being an error. It is on the open LDM issue list for pattern-matching.
// This permits us to match a value of type `IComparable<T>` with a pattern of type `int`. | ||
if (inputType.ContainsTypeParameter() && | ||
// But do not permit matching null against a struct type. | ||
(expression.ConstantValue != ConstantValue.Null || !inputType.IsNonNullableValueType())) |
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.
If inputType
is known at compile-time not to be convertible to the expression type, are we reporting an error? #Resolved
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.
Not if the input type is an open type. We need to be able to match an expression of type IComparable<T>
to 3
even though there is no conversion between them. That change is the purpose of this PR.
In reply to: 180636955 [](ancestors = 180636955)
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.
Ideally we should generate a warning for cases such as new C<T>() is 3
. Please add a test.
In reply to: 180840789 [](ancestors = 180840789,180636955)
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.
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.
LGTM
// But do not permit matching null against a struct type. | ||
(expression.ConstantValue != ConstantValue.Null || !inputType.IsNonNullableValueType())) | ||
bool inputContainsTypeParameter = inputType.ContainsTypeParameter(); | ||
HashSet<DiagnosticInfo> useSiteDiagnostics = null; |
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.
useSiteDiagnostics
declaration can be moved inside if
. #Resolved
(expression.ConstantValue != ConstantValue.Null || !inputType.IsNonNullableValueType())) | ||
bool inputContainsTypeParameter = inputType.ContainsTypeParameter(); | ||
HashSet<DiagnosticInfo> useSiteDiagnostics = null; | ||
if (inputContainsTypeParameter) |
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.
We should report the same warning for non-generic cases such as new C() is 3
?
#Resolved
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 is accomplished by the call to ExpressionOfTypeMatchesPatternType
below, and there is a test for it.
In reply to: 180890835 [](ancestors = 180890835)
@cston I believe I have addressed all of your comments. Do you have any more comments? |
if (inputType.IsNullableType() && (convertedExpression.ConstantValue == null || !convertedExpression.ConstantValue.IsNull)) | ||
{ | ||
// Null is a special case here because we want to compare null to the Nullable<T> itself, not to the underlying type. | ||
var discardedDiagnostics = DiagnosticBag.GetInstance(); // We are not intested in the diagnostic that get created here |
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.
Typo: interested
Fixes #24550
Fixes dotnet/csharplang#1284
Also includes a slight code improvement for these and related cases.
@cston @agocke Please review
@dotnet/roslyn-compiler Additional reviews welcome