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]: System.Runtime.CompilerServices.CompilerFeatureRequiredAttribute #66167

Closed
333fred opened this issue Mar 3, 2022 · 16 comments
Closed
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime.CompilerServices blocking Marks issues that we want to fast track in order to unblock other important work
Milestone

Comments

@333fred
Copy link
Member

333fred commented Mar 3, 2022

Background and motivation

We've had a few times now in C# language design where the traditional method of requiring that a compiler understand a feature, a modreq, has been insufficient in one of a couple of ways:

  • The context doesn't allow modreqs to be applied, such as a type declaration. This is the case for ref structs.
  • The fact that modreqs affect binary compatibility isn't desired. For example, the required members feature doesn't want to mark constructors with a modreq because that would break binary compat if all required members were later removed from a type, as the modreq is part of the signature.

For these cases, our strategy so far has been to use ObsoleteAttribute with a specific error message the compiler knows to ignore. This is not a great solution, however. If the user puts an obsolete attribute of their own set to warning, it overrides ours. Obsolete methods and types are also perfectly fine to use from an obsolete context. To address these deficiencies going forward, we'd like to add a new tool to our toolbox, a poison attribute of sorts, that allows the compiler to mark a type or other symbol that supports attributes as requiring an understanding of a specific feature. For equal flexibility with modopt vs modreq, I've also added an ability to say that this feature only applies to a single language, if that language wants to allow other languages free access, but prevent older versions of its own compiler from using the API without correct understanding.

The semantics of this attribute are:

  • If Language is null, any compiler that does not understand FeatureName is required to disallow usage of the member attributed with it.
  • If Language is not null, any compiler for that language that does not understand FeatureName is required to disallow usage of the member attributed with it. Compilers for other languages are free to ignore the attribute.

API Proposal

namespace System.Runtime.CompilerServices
{
    [AttributeUsage(AttributeTargets.All, AllowMultiple = true, Inherited = false)]
    public sealed class CompilerFeatureRequiredAttribute : Attribute
    {
        public CompilerFeatureRequiredAttribute(string featureName)
        {
            FeatureName = featureName;
        }
        public string FeatureName { get; }
        public string? Language { get; init; }
    }
}

API Usage

ref struct S {}

will be emitted as

[CompilerFeatureRequired("ref-struct")]
struct S {}

Alternative Designs

We might also consider putting a list of features into the runtime, but that runs the risk of the runtime effectively becoming an arbiter of language features: if some third-party .NET language wanted to add their own strings, what would the process for that be?

Risks

It is important to note that this enforcement mechanism will only work going forward. Older compilers are going to have no idea that this is a thing and not block access. That being said, if we don't add something like this now, we'll never get to a point where we can solely rely on this attribute as the enforcement mechanism.

@333fred 333fred added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Mar 3, 2022
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Runtime.CompilerServices untriaged New issue has not been triaged by the area owner labels Mar 3, 2022
@ghost
Copy link

ghost commented Mar 3, 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

We've had a few times now in C# language design where the traditional method of requiring that a compiler understand a feature, a modreq, has been insufficient in one of a couple of ways:

  • The context doesn't allow modreqs to be applied, such as a type declaration. This is the case for ref structs.
  • The fact that modreqs affect binary compatibility isn't desired. For example, the required members feature doesn't want to mark constructors with a modreq because that would break binary compat if all required members were later removed from a type, as the modreq is part of the signature.

For these cases, our strategy so far has been to use ObsoleteAttribute with a specific error message the compiler knows to ignore. This is not a great solution, however. If the user puts an obsolete attribute of their own set to warning, it overrides ours. Obsolete methods and types are also perfectly fine to use from an obsolete context. To address these deficiencies going forward, we'd like to add a new tool to our toolbox, a poison attribute of sorts, that allows the compiler to mark a type or other symbol that supports attributes as requiring an understanding of a specific feature. For equal flexibility with modopt vs modreq, I've also added an ability to say that this feature only applies to a single language, if that language wants to allow other languages free access, but prevent older versions of its own compiler from using the API without correct understanding.

API Proposal

namespace System.Runtime.CompilerServices
{
    [AttributeUsage(AttributeTargets.All, AllowMultiple = true, Inherited = false)]
    public sealed class CompilerFeatureRequiredAttribute : Attribute
    {
        public CompilerFeatureRequiredAttribute(string featureName)
        {
            FeatureName = featureName;
        }
        public string FeatureName { get; }
        public string Language { get; init; }
    }
}

API Usage

ref struct S {}

will be emitted as

[CompilerFeatureRequired("ref-struct")]
struct S {}

Alternative Designs

We might also consider putting a list of features into the runtime, but that runs the risk of the runtime effectively becoming an arbiter of language features: if some third-party .NET language wanted to add their own strings, what would the process for that be?

Risks

No response

Author: 333fred
Assignees: -
Labels:

api-suggestion, area-System.Runtime.CompilerServices, untriaged

Milestone: -

@333fred
Copy link
Member Author

333fred commented Mar 3, 2022

/cc @stephentoub

@tannergooding
Copy link
Member

What's the plan to ensure that new/existing .NET languages are aware of this attribute and the requirements around understanding/respecting it? What about older compilers (say the C# native compiler) that have no awareness of this attribute?

IIRC, that's one reason why Obsolete was used for Span, to avoid the issue where some languages were known to ignore modreq and to simultaneously ensure that older compilers were also "properly poisoned".

@MadsTorgersen
Copy link

Looks great! You might want to call out the fact that this only blocks the feature back to the version of the language that first knows about this attribute; e.g. C# 11. So we might need to allow some time to pass before we rely solely on this rather than Obsolete.

@tannergooding
Copy link
Member

The compiler could, I guess, add both Obsolete and CompilerFeatureRequiredAttribute to try and cover that scenario (ensure older compilers are aware and that user specified Obsolete can't override compiler specified Obsolete)?

@333fred
Copy link
Member Author

333fred commented Mar 3, 2022

What's the plan to ensure that new/existing .NET languages are aware of this attribute and the requirements around understanding/respecting it?

We will work with our partners to make sure that they are aware, and I plan to reach out to third parties I know of (such as peachpie/cobol.net).

What about older compilers (say the C# native compiler) that have no awareness of this attribute?

We will continue to be using both Obsolete and CompilerFeatureRequired, but going forward we want to have a mechanism that doesn't have the obsolete drawbacks.

IIRC, that's one reason why Obsolete was used for Span, to avoid the issue where some languages were known to ignore modreq and to simultaneously ensure that older compilers were also "properly poisoned".

Right, and in LDM we acknowledged that, and also acknowledged that we should have done this at that time as well. Better late than never 🙂

@RikkiGibson
Copy link
Contributor

maybe a dumb question: is it possible to make old compilers ignore or disallow usage of declarations which have this attribute by putting a modreq on the constructor of CompilerFeatureRequiredAttribute itself?

@jcouv
Copy link
Member

jcouv commented Mar 4, 2022

  1. Could you provide an example of error message when the compiler disallows using a marked member? Just having a "feature name" may not be sufficient information to present to the user.
  2. What the scenario that would involve Language being non-null?
    If we decide to keep Language: Do we have a precedent for an attributes using properties and meant for compiler usage? I would just make it an optional parameter of the constructor or add a second constructor.
  3. Adding a list of features in the runtime seems a good idea and will allow providing a pointer to the docs. This doesn't mean that the attribute would reject any other values.

@333fred
Copy link
Member Author

333fred commented Mar 4, 2022

Could you provide an example of error message when the compiler disallows using a marked member? Just having a "feature name" may not be sufficient information to present to the user.

Exactly the same as with modreqs today, it will just be marked as unsupported by the language.

What the scenario that would involve Language being non-null?

I did not have a specific scenario in mind, I simply wanted to make this option as flexible as modopt vs modreq, where a language could use this to communicate just between itself, and not to all other .NET languages.

If we decide to keep Language: Do we have a precedent for an attributes using properties and meant for compiler usage? I would just make it an optional parameter of the constructor or add a second constructor.

Not AFAIK. The general pattern in the BCL (that I know of) is to not used default parameters for attributes, and instead use properties. I don't see a particular issue with doing this.

Adding a list of features in the runtime seems a good idea and will allow providing a pointer to the docs. This doesn't mean that the attribute would reject any other values.

Sure, I just wanted to point out the potential risk of putting such things in the BCL.

@stephentoub stephentoub added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation untriaged New issue has not been triaged by the area owner labels Mar 8, 2022
@stephentoub stephentoub added this to the 7.0.0 milestone Mar 8, 2022
@terrajobst
Copy link
Member

terrajobst commented Mar 8, 2022

Video

  • Makes sense
  • It's not clear why the Language property exists; it seems it's geared towards expressing optionality. But if other languages later also support the feature, would they emit the attribute using their language or would the emit with the name of the first language?
namespace System.Runtime.CompilerServices
{
    [AttributeUsage(AttributeTargets.All, AllowMultiple = true, Inherited = false)]
    public sealed class CompilerFeatureRequiredAttribute : Attribute
    {
        public CompilerFeatureRequiredAttribute(string featureName)
        {
            FeatureName = featureName;
        }
        public string FeatureName { get; }
        public string? Language { get; init; }
    }
}

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented api-needs-work API needs work before it is approved, it is NOT ready for implementation and removed api-ready-for-review API is ready for review, it is NOT ready for implementation api-approved API was approved in API review, it can be implemented labels Mar 8, 2022
@333fred
Copy link
Member Author

333fred commented Mar 8, 2022

But if other languages later also support the feature, would they emit the attribute using their language or would the emit with the name of the first language?

Up to the implementation. If we don't think the optionality is relevant, we can strip it from the proposal. We could also change it to a boolean Optional flag if we don't think the language is the right way to expression optionality per-language.

@333fred 333fred added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-needs-work API needs work before it is approved, it is NOT ready for implementation labels Mar 8, 2022
@333fred 333fred added the blocking Marks issues that we want to fast track in order to unblock other important work label Mar 25, 2022
@333fred
Copy link
Member Author

333fred commented Mar 25, 2022

I've marked this blocking so we get to it soon. I'm getting to the point with required members that I will need a finalized design for this attribute.

@terrajobst
Copy link
Member

terrajobst commented Apr 12, 2022

Video

  • The Language property is meant to express optionality, we should just make it a bool that is false by default
namespace System.Runtime.CompilerServices
{
    [AttributeUsage(AttributeTargets.All, AllowMultiple = true, Inherited = false)]
    public sealed class CompilerFeatureRequiredAttribute : Attribute
    {
        public CompilerFeatureRequiredAttribute(string featureName)
        {
            FeatureName = featureName;
        }

        public string FeatureName { get; }

        public bool IsOptional { get; init; }

        public const string RefStructs = nameof(RefStructs);

        public const string RequiredMembers = nameof(RequiredMembers);
    }
}

@terrajobst terrajobst 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 Apr 12, 2022
333fred added a commit that referenced this issue Apr 12, 2022
@RikkiGibson
Copy link
Contributor

CompilerFeatureRequired with an IsOptional property seems like a strange combination of names.

@terrajobst
Copy link
Member

terrajobst commented Apr 12, 2022

CompilerFeatureRequired with an IsOptional property seems like a strange combination of names.

This was brought up and discussed. We didn't feel strongly enough to address this because in order to make this clean, we'd basically need three types:

namespace System.Runtime.CompilerServices;

[AttributeUsage(AttributeTargets.All, AllowMultiple = true, Inherited = false)]
public sealed class CompilerFeatureRequiredAttribute : Attribute
{
    public CompilerFeatureRequiredAttribute(string featureName);
    public string FeatureName { get; }
}

[AttributeUsage(AttributeTargets.All, AllowMultiple = true, Inherited = false)]
public sealed class CompilerFeatureOptionalAttribute : Attribute
{
    public CompilerFeatureOptionalAttribute(string featureName);
    public string FeatureName { get; }
}

public static class CompilerFeatures
{
    public const string RefStructs = nameof(RefStructs);
    public const string RequiredMembers = nameof(RequiredMembers);
}

Considering that these annotations aren't written nor read by humans, it felt like over engineering.

@svick
Copy link
Contributor

svick commented Apr 13, 2022

Would it help if the type was renamed to something like CompilerFeatureAttribute?

@ghost ghost locked as resolved and limited conversation to collaborators May 14, 2022
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 blocking Marks issues that we want to fast track in order to unblock other important work
Projects
None yet
Development

No branches or pull requests

8 participants