Invocations statically bound in presence of dynamic arguments should have dynamic result if their dynamic binding succeeded in C# 12 by AlekseyTs · Pull Request #72964 · dotnet/roslyn

AlekseyTs · 2024-04-09T23:00:21Z

Related to #72750.

This is a hybrid approach

Invocations statically bound in presence of dynamic arguments should have dynamic result if their dynamic binding succeeded in C# 12 #72885 (the first six commits in this PR)
Plus an approach from Statically bind invocations in presence of dynamic arguments only for local functions, extension methods or expanded non-array params cases. #72950, but applied conditionally for C# 12 and earlier.

…have `dynamic` result if their dynamic binding succeeded in C# 12 Fixes dotnet#72750. Corresponding spec update - dotnet/csharplang#8027

jaredpar · 2024-04-09T23:22:34Z

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

                case BoundKind.IndexerAccess:
-                    expr = BindIndexerDefaultArgumentsAndParamsCollection((BoundIndexerAccess)expr, valueKind, diagnostics);
+                    expr = BindIndexerDefaultArgumentsAndParamsCollection((BoundIndexerAccess)expr, valueKind, diagnostics
+#if DEBUG


Thought we were going to remove these. #Closed

@jaredpar

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.

jcouv · 2024-04-10T02:39:24Z

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

-        private BoundExpression CheckValue(BoundExpression expr, BindValueKind valueKind, BindingDiagnosticBag diagnostics)
+        private BoundExpression CheckValue(BoundExpression expr, BindValueKind valueKind, BindingDiagnosticBag diagnostics
+#if DEBUG
+            , bool dynamificationOfAssignmentResultIsHandled = false


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

jcouv · 2024-04-10T02:40:44Z

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

+            }
+        }
+
+        private static BoundExpression AdjustAssignmentTarget(BoundExpression left, out bool forceDynamicResult)


This method also deserves a comment as generous context #Closed

jcouv · 2024-04-10T02:42:45Z

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

-                leftPlaceholder, leftConversion, finalPlaceholder, finalConversion, resultKind, originalUserDefinedOperators, leftType, hasError);
+                leftPlaceholder, leftConversion, finalPlaceholder, finalConversion, resultKind, originalUserDefinedOperators, getResultType(left, forceDynamicResult), hasError);
+
+            TypeSymbol getResultType(BoundExpression left, bool forceDynamicResult)


Would it be possible to use a common helper (similar to AdjustAssignmentTarget) for adjusting the return type of assignments? (maybe AdjustAssignmentType?) #Closed

jcouv · 2024-04-10T02:44:31Z

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

+
+            if (forceDynamicResult)
+            {
+                result = result.Update(result.Left, result.Right, result.IsRef, Compilation.DynamicType);


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

jcouv · 2024-04-10T02:45:22Z

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

        {
            int count = variables.Count;
            var valuesBuilder = ArrayBuilder<BoundExpression>.GetInstance(count);
+            var resultTypesWithAnnotationsBuilder = ArrayBuilder<TypeWithAnnotations>.GetInstance(count);


~~Comment on this builder would be useful too~~ Never mind, I see you've already included one below #Closed

jcouv · 2024-04-10T02:47:46Z

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

+                               boundMember :
+                               CheckValue(boundMember, valueKind, diagnostics
+#if DEBUG
+                                          , dynamificationOfAssignmentResultIsHandled: boundMember is not BoundIndexerAccess


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

jcouv · 2024-04-10T03:14:12Z

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

+            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)))


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

jcouv · 2024-04-10T03:15:56Z

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

+            {
+                var tryDynamicInvocationDiagnostics = BindingDiagnosticBag.GetInstance(withDiagnostics: true, withDependencies: false);
+                BindDynamicInvocation(node, targetMethodGroupOrDelegateInstance, analyzedArguments, ImmutableArray.Create(method), tryDynamicInvocationDiagnostics, queryClause);
+                forceDynamicResultType = !tryDynamicInvocationDiagnostics.HasAnyResolvedErrors();


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

jcouv · 2024-04-10T03:18:25Z

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

                resultKind,
                originalUserDefinedOperators,
-                operandType,
+                forceDynamicResult ? Compilation.DynamicType : operandType,


forceDynamicResult ? Compilation.DynamicType : operandType,

Here's we'd use the helper method for return type, in response to use of AdjustAssignmentTarget above. Same comment applies at other locations where AdjustAssignmentTarget is used #Closed

jcouv · 2024-04-10T03:23:55Z

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


+            var adjustedNodeType = node.Type;
+
+            if (node.Type?.IsDynamic() == true && leftOperand is BoundIndexerAccess { Type.TypeKind: not TypeKind.Dynamic } indexerAccess)


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

jcouv · 2024-04-10T03:26:59Z

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

+            {
+                Debug.Assert(!node.Indexer.ReturnsByRef);
+                ForceDynamicResultType(node, node.Indexer.Type);
+            }


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

jcouv · 2024-04-10T03:28:52Z

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

            return null;
        }

+        private void ForceDynamicResultType(BoundExpression node, TypeSymbol sourceType)


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

jcouv · 2024-04-10T03:31:16Z

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

                TrackNullableStateForAssignment(right, leftLValueType, MakeSlot(left), rightState, MakeSlot(right));
            }

+            ForceDynamicTypeForAssignmentResultIfNecessary(node, left);


My suggestion to call SetResult or ForceDynamicResultType wouldn't work too easily here :-/ #Closed

jcouv · 2024-04-10T04:02:51Z

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

                        }
                    }
-                    else
+                    // For C# 12 and earlier statically bind invocations in presence of dynamic arguments only for expanded non-array params cases.


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

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.

jcouv

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)

jcouv · 2024-04-10T17:14:54Z

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

+            // 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 ||


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

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?

Yes, sorry for lack of clarity. With the known exception of local functions, he thought we should generally get a dynamic result back.

I am not sure what, if anything, is suggested here. Extension methods cannot be invoked dynamically. See if (resolution.IsExtensionMethodGroup) on line 833.

From offline discussion, making extension methods error is safer behavior for C# 12.

jcouv · 2024-04-10T17:23:00Z

        AssertEx.Equal("System.Int32 System.Int32.op_Addition(System.Int32 left, System.Int32 right)", symbolInfo.Symbol.ToTestDisplayString());

If we consider the lowered form of c[d] += 2 to be c[d] = c[d] + 2, it's not obvious that the addition should be statically-bound as opposed to dynamically-bound based on proposed spec.
If the result of the getter and the setter invocations are dynamified, then only the getter would be statically-bound, its result would be dynamified, then the rest of operations would be dynamic. #ByDesign

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/DynamicTests.cs:7297 in 376f6f9. [](commit_id = 376f6f9, deletion_comment = False)

jcouv · 2024-04-10T17:50:31Z

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

+            // 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 ||


methodGroup.IsExtensionMethodGroup

Shouldn't extension methods get dynamified? (their result converted to dynamic type) #ByDesign

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.

AlekseyTs · 2024-04-10T18:36:37Z

        AssertEx.Equal("System.Int32 System.Int32.op_Addition(System.Int32 left, System.Int32 right)", symbolInfo.Symbol.ToTestDisplayString());

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)

jcouv · 2024-04-10T18:55:27Z

        AssertEx.Equal("System.Int32 System.Int32.op_Addition(System.Int32 left, System.Int32 right)", symbolInfo.Symbol.ToTestDisplayString());

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)

jcouv

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.

AlekseyTs added 7 commits April 5, 2024 03:58

Invocations statically bound in presence of dynamic arguments should …

a5b55ad

…have `dynamic` result if their dynamic binding succeeded in C# 12 Fixes dotnet#72750. Corresponding spec update - dotnet/csharplang#8027

Merge remote-tracking branch 'dotnet/main' into Issue72750

262a733

Fixup

0961c6a

Beef up tests

62cba14

Fixup

c434258

fixup

9ce7dd6

Hybrid approach with more conservative logic for C# 12 and earlier.

50a5743

AlekseyTs added the Area-Compilers label Apr 9, 2024

AlekseyTs requested review from a team as code owners April 9, 2024 23:00

ghost added the untriaged Issues and PRs which have not yet been triaged by a lead label Apr 9, 2024

AlekseyTs requested review from RikkiGibson and jcouv April 9, 2024 23:00

jaredpar reviewed Apr 9, 2024

View reviewed changes

jcouv reviewed Apr 10, 2024

View reviewed changes

jcouv self-assigned this Apr 10, 2024

jcouv reviewed Apr 10, 2024

View reviewed changes

AlekseyTs added 2 commits April 9, 2024 21:07

Tests

a50570f

Get rid of the conditional compilation

6abf19c

RikkiGibson self-assigned this Apr 10, 2024

AlekseyTs added 2 commits April 10, 2024 09:37

PR feedback

a9d9414

Add a comment

376f6f9

jcouv reviewed Apr 10, 2024

View reviewed changes

RikkiGibson approved these changes Apr 10, 2024

View reviewed changes

Keep reporting an error for extension method in C# 12 and earlier

ae9b1cf

AlekseyTs requested a review from jcouv April 10, 2024 19:26

jcouv reviewed Apr 10, 2024

View reviewed changes

jcouv approved these changes Apr 10, 2024

View reviewed changes

AlekseyTs merged commit 5a49045 into dotnet:main Apr 11, 2024

dotnet-policy-service bot added this to the Next milestone Apr 11, 2024

This was referenced Apr 16, 2024

[Automated] PRs inserted in VS build feature.debugger.main-34815.163 #73037

Closed

[Automated] PRs inserted in VS build main-34815.147 #73038

Closed

AlekseyTs mentioned this pull request Apr 16, 2024

Port https://github.com/dotnet/roslyn/pull/72964 to release/dev17.10 #73058

Merged

AlekseyTs added a commit that referenced this pull request Apr 19, 2024

Revert "Port #72964 to release/dev17.10"

123d88b

dibarbet modified the milestones: Next, 17.11 P1 Apr 29, 2024

AlekseyTs mentioned this pull request May 3, 2024

Statically bind invocations in presence of dynamic arguments only for local functions #73314

Merged


		var adjustedNodeType = node.Type;

		if (node.Type?.IsDynamic() == true && leftOperand is BoundIndexerAccess { Type.TypeKind: not TypeKind.Dynamic } indexerAccess)

Conversation

AlekseyTs commented Apr 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jaredpar Apr 9, 2024 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Apr 10, 2024

Choose a reason for hiding this comment

Uh oh!

jcouv Apr 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Apr 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Apr 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Apr 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Apr 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Apr 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Apr 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Apr 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Apr 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Apr 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Apr 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Apr 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Apr 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Apr 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Apr 10, 2024

Choose a reason for hiding this comment

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

jcouv Apr 10, 2024 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RikkiGibson Apr 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

AlekseyTs commented Apr 9, 2024 •

edited

Loading

jaredpar Apr 9, 2024 •

edited by jcouv

Loading

jcouv Apr 10, 2024 •

edited

Loading

jcouv Apr 10, 2024 •

edited

Loading

jcouv Apr 10, 2024 •

edited

Loading

jcouv Apr 10, 2024 •

edited

Loading

jcouv Apr 10, 2024 •

edited

Loading

jcouv Apr 10, 2024 •

edited

Loading

jcouv Apr 10, 2024 •

edited

Loading

jcouv Apr 10, 2024 •

edited

Loading

jcouv Apr 10, 2024 •

edited

Loading

jcouv Apr 10, 2024 •

edited

Loading

jcouv Apr 10, 2024 •

edited

Loading

jcouv Apr 10, 2024 •

edited

Loading

jcouv Apr 10, 2024 •

edited

Loading

jcouv Apr 10, 2024 •

edited

Loading

jcouv Apr 10, 2024 •

edited by AlekseyTs

Loading

RikkiGibson Apr 10, 2024 •

edited

Loading

jcouv commented Apr 10, 2024 •

edited

Loading

jcouv Apr 10, 2024 •

edited

Loading