dotnet / csharplang

The official repo for the design of the C# programming language
11.53k stars 1.03k forks source link

allow references to structs to be stored in fields of ref structs #1147

Open lucasmeijer opened 6 years ago

lucasmeijer commented 6 years ago

Unity is a game engine that uses C# extensively.

We're in need of a way to have a struct that contains one or more pointers to other structs that we want to be able to read/write from/to. We do this with unsafe code today:

    struct Color { float r,g,b; }

    unsafe struct Group
    {
      Color* _color;
      ref Color color => ref *_color;
    }

But we would love to do this without unsafe code, and would like to suggest a feature where it's allowed to store references to structs in fields of ref structs, that can guarantee that these pointers don't escape onto the heap:

    struct Color { float r,g,b; }

    ref struct Group
    {
        ref Color color;
    }

EDIT link to spec for this feature https://github.com/dotnet/csharplang/blob/main/proposals/low-level-struct-improvements.md

AlexRadch commented 4 years ago

No, this is different. This is ref fields, a thing not allowed in the language today.

You can use Span<Color> as ref to Color variable

    struct Color { public float r,g,b; }

    ref struct Group
    {
        public Span<Color> Color;
    }

    public static void Main()
    {
        var color = new Color() { r = 255, g = 0, b = 0 };
        var group =  new Group() { Color = MemoryMarshal.CreateSpan(ref color, 1) };

        Console.WriteLine($"Red = {group.Color[0].r}");
    }

See https://dotnetfiddle.net/c4J6r2 also

jaredpar commented 4 years ago

@AlexRadch

From the [documentation] on MemoryMarshal.CreateSpan

This method should be used with caution. It is dangerous because the length argument is not checked. In addition, the lifetime of the returned span is not validated for safety by span-aware languages.

Essentially this method is incredibly unsafe. The returned ref has no safety rules associated with it. The idea of ref fields is to do this in a safe fashion where the compiler will provide the same rules around lifetime safety that it does for Span<T> today.

AlexRadch commented 4 years ago

Essentially this method is incredibly unsafe. The returned ref has no safety rules associated with it. The idea of ref fields is to do this in a safe fashion where the compiler will provide the same rules around lifetime safety that it does for Span<T> today.

It is unsafe only for lifetime. But if lifetime of ref is less than lifetime of variable then it is safe.

c# is not Rust it does not have lifetime safety. You try to add it.

For lifetime safety use array of length 1 and Span to it.

AlexRadch commented 4 years ago

Span is more safe than pointer * and it does not requare to pin memory.

jaredpar commented 4 years ago

@AlexRadch

It is unsafe only for lifetime. But if lifetime of ref is less than lifetime of variable then it is safe.

Indeed. 100% of unsafe code is safe when used safely.

The point of unsafe code though is that the training wheels are off, the language is no longer providing the typical guarantees it provides (type and memory safety).

c# is not Rust it does not have lifetime safety. You try to add it.

Sorry this is incorrect. For ref and ref struct values the language has well defined and enforced safety rules. As long as users stay in safe code it's not possible to create issues like dangling refs. Once a user ventures into unsafe code, just as when they venture into unsafe in Rust, it is possible to create type and memory safety issues. That is one of the dangers posed by MemoryMarshal.CreateSpan.

En3Tho commented 4 years ago

@AlexRadch Span itself uses a quirk to store a ref instead of using proper language/runtime feature. Now you propose using even more quirks instead of having a proper runtime/language feature. Ref field Color != field Color*, there is huge difference between references and pointers. Also I do not get that assurance that Span[1] is the only and proper way to go. Spans are not a 100% safe construct, you break things easily if you're not careful with them.

ufcpp commented 4 years ago

You try to add it.

Already. https://github.com/dotnet/csharplang/pull/3936

jaredpar commented 4 years ago

@En3Tho

Indeed Span<T> uses an intrinsic today called ByReference<T>. That type is fundamentally unsafe as it's essentially a ref field with no language safety attached. The proposal to add ref fields would let us remove that type entirely and have Span<T> be defined entirely in C# terms

https://github.com/dotnet/csharplang/blob/master/proposals/low-level-struct-improvements.md#provide-ref-fields

AlexRadch commented 4 years ago

That is one of the dangers posed by MemoryMarshal.CreateSpan.

Yes. MemoryMarshal.CreateSpan is what you want but only without safety from compiler.

En3Tho commented 4 years ago

@jaredpar I know about it, yes. I call it a quirk because it's internal, hard to use and has no language guarantees. I know of cases where people had to copy that stuff from runtime but there is a meaning in keeping stuff like that hidden from the general community. And thank you for sharing this proposal, it's a good read :) @AlexRadch

Yes. But MemoryMarshal.CreateSpan is what you want but only without safety from compiler.

You mean with safety, not without Also, do not forget that spans have a cost - it's length and I do not believe jit can elide it

AlexRadch commented 4 years ago

Indeed Span<T> uses an intrinsic today called ByReference<T>.

As I know ByReference<T> is not public and can not be used.

AJosephsen commented 4 years ago

I hate that I cannot implement functionality similar to Span in C# - it feels like I am being cheated. Span feels like a workaround for missing ref fields.

@jaredpar https://github.com/dotnet/csharplang/blob/master/proposals/low-level-struct-improvements.md#provide-ref-fields Seems to solve all that. Why isn't this on the official rodamap?

Sergio0694 commented 4 years ago

I hate that I cannot implement functionality similar to Span in C# - it feels like I am being cheated. Span feels like a workaround for missing ref fields.

As you said, you can actually do that already, it's just not ideal. You can make it work and with very good codegen as well though. For instance, you can piggyback one 4 bytes value in your custom span-like type into the int length parameter of Span<T>, so that that space doesn't go wasted either. For instance, in the Span2D<T> type in the Microsoft.Toolkit.HighPerformance package (here) I'm using a single Span<T> field to store both a ref to the target memory area and the height of the target 2D memory area. This way you basically save one field, and the resulting code is fairly optimal. It's just definitely trickier and you need to be careful to ensure the APIs that publicly exposed from your types are safe to use, but it can technically be done already with no real downsides, other than the compiler not helping you out to ensure lifetime safety or bounds checks and having to always use manual GC-ref offsetting everywhere, which is arguably a bit error prone. You can make it work and make it safe too though 😄

Of course, being able to do this with ref T fields in C# 10 (hopefully?) will be infinitely better for a number of reasons and I'm not saying the two things are equivalent, just pointing out a workarounds for those absolutely in need of such a thing today.

333fred commented 4 years ago

Seems to solve all that. Why isn't this on the official rodamap?

What more are you expecting to see? This is a championed issue, triaged into the Working Set milestone, with a proposal checked in, that the LDM has met twice on within the last month. If that's not on the roadmap, I don't know what is.

PathogenDavid commented 4 years ago

triaged into the Working Set milestone

Maybe I'm missing something, but I don't think it is:

image

333fred commented 4 years ago

I... Huh. Apparently we missed this when triaging things. Well, it's in the working set now.

ilexp commented 3 years ago

I'm confused about the state of this issue - the linked PR appears to add support for ref fields, it has been merged to master, but when checking the master branch via sharplab, it doesn't work. What am I missing? Is this already scheduled for a specific C# version release?

jnm2 commented 3 years ago

@ilexp Is the linked PR you're referring to the one that merged spec updates into the dotnet/csharplang master branch? The master branch that Sharplab refers to is the dotnet/roslyn master branch. Here's how you can keep tabs on implementation status in dotnet/roslyn: https://github.com/dotnet/roslyn/blob/master/docs/Language%20Feature%20Status.md

ilexp commented 3 years ago

Oh - yes, that is the one I was referring to. I didn't actually check the modified files 🤦 Thanks for pointing it out 👍