Improve method type argument inference around tuples and address backwards compatibility break. #22295

AlekseyTs · 2017-09-22T18:50:13Z

Fixes #20583.

@dotnet/roslyn-compiler, @VSadov, @jcouv Please review.

…wards compatibility break. Fixes dotnet#20583.

AlekseyTs · 2017-09-22T18:50:35Z

CC @jaredpar for approval

jaredpar · 2017-09-22T19:52:51Z

Approved pending signoffs

gafter · 2017-09-22T20:34:08Z

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

            {
-                MakeExplicitParameterTypeInferences(binder, (BoundTupleLiteral)argument, target, isExactInference, ref useSiteDiagnostics);
+                ;


; [](start = 16, length = 1)

Please add a comment explaining the control-flow here (perhaps instead of the empty statement)

@gafter

Please add a comment explaining the control-flow here (perhaps instead of the empty statement)

Would saying "We were able to do inference from elements of the tuple literal, no need to try to infer from literal's type" be sufficient?

I'd just make it a sequence of separate if statements with returns. Trying to preserve "else if" indeed makes it look strange.

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

@AlekseyTs Yes, that would be a perfect comment to help the reader.

gafter · 2017-09-22T20:44:16Z

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

+        M2((()=>1, ()=>2, ()=>3, ()=>4, ()=>5, ()=>6, ()=>7, ()=>8));
+    }
+    public static void M2<T1, T2, T3, T4, T5, T6, T7, T8>(ValueTuple<Func<T1>, Func<T2>, Func<T3>, Func<T4>, Func<T5>, Func<T6>, Func<T7>, ValueTuple<Func<T8>>> a) { Console.Write(2); }
+    public static void M2<T1, T2, T3, T4, T5, T6, T7, TRest>(ValueTuple<Func<T1>, Func<T2>, Func<T3>, Func<T4>, Func<T5>, Func<T6>, Func<T7>, TRest> a) where TRest : struct { Console.Write(3); }


public [](start = 4, length = 6)

The following test demonstrates that the behavior here is due to a failure to infer type parameters for the second overload. Can you formulate a test in which the tuple does not have a (natural) type, type inference would succeed with both overloads, and the appropriate overload is selected due to the fix? Perhaps one way to do this would be to cast the eighth member of the tuple literal to Func<int>.

Can you formulate a test in which the tuple does not have a (natural) type, type inference would succeed with both overloads, and the appropriate overload is selected due to the fix? Perhaps one way to do this would be to cast the eighth member of the tuple literal to Func.

I don't think it is possible given the current language rules. The type for TRest cannot be inferred unless the tuple literal has a natural type, the ValueTupe`1 is never materialized until we are ready to create a type for the entire tuple.

Potentially, we could implement such inference, but it is outside of scope of this PR.

I understand now. Thanks!

gafter · 2017-09-22T20:50:20Z

src/Compilers/VisualBasic/Portable/Semantics/TypeInference/TypeArgumentInference.vb

@@ -586,7 +586,8 @@ HandleAsAGeneralExpression:
                            ParameterType,
                            Parameter,
                            MatchGenericArgumentToParameter.MatchBaseOfGenericArgumentToParameter,
-                            inferenceRestrictions)
+                            inferenceRestrictions,
+                            addedHints:=Nothing)


addedHints [](start = 28, length = 10)

I didn't find where addedHints is used. What is its purpose in the context of this change?

It is used on line 1659 in this file (https://github.com/dotnet/roslyn/pull/22295/files#diff-6e2be44971b9721836d1f298f24ccae7R1659).

What is its purpose in the context of this change? e.g. how it is used to more correctly implement the language feature, if it fixes a bug, and what is the symptom that is fixed? It isn't obvious what the purpose of the change on line 1659 is.

@gafter The purpose of the logic on line 1659 is to continue with "indirect" inference when direct inference doesn't run into any error conditions and doesn't result in any new hints.

A number of tuple tests were affected by this as well as non-tuple scenario (see ShapeMismatchInOneArgument unit-test in TypeArgumentInference.vb). Before, the indirect inference was performed only when InferTypeArgumentsFromArgumentDirectly was returning false.

gafter · 2017-09-22T20:57:26Z

Finished review pass.

VSadov · 2017-09-22T23:05:01Z

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

@@ -1604,13 +1577,13 @@ private bool ExactConstructedInference(TypeSymbol source, TypeSymbol target, ref
            // SPEC:   type C<U1...Uk> then an exact inference 
            // SPEC:   is made from each Ui to the corresponding Vi.

-            var namedSource = source as NamedTypeSymbol;
+            var namedSource = source.TupleUnderlyingTypeOrSelf() as NamedTypeSymbol;


NIT: I'd put this in the caller to be sure unwrap is not called when we do not expect. However, since there is only one caller, unwrapping here seems ok as well.

VSadov

VSadov

LGTM

gafter · 2017-09-25T19:41:13Z

src/Compilers/VisualBasic/Portable/Semantics/TypeInference/TypeArgumentInference.vb

                ' First try to the things directly. Only if this fails will we bother searching for things like List->IEnumerable.
-                Dim Inferred As Boolean = InferTypeArgumentsFromArgumentDirectly(
+                Dim addedDirectHints As Boolean = False
+                Dim directInferenceResult As Boolean = InferTypeArgumentsFromArgumentDirectly(


Offline, @AlekseyTs explained to me that this is being used to fix a separate, unreported bug in the VB compiler whereby an invocation fails in certain situations when one argument fails to contribute to the inference of one type parameter, even though that type parameter can be inferred from another argument. See the change in TypeArgumentInference.vb for a test more focused on that bug. However, it seems that this fix should be done on a type parameter by type parameter basis, as the language requires each type parameter to be inferred from some argument (not just one of them). By tracking this as a single bool, it would appear to permit an invocation to succeed (because some type parameter was inferred from an argument) even though some other type parameter was not inferred from an argument. I think a correct fix would require a Dictionary<TypeParameterSymbol, Boolean> rather than a single Boolean to be correct.

I would suggest either fixing this correcly, or leaving this bug unfixed in VB for now (i.e. reverting the VB changes).

AlekseyTs · 2017-09-25T23:50:35Z

@gafter I believe your feedback has been addressed.

gafter · 2017-09-26T20:24:39Z

src/Compilers/VisualBasic/Portable/Semantics/TypeInference/TypeArgumentInference.vb

@@ -1633,6 +1629,10 @@ HandleAsAGeneralExpression:
                inferenceRestrictions As RequiredConversion
            ) As Boolean

+                If Not RefersToGenericParameterToInferArgumentFor(parameterType) Then


RefersToGenericParameterToInferArgumentFor [](start = 23, length = 42)

I think the name of RefersToGenericParameterToInferArgumentFor should be RefersToGenericParameterToInferTypeArgument if I understand its purpose.

gafter

* dotnet/master: (35 commits) removed unused options (dotnet#22389) Make readonlyness of ref-ternary the AND of operands (dotnet#22384) Refine parsing look-ahead for type arguments (dotnet#22102) IDE Features fix for code generation for ref-readonly (dotnet#22285) Added tests for `out` and `ref` completion in local function Record "allow digit separator after 0b or 0x" (dotnet#22371) Responded to feedback Don't show snippets in VB comments Fix bug when trailing and leading underscores are not the same (dotnet#22141) Add breaking change doc for TypedReference delegate conversion. (dotnet#22084) Add no-PDB-path test for VB Ref ternary shoudl be shielded form optimization in the containing expression that may expose it to mutations Improve method type argument inference around tuples and address backwards compatibility break. (dotnet#22295) Added couple more tests Add unit tests for IStopStatement and IEndStatement and make the APIs public again (dotnet#22225) Revert changes to compilers.sln Support for object initializers. supports all expressions in escape analysis that can possibly produce ref-like values Update tests after merge. Add an option to disable name suggestions ...

Improve method type argument inference around tuples and address back…

6fda90f

…wards compatibility break. Fixes dotnet#20583.

AlekseyTs added Area-Compilers Language-C# Language-VB New Language Feature - Tuples Tuples Tenet-Compatibility Violation of forwards/backwards compatibility in a design-time piece. labels Sep 22, 2017

dnfclas added the cla-already-signed label Sep 22, 2017

AlekseyTs requested review from VSadov, jcouv and a team September 22, 2017 18:50

gafter reviewed Sep 22, 2017

View reviewed changes

VSadov reviewed Sep 22, 2017

View reviewed changes

VSadov approved these changes Sep 22, 2017

View reviewed changes

CR feedback

bf18ff5

gafter reviewed Sep 25, 2017

View reviewed changes

Revert an attempt to fix dotnet#22329.

9fd21d4

gafter reviewed Sep 26, 2017

View reviewed changes

gafter approved these changes Sep 26, 2017

View reviewed changes

AlekseyTs merged commit 133364a into dotnet:master Sep 26, 2017

jcouv added the Approved to merge label Sep 29, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve method type argument inference around tuples and address backwards compatibility break. #22295

Improve method type argument inference around tuples and address backwards compatibility break. #22295

AlekseyTs commented Sep 22, 2017

AlekseyTs commented Sep 22, 2017

jaredpar commented Sep 22, 2017

gafter Sep 22, 2017

AlekseyTs Sep 22, 2017

VSadov Sep 22, 2017

gafter Sep 23, 2017

gafter Sep 22, 2017

AlekseyTs Sep 22, 2017 •

edited

Loading

AlekseyTs Sep 22, 2017

gafter Sep 23, 2017

gafter Sep 22, 2017

AlekseyTs Sep 22, 2017

gafter Sep 23, 2017

AlekseyTs Sep 25, 2017

AlekseyTs Sep 25, 2017

gafter commented Sep 22, 2017

VSadov Sep 22, 2017

VSadov left a comment

VSadov left a comment

gafter Sep 25, 2017

AlekseyTs commented Sep 25, 2017

gafter Sep 26, 2017

gafter left a comment

Improve method type argument inference around tuples and address backwards compatibility break. #22295

Improve method type argument inference around tuples and address backwards compatibility break. #22295

Conversation

AlekseyTs commented Sep 22, 2017

AlekseyTs commented Sep 22, 2017

jaredpar commented Sep 22, 2017

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

AlekseyTs Sep 22, 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

gafter commented Sep 22, 2017

Choose a reason for hiding this comment

VSadov left a comment

Choose a reason for hiding this comment

VSadov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented Sep 25, 2017

Choose a reason for hiding this comment

gafter left a comment

Choose a reason for hiding this comment

AlekseyTs Sep 22, 2017 •

edited

Loading