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

Indicate that a non-nullable Field must be initialized where the type is constructed #2328

Closed
YairHalberstadt opened this issue Mar 11, 2019 · 94 comments

Comments

@YairHalberstadt
Copy link
Contributor

Problem

Consider the following three outstanding issues with NRTs

1. Object Initializers

Object initializers are greatly loved by many C# programmers. However they don't interact very well with NRTs.

Consider this for example:

public class C
{
    public string Str {get; set;}
}
...
var c = new C { Str = "Hello World" };

If Str is to be non-nullable, then when enabling nullability we are forced to completely change all this code:

public class C
{
    public C(string str) => Str = str;
    public string Str {get;}
}
...
var c = new C("Hello World");

Making object initializers usable only be nullable properties and fields.

2. DTOs

Many DTOs are initialized by ORM/Serialization frameworks. As a result they often contain only property getters and setters, without any constructors.

This of course causes warnings when NRTs are enabled, as it doesn't know the properties are always initialized by the ORM/Serialization framework.

3. Structs

Structs always have a default constructor, which cannot be overridden.

As a result whenever a struct containing a field of a non-nullable reference type is constructed using the default constructor, the field will initially be null. This leaves a big hole in the nullable-type-system:

public struct S
{
    public string Str { get; set; } //no warning
}
...
var s = new S(); //no warning
var l = s.Str.Length; // kabam! NullReferenceException! - but no warning

Solution

Based on a suggestion by @Joe4evr #36 (comment)

The real issue here is that present mechanisms only allow us to guarantee a field is initialized inside the constructor, but currently many types rely on their invariants being upheld by whoever constructs the type.

It should be possible to mark that a field/auto-property must be initialized at the point it is constructed, before it is used. This could be done via an attribute for example:

public class C
{
    [MustInitialise]
    public string Str {get; set;}
}
...
public struct S
{
    [MustInitialise]
    public string Str { get; set; }
}

Then when an instance of the type is constructed, it should be a warning to use the type before all such fields are initialised:

public class C
{
    [MustInitialise]
    public string Str {get; set;}
}

...

var c = new C { Str = "Hello World" };
var l = c.Str.Length; //no warning
c = new C();
var x = c.Str; //warning
c.Str = "Hello"
var l = c.Str.Length;  //no warning

...

public struct S
{
    [MustInitialise]
    public string Str { get; set; }
}

...

var s = new S { Str = "Hello World" };
var l = s.Str.Length; //no warning
s = new S();
var x = s.Str; //warning
s.Str = "Hello"
var l = s.Str.Length;  //no warning

All structs should have this attribute on all their fields implicitly.

Further issues

Consider this struct:

public struct S
{
    public S(string str) => Str =str;

    public string Str { get; set; }
}

Then it would be a warning not to initialize S.Str when the the new S(string) constructor was called, even though S.Str was initialized by the constructor!

I think the solution to this would be for the compiler to detect which constructors initialize a field, and implicitly add parameters to the MustInitialize attribute which indicate which constructors did not initialize the struct.

ie. the compiler should generate code equivalent to the following:

public struct S
{
    public S(string str) => Str =str;
    
    [MustInitialize("DefaultConstructor")]
    public string Str { get; set; }
}

then, when the default constructor is called, the compiler knows it must make sure S.Str is initialized.

@Kukkimonsuta
Copy link

image

This seem to be already the case for structs, however follwing is an issue right now:

static void Foo(S s)
{
    var l = s.Str.Length; // kabam! NullReferenceException! - but no warning
}

static void Main(string[] args)
{
    var s = new S();
    Foo(s);
}

@GeirGrusom
Copy link

GeirGrusom commented Mar 11, 2019

In Kotlin this feature is called lateinit and is a modifier applied to properties.

class C {
  lateinit val str : String;
}

Basically it says that the property is not properly assigned by the constructor but will be assigned at a later point.

@DarthVella
Copy link

Great idea, but there's some discussion points that should be addressed quickly.

What of internal initialization methods?

class C
{
    [MustInitialize("DefaultConstructor")]
    public string Str { get; private set; }

    public C() { }

    [Initializes("Str")]
    public void Init(CHelper helper)
    {
        Str = helper.GetHello();
    }
}

Similarly, what of external methods that might allow or disallow uninitialized values?

class C
{
    [MustInitialize("DefaultConstructor")]
    public string Str { get; set; }
}

static void Main()
{
    C c = new C();
    
    UseString(c); //warning?
    SetString(c); //should not be a warning?
    UseString(c); //no warning?
}

[Initializes("c.Str")]
static void SetString(C c)
{
    c.Str = "Hello!";
}

[MustBeInitialized("c.Str")]
static void UseString(C c)
{
    Console.WriteLine("String length is: " + C.Str.Length);
}

For the record, I would assume that this proposal strictly deals with immediate initialization by the caller, and that these considerations are not within the intended scope.

@YairHalberstadt
Copy link
Contributor Author

@DarthVella
In the long term some sort of attribute language might be necessary, but in the short term I believe this proposal represents a minimum viable product.

@DarthVella
Copy link

I should have mentioned that the attributes were only to show the intent, not to suggest that that level of user input was necessary.

@Joe4evr
Copy link
Contributor

Joe4evr commented Mar 12, 2019

I'm generally supportive of the case to have finer granularity over what fields should get initialized by the caller, but I think there's a lot to be said to also have an attribute that can simply cover the whole type in one go. It could be the same, if an appropriate name is available, but my point is that code looks tidier with less attributes all over the place.

EDIT: I just realized there's one more caveat that would need to be filled for that to work smoothly: Externally generated values. A typical EF entity would have the ID generated when added to the database, and it'd be quite annoying to see warnings for those. 🤔

@Joe4evr
Copy link
Contributor

Joe4evr commented Mar 14, 2019

Brainstorming another potential name for this attribute: CallerInitialize?

@ZeroUltimax
Copy link

ZeroUltimax commented Mar 25, 2019

All examples so far were shown using a String field, but I'm certain this could very well apply to value types as well. It seems to me that, since value types have an automatic default constructor, this would ensure proper initialization of nested members.

Ex.:

public struct S
{
    public string A {get; set;}
}

public class C
{
    [MustInitialise]
    public S Data {get; set;}
    
    [MustInitialise]
    public int MoreData {get; set;}
}

Does this seem like a proper usage? It seems to fit in with the spirit of annotating what must be initialized by the caller.

@YairHalberstadt
Copy link
Contributor Author

@ZeroUltimax
Related: #146

@ZeroUltimax
Copy link

#146 is about the usage of default() for all instances of the structure. Here, I'm suggesting that in some cases, you might want to enforce initialization a member, which happens to be a value type.
For example, int MoreData could be an application specific code that shouldn't be left to default. In that case, #146 doesn't apply.

@YairHalberstadt
Copy link
Contributor Author

@ZeroUltimax
I understand. I was just linking another issue which involved treating the default state of a struct as null.

@alexk8
Copy link

alexk8 commented Apr 11, 2019

I think I'm going to close my #2411 (as a duplicate).
Therefore I'm copying here the general idea from there:
[MustInitialize] would be quite messy (used everywhere), this behaviour should be default by design IMHO. My suggestion:

class A{
  public string str; //do not warn "not initialized"
}

void Main(){
  A a1 = new A(); //do warn: "str is not initialized"
  A a2 = new A{str="str"}; //ok
}

that becomes a bit tricky with constructors, I suggest this

class A{
  public string str1;
  public string str2;
  public A(){  
    this.str1="123";
  } //warn: "str2 is also must be initialized"
}

Just a suggestion

@roji
Copy link
Member

roji commented Apr 17, 2019

The fact that this proposal tries to address several different problems (DTOs, object initializers, struct fields) may make things more complicated than they need to be...

It should be possible to mark that a field/auto-property must be initialized at the point it is constructed, before it is used.

Tracking whether a field has been initialized or not seems like an extremely heavy ask... While it's relatively easy to imagine this happening within a single method, once an object starts crossing method (and assembly!) boundaries it's hard to imagine how this would be done. If I receive some instance from a method somewhere, how is the compiler supposed to know which fields on it have been initialized, and which haven't?

In other words, while this could solve the object initializer problem (because initialization occurs within a single function), it's hard to imagine how this could address the general problem of deserialized DTOs, or knowing that some field on a struct is uninitialized.

I've opened #2452 as an alternative proposal to address the same problems, but hopefully in a simpler way. In a nutshell, this would be a syntactic sugar attribute on auto-properties which produces the following:

private Foo? _foo;

public Foo Foo
{
    get => _foo ?? throw new InvalidOperationException($"The property has not been initialized");
    set => _foo = value;
}

This makes the above 3 scenarios work without any additional compiler tracking, but of course doesn't catch cases where a field is accessed prematurely at compile-time. This seems acceptable to me, considering that prematurely accessing a late-initialized field goes against the API contract.

@YairHalberstadt
Copy link
Contributor Author

@roji

Tracking whether a field has been initialized or not seems like an extremely heavy ask.

Definite assignment already does this, and the legwork is literally already in place. I'm not going to say extending definite assignment analysis to definite initialistion analysis is trivial, but it's not a huge issue.

once an object starts crossing method (and assembly!) boundaries it's hard to imagine how this would be done. If I receive some instance from a method somewhere, how is the compiler supposed to know which fields on it have been initialized, and which haven't?

In the long run this could be done by attributes if necessary. However I don't think that's necessarily the point here. The idea is to give you more safety than you currently have. It allows you to safely say these fields won't be initialised by the producer, and must be initialised by the consumer. In 95% of cases that will be done immediately upon creation of the object. The other 5%? That's what the ! operator is for.

it's hard to imagine how this could address the general problem of deserialized DTOs

The idea is that certain objects must always be initialised by the person who creates the objects. However the serialisation framework used may not work with constructors, and so object Initialisers must be used. The serialisation framework itself works through reflection, and so these warnings are irrelevant to them.

I've opened #2452 as an alternative proposal to address the same problems

I don't think it does. That's about providing runtime safety. The NRT feature (and this proposal) is about providing compile time safety.

@roji
Copy link
Member

roji commented Apr 17, 2019

Tracking whether a field has been initialized or not seems like an extremely heavy ask.

Definite assignment already does this, and the legwork is literally already in place. I'm not going to say extending definite assignment analysis to definite initialistion analysis is trivial, but it's not a huge issue.

You mean within the same method, right? If so then you're right, it's essentially already being done.

once an object starts crossing method (and assembly!) boundaries it's hard to imagine how this would be done. If I receive some instance from a method somewhere, how is the compiler supposed to know which fields on it have been initialized, and which haven't?

In the long run this could be done by attributes if necessary.

How? We don't know at compile-time which fields are going to be initialized - that may be conditional on some runtime behavior...

However I don't think that's necessarily the point here. The idea is to give you more safety than you currently have. It allows you to safely say these fields won't be initialised by the producer, and must be initialised by the consumer. In 95% of cases that will be done immediately upon creation of the object. The other 5%? That's what the ! operator is for.

I do agree that something is needed that's better than what we currently have, and I think it makes sense to be able to say "this will be initialized later" (as I also proposed in #2452). However, trying to enforce or detect that by the compiler is a different business...

I don't think see how the ! operator could be relevant here - the way I understand it, we're discussing only non-nullable fields here, and not nullable ones. We're just trying to manage the initialization of those non-nullable fields better.

However, looking at it another way, it seems like it's possible to first introduce an attribute as in #2452, without any compile-time tracking, and at some point in the future to add the tracking you propose as an additional improvement (although again, I don't see this working outside of the scope of a single method).

it's hard to imagine how this could address the general problem of deserialized DTOs

The idea is that certain objects must always be initialised by the person who creates the objects. However the serialisation framework used may not work with constructors, and so object Initialisers must be used. The serialisation framework itself works through reflection, and so these warnings are irrelevant to them.

I understand the idea, but in reality it seems difficult to define "the person who creates the objects". Typical deserialization/materialization logic instantiates the object in one method, passes it to another one for populating the properties... I believe that initialization tracking that stops at the method boundary is likely to be of little value in many cases.

More importantly, I'm not sure that there's that much value in tracking this in compile-time. The end user is supposed to already receive instances which have been fully initialized - so they don't really need our help - and the people writing the deserialization code are less likely to need our help tracking this.

I've opened #2452 as an alternative proposal to address the same problems

I don't think it does. That's about providing runtime safety. The NRT feature (and this proposal) is about providing compile time safety.

I understand what you're saying, and #2452 wouldn't provide an ideal situation. But it's worth keeping in mind that NRT is supposed to provide compile-time safety around nullability, not around initialization - that's a very important distinction.

I'm also trying to be pragmatic here. The chance of a full-fledged compile-time initialization-tracking being introduced in the near future is, well, quite low (and again, I don't believe it could be meaningful as initialization can depend on runtime behavior). #2452 could be implemented pretty easily, and would it make it just as easy to write DTOs as your proposal.

I'm concerned less with providing absolute compile time safety, but rather with the fact that the NRT uninitialized warnings make it very difficult/verbose to write DTOs and similar objects in the NRT world, to the point where people are likely to just turn nullability off.

@roji
Copy link
Member

roji commented Apr 17, 2019

One more note about your proposal to use attributes to express the initialization status of fields on returned types... The NullableAttribute introduced for NRTs exists on the property of a type. The initialization status, however, would have to be defined by each and every method on the types they return, which is something very different (and much more verbose). So Foo() could return type X with property P initialized, while Bar() could return X with P uninitialized...

@YairHalberstadt
Copy link
Contributor Author

The initialization status, however, would have to be defined by each and every method on the types they return, which is something very different (and much more verbose)

I don't imagine anything as complicated as that.

As I stated above, in 95% of cases we initialise an object in the method in which it is declared. In most of the other 5% we may pass it to one or two helpers two initialise it. We can mark the specific helpers with an attribute indicating they initialise the struct manually.

@roji
Copy link
Member

roji commented Apr 17, 2019

As I stated above, in 95% of cases we initialise an object in the method in which it is declared.

That may be true of object initializers (in fact, in that case it's even 100%), but I strongly disagree that 95% of all relevant initialization cases happen within a single method. Once again, DTOs and various other types are frequently instantiated here, populated there, etc. etc.

In general, the value of providing compile-time warnings about accessing uninitialized fields only within the same method simply seems pretty low to me.

In most of the other 5% we may pass it to one or two helpers two initialise it. We can mark the specific helpers with an attribute indicating they initialise the struct manually.

You seem to have a very specific/restricted scenario in mind... For a feature that goes so deep into the language/compiler, it feels like a more complete/comprehensive proposal is appropriate.

Finally, as I wrote above, there's nothing wrong with introducing the attribute as a syntactic sugar for runtime checks and warning suppression (#2452) - this solves the immediate difficulty of writing DTOs and similar objects in the NRE world. We can then investigate other compile-time options for that same attribute in the future...

@chucker
Copy link

chucker commented May 3, 2019

Brainstorming another potential name for this attribute: CallerInitialize?

I like the caller part because it makes it clearer who is supposed to initialize it, but to bikeshed this further, I'd go with CallerMustInitialize. (Or possibly CallerInitialized.)

However, this is probably not that important as long as the associated warning message is sufficiently clear, e.g.:

Warning | CS8618 | Non-nullable property 'Bar' is uninitialized. It must be initialized by the caller.

(This probably shouldn't get the same warning code CS8618, as the expectation of the developer is different.)

@jamesfoster
Copy link

To clarify an earlier comment. The lateinit keyword from Kotlin effectively disables the warning you get when you fail to initialise a non-nullable field during construction. You're effectively disabling the compile-time checking at that point. However, when dereferencing it you don't get a NRE, you get an UninitializedPropertyAccessException which clearly identifies the field being accessed and that it hasn't been initialised.

I found myself using this a lot during unit testing and in rare occasions where a DI container could only initialise my types through property injection. The case for using object initialisers over calling the constructor is weak, however, I can see its usefulness in not breaking existing code.

lateinit is like saying "I promise to initialise this before I access it".

class C
{
  private lateinit string S { get; set; }
}

effectively becomes

class C
{
#pragma warning disable nullable
  public string S { get; set; }
#pragma warning enable nullable
}

new C().S.Length; // Throw a richer exception here than just NRE.

@YairHalberstadt
Copy link
Contributor Author

@jamesfoster

That sounds more like #2452

@charlesroddie
Copy link

charlesroddie commented Jun 5, 2019

Please don't do this! Initialization results in many smelly C# libraries and NRT is a huge opportunity to clean this up. It would be a missed opportunity if these workarounds are introduced.

@YairHalberstadt

public class C
{
    public string Str {get; set;}
}
...
var c = new C { Str = "Hello World" };

... when enabling nullability we are forced to completely change all this code:

public class C
{
    public C(string str) => Str = str;
    public string Str {get;}
}
...
var c = new C("Hello World");

So much better! The first example is unsafe and it's undiscoverable how to construct a valid object. The second is done right. As a result NRTs will improve APIs across the .Net ecosystem.

Many DTOs are initialized by ORM/Serialization frameworks. As a result they often contain only property getters and setters, without any constructors.

This of course causes warnings when NRTs are enabled, as it doesn't know the properties are always initialized by the ORM/Serialization framework.

These ORMs/Serializers are badly structured. But that doesn't matter. What matters is they require a class with a default constructor which doesn't construct a valid object, and after doing various mutations they may guarantee that they have produced a valid object. This is OK. The ORM/Serializers are the ones who face the warnings. The warnings don't seep into user code unless the user is using the default constructor, which should generate an NRT warning (see below).

public struct S
{
    public string Str { get; set; } //no warning
}
...
var s = new S(); //no warning
var l = s.Str.Length; // kabam! NullReferenceException! - but no warning

This is indeed a problem but the solution is to warn on using the default constructor. This would then encourage the author of the struct to give a constructor which creates a valid object.

@chucker
Copy link

chucker commented Jun 5, 2019

As a result NRTs will improve APIs across the .Net ecosystem.

NRTs will improve either way. This proposal by Yair means you'll receive a warning at compile time.

Suppose the library has:

public class C
{
    [MustInitialize]
    public string Str1 {get; set;}

    [MustInitialize]
    public string Str2 {get; set;}
}

And your code does: var c = new C { Str1 = "foo" };, then you get the warning that you failed to initialize Str2. It'll still compile; if you don't want that, you could always include that particular warning in treat warnings as errors.

@YairHalberstadt
Copy link
Contributor Author

@charlesroddie

The LDC have been very clear all this time that they do not intend to boil the ocean. How existing APIs work is fixed, and there is no intention to change them, or to warn on existing legitimate usage of these APIs.

Therefore it's impossible for the APIs to start warning when you use a default constructor.

At least with the features suggested here, it will be possible to warn if the default constructor is used incorrectly.

@HaloFour
Copy link
Contributor

@canton7

Seems the reasoning is that init by itself doesn't indicate that the property must be initialized, and that the consumer is unaware of the possibility that the property has a default value:

public class C
{
    public string Str {get; init;} = "";
}

In this case it would be inappropriate to warn on not setting Str in an initializer. While this is true it feels that there are a number of ways that this could be solved right now, even without tying the feature to a potential future "required init" or "validators".

It is mentioned that NRTs and init-only properties are orthogonal, with which I disagree. NRTs are cross-cutting and already affect virtually every other feature in the language. It is also mentioned that trying to solve this via an analyzer would make it not a first class citizen, which is weird considering that NRT analysis is already driven by a huge number of attributes. In the meantime this leaves a massive gap in NRT analysis.

@markm77
Copy link

markm77 commented May 21, 2020

@canton7 I understand your point but I also think it's worth considering things from the PoV of the consumer of a class. Don't you think it would be pretty unfriendly to receive nullable warnings relating to a class when trying to initialise it? Rather than more straightforward errors relating to required properties, i.e. clear violation of code contract?

req / req set also solves a similar problem in other cases, e.g. for uses of bools and ints where the default value has no meaning and should not be used but I accept that is not why this thread was started.

@HaloFour
Copy link
Contributor

@markm77

Don't you think it would be pretty unfriendly to receive nullable warnings relating to a class when trying to initialise it?

No more than receiving nullable warnings when trying to call a method on that same class. You'll already get nullable warnings if you try to set one of those properties to null explicitly, allowing them to remain null is just as incorrect and as much a vector for bugs.

@bbarry
Copy link
Contributor

bbarry commented May 21, 2020

required ...

you could always hang on a bang:

public string Str {get; init!;}

@andre-ss6
Copy link

Seems the reasoning is that init by itself doesn't indicate that the property must be initialized, and that the consumer is unaware of the possibility that the property has a default value

It's also been mentioned that this would make init even more of a replacement for set. The solution to this issue should be one that is applicable for both set and init.

@canton7
Copy link

canton7 commented May 21, 2020

@markm77

I understand your point but I also think it's worth considering things from the PoV of the consumer of a class. Don't you think it would be pretty unfriendly to receive nullable warnings relating to a class when trying to initialise it?

No? If a property is init, non-nullable, and doesn't have a default value, it's obviously intended that it should be set from the initializer. For consistency with a corresponding set property, you should get a nullable warning somewhere. Your options are:

  1. Warn on the property declaration (as with set)
  2. Warn on initialization

I think option 1 is always going to annoy. If someone has made a non-nullable init property without a default value, they obviously intend that it's set on construction. Warning on the property declaration would be unintended and annoying 100% of the time, I think. That leaves option 2.

Rather than more straightforward errors relating to required properties, i.e. clear violation of code contract?

I'm not against the concept of required properties, but I do think they're orthogonal to this issue. Whether you have required properties or not, you still have to design what happens in the case that someone declares a non-nullable init property without a default value. My argument is that the natural decision in that case covers the specific gripe in the OP, which involves serializers.


@andre-ss6

It's also been mentioned that this would make init even more of a replacement for set. The solution to this issue should be one that is applicable for both set and init.

I don't think init is a replacement for set. set currently covers three cases: "must be initialized during construction", "should be set only during construction" and "can be set at any time". init takes care of one of those. Some sort of required set will take the other. set will still mean "can be set at any time": that's not going to go away.

Coming back to the specific issue in the OP, this issue is about the annoyance of unnecessary warnings when declaring POCOs used by serializers. init + careful design of nullable warnings covers this case, I think. I still think there's a separate issue around "must be initialized during construction" properties, but I think that's a separate issue, which will have its own relationship with NRTs.

EDIT: I got nullable and non-nullable mixed up

@markm77
Copy link

markm77 commented May 21, 2020

@canton7 @HaloFour

What I believe you both are defending/proposing is effectively an implicit init required for the case of an init non-nullable reference type with no default value. I, on the other hand, am advocating an explicit init required.

I prefer an explicit declaration because it allows better bug catching by requiring intent and offers clearer error messaging to API consumers. I believe it will also make code easier to read (more declarative) and aid automated tooling and analysis. Finally it has the potential to cover additional problem areas such as the value type issue I mentioned.

But no problem to agree to differ. I respect you are looking for a simple solution with minimal change. My concern would be it makes a complicated language even more complicated.

@HaloFour
Copy link
Contributor

@markm77

What I am proposing has nothing to do with additional language features or behavior. It has to do with the flow analysis of NRTs, which already has a major gap when it comes to POCOs. That same flow analysis should also apply to set.

required init is a different ballgame altogether. It does affect the compilers behavior since it would be an error to not initialize that property. But that doesn't prevent you from initializing it to null thus satisfying that requirement.

@andre-ss6
Copy link

andre-ss6 commented May 21, 2020

I don't think init is a replacement for set.

@canton7 Precisely, init is not a replacement for set. What you're proposing, however, would steer init further in that direction, as you would only get NRT warnings provided your property has a init accessor. No candy for set.

Please note I don't really have yet an opinion on this topic. I was just explaining to @HaloFour an adittional issue with his idea that was exposed by @333fred (correct me if I misinterpreted your point @333fred)

No? If a property is init, non-nullable, and doesn't have a default value, it's obviously intended that it should be set from the initializer.

(Emphasis mine)

@canton7 The consumer today has no way of knowing that a property has a default value. Yes, an attribute could solve that.

@markm77
Copy link

markm77 commented May 21, 2020

@HaloFour

What I am proposing has nothing to do with additional language features or behavior. It has to do with the flow analysis of NRTs, which already has a major gap when it comes to POCOs. That same flow analysis should also apply to set.

I guess I am nervous of special-casing the flow analysis to solve one problem and in a way I think could be a head-scratcher for many people.

But that doesn't prevent you from initializing it to null thus satisfying that requirement.

Well if you set a non-nullable to null you should obviously get a compiler warning as per normal.

@markm77
Copy link

markm77 commented May 21, 2020

Just to add @HaloFour , I sympathise. Using NRT with POCOs is hard work. I have in some cases had to make duplicate classes (with and without non-nullables) to work around issues.....

Basically I want to use NRT to tighten my code and be explicit with my types. I want to use initializers rather than constructors to offer friendly and flexible API syntax. I want to use immutable types and records where appropriate for data objects. But, even with C# 9 as announced, doing all these things together will not be fully possible. To my mind the biggest problems are solved via "required init" and "required set".

I suspect in the end you are having similar issues to me. Hopefully we can all find a solution.

I'm signing off so bye for now.

@canton7
Copy link

canton7 commented May 21, 2020

@markm77

You seen to think that I'm advocating against required set. I am not, as I have said very clearly several times.

I am looking at the issue in the OP. Forget about required properties for a minute, the problem in the OP is not about them. The problem in the OP is that you get unwanted warnings when declaring POCOs meant for serialisation.

Required properties came up because they are a larger feature which might address the problem in the OP. Fine. I'm not arguing against them. But forget about that potential solution for just a minute.

init properties are coming in C#9. They will interact with nullable warnings, like it or not. In my opinion the only sensible way for them to interact also solves the specific problem in the OP. Happy days.

If that is the case, then we can go and design required properties independently of this issue.

@andre-ss6
Copy link

andre-ss6 commented May 21, 2020

You seen to think that I'm advocating against required set.

Actually I don't even know what it is that you're advocating for and I'm indifferent to it. I was just replying to your message, since you tagged me.

Forget about required properties

The discussion about required properties came up. I've chimed in. End of story.

Required properties came up because they are a larger feature which might address the problem in the OP. Fine.

And I was trying to explain to @HaloFour that his suggestion has another potential obstacle evidenced by @333fred.

@canton7
Copy link

canton7 commented May 21, 2020

@andre-ss6 my bad, I thought you were part of that thread of conversation.

@markm77
Copy link

markm77 commented May 21, 2020

@canton7 For the record I don't think you are against required init/required set. My understanding is you want a solution for C# 9 and believe HaloFour's to be realistic and without negative effects. Fair enough position. 😊

@pablocar80
Copy link

Some of the examples discussed (string, booleans) have corresponding trivial values (empty string, false). The required init becomes more interesting for fields that are non-nullable references to classes.

@canton7
Copy link

canton7 commented May 21, 2020

@markm77 it's more that I think this solution falls naturally out of the only sensible interaction between NRTs and init properties. That it is in the C#9 timeframe doesn't rely on a tricky bit of design is a bonus 😀 (and it will be tricky to make required properties and reflection-based serializers, which is what the OP references, work nicely together I think).

I'm also trying to point out that this issue and required properties are more or less two different things, and we can consider them separately.

@Richiban
Copy link

@canton7 NRT is not going to help you if the property is a value type though, is it?

@canton7
Copy link

canton7 commented May 22, 2020

@Richiban We're talking about two different things. If the property is a value type, you won't get the nullable warnings that this issue is complaining about, so it's moot.

Required properties are different. It's correct that init properties + NRT don't cover the same cases as required properties.

The point I'm trying to make is that the specific warnings in the OP can, I think, be solved by init + NRT. Required properties are a separate (and worthy) issue, independent of the warnings which the OP is about, even though they were proposed as a solution to the warnings in the OP.

@chucker
Copy link

chucker commented May 22, 2020

I am looking at the issue in the OP. Forget about required properties for a minute, the problem in the OP is not about them. The problem in the OP is that you get unwanted warnings when declaring POCOs meant for serialisation.

I don't think that's right. The problem in the OP is not that they get unwanted warnings. It's that they want the warnings to appear at the callsite.

So this:

public class C
{
    public string Str { get; set; } // produces warning
}
...
var c = new C { Str = "Hello World" };
var d = new C(); // doesn't warn, but should

Becomes:

public class C
{
    [CallerInitialize]
    public string Str { get; set; } // no longer warns
}
...
var c = new C { Str = "Hello World" }; 
var d = new C(); // warns

Notice that C might be in a completely different assembly that you don't have control over.

Suggestions to add the ! suffix or #nullable disable miss the point. Of course that's possible, but you lose compile-time safety. Instead, we're hoping for a way to signal to the compiler that the warning should surface when constructing an instance of C, not when implementing C itself.

@canton7
Copy link

canton7 commented May 22, 2020

@chucker you didn't bold the words "When declaring POCOs", but they're important.

I agree with you: if you have a non-nullable init property, I'd expect a warning at the point of initialization if that property isn't set. Not at the point it's declared.

Please read my original post where I make this clear.

@chucker
Copy link

chucker commented May 22, 2020

you didn't bold the words "When declaring POCOs", but they're important.

Why? POCOs are just one example. Razor Components are another. Settings/options types (I guess those are arguably a form of POCO?) are yet another.

I agree with you: if you have a non-nullable init property, I'd expect a warning at the point of initialization if that property isn't set. Not at the point it's declared.

I don't agree with that.

A property doesn't have to be immutable (init;) for me to care that it will be set at initialization. While those two might have overlap, they're not the same. Taking Yair's example again:

public class C
{
    [CallerInitialize]
    public string Str { get; set; } // no longer warns
}

var c = new C { Str = "Hello World" }; 
c.Str = "Hello again!!"; // is fine; mutability is a largely separate issue

@olmobrutall
Copy link

I'm using Signum Framework, and we can declare an entity once and serves as DB entity model, DTO, and view model at the same time.

We abuse using not-nullable properties to mean mandatory in the UI and not-nullable in the database but really they could be null while you are filling the form. Example: https://github.com/signumsoftware/extensions/blob/master/Signum.Entities.Extensions/Notes/Note.cs

We also use a lot of object initializers, not only because the ORM requires a default constructor (it could be private), but because when an entity has more than say three or four properties a constructor becomes harder to read (what was the parameter number 4???) and also is more code that have to be written and maintained.

I love NRTs but CS8618 is annoying. So much that we have disabled it https://github.com/signumsoftware/extensions/blob/87e9470a970307339926a478c2ee1b97578fcbdb/Signum.Entities.Extensions/Signum.Entities.Extensions.csproj#L13

I agree that the new init; property in C# 9 doesn't really fix this problem. init means you can only set it in the initialisier, not that you must set it.

I like the CallerInitialize per property (or field) solution, but it would be even better if we could write an attribute in the class with a general rule and make it inheritable:

[InitializeOptions(memberWarning=InitWarning.None,  callerWarning=InitWarning.NotNullableReferenceType)]
public class C
{
    public C(string name)
    {
       this.Name = name; 
    }
    public string Name { get; set; } //No warn because constructor
    public string Str1 { get; set; } // Warns in caller because not nulalble reference type
    public string? Str2 { get; set; } // No warns because nullable
}

new C("John"); //Warning because  Str1 is Not nullable reference type and is not initialized in constructor

The options that I can see are:

  • InitWarning.All: Warns for all the not initialized members, even the nullable ones, because maybe you want to set the null explicitly.
  • InitWarning.NotNullable: Warns for all the not nullable members, including structs, because sometimes the default value (0, DateTime.MinValue, Guid.Empty, ...) is an invalid value anyway
  • InitWarning.NotNulalbleReferenceTypes: more common option Warns only for not nullable reference types because it breaks the type system.

With this feature we could have more fine control about what type-safety we want for different hierarchies of objects and enjoy object initializers without feeling guilty for not writing the constructor.

For example for DTOs I would use [InitWarning(memberWarning=InitWarning.None, callerWarning=InitWarning.All)] to never forget setting a property, while for Entities I maybe use [InitializeOptions(memberWarning=InitWarning.None, callerWarning=InitWarning.All)] because they are very often created incomplete and given to the user to fill the missing values in the UI.

Of course, when using reflection to create the instance (Activator.CreateInstance) all the required properties are empty and up to be set by some run-time magic like a deserializer ignoring InitWarning.

@Suchiman
Copy link

@YairHalberstadt Should be closed as championed in #3630 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests