dotnet / csharplang

The official repo for the design of the C# programming language
11.59k stars 1.03k forks source link

[Proposal] Required Properties #3630

Open 333fred opened 4 years ago

333fred commented 4 years ago

Required Properties

Specification: https://github.com/dotnet/csharplang/blob/master/proposals/csharp-11.0/required-members.md

Design meetings

https://github.com/dotnet/csharplang/blob/master/meetings/2020/LDM-2020-09-16.md#required-properties https://github.com/dotnet/csharplang/blob/master/meetings/2020/LDM-2020-12-07.md https://github.com/dotnet/csharplang/blob/master/meetings/2021/LDM-2021-01-11.md https://github.com/dotnet/csharplang/blob/master/meetings/2021/LDM-2021-03-03.md#required-members https://github.com/dotnet/csharplang/blob/main/meetings/2021/LDM-2021-10-25.md#required-members https://github.com/dotnet/csharplang/blob/main/meetings/2021/LDM-2021-12-15.md#required-parsing https://github.com/dotnet/csharplang/blob/main/meetings/2022/LDM-2022-03-21.md#open-question-in-required-members https://github.com/dotnet/csharplang/blob/main/meetings/2022/LDM-2022-03-23.md#open-questions-in-required-members https://github.com/dotnet/csharplang/blob/main/meetings/2022/LDM-2022-05-02.md#effect-of-setsrequiredmembers-on-nullable-analysis https://github.com/dotnet/csharplang/blob/main/meetings/2022/LDM-2022-11-30.md#revise-membernotnull-for-required

wanton7 commented 4 years ago

Then why can't it be like this?

Because adding that init there feels really redundant.

Serentty commented 4 years ago

I like @wanton7's proposal. While it may be a bit asymmetrical, it helps avoid modifier stew for a common use case, and a use case that I think should be encouraged.

chucker commented 4 years ago

I think the symmetry is better. A bit verbose, yes, but it makes the distinction clearer.

Serentty commented 4 years ago

The issue I see with it is that the safer option being the more verbose option is usually trouble. Think of how many const modifiers could be used in C++ but aren't because it's more verbose. The clarity of the name is something that can be resolved easily with a quick glance at the documentation, but its verbosity is something that subtly impacts the way that people write code in the long run.

orthoxerox commented 4 years ago

Whatever option is selected, I hope deserializers can use it. If I define a DTO that looks something like

#nullable enable
public record Schema
{
    //NB: I have deliberately omitted all modifiers
    public string Name;
    public List<Table> Tables;
}

I expect the following:

  1. When I consume a Schema, I can rely on Name being not null and Tables being not null
  2. When I create an instance of Schema by hand, I can rely on the compiler warning me if I don't initialize either property
  3. When I invoke a deserializer from JSON or DbDataReader or whatever, it can validate that all properties are initialized on the instance of Schema before returning it to me, either by trapping an exception or reading the metadata.
markm77 commented 4 years ago

I just want to note this feature will be super useful even when using constructors because tooling will be able to auto-generate/auto-update constructors based on required properties/fields. Which will save a lot of time. And of course bugs will be reduced because complier errors will result if one forgets to add a required property to a constructor which is in use somewhere in the code.

IMHO the problem up till now is programmers have no way to express the intent of "initialisation required" at the field/property level. In Swift, a programmer simply doesn't specify an explicit default to express this*. But that doesn't work in C# due to automatic default values. We work around this by using constructor arguments as a required property list (which requires maintenance) or via various attributes or by using validators following initialisation etc. All of which have issues and complicate code.

I think adding at field/property-level a consistent way to express "required" will be a massive step forward for C# and make tight and robust code a lot more simple to create. I'm really looking forward to it. 😀 It's the number one C# feature I'm waiting for.

* NB optionals (nullables) in Swift work differently and do have an automatic default and hence there is similar ambiguity for these about whether the programmer “requires initialisation”

alrz commented 4 years ago

I find myself wanting this today. Even though I'd prefer "Retconn nullable and property defaults" option, here's my vote:

public string FirstName { get; } = init;
public string LastName { get; } = init;
orthoxerox commented 4 years ago

@alrz I also prefer the retconning, to the point where I would be happier if nominal records were cut from 9.0 and introduced in 10.0 with required properties by default.

alrz commented 4 years ago

Yup. Any time you want "required properties" usually you want all of them to be required and perhaps some to be optional. It would add much boilerplate code to specify the opposite. This is now more evident with NRT when you can actually annotate such properties with a ?. That should be enough hint for the compiler to infer the intent.

Serentty commented 4 years ago

The good news is that it seems that the syntax for automatic record properties (the data keyword) has been postponed to C# 10, so there's a chance that it will use required initialization.

wanton7 commented 4 years ago

@alrz Do you mean this would be used for optional init properties?

public string FirstName { get; } = init;
public string LastName { get; } = init;

Because it doesn't allow you to specify default values. But one currently in preview does

public string FirstName { get; init; } = "John";
public string LastName { get; init; } = "Doe";

But did you mean that for required? If you did I think it would made code harder to read

public string FirstName { get; init; }

for optional and

public string FirstName { get; } = init

for required

alrz commented 4 years ago

@wanton7 Good point. Though I believe any syntax here is redundant. the type being nullable is enough and it's already there. We should just warn on object construction (new) rather than inside the type.

Since that ship has sailed I think the only workable approach is to do this only for data properties as @Serentty said.

sjb-sjb commented 4 years ago

To risk an oversimplification, we are looking at "init" as meaning "can be initialized" and "req" as meaning "must be initialized". The idea of initialization debt is appealing and it suggests to me that initialization must occur, obviously, before use i.e. before 'get' is invoked.

I know that the proposal involves initializing at specific places/times related to construction, rather than at any point prior to use of 'get'. However my main goal in using non-nullables is to prevent nulls from being propagated throughout my code, and for that purpose it is perfectly acceptable to leave properties uninitialized until a time prior to use. A perfect example of this is when an object is going to be initialized (and then validated) through a UI interaction. From a domain modeling point of view all properties may be required, but in terms of the process of obtaining those required values, the properties will not be initialized at construction time or through an initializer expression but later on as part of user interaction. It would not be satisfactory to mark all such properties as nullable in the model because that does not match the domain meaning of the object, nor does it allow us to take advantage of non-nullability features in the language.

Consequently one part of my suggestion is to relax the required initialization points so that initialization can occur at any time prior to 'get' being called. This will enable the scenario I describe and is consistent with the goal of not allowing nulls to propagate throughout a body of source code.

The other part of my suggestion here is to fill in a logical gap that arises in this approach. In addition to "can be initialized" and "must be initialized", we need to be able to express "is initialized" or "has been initialized". Let's look at a simple example of a non-nullable uninitialized property supported by a required nullable backing field:

public Wheel FrontLeft { 
      get => this._frontLeft ?? throw new UninitializedPropertyException();
      set => this._frontLeft = value;
} 
private Wheel? _frontLeft;

Allowing the delayed initialization that I am suggesting for 'req', the auto-implemented version of this would be

public Wheel FrontLeft { get; req init; } 

Now, the piece that is missing in the context of delayed initialization is being able to test for whether the property is initialized or not. As it is written above, it is hard to use the property without risking an exception. The exception makes sense from a domain model point of view because the property is intended to be required. However when using the object prior to completion of the delayed initialization, the exception presents a danger. We need to be able to test at run time whether or not initialization has occurred. I propose recycling the "is" keyword for this:

public Wheel FrontLeft { 
      get => this._frontLeft ?? throw new UninitializedPropertyException();
      set => this._frontLeft = value;
      is => this._frontLeft != null; 
} 
private Wheel? _frontLeft;

The auto-implemented version of this would be:

public Wheel FrontLeft { get; req init; is; }

The addition of 'is' in the nullable context would enable the compiler to adapt nullability checking so that a test such as "this.FrontLeft != null" would not result in assuming that FrontLeft is nullable. On the contrary, this.FrontLeft would be considered a non-nullable except for the specific case of being compared to null (although, some thought should be given to use of such a property in 'is' pattern matching). When nullability checking is disabled, 'is' would be syntactically disallowed, much as "?" on a reference type is not accepted syntax when nullability checking is disabled. We could also extend the use of 'is' to value type properties, where it would imply that the backing store is a nullable value type (this increases expressibility over the nullable-oblivious language, but not in a breaking way).

An example benefit of being able to use 'is' explicitly in the non-auto-implemented case is it enables us to work more easily with the dependency property system. Instead of being backed by a private field as shown above, the property may be backed by a dependency property, which obviously is a different implementation of a nullable backing store.

In this way we can achieve multiple goals: domain modeling of non-nullable properties; good interaction with the UI system; allowing delayed initialization; and avoiding boilerplate for required constructors.

markm77 commented 4 years ago

Thanks @sjb-sjb for this interesting proposal.

Consequently one part of my suggestion is to relax the required initialization points so that initialization can occur at any time prior to 'get' being called.

The downside of allowing initialisation after the object initialiser is that we can no longer take advantage of compile-time checking and warnings/errors to ensure our object initialisers include all required properties. These compile-time checks were to my mind one of the biggest benefits of this proposal and would allow e.g. auto-population of object initialisers with all required properties via IDE tooling. This would greatly simplify initialisation and use of third-party classes etc. Pushing checks for required initialisations into run-time (or even compile-time) errors at point of access would be much less helpful IMHO.

In your proposal there is also the problem of init; and req init; now having different meanings for the word "init".

As an alternative idea, why not use req set; for your use-case of initialisation required but might happen after the object initialiser but before use (and enforced by compile-time or run-time checks)? This would keep the meaning of "init" consistent (i.e. property can be set during constructor or object initialiser) and would also allow the object initialiser compile-time checks described above.

I also think if we have is it should be valid for all types (not just nullables) as req ("required") implies the default value is meaningless so having a check for "do we have a meaningful value?" (i.e. "has this been initialised?") is relevant to any type. (Note also the user-initialised value might be the same as the automatic default value (e.g. user initialises integer to 0) so a comparison check doesn't work either in your case without special adjustment or in general.)

sjb-sjb commented 4 years ago

Hi @markm77 thanks for the comments.

I don't think init; and req; init; would involve different meanings for 'init'. It would always mean "can be set during construction/initialization" as compared to 'set;' meaning "can be set at any time".

I agree it would be useful to distinguish at-creation initialization tests from subsequent run-time initialization tests. However I am not as sure whether allowing req; with set; would make sense. I believe that "set" is something that could occur repeatedly and it generally allows you to set the value back to null (when #nullable disable), which is not the intention here. Arguing on the other side, when #nullable enable one could say that the non-nullable property can't really be set back to null anyway ... on the third hand I wonder if that might lead you down the path to different behaviors under different settings of #nullable.

Also would there not be some redundancy inasmuch as specifying "is;" already could be used to infer that "get" should throw an exception if the backing store is null?

Maybe one could define 'is;' so that: (a) the compiler adds code to the 'get' accessor to throw an exception at run-time if it returns null and (b) if the property is auto-implemented then the backing store is nullable (c) comparisons to null do not result in the inference that the property is nullable. Then we could reserve 'req' and 'init' for the at-construction meanings in the original proposal.

If we did that then I believe 'is' and 'req'/'init' would be orthogonal to each other. If you specified both 'is' and 'req' then outside of the initialization/construction code the complier could optimize away the run-time test in 'get' because the 'req' will have already forced assignment during initialization/construction. Technically 'is' would still have meaning when combined with 'req' since during construction the property may or may not have been assigned.

markm77 commented 4 years ago

don't think init; and req; init; would involve different meanings for 'init'. It would always mean "can be set during construction/initialization" as compared to 'set;' meaning "can be set at any time".

Okay, so you want to extend both init; and req init; to include initialisation outside the object initialiser. I understand although feel this undermines initialisers and the convenience of compile-time checks at point of object creation.

The suggestion of req set; was made because I understand you do actually want to perform initialisation with the "setter" rather than in the "initialiser". But I agree this means the property is not read-only.

Do you really have a hard requirement not to use constructors or object initialisers but want to only allow use of the "setter" once/prior to first access? This seems to trample over the existing concepts in C# (constructors, object initialisers) to create a third kind of read-only property. A high degree of complexity and confusion are likely outcomes IMHO....

sjb-sjb commented 4 years ago

@markm77, in my last post I am not proposing to extend req or init at all, but to add a separate ‘is’.

I gave what I think is a pretty good use case in my earlier post, relating to initializing values from the UI. This is in fact a live situation as I am actually doing this and encountering this problem. The compile-time features of req and init really do not address this situation. Of course glad to hear any constructive suggestion people may have addressing that use case.

I believe it is reasonable to have both run time and compile time features supporting uninitialized non-nullables.

markm77 commented 4 years ago

Thanks @sjb-sjb , probably best for me to stop responding to free up this thread. My main discussion point with you was about allowing initialisation after the object initialiser but seems you are moving away from that.

HaloFour commented 4 years ago

@sjb-sjb

I don't think is is required for that. Whether or not the backing field is null (or any other value) doesn't tell you whether or not it was initialized. Setting the property to null would be perfectly legal and would satisfy required properties.

I think what you're looking to achieve, namely throwing at runtime if a specific field has not been set to a non-null value, could be easily accomplished through #140 :

public string Name { get => field ?? throw; set; } 
sjb-sjb commented 4 years ago

@HaloFour, the backing field being null does tell me whether or not the property was initialized, for a property having a non-nullable type which is the use case here.

I agree #140 could be part of the solution but it does not address the need to be able to test whether or not the property has been initialized. That is the purpose of ‘is’.

@markm77 I am not moving away from the idea of post-constructor initialization at all, that is still the main point of my suggestion. Well it may be a little confusing to use the word “initialization” here since that word usually refers just to a few specific times (the page on init lists them) while I am using the word on a more general sense.

HaloFour commented 4 years ago

@sjb-sjb

the backing field being null does tell me whether or not the property was initialized, for a property having a non-nullable type which is the use case here.

Required properties doesn't care about the value, it only cares that the property setter was called during initialization. There is no runtime component and it's not intended to be a method to enforce NRTs, beyond stating that the property is required and warning if the consumer sets it to null.

If you're looking for a proposal that can throw at runtime on a property getter if a certain condition is met it sounds like you should open a separate discussion.

spydacarnage commented 4 years ago

the backing field being null does tell me whether or not the property was initialized, for a property having a non-nullable type which is the use case here.

If this functionality was implemented, it would apply to all properties, including nullable ones. Meaning that setting null during initialisation is still a valid choice. To achieve your example of

public Wheel FrontLeft { get; req init; is; }

I think it would need to lower to:


private Wheel? _frontLeft;
private bool _frontLeft_init = false;
public Wheel? FrontLeft { 
      get => this._frontLeft_init ? this._frontLeft : throw new UninitializedPropertyException();
      set => { this._frontLeft = value; this._frontLeft_init = true; }
      is => this._frontLeft_init 
} 
sjb-sjb commented 4 years ago

@spydacarnage that’s one way to look at it. But if ‘is’ is supposed to tell us whether or not the property has been initialized then it should be true when the property has a valid value. For a nullable, all values are valid. So rather than have another field to indicate whether or not a nullable has been initialized, I think we could just have: is => true for nullable types. The question mark for me is on value types: whether including’is’ should imply that the backing field is nullable. I tend to think it should.

I agree this could go in a different thread, either a new one or possibly #140.

sjb-sjb commented 4 years ago

I have opened #3963 for this.

ericsampson commented 4 years ago

@333fred , what's the interaction of this proposal with data ? I'm super excited by this proposal happening in some form, BTW, so thanks a lot for championing it. I like the 'initialization debt' concepts too. And having some sort of a factory pattern support would be very nice. Everyone uses different naming for them, and it would be sure nice to have some way to indicate them in the language (and enable how it would play into the other parts of this proposal)

ericsampson commented 4 years ago

I think the 'retcon' concept is an interesting idea, even though at first it seems like a huge break. I'm guessing that most people who are excited/have implemented NRT would wish that the behavior that's coming out with this proposal was the way it had come from at the start. And are the type of people who'd be OK with refactoring around the break, and pretending that the previous behavior never existed - in exchange for clean behavior going forward. One downside I can think of is that a person would have to be aware of the nullable setting of a codebase if they're just casually reading it in GitHub etc, in order to understand the behavior of the same lines of code. It kind of introduces C#10 version A and B... It's still worth seeing what people think of the idea, I'd say. Cheers

333fred commented 4 years ago

what's the interaction of this proposal with data ?

They're separate features. If we even have data, it may imply this, or it may have nothing to do with this.

ericsampson commented 4 years ago

Thanks Fred. If anyone has links to the data related proposals/discussions, I'd appreciate that. I'm having a hard time finding them now.

mharthoorn commented 4 years ago

So I understand that the init means that you can only set a property during initialization, and that is does not mean that a property is required to be initialized.

But with nullable being enabled, the current warning is literally: "Non-nullable property [..] must contain a non-null value when exiting constructor".

It seems very natural to make the very combination of a non-nullable property together with init to mean "must be initialized during construction". It feels more logical than to add yet another keyword in front of your property.

The previously mentioned problem is that if nullability is not enabled in your project, you cannot enforce the requirement. That is true, but currently, nullability enabled with an init on a non-nullable property, always gives a warning which make it harder to use.

ericsampson commented 4 years ago

@mharthoorn you're basically advocating for the alternative labeled "Retconn nullable and property defaults" in the original post, right?

Pyrdacor commented 4 years ago

As this is basically a hint for the compiler to move the warning from the property to the contructor I would strongly vote for an attribute on the property. Either [Required], [NotNull] or something like [MustInit].

But as I understand correctly this is only an issue for nullable types so maybe the [NotNull] attribute would fit best. The compiler can then switch to a different warning or error when it finds that attribute.

mharthoorn commented 4 years ago

@ericsampson, I think i am advocating for the "initialization debt" paragraph. But I'll try to be more explicit. The core is that it's not a matter of the class definition to have warnings in them, since initialization can happen else where. The warnings should happen at actual construction / initialization. It would help the adoption of the nullability feature a lot, because it gets rid of a lot of unnessessary warnings.

Current behaviour:

#nullable enable
class Person
{
    string FirstName { get; init; }  // warning: non-nullable property must contain a non-null value
    string? MiddleName { get; init; }
    string LastName { get; init; }   // warning: non-nullable property must contain a non-null value
    string City { get; init; }  = "Roslyn Washington" ;
}

var p = new Person { FirstName = "Mads" } 
// No warnings.

But:

Proposed behaviour:

#nullable enable
class Person
{
    string FirstName { get; init; }  
    string? MiddleName { get; init; }
    string LastName { get; init; } 
    string City { get; init; }  = "Roslyn Washington" ;
}

var p = new Person { FirstName = "Mads" } 
// Warning: LastName is a required property and must be initialized during construction.

With a constructor:

If the class did have a constructor, It becomes a little bit less obvious, if it leaves properties empty.


#nullable enable
class Person
{
    string FirstName { get; init; }  
    string? MiddleName { get; init; }
    string LastName { get; init; }  
    string City { get; init; } 

    public Person(string city)
    {
        this.City = city;
    }
}

var p = new Person("Roslyn WA") { FirstName = "Mads" } 
// Warning: LastName is a required property and must be initialized during construction

With nullable disabled,

it's a different story, but there would be a lot less need for disable nullability if we dealt with it this way.

hez2010 commented 4 years ago

Why not extend it to more general scenario which can be applied to both properties and parameters, with more flexible requirement customization.

requires {
    expr; expr; expr; ......
}
void Foo(int v) requires {
    v > 3 && v < 10;
    v % 2 == 0;
} {
    Console.WriteLine(v);
}

class C
{
    private object v;
    public object Value 
    {
        get => v;
        init requires {
            must;
            value is int x && x is > 5 or < 1 || value is string;
            value is not null;
        } {
            v = value;
        };
    }
}

C x; // error: must init Value
C y = new C {
    Value = 2
}; // error: "value is int x && x is > 5 or < 1 || value is string" cannot be fulfilled
C z = new C { Value = "test" }; // ok
ronnygunawan commented 4 years ago
class Person(
    string FirstName, // public, required, get, init
    string LastName // public, required, get, init
) {
    public string? MiddleName { get; init; } // optional
}

Person p = new("John", "Doe") {
    MiddleName = "C#"
};

Required properties in this syntax can only be public, required, and init only. Any other cases should be implemented using standard constructor.

SWATOPLUS commented 4 years ago

Looks like that properties of Positional Record should be required by default. See: [Proposal]: Allow skipping constructor of Positional Records #4178

We should get rid of following compile error:

public sealed record SequenceStats(double Average, double Median, double StandardDeviation, double Minimum, double Maximum);

var scores = new [] { 1, 2, 3 };

var static = new SequenceStats // CS7036: There is no argument given that corresponds to the required formal parameter 'Average' of 'SequenceStats.SequenceStats(double, double, double, double, double)
{
    Average = scores.Average(), 
    Median = scores.Median(),
    StandardDeviation = scores.StandardDeviation(),
    Minimum = scores.Min(),
    Maximum = scores.Max(),
};
jods4 commented 3 years ago

This thread seems to focus a lot on the syntax to mark properties required.

Personally, I don't have much use of the "required" concept but I'd love if the "uninitialized nullable ref" problem was handled better. The two are related but may weigh pros and cons differently.

Because I care most about nullable initialisation, I am definitely in favor of the implicit (no syntax/annotation) approach.

I have tons of properties that look like that

string Name { get; set; } = null!;

I'll be honest, I don't think looking like either of those is a big improvement:

[MustInit]
string Name { get; set; }
string LastName { get; req set; }

What is a huge improvement IMHO is moving the error from ctor to construction site. If I have

#nullable enable
string Name { get; set; }

Then obviously I don't intend Name to ever be null. So if the ctor doesn't set this property, the caller should do it in an initializer. If it doesn't it's a violation of the type system, so it's normal to report it. Whether the fix should be adding an initializer, modifying the ctor, or making the prop nullable string? can't be guessed by the compiler.

Let's look through this lens at some pros/cons from above:

Should it be a warning or an error? Today C# has decided to use nullable warnings (you can always "treat warning as errors"). I'm talking about moving where CS8618 is reported, so it would naturally stay a warning.

Is this a breaking change? In some cases, I'm afraid. Generally it won't if it only applies to nullable-enabled code. Because today any property that is not initialized after construction is already reporting CS8618, after this change there would be strictly less warnings, not more. I see two big exceptions, though.

1) New warnings w/ library types. A library might have disabled CS8618 when it's compiled. With this change, the warning has moved from library compilation to end-user compilation. It's probably good because legit cases for this aren't newing the class but reflection, serialization, etc. It's still a break, though. It's mitigated by the fact that it's only a warning and could be turned off, but I don't have better ideas.

2) What about structs? If a property is of non-nullable type and ends up as null after construction, it should be a warning. No reason to behave differently than classes. Unfortunately this is breaking because CS8618 isn't reported for structs today. I think it's for the better to warn here; although this could be mitigated by using a new CSxxxx warning for structs and making it opt-in.

What about factories and other partial initialisation situation? Tricky. It would be awesome to track these correctly and that's basically the "initialisation debt" idea above. If awesomeness can't be achieved, moving the ctor warnings into factories would already solve lots of other real-world use cases and be a big improvement.

In conclusion: what has a huge value for me is proper null handling, I don't care as much about the more general concept of "required initialisation". Nullable dataflow analysis is a general concept that should "just work" without annotations, so req doesn't feel right. To me CS8618 was an error. It leads to a lot of boilerplate "fake" initialisations, that compromise correctness. Not only does string name = null! feel bad, there will be no warning if I new Person() without an initialiser and then proceed to do person.Name.Length. From this point of view, fixing it implicitly is a no-brainer.

333fred commented 3 years ago

We discussed this LDM on Monday (December 7th 2020), with a new revision of the proposal: https://github.com/dotnet/csharplang/discussions/4209.

chucker commented 3 years ago

@jods4

What is a huge improvement IMHO is moving the error from ctor to construction site.

That's what https://github.com/dotnet/csharplang/issues/2109 was originally about, which was merged into this. So, presumably, you'll get exactly that once this issue is closed.

brunosaboia commented 3 years ago

What's the reasoning being init props being non-mandatory? I may have missed something, but I imagine that they'll be very rarely used without a req modifier.

One such case could be a report where some of the data is not present, depending on the year. You still want immutability for the report, but not every year you would have, for instance, M&A costs.

Bellarmine-Head commented 3 years ago

Input from a line-of-business application developer:-

A beautiful thing that I used to enjoy in F# was introducing a new field into one of my record types... and have all the construction sites break on re-compilation.

I have missed this feature so much in C#, but I'm too lazy to make all my classes have ctors with all the necessary args to initialize all the (read-only) properties.

Today I got to grips with C# 9 records for the first time, and looked forward to that sweet "must init" goodness.... to little avail!! Unless I've read it wrong, the only way to get such a thing in C# records is to only use positional records / primary constructors, and never introduce a standalone property definition.

The problems with positional records / primary constructors are:-

The simple addition of:-

public object Prop { get; req init; }

would seem to give us the best of all worlds. Add req to make it required; leave out req if it's not required... an improvement on F# even.

ericsampson commented 3 years ago

The simple addition of public object Prop { get; req init; } would seem to give us the best of all worlds. Add req to make it required; leave out req if it's not required...

I also like the simplicity of this idea, it's how I thought things were going to go. Now I'm not sure it is?

EnCey commented 3 years ago

Just wanted to chime in and add my vote for such a feature. Personally I like must set or must init because of how natural it reads, I think it's even somewhat self-explanatory, and because "must" is more concise than "required".

I would not be opposed to a keyword for the entire property, though. I'm not sure if this analogy has been made yet, but in terms of how it fits into the language and how difficult it is to grasp for new developers, I believe it makes sense to compare this feature to abstract:

While technically "required" will only affect the set portion of a property, I believe that conceptually you would probably view the entire property as "required". As for readability, a keyword "in front" of the property makes it (arguably) simpler to quickly scan a class's properties, especially if we have longer property types and names like IReadOnlyDictionary<string, SomeOtherVerboseType> MyVerboseProperty {}

public required string Firstname {get; set;}
public abstract string Lastname {get; set;}

As a final point, I must note that "abstract" and "required" both are 8 characters long, which means they would nicely align, which I can't help but find appealing. I believe it's also an argument that "required" wouldn't be too verbose seeing as I've never heard anyone complaining about "abstract" being verbose. Furthermore, auto completion features of IDEs take care of typing anyway, and "required" doesn't overlap with any other keywords, hence it would work well there too.

DQRC69420 commented 3 years ago

Excuse my ignorance but why not set the required property through constructor parameters? If you make the default constructot private and create a constructor that takes the parameters to set the required properties, doesnt that already solve the problem?

adambajguz commented 3 years ago

In my opinion object initializer is more elegant and error proof than ctor or default ctor with init props. Imagine you have a ctor with 10 string props, a programmer has to pay extra attention about what he passes to ctor arguments. He can of course use named arguments, but he is not forced to. required init forces programmer to pay extra attention when assigning values, and provides a language solution for init/set props misuse during object creation (not initializing of what is required). I think programmers will more likely use { get; required init; } than { get; } with ctor.

init tried to solved a problem of writing constructors for props that shouldn't change after object creation (i.e. { get; }), but didn't provide a feature that ctor always had, i.e, constraint that you have to set a value for all arguments without default values that are defined in ctor.

However I don't know if required init; would allow for changing prop from the inside of the object e.g. { get; private set; required init; } because { get; private set; init; } is also not possible (this is a bit disappointing, but { get; private set; init; } can be achieved with a backing field).

Summarizing, 1) it's much faster to write required or other keyword than complex ctor; 2) this solves the biggest problem with init props - trusting programmer that the prop will be initialized; 3) removes pointless default! \ null! assignments; 4) adds language level feature to ensure init prop was initialized; 5) for me current init implementation without required is good only for DTOs and other externally initialized objects (when you don't create object by yourself, i.e. by using new).

333fred commented 3 years ago

I've removed the initial specification from this proposal, as it is now in PR here: https://github.com/dotnet/csharplang/pull/4493. When merged, I'll update the link to point to the merged specification itself.

TonyValenti commented 3 years ago

I just added a new parameter to a base class's constructor. Then I had to refactor 50 derived classes to pass in that parameter. Man, I wish I had required properties and a DI framework that would inject using them instead of constructors.

loligans commented 3 years ago

I guess I'll throw in my two cents. I am going to cover 3 areas that I think this proposal should deal with. I hope this helps someone out there.

The warning location when nullable is enabled

The issue I have with init by itself is that when nullable is enabled, the compiler will give a warning to types who have not been initialized in the constructor. The warning is: "Non-nullable property 'ActionName' must contain a non-null value when exiting constructor" image

This is technically correct behavior, but the location of the warning feels inconvenient. The warning should show up on object initializers. Note: this specifically only applies when nullable is enabled.

image

Using required with init properties

Summary: Properties are required to be set during object initialization and cannot be mutated afterwords.

The following should throw a compiler error because the "required" properties ActionId and ActionName are not specified during object initialization. The important bit in this example is that it forces a programmer to initialize the required properties which are normally set to default(T). Note: It should also require value types to be initialized.

public record Toast
{
    public required int ActionId { get; init; }
    public required string ActionName { get; init; }
    public string ActionText { get; set; }
}
new Toast
{
    ActionText = ""
}

Using required with set properties

Summary: Properties are required to be set during object initialization and can be mutated afterwords

The following should be valid because the "required" properties ActionId and ActionName are set during object initialization. Like the previous example, the programmer is forced to initialize the required properties which are normally set to default(T). The important bit in this example is that the properties are mutable. Note: It should also require value types to be initialized.

public record Toast
{
    public required int ActionId { get; set; }
    public required string ActionName { get; set; }
    public string ActionText { get; set; }
}
var toast = new Toast
{
    ActionId = 1,
    ActionName = ""
    ActionText = ""
}

toast.ActionId = 2;
333fred commented 3 years ago

The warning should show up on object initializers.

Because C# properties are not required anywhere today, this is not possible without the rest of the feature :).

AFAICT, other two sections are proposing a must modifier instead of the existing syntax proposal, is that correct?

loligans commented 3 years ago

@333fred

Because C# properties are not required anywhere today, this is not possible without the rest of the feature :).

This is true.

AFAICT, other two sections are proposing a must modifier instead of the existing syntax proposal, is that correct?

I use the must modifier in my proposal, but I don't have a preference on what modifier we use. If the existing syntax proposal uses something different, then we should use that instead. My goal was to describe the functionality I think this feature should have.

Did we settle on a specific modifier for this feature already? If we have I'll update my post to reflect the modifier we settled on.

333fred commented 3 years ago

Did we settle on a specific modifier for this feature already? If we have I'll update my post to reflect the modifier we settled on.

So far, we've been leaning towards required as a modifier for the property, not for the accessor. Specifically, remember that we want to be able to annotate fields with this as well, not just properties. That's why the checked-in proposal is named required-members.md, not required-properties.md.

My main question was, it didn't look like there was anything particularly different in your proposal from what is checked in, but I wanted to be sure I wasn't misreading anything you wrote.