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 ref field feature support to RuntimeFeature class #64165

Closed
Tracked by #63768
AaronRobinsonMSFT opened this issue Jan 23, 2022 · 8 comments · Fixed by #64167
Closed
Tracked by #63768

API Proposal: Add ref field feature support to RuntimeFeature class #64165

AaronRobinsonMSFT opened this issue Jan 23, 2022 · 8 comments · Fixed by #64167
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime.CompilerServices
Milestone

Comments

@AaronRobinsonMSFT
Copy link
Member

AaronRobinsonMSFT commented Jan 23, 2022

Background and motivation

This is desired to help Roslyn perform down-level targeting for the ref field support added in #63768.

API Proposal

Implementation PR at #64167.

namespace System.Runtime.CompilerServices
{
    public static partial class RuntimeFeature
    {
+        /// <summary>
+        /// Represents a runtime feature where types can define ref fields.
+        /// </summary>
+        public const string ByRefFields = nameof(ByRefFields);
    }
}

API Usage

Used by Roslyn to detect if a runtime supports ref fields.

Alternative Designs

This is the typical way to indicate a feature to Roslyn.

Risks

None.

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Jan 23, 2022
@jkotas
Copy link
Member

jkotas commented Jan 23, 2022

I do not think we need a feature flag for this.

@AaronRobinsonMSFT
Copy link
Member Author

AaronRobinsonMSFT commented Jan 23, 2022

This was a request by the Roslyn team for down-level targeting.

/cc @jaredpar @davidwrighton

@AaronRobinsonMSFT AaronRobinsonMSFT added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Runtime.CompilerServices and removed untriaged New issue has not been triaged by the area owner labels Jan 23, 2022
@AaronRobinsonMSFT AaronRobinsonMSFT added this to the 7.0.0 milestone Jan 23, 2022
@ghost
Copy link

ghost commented Jan 23, 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

This is desired to help Roslyn perform down-level targeting for the ref field support added in #63768.

Proposed API

namespace System.Runtime.CompilerServices
{
    public static partial class RuntimeFeature
    {
+.       public const string RefFields = nameof(RefFields);
    }
}
Author: AaronRobinsonMSFT
Assignees: -
Labels:

api-suggestion, area-System.Runtime.CompilerServices

Milestone: -

@AaronRobinsonMSFT AaronRobinsonMSFT changed the title Add ref field feature flag in SPCL. API Proposal: Add ref field feature flag in SPCL. Jan 23, 2022
@AaronRobinsonMSFT
Copy link
Member Author

AaronRobinsonMSFT commented Jan 23, 2022

@jkotas I will leave this as "api-suggestion" until we get consensus on whether this is needed or not. I will however push a draft PR with the change in case it is needed.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jan 23, 2022
@jkotas
Copy link
Member

jkotas commented Jan 23, 2022

I though you meant FEATURE_... ifdef. I agree we need a flag for this under RuntimeFeatures. We should run it through API review before merging the PR.

RefFields

We have a clash of runtime and C# names here.

We have Type.IsByRef, Type.IsByRefLike and IsByRefLikeAttribute in the runtime vs. ref and ref structs in C#.

Should this flag by called ByRefFields for consistency with what we have already?

@AaronRobinsonMSFT AaronRobinsonMSFT added api-ready-for-review API is ready for review, it is NOT ready for implementation blocking Marks issues that we want to fast track in order to unblock other important work and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Jan 23, 2022
@AaronRobinsonMSFT AaronRobinsonMSFT changed the title API Proposal: Add ref field feature flag in SPCL. API Proposal: Add ref field feature support to RuntimeFeature class Jan 23, 2022
@AaronRobinsonMSFT AaronRobinsonMSFT self-assigned this Jan 23, 2022
@jaredpar
Copy link
Member

@cston, @RikkiGibson FYI

@terrajobst
Copy link
Member

terrajobst commented Jan 25, 2022

Video

  • Looks good as proposed
  • We should make sure that other runtimes, such as Unity, that features listed in the RuntimeFeature are required
    • Practically, if another runtime pulls in our library code they will likely find out, given that we usually use those features
namespace System.Runtime.CompilerServices
{
    public static partial class RuntimeFeature
    {
        public const string ByRefFields = nameof(ByRefFields);
    }
}

@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 Jan 25, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jan 25, 2022
@AaronRobinsonMSFT AaronRobinsonMSFT removed the blocking Marks issues that we want to fast track in order to unblock other important work label Jan 25, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Feb 25, 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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants