Add speclet for ref readonly parameters

[jjonescz]

• [Proposal]: ref readonly method parameters (VS 17.8, .NET 8) #6010
• Test Plan: ref readonly roslyn#68056

[hamarb123]

(Previously had this as a comment, deleted that and put it as a review so it can be replied to easier, and marked as resolved when done - edit: I thought this would achieve something 😅)
How does this interact with function pointers?
It would be nice if we could have ref readonly instead of in on function pointers, not just actual methods.
e.g. delegate*<ref int, in int, out int, void> currently gets emitted as method void *(int32&, int32& modreq([System.Runtime]System.Runtime.InteropServices.InAttribute), int32& modreq([System.Runtime]System.Runtime.InteropServices.OutAttribute)).
Presumably we could emit a ref readonly int input parameter as int32& modreq([System.Runtime]System.Runtime.InteropServices.InAttribute) modreq([System.Runtime]System.Runtime.CompilerServices.RequiresLocationAttribute) (or a modopt).
If we allowed this, we'd probably want to allow &Method which has ref readonly parameters to be implicitly castable to the same with in, and vice versa, to avoid source breaking changes.
Otherwise, we'd have to disallow ref readonly for function pointer parameters, or not actually encode it in binary, neither of which are ideal imo.

[jaredpar]

@hamarb123

How does this interact with function pointers?

Function pointers, method group conversions, method override and interface implementation generally fall into the same categories. The behavior in one generally follows to the other. My expectation is that whatever decisions comes from the override case will apply to the others.

[hamarb123]

@jaredpar Function pointers already encode in and out differently to normal methods; in has an extra modreq (which is only present when virtual normally), and out has an extra modreq that is never present normally. Imo function pointers with ref readonly parameters warrant a mention in the spec as to how it's encoded, and what the expected behaviour is.

[jaredpar]

in has an extra modreq (which is only present when virtual normally),

It's also present on delegate declarations.

and out has an extra modreq that is never present normally.

out does not have a modreq

Imo function pointers with ref readonly parameters warrant a mention in the spec as to how it's encoded, and what the expected behaviour is.

Where did I say they shouldn't be mentioned? My statement is that their behavior should follow how method group, overidde, etc ... work. That is fairly standard for this type of question. Once we establish that we'll codify how all of them work.

[jjonescz]

out does not have a modreq

I think @hamarb123 meant function pointers, which indeed do have modreq for out parameters: https://github.com/dotnet/csharplang/blob/0376b4cc500b1370da86d26be634c9acf9d60b71/proposals/csharp-9.0/function-pointers.md#out

[AlekseyTs]

I would like to address a concern that somehow the feature is "dead on arrival" if the existing APIs cannot simply use the new nice syntax without introducing a binary breaking change.

I do recognize that there is a set of existing APIs that is relatively hard to consume today because C# doesn't allow passing readonly lvalues to them. The APIs have this shape because the authors would never want a reference to an rvalue to be passed as an argument, the in parameter doesn't provide this guarantee, so a ref parameter was used. At the time in was introduced, I think we should have added ref readonly with the meaning - I want "strict" Lvalues, but readonly lvalues are Ok. Unfortunately, we haven't. And here we are.

On the other hand, since we are proposing to spend a nice language syntax, which is a very very scarce "resource", we should spend it very wisely. And I would say, we should spend it on the design that we would arrive to at the time we designed in modifier.
In my opinion, what is being proposed is not that design. The proposed design feels to me as a band-aid for existing APIs. It doesn't go beyond that in my opinion, and, in fact, if we switch existing APIs to use the band-aid, some of them would lose the lvalue guarantees that they have today. Spending a language syntax on a band-aid, is extremely unwise.

I think that the issue around consumption of the existing APIs can be addressed slightly differently. For example, the affected ref parameters could be decorated with an special attribute (quite possibly we will be able to reuse some existing attribute), which simply instructs the compiler to let readonly lvalues through because the method guarantees to not write through the reference. Support for this attribute can be implemented under umbrella of this feature as well. The issue with consumption of the APIs is addressed, there is no back-compatibility issue and we can use language syntax for a nice, clean language design with strong very useful guarantees to use for anyone on new APIs.

[tannergooding]

The APIs have this shape because the authors would never want a reference to an rvalue to be passed as an argument, the in parameter doesn't provide this guarantee, so a ref parameter was used

This is largely inaccurate, at least for the BCL. The majority of impacted APIs (dotnet/runtime#85911 is a non comprehensive list, but covers most of the core cases) were exposed in the 15 years before in even existed and used ref simply because that was the only thing that existed. There was no consideration of lvalue vs rvalue at all simply the need to take something byref.

Once in existed, there was a subset of these already exposed APIs that would've moved to in (no care about lvalue vs rvalue) but couldn't because ref->in is a source breaking change. Newer APIs did take advantage of in where possible and there was only a very small subset where we opted to explicitly avoid using in because it allowed rvalues (such as Unsafe.IsNullRef).

The reason for these latter cases is because it is most typically a user bug to pass in an rvalue. IsNullRef(rvalue) can never be true. Likewise the many Read(rvalue) APIs are just the equivalent of doing a copy for an rvalue. None of these are "terrible" and the concerned API owners have already said that a warning would be more than sufficient to address the concerns here.

In my opinion, what is being proposed is not that design. The proposed design feels to me as a band-aid for existing APIs. It doesn't go beyond that in my opinion, and, in fact, if we switch existing APIs to use the band-aid, some of them would lose the lvalue guarantees that they have today. Spending a language syntax on a band-aid, is extremely unwise.

They don't lose that guarantee, no more than they already have today. The codebase will emit a warning, and as with all warnings a user ignoring it is on them. A user already could do the wrong thing by explicitly spilling the value to the stack and calling ref local which is the same thing we're trying to avoid with ref readonly.

APIs do gain something much more valuable in that they can:

1. Safely version to the desired behavior with users recieving a warning if they're doing something "wrong"
2. The semantics of the byref are correctly handled such as to avoid copies or needing to use actually dangerous unsafe code to workaround the lack of a feature today

I think that the issue around consumption of the existing APIs can be addressed slightly differently. For example, the affected ref parameters could be decorated with an special attribute (quite possibly we will be able to reuse some existing attribute), which simply instructs the compiler to let readonly lvalues through because the method guarantees to not write through the reference. Support for this attribute can be implemented under umbrella of this feature as well. The issue with consumption of the APIs is addressed, there is no back-compatibility issue and we can use language syntax for a nice, clean language design with strong very useful guarantees to use for anyone on new APIs.

This sounds like introducing a third feature that does the same general thing. I don't think we need an extra feature to cover the transition of in to ref readonly and that just unnecessarily complicates the language even more for something that is extra niche over having a warning.

[AlekseyTs]

My point is that the desire to transition the existing APIs without binary breaking change should not be the main driving factor for the design. Let's figure out how do we want the feature to work. Then we can evaluate if that helps addressing issues around existing APIs, and alternatives for cases that aren't addressed.

And, BTW, a warning doesn't guarantee anything, in my opinion.

[tannergooding]

My point is that the desire to transition the existing APIs without binary breaking change should not be the main driving factor for the design. Let's figure out how do we want the feature to work. Then we can evaluate if that helps addressing issues around existing APIs, and alternatives for cases that aren't addressed.

I understand and the primary interested parties heavily weighed in on the initial design proposal (#6010) which has already gone to LDM and had many of these points raised/discussed.

This comes across as rehashing the same points that were already debated fairly extensively.

And, BTW, a warning doesn't guarantee anything, in my opinion.

Different users have different opinions. It isn't a strong guarantee but for most users they are more than sufficient and the more common scenario to be encountered. This is particularly the case for those that have WarnAsError, that turn on Warning Waves, that use Analyzers (where "errors" can be disabled), etc.

Most things we surface to users about "you might be doing something wrong" are warnings and this is really no different. It's why we had the discussion around whether this could be an analyzer with the reason why not largely being that a language feature was needed anyways (to allow safely changing from ref -> in) and that an analyzer identifying the hidden copies inserted by the compiler is itself incredibly difficult/error prone and thus the compiler handling it is likely better.

[AlekseyTs]

Different users have different opinions.

Yes, that is the case. And my comments are not blocking for this PR. However, I think it is worth brining this to LDM as soon as possible to confirm the course.

[hamarb123]

As a likely user of this feature, if it were made a breaking change from in or ref, then it would be dead-on-arrival to me, and would take a long time to recover from the death, since I would only be able to use it on new APIs.

[hamarb123]

Mostly lgtm, but I think we/LDM needs to discuss behaviour with function pointers more.

[hamarb123]

Does this mean I wouldn't be able to do the following? If so, it seems a bit weird:

int a = 1;
ref readonly int b = ref a; //note we use ref for this
ref readonly int c = ref b; //and for this

//would the following 2 lines be errors?
X1(ref c);
X2(ref c); //warning here obviously

void X1(ref readonly int x) { }
void X2(in int x) { }

I was hoping we would be able to just use ref everywhere. It seems a bit odd to disallow ref here, since we use ref (not in) for ref readonly locals etc.

[jjonescz]

Yes, X1(ref c) will be an error no matter whether X1 is defined with ref/in/ref readonly parameter (the value kind of c is checked against the ref modifier at the callsite), you will have to use X1(in c) when c is readonly. The reason is that ref at the call site says "passing a mutable reference" whereas in says "passing a readonly reference" - ref readonly parameters just allow both of them.

Ref assignments are special, they use ref always, but it's pretty clear if it's readonly or not, since that's declared right there on the left-hand side.