-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Allow ref struct to contain ref fields #32060
Comments
We would prefer this to be exposed as |
Hi @jkotas - thanks for linking that issue, I had missed that! 😊 Is there an ETA for that though? By the looks of it, and the fact it has been opened for over two years already, it looks like it still has a way to go, plus all the additional difficulties caused by the fact that that would require at least some Roslyn updates to support that, if not some CLR changes too. My rationale here was that making the |
We are here for the long haul and want to do things properly. We try hard to avoid exposing temporary workarounds that we are stuck forever even once the more appropriate solution is available. |
Sure thing, that's fair enough 😄 Is there any approximate ETA on that proposal? I see the issue has been open for over two years already and it's not currently assigned to any milestone, not even Thanks again for your time! 😊 |
It did not make it into the list of C# features we are looking at for .NET 5. |
Wouldn't exposing this type make it possible to create |
This type as it exists today is not safe construct. It is very easy to create dangling pointers with it, without guardrails in the language. If we were to expose it, we would likely mark it with the special ObsoleteAttribute to block its use in languages that are not aware of it (e.g. similar to how |
On a related note, Jan, you and I previously talked about a syntax that would allow operating with gcrefs as naturally as operating with pointers. Things that would allow natural syntax like the + operator without bouncing through |
Hey @jkotas - I have a follow up question on this, specifically regarding the general criteria being used to expose APIs or add explicit support for "unsafe" operations. You mentioned how with the public static Span<T> GetFaultySpan<T>()
{
T value = default;
return MemoryMarshal.CreateSpan(ref value, 1);
} This compiles fine, but results in a dangling pointer just like the one you mentioned. It's not perfectly clear to me why is this API public, but Or, just like the BCL already includes dedicated code to properly handle devs doing stuff that's technically not valid (eg. I just can't stress enough how useful the Just though I'd share my thoughts on this, thank you in advance for your time! 😊 P.S. Just so I'm perfectly clear, I'm not criticizing your work in any way, the .NET team is doing an incredible job and I really love where the whole platform is going! Just trying to make my case here as to why I think making |
@jaredpar had some reasons on why this wasn't safe from the language perspective and would be extremely likely to cause bugs without proper language support for tracking the lifetime. |
Methods in System.Runtime.InteropServices are unsafe. They can produce dangling pointers, corrupt memory, etc. You have to know what you are doing when using them and they should be avoided where possible.
Correct. We would like ref fields to have proper language that guarantees its safety. |
Ah, yeah that's a fair point, having And yeah I agree that just having language-level support for Thank you both again for your time, I really appreciate the conversation here! |
The reasons are pretty straight forward: the lifetime safety rules, as written today, are predicated on such a constructor not existing. It's explicitly called out in the design. That's not to say we can't do |
+1 on this. While the span workaround, well, works, it isn't ideal. Particularly where you have an interop scenario where you are using |
Having it actually added to |
Personally I'd already be more than happy just to have the public readonly ref struct Ref<T>
{
public readonly ref T Value;
public Ref(ref T value)
{
Value = value;
}
public ref T this[int i]
{
[MethodImpl(MethodImplOptions.AggressiveInlining)]
get => Unsafe.Add(ref Value, i);
}
} Which personally I'm very looking forward to, as it'd be a nice and fast abstraction over having to manually use |
well yeah, that is just better, but it is far away, will take a lot of compiler work, new language rules, etc. i just want the dangerous scary thing exposed that if you f#@! up then it doesn't help you |
~~If you reeeeeeeally wanted I don't think such an out-of-band type would be blittable or would play well with the interop system. I don't even think the internal in-box type fulfills these characteristics. You'd need to pin the T& to a T* if you wanted it to be blittable.~~ This idea is shot. See below. |
@GrabYourPitchforks I am not sure what you have in mind, but I do not think it would work. |
@GrabYourPitchforks |
@jkotas I thought as of netcoreapp2.1 the runtime itself had support for byref members of structs, even though the C# compiler doesn't? I was thinking you could write a class entirely in IL that looked something like: // pseudo-code
public ref struct ByReference<T>
{
private readonly T& _value;
public ByReference(ref T value) { _value = ref value; }
public ref T Value => ldfld(_value);
} The public API surface uses only constructs that the C# compiler can handle. Admittedly I haven't actually tried this. Just spitballing. |
That is not correct. You should see type load exception if you try to run your example. |
@GrabYourPitchforks nope, invalid IL. I believe the current way it works is by having a dummy Won't even assemble, iirc, @jkotas - |
Well, there goes that idea. :) |
Is there a good way to make sure it will not 'fell off the priority list' next time? |
Keep an eye on dotnet/csharplang#1147. That is the main issue I'm using to track the request at the moment (there are actually many overlapping issues for this feature). Once the proposal is finished that issue will have an update to point at it and from there you can see the LDM discussions on it. Probably won't make it up there for at least a few more weeks though. Q: Well what if @jaredpar never actually writes a proposal for this? |
@Sergio0694 I guess it is safe to close this issue now
Now, that we know where you live on GitHub, we will be haunting you as the shadow of Hamlet's father... :). |
Several of the features I want in are dependent on the feature, so I poke @jaredpar about it every know and again, as he can attest 😄 |
FYI: the initial proposal for allowing |
Given that Jared is driving this from the C# side, is there anything left for this runtime issue to track? Will close this issue after a few days if there's no remaining work. |
How discoverable are closed vs open issues? I know, for example, they don't show by default in issue search so that might be a point towards keeping it open until the C# feature is implemented. |
(We do have the |
The current plan is to add ref fields as first class construct to the type system. See proposal at dotnet/csharplang#3936 for more details.
Original proposal
Overview
Make the
System.ByReference<T>
type (here) public instead of internal.Rationale
It's already possible to achieve the same behavior of
ByReference<T>
, namely to store aref T
to some variable as a field in aref struct
, by leveraging theSpan<T>
andMemoryMarshal
types, like so:This can either be used as is, as a field in some other
ref struct
type, or the same technique (storing a 1-lengthSpan<T>
in place of the missingByReference<T>
type) can be used in otherref struct
types to reproduce this same behavior: storing a managed reference as a field in aref struct
type.Given that this is already possible with existing APIs, as shown above, but it's less performant than the original
ByReference<T>
, more verbose and more error prone, why not just make the originalByReference<T>
type public so everyone can use it with no need to reinvent the wheel?It'd be a neat API to have, and at basically no cost, since it's already in the .NET Core BCL anyway. 😄
The text was updated successfully, but these errors were encountered: