Lookup for Deconstruct method #11873

jcouv · 2016-06-08T23:39:05Z

Previously the finding of the Deconstruct method was very simple (find a member called "Deconstruct" and with the appropriate number of parameters).
This PR implements a full resolution, including base classes and extension methods.

To do this, we basically generate an invocation that looks like rhs.Deconstruct(out var x1, out var x2, ...). So the change is modeled after Aleksey's recent out vars change.

The trick here is that the invocation receives a new kind of out var bound node, called OutDeconstructVarPendingInference. When the overload resolution is complete, those get replaced with placeholder of the inferred type.
Those placeholders are collected into a BoundDeconstructionDeconstructStep as before, so that they can be replaced during lowering. This deconstruct step now holds the invocation expression generated above instead of just the MethodSymbol for Deconstruct.

The reason for not simply re-using the bound node introduced for out var (OutVarLocalPendingInference, which is in the features/outvar branch at the moment) because that one generates a local after the resolution completes.

CC @dotnet/roslyn-compiler for review.

cston · 2016-06-09T06:35:06Z

src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs

+
+                BoundExpression result = BindInvocationExpression(
+                                            receiverSyntax, receiverSyntax, methodName, boundExpression, analyzedArguments, diagnostics, queryClause: null,
+                                            allowUnexpandedForm: true);


Will this bind successfully to a field: dynamic Deconstruct;? Is that allowed?

I added two tests: one with an object Deconstruct field (which properly reports that no invocable Deconstruct method was found), and one with dynamic Deconstruct field (as you suggested).
This latter one crashes. I'll investigate and fix.
Thanks for the suggestion!

I added a check that the member we're trying to use is a MethodGroup before we proceed with its invocation.

If we're checking for MethodGroup consider calling BindMethodGroupInvocation directly instead of BindInvocationExpression.

Done (calling BindMethodGroupInvocation directly).

VSadov · 2016-06-09T22:15:10Z

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

+        [Fact]
+        public void DeconstructUsingDynamicMethod()
+        {
+            string source = @"


but Deconstruct can have dynamic parameters, right? - Deconstruct(out dynamic arg1)

Yes. The "Dynamic" test covers the scenario (dynamic types on the left and as output parameters):

class C { dynamic Dynamic1; dynamic Dynamic2; static void Main() { C c = new C(); (c.Dynamic1, c.Dynamic2) = c; System.Console.WriteLine(c.Dynamic1 + "" "" + c.Dynamic2); } public void Deconstruct(out int a, out dynamic b) { a = 1; b = ""hello""; } }

VSadov · 2016-06-10T00:08:31Z

Some more test scenarios to try:

void Deconstruct(out int x, out int y, int z = 42) // is this an operator?
void Deconstruct(out int x, out int y, params int[] z) // is this an operator?
void Deconstruct(out int x, out int y, __arglist) // is this an operator?
Is this an operator?

    public delegate void D1(out int x, out int y);

    class c1
    {
        public D1 Deconstruct;
    }

is this an operator:

    public delegate void D1(out int x, out int y);

    class c1
    {
        public event D1 Deconstruct;

        static void Test()
        {
            var c = new c1();

            int x;

            // does this work?
            (x, x) = c;
        }
    }

in the above examples, assuming they are not operators, if base has normal void Deconstruct(out int x, out int y), does it work?
try defining Deconstruct in VB (I think you need to use [Out] attribute), low priority. It should work in theory

cston · 2016-06-10T16:52:03Z

src/Compilers/CSharp/Portable/BoundTree/OutDeconstructVarPendingInference.cs

+
+        public BoundDeconstructValuePlaceholder FailInference(Binder binder)
+        {
+            return SetInferredType(binder.CreateErrorType("var"), success: false);


When there is no obvious type name, other callers use CreateErrorType() with the default name argument value.

Done. Thanks

jcouv · 2016-06-10T17:32:28Z

@VSadov The latest commit adds the tests you suggested (except for VB one, which I recorded as work item).

The results:

Deconstruct with optional parameter and params parameter is resolved.
But not Deconstruct delegate or event or method with __arglist. In the __arglist case, you can have a proper Deconstruct next to it, and it will get picked up.

cston · 2016-06-10T20:17:29Z

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

@@ -924,7 +924,7 @@ private static bool HadLambdaConversionError(DiagnosticBag diagnostics, Analyzed
            // If the expression is untyped because it is a lambda, anonymous method, method group or null
            // then we never want to report the error "you need a ref on that thing". Rather, we want to
            // say that you can't convert "null" to "ref int".
-            if (!argument.HasExpressionType())
+            if (!argument.HasExpressionType() && argument.Kind != BoundKind.OutDeconstructVarPendingInference)


Is there a test for this case (where the out var has no type)?

Yes, for instance "DeconstructWithInParam" reaches this code and the second argument has no type.

static void Main() { int x; int y; (x, y) = new C(); } public void Deconstruct(out int x, int y) { x = 1; }

The overload resolution code generates this error, a few lines below:
// (8,9): error CS1615: Argument 2 may not be passed with the 'out' keyword

VSadov · 2016-06-11T01:43:55Z

src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs

+
+                var deconstructMethod = ((BoundCall)result).Method;
+                var parameters = deconstructMethod.IsExtensionMethod ? deconstructMethod.Parameters.Skip(1) : deconstructMethod.Parameters;
+                if (parameters.Any(p => p.RefKind != RefKind.Out && !p.IsThis))


Since you are skipping the first parameter for extension methods, do you need to check "IsThis" ?

It might be easier to just do a small for loop here and start with 1 in case of extension methods. That would also avoid allocation since skip allocates. That is not a big deal though.

Yes, I'll remove IsThis. It did not do what I expected (ie. returning true on the first parameter of the extension method) anyways. I'll change to a loop as you suggested too.

VSadov · 2016-06-11T01:51:06Z

LGTM

VSadov · 2016-06-11T01:52:17Z

@jcouv - I am not sure if unacceptable deconstruct methods should shadow correct ones from the base. We need to check that with LDM, maybe.

I.E. if derived has Deconstruct(out int x, out int y, int x = 1) while base has Deconstruct(out int x, out int y). Should we ignore the derived one and pick the base one or should we fail?

AlekseyTs · 2016-06-12T20:41:11Z

docs/features/deconstruction.md

+
+The resolution is equivalent to typing `rhs.Deconstruct(out var x1, out var x2, ...);` with the appropriate number of parameters to deconstruct into.
+It is based on normal overload resolution.
+This implies that `rhs` cannot be dynamic.


I am not sure if the Out Var limitation should necessarily translate into limitation for Deconstruction. I think it would be reasonable to treat all the output values as dynamic if the rhs is dynamic. #Resolved

I'll take a note for LDM on the dynamic question.

gafter · 2016-06-13T17:54:50Z

src/Compilers/CSharp/Portable/BoundTree/OutDeconstructVarPendingInference.cs

+    {
+        public BoundDeconstructValuePlaceholder Placeholder;
+
+        public BoundDeconstructValuePlaceholder SetInferredType(TypeSymbol type, bool success)


This is implementing an essentially mutable bound node, which violates one of the design principles of the bound nodes. Why can this happen only once? What happens during overload resolution?

Overload resolution doesn't mutate them. Coercion of the arguments does, which is done once after target method is known.

And we do have other examples of mutable bound nodes, UnboundLambda, for example. So, the principle is not that sacred, but we do prefer to follow it, unless there are good reasons not to.

UnboundLambda's mutation is just mutation of a cache. It acts the same no matter what order entries are placed in the cache.

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

Coercion of the arguments should create new nodes, not modify existing nodes.

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

It replaces the nodes in the tree, the original nodes (the ones that are mutated are removed from the tree). And the property that is mutated is a cache.

AlekseyTs · 2016-06-13T18:03:37Z

LGTM, but could you please add a test with use-site error scenario?

gafter · 2016-06-13T18:10:40Z

Please revise the bound tree to not be mutable, and I'll continue reviewing then.

gafter · 2016-06-13T18:11:58Z

src/Compilers/CSharp/Portable/BoundTree/Formatting.cs

+            get
+            {
+                return MessageID.IDS_FeatureTuples.Localize();
+            }


I don't understand how this could make sense. Do you have a test for it?

This is the same as #11493. I did not investigate in much details.

This code is not meant to be reachable.

More info: OverloadResolutionResult.ReportDiagnostics checks that all the arguments have a non-null Display.

// Each argument must have non-null Display in case it is used in a diagnostic. Debug.Assert(arguments.Arguments.All(a => a.Display != null));

I'd be fine with Display returning an empty string for this out var node.

Fine with me, if you change it here, I'll make similar change for Out Vars.

Changed to return string.Empty

jcouv · 2016-06-13T18:14:34Z

@gafter, I don't see a reasonable way to avoid mutating the out var bound nodes. The options I see and that we discussed are:

extensive re-writing of overload resolution
fishing for the placeholders in the tree returned by BindMethodGroupInvocation

Both seem less desirable than the current solution.

Also, note that in the current solution coercion does indeed return new nodes.

gafter · 2016-06-13T18:19:15Z

@jcouv When you bind something for the purpose of getting information about it, you should expect to look at the result of that binding to see what you got. In this case it means essentially looking at the arguments of the invocation. Each should be a direct placeholder, so no fishing should be needed.

AlekseyTs · 2016-06-13T18:27:01Z

@gafter I think it is not constructive and counter-productive to stop your review process in ultimatum form just because you run int something you disagree with. There are plenty other changes in this PR that could benefit from more eyes. We can continue discussing that particular issue.

VSadov · 2016-06-13T19:33:38Z

LGTM

jcouv · 2016-06-13T19:41:18Z

Latest commit addresses the this morning's feedback. I'll go ahead with checkin once CI is green.

AlekseyTs · 2016-06-13T19:52:42Z

src/Compilers/CSharp/Portable/FlowAnalysis/DataFlowPass.cs

@@ -1735,6 +1735,12 @@ public override BoundNode VisitDeconstructionAssignmentOperator(BoundDeconstruct
            return null;
        }

+        public override BoundNode VisitOutDeconstructVarPendingInference(OutDeconstructVarPendingInference node)


I think this should go into PreciseAbstractFlowPass and be sealed. Also, should we have the same override in the LocalRewriter?

Done. Thanks

AlekseyTs · 2016-06-13T20:27:20Z

LGTM

jcouv added 2 commits June 7, 2016 16:46

Refreshing ValueTuple library

43cf0c5

Lookup for Deconstruct method

c52e3d4

jcouv added Area-Compilers New Language Feature - Tuples Tuples labels Jun 8, 2016

jcouv self-assigned this Jun 8, 2016

dnfclas added the cla-already-signed label Jun 8, 2016

cston reviewed Jun 9, 2016
View reviewed changes

PR feedback (1)

cc63d93

jcouv force-pushed the tuple-deconstruction branch from f030603 to cc63d93 Compare June 9, 2016 18:35

VSadov reviewed Jun 9, 2016
View reviewed changes

cston reviewed Jun 10, 2016
View reviewed changes

jcouv mentioned this pull request Jun 10, 2016

[Umbrella] Deconstruction work items #11299

Closed

Adding suggested tests

9b003a7

cston reviewed Jun 10, 2016
View reviewed changes

PR feedback (2)

0d86cc0

jcouv force-pushed the tuple-deconstruction branch from 740ba17 to 0d86cc0 Compare June 10, 2016 23:45

VSadov reviewed Jun 11, 2016
View reviewed changes

AlekseyTs reviewed Jun 12, 2016
View reviewed changes

jcouv added 3 commits June 13, 2016 08:50

PR feedback (4)

f03fc0d

Reporting overload resolution errors plus Deconstruct message

0e0bda3

Test with inaccessible method

bc3bd43

gafter reviewed Jun 13, 2016
View reviewed changes

Empty Display string, test for use-site errors, Visit throws

3496abb

jcouv force-pushed the tuple-deconstruction branch from 7ab1ab4 to 3496abb Compare June 13, 2016 19:39

AlekseyTs reviewed Jun 13, 2016
View reviewed changes

Moving throwing Visit code and adding to lower rewriter too

079ccce

jcouv merged commit 8e15fe4 into dotnet:features/tuples Jun 13, 2016

jcouv deleted the tuple-deconstruction branch June 13, 2016 22:48

pavel-mikula-sonarsource mentioned this pull request Sep 23, 2020

S1128 FP: Custom namespace is needed for ValueTuple deconstruction in .NET Framework SonarSource/sonar-dotnet#3631

Open

Lookup for Deconstruct method #11873

Lookup for Deconstruct method #11873

Conversation

jcouv commented Jun 8, 2016

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

VSadov commented Jun 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv commented Jun 10, 2016

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

VSadov commented Jun 11, 2016

VSadov commented Jun 11, 2016

AlekseyTs Jun 12, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gafter Jun 13, 2016 • 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

AlekseyTs commented Jun 13, 2016

gafter commented Jun 13, 2016

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 commented Jun 13, 2016

gafter commented Jun 13, 2016

AlekseyTs commented Jun 13, 2016

VSadov commented Jun 13, 2016

jcouv commented Jun 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented Jun 13, 2016

AlekseyTs Jun 12, 2016 •

edited

Loading

gafter Jun 13, 2016 •

edited

Loading