Lookup for Deconstruct method #11873
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,5 @@ | ||
| // Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
|
||
| using System.Collections.Generic; | ||
| using System.Collections.Immutable; | ||
| using System.Diagnostics; | ||
|
|
@@ -1831,7 +1830,7 @@ private BoundExpression BindDeconstructionAssignment(AssignmentExpressionSyntax | |
| /// This will generate and stack appropriate deconstruction and assignment steps for a non-tuple type. | ||
| /// Returns null if there was an error (if a suitable Deconstruct method was not found). | ||
| /// </summary> | ||
| private BoundDeconstructionDeconstructStep MakeNonTupleDeconstructStep( | ||
| BoundDeconstructValuePlaceholder targetPlaceholder, | ||
| AssignmentExpressionSyntax syntax, | ||
| DiagnosticBag diagnostics, | ||
|
|
@@ -1840,16 +1839,22 @@ private BoundExpression BindDeconstructionAssignment(AssignmentExpressionSyntax | |
| ArrayBuilder<BoundDeconstructionAssignmentStep> assignmentSteps) | ||
| { | ||
| // symbol and parameters for Deconstruct | ||
| ImmutableArray<BoundDeconstructValuePlaceholder> outPlaceholders; | ||
| var deconstructInvocation = MakeDeconstructInvocationExpression(variables.Length, targetPlaceholder, syntax, diagnostics, out outPlaceholders); | ||
|
|
||
| if (deconstructInvocation.HasAnyErrors) | ||
| { | ||
| return null; | ||
| } | ||
| else | ||
| { | ||
| return new BoundDeconstructionDeconstructStep(syntax, deconstructInvocation, targetPlaceholder, outPlaceholders); | ||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Holds the variables on the LHS of a deconstruction as a tree of bound expressions. | ||
| /// </summary> | ||
| private class DeconstructionVariable | ||
| { | ||
| public readonly BoundExpression Single; | ||
|
|
@@ -1898,7 +1903,7 @@ public DeconstructionVariable(ImmutableArray<DeconstructionVariable> variables) | |
| } | ||
| else | ||
| { | ||
| var assignment = MakeDeconstructionAssignmentStep(variable.Single, valuePlaceholder.Type, valuePlaceholder, syntax, diagnostics); | ||
| assignmentSteps.Add(assignment); | ||
| } | ||
| } | ||
|
|
@@ -1962,7 +1967,9 @@ private ImmutableArray<DeconstructionVariable> BindDeconstructionVariables(Separ | |
| /// <summary> | ||
| /// Figures out how to assign from sourceType into receivingVariable and bundles the information (leaving holes for the actual source and receiver) into an AssignmentInfo. | ||
| /// </summary> | ||
| private BoundDeconstructionAssignmentStep MakeDeconstructionAssignmentStep( | ||
| BoundExpression receivingVariable, TypeSymbol sourceType, BoundDeconstructValuePlaceholder inputPlaceholder, | ||
| AssignmentExpressionSyntax node, DiagnosticBag diagnostics) | ||
| { | ||
| var outputPlaceholder = new BoundDeconstructValuePlaceholder(receivingVariable.Syntax, receivingVariable.Type) { WasCompilerGenerated = true }; | ||
|
|
||
|
|
@@ -1996,55 +2003,114 @@ static private void FlattenDeconstructVariables(ImmutableArray<DeconstructionVar | |
| } | ||
|
|
||
| /// <summary> | ||
| /// Find the Deconstruct method for the expression on the right, that will fit the number of assignable variables on the left. | ||
| /// Returns an invocation expression if the Deconstruct method is found. | ||
| /// If so, it outputs placeholders that were coerced to the output types of the resolved Deconstruct method. | ||
| /// The overload resolution is similar to writing `receiver.Deconstruct(out var x1, out var x2, ...)`. | ||
| /// </summary> | ||
| private BoundExpression MakeDeconstructInvocationExpression( | ||
| int numCheckedVariables, BoundExpression receiver, AssignmentExpressionSyntax assignmentSyntax, | ||
| DiagnosticBag diagnostics, out ImmutableArray<BoundDeconstructValuePlaceholder> outPlaceholders) | ||
| { | ||
| var receiverSyntax = receiver.Syntax; | ||
|
|
||
| if (receiver.Type.IsDynamic()) | ||
| { | ||
| Error(diagnostics, ErrorCode.ERR_CannotDeconstructDynamic, receiverSyntax); | ||
| outPlaceholders = default(ImmutableArray<BoundDeconstructValuePlaceholder>); | ||
|
|
||
| return BadExpression(receiverSyntax, receiver); | ||
| } | ||
|
|
||
| var analyzedArguments = AnalyzedArguments.GetInstance(); | ||
| var outVars = ArrayBuilder<OutDeconstructVarPendingInference>.GetInstance(numCheckedVariables); | ||
| DiagnosticBag bag = null; | ||
|
|
||
| try | ||
| { | ||
| for (int i = 0; i < numCheckedVariables; i++) | ||
| { | ||
| var variable = new OutDeconstructVarPendingInference(assignmentSyntax); | ||
| analyzedArguments.Arguments.Add(variable); | ||
| analyzedArguments.RefKinds.Add(RefKind.Out); | ||
| outVars.Add(variable); | ||
| } | ||
|
|
||
| const string methodName = "Deconstruct"; | ||
| var memberAccess = BindInstanceMemberAccess( | ||
| receiverSyntax, receiverSyntax, receiver, methodName, rightArity: 0, | ||
|
There was a problem hiding this comment. Do we have a test when Deconstruct method is a generic extension method? #Resolved There was a problem hiding this comment. Not yet. Thanks for the suggestion. I'll add. There was a problem hiding this comment. Actually, the tests for deconstructing |
||
| typeArgumentsSyntax: default(SeparatedSyntaxList<TypeSyntax>), typeArguments: default(ImmutableArray<TypeSymbol>), | ||
| invoked: true, diagnostics: diagnostics); | ||
|
|
||
| memberAccess = CheckValue(memberAccess, BindValueKind.RValueOrMethodGroup, diagnostics); | ||
| memberAccess.WasCompilerGenerated = true; | ||
|
|
||
| if (memberAccess.Kind != BoundKind.MethodGroup) | ||
| { | ||
| return MissingDeconstruct(receiver, assignmentSyntax, numCheckedVariables, diagnostics, out outPlaceholders, receiver); | ||
| } | ||
|
|
||
| // After the overload resolution completes, the last step is to coerce the arguments with inferred types. | ||
| // That step returns placeholder (of correct type) instead of the outVar nodes that were passed in as arguments. | ||
| // So the generated invocation expression will contain placeholders instead of those outVar nodes. | ||
| // Those placeholders are also recorded in the outVar for easy access below, by the `SetInferredType` call on the outVar nodes. | ||
| bag = DiagnosticBag.GetInstance(); | ||
| BoundExpression result = BindMethodGroupInvocation( | ||
| receiverSyntax, receiverSyntax, methodName, (BoundMethodGroup)memberAccess, analyzedArguments, bag, queryClause: null, | ||
| allowUnexpandedForm: true); | ||
|
There was a problem hiding this comment. Will this bind successfully to a field: There was a problem hiding this comment. I added two tests: one with an There was a problem hiding this comment. I added a check that the member we're trying to use is a There was a problem hiding this comment. If we're checking for There was a problem hiding this comment. Done (calling |
||
|
|
||
| result.WasCompilerGenerated = true; | ||
| diagnostics.AddRange(bag); | ||
|
|
||
| if (bag.HasAnyErrors()) | ||
| { | ||
| return MissingDeconstruct(receiver, assignmentSyntax, numCheckedVariables, diagnostics, out outPlaceholders, result); | ||
|
There was a problem hiding this comment. I suspect that at this point the There was a problem hiding this comment. I'll stop by to discuss with Vlad and you today. There was a problem hiding this comment. The latest commit I pushed shows what the errors look like if we put both (the regular errors from overload resolution plus the Deconstruct-specific one). |
||
| } | ||
|
|
||
| // Verify all the parameters (except "this" for extension methods) are out parameters | ||
| if (result.Kind != BoundKind.Call) | ||
| { | ||
| return MissingDeconstruct(receiver, assignmentSyntax, numCheckedVariables, diagnostics, out outPlaceholders, result); | ||
| } | ||
|
|
||
| var deconstructMethod = ((BoundCall)result).Method; | ||
|
There was a problem hiding this comment. The conversion to BoundCall doesn't feel safe. If BindMethodGroupInvocation always returns BoundCall, we should change its return type to BoundCall and avoid conversion here. #Resolved There was a problem hiding this comment. It could return |
||
| var parameters = deconstructMethod.Parameters; | ||
| for (int i = (deconstructMethod.IsExtensionMethod ? 1 : 0); i < parameters.Length; i++) | ||
| { | ||
| if (parameters[i].RefKind != RefKind.Out) | ||
| { | ||
| return MissingDeconstruct(receiver, assignmentSyntax, numCheckedVariables, diagnostics, out outPlaceholders, result); | ||
|
There was a problem hiding this comment. Is this code reachable? #Resolved There was a problem hiding this comment. Yes, in the tests that involve optional or params parameters. For example, |
||
| } | ||
| } | ||
|
|
||
| if (outVars.Any(v => (object)v.Placeholder == null)) | ||
| { | ||
| return MissingDeconstruct(receiver, assignmentSyntax, numCheckedVariables, diagnostics, out outPlaceholders, result); | ||
| } | ||
|
|
||
| outPlaceholders = outVars.SelectAsArray(v => v.Placeholder); | ||
|
There was a problem hiding this comment. Is There was a problem hiding this comment. Yes, if (!kind.IsIdentity)
{
TypeSymbol type = GetCorrespondingParameterType(ref result, parameters, arg);
// [removed some code]
arguments[arg] = CreateConversion(argument.Syntax, argument, kind, false, type, diagnostics);
}
else if (argument.Kind == BoundKind.OutDeconstructVarPendingInference)
{
TypeSymbol parameterType = GetCorrespondingParameterType(ref result, parameters, arg);
arguments[arg] = ((OutDeconstructVarPendingInference)argument).SetInferredType(parameterType, success: true);
}As for the There was a problem hiding this comment. It seems fragile. You are building a mutable bound tree, then calling some binding methods and as sideeffect of binding the nodes are supposed to be mutated to the right values. It is hard to track when that happens, why it is guaranteed to happen. Why it would happen only once. This is the only place where you are relying on implicit mutations during binding. There was a problem hiding this comment. Given that There was a problem hiding this comment. @cston, that is what I tried to do initially, but in the case of extension methods, the arguments object that is present at time of coercion is not the one that is passed in. The arguments seen during coercion have one extra parameter (for In stacktrace format:
The arguments are substituted with a different object when result = BindInvocationExpressionContinued(
syntax, expression, methodName, resolution.OverloadResolutionResult, resolution.AnalyzedArguments,
resolution.MethodGroup, null, diagnostics, resolution.ExtensionMethodsOfSameViabilityAreAvailable, queryClause);There was a problem hiding this comment. Thanks for explaining. Are there tests for There was a problem hiding this comment. Yes, They're using the There was a problem hiding this comment. We should at least assert that there are no null placeholders. I would call MissingDeconstruct if that happened. #Resolved There was a problem hiding this comment. Added, but I don't think I can his that case presently. |
||
|
|
||
| return result; | ||
| } | ||
| finally | ||
| { | ||
| analyzedArguments.Free(); | ||
| outVars.Free(); | ||
|
|
||
| if (bag != null) | ||
| { | ||
| bag.Free(); | ||
| } | ||
| } | ||
|
|
||
| } | ||
|
|
||
| private BoundExpression MissingDeconstruct(BoundExpression receiver, AssignmentExpressionSyntax syntax, int numParameters, DiagnosticBag diagnostics, out ImmutableArray<BoundDeconstructValuePlaceholder> outPlaceholders, BoundNode childNode) | ||
| { | ||
| Error(diagnostics, ErrorCode.ERR_MissingDeconstruct, receiver.Syntax, receiver.Type, numParameters); | ||
| outPlaceholders = default(ImmutableArray<BoundDeconstructValuePlaceholder>); | ||
|
|
||
| return BadExpression(syntax, childNode); | ||
| } | ||
|
|
||
| private BoundAssignmentOperator BindAssignment(AssignmentExpressionSyntax node, BoundExpression op1, BoundExpression op2, DiagnosticBag diagnostics) | ||
|
|
@@ -2196,6 +2262,10 @@ private BoundExpression CheckValue(BoundExpression expr, BindValueKind valueKind | |
| case BoundKind.PropertyGroup: | ||
| expr = BindIndexedPropertyAccess((BoundPropertyGroup)expr, mustHaveAllOptionalParameters: false, diagnostics: diagnostics); | ||
| break; | ||
|
|
||
| case BoundKind.OutDeconstructVarPendingInference: | ||
| Debug.Assert(valueKind == BindValueKind.RefOrOut); | ||
| return expr; | ||
| } | ||
|
|
||
| bool hasResolutionErrors = false; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll take a note for LDM on the dynamic question.