Give default literals a type in tuple comparisons#25327
Give default literals a type in tuple comparisons#25327jcouv merged 4 commits intodotnet:features/tuple-equalityfrom
Conversation
There was a problem hiding this comment.
binary operator [](start = 54, length = 15)
" == or != operator" ? For precision and consistency with other messages #Closed
There was a problem hiding this comment.
return expr; [](start = 20, length = 12)
Should we still convert if we have convertedType? #Closed
There was a problem hiding this comment.
That'll be fixed shortly in the PR related to semantic model and converted types. If there is a converted type for the tuple, we just apply that (and not do the element-wise conversions). #Resolved
There was a problem hiding this comment.
ERR_AmbigBinaryOps [](start = 45, length = 18)
ERR_AmbigBinaryOps [](start = 45, length = 18)
It is not obvious why we consider the operator ambiguous in this case. #Closed
There was a problem hiding this comment.
The case here is really that we weren't able to give default a tuple type.
But I wanted to write the condition a bit more broadly, so that the code below can clearly move ahead with assumption that we either have a tuple literal (which may be typeless) or a typed expression (on both sides).
I could add a comment or use a new error code if that feels more appropriate.
In reply to: 173291602 [](ancestors = 173291602)
There was a problem hiding this comment.
But I wanted to write the condition a bit more broadly
Consider if you can add an assert with more narrow condition that would verify your assumption.
In reply to: 173310337 [](ancestors = 173310337,173291602)
There was a problem hiding this comment.
internal [](start = 8, length = 8)
Do you expect this method to be used outside of the Binder type? #Closed
There was a problem hiding this comment.
There was a problem hiding this comment.
default [](start = 49, length = 7)
Do we have a test against nullable non-tuple? #Closed
|
Done with review pass (iteration 6) #Closed |
There was a problem hiding this comment.
return [](start = 12, length = 6)
Nit: empty space missing before.
There was a problem hiding this comment.
TestNestedDefaultWithNullableNonTupleType [](start = 20, length = 41)
Do we have a test where default is paired with null? #Closed
There was a problem hiding this comment.
Yes, that's covered in TestTypelessTupleAndTupleOfDefaults at line 1041.
System.Console.Write((null, () => 1) == (default, default)); #Resolved
|
Iteration 7 looks good, but I assume some re-basing should happen after the other PR is merged. How do you plan to handle this? #Closed |
|
Since not too many comments, my plan is to merge the earlier PR first then rebase this one on top. #Resolved |
|
Rebased. I'll address the two remaining comments tonight. Thanks! |
|
test windows_debug_unit32_prtest please |
|
@AlekseyTs I've narrowed the check and added assert. Good to go? |
3 participants
Note:
Only the last two commits in this PR are new. The rest is the work to compute converted left/right in tuple equality (from PR #25297).
The second-to-last commit handles
defaultbeing compared to a tuple (or nullable tuple).Examples:
(0, 0) == default \\ true, same as default((int, int))(null, 0) == (null, default) \\ true(int, int)? nullableTuple; nullableTuple == default \\ same as default( (int, int)? )The last commit blocks tuple binops in expression trees.