-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Lookup for Deconstruct method #11873
Changes from 2 commits
43cf0c5
c52e3d4
cc63d93
9b003a7
0d86cc0
b9a6233
f03fc0d
0e0bda3
bc3bd43
3496abb
079ccce
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -114,6 +114,7 @@ private static ImmutableArray<MethodSymbol> GetOriginalMethods(OverloadResolutio | |
|
||
var analyzedArguments = AnalyzedArguments.GetInstance(); | ||
analyzedArguments.Arguments.AddRange(args); | ||
Debug.Assert(!args.Any(e => e.Kind == BoundKind.OutDeconstructVarPendingInference)); | ||
BoundExpression result = BindInvocationExpression( | ||
node, node, methodName, boundExpression, analyzedArguments, diagnostics, queryClause, | ||
allowUnexpandedForm: allowUnexpandedForm); | ||
|
@@ -1100,7 +1101,8 @@ private ImmutableArray<BoundExpression> BuildArgumentsForErrorRecovery(AnalyzedA | |
{ | ||
BoundKind argumentKind = oldArguments[i].Kind; | ||
|
||
if (argumentKind == BoundKind.UnboundLambda && i < parameterCount) | ||
if (argumentKind == BoundKind.OutDeconstructVarPendingInference || | ||
(argumentKind == BoundKind.UnboundLambda && i < parameterCount)) | ||
{ | ||
ArrayBuilder<BoundExpression> newArguments = ArrayBuilder<BoundExpression>.GetInstance(argumentCount); | ||
newArguments.AddRange(oldArguments); | ||
|
@@ -1120,8 +1122,15 @@ private ImmutableArray<BoundExpression> BuildArgumentsForErrorRecovery(AnalyzedA | |
newArguments[i] = ((UnboundLambda)oldArgument).Bind(parameterType); | ||
} | ||
break; | ||
case BoundKind.OutDeconstructVarPendingInference: | ||
newArguments[i] = ((OutDeconstructVarPendingInference)oldArgument).SetInferredType(parameters[i].Type, success: true); | ||
break; | ||
} | ||
} | ||
else if (oldArgument.Kind == BoundKind.OutDeconstructVarPendingInference) | ||
{ | ||
newArguments[i] = ((OutDeconstructVarPendingInference)oldArgument).FailInference(this); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We probably need to have similar code in BuildArgumentsForDynamicInvocation, or at least an assert that we cannot run into OutDeconstructVarPendingInference there. #Resolved There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added assertion. |
||
} | ||
|
||
i++; | ||
} | ||
|
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; | ||
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> | ||
static private BoundDeconstructionDeconstructStep MakeNonTupleDeconstructStep( | ||
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 | ||
MethodSymbol deconstructMethod = FindDeconstruct(variables.Length, targetPlaceholder, syntax, diagnostics); | ||
ImmutableArray<BoundDeconstructValuePlaceholder> outPlaceholders; | ||
var deconstructInvocation = MakeDeconstructInvocationExpression(variables.Length, targetPlaceholder, syntax, diagnostics, out outPlaceholders); | ||
|
||
if ((object)deconstructMethod == null) | ||
if (deconstructInvocation.HasAnyErrors) | ||
{ | ||
return null; | ||
} | ||
|
||
return new BoundDeconstructionDeconstructStep(syntax, deconstructMethod, targetPlaceholder, deconstructMethod.Parameters.SelectAsArray((p, s) => new BoundDeconstructValuePlaceholder(s, p.Type), syntax)); | ||
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 = MakeAssignmentInfo(variable.Single, valuePlaceholder.Type, valuePlaceholder, syntax, diagnostics); | ||
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 MakeAssignmentInfo(BoundExpression receivingVariable, TypeSymbol sourceType, BoundDeconstructValuePlaceholder inputPlaceholder, AssignmentExpressionSyntax node, DiagnosticBag diagnostics) | ||
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,69 @@ static private void FlattenDeconstructVariables(ImmutableArray<DeconstructionVar | |
} | ||
|
||
/// <summary> | ||
/// Find the Deconstruct method for the expression on the right, that will fit the assignable bound expressions on the left. | ||
/// Returns true if the Deconstruct method is found. | ||
/// If so, it outputs the method. | ||
/// 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 static MethodSymbol FindDeconstruct(int numCheckedVariables, BoundExpression boundRHS, SyntaxNode node, DiagnosticBag diagnostics) | ||
private BoundExpression MakeDeconstructInvocationExpression( | ||
int numCheckedVariables, BoundExpression receiver, AssignmentExpressionSyntax assignmentSyntax, | ||
DiagnosticBag diagnostics, out ImmutableArray<BoundDeconstructValuePlaceholder> outPlaceholders) | ||
{ | ||
// find symbol for Deconstruct member | ||
ImmutableArray<Symbol> candidates = boundRHS.Type.GetMembers("Deconstruct"); | ||
switch (candidates.Length) | ||
var receiverSyntax = receiver.Syntax; | ||
|
||
if (receiver.Type.IsDynamic()) | ||
{ | ||
case 0: | ||
Error(diagnostics, ErrorCode.ERR_MissingDeconstruct, boundRHS.Syntax, boundRHS.Type); | ||
return null; | ||
case 1: | ||
break; | ||
default: | ||
Error(diagnostics, ErrorCode.ERR_AmbiguousDeconstruct, boundRHS.Syntax, boundRHS.Type); | ||
return null; | ||
Error(diagnostics, ErrorCode.ERR_CannotDeconstructDynamic, receiverSyntax); | ||
outPlaceholders = default(ImmutableArray<BoundDeconstructValuePlaceholder>); | ||
|
||
return BadExpression(receiverSyntax, receiver); | ||
} | ||
|
||
Symbol deconstructMember = candidates[0]; | ||
var analyzedArguments = AnalyzedArguments.GetInstance(); | ||
var outVars = ArrayBuilder<OutDeconstructVarPendingInference>.GetInstance(numCheckedVariables); | ||
|
||
// check that the deconstruct fits | ||
if (deconstructMember.Kind != SymbolKind.Method) | ||
try | ||
{ | ||
Error(diagnostics, ErrorCode.ERR_MissingDeconstruct, boundRHS.Syntax, boundRHS.Type); | ||
return null; | ||
} | ||
for (int i = 0; i < numCheckedVariables; i++) | ||
{ | ||
var variable = new OutDeconstructVarPendingInference(assignmentSyntax); | ||
analyzedArguments.Arguments.Add(variable); | ||
analyzedArguments.RefKinds.Add(RefKind.Out); | ||
outVars.Add(variable); | ||
} | ||
|
||
MethodSymbol deconstructMethod = (MethodSymbol)deconstructMember; | ||
if (deconstructMethod.MethodKind != MethodKind.Ordinary) | ||
{ | ||
Error(diagnostics, ErrorCode.ERR_MissingDeconstruct, boundRHS.Syntax, boundRHS.Type); | ||
return null; | ||
} | ||
string methodName = "Deconstruct"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. const? #Resolved There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure. Added. |
||
var boundExpression = BindInstanceMemberAccess( | ||
receiverSyntax, receiverSyntax, receiver, methodName, rightArity: 0, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we have a test when Deconstruct method is a generic extension method? #Resolved There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not yet. Thanks for the suggestion. I'll add. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, the tests for deconstructing |
||
typeArgumentsSyntax: default(SeparatedSyntaxList<TypeSyntax>), typeArguments: default(ImmutableArray<TypeSymbol>), | ||
invoked: true, diagnostics: diagnostics); | ||
|
||
if (deconstructMethod.ParameterCount != numCheckedVariables) | ||
{ | ||
Error(diagnostics, ErrorCode.ERR_DeconstructWrongParams, boundRHS.Syntax, deconstructMethod, numCheckedVariables); | ||
return null; | ||
} | ||
boundExpression = CheckValue(boundExpression, BindValueKind.RValueOrMethodGroup, diagnostics); | ||
boundExpression.WasCompilerGenerated = true; | ||
|
||
BoundExpression result = BindInvocationExpression( | ||
receiverSyntax, receiverSyntax, methodName, boundExpression, analyzedArguments, diagnostics, queryClause: null, | ||
allowUnexpandedForm: true); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will this bind successfully to a field: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added two tests: one with an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added a check that the member we're trying to use is a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we're checking for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done (calling |
||
|
||
if (deconstructMethod.Parameters.Any(p => p.RefKind != RefKind.Out)) | ||
result.WasCompilerGenerated = true; | ||
|
||
if (result.HasAnyErrors) | ||
{ | ||
outPlaceholders = default(ImmutableArray<BoundDeconstructValuePlaceholder>); | ||
return BadExpression(assignmentSyntax, result); | ||
} | ||
|
||
outPlaceholders = outVars.SelectAsArray(v => v.Placeholder); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for explaining. Are there tests for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, They're using the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added, but I don't think I can his that case presently. |
||
|
||
return result; | ||
} | ||
finally | ||
{ | ||
Error(diagnostics, ErrorCode.ERR_DeconstructRequiresOutParams, boundRHS.Syntax, deconstructMethod); | ||
return null; | ||
analyzedArguments.Free(); | ||
outVars.Free(); | ||
} | ||
|
||
return deconstructMethod; | ||
} | ||
|
||
private BoundAssignmentOperator BindAssignment(AssignmentExpressionSyntax node, BoundExpression op1, BoundExpression op2, DiagnosticBag diagnostics) | ||
|
@@ -2196,6 +2217,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; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a test for this case (where the out var has no type)? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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: |
||
{ | ||
// If the problem is that a lambda isn't convertible to the given type, also report why. | ||
// The argument and parameter type might match, but may not have same in/out modifiers | ||
|
@@ -971,6 +971,8 @@ private static bool HadLambdaConversionError(DiagnosticBag diagnostics, Analyzed | |
} | ||
else | ||
{ | ||
Debug.Assert(argument.Kind != BoundKind.OutDeconstructVarPendingInference); | ||
|
||
TypeSymbol argType = argument.Display as TypeSymbol; | ||
Debug.Assert((object)argType != null); | ||
|
||
|
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.