Skip to content
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

Change TypedReference to standard ref struct semantics + support __makeref(ref struct) #65255

Open
steveharter opened this issue Nov 7, 2022 · 3 comments

Comments

@steveharter
Copy link
Member

steveharter commented Nov 7, 2022

To support ref struct with reflection (invoking and passing), the current 8.0 runtime plan for "safe" code is to leverage the existing TypedReference type to create and obtain ref struct references through the existing __makeref and __refvalue keywords.

In addition, TypedReference, being a stack-only construct, was added in 1.0 well before ref struct support (or byref-like types) and currently has more restricted compiler semantics than ref struct causing various limitations such as the inability to use ref struct as a field. This has been discussed as fixing or sunsetting "restricted types" in
https://github.com/dotnet/csharplang/blob/main/proposals/csharp-11.0/low-level-struct-improvements.md but this has not yet been addressed. See also #64811. cc @jaredpar

An alternative to supporting __makeref\__refvalue\__reftype with ref struct but much more expensive, is to support generic method parameters for ref struct. This could be added in the future and new static methods added to TypedReference to support that. However, based on prior discussions, adding support for __makeref et al. would be much easier for now.

Misc prototyping in the roslyn repro to remove the various compiler restrictions shows the 8.0 .NET runtime seems to handle the changes fine. Whether the runtime needs a new "feature switch" should be discussed -- I assume most runtimes, if they support ref struct and TypedReference and __makeref et al., should work as-is without additional work. cc @AaronRobinsonMSFT @jkotas

Version Used:
MSBuild17.4.0+18d5aef85 (C# 11)

Steps to Reproduce:
Sample code that repro some of the restricted usages of TypedReference but are valid with ref structs such as Span<object>.:

Console.WriteLine("Hello, World!");

Span<object> obj = default;
TypedReference tr = __makeref(obj); // CS1601: Cannot make reference to variable of type 'Span<object>'

public class MyClass
{
    public TypedReference _field; // CS0610: Field or property cannot be of type 'TypedReference'
    public TypedReference CallMe() => default;  // CS1599: The return type of a method, delegate, or function pointer cannot be 'TypedReference'
    public void CallMe(ref TypedReference tr) {} //  CS1601: Cannot make reference to variable of type 'TypedReference' error CS1601
}

Related Links:

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged Issues and PRs which have not yet been triaged by a lead label Nov 7, 2022
@jaredpar
Copy link
Member

jaredpar commented Nov 7, 2022

The issue with TypedReference is that the underlying IL support is extremely flexible. In discussion with @davidwrighton he confirmed it's legal for a TypedReference to store a ref to a ref struct. That means from a lifetime perspective that TypedReference is equivalent to a ref struct that has ref fields to a ref struct. Support for those was cut from C# 11 due to the complexity they will add to lifetime rules. Before supporting TypedReference officially this needs to be rationalized.

There are a couple of ways we can approach this. The first is that we go the full distance and support everything TypedReference can support in IL in C# as well. That will be a relatively expensive feature as it requires significant additions to our lifetime rules.

The second approach is that we limit the set of TypedReference behaviors that we support in C#. For example we could limit the support in __makeref / __refvalue to what we support on ref struct today. By disallowing essentially a ref to ref struct but allowing ref to other types we could artificially the capability of TypedReference and fit it into our existing lifetime rules. That still requires a bit of design work but it's much more reasonable.

int i = 42;
Span<int> span = stackalloc int[42];
TypedReference tr1 = __makeref(ref i); // okay
TypedReference tr2  = __makeref(span); // okay
TypedReference tr3 = __makeref(ref span); // error

A third approach is to support a ref to ref struct in __makeref / __refvalue but in a very limited way. Essentially ways that force the value to always be downward facing only. That likely can work with some minimal adjustment to the rules but it's also possible the restrictions that come with it will be too limiting.

Before going any route though we'd need some more input from the runtime team. Essentially we'd need to see samples of what code you want to write, the full set of scenarios you want to support, etc ... Samples are critical to us getting the right defaults with lifetime rules.

Note: while __mkref / __refvalue don't fully support ref today it needs to be rationalized before we can move forward with TypedReference suppport. Otherwise we could back ourselves into a corner in future language versions.

Misc prototyping in the roslyn repro to remove the various compiler restrictions shows the 8.0 .NET runtime seems to handle the changes fine.

Removing the restriction is easy, adding the correct lifetime rules for TypedReference is not.

Whether the runtime needs a new "feature switch" should be discussed -- I assume most runtimes, if they support ref struct and TypedReference and __makeref et al., should work as-is without additional work.

From a language perspective I don't think we need a switch. I think that we can simply assume that if a runtime supports ref fields then we can extend language support to TypedReference.

@jaredpar jaredpar self-assigned this Nov 7, 2022
@jaredpar
Copy link
Member

jaredpar commented Nov 7, 2022

@RikkiGibson, @cston

@jaredpar jaredpar removed the untriaged Issues and PRs which have not yet been triaged by a lead label Nov 7, 2022
@jaredpar jaredpar added this to the C# 12.0 milestone Nov 7, 2022
@jaredpar jaredpar assigned RikkiGibson and unassigned jaredpar Jan 9, 2023
@jaredpar
Copy link
Member

jaredpar commented Jan 9, 2023

Posting a few thoughts I had after reading through the reflection invoke API draft about how TypedReference could potentially be handled.

I'll be honest, one thing I did not realize is that __makeref implicitly stores a ref to the argument, not the value. Effectively when you see __makeref(i) you have to kinda pretend you wrote __makeref(ref i). In the case that i was already a ref the argument is loaded directly (so still gets the ref) behavior. 

That does mean though that even the simple examples jump straight into the complicated ref field to ref struct (RFRS). 

Span<byte> span = ...; 
TypedReference tr = __makeref(span);

This effectively creates a ref field that points to a ref struct. It's undisguisable from the following code:

ref struct TR { 
  ref Span<byte> field;
  internal TR(ref Span<byte> span) {
    field = ref span;
  }
}

TR tr = new(ref span);

That means that unless we limit TypedReference in some way that it becomes an extremely hard to use type. For example consider that the following definitions are equally powerful:

void Swap(ref Span<byte> p1, ref Span<byte> p2);
void Swap(TypedReference p1, TypedReference p2);

The MAMM rules would suddenly start needing to treat every TypedReference as a ref Span<T>. It's a value that can smuggle out ref struct state. That would make TypedReference very difficult to use as even simple invocations would break MAMM.

// This is an API shape that meets the Swap criteria. 
void M(TypedReference tr1, TypedReference tr2)
G(TypedReference p1) {  
  Span<byte> span = stackalloc byte[42];  
  // Error!!!
  // Have to assume `span` assigned out through p1 which is a stack safety issue
  M(p1, __makeref(span));
}

The good news though is that this is an extremely controlled ref field. It requires an explicit action to unwrap and see it. I'm hopeful that we can use that to simplify things a bit and not have to do full RFRS. The initial thought that is bouncing around my head is the following: 

  • The STE of a TypedReference is the STE of the value passed in. 
  • The STE of __refvalue(tr) is the STE of the tr argument (which must be an instance of TypedReference)
  • The RSTE of ref __refvalue the RSTE is always current method
  • An expression of the form ref __refvalue(tr, T) where T is a ref struct is considered unsafe

That gives us safety and puts the burden on correctness for the very small number of cases which grab the ref struct out by ref.

Still need to think deeper about this and spec it out but I think this is the outline of a way forward here. @RikkiGibson will be doing the spec work on this.

@jaredpar jaredpar modified the milestones: C# 12.0, Backlog Sep 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants