Do not copy nullable value type receiver of a constrained call. #66311

AlekseyTs · 2023-01-08T00:14:07Z

The check that we used to detect value types at runtime didn’t quite work for nullable value types due to special boxing behavior for them. Switching to typeof(T).IsValueType check instead.

Fixes #66162.
Related to #63221.

The check that we used to detect value types at runtime didn’t quite work for nullable value types due to special boxing behavior for them. Switching to `typeof(T).IsValueType` check instead. Fixes dotnet#66162. Related to dotnet#63221.

AlekseyTs · 2023-01-09T15:19:07Z

@dotnet/roslyn-compiler Please review

AlekseyTs · 2023-01-09T15:21:22Z

@333fred Please review, you should know the context since you reviewed #65642

jcouv · 2023-01-09T21:49:57Z

FWIW, I took a look (trying to understand the problem in the bug and how it was resolved), but couldn't figure it out after 20 minutes so I'm leaving it for someone else or if some more context could be provided (what's wrong with the IL from Call1 or Call2 in example, how does the fix address the problem?)

In reply to: 1376375491

AlekseyTs · 2023-01-09T22:47:41Z

@jcouv

I took a look (trying to understand the problem in the bug and how it was resolved), but couldn't figure it out after 20 minutes so I'm leaving it for someone else or if some more context could be provided (what's wrong with the IL from Call1 or Call2 in example, how does the fix address the problem?)

If you look at the IL diff for affected tests, you'll notice that the change replaces sequences like

  IL_0002:  ldloca.s   V_0
  IL_0004:  initobj    ""T""
  IL_000a:  ldloc.0
  IL_000b:  box        ""T""
  IL_0010:  brtrue.s   IL_001a

with sequences like:

  IL_0002:  ldtoken    ""T""
  IL_0007:  call       ""System.Type System.Type.GetTypeFromHandle(System.RuntimeTypeHandle)""
  IL_000c:  call       ""bool System.Type.IsValueType.get""
  IL_0011:  brtrue.s   IL_001b

These IL sequences produce the same result for all types, but for nullable value types. There is a special boxing behavior for nullable value types, boxing of a default value of a nullable value type yields null. Therefore, the old IL is not going to jump for a nullable value type, but instead going to fall through to the code that is supposed to be executed only for a class. That is wrong because a nullable value type is a value type.

jcouv · 2023-01-09T23:49:21Z

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

@@ -4579,6 +4579,21 @@ internal enum AddressKind
                        stackLocalsOpt));
                    goto case BoundKind.ConditionalReceiver;

+                case BoundKind.ComplexReceiver:


Can we reach this? I thought this node would only be introduced in a later phase (but not during initial binding). #Resolved

Can we reach this? I thought this node would only be introduced in a later phase (but not during initial binding).

The same goes for BoundKind.ComplexConditionalReceiver. Just follow the pattern.

Why not throw unreachable exception since this is unreachable? I'd do that for ComplexConditionalReceiver too

Why not throw unreachable exception since this is unreachable? I'd do that for ComplexConditionalReceiver too

I am comfortable going as is

jcouv · 2023-01-10T00:26:06Z

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

+
+        private void EmitComplexReceiver(BoundComplexReceiver expression, bool used)
+        {
+            Debug.Assert(!used);


nit: What's the benefit of only asserting? Throwing seems better.
We can still keep the conditional AdjustStack below for the day we could reach with here with used==true. #Resolved

What's the benefit of only asserting? Throwing seems better.

The implementation is doing the right thing. I do not see any point in throwing. The assert will help us identify the test scenario.

jcouv · 2023-01-10T00:30:51Z

);

We have a note in the compiler test plan: "If you add a new expression form that requires spilling, test it in the catch filter". Consider adding a test for this.

In reply to: 1376544884

Refers to: src/Compilers/CSharp/Test/Emit2/CodeGen/CodeGenCallTests.cs:30721 in 208dc2a. [](commit_id = 208dc2a, deletion_comment = False)

jcouv

LGTM Thanks (iteration 1) with test suggestion to consider

333fred · 2023-01-10T01:19:20Z

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

@@ -2940,6 +2940,20 @@ public override BoundNode VisitComplexConditionalReceiver(BoundComplexConditiona
            return null;
        }

+        public override BoundNode VisitComplexReceiver(BoundComplexReceiver node)


Are any flow passes actually going to see one of these? It looks like they're only created during local lowering, so I wouldn't expected them to be seen. #Resolved

Are any flow passes actually going to see one of these? It looks like they're only created during local lowering, so I wouldn't expected them to be seen.

This code path is reachable. Flow analysis is done during async rewrite, etc.

333fred

Done review pass (commit 1)

AlekseyTs · 2023-01-10T02:03:51Z

);

We have a note in the compiler test plan: "If you add a new expression form that requires spilling, test it in the catch filter". Consider adding a test for this.

I think that refers to language constructs that themselves trigger spilling, like async or patterns, and is not talking about new bound nodes. I am not adding anything like that.

In reply to: 1376544884

Refers to: src/Compilers/CSharp/Test/Emit2/CodeGen/CodeGenCallTests.cs:30721 in 208dc2a. [](commit_id = 208dc2a, deletion_comment = False)

…l. (#66311)" This reverts commit 8bdab1a.

…l." (#66407) Reverts #66311 due to dotnet/runtime#80429 (comment)

Related to dotnet#63221.

Related to #63221.

AlekseyTs added the Area-Compilers label Jan 8, 2023

AlekseyTs requested a review from a team as a code owner January 8, 2023 00:14

jcouv reviewed Jan 9, 2023

View reviewed changes

jcouv reviewed Jan 10, 2023

View reviewed changes

jcouv approved these changes Jan 10, 2023

View reviewed changes

jcouv self-assigned this Jan 10, 2023

333fred reviewed Jan 10, 2023

View reviewed changes

333fred approved these changes Jan 10, 2023

View reviewed changes

AlekseyTs merged commit 8bdab1a into dotnet:main Jan 10, 2023

ghost added this to the Next milestone Jan 10, 2023

allisonchou mentioned this pull request Jan 11, 2023

[Automated] PRs inserted in VS build main-33311.152 #66362

Closed

MichalStrehovsky mentioned this pull request Jan 13, 2023

[main] Update dependencies from 9 repositories dotnet/runtime#80429

Merged

AlekseyTs added a commit that referenced this pull request Jan 13, 2023

Revert "Do not copy nullable value type receiver of a constrained cal…

6c097b6

…l. (#66311)" This reverts commit 8bdab1a.

AlekseyTs mentioned this pull request Jan 13, 2023

Revert "Do not copy nullable value type receiver of a constrained call." #66407

Merged

AlekseyTs added a commit that referenced this pull request Jan 13, 2023

Revert "Do not copy nullable value type receiver of a constrained cal…

1dd4753

…l." (#66407) Reverts #66311 due to dotnet/runtime#80429 (comment)

AlekseyTs added a commit to AlekseyTs/roslyn that referenced this pull request Jan 17, 2023

Preserve changes that are still relevant after revert of dotnet#66311

ed3ed3a

Related to dotnet#63221.

AlekseyTs added a commit that referenced this pull request Jan 21, 2023

Preserve changes that are still relevant after revert of #66311 (#66440)

4d0af36

Related to #63221.

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not copy nullable value type receiver of a constrained call. #66311

Do not copy nullable value type receiver of a constrained call. #66311

AlekseyTs commented Jan 8, 2023

AlekseyTs commented Jan 9, 2023

AlekseyTs commented Jan 9, 2023

jcouv commented Jan 9, 2023 •

edited by AlekseyTs

Loading

AlekseyTs commented Jan 9, 2023

jcouv Jan 9, 2023 •

edited by AlekseyTs

Loading

AlekseyTs Jan 9, 2023

jcouv Jan 10, 2023

AlekseyTs Jan 10, 2023

jcouv Jan 10, 2023 •

edited by AlekseyTs

Loading

AlekseyTs Jan 10, 2023

jcouv commented Jan 10, 2023 •

edited by AlekseyTs

Loading

jcouv left a comment

333fred Jan 10, 2023 •

edited by AlekseyTs

Loading

AlekseyTs Jan 10, 2023

333fred left a comment

AlekseyTs commented Jan 10, 2023

Do not copy nullable value type receiver of a constrained call. #66311

Do not copy nullable value type receiver of a constrained call. #66311

Conversation

AlekseyTs commented Jan 8, 2023

AlekseyTs commented Jan 9, 2023

AlekseyTs commented Jan 9, 2023

jcouv commented Jan 9, 2023 • edited by AlekseyTs Loading

AlekseyTs commented Jan 9, 2023

jcouv Jan 9, 2023 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs Jan 9, 2023

Choose a reason for hiding this comment

jcouv Jan 10, 2023

Choose a reason for hiding this comment

AlekseyTs Jan 10, 2023

Choose a reason for hiding this comment

jcouv Jan 10, 2023 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs Jan 10, 2023

Choose a reason for hiding this comment

jcouv commented Jan 10, 2023 • edited by AlekseyTs Loading

jcouv left a comment

Choose a reason for hiding this comment

333fred Jan 10, 2023 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs Jan 10, 2023

Choose a reason for hiding this comment

333fred left a comment

Choose a reason for hiding this comment

AlekseyTs commented Jan 10, 2023

jcouv commented Jan 9, 2023 •

edited by AlekseyTs

Loading

jcouv Jan 9, 2023 •

edited by AlekseyTs

Loading

jcouv Jan 10, 2023 •

edited by AlekseyTs

Loading

jcouv commented Jan 10, 2023 •

edited by AlekseyTs

Loading

333fred Jan 10, 2023 •

edited by AlekseyTs

Loading