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

System.Runtime.CompilerServices.RuntimeFeature.VirtualStaticsInInterfaces #49905

Closed
jcouv opened this issue Mar 19, 2021 · 10 comments
Closed

System.Runtime.CompilerServices.RuntimeFeature.VirtualStaticsInInterfaces #49905

jcouv opened this issue Mar 19, 2021 · 10 comments
Assignees
Labels
api-approved API was approved in API review, it can be implemented api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Runtime.CompilerServices blocking Marks issues that we want to fast track in order to unblock other important work
Milestone

Comments

@jcouv
Copy link
Member

jcouv commented Mar 19, 2021

The runtime should expose the following feature flag for runtimes that support static abstract members in interfaces.

namespace System.Runtime.CompilerServices
{
    public static class RuntimeFeature
    {
        public const string VirtualStaticsInInterfaces = nameof(VirtualStaticsInInterfaces);
    }
}

The implementation of static bool IsSupported(string feature) should be updated accordingly.

FYI @davidwrighton @AlekseyTs

Relates to language feature: dotnet/csharplang#4436

Per discussion with the overall feature crew, we'd like this change in .NET 6 preview 5, to line up with runtime and C# compiler work.

@jcouv jcouv added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Mar 19, 2021
@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 19, 2021
@stephentoub
Copy link
Member

I assume this gets attributed as [Preview] as well?

@davidwrighton
Copy link
Member

@jcouv I agree with @stephentoub that this constant should be marked with the preview attribute. Do we think that the IsSupported function result should also be conditional on the runtime preview flag being set as well?

@jcouv
Copy link
Member Author

jcouv commented Mar 20, 2021

I think marking it as [Preview] (or whatever attribute we end up using to gate preview features) is okay, but would have minimal benefit as the compiler is the main consumer of RuntimeFeature.

It is also okay to have the IsSupported logic conditional on the runtime preview flag, but the difference is only observable if you use "preview" language version without setting the runtime preview flag. In such a scenario the user is aware of being outside of the supported boundaries.

Neither seems needed for preview 5. I'm not sure what's the best way to track such a change (once the design of preview flag is done). I'll add a note in roslyn work items for now.

@marek-safar
Copy link
Contributor

but would have minimal benefit as the compiler is the main consumer of RuntimeFeature.

Why are we adding this API? The compiler will use the string literal because it does not target net6, right?

@huoyaoyuan
Copy link
Member

Why are we adding this API? The compiler will use the string literal because it does not target net6, right?

Like DefaultImplementationsOfInterfaces, the compiler detects the existence of the property to decide feature availability.

@davidwrighton davidwrighton added the api-ready-for-review API is ready for review, it is NOT ready for implementation label May 11, 2021
@davidwrighton davidwrighton self-assigned this May 11, 2021
@davidwrighton davidwrighton added this to the 6.0.0 milestone May 11, 2021
@davidwrighton davidwrighton added the blocking Marks issues that we want to fast track in order to unblock other important work label May 11, 2021
@bartonjs
Copy link
Member

Looks good as-is (with the RequiresPreviewFeatures attribute)

namespace System.Runtime.CompilerServices
{
    public static class RuntimeFeature
    {
        [RequiresPreviewFeatures]
        public const string VirtualStaticsInInterfaces = nameof(VirtualStaticsInInterfaces);
    }
}

@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 May 13, 2021
@tannergooding
Copy link
Member

Notably, as raised by @333fred, this may need to be changed to AbstractStaticsInInterfaces if we believe default implementation support will not ship in the runtime for v1.

The library team has stated that default implementation support will be eventually required (at least by v2) so we can add members to existing interfaces such as INumber (otherwise we are forced to create INumber2 and friends, which is not desired).

CC. @davidwrighton and @AlekseyTs to determine when that support will go in and whether that has any impact for what we use in .NET 6 vs when the feature ships as stable (v1) vs when we need to version the interfaces (v2)

@joperezr joperezr removed the untriaged New issue has not been triaged by the area owner label May 13, 2021
@davidwrighton
Copy link
Member

I think its highly likely that default implementation support will ship in the runtime, even if it doesn't make the language.

@AlekseyTs
Copy link
Contributor

@davidwrighton

I think its highly likely that default implementation support will ship in the runtime

I am not sure how runtime would be able to do that since specification doesn't define a way to describe that in metadata. I believe according to specification, virtual statics in interfaces can be only abstract, which doesn't allow them to have an implementation.

@davidwrighton
Copy link
Member

davidwrighton commented May 17, 2021

My expectation is that we'll take an update to our spec in time for Preview 6. There have been enough consistent ask from @tannergooding for this during the feature, that I'd like to take a stab at the design so that the runtime work in .NET 7 to move this from a preview feature is performance focused and not behavior focused.

@ghost ghost locked as resolved and limited conversation to collaborators Jun 19, 2021
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 api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Runtime.CompilerServices blocking Marks issues that we want to fast track in order to unblock other important work
Projects
No open projects
Development

No branches or pull requests

9 participants