Allow extension methods on tuple receivers when underlying conversions are "upcasts" #16575
Conversation
…s are "upcasts" Extensions methods declared on bases and implemented interfaces are allowed to be applied to receivers that of derived or implementing type. This change extends this rule to tuple receivers as long as the rule holds for individual elements, recursively. I.E. valid "this" conversions are: - identity conversion - implicit reference conversion - boxing conversion - (new) implicit tuple[literal] conversions as long as all underlying conversions are valid "this" conversions.
| @@ -5360,7 +5360,12 @@ private void BindMemberAccessReportError(BoundMethodGroup node, DiagnosticBag di | |||
| } | |||
| else | |||
| { | |||
| if (boundLeft.Kind == BoundKind.TypeExpression || | |||
|
|
|||
| if ((object)boundLeft.Type == null) | |||
There was a problem hiding this comment.
We had a bug here, that could be hit even without this change.
Typeless expressions could cause crashes or serialization asserts during error reporting.
There was a problem hiding this comment.
Is there a test in this PR that would have crashed before?
There was a problem hiding this comment.
yes, ExtensionMethodOnConvertedTuple002Err would crash
|
@dotnet/roslyn-compiler - please review |
| case ConversionKind.ImplicitTuple: | ||
| case ConversionKind.ImplicitTupleLiteral: | ||
| // check if all element conversions satisfy the requirement | ||
| foreach (var elementConversion in conversion.UnderlyingConversions) |
There was a problem hiding this comment.
I can never remember which ImmutableArray extensions we have and which we are still missing, but if we have it, consider conversion.UnderlyingConversions.All(c => IsValidExtensionMethodThisArgConversion(c)).
| @@ -5360,7 +5360,12 @@ private void BindMemberAccessReportError(BoundMethodGroup node, DiagnosticBag di | |||
| } | |||
| else | |||
| { | |||
| if (boundLeft.Kind == BoundKind.TypeExpression || | |||
|
|
|||
| if ((object)boundLeft.Type == null) | |||
There was a problem hiding this comment.
Is there a test in this PR that would have crashed before?
|
|
||
| var comp = CreateCompilationWithMscorlib(source, references: s_valueTupleRefs.Concat(new[] { LinqAssemblyRef })); | ||
| comp.VerifyDiagnostics( | ||
| // (10,41): error CS1928: '(int, int)' does not contain a definition for 'M' and the best extension method overload 'C.M((int x, long y))' has some invalid arguments |
There was a problem hiding this comment.
I don't understand this diagnostic. What argument is invalid?
There was a problem hiding this comment.
"this" is of type (int int), but expected to be (int, long).
While that has implicit conversion, it is not in the set of the allowed upcast-like conversions, int->long is not a cast to base or to an implemented interface, so it fails.
The error could be more detailed, but it is a preexisting error, that we are not fixing here.
|
@MattGertz - need RTW approval. Thanks! |
|
Corresponding VSO bug: |
|
Side chat with Neal: "Yes, it would be a breaking change to fix it later. That’s because it changes what methods are candidates in overload resolution. The method selected in overload resolution after this change may be different from the method selected in overload resolution without the change. So making the change later could silently change the meaning of existing code. That’s a kind of breaking change that we try very hard to avoid." Customer-reported omission, and it would be a breaking change to fix later, so approved. |
|
@VSadov Is there a reason for not making similar change, or not testing similar scenarios for VB? |
|
Vlad said he'll file an issue for VB, but the fix is more involved there, so he's not recommending it for RTM. |
|
@VSadov Did you file a follow-up issue for VB? |
|
BTW, the VB bug is |
Allow extension methods on tuple receivers when underlying conversions are "upcasts"
Fixes:#16159
Extensions methods declared on bases and implemented interfaces are allowed to be applied to receivers that of derived or implementing type.
This change extends this rule to tuple receivers as long as the rule holds for individual elements, recursively.
I.E. valid "this" conversions are:
Customer scenario
Customer defines an extension method on
(Base, Base)and expects it to be applicable to expressions of type(Derived, Derived)To his surprise, code does not compile or compiles, but picks a different less specific overload.
Bugs this fixes:
#16159
Workarounds, if any
Customer can cast to the exact type on which extension was declared.
This could be inconvenient, especially in cases where extension is defined in terms of very abstract types like the proposed
Ziplinq operator that takes(IEnumerable<T>, IEnumerable<U>)The usefulness of an abstract implementation diminishes if user needs to cast when calling it with a concrete receiver types (which is most of the time)
Risk
The fix is very specific to the kind of tuple conversions that are permitted on a receiver when validating an extension method candidate
Performance impact
"Low"
Is this a regression from a previous update?
New feature.
Root cause analysis:
We did not consider this scenario.
How was the bug found?
customer reported