-
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
Learn from bool constants and conditional accesses inside ==/!= #52425
Learn from bool constants and conditional accesses inside ==/!= #52425
Conversation
// : y.ToString(); // 2 | ||
Diagnostic(ErrorCode.ERR_UseDefViolation, "y").WithArguments("y").WithLocation(12, 15) | ||
); | ||
} |
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.
Does this change include:
if (e is string s == false) return;
s.ToString(); // ok
? From the title I thought that should be covered.
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 expect it does (comparing to bool constants should propagate conditional state). I'll add some tests. Thanks!
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.
Done
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 Thanks (iteration 11)
|| (expr is BoundConversion | ||
{ | ||
ConversionKind: ConversionKind.ExplicitNullable or ConversionKind.ImplicitNullable, | ||
} conv && conv.Operand.Type.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.
&& conv.Operand.Type.IsNonNullableValueType() [](start = 27, length = 45)
When is this part not true? #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.
One test where this is not true is EqualsCondAccess_14
. Conversion input is an int?
, result type is a long?
, and the conversion kind is ImplicitNullable. Further discussion in #52425 (comment)
#Resolved
@@ -2235,20 +2240,106 @@ private void VisitBinaryOperatorChildren(BoundBinaryOperator node) | |||
protected virtual void VisitBinaryOperatorChildren(ArrayBuilder<BoundBinaryOperator> stack) |
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.
Consider nullable-enabling this method. #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.
Done
} | ||
|
||
static bool isEquals(BoundBinaryOperator binary) | ||
=> binary.OperatorKind.Operator() == BinaryOperatorKind.Equal; |
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.
It's not in the spec, but should we also consider lifted nullable relational operators? ie, >
, >=
, etc? #Closed
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.
Perhaps, although I think we'll have to reason out the implications of the fact that:
(int?)null == (int?)null
returnstrue
, but(int?)null >= (int?)null
returnsfalse
.
https://github.com/dotnet/csharplang/blob/main/spec/expressions.md#lifted-operators
I'd like to file it as a "stretch goal" in dotnet/csharplang#4465. #Closed
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'd like to file it as a "stretch goal" in dotnet/csharplang#4465.
Fine with me.
In reply to: 610945102 [](ancestors = 610945102)
|
||
struct S | ||
{ | ||
public static bool operator ==(S left, S right) => false; |
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.
Would be good to have an example with reference types instead of struct types.
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.
Have added test EqualsCondAccess_18
for this scenario.
|| (expr is BoundConversion | ||
{ | ||
ConversionKind: ConversionKind.ExplicitNullable or ConversionKind.ImplicitNullable, | ||
} conv && conv.Operand.Type!.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.
!. [](start = 51, length = 2)
Why is this safe?
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 ExplicitNullable and ImplicitNullable conversions are only defined on operands with types, so if we get a conversion node with one of these kinds whose operand has no type, it means the conversion is malformed.
https://github.com/dotnet/csharplang/blob/main/spec/conversions.md#implicit-nullable-conversions
https://github.com/dotnet/csharplang/blob/main/spec/conversions.md#explicit-nullable-conversions
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.
Have expanded a bit on the test EqualsCondAccess_12
to show that the null
literal for example is not subject to this conversion (null
being the most common typeless expression that we might think would be subject to this conversion).
Done review pass (commit 14) |
I believe I've addressed all your feedback @333fred. Please let me know if you have any 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 (commit 16)
Test plan: #51463
Specification: https://github.com/dotnet/csharplang/blob/main/proposals/improved-definite-assignment.md#-relational-equality-operator-expressions