-
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
Invocations statically bound in presence of dynamic arguments should have dynamic result if their dynamic binding succeeded in C# 12 #72964
Conversation
…have `dynamic` result if their dynamic binding succeeded in C# 12 Fixes dotnet#72750. Corresponding spec update - dotnet/csharplang#8027
@@ -430,7 +449,11 @@ private BoundExpression CheckValue(BoundExpression expr, BindValueKind valueKind | |||
return expr; | |||
|
|||
case BoundKind.IndexerAccess: | |||
expr = BindIndexerDefaultArgumentsAndParamsCollection((BoundIndexerAccess)expr, valueKind, diagnostics); | |||
expr = BindIndexerDefaultArgumentsAndParamsCollection((BoundIndexerAccess)expr, valueKind, diagnostics | |||
#if DEBUG |
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.
Thought we were going to remove these. #Closed
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.
Thought we were going to remove these.
We still are. That change however doesn't affect the behavior and I wanted to get something out for review sooner.
private BoundExpression CheckValue(BoundExpression expr, BindValueKind valueKind, BindingDiagnosticBag diagnostics) | ||
private BoundExpression CheckValue(BoundExpression expr, BindValueKind valueKind, BindingDiagnosticBag diagnostics | ||
#if DEBUG | ||
, bool dynamificationOfAssignmentResultIsHandled = 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.
Let's add a comment for this parameter. The purpose is to catch any callers that do not "handle dynamification of assignment". Let's clarify what that means to handle.
From my understanding:
- some indexer accesses with dynamic argument get resolved statically, and they get marked with type
dynamic
- but assignment scenarios should adjust their indexer/operand back to their original type (non-dynamic) and instead use type
dynamic
as the result of the assignment itself - this flag and the assertion below help catch any new assignment scenarios and make them aware of this subtlety #Closed
} | ||
} | ||
|
||
private static BoundExpression AdjustAssignmentTarget(BoundExpression left, out bool forceDynamicResult) |
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.
This method also deserves a comment as generous context #Closed
leftPlaceholder, leftConversion, finalPlaceholder, finalConversion, resultKind, originalUserDefinedOperators, leftType, hasError); | ||
leftPlaceholder, leftConversion, finalPlaceholder, finalConversion, resultKind, originalUserDefinedOperators, getResultType(left, forceDynamicResult), hasError); | ||
|
||
TypeSymbol getResultType(BoundExpression left, bool forceDynamicResult) |
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.
Would it be possible to use a common helper (similar to AdjustAssignmentTarget
) for adjusting the return type of assignments? (maybe AdjustAssignmentType
?) #Closed
|
||
if (forceDynamicResult) | ||
{ | ||
result = result.Update(result.Left, result.Right, result.IsRef, Compilation.DynamicType); |
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.
If we add a common helper for AdjustAssignmentType
, we could do this update unconditionally (it should be lazy if nothing changed). Something like:
result = result.Update(..., AdjustAssignmentType(..., forceDynamicResult));
#Closed
BindingDiagnosticBag diagnostics, bool ignoreDiagnosticsFromTuple) | ||
{ | ||
int count = variables.Count; | ||
var valuesBuilder = ArrayBuilder<BoundExpression>.GetInstance(count); | ||
var resultTypesWithAnnotationsBuilder = ArrayBuilder<TypeWithAnnotations>.GetInstance(count); |
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.
Comment on this builder would be useful too Never mind, I see you've already included one below #Closed
boundMember : | ||
CheckValue(boundMember, valueKind, diagnostics | ||
#if DEBUG | ||
, dynamificationOfAssignmentResultIsHandled: boundMember is not BoundIndexerAccess |
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.
For the various dynamificationOfAssignmentResultIsHandled: boundMember is not BoundIndexerAccess
instances, I think a direct assertion Debug.Assert(boundMember is not BoundIndexerAccess)
or a comment would be helpful to clarify the intent. #Closed
if (hasDynamicArgument && !methodGroup.IsExtensionMethodGroup && method.MethodKind != MethodKind.LocalFunction && | ||
!method.ReturnsVoid && !method.ReturnsByRef && !returnType.IsDynamic() && | ||
Conversions.ClassifyConversionFromExpressionType(returnType, Compilation.DynamicType, isChecked: false, ref useSiteInfo).IsImplicit && | ||
!HasApplicableMemberWithPossiblyExpandedNonArrayParamsCollection(analyzedArguments.Arguments, ImmutableArray.Create(methodResult))) |
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.
nit: inverting most of the conditions makes in more readable in my opinion:
if (hasDynamicArgument && !(methodGroup.IsExtensionMethodGroup || method.MethodKind == MethodKind.LocalFunction ||
method.ReturnsVoid || method.ReturnsByRef || returnType.IsDynamic() ||
!Conversions.ClassifyConversionFromExpressionType(returnType, Compilation.DynamicType, isChecked: false, ref useSiteInfo).IsImplicit ||
HasApplicableMemberWithPossiblyExpandedNonArrayParamsCollection(analyzedArguments.Arguments, ImmutableArray.Create(methodResult))))
``` #Closed
{ | ||
var tryDynamicInvocationDiagnostics = BindingDiagnosticBag.GetInstance(withDiagnostics: true, withDependencies: false); | ||
BindDynamicInvocation(node, targetMethodGroupOrDelegateInstance, analyzedArguments, ImmutableArray.Create(method), tryDynamicInvocationDiagnostics, queryClause); | ||
forceDynamicResultType = !tryDynamicInvocationDiagnostics.HasAnyResolvedErrors(); |
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.
Consider adding a comment to summarize intent: for scenarios that bound dynamically prior to C# 13, we force a dynamic result type despite compile-time resolution of the call for back compat. #Closed
@@ -2369,7 +2419,7 @@ private BoundExpression BindIncrementOperator(CSharpSyntaxNode node, ExpressionS | |||
resultConversion, | |||
resultKind, | |||
originalUserDefinedOperators, | |||
operandType, | |||
forceDynamicResult ? Compilation.DynamicType : operandType, |
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.
@@ -5440,20 +5441,37 @@ private static BoundExpression SkipReferenceConversions(BoundExpression possibly | |||
LearnFromNonNullTest(leftOperand, ref leftState); | |||
LearnFromNullTest(leftOperand, ref this.State); | |||
|
|||
var adjustedNodeType = node.Type; | |||
|
|||
if (node.Type?.IsDynamic() == true && leftOperand is BoundIndexerAccess { Type.TypeKind: not TypeKind.Dynamic } indexerAccess) |
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.
node.Type?.IsDynamic() == true && leftOperand is BoundIndexerAccess { Type.TypeKind: not TypeKind.Dynamic } indexerAccess
Consider extracting the various checks that follow this recurring pattern into one helper. That would help clarify the intent (detect a statically-bound indexer access with dynamic arguments in an assignment node) and offers a place to leave a comment and tie the various places that do similar checks together.
I'd even include related checks like originalIndexerAccessOrObjectInitializerMember.Type.IsDynamic() == true && !indexer.Type.IsDynamic()
, as those checks share the same purpose (detecting if the types of the assignment nodes were adjusted). #Closed
{ | ||
Debug.Assert(!node.Indexer.ReturnsByRef); | ||
ForceDynamicResultType(node, node.Indexer.Type); | ||
} |
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.
Consider doing ForceDynamicResultType
OR SetResult
, but not both. Applies here and elsewhere in nullable walker.
if (node.Type.IsDynamic() && !node.Indexer.Type.IsDynamic())
{
Debug.Assert(!node.Indexer.ReturnsByRef);
ForceDynamicResultType(node, node.Indexer.Type);
}
else
{
SetResult(node, resultType, indexer.TypeWithAnnotations);
}
Then ForceDynamicResultType
can be renamed to SetDynamicResult
. #Closed
return null; | ||
} | ||
|
||
private void ForceDynamicResultType(BoundExpression node, TypeSymbol sourceType) |
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.
Although the method is simple (it's easy to see what it does), a comment with context will be helpful for future readers that wonder why scenarios need to do this. #Closed
@@ -9785,9 +9838,22 @@ private void VisitThisOrBaseReference(BoundExpression node) | |||
TrackNullableStateForAssignment(right, leftLValueType, MakeSlot(left), rightState, MakeSlot(right)); | |||
} | |||
|
|||
ForceDynamicTypeForAssignmentResultIfNecessary(node, left); |
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.
My suggestion to call SetResult
or ForceDynamicResultType
wouldn't work too easily here :-/ #Closed
@@ -9719,13 +9756,16 @@ private BoundExpression BindIndexedPropertyAccess(SyntaxNode syntax, BoundExpres | |||
argumentSyntax, singleCandidate); | |||
} | |||
} | |||
else | |||
// For C# 12 and earlier statically bind invocations in presence of dynamic arguments only for expanded non-array params cases. |
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'm confused, did we support non-array params cases in C# 12 or earlier? Also applies to other places that check language version #Closed
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.
did we support non-array params cases in C# 12 or earlier? Also applies to other places that check language version
We didn't. In general we prefer to not change semantic analysis based on language version. And I prefer to keep it this way specifically for params collections. Using them in C# 12 is an error, but we still force C# 13 binding.
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.
Thanks much for the walk through today. There's many subtleties... I only have minor comments (mostly documentation, trying to make the code easier to follow by next reader). Tests not looked at yet (iteration 7)
// For C# 12 and earlier statically bind invocations in presence of dynamic arguments only for local functions, extension methods or expanded non-array params cases. | ||
if (Compilation.LanguageVersion > LanguageVersion.CSharp12 || | ||
singleCandidate.MethodKind == MethodKind.LocalFunction || | ||
resolution.IsExtensionMethodGroup || |
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.
Had a brief chat with Mads. He said LDM cares about the QB fix to the extent that it affects C# 12 and older behavior.
Aside from the exception of local functions, he thought we should actually dynamify more. So it's not clear that just statically binding extension methods is the desired behavior for C# 12 (or 13). #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 wasn't sure what was meant by dynamify in this context. Is it referring to the step of converting a statically resolved call value to dynamic?
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.
Yes, sorry for lack of clarity. With the known exception of local functions, he thought we should generally get a dynamic
result back.
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 what, if anything, is suggested here. Extension methods cannot be invoked dynamically. See if (resolution.IsExtensionMethodGroup)
on line 833.
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.
From offline discussion, making extension methods error is safer behavior for C# 12.
If we consider the lowered form of Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/DynamicTests.cs:7297 in 376f6f9. [](commit_id = 376f6f9, deletion_comment = False) |
// should have dynamic result if their dynamic binding succeeded in C# 12 and there are no | ||
// obvious reasons for the runtime binder to fail (ref return, for example). | ||
if (hasDynamicArgument && | ||
!(methodGroup.IsExtensionMethodGroup || method.MethodKind == MethodKind.LocalFunction || |
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.
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.
From offline discussion, this falls out of proposed spec. We'll leave question of whether the result should be converted to dynamic in C# 13 and forward to LDM discussion.
We discussed this offline during the walkthrough. In case of the assignment the Invocation refers to the setter. Obviously, dynamically invoking the getter will be unexpected in for the compound assignment. In reply to: 2048096364 Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/DynamicTests.cs:7297 in 376f6f9. [](commit_id = 376f6f9, deletion_comment = False) |
From offline discussion, a mention of this will be added to spec and we can leave PR as-is. Thanks In reply to: 2048207874 Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/DynamicTests.cs:7297 in 376f6f9. [](commit_id = 376f6f9, deletion_comment = 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.
LGTM Thanks (iteration 12)
…c` arguments (dotnet#72964) Fixes dotnet#72750. For C# 12 language version: behavior of the compiler in regards to deciding between whether binding should be static or dynamic is the same as behavior of C# 12 compiler. As a result, for affected scenarios, what used to have `dynamic` type in C# 12 compiler will have `dynamic` type when C# 12 language version is targeted. For newer language versions: invocations statically bound in presence of dynamic arguments should have dynamic result if their dynamic binding succeeded in C# 12. Corresponding spec update - dotnet/csharplang#8027 at commit 8. Related to dotnet#33011, dotnet#72906, dotnet#72912, dotnet#72913, dotnet#72914, dotnet#72916, dotnet#72931.
Restore dynamic as result type of some operations involving dynamic arguments (#72964) Fixes #72750. For C# 12 language version: behavior of the compiler in regards to deciding between whether binding should be static or dynamic is the same as behavior of C# 12 compiler. As a result, for affected scenarios, what used to have dynamic type in C# 12 compiler will have dynamic type when C# 12 language version is targeted. For newer language versions: invocations statically bound in presence of dynamic arguments should have dynamic result if their dynamic binding succeeded in C# 12. Corresponding spec update - dotnet/csharplang#8027 at commit 8. Related to #33011, #72906, #72912, #72913, #72914, #72916, #72931.
… `dynamic` arguments (dotnet#72964)" This reverts compiler changes (tests changes are not reverted) made in commit 5a49045.
… local functions (#73314) Fixes #72750. This implements the latest LDM decision. In order to make sure all artifacts of the previous fix (#72964) were removed, I reverted all implementation (but not test changes) from that PR by using 'git revert`. All cleanups/refactorings that are still relevant were manually ported back.
Related to #72750.
This is a hybrid approach
dynamic
result if their dynamic binding succeeded in C# 12 #72885 (the first six commits in this PR)