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

New API: InitOnlyAttribute #34978

Closed
jcouv opened this issue Apr 15, 2020 · 28 comments · Fixed by #37763
Closed

New API: InitOnlyAttribute #34978

jcouv opened this issue Apr 15, 2020 · 28 comments · Fixed by #37763
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime.CompilerServices

Comments

@jcouv
Copy link
Member

jcouv commented Apr 15, 2020

Note: the proposed API was modified significantly during API design review.

See notes below: #34978 (comment)

The type will be used as a modreq on the return type of the set accessor:
public modreq(typeof(IsExternalInit) void set_InitOnlyProperty(string value) { ... }


Out-of-date:

As part of the C# 9 records feature, we want to allow some properties to be settable during "initialization". Initialization means construction and also object creation. So we would allow new C() { InitOnlyProperty = "hello" }.

To declare such a property, we'll allow the following syntax:

public string InitOnlyProperty
{
   get { ... }
   init { ... } // this is an init-only setter
}

We will emit this property as:

// pseudo code
private readonly string backingField;
[InitOnly]
public void InitOnlyProperty_setAccessor(modreq(typeof(InitOnlyAttribute)) string value) { ... }

The attribute is used on the set accessor and in a modreq.
There are two potential expansion of the feature: init-only fields and init-only methods. We can discuss whether it is better to include those now, or add later.

So we are proposing to add the following API:

namespace System.Runtime.CompilerServices
{
    [AttributeUsage(AttributeTargets.Field | AttributeTargets.Method, AllowMultiple = false, Inherited = false)]
    public sealed class InitOnlyAttribute : Attribute
    {
    }
}

Tagging @jaredpar @agocke FYI

Updates:

  • removed AttributeTargets.Method, added AttributeTargets.Field (jcouv)
  • removed [InitOnly] attribute on backing field of auto-property (jcouv)
  • added AttributeTargets.Method back, with explanation (jcouv)
  • added [InitOnly] on set accessor (jcouv)
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-Meta untriaged New issue has not been triaged by the area owner labels Apr 15, 2020
@Entomy
Copy link

Entomy commented Apr 15, 2020

I like this idea. I've got a situation where I'm dependency injecting a buffer into a specialized stream wrapper, which uses inversion of control. Without init-only, the interfaces have to keep a public getter/setter for the underlying stream, which isn't a huge issue but certainly causes a leaky abstraction that this would resolve.

@terrajobst terrajobst added api-ready-for-review area-System.Runtime.CompilerServices blocking Marks issues that we want to fast track in order to unblock other important work and removed area-Meta untriaged New issue has not been triaged by the area owner labels Apr 15, 2020
@jkotas
Copy link
Member

jkotas commented Apr 15, 2020

The attribute is used by the compiler for two purposes:

  • on the backing field

The proposed API you got allows it on AttributeTargets.Method only. Which one is correct?

Note that fields have FieldAttributes.InitOnly flag already. Having this attribute on fields is going to be confusing.

@jcouv
Copy link
Member Author

jcouv commented Apr 15, 2020

Thanks. I fixed the proposed API to have AttributeTargets.Field.
Whether we'll want to introduce the notion of init methods (init void Initialize() { ... }) is an open question for LDM.
Yes, the name is unfortunate, as IL already has a flag called initonly used for emitting C#'s readonly.

@jkotas
Copy link
Member

jkotas commented Apr 15, 2020

Why do you need the attribute for fields? Why is not the existing FieldAttributes.InitOnly sufficient for fields?

@ltrzesniewski
Copy link
Contributor

Is the backing field really supposed to be readonly?

ECMA-335 doesn't seem to allow this (emphasis mine):

initonly marks fields which are constant after they are initialized. These fields shall only be mutated inside a constructor. If the field is a static field, then it shall be mutated only inside the type initializer of the type in which it was declared. If it is an instance field, then it shall be mutated only in one of the instance constructors of the type in which it was defined. It shall not be mutated in any other method or in any other constructor, including constructors of derived classes.

@Joe4evr
Copy link
Contributor

Joe4evr commented Apr 15, 2020

ECMA-335 doesn't seem to allow this

You're not wrong, but the runtime doesn't fully enforce that restriction (use of the word "shall" means it's looser than "must"). In the recent .NET Language Standup it's said that they intend to exploit this fact that the runtime allows changing readonly instance fields without a problem in order to implement this feature.

@jkotas
Copy link
Member

jkotas commented Apr 15, 2020

Right. Also, the ECMA-335 spec is not set in stone. It has been relaxed and augmented for number of other new runtime features. (We do not have a way to publish these edits today - but I hope that is going to change.)

@GrabYourPitchforks
Copy link
Member

I'd be very nervous with any feature taking a dependency on initonly instance fields being mutable. That might tie our hands at making future optimizations. Recall that the core runtime forbids setting initonly static fields outside the cctor, even though the ECMA spec uses "shall" instead of "must".

@jkotas
Copy link
Member

jkotas commented Apr 15, 2020

I'd be very nervous with any feature taking a dependency on initonly instance fields being mutable.

This makes the initonly instance fields mutable in a specific context only (only in methods tagged this attribute). I do not see why it would tie our hands at making future optimizations.

@GrabYourPitchforks
Copy link
Member

Was the plan not also to allow this syntax? Did I misunderstand?

class MyClass
{
    public init readonly string PublicField;
    
    // property below has its own compiler generated backing field
    public string PublicProperty { get; init; }
}

var obj = new MyClass
{
    PublicField = "hi!",
    PublicProperty = "hello!"
};

@jcouv
Copy link
Member Author

jcouv commented Apr 15, 2020

Why do you need the attribute for fields? Why is not the existing FieldAttributes.InitOnly sufficient for fields?

I think we could omit [InitOnly] attribute from backing fields on auto-properties. But we would need it if bring back init-only fields (was in initial design, but is now out-of-scope).
Jared agreed with that. I'll update the proposal.

@jcouv
Copy link
Member Author

jcouv commented Apr 15, 2020

@GrabYourPitchforks Init-only fields were pushed out of scope for C# 9 in Monday's LDM discussion. Notes should be available shortly. We could add this back (in C# 9 or later) based on scenarios and feedback.

@GrabYourPitchforks
Copy link
Member

Thanks all for the context - much appreciated! :)

@Joe4evr
Copy link
Contributor

Joe4evr commented Apr 15, 2020

Init-only fields were pushed out of scope for C# 9 in Monday's LDM discussion. Notes should be available shortly.

I very much prefer this approach first over depending on the aforementioned runtime quirk. 👍

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review blocking Marks issues that we want to fast track in order to unblock other important work labels Apr 16, 2020
@terrajobst
Copy link
Member

terrajobst commented Apr 16, 2020

  • Seems like we should think about how reflection will work with this feature
    • Reflection doesn't honor modreqs.
    • Should we have a reflection feature for init only? It would construct, init, and call the validator. PropertyInfo.SetValue could block setting init only.
  • Serializers probably need to handle init setters (as in, they shoud be able to call them)
  • ModReqs and ModOpts are usually plain marker types, not attributes. The reason this was made an attribute was to make reflection easier, but it seems we should have a reflection API anyway in which case it doesn't matter. We should be consistent with existing modifier types and not derive from attribute.
  • We should prefix it with Is to be consistent with the other modifier types
  • It was asked whether it only applies to properties and the answer is no, it might apply to fields and methods in the future, so the name should be generic
  • Update. Based on the discussion below we decided that IsInitOnly would be unfortuantel becaue initonly already has meaning in IL/the runtime. We settled on IsExternalInit.
namespace System.Runtime.CompilerServices
{
    public sealed class IsExternalInit
    {
    }
}

@agocke
Copy link
Member

agocke commented Apr 20, 2020

I share the concern that initonly means something very different at the runtime level. I think that initonly is a good name for the C# language and a bad name for the runtime layer.

I think a better name would be IsExternalInit. This is much closer to how the actual feature looks at the runtime level.

@terrajobst
Copy link
Member

terrajobst commented Apr 20, 2020

Oh, apparently we missed that concern. Yeah, since initonly is an IL keyword, we should probably not use that. I like IsExternalInit.

@dotnet/fxdc, anybody else?

@bartonjs
Copy link
Member

IsExternalInit isn't a great name, but if it's only used by modreq then it's not really in anyone's face, and it seems good enough.

@agocke
Copy link
Member

agocke commented Apr 20, 2020

Not sure if it will be a modreq or an actual attribute, but either way it will be turned into syntax in C#, so it will not be commonly visible.

Did you have a better suggestion? I was just proposing staying away from initonly

@terrajobst
Copy link
Member

Nah, @bartonjs is just disappointed that the name doesn't include ALL_CAPS or a crypto algorithm ;-)

@jcouv
Copy link
Member Author

jcouv commented Apr 20, 2020

We discussed the unfortunate naming in relation to initonly flag in the thread above.
Personally, I think IsInitOnly is fine because it is only used by the compiler (does not appear in user code anyways).

@agocke
Copy link
Member

agocke commented Apr 20, 2020

Personally I think that's the exact reason why it's a bad name. If it's mostly metadata-level visible (the only way you'll see it is if you're looking directly at metadata), it should use metadata-relevant names. Remember that we'll be encoding this into IL-verification, so this should become a part of a new metadata specification in the future.

@davkean
Copy link
Member

davkean commented Apr 21, 2020

Is InitOnlyProperty_setAccessor going to be declared an accessor of InitOnlyProperty and marked special named?

@Joe4evr
Copy link
Contributor

Joe4evr commented Apr 22, 2020

Regarding Validators and reflection: I presume that the implementation of validators will be something that's easily identifiable from metadata. Because of that, I think it'd make sense to at least allow reflection scenarios to explicitly call a validator, even if normal C# can't. A strawman of that may look something like this:

public abstract partial class Type
{
    public virtual void InvokeValidatorIfExists(object obj); //no-op if the type has no validator
}

This can go out into a separate discussion if it comes to that, but since it briefly came up in the design meeting, I figured I'd put my 2 cents here first.

@terrajobst
Copy link
Member

terrajobst commented Apr 22, 2020

Is InitOnlyProperty_setAccessor going to be declared an accessor of InitOnlyProperty and marked special named?

@jcouv should answer this, but based on how I understand the compiler feature I'd expect that set_InitOnlyProperty() will be marked just like any other set accessor with the only difference that the return type is modreq'ed with IsExternalInit.

@KalleOlaviNiemitalo
Copy link

Metadata for the init accessor is being discussed in dotnet/csharplang#3376.

@davkean
Copy link
Member

davkean commented Apr 23, 2020

@jcouv Is the IL called out above out of sync with the current design?

Above states:

public void InitOnlyProperty_setAccessor(modreq(typeof(InitOnlyAttribute)) string value) { ... }

But on other threads, it appears to be:

public modreq(typeof(InitOnlyAttribute)void set_InitOnlyProperty(string value) { ... }

@jcouv
Copy link
Member Author

jcouv commented Apr 24, 2020

Yes, the modreq was moved to the return type following API design review.
The type also is no longer InitOnlyAttribute, but IsExternalInit (which isn't an attribute).

public modreq(typeof(IsExternalInit) void set_InitOnlyProperty(string value) { ... }

(Updated OP to reflect)

@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 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

Successfully merging a pull request may close this issue.