4

Having a required init property which sets a backing field still gives a null warning.

The below code gives a warning:

Warning CS8618 Non-nullable field '_name' must contain a non-null value when exiting constructor. Consider declaring the field as nullable.

public class TestRequiredInit
{
    private readonly string _name;

    public required string Name
    {
        get => _name;
        init => _name = value;
    }
}

nullable warning on field

I don't see a way to create TestRequiredInit without _name being set to a non null value. different ways trying to create the class which all gives warnings or errors

Is this a bug in MSBuild / VS, or am I missing something?

Update to prevent people from recommending using an auto property

I simplified the code above a bit for the purpose of asking a question. I want to be able to add initialization logic to the property initializer.

public class TestRequiredInit
{
    private readonly string _name;

    public required string Name
    {
        get => _name;
        init
        {
            if (value.Length > 50)
            {
                throw new ArgumentException();
            }

            _name = value;
        }
    }
}
Guru Stron
  • 102,774
  • 10
  • 95
  • 132
Joas
  • 1,796
  • 1
  • 12
  • 25
  • 2
    Does this answer your question? [Non-nullable property must contain a non-null value when exiting constructor. Consider declaring the property as nullable](https://stackoverflow.com/questions/67505347/non-nullable-property-must-contain-a-non-null-value-when-exiting-constructor-co) – Vivek Nuna Nov 28 '22 at 12:10
  • @viveknuna No, it doesn't, this is a different case with `required init`. I understand the warning. It shouldn't be showing it in this case as `_name` can not have a null value after initialization. – Joas Nov 28 '22 at 12:13
  • 1
    I wouldn't call this a bug. This code gets compiled to a constructor call followed by a property assignment, so semantically, it really is possible to observe `_name` being `null` (at least inside the object). Requiring you to write `= null!` to acknowledge this seems like good policy. (Of course, for this trivial case you can just use an auto-property, which also avoids the problem.) – Jeroen Mostert Nov 28 '22 at 12:14
  • That's just how required properties work now. You might disable checking for nulls, but that's a bad idea. – Rafał Kopczyński Nov 28 '22 at 12:17
  • @JeroenMostert That doesn't make sense. Could you show me the code which can compile without warnings of `_name` being null without using the `!`? It doesn't exist as far as I can see. Using `= null!` defeats the entire purpose of the nullable reference type check. If the property is removed I want it to show the warning. – Joas Nov 28 '22 at 12:17
  • 2
    I could see the implementation going either way on this. From a C# perspective it is indeed impossible to observe the `null` state, as the `required` "forces" well-behaved clients to initialize the property, and therefore the fact that it remains `null` after the constructor call is immaterial. Anything you could do from the object to schedule an action that runs after the constructor would normally be unsafe anyway, in terms of being able to observe a partially constructed object. From a pure IL point of view the value is nevertheless `null` after invoking the constructor. – Jeroen Mostert Nov 28 '22 at 12:25
  • @JeroenMostert That makes sense, but the compiler shouldn't be looking at it from a pure IL point of view. If it would be doing that, a lot of other C# features shouldn't be working right. – Joas Nov 28 '22 at 12:27
  • 2
    Some more background on interaction with nullability can be found in the [feature proposal](https://github.com/dotnet/csharplang/blob/main/proposals/csharp-11.0/required-members.md#effect-on-nullable-analysis) (not specifically mentioning separate backing fields, though). – Jeroen Mostert Nov 28 '22 at 12:30
  • One thing that would definitely complicate this is the requirement to know that the `init` setter of `Name` assigns a non-`null` value to the backing field. This is easy enough to conclude in this trivial declaration, but setters can be arbitrarily complicated. The nullability analysis would somehow need to know that `_name` is set through a `required` `init` setter, *any* `required` `init` setter, which is far more complicated than enforcing that the constructor assign it, especially when multiple properties get involved. Not undoable, but possibly introducing more complication than desired. – Jeroen Mostert Nov 28 '22 at 12:40
  • 2
    And that in turn also allows me to give an example of what you demanded in terms of unsafe code: consider two properties with setter logic that depends on the value of the backing field of the other property. These setters may see `null` values of either field, despite the fields being declared non-nullable. This is true especially because `required` enforces initialization, but not an order of that initialization. Of course such setters are contrived and undesirable, but they can't exactly be forbidden. – Jeroen Mostert Nov 28 '22 at 12:48
  • @JeroenMostert The complexity for the c# language team is a good argument. I can not speak to that and it might be the reason that this warning is being shown. I've also posted this question on the github csharplang discussion. Maybe they can give us more insight. https://github.com/dotnet/csharplang/discussions/6752 – Joas Nov 28 '22 at 14:43
  • @JeroenMostert The order of the initialization is a good insight, however, I do not think it applies here. As in that case I would simply expect the body of the initializer to show a warning that the referenced property may be null (as we do not know if the referenced property's initializer has already ran). This warning explicitly states that this property will not have a value after initialization, which is simply not true. – Joas Nov 28 '22 at 14:45
  • 1
    My point was specifically about accessing the *backing field*. There is no referenced property in my scenario and no way of knowing if the field has been assigned or not, if we are no longer allowed to assume the constructor did it. The warning also says nothing about properties or initialization -- it says the *field* must have a value after *exiting the constructor*, no more, no less. – Jeroen Mostert Nov 28 '22 at 14:57
  • 2
    There are two key takeaways here: first, there is no formal link between properties and their manually declared backing fields (if they even have any) so fields are analyzed in isolation of whatever properties happen to be, and second, `init` members are not considered part of construction. Extending them special status so they are (in a limited way, for analysis purposes only) is possible in principle but comes with its own caveats. – Jeroen Mostert Nov 28 '22 at 14:59
  • @JeroenMostert That's a good point. The warning does not say anything about property initializers. Just specifically states constructor. I guess I simply assumed that initializers would be part of that. I would still recommend considering changing this warning to also look at initializers, but that's up to the C# team I guess. If you post the part about the language of the warning vs initializers as an answer I will accept it. – Joas Nov 28 '22 at 15:25

1 Answers1

4

There is no formal link between properties and their backing fields (if they even have any, which is not required), so the nullability of _name is by necessity analyzed in isolation of Name. What's more, while init setters are considered to happen as part of the construction phase (so they are allowed to do things like initialize other init properties) they are not formally part of construction for nullability analysis purposes -- which was correct up until the introduction of required, since there was no requirement to call them.

Nullability analysis could be extended to specifically consider init setters of required properties to be part of construction for analysis purposes, so the Name init setter would be considered to leave _name in a known non-null state, but getting this right is not trivial -- for example, because we can't assume any order in which the init setters are called, we should assume inside every init setter that fields might be null if the constructor doesn't definitely assign them not null, so we can give appropriate warnings. This is an inversion of the current semantics, where setters are allowed to assume fields are not null, since they can currently rely on the constructor having done so, gated by the existing warnings. It gets even hairier if the setter of a required property is not an init setter, since it might be called both during and after construction -- should we always warn just to be on the safe side?

All that and I haven't even considered how this interacts with derived classes. I'd definitely go with "this might be considered a bug, but formally defining the desired semantics and implementing them with as few surprises as possible is quite a bit of work, which has not yet been undertaken".

Jeroen Mostert
  • 27,176
  • 2
  • 52
  • 85
  • 1
    Microsoft has created a language proposal based on the discussion I created in their github: https://github.com/dotnet/csharplang/issues/6754 They will have the analyzer respect the `MemberNotNullAttribute` – Joas Nov 29 '22 at 09:29