Allow object initialization to work with nullable analysis

[erik-kallen]

Background

I have just enabled nullable reference types, and they do seem to work fine in many cases. However, when it comes to members, they mostly work fine for types that are constructed using a constructor that sets all members based on parameters. At least in our codebase, that is actually a small minority of types. Most types are constructed with inline initialization or with deserialization.

A very common use-case for us is that service A creates a request object, serializes it to JSON, posts it to service B that deserializes it and uses it, eg:

// Common.cs
class UserSearchRequest {
    public string Company { get; set; }
    public string Department { get; set; }
    public string LastName { get; set; }
    public string? FirstName { get; set; }
    public string? HairColor { get; set; }
}

class UserSearchResponse
{
    public int UserId { get; set; }
}

// ServiceA.cs
UserSearchResponse FindUser(string company, string department, string lastName, string? firstName)
{
    var req = new UserSearchRequest
    {
        Company = company,
        Department = department,
        // LastName = lastName, Oops I accidentally commented out this line
        FirstName = firstName,
    };
    var reqStr = JsonSerializer.Serialize(req);

    var responseStr = HttpPost("http://service-b.com", reqStr);

    return JsonSerializer.Deserialize<UserSearchResponse>(responseStr);
}

// ServiceB.cs
UserSearchResponse HandleFindUser(string json)
{
    var req = JsonSerializer.Deserialize<UserSearchRequest>(json);
    var id = LookupInDatabase(req.Company, req.Department, req.LastName, req.FirstName, req.HairColor);
    var result = new UserSearchResponse { UserId = id };
    return JsonSerializer.Serialize(result);
}

int LookupInDatabase(string company, string department, string lastName, string? firstName, string? hairColor)
{
    ...
}

The task is to get this code to compile without warnings, and without losing the benefits of nullability analysis.

Issues with the original (unmodified) code

The code gives warnings CS8618: Non-nullable property 'X' is uninitialized for Company, Department, and LastName in the UserSearchRequest. Additionally, due to my accidentally commented out line, there will be an NRE when ServiceB eventually tries to access req.LastName, but that is to be expected since I had a nullable warning.

Potential fix 1: #nullable disable warnings

Of course, an easy "fix" is to just surround the definition of UserSearchRequest with #nullable disable warnings.

Pros:

• The warning goes away

Cons:

• Removes the benefits of nullability analysis. I get no indication that I will have an NRE in ServiceB.HandleFindUser()

Potential fix 2: Make all properties nullable (as the message suggests)

Pros:

• The warning goes away
• There is now a warning that alerts me that I might get an NRE in ServiceB.HandleFindUser()

Con:

• I have lost the automatic, implicit documentation of which properties are required and which are optional
• There will be a lot of compiler warnings for the fields that are not supposed to be null. This means that I will end up doing something like

    var id = LookupInDatabase(
        req.Company ?? throw new InvalidOperationException(),
        req.Department ?? throw new InvalidOperationException(),
        req.LastName ?? throw new InvalidOperationException(),
        req.FirstName,
        req.HairColor);

Sure, my NullReferenceException will be replaced by an InvalidOperationException, but I have received no help from the compiler to find my accidentally commented out line. Also, since this is a lot of typing (and I "know" which fields must be set), I will probably end up doing just

    var id = LookupInDatabase(req.Company!, req.Department!, req.LastName!, req.FirstName, req.HairColor);

in which case I am no different off than if I just went with the #nullable disable warnings fix

Potential fix 3: Create a non-default constructor

A third option is to add a constructor that initializes the required members:

class UserSearchRequest {
    public string Company { get; set; }
    public string Department { get; set; }
    public string LastName { get; set; }
    public string? FirstName { get; set; }
    public string? HairColor { get; set; }

    public UserSearchRequest(string company, string department, string lastName)
    {
        Company = lastName;
        Department = lastName;
        LastName = lastName;
    }
}

Pros:

• I can't accidentally forget the commented-out line. Once I do the refactorings and the code compiles without warnings, my NRE is gone.

Cons:

• I need to refactor all my request classes and usages of them. This is a non-trivial effort (and changing code always comes with the risk of something breaking)
• I just added some boilerplate. C# has done a great job reducing boilerplace since 1.0, so it seems wrong that this (at least in code I have worked on very common) pattern should need more boilerplate.
• I now have an assymetry where some members need to be parameters to the constructor and some are set in the object initializer. Sure, I can replace the constructor with

public UserSearchRequest(string company, string department, string lastName, string? firstName = null, string? hairColor = null)

to remove this assymetry, but it comes with the price of even more boilerplate.

• While it does guard against the NRE, I have now introduced the risk of transposing arguments (thus ending up using the company as the department and vice versa), which is arguably a harder bug to find than the original NRE. This risk can be mitigated by always using named constructor arguments, but it requires discipline from the entire team.

• I actually just silently broke the program! System.Text.Json.JsonSerializer cannot use non-default constructors.

So in order for me to get rid of all warnings, have symmetry for all members, be able to deserialize successfully, and have nullable protection, my class would need to be something like

class UserSearchRequest {
    public string Company { get; set; }
    public string Department { get; set; }
    public string LastName { get; set; }
    public string? FirstName { get; set; }
    public string? HairColor { get; set; }

    public UserSearchRequest(string company, string department, string lastName, string? firstName, string? hairColor)
    {
        Company = lastName;
        Department = lastName;
        LastName = lastName;
        FirstName = firstName;
        HairColor = hairColor;
    }

#nullable disable warnings
    [Obsolete("Serialization only", true)]
    public UserSearchRequest()
    {
    }
#nullable restore warnings
}

This is most likely doable, but it is a ridiculous amount of boilerplate, not to mention that all initializations of these request classes must be updated to use constructor syntax.

Proposed feature: NullableUninitializedAttribute

I propose creating an attribute called NullableUninitializedAttribute. When a constructor has this attribute, it means that CS8618 is not generated for the constructor. But it means that CS8618 (or some other warning) is generated wherever this constructor is invoked, unless all non-nullable properties and fields are initialized in an object initializer. So relevant parts of my example become:

class UserSearchRequest {
    public string Company { get; set; }
    public string Department { get; set; }
    public string LastName { get; set; }
    public string? FirstName { get; set; }
    public string? HairColor { get; set; }

    [NullableUninitialized]
    public UserSearchRequest()
    {
    }
}

...

    var req = new UserSearchRequest // CS8618: Non-nullable property 'LastName' is uninitialized...
    {
        Company = company,
        Department = department,
        // LastName = lastName, Oops I accidentally commented out this line
        FirstName = firstName,
    }

For some slightly improved ergonomics, it could also be possible to add the attribute to the type:

[NullableUninitialized]
class UserSearchRequest {
    public string Company { get; set; }
    public string Department { get; set; }
    public string LastName { get; set; }
    public string? FirstName { get; set; }
    public string? HairColor { get; set; }
}

which means that the attribute is applied to the automatically generated default constructor.

It should still be possible to specify this attribute on only some constructors:

class UserSearchRequest {
    public string Company { get; set; }
    public string Department { get; set; }
    public string LastName { get; set; }
    public string? FirstName { get; set; }
    public string? HairColor { get; set; }

    // CS8618 because this constructor does not have the [NullableUninitialized] attribute
    public UserSearchRequest(string company, string department, string lastName) 
    {
    }

    [NullableUninitialized]
    public UserSearchRequest() {}
}

Inheritance

If a constructor in a derived class invokes a [NullableUninitialized] constructor, a CS8618 is generated for any nullable member of the base type that is not definitely assigned in the derived constructor (unless the derived constructor is also marked with [NullableUninitialized]).

class UserSearchRequest {
    public string Company { get; set; }
    public string Department { get; set; }
    public string LastName { get; set; }
    public string? FirstName { get; set; }
    public string? HairColor { get; set; }

    [NullableUninitialized]
    public UserSearchRequest() {}
}

class ExtendedUserSearchRequest : UserSearchRequest
{
    public ExtendedUserSearchRequest(string company, string department)
    {
        Company = company;
        Department = department;
    }  // CS8618: Non-nullable property 'LastName' is uninitialized
}

Possible nice-to-haves

Some initialized members

Perhaps it would be useful to specify that some members are initialized, but some need to be initialized by the caller:

class UserSearchRequest {
    public string Company { get; set; }
    public string Department { get; set; }
    public string LastName { get; set; }
    public string? FirstName { get; set; }
    public string? HairColor { get; set; }

    [NullableUninitialized(Except = new[] { nameof(Company) }]
    public UserSearchRequest(string company)
    {
        Company = company; // If I had forgotten this line, I would have got CS8618
    }
}

Automatic insertion of this attribute

When the compiler today analyzes a constructor to determine which members to generate CS8618 for, it could instead use this information to automatically populate a [NullableUninitialized] attribute for the constructor. I guess this would be a breaking change since it would change where error messages appear, but I think it would be a great improvement for nullable adoption since object initialization is a very common pattern.

Related issues and why I chose to create this issue even though they exist:

#2109: Not a very detailed proposal, and it is also buried.
#2328: I think the attribute belongs to constructors rather than the individual members. Also, that issue is buried with no reply since Jun 5.
#2825: I had already started writing this proposal when that issue was opened, and I think I provide more of a solution to the problem than that issue.

[HaloFour]

unless all non-nullable properties and fields are initialized in an object initializer.

I would also like to see flow analysis be able to track which fields have been set prior to the instance escaping the method:

public void M1() {
    var r = new UserSearchRequest {
        Company = "Foo",
        Department = "Bar"
    }; // no warning for not setting LastName here
    
    // do some stuff here

    M2(r); // warning, passing instance to a method before setting LastName
    r.LastName = "Baz";
    M2(r); // no warning
}

[YairHalberstadt]

I would also like to see flow analysis be able to track which fields have been set prior to the instance escaping the method:

Note that the compiler already does such analysis for fields of structs. According to the spec, once all fields of a struct are assigned, it is considered definitely assigned:

(string, string) t;
t.Item1 = "";
t.Item2 = "";

// t is definitely assigned here

So this is actually consistent with the language.

[erik-kallen]

Yes, that would make it even better! But I guess it could be done in a second step if necessary (I don't suppose removing spurious warnings could count as breaking changes)

[nillkitty]

I completely agree with this issue; I have several classes where members are initialized using reflection (similar to the Json deserialization in your example) and have the same issue.

[JoshuaNitschke]

I think this is a great idea, I have the same issue with the implementation of nullable reference types. The only solution that will work for me is Potential fix 3: Create a non-default constructor and that is just too much boiler plate. I'm going to revert to disabling nullable references for now.

[Kinematics]

I have a similar issue, but with dependency injection. Some information isn't known at the time of object construction, so it must be provided in a separate initialization step. But that again leads to a "What is allowed to be null?" scenario. (Aside: Some DI frameworks allow passing parameters when setting up, but Microsoft's does not.)

Further, some of the init values are intended to be readonly. However I can't set readonly fields or get-only properties outside the constructor, so I end up with private setters and juggling some sort of Initialized gate variable.

Conceptually, I would like write-once properties, as opposed to readonly properties, and that the instance object only be considered valid once all the values that need to be initialized have been. The initialization may happen in the constructor, or by setting properties during object creation, or by some initialization function. Of course, that raises the question of how you can call an initialization function when the object itself isn't considered valid, but perhaps that can be resolved with attributes or a variant setter keyword.

[InitializableOnce]
public string MyLateBindProperty { get; }
//or
public string MyLateBindProperty { get; oninit; }

[CallableWhenUninitialized]
public void Initialize(string stuff)
{
    MyLateBindProperty = stuff;
}

And that finishes up setting any uninitialized properties. Basically, Initialize has the same setter permissions as the constructor, but by having it exist, that object isn't valid until it is called, and would fail the same flow analysis as simple null flow checking. From that point on, though, the instance object would be considered valid – the equivalent of "not null" in flow analysis.

[ericsampson]

See #3630

[wizofaus]

Not sure why the attribute is needed. What's the advantage of the current mechanism where you must initialise as part of the declaration in order to avoid the warning? (and in no case can you seem to get a warning if you don't provide values as part of the initializer list when constructing?)

[333fred]

Have you seen required members from C# 11? https://learn.microsoft.com/en-us/dotnet/csharp/whats-new/csharp-11#required-members

[wizofaus]

Yep, thanks, and I agree it makes this request redundant.
Though there is some contention around what the expected behaviour might be when constructing via reflection (as, e.g. the Newstonsoft.Json package will do for you when deserializing from JSON, potentially leaving required members null).