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

Expose System.Runtime.CompilerServices.SkipLocalsInitAttribute #25850

Closed
tannergooding opened this issue Apr 11, 2018 · 15 comments
Closed

Expose System.Runtime.CompilerServices.SkipLocalsInitAttribute #25850

tannergooding opened this issue Apr 11, 2018 · 15 comments
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime.CompilerServices
Milestone

Comments

@tannergooding
Copy link
Member

Rationale

C# is exposing new functionality which allows you to drop the init flag from a method's .local directive (see dotnet/roslyn#25780 for more details).

It would be beneficial if CoreFX/NetStandard exposed this standard type so that users are not required to always manually define this type themselves.

Proposed API

namespace System.Runtime.CompilerServices
{
    [AttributeUsage(AttributeTargets.Module | AttributeTargets.Class | AttributeTargets.Struct | AttributeTargets.Constructor | AttributeTargets.Method | AttributeTargets.Property | AttributeTargets.Event, Inherited = false)]
    public sealed class SkipLocalsInitAttribute : Attribute
    {
        public SkipLocalsInitAttribute()
        {
        }
    }
}
@tannergooding
Copy link
Member Author

FYI. @jcouv, @agocke, @t-camaia

@terrajobst
Copy link
Member

terrajobst commented Apr 17, 2018

Video

Questions:

  • Will there be a keyword in the language too or will developers use the attribute?
  • Presumably people will be able to define the attribute in their own code and get the same behavior?
  • Does it make sense to be applied to fields? Presumably no, because what matters is the constructor in case initializers are present?
  • Do you plan to support override behavior, i.e. apply to assembly/module/type to turn on/off and then override on the a per member basis?
namespace System.Runtime.CompilerServices
{
    [AttributeUsage(AttributeUsage.All, Inherited = false)]
    public sealed class SkipLocalsInitAttribute : Attribute
    {
        public SkipLocalsInitAttribute(bool isEnabled);
        public bool IsEnabled { get; }
    }
}

@tannergooding
Copy link
Member Author

Will there be a keyword in the language too or will developers use the attribute

This is a compiler, not a language, feature. So i doubt any language support will be added (see dotnet/csharplang#1223).

Presumably people will be able to define the attribute in their own code and get the same behavior

Yes, that is the workaround today.

Does it make sense to be applied to fields? Presumably no, because what matters is the constructor in case initializers are present?

Right, it should only matter for Methods (including constructors/properties/etc) and things which can contain methods (Types, Modules, Assemblies).

Do you plan to support override behavior, i.e. apply to assembly/module/type to turn on/off and then override on the a per member basis?

This seems like a reasonable thing to support. @jcouv ?

@jcouv
Copy link
Member

jcouv commented Apr 17, 2018

Do you plan to support override behavior, i.e. apply to assembly/module/type to turn on/off and then override on the a per member basis?

Talked to @agocke and confirmed that we do not intend allowing the feature to be turned off per-member when applied to a container.
We expect the attribute to be applied on specific members, rather than on containers (that's allowed because it was strange to disallow, but we wouldn't recommend using on containers).

Does it make sense to be applied to fields? Presumably, no...

You're correct. The attribute should be applied to constructors in such case.
Per speclet: Permitted and recognized attribute targets are: Method, Property, Module, Class, Struct, Interface, Constructor.

Inherited = false

Also confirmed with him that Inherited = false is correct. The behavior (skipping init) should only kick in where the attribute is explicitly applied, not inherited by sub-types or overrides.

Will there be a keyword in the language too or will developers use the attribute

No. Per speclet: This is explicitly a compiler feature and not a language feature :-)

Tagging @VSadov in case anything to add.

@jcouv
Copy link
Member

jcouv commented Apr 17, 2018

@tannergooding AttributeUsage.All is incorrect.

@VSadov
Copy link
Member

VSadov commented Apr 17, 2018

Yes. All the above is correct. In particular - the "override/on/off" behavior was discussed at least two times and both times dropped due to being unnecessary complication for too little gain.

It is expected that the attribute will be used either on the whole module (most likely) or on select leaf methods (distant second).

It is highly unlikely that user will want an elaborate system of mix and match behavior. It is still possible by attributing every piece, if needed, but we did not think it was worth the trouble to make this scenario easy.

@VSadov
Copy link
Member

VSadov commented Apr 17, 2018

Yes, adding an attribute to the S.R.C that is shipping would be a good idea.

@tannergooding
Copy link
Member Author

tannergooding commented Apr 19, 2018

AttributeUsage.All is incorrect.

@jcouv, right. Hence the comment // TODO: Update AttributeUsage based on the compiler spec

@tannergooding
Copy link
Member Author

This is linked to the compiler feature: dotnet/csharplang#1738

@tannergooding
Copy link
Member Author

tannergooding commented Jul 27, 2018

Updated the OP with the correct AttributeTargets values:

  • Module
  • Class
  • Struct
  • Constructor
  • Method
  • Property
  • Event

Assembly isn't included because you can have multiple modules from different sources

Enum and Interface aren't included because you can't implement methods on them today. We will need to revisit if such functionality becomes possible (both are currently tracked as language proposals)

Field, Parameter, Delegate, ReturnValue, and GenericParameter don't make sense as valid targets (they can't have any code that would end up with a localsinit flag)

@stephentoub
Copy link
Member

@tannergooding, what's the status of this? Is it still for 3.0? Is the compiler feature moving along accordingly?

@tannergooding
Copy link
Member Author

AFAIK, the feature is basically implemented in https://github.com/dotnet/roslyn/tree/features/localsinit, but it requires additional review and merging back into master.

@jaredpar or @agocke might be able to comment more about timeline

@stephentoub
Copy link
Member

@agocke, it looks like the feature was merged into Roslyn, correct? If so, we should go ahead with exposing it and using it in coreclr/corefx.

@tannergooding
Copy link
Member Author

I believe it is still in the features/localsinit branch; as that shows it is still 14 commits ahead, 1210 commits behind master.

@agocke
Copy link
Member

agocke commented Oct 7, 2019

Yup we have to do a feature review before we merge it. Hope to get that done in the next week or so.

@jkotas jkotas closed this as completed in 4359ebf Dec 15, 2019
@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 5.0 milestone Jan 31, 2020
@dotnet dotnet locked as resolved and limited conversation to collaborators Dec 17, 2020
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

No branches or pull requests

7 participants