Emit defensive copy for constrained call on `in` parameter #66189

jcouv · 2023-01-02T18:42:30Z

jjonescz · 2023-01-05T10:03:52Z

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

+    IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();
+}";
+            var verifier = CompileAndVerify(source, expectedOutput: "00");
+            // Note: we use a temp instead of directly doing a constrained call on `in` parameter


Do we use a temp even when the target method is marked as readonly? #Closed

It's not possible to have exactly this scenario with the only modification being an additional readonly because this scenario uses an interface and readonly is only allowed in struct types (not in interfaces).
Prior to the code change, we'd produce a direct constrained call, so the comment calls out what is important to observe in the IL.

~~That said, it should be possible to emit a pattern-based GetEnumerator call directly on the struct (ie. foreach without IEnumerable interface). I'll add a test for that.~~ On second thought, that scenario isn't so interesting, as it will not cover the modified test (it'll go down a different branch of the same method, see line 1600).

FYI, amended my earlier comment

jcouv · 2023-01-05T15:52:09Z

@AlekseyTs @dotnet/roslyn-compiler Please take a look. Thanks

AlekseyTs · 2023-01-05T16:06:47Z

src/Compilers/CSharp/Portable/CodeGen/EmitExpression.cs

@@ -1626,15 +1626,15 @@ private void EmitInstanceCallExpression(BoundCall call, UseKind useKind)
                }
                else
                {
-                    // calling a method defined in a base class.
+                    // calling a method defined in a base class or interface.

                    // When calling a method that is virtual in metadata on a struct receiver, 
                    // we use a constrained virtual call. If possible, it will skip boxing.
                    if (method.IsMetadataVirtual())
                    {
                        // NB: all methods that a struct could inherit from bases are non-mutating


all methods that a struct could inherit from bases are non-mutating

This assumption looks suspicious. For example, ToString when overridden in a struct can mutate the struct. #Closed

I have a test for that (InvokeStructToStringOverrideOnInParameter) and we have existing coverage as well (CodeGenReadOnlyStructTests.ReadOnlyMethod_CallBaseMethod for non-override scenario, and other related ones). In that case we fall into the condition above (at line 1600), not here.
I'd be okay always using AddressKind.Writeable here. It's definitely safe, but a bit less optimal for ToString scenarios. Let me know what you think.

AlekseyTs · 2023-01-05T16:08:29Z

src/Compilers/CSharp/Portable/CodeGen/EmitExpression.cs


                    // When calling a method that is virtual in metadata on a struct receiver, 
                    // we use a constrained virtual call. If possible, it will skip boxing.
                    if (method.IsMetadataVirtual())
                    {
                        // NB: all methods that a struct could inherit from bases are non-mutating
                        //     treat receiver as ReadOnly
-                        tempOpt = EmitReceiverRef(receiver, AddressKind.ReadOnly);
+                        tempOpt = EmitReceiverRef(receiver, methodContainingType.IsInterface ? AddressKind.Writeable : AddressKind.ReadOnly);


methodContainingType.IsInterface

Why only in this case? See a comment above #Closed

Thanks much for suggesting the scenario where an override is added to the struct.
That showed that the assumption was broken. Removed it.

AlekseyTs · 2023-01-05T16:11:40Z

src/Compilers/CSharp/Portable/CodeGen/EmitExpression.cs


                    // When calling a method that is virtual in metadata on a struct receiver, 
                    // we use a constrained virtual call. If possible, it will skip boxing.
                    if (method.IsMetadataVirtual())
                    {
                        // NB: all methods that a struct could inherit from bases are non-mutating
                        //     treat receiver as ReadOnly
-                        tempOpt = EmitReceiverRef(receiver, AddressKind.ReadOnly);
+                        tempOpt = EmitReceiverRef(receiver, methodContainingType.IsInterface ? AddressKind.Writeable : AddressKind.ReadOnly);


tempOpt = EmitReceiverRef(receiver, methodContainingType.IsInterface ? AddressKind.Writeable : AddressKind.ReadOnly);

Does this change also affect readonly fields? #Closed

~~Readonly fields are not affected by this change. Here we're only dealing with calls to a method.~~
We clarified offline. The question is about using a readonly field as the receiver. If C# is affected, maybe VB is too.

Did some investigation. It is indeed possible to hit this with a readonly field as well.

On the VB side, it's a bit confusing because the AddressKind enum values have different meaning than the C# AddressKinds of the same name, but as far as I could tell we didn't have the same problem. Added tests.

It's worth noting that in VB, the For Each generates a conversion (boxing) instead of a constrained call, so I couldn't get exactly the same scenario that was broken in C#.

jcouv · 2023-01-09T19:18:56Z

@AlekseyTs This is ready for another look. Thanks

jcouv · 2023-01-11T01:31:20Z

@AlekseyTs This is ready for another look. Thanks

AlekseyTs · 2023-01-11T17:11:07Z

This is ready for another look.

Looking

AlekseyTs · 2023-01-11T17:27:34Z

        System.Console.Write(ToString());

This scenario is actually interesting. Perhaps we could optimize cases like this as long as consumer and the consumed type are in the same module.

Refers to: src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenReadonlyStructTests.cs:507 in c711d2b. [](commit_id = c711d2b, deletion_comment = False)

AlekseyTs · 2023-01-11T18:03:59Z

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

+{
+    public static string M(in S s)
+    {
+        return s.ToString();


return s.ToString();

It would be good to test this scenario on a read-only field and confirm that mutations are preserved when the method is invoked in an instance constructor and not preserved in a regular method. #Closed

AlekseyTs · 2023-01-11T18:06:10Z

Done with review pass (commit 2)

AlekseyTs

LGTM (commit 4)

jcouv · 2023-01-12T16:53:38Z

/azp run

azure-pipelines · 2023-01-12T16:53:57Z

Azure Pipelines successfully started running 2 pipeline(s).

hamarb123 · 2023-01-17T21:43:34Z

This change seems to cause defensive copies to be emitted when they don't need to:

https://sharplab.io/#v2:EYLgtghglgdgNAFxFANgHwAICYCMBYAKAwAYACDHAOgGEB7FFAUwGMEpaYBnAbkJPJwAWXkTIVKAGVgBHEXwDM5LKWqEA3oVJalWTdo0FtR0gGVSANwgoAro1IBeUjEYB3ABQBKEce0AnRgBmpP4QACYcKACephZWtgBKgQ7BSZY2jN4+AgCcbmkJgV56PgCybrCx6YkBRYZZFLn5jNW1RgC+xeSKFABs5IKkZRVmTR6dBlmkAPRTAJKcnLakUJykCAAWdht2zLRgAA6ojL6k4YycMADkCKScjIxga7QpuwDmMCuMnUYzFPIAPLAEAA+SgAcUYCAAojBrGBjhAELQTitSJBfABrRihUgQVYhcIwKKUb7aALIxgQZjrUh5CAnAAeyxglVsHlIalIHTqWm53IUtwQvmsrBiIAEAKBwPUnSBuMyWgwigJEWitHMx18UFCdgoZAAKrQTELYK9PA5gbjKIbjVqYGbWtp9lrLAg7CqidFZjC4Qikb5ATAQaQIdDYfDfIjkeb7JafRGIMAmJR4hB7Yw3MQ4LiPODIfG/dHHVoPVFSN7w4WA1KJYGQXmw77I/6Y5bQwXm0WFSkwqryx2oyc/g2By32bGQ/nK53fJ4RNygA==

The IL shows that a defensive copy is being emitted when it is not necessary. It is not necessary because the implementation that foreach uses (S.IEnumerable<int>.GetEnumerator()) is marked as readonly. If there was a public member that was readonly called GetEnumerator, this should also not cause a defensive copy, but if it was not readonly, then it should. I'd think that the correct check would be to determine what method the foreach will be using, lookup the interface implementation based on the type if it's something like IEnumerable<int>.GetEnumerator(), and then check if it's marked as readonly or an extension method (both of which wouldn't need any defensive copies (the reason for extension methods is that it would be illegal to call a mutating extension method on a readonly value currently)).

jcouv · 2023-01-17T22:28:07Z

@hamarb123 Interface members cannot be marked readonly. The fact that the method that implements the interface is marked readonly would not come into play.

hamarb123 · 2023-01-17T22:52:48Z

The fact that the method that implements the interface is marked readonly would not come into play.

Why can they be marked readonly then? Was it an accident to allow it, because I don't see any other reason to allow it (hence why I'm asking)?
And also, why not take advantage of it, since removing the readonly would obviously be a breaking change?

jcouv · 2023-01-17T23:46:50Z

Why can they be marked readonly then?

I think by "they" you mean private interface implementations, such as readonly IEnumerator<int> IEnumerable<int>.GetEnumerator() => GetEnumerator();.
It's correct that the readonly here doesn't help the caller of this API, since the caller will only see the signature of the interface member (ie. without readonly).
As for why that is allowed, the readonly here still provides some enforcement on the method body, so that could be useful.

Tagging @RikkiGibson (dev on readonly struct members) in case anything to add.

RikkiGibson · 2023-01-18T06:47:01Z

I don't recall specifically. #32911 does indicate it was intentional though 😊

explicit interface implementations (should allow)

jcouv self-assigned this Jan 2, 2023

dotnet-issue-labeler bot added the Area-Compilers label Jan 2, 2023

jcouv force-pushed the constrained-in branch from 59196ec to 0085a72 Compare January 3, 2023 05:28

Emit defensive copy for constrained call on in parameter

86c4d13

jcouv force-pushed the constrained-in branch from 0085a72 to 86c4d13 Compare January 3, 2023 19:51

jcouv marked this pull request as ready for review January 4, 2023 18:08

jcouv requested a review from a team as a code owner January 4, 2023 18:08

jcouv requested a review from AlekseyTs January 4, 2023 18:08

jcouv added this to the 17.6 milestone Jan 5, 2023

jjonescz reviewed Jan 5, 2023

View reviewed changes

AlekseyTs reviewed Jan 5, 2023

View reviewed changes

jcouv marked this pull request as draft January 5, 2023 19:56

Address feedback

c711d2b

jcouv marked this pull request as ready for review January 6, 2023 07:42

jcouv requested a review from AlekseyTs January 6, 2023 07:42

AlekseyTs reviewed Jan 11, 2023

View reviewed changes

Add test

fa16a33

jcouv mentioned this pull request Jan 11, 2023

Consider optimizing constrained calls to base methods on readonly struct in some cases #66365

Open

Add tracked issue

73767e8

jcouv requested a review from AlekseyTs January 11, 2023 22:50

AlekseyTs approved these changes Jan 12, 2023

View reviewed changes

jcouv requested a review from jjonescz January 12, 2023 10:18

jjonescz approved these changes Jan 12, 2023

View reviewed changes

jcouv merged commit b06c252 into dotnet:main Jan 13, 2023

jcouv deleted the constrained-in branch January 13, 2023 23:03

ghost modified the milestones: 17.6, Next Jan 13, 2023

allisonchou mentioned this pull request Jan 18, 2023

[Automated] PRs inserted in VS build main-33318.109 #66470

Closed

Cosifne modified the milestones: Next, 17.6 P1 Jan 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Emit defensive copy for constrained call on `in` parameter #66189

Emit defensive copy for constrained call on `in` parameter #66189

jcouv commented Jan 2, 2023

jjonescz Jan 5, 2023 •

edited

Loading

jcouv Jan 5, 2023 •

edited

Loading

jcouv Jan 5, 2023

jcouv commented Jan 5, 2023

AlekseyTs Jan 5, 2023 •

edited

Loading

jcouv Jan 5, 2023 •

edited

Loading

AlekseyTs Jan 5, 2023 •

edited

Loading

jcouv Jan 6, 2023

AlekseyTs Jan 5, 2023 •

edited

Loading

jcouv Jan 5, 2023 •

edited

Loading

jcouv Jan 6, 2023 •

edited

Loading

jcouv commented Jan 9, 2023

jcouv commented Jan 11, 2023

AlekseyTs commented Jan 11, 2023

AlekseyTs commented Jan 11, 2023 •

edited

Loading

AlekseyTs Jan 11, 2023 •

edited

Loading

AlekseyTs commented Jan 11, 2023

AlekseyTs left a comment

jcouv commented Jan 12, 2023

azure-pipelines bot commented Jan 12, 2023

hamarb123 commented Jan 17, 2023 •

edited

Loading

jcouv commented Jan 17, 2023

hamarb123 commented Jan 17, 2023 •

edited

Loading

jcouv commented Jan 17, 2023

RikkiGibson commented Jan 18, 2023

Emit defensive copy for constrained call on in parameter #66189

Emit defensive copy for constrained call on in parameter #66189

Conversation

jcouv commented Jan 2, 2023

jjonescz Jan 5, 2023 • edited Loading

Choose a reason for hiding this comment

jcouv Jan 5, 2023 • edited Loading

Choose a reason for hiding this comment

jcouv Jan 5, 2023

Choose a reason for hiding this comment

jcouv commented Jan 5, 2023

AlekseyTs Jan 5, 2023 • edited Loading

Choose a reason for hiding this comment

jcouv Jan 5, 2023 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jan 5, 2023 • edited Loading

Choose a reason for hiding this comment

jcouv Jan 6, 2023

Choose a reason for hiding this comment

AlekseyTs Jan 5, 2023 • edited Loading

Choose a reason for hiding this comment

jcouv Jan 5, 2023 • edited Loading

Choose a reason for hiding this comment

jcouv Jan 6, 2023 • edited Loading

Choose a reason for hiding this comment

jcouv commented Jan 9, 2023

jcouv commented Jan 11, 2023

AlekseyTs commented Jan 11, 2023

AlekseyTs commented Jan 11, 2023 • edited Loading

AlekseyTs Jan 11, 2023 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Jan 11, 2023

AlekseyTs left a comment

Choose a reason for hiding this comment

jcouv commented Jan 12, 2023

azure-pipelines bot commented Jan 12, 2023

hamarb123 commented Jan 17, 2023 • edited Loading

jcouv commented Jan 17, 2023

hamarb123 commented Jan 17, 2023 • edited Loading

jcouv commented Jan 17, 2023

RikkiGibson commented Jan 18, 2023

Emit defensive copy for constrained call on `in` parameter #66189

Emit defensive copy for constrained call on `in` parameter #66189

jjonescz Jan 5, 2023 •

edited

Loading

jcouv Jan 5, 2023 •

edited

Loading

AlekseyTs Jan 5, 2023 •

edited

Loading

jcouv Jan 5, 2023 •

edited

Loading

AlekseyTs Jan 5, 2023 •

edited

Loading

AlekseyTs Jan 5, 2023 •

edited

Loading

jcouv Jan 5, 2023 •

edited

Loading

jcouv Jan 6, 2023 •

edited

Loading

AlekseyTs commented Jan 11, 2023 •

edited

Loading

AlekseyTs Jan 11, 2023 •

edited

Loading

hamarb123 commented Jan 17, 2023 •

edited

Loading

hamarb123 commented Jan 17, 2023 •

edited

Loading