Fixes overload resolution regression with ValueTuple #20587

gafter · 2017-06-30T22:56:53Z

Customer scenario

The C# overload resolution tie-breaker involving a more specific type argument isn't respected for instances of ValueTuple. When such a scenario arises, the programmer get an ambiguity error on the invocation.

Bugs this fixes:

Fixes #20494
Adds a test for #20583

Workarounds, if any

The programmer can provide explicit type arguments to resolve the ambiguity.

Risk

Small. The fix is a very local workaround for the symptom. The underlying issue (ValueTuple instances are not recognized by the compiler as generic), may be addressed separately in a more extensive change.

Performance impact

Tiny or none.

Is this a regression from a previous update?

It is a regression in VS2017 versus VS2015.

Root cause analysis:

This is a symptom of our choice of internal representation for tuple types. I am separately looking at a more uniform change to address the underlying issue, which is that tuple types are not recognized by the compiler as generic.

How was the bug found?

Ad-hoc testing.

@dotnet/roslyn-compiler May I please have a couple of reviews of this tiny bug fix?

Fixes dotnet#20494 Adds a test for dotnet#20583

VSadov · 2017-06-30T23:04:53Z

src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/OverloadResolution.cs

-            var n1 = t1 as NamedTypeSymbol;
-            var n2 = t2 as NamedTypeSymbol;
+            var n1 = t1.TupleUnderlyingTypeOrSelf() as NamedTypeSymbol;
+            var n2 = t2.TupleUnderlyingTypeOrSelf() as NamedTypeSymbol;


Do we need the same change for VB?

Yes, we do.

VSadov · 2017-06-30T23:16:59Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenTupleTest.cs

+
+        [Fact]
+        [WorkItem(20494, "https://github.com/dotnet/roslyn/issues/20494")]
+        public void MoreGenericTieBreaker_01()


Can we have a "literal" form of these new tests as well?
I am sure they will behave the same, - just for accidental regression purposes.

VSadov · 2017-06-30T23:20:15Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenTupleTest.cs

+        [Fact(Skip = "https://github.com/dotnet/roslyn/issues/20583")]
+        [WorkItem(20494, "https://github.com/dotnet/roslyn/issues/20494")]
+        [WorkItem(20583, "https://github.com/dotnet/roslyn/issues/20583")]
+        public void MoreGenericTieBreaker_02()


Can we also have a negative test when, for example, element type #1 is more specific for one overload and element type #20 is more specific for another. That should fail, but since we are not operating on the flat list of element types, we may accidentally give preference to the earlier or later types in the element type order.

It is more subtle than you would expect. If the first overload is more specific in the first and 10th parameter positions, but the second is more specific in the 11th parameter position, then the first overload is selected. I'll add that as a test too.

There should not be any preference between position of tuple elements. How is N1 element more important than N11?
This behavior seems to be a bug unnecessarily leaking implementation details of tuple types.

No, it is intentional, and it is not a bug. Between the two overloads

M<T, U, V>(((int, T), int, int, int, int, int, int, int, int, (int, U), V)) M<T, U, V>((T, int, int, int, int, int, int, int, int, U, (int, V)))

which could also be written

M<T, U, V>(VT<VT<int, T>, int, int, int, int, int, int, VT<int, int, VT<int, U>, V>>) M<T, U, V>(VT<T, int, int, int, int, int, int, VT<int, int, U, VT<int, V>>>)

when called on a value of type

((int, int), int, int, int, int, int, int, int, int, (int, int), (int, int))

which could also be written

VT<VT<int, int>, int, int, int, int, int, int, VT<int, int, VT<int, int>, VT<int, int>>

The former is more specific on the first type parameter of the argument, and they agree on type arguments 2-7, and in type argument position 8 we have a value of type

VT<int, int, VT<int, int>, VT<int, int>

for which neither

VT<int, int, VT<int, U>, V>

nor

VT<int, int, U, VT<int, V>>

is more specific.

Since the former is more specific for type argument 1 of its first parameter, and the second is not more specific for any type argument of any parameter, the first overload is more specific.

If I understood correctly, there is tension between the flat view of tuples and the nested view of ValueTuple. The trouble is that the nested view is what determines the behavior when compiling from C# 6. So it seems we have to adhere to that (although I prefer the flat view when possible) :-(

In reply to: 125343475 [](ancestors = 125343475)

This looks not desirable. In all other places when N-ary nature of tuples is in conflict with the underlying implementation - the tuple semantica wins.
This behavior is an artefact of recursing via underlying type instead of recursing via TupleElementTypes. Initially it looked as if there was no difference. Now, since there is an observable difference the recursion should happen via TupleElementTypes.

100-element tuple is primarily a flat aggregate of 100 typed elements. The part where it is mapped to a 10+ times nested struct is an implementation detail and plays secondary part. The 'tuple' semantics takes precedence when there is a conflict.
The primary goal of tuples is to provide tuple experience not the nested struct experience.

In all other places when N-ary nature of tuples is in conflict with the underlying implementation - the tuple semantics wins.

@VSadov I can't think of any places where we changed the semantics from those already defined for C# 6 (for instances of System.ValueTuple). Though I sympathize with the desire to think of a 100-long tuple as being generic with arity 100, that would be an incompatibility with code compiled using the C# 6 compiler. The mapping to nested generics isn't merely an implementation detail, it is part of the language specification and it affects examples like these.

We should bring this up to the LDM. I think in the past the guiding principle was that the tuple aspect takes precedent in conflicts like this.

I think in the past the guiding principle was that the tuple aspect takes precedent in conflicts like this.

Can you give an example where there was a conflict resolved in this way (for code that was legal in C# 6)?

jcouv

LGTM Thanks

gafter · 2017-07-04T00:31:54Z

@VSadov Do these changes address your suggestions? Do you have any further suggestions?

VSadov

The recursion should happen via TupleElementTypes not via underlying type. In all other places where there is an observable difference we recurse via tuple elements.

gafter · 2017-07-04T22:03:39Z

@VSadov

The recursion should happen via TupleElementTypes not via underlying type. In all other places where there is an observable difference we recurse via tuple elements.

We recurse over tuple elements for the new semantics that we added for tuples. I am not aware of any effort to change the behavior of overload resolution or type inference for tuples except as a side-effect of the addition of some new conversions.

In any case, doing as you suggest would be a breaking change. See MoreGenericTieBreaker_03, which compiles without error, even if converted to use the explicit ValueTuple syntax. With your suggestion, such code would no longer compile. As far as I know there is no specified or implemented change to type inference that was intended to make such code illegal.

VSadov

Per LDM discussion the behavior of the tiebreaker with long tuples as implemented in this PR seems mildly better.

There is no large semantical benefit from doing this, one way or another, since the scenario where the distinction is observed seems to be highly unlikely to happen in real code.
Implementation costs are also roughly equal.
While compatibility with C#6 would not be considered a key value for scenarios like this, it did tip the scales towards the behavior as implemented in this PR.

gafter added 2 commits June 30, 2017 14:03

Add test

ab2e582

Fix one symptom of the non-generic tuple type issue

8122d00

Fixes dotnet#20494 Adds a test for dotnet#20583

gafter added 4 - In Review A fix for the issue is submitted for review. Area-Compilers Bug Feature - Tuples Tuples labels Jun 30, 2017

gafter added this to the 15.5 milestone Jun 30, 2017

dnfclas added the cla-already-signed label Jun 30, 2017

gafter self-assigned this Jun 30, 2017

VSadov reviewed Jun 30, 2017

View reviewed changes

gafter mentioned this pull request Jun 30, 2017

Inference failure for tuple type vs tuple underlying type (regression) #20583

Closed

VSadov reviewed Jun 30, 2017

View reviewed changes

jcouv approved these changes Jul 3, 2017

View reviewed changes

Fix for VB and additional tests using tuple syntax.

0bf15f7

VSadov requested changes Jul 4, 2017

View reviewed changes

VSadov approved these changes Jul 5, 2017

View reviewed changes

gafter merged commit 3565c81 into dotnet:master Jul 9, 2017

gafter added Resolution-Fixed The bug has been fixed and/or the requested behavior has been implemented and removed 4 - In Review A fix for the issue is submitted for review. labels Jul 9, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes overload resolution regression with ValueTuple #20587

Fixes overload resolution regression with ValueTuple #20587

gafter commented Jun 30, 2017

VSadov Jun 30, 2017

gafter Jul 3, 2017

gafter Jul 4, 2017

VSadov Jun 30, 2017 •

edited

Loading

gafter Jul 4, 2017

VSadov Jun 30, 2017

gafter Jul 3, 2017

VSadov Jul 3, 2017 •

edited

Loading

gafter Jul 3, 2017

jcouv Jul 3, 2017

VSadov Jul 4, 2017

VSadov Jul 4, 2017

gafter Jul 4, 2017

VSadov Jul 4, 2017

gafter Jul 4, 2017

jcouv left a comment

gafter commented Jul 4, 2017

VSadov left a comment

gafter commented Jul 4, 2017

VSadov left a comment •

edited

Loading

Fixes overload resolution regression with ValueTuple #20587

Fixes overload resolution regression with ValueTuple #20587

Conversation

gafter commented Jun 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VSadov Jun 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VSadov Jul 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

gafter commented Jul 4, 2017

VSadov left a comment

Choose a reason for hiding this comment

gafter commented Jul 4, 2017

VSadov left a comment • edited Loading

Choose a reason for hiding this comment

VSadov Jun 30, 2017 •

edited

Loading

VSadov Jul 3, 2017 •

edited

Loading

VSadov left a comment •

edited

Loading