Treat `TypedReference` as managed #66328

jcouv · 2023-01-09T18:49:10Z

From discussion with runtime team, ArgIterator and RuntimeArgumentHandle should be left as unmanaged for now.

Filed API request for SRM: dotnet/runtime#80812

jaredpar · 2023-01-11T01:26:38Z

Note: it's possible this will cause a build break in dotnet/runtime, we should do a test insertion there to make sure we get ahead of any issues.

@AaronRobinsonMSFT, @stephentoub FYI

AaronRobinsonMSFT · 2023-01-11T01:33:08Z

Note: it's possible this will cause a build break in dotnet/runtime, we should do a test insertion there to make sure we get ahead of any issues.

@AaronRobinsonMSFT, @stephentoub FYI

@jcouv If you can kick off a build in dotnet/runtime and tag me, I can handle investigating any failures and trying to fix things up.

jcouv · 2023-01-11T04:42:07Z

@AaronRobinsonMSFT Here's the PR showing impact on runtime repo: dotnet/runtime#80484 (tagged you on it)

RikkiGibson

tbh, I'd also be concerned that VS is treating TypedReference as unmanaged. Maybe give heads up to infraswat when you merge?

jcouv · 2023-01-11T21:08:13Z

tbh, I'd also be concerned that VS is treating TypedReference as unmanaged. Maybe give heads up to infraswat when you merge?

Will do! Also, I started a VS test build (hopefully I did that right).

AlekseyTs · 2023-01-11T22:55:40Z

docs/compilers/CSharp/Compiler Breaking Changes - DotNet 7.md

+```csharp
+unsafe
+{
+    TypeReference* r = null; // warning: This takes the address of, gets the size of, or declares a pointer to a managed type


TypeReference* r = null; // warning: This takes the address of, gets the size of, or declares a pointer to a managed type

Is this the only possible negative effect? Could this cause an error in some scenarios? #Closed

Yes, there would also be errors for example with the unmanaged constraint.

Never mind, we have an error already for TypedReference as a type argument (regardless of constraints). So no other negative effect.

AlekseyTs · 2023-01-11T22:58:41Z

src/Compilers/CSharp/Test/Semantic/Semantics/StackAllocInitializerTests.cs

@@ -62,6 +62,9 @@ unsafe void M()
                // (10,29): error CS0208: Cannot take the address of, get the size of, or declare a pointer to a managed type ('RefG<string>')
                //         var x2 = stackalloc RefG<string>[10];
                Diagnostic(ErrorCode.ERR_ManagedAddr, "RefG<string>").WithArguments("RefG<string>").WithLocation(10, 29),
+                // (12,29): error CS0208: Cannot take the address of, get the size of, or declare a pointer to a managed type ('TypedReference')


// (12,29): error CS0208: Cannot take the address of, get the size of, or declare a pointer to a managed type ('TypedReference')

It looks like this is a "hard" break for a scenario that used to compile. Not sure if it would work at runtime though. #Resolved

stackalloc for a managed type wouldn't work at runtime today but could be supported at some point (from offline discussion with David W as part of PR #64294). That's why we kept the CheckManagedAddr check an error for stackalloc rather than downgrading it to a warning.

Looking at the implementation of TypedReference in the runtime repo, the type doesn't actually contain a reference yet (it's IntPtr). So the error David described wouldn't kick in yet. I'll add stackalloc to the break doc.

Note: I've significantly edited this after some discussions on the C# discord, feel free to look through the edits if you want.

I would think stackalloc should not work for any of the special types TypedReference, ArgIterator and RuntimeArgumentHandle as these can all contain references that only the runtime may understand (and thus keep correct with objects moving around), meaning it should not just be disallowed for TypedReference. These types would have to be on the stack as their appropriate types for the runtime to be able to understand them, meaning that they shouldn't be able to be stackalloc'd as it could be an invalid operation (apparently current .NET runtime considers ArgIterator to not have any tracking information, presumably older runtimes also have this, meaning it's not currently invalid, but also not guaranteed to be valid currently). If any of these are not tracked, that is currently an implementation detail and not guaranteed by the spec. Note that in the ECMA-335 spec these types are called byref-like types (I.8.2.1.1), so any restrictions that applies to spans (which are post ECMA-335) should also apply to these types (note also, that the ECMA-335 addendum doesn't change this).

I also do not see what functional difference it would make other than disallowing always incorrect behaviour from being legal in C# without any warning (for something like ArgIterator* which is not necessarily wrong, but may be) or error (for something like stackalloc ArgIterator[10] which could be wrong).

Another option could be to add a note to the ECMA-335 addendum stating that ArgIterator and RuntimeArgumentHandle do not contain any tracking information, thus making everything valid as it currently is.

Tagging @AaronRobinsonMSFT in case anything to add.
I'll quote from our chat, "For the current implementation and supported platforms they [ArgIterator and RuntimeArgumentHandle] can be considered unmanaged without issue - they will always point to the stack. I do not expect that we would ever run into a problem with unmanaged constraint for these two."

I see, that's fair then. I think there should be an addition to the ECMA-335 addendum that these types do not contain any tracking information so that this is actually documented and guaranteed.

This is ill-advised. The ECMA-335 is designed to be agnostic to OS implementation details - even though we violate that occasionally. At present the entire varargs scenario is only supported on Windows and is primarily for the C++/CLI scenario. We could update things and add notes but unless people are using .NET Core and actively innovating in the C++/CLI domain, I don't think doing much them at present is appropriate. The only reason this even came up was because of TypedReference improvements and @jcouv being curious :) Once we officially decide the varargs message going forward, I agree we should update all of this.

Tagging @AaronRobinsonMSFT in case anything to add. I'll quote from our chat, "For the current implementation and supported platforms they [ArgIterator and RuntimeArgumentHandle] can be considered unmanaged without issue - they will always point to the stack. I do not expect that we would ever run into a problem with unmanaged constraint for these two."

@jcouv Just thought I'd mention that this may not be the case with async2 if it allows byrefs in it at some point, as is widely requested.

/cc @agocke

As far as async2 and ArgIterator and RuntimeArgumentHandle, it is unlikely and I would push back on adding support for use of either of these types in those scenarios.

AlekseyTs

LGTM (commit 2)

jcouv · 2023-01-12T18:00:06Z

It looks like there's a legitimate test failure in the used assemblies leg with test IsManagedType_TypedReference. Will investigate

src/Compilers/Core/Portable/PEWriter/MetadataWriter.cs

jcouv · 2023-01-18T22:02:50Z

@AlekseyTs @cston @RikkiGibson MetadataWriter could not handle emitting TypedReference* properly (a parameter with that type hits the assertion in SerializeTypeReference today). Fixed now. Please take another look.

RikkiGibson · 2023-01-18T23:56:07Z

docs/compilers/CSharp/Compiler Breaking Changes - DotNet 7.md

@@ -9,7 +9,8 @@ Moving forward the `System.TypedReference` type is considered managed.
 ```csharp
 unsafe
 {
-    TypeReference* r = null; // warning: This takes the address of, gets the size of, or declares a pointer to a managed type
+    TypedReference* r = null; // warning: This takes the address of, gets the size of, or declares a pointer to a managed type
+    var a = stackalloc TypedReference[1]; // error: Cannot take the address of, get the size of, or declare a pointer to a managed type


Should this be warning: This takes the address of, ...?

No, stackalloc still reports managed types as errors. See #66328 (comment)

AlekseyTs · 2023-01-19T02:23:57Z

src/Compilers/Core/Portable/PEWriter/MetadataWriter.cs

@@ -3020,12 +3020,6 @@ protected void SerializeLocalVariableType(LocalVariableTypeEncoder encoder, ILoc
                SerializeCustomModifiers(encoder.CustomModifiers(), local.CustomModifiers);
            }

-            if (module.IsPlatformType(local.Type, PlatformType.SystemTypedReference))
-            {
-                encoder.TypedReference();


encoder.TypedReference();

I assume we had an existing test covering this code path? #Closed

Yes, we have multiple tests creating locals from __makeref. I also added in test along with TypedReference* local.

AlekseyTs · 2023-01-19T02:25:15Z

src/Compilers/CSharp/Test/Semantic/Semantics/UnsafeTests.cs

+            var libSrc = @"
+public unsafe class C
+{
+    public static System.TypedReference* M(System.TypedReference* r)


System.TypedReference* r

Are we testing similar scenario with a local? #Closed

AlekseyTs · 2023-01-19T02:26:59Z

Done with review pass (commit 5)

AlekseyTs

LGTM (commit 6)

cston · 2023-01-19T16:29:57Z

src/Compilers/CSharp/Test/Semantic/Semantics/UnsafeTests.cs

+            comp.VerifyEmitDiagnostics(
+                // (4,12): error CS0610: Field or property cannot be of type 'TypedReference'
+                //     public System.TypedReference field;
+                Diagnostic(ErrorCode.ERR_FieldCantBeRefAny, "System.TypedReference").WithArguments("System.TypedReference").WithLocation(4, 12)


ERR_FieldCantBeRefAny

We should test ref field as well.

cston

LGTM, with a test suggestion.

jcouv self-assigned this Jan 9, 2023

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

jcouv marked this pull request as ready for review January 11, 2023 01:21

jcouv requested a review from a team as a code owner January 11, 2023 01:21

jcouv marked this pull request as draft January 11, 2023 01:53

jcouv mentioned this pull request Jan 11, 2023

Suppress warnings for taking address of TypedReference dotnet/runtime#80484

Merged

Treat TypedReference as managed

a4a4a72

jcouv force-pushed the typed-reference branch from 2c51bac to a4a4a72 Compare January 11, 2023 04:41

jcouv marked this pull request as ready for review January 11, 2023 15:34

cston approved these changes Jan 11, 2023

View reviewed changes

jcouv added this to the 17.5 milestone Jan 11, 2023

RikkiGibson approved these changes Jan 11, 2023

View reviewed changes

jcouv added the Pending director approval label Jan 11, 2023

AlekseyTs reviewed Jan 11, 2023

View reviewed changes

Document stackalloc

5fe1e48

AlekseyTs approved these changes Jan 12, 2023

View reviewed changes

jcouv removed the Pending director approval label Jan 12, 2023

jcouv modified the milestones: 17.5, 17.6 Jan 12, 2023

jcouv marked this pull request as draft January 12, 2023 23:20

Show emit issue

f062552

jcouv mentioned this pull request Jan 18, 2023

[API Proposal]: SignatureTypeEncoder.TypedReference() dotnet/runtime#80812

Closed

Allow emitting TypedReference in new contexts

557fea7

jcouv commented Jan 18, 2023

View reviewed changes

src/Compilers/Core/Portable/PEWriter/MetadataWriter.cs Show resolved Hide resolved

jcouv requested review from AlekseyTs, RikkiGibson and cston January 18, 2023 21:58

jcouv marked this pull request as ready for review January 18, 2023 21:58

jcouv force-pushed the typed-reference branch from a8e8691 to 557fea7 Compare January 18, 2023 22:12

Add test for array

5490523

RikkiGibson reviewed Jan 18, 2023

View reviewed changes

AlekseyTs reviewed Jan 19, 2023

View reviewed changes

Address feedback

db208b8

jcouv requested a review from AlekseyTs January 19, 2023 07:10

AlekseyTs approved these changes Jan 19, 2023

View reviewed changes

cston reviewed Jan 19, 2023

View reviewed changes

cston approved these changes Jan 19, 2023

View reviewed changes

Add test with ref field

d7b783c

jcouv enabled auto-merge (squash) January 19, 2023 19:36

jcouv merged commit bebe09e into dotnet:main Jan 20, 2023

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

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

jcouv deleted the typed-reference branch March 23, 2023 17:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Treat `TypedReference` as managed #66328

Treat `TypedReference` as managed #66328

jcouv commented Jan 9, 2023 •

edited

Loading

jaredpar commented Jan 11, 2023

AaronRobinsonMSFT commented Jan 11, 2023

jcouv commented Jan 11, 2023

RikkiGibson left a comment

jcouv commented Jan 11, 2023

AlekseyTs Jan 11, 2023 •

edited

Loading

jcouv Jan 12, 2023

jcouv Jan 12, 2023

AlekseyTs Jan 11, 2023 •

edited by jcouv

Loading

jcouv Jan 12, 2023 •

edited

Loading

jcouv Jan 12, 2023

hamarb123 Jan 12, 2023 •

edited

Loading

jcouv Jan 12, 2023

hamarb123 Jan 12, 2023

AaronRobinsonMSFT Jan 13, 2023

hamarb123 Apr 10, 2024 •

edited

Loading

AaronRobinsonMSFT Apr 10, 2024

AlekseyTs left a comment

jcouv commented Jan 12, 2023

jcouv commented Jan 18, 2023 •

edited

Loading

RikkiGibson Jan 18, 2023

jcouv Jan 19, 2023

AlekseyTs Jan 19, 2023 •

edited

Loading

jcouv Jan 19, 2023

AlekseyTs Jan 19, 2023 •

edited

Loading

jcouv Jan 19, 2023

AlekseyTs commented Jan 19, 2023

AlekseyTs left a comment

cston Jan 19, 2023 •

edited

Loading

cston left a comment

Treat TypedReference as managed #66328

Treat TypedReference as managed #66328

Conversation

jcouv commented Jan 9, 2023 • edited Loading

jaredpar commented Jan 11, 2023

AaronRobinsonMSFT commented Jan 11, 2023

jcouv commented Jan 11, 2023

RikkiGibson left a comment

Choose a reason for hiding this comment

jcouv commented Jan 11, 2023

AlekseyTs Jan 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Jan 11, 2023 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv Jan 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hamarb123 Jan 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hamarb123 Apr 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs left a comment

Choose a reason for hiding this comment

jcouv commented Jan 12, 2023

jcouv commented Jan 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Jan 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Jan 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented Jan 19, 2023

AlekseyTs left a comment

Choose a reason for hiding this comment

cston Jan 19, 2023 • edited Loading

Choose a reason for hiding this comment

cston left a comment

Choose a reason for hiding this comment

Treat `TypedReference` as managed #66328

Treat `TypedReference` as managed #66328

jcouv commented Jan 9, 2023 •

edited

Loading

AlekseyTs Jan 11, 2023 •

edited

Loading

AlekseyTs Jan 11, 2023 •

edited by jcouv

Loading

jcouv Jan 12, 2023 •

edited

Loading

hamarb123 Jan 12, 2023 •

edited

Loading

hamarb123 Apr 10, 2024 •

edited

Loading

jcouv commented Jan 18, 2023 •

edited

Loading

AlekseyTs Jan 19, 2023 •

edited

Loading

AlekseyTs Jan 19, 2023 •

edited

Loading

cston Jan 19, 2023 •

edited

Loading