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

[API Proposal]: Add System.Runtime.CompilerServices.RefSafetyRulesAttribute #76032

Closed
cston opened this issue Sep 22, 2022 · 16 comments · Fixed by #80379
Closed

[API Proposal]: Add System.Runtime.CompilerServices.RefSafetyRulesAttribute #76032

cston opened this issue Sep 22, 2022 · 16 comments · Fixed by #80379
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime.CompilerServices
Milestone

Comments

@cston
Copy link
Member

cston commented Sep 22, 2022

Background and motivation

The C# compiler emits a [module: RefSafetyRules(11)] attribute to modules compiled with -langversion:11 or above, or compiled with .NET 7 or above, to indicate that the module was compiled with C#11 ref safety rules.

The RefSafetyRulesAttribute type definition should be added to the BCL to avoid having the compiler emit the attribute definition to each module.

See RefSafetyRulesAttribute.

API Proposal

namespace System.Runtime.CompilerServices
{
    [AttributeUsage(AttributeTargets.Module, AllowMultiple = false, Inherited = false)]
    public sealed class RefSafetyRulesAttribute : Attribute
    {
        public RefSafetyRulesAttribute(int version) { Version = version; }
        public int Version;
    }
}

API Usage

The RefSafetyRulesAttribute type is for compiler use only - it is not permitted in source.

Alternative Designs

No response

Risks

No response

@cston cston added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Sep 22, 2022
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Sep 22, 2022
@ghost
Copy link

ghost commented Sep 22, 2022

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

The C# compiler emits a [module: RefSafetyRules(11)] attribute to modules compiled with -langversion:11 or above, or compiled with .NET 7 or above, to indicate that the module was compiled with C#11 ref safety rules.

The RefSafetyRulesAttribute type definition should be added to the BCL to avoid having the compiler emit the attribute definition to each module.

See RefSafetyRulesAttribute.

API Proposal

namespace System.Runtime.CompilerServices
{
    [AttributeUsage(AttributeTargets.Module, AllowMultiple = false, Inherited = false)]
    public sealed class RefSafetyRulesAttribute : Attribute
    {
        public RefSafetyRulesAttribute(int version) { Version = version; }
        public int Version;
    }
}

API Usage

The RefSafetyRulesAttribute type is for compiler use only - it is not permitted in source.

Alternative Designs

No response

Risks

No response

Author: cston
Assignees: -
Labels:

api-suggestion, area-System.Runtime.CompilerServices

Milestone: -

@cston cston changed the title [API Proposal]: [API Proposal]: Add System.Runtime.CompilerServices.RefSafetyRulesAttribute Sep 22, 2022
@cston
Copy link
Member Author

cston commented Sep 22, 2022

cc @jaredpar, @AaronRobinsonMSFT

@AaronRobinsonMSFT
Copy link
Member

@cston This looks like a .NET 7 need, correct?

@AaronRobinsonMSFT AaronRobinsonMSFT added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed untriaged New issue has not been triaged by the area owner api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Sep 22, 2022
@cston
Copy link
Member Author

cston commented Sep 22, 2022

@AaronRobinsonMSFT, this can wait until .NET 8.

@stephentoub
Copy link
Member

There are other attributes the compiler will emit as internal into an assembly and that we haven't exposed from corelib, e.g. NullableAttribute, NullableContextAttribute, EmbeddedAttribute, maybe others? They are similarly not meant to be uttered in source. Are we interested in bringing them all into corelib as public? I'm wondering if/how RefSafetyRules is special.

@AaronRobinsonMSFT AaronRobinsonMSFT added this to the 8.0.0 milestone Sep 22, 2022
@jaredpar
Copy link
Member

I'm wondering if/how RefSafetyRules is special

The other ones require some level of opt in. For instance you aren't going to see any of the Nullable* ones unless you opt into nullability in some way. It's pay for play in that way.

The ref safety rules one though we include on almost all assemblies. There isn't as much of a trigger to opt into ref rules. Originally we conditioned the rule on a signature having a out, ref or ref struct parameter or return. That didn't sit 100% right with us. There was worry about missing checks, future proofing, etc ... and eventually felt it was better to just emit always. At that point it felt like it was better to have in corelib.

Also if there was a better trigger for when to emit this, be willing to listen to that option 😄

@vitek-karas
Copy link
Member

Is the RefSafetyRulesAttribute used for anything at runtime? If not we should probably add it to the list of attributes aggressive trimming removes by default - otherwise this adds two types into every assembly (RefSafetyRulesAttribute and EmbeddedAttribute), yes they are small... but still.

/cc @marek-safar

@AaronRobinsonMSFT
Copy link
Member

No, it is not. It can be removed during trimming of a final application. It must be kept on assemblies that will be used during build or else things could be very very very ... bad.

@bartonjs
Copy link
Member

bartonjs commented Nov 15, 2022

Video

  • We changed the Version field to a Version get-only property
  • Otherwise, looks good as proposed.
namespace System.Runtime.CompilerServices
{
    [AttributeUsage(AttributeTargets.Module, AllowMultiple = false, Inherited = false)]
    public sealed class RefSafetyRulesAttribute : Attribute
    {
        public RefSafetyRulesAttribute(int version) { Version = version; }
        public int Version { get; }
    }
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Nov 15, 2022
@deeprobin
Copy link
Contributor

@AaronRobinsonMSFT
Does this API require a special handling in coreclr or is it just the implementation of this attribute?

@stephentoub
Copy link
Member

It's just the attribute.

@AaronRobinsonMSFT
Copy link
Member

It's just the attribute.

Right. This issue is just about adding it to the product and updating the appropriate ref assembly. We typically add a test too, for completeness and verifying it works with reflection.

@stephentoub
Copy link
Member

We typically add a test too, for completeness and verifying it works with reflection.

In this case, we'll also want to validate that the compiler recognizes this attribute and stops emitting into all of our assemblies its own internal copy.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jan 9, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jan 10, 2023
@hamarb123
Copy link
Contributor

Out of curiosity, is there any reason to not do this for the other generated attribute types that always seem to be generated?
image

@Zastai
Copy link
Contributor

Zastai commented Feb 9, 2023

Out of curiosity, is there any reason to not do this for the other generated attribute types that always seem to be generated? image

As mentioned near the top of this issue, the nullability ones are opt-in (usually by having <Nullable>enable</Nullable> in the project). I think EmbeddedAttribute only gets emitted when used, i.e. when any other embedded attribute is emitted.

@hamarb123
Copy link
Contributor

As mentioned near the top of this issue, the nullability ones are opt-in (usually by having <Nullable>enable</Nullable> in the project). I think EmbeddedAttribute only gets emitted when used, i.e. when any other embedded attribute is emitted.

Sure, but I don't think there is any cost with declaring them in the BCL, because nullable annotations are enabled by default anyway, and System.Runtime/System.Private.CoreLib would almost definitely already have these declared already (just not exposed for external consumption) since they support nullable annotations. There is a reward, which is not having to declare them in every single other BCL library, and every other library that wants to use nullable annotations (which I remind you, is the default).

Also, since nullable annotations is the default for any new project, it's definitely not opt-in, but instead opt-out.

@ghost ghost locked as resolved and limited conversation to collaborators Mar 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime.CompilerServices
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants