Adjust tuple and nullability checks on duplicate implementations and type constraints#38563
Adjust tuple and nullability checks on duplicate implementations and type constraints#38563jcouv merged 10 commits intodotnet:masterfrom
Conversation
1cfd986 to
71f99fc
Compare
71f99fc to
de567fb
Compare
There was a problem hiding this comment.
WRN_DuplicateInterfaceWithNullabilityMismatchInBaseList [](start = 62, length = 55)
From the description of this PR I got an impression that we should report an error now: "we're treating such duplicates (whether direct or indirect) as an error (instead of a warning previously)" #Closed
There was a problem hiding this comment.
Sorry, the compat council went back and forth on this and I forgot to update the description. We're sticking with a warning as opposed to an error because of multi-assembly scenarios. #Resolved
There was a problem hiding this comment.
if (!other.Equals(@interface, TypeCompareKind.ObliviousNullableModifierMatchesAny)) [](start = 32, length = 83)
Should we still report a warning for an oblivious mismatch? #Closed
There was a problem hiding this comment.
I don't think so. That seems the most likely/common/useful scenario: you have a base type with oblivious from a library, then your type implemented the interface with specific nullability.
In reply to: 324916964 [](ancestors = 324916964)
There was a problem hiding this comment.
expected [](start = 16, length = 8)
Remove? #Closed
There was a problem hiding this comment.
expected [](start = 16, length = 8)
remove? #Closed
There was a problem hiding this comment.
I, I<object?> [](start = 29, length = 21)
Should we report a warning even for an oblivious mismatch when it happens for interfaces directly present in the base list? #Closed
There was a problem hiding this comment.
I'm going for an approach mimicking tuple name differences. We could have special rules for nullability, but I'm not sure it helps or is worthwhile.
The email thread landed on warnings for all cases, which is also consistent with the general approach we took for nullability.
Do you think it should be an error?
In reply to: 325296374 [](ancestors = 325296374)
There was a problem hiding this comment.
~TypeCompareKind.ObliviousNullableModifierMatchesAny [](start = 96, length = 52)
Does this make any difference? Since we are ignoring any nullable differences. #Closed
|
Done with review pass (iteration 8) #Closed |
There was a problem hiding this comment.
continue; [](start = 32, length = 9)
Do we want to continue here though? This will drop the interface from the list. How do we decide which one is better to keep? Perhaps we should report a warning and keep it? Or report a different warning that would make it clear that the thing is ignored? #Closed
There was a problem hiding this comment.
This continue is scoped to foreach. It doesn't affect the list of interfaces that are collected.
Added test to illustrate and removed both continue statements to improve readability.
In reply to: 325899242 [](ancestors = 325899242)
There was a problem hiding this comment.
|
Done with review pass (iteration 9). #Closed |
|
@dotnet/roslyn-compiler for a second review. Thanks |
55d5424 to
fec2789
Compare
|
Rebased to resolve conflict on doc file. |
37492a9 to
b70a55f
Compare
b70a55f to
705ab18
Compare
|
@dotnet/roslyn-compiler for a second review. Thanks |
1 similar comment
|
@dotnet/roslyn-compiler for a second review. Thanks |
|
Consider adding a test that is explicitly about the mismatched tuple names and does not involve nullability in any way. |
333fred
left a comment
There was a problem hiding this comment.
LGTM (commit 10) with possible test suggestion.
|
@333fred The tuple scenario with no nullability differences is covered in tuple tests ( |
Fixes #38560 (open questions on duplicate implementations with nullability differences): in this change,
we're treating such duplicates (whether direct or indirect) as an error (instead of a warning previously), except thatwe're relaxing the comparison to account for oblivious.Note: there is still an active email thread on this behavior, as other nullability checks only produce warnings (not errors).
Fixes #38427 (missing diagnostic on duplicate type constraint with tuple name differences): in this change, we're treating such duplicates as an error, as well as duplicate type constraints with any nullability differences.
At the same time, I'm fixing a small issue with how those duplicates are checked: we were not detecting duplicates when they differed in tuple names and nullability at the same time.