[API Proposal]: RequiresLocationAttribute (supporting ref readonly parameters)

[Sergio0694]

Background and motivation

As part of the new ref readonly parameters feature (dotnet/csharplang#6010), Roslyn will emit a new [RequiresLocation] attribute in some cases, all documented in the spec in that linked issue (as well as in the speclet at dotnet/csharplang#7165). This issue tracks adding the attribute to the runtime as well, so it can be shared (just like eg. [RefSafetyRules]).

API Proposal

namespace System.Runtime.CompilerServices
{
    [AttributeUsage(AttributeTargets.Parameter, AllowMultiple = false, Inherited = false)]
    public sealed class RequiresLocationAttribute : Attribute
    {
    }
}

[ghost]

Tagging subscribers to this area: @dotnet/area-system-runtime-compilerservices
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Background and motivation

As part of the new ref readonly parameters feature (dotnet/csharplang#6010), Roslyn will emit a new [RequiresLocation] attribute in some cases, all documented in the spec in that linked issue (as well as in the speclet at dotnet/csharplang#7165). This issue tracks adding the attribute to the runtime as well, so it can be shared (just like eg. [RefSafetyRules]).

API Proposal

namespace System.Runtime.CompilerServices
{
    [AttributeUsage(AttributeTargets.Parameter, AllowMultiple = false, Inherited = false)]
    public sealed class RequiresLocationAttribute : Attribute
    {
    }
}

API Usage

n/a

Alternative Designs

No response

Risks

No response

Author:	Sergio0694
Assignees:	-
Labels:	api-suggestion, area-System.Runtime.CompilerServices, untriaged
Milestone:	-

[stephentoub]

We're not going to add such an attribute until it's actually used by the language/compiler.

[Sergio0694]

Of course. This isn't meant to go to to API review yet 🙂

[tannergooding]

Why is this attributed needed? Won't the compiler be synthesizing the attribute if it doesn't exist, as it does with most of the other attributes it needs?

API review has typically rejected such attributes in the past.

[Sergio0694]

Wouldn't that be the same as with eg. [RefSafetyRules]? That's also synthesized by the compiler when missing, but the runtime does bundle it. I always assumed this was mostly to save size (ie. to avoid having multiple internal definitions in each project?), is that not the case? @jaredpar would the compiler team have a preference for this being included in the runtime or not, and/or would there be other considerations to make here to support this point one way or the other? 🙂

[jaredpar]

Why is this attributed needed?

It's necessary to differentiate between in and ref readonly parameters. See speclet details.

Won't the compiler be synthesizing the attribute if it doesn't exist, as it does with most of the other attributes it needs?

Yes. This isn't part of a modreq hence it will fall into our "use if available, synthesize otherwise" bucket. Similar to say nullable attributes.

API review has typically rejected such attributes in the past.

This particular API impacts calling conventions. True for the moment it is C# only but seems completely reasonable for other languages to follow suit in the future (for all the same reasons C# is making the change). Given that it, from my chair at least, it seems like that should be in the System.Runtime.CompilerServices namespace just as IsReadOnlyAttribute is.

The compiler does synthesize attributes into System namespaces today but that is usually only as a down targeting mechanism. Essentially we do it to make working on earlier target frameworks easier with the expectation that over time customers move to new target frameworks, the synthesis is not necessary and eventually fades to very limited use.

If we are never going to put this attribute into the runtime then I feel the S.R.CompilerServices name is wrong. That should be reserved for libraries level components. Thus we should move to Microsoft.CodeAnalysis but that is not ideal for an attribute that impacts calling convention.

Happy to hear others thoughts on this 😄

[tannergooding]

Marked as ready-for-review, but keeping in future until the language feature gets closer to completion.

We can push update the milestone and push forward as/when needed.

[bartonjs]

Video

Looks good as proposed. Marking the issue as blocked, though, since it's intentional that this attribute not be added until .NET 9 (to give the compiler feature one version of prototyping/experimentation/etc that might affect the public attribute).

namespace System.Runtime.CompilerServices
{
    [AttributeUsage(AttributeTargets.Parameter, AllowMultiple = false, Inherited = false)]
    public sealed class RequiresLocationAttribute : Attribute
    {
    }
}

[jaredpar]

This is the first I've heard of us holding back for a release to give the compiler time to experiment. When did that policy come into place? The compiler feature is going to RTM in net8.0 so, at least at the compiler level, this attribute is supported forever. Unsure why runtime is holding back here.

[stephentoub]

This is the first I've heard of us holding back for a release to give the compiler time to experiment. When did that policy come into place? The compiler feature is going to RTM in net8.0 so, at least at the compiler level, this attribute is supported forever. Unsure why runtime is holding back here.

As part of #85612, where we decided to add all such attributes, but recognizing that there's sometimes churn in the compiler and its needs and runtime locks down much earlier (e.g. we're going to fork main for .NET 9 in the very near future). So for attributes that are coming in later in the release and where we won't have runway to react to any changes the compiler decides to make around the attribute, we're better off just waiting until the compiler has RTM'd and then we can add the attribute, which means the next release.

If you can promise there's absolutely nothing about this attribute that will change in any way after today, whether it be name, or attribute usage, or properties, or arguments, or whatever, we could get it in. I just don't see a big advantage to rushing it. This particular attribute is not expected to have widespread use.

[jaredpar]

Ah okay. A deliberate slip between when compiler finishes and when we commit to the shape in runtime makes sense. Based on your comment i would expect that in most cases we ship the attribute and compiler feature in same release. Cases like this where we merge too late would be the rarer case. I read the previous comment as waiting a release being the general policy not the reflection of the specifics of this feature.

[hamarb123]

It would be ideal to add this in .NET 8 imo if at all possible (or do the alternative (my preferred solution), which I added in an edit).
Here's why:

Using it like so: delegate* managed<ref readonly int, void> causes it to be emitted as a modopt, meaning that for binary compatibility purposes, the assembly it's defined in can be important.

Let's say with .NET 8 I write an API like so:

public void X(delegate* managed<ref readonly int, void> ptr);

when .NET 9 happens (assuming we add it to corelib here), I now need to add a type forwarding to maintain binary compatibility, even though this is a fully supported feature in .NET 8 and .NET 9 with C# 12 and 13 respectively.
Even worse is the scenario that I reference an assembly with either a public definition, or with [InternalsVisibleTo], as I now also have to change that assembly to also type forward it to maintain binary compatibility for both.
Even just adding a reference to an assembly with a public definition, or with [InternalsVisibleTo] would be a binary breaking change if I don't set up type forwarding correctly.
It is a similar problem to IsExternalInit, except that roslyn will generate it for you (unlike init, which requires you to manually define it, and thus deal with it yourself).

The best solution imo would be to: let roslyn generate it every time if used as a modifier, ignore any public definition in a dependency, unless in corelib (ie. < .NET 8), otherwise use the definition from corelib (if >= .NET 8) & have roslyn generate a type forward if it's used as a modifier. If it's not used as a modifier (ie. no uses in as a function pointer parameter), then it doesn't matter which definition is used as it's not significant for binary compatibility purposes, so there would be no need to generate type forwards or worry about which version we're referencing in this case.

The main part of this solution is to add it to corelib asap (that way there will be no issues transitioning from .NET 8 to 9, which are the only "supported" .NET versions for the C# version with this feature), I'm happy to add type forwards myself in a library that targets multiple tfms if this is not something we want to burden roslyn with.

Edit (background):
Note that it's important that the following are distinct:

public void X(delegate* managed<in int, void> ptr);
public void X(delegate* managed<ref readonly int, void> ptr);

unless we want it to be meaningless to ever declare a ref readonly parameter on a function pointer, since it would just appear as in to any callers.
This is why I suggested adding a different modifier than what we add for in to ref readonly in the first place, but I originally suggested modopt(InAttribute) iirc, since that wouldn't share this issue. This is probably still the best solution overall imo.

[jjonescz]

The attribute isn't synthesized for function pointers, they require the attribute to be defined manually somewhere (by user or corelib), otherwise it's an error to declare function pointer signature with ref readonly. That's consistent with in, for example.

So there could be a break if user defines the attribute manually, but that seems expected.

Adding some more tests to demonstrate this behavior: dotnet/roslyn#69328

[Sergio0694]

Out of curiosity: if we do need to add the attribute to .NET 8 to avoid breaking changes with function pointers (which makes perfect sense), does it still make sense to have Roslyn synthesize the attribute at all if it doesn't exist already? 🤔

[tannergooding]

The compiler won't synthesize the attribute for the modreq only for cases where its safe. Hence if we didn't provide the type the ability to use ref readonly in function pointers would be restricted.

Users could still define their own public type, but then they'd never be able to remove it or would have to appropriately type forward in the future, etc.

[Sergio0694]

Yeah no I get that, but what I'm saying is, if due to the type being used as a modreq in function pointer we're in fact going to add the attribute in .NET 8, then what's the point of the additional complexity in Roslyn to synthesize the type if not present? Especially given that .NET 8+ would be the only officially supported use case scenario for C# 12 anyway? 🤔

(and of course, folks downlevel can just polyfill and type forward as usual)

[tannergooding]

The support to synthesize the type is general purpose and it is likely more work (and more risk this late in the cycle) to remove that support than it is to simply keep it.

It also allows the feature to be used on older frameworks for the power users that do that (noting its still officially an unsupported scenario; but that's never stopped people in the past).

Typically the types that aren't synthesized have some pretty tough restrictions, such as being required to be defined in "corelib" (that is the assembly that defines object and has no dependencies).