SetsRequiredMembers - a pit of failure

[MgSam]

In the recent blog post announcing the required members feature- a number of the comments make the same point about the brittle nature of the [SetsRequiredMembers] attribute the C# team has settled on. The fact that checking a constructor sets all required members is entirely developer-dependent and lacks any static analysis backing it up seems to be an obvious pit of failure that will result in a common source of bugs for anyone relying on this feature.

To summarize the problem briefly:

V1:

class Foo 
{
     public required int Bar { get; init; }

    [SetsRequiredMembers]
    public Foo(int bar) 
    {
        Bar = bar;
    }
}

V2 - Boss wants us to add a new property "Baz"

class Foo 
{
     public required int Bar { get; init; }
     public required int Baz { get; init; } //Whew, that was easy

    [SetsRequiredMembers] //Whoops, now we are liars
    public Foo(int bar) 
    {
        Bar = bar;
    }
}

Although this example is trivial because the property declarations are right next to the constructor- they need not be. They could be 2000 lines away from each other, in a totally separate file, or even a totally separate class we're inheriting from and don't have the source code from.

In short, I think it would be a huge mistake to ship this feature in such a state. It is repeating the same problem nullability tracking has- that it's really more of a suggestion than anything concrete that can be relied upon at runtime.

If required cannot be enforced absolutely by the compiler or runtime the feature should be scrapped. Not only can it lead to the bugs shown above, but just imagine if tools like ORMs or client generators try to integrate requiredness into code generation. You end up propagating the potential bugs up and down the tech stack. A feature that works well only some of the time in simple scenarios is worse than no feature at all.

[HaloFour]

While I probably agree that at least a warning should be standard fare for the V2 version of the class, whether that is provided by the compiler or an analyzer that is on by default doesn't seem to be a big difference. It's always going to be incumbent on ORMs, deserializers or other tools based on reflection to respect the metadata, as it already is with init.

[foxesknow]

Where did you get this impression from?

I got that impression when @MgSam wrote his original post highlighting the issue and your response was:

If you are concerned about this class of bugs, then writing an analyzer for your domain needs seems reasonable.

So are you saying that there will be an analyzer that ensures definitive assignment of all required properties when [SetsRequiredMembers] is present? Because if that is what you're saying you've done a terrible job of communicating it.

[CyrusNajmabadi]

So are you saying that there will be an analyzer that ensures definitive assignment of all required properties when [SetsRequiredMembers] is present?

I don't know if there will be or won't be. As i've said, i would be totally in support of one being created for this purpose. If you are interested in contributing to that effort, i'm also happy to help out. :)

Because if that is what you're saying you've done a terrible job of communicating it.

You are inferring from my lack of knowledge if there will or won't be an analzyer for this that "this analyzer won't be shipping as part of the default tooling." Lack of knowledge doesn't mean something will or won't happen. It just means "i don't know". I do know that if you are interested in contributing efforts here, that greatly increases the chances of such a thing happening :)

[foxesknow]

So are you saying that there will be an analyzer that ensures definitive assignment of all required properties when [SetsRequiredMembers] is present?

I don't know if there will be or won't be.

Isn't C# 11 due for release in November? The fact you work on the language design team and don't know this is a bit concerning.

As i've said, i would be totally in support of one being created for this purpose. If you are interested in contributing to that effort, i'm also happy to help out. :)

Just out of interest, would you be helping out in your free time, like you're asking me to do, or would you be doing it during Microsoft's time, and thus getting paid to help out?

[CyrusNajmabadi]

Just out of interest, would you be helping out in your free time

I would be helping out in my free time. I try to do this for all contributors as i know they're having to do the same.

[CyrusNajmabadi]

Isn't C# 11 due for release in November? The fact you work on the language design team and don't know this is a bit concerning.

Language design and planning happens on different schedules and priorities than tooling designs and planning. The .net space is enormous and i don't think anyone particularly can hold it all in their heads. I'm sorry that i don't know everything going on :)

[CyrusNajmabadi]

Not only can it lead to the bugs shown above

If you are concerned about this class of bugs, then writing an analyzer for your domain needs seems reasonable.

A feature that works well only some of the time

All features only work some of the time. All of them have limitations and ways you can still have some classes of bugs. That's why things like analyzers exist. And that's why we still need to write tests.

[foxesknow]

Everyone will have a different definition of "correctly implement".

Will they? Is there any actual evidence of this? It just seems like your making vacuous statements to avoid the fundamental issue that the language design team are implementing half-baked idea.

[CyrusNajmabadi]

That applies to nearly all features. Effectively all features we have shipped have associated analyzers for them that at additional restrictions that some part of the community want.

I get you want the in-language feature to be exactly aligned with your needs. Everyone always wants that. However, with every release (including 1.0), it's never been the case that everyone gets that. The language will always not align with the needs and wants if some subset if the community. The virtue that we've seen with analyzers is that now at least you have recourse if your issue is that the language is too broadly accepting of things you think should be an error. You may not like that answer. But it's one that has worked well for millions for numerous cases. So there's no reason we're going to stop now given the success so far.

[HaloFour]

@foxesknow

It just seems like your making vacuous statements to avoid the fundamental issue that the language design team are implementing half-baked idea.

You seem to be the one ignoring the fact that the teams did investigate this and ran into numerous niche cases that became prohibitively expensive to solve.

Not to mention, such a statement is incredibly unprofessional and does not adhere to the code of conduct of this repository.

[CyrusNajmabadi]

Will they?

Yes. Definitionally. This issue is an example. Anders, for example, far preferred this approach. As did others.

The very fact that we're going this direction is precisely because it's felt to be the "right" choice. And we're absolutely know (even within the team) that that is not universal. Just like practically all the work we do.

[CyrusNajmabadi]

Is there any actual evidence of this?

Yes. 20 years of language design has presented this. The past was much worse honestly, back when the community wasn't part of the language design process. Back then there was outright superlative anger and grief over some of our decisions. And that was about features that are part and parcel of c# and used by the broad majority of the ecosystem (like lambdas).

[TahirAhmadov]

I would second the concern raised in the OP. I was under the impression this whole time that this attribute triggers a requirement that the required members are set. I'm extremely surprised by the fact that it does not.

Regarding writing a custom analyzer, this may or may not be a realistic expectation. If the Roslyn API that analyzers can utilize provides a helper to determine things like "look for stuff done under all possible control flows", then yes, it becomes straightforward to enforce that members are assigned. Otherwise, an analyzer author has to essentially re-write the code which the C# compiler already has to traverse all the possible control flows; this quickly becomes much more complicated.

WRT language design, I cannot think of any scenarios where it may be desirable to decorate the ctor with the attribute, and not fulfill that promise; this leads me to think that it should indeed be a warning, if not an error, in the language, and not an optional analyzer.

[TahirAhmadov]

To me, this doesn't seem like the kind of thing analyzers were made for. Like I wrote, the sticking point here is that I cannot envision the point of decorating with that attribute, and not fulfilling said promise. There are other places where analyzers are useful in some scenarios and not others, and I myself use some for my purposes, as needed. So I wouldn't take this hardline stance that "the LDT unjustifiably always says use analyzers"; but I do think a language rule is in order in this particular case.

[erik-kallen]

Why is this required feature even implemented? It could have been done with an analyzer!

(Please don't get the idea to cut it from me, I really feel it's needed, and I find the #nullable enable to be unusable for web applications without it)

[CyrusNajmabadi]

Like I wrote, the sticking point here is that I cannot envision the point of decorating with that attribute, and not fulfilling said promise

The point of decorating is to state the invariant afterwards. Similar to how NRT attributes are used to state the relations of things at the declaration point. These attributes then inform the consumption side for users (the primary case here of needing requirement to be satisfied).

[CyrusNajmabadi]

but I do think a language rule is in order in this particular case.

Thanks for that feedback. We'll see how the community feels and potentially make future language and compiler changes here.

[TahirAhmadov]

The point of decorating is to state the invariant afterwards. Similar to how NRT attributes are used to state the relations of things at the declaration point. These attributes then inform the consumption side for users (the primary case here of needing requirement to be satisfied).

I was just thinking about those; with NRT attributes, it works "both ways", say, [NotNullWhen(true)] informs that the out param is not-null when return is true, but it also warns inside the method if true is returned and the out param is set to maybe-null value. Whereas with SetsRequiredMembers, it does the "informing" but doesn't do the "warning".

[CyrusNajmabadi]

I was just thinking about those; with NRT attributes, it works "both ways", say, [NotNullWhen(true)] informs that the out param is not-null when return is true, but it also warns inside the method if true is returned and the out param is set to maybe-null value.

This is not true. You can trivially make a method that uses [NotNullWhen(true)] but still allows for the out-param to be null. We only have extremely minimal analysis here, and we give up very quickly.

[TahirAhmadov]

Oh yes, I didn't mean to say that the analysis with NRTs is perfect; obviously the compiler cannot guarantee it (even leaving aside the ! operator).

One gap which continues to haunt me is [NotNullIfNotNull("param")] string? Method(string? param) => null;. :)

This should be an easy one to analyze, right? (I know its effort, dev, test) But it's possible? (as opposed to tracking variable values)

However, going back to the SetsRequiredMembers, isn't that actually easier than the NRT ones to analyze? C# already enforces (by errors, no less) the definite assignment of variables; (not saying this attribute should produce errors necessarily) certainly the same logic can be re-used here, with a little bit of refactoring/modularization? That's why I was so surprised at the OP; my thinking was, they already have this analysis worked out, it's just adding an input to the existing analysis.

[CyrusNajmabadi]

However, going back to the SetsRequiredMembers, isn't that actually easier than the NRT ones to analyze?

No :). The constructor could call anything (including virtuals) that might set this. Internal setup of types is completely up to the author and may have complex, inter-method, relationships involved. NRT can at least look within a method. Required needs to understand the entire type hierarchy (which may not even be visible at that point).

[TahirAhmadov]

Oh OK, I thought the constructor body needs to set these members, not external methods. Perhaps it can be mandated that if the dev wants to move some work to a different method, then value or values (using a tuple) should be returned, so that properties are set in the ctor body explicitly? Then, we don't have to worry about all the stuff you wrote.

[TahirAhmadov]

Or, said methods should get their own attributes. Similarly to MemberNotNull, SetsRequiredMembers becomes SetsMembeb and parameter is string[], and the rule is, set it yourself, or call another method which has this attribute.

[CyrusNajmabadi]

and the rule is, set it yourself, or call another method which has this attribute.

That's an analyzer :)

[RikkiGibson]

I'm sympathetic about the difficulty around [SetsRequiredMembers] being unchecked. There are a few reasons we landed here that come to mind. (I'm using the word we for brevity, but I'm not actually on the language team.)

1. We want to permit indirect initialization patterns.
2. We don't want to introduce an interprocedural analysis to produce diagnostics.
3. We don't want to introduce a set of annotations a la [MemberNotNull] to describe the initialization patterns in use across methods.
4. [SetsRequiredMembers] is viewed to some degree as an escape hatch, which is meant to be used in scenarios where users need to break the rules of required members.

class C
{
    [SetsRequiredMembers]
    public C()
    {
        Helper();
    }

    public void Helper()
    {
        X = 42;
    }

    public required int X { get; set; }
}

It's possible that it might be more useful to give a warning in the above scenario to the effect of hey, we don't know if you set X here. Then users who want to use non-trackable initialization patterns can simply suppress the warning.

FWIW, we've also adjusted nullable analysis slightly here to provide a guardrail specifically when non-nullable reference types are in use.

#nullable enable

using System.Diagnostics.CodeAnalysis;

class C
{
    public C() // no warning
    {
    }

    [SetsRequiredMembers]
    public C(bool unused) // warning CS8618: Non-nullable property 'X' must contain a non-null value when exiting constructor.
    {
    }

    public required string X { get; set; }
}

[BillWagner]

I just wrote dotnet/docs#30845 to remind myself to address this.

[pinkfloydx33]

Isn't that at odds with Kathleen's comment on the blog post:

We expect it will be common to have constructors that do some part of the initialization work, but still require the user to set some fields. Thus, constructors are not assumed to set all values unless you state that they take this responsibility via the attribute.

If you have a constructor that sets some properties and the compiler isn't tracking which ones, then the consumer has to re-set the same properties in the object initializer. You could annotate the constructor but that would permit consumption without setting any properties in the initializer at all which seems dangerous.

If you only have a single constructor and it has parameters, I assume you (the developer) could just remove the required-ness of the properties in question as it kinda doesn't make sense to annotate them in that case.

[CyrusNajmabadi]

We considered that, but felt that the complexity to support that case wasn't worth it for the initial release of this feature. We'll use ecosystem feedback to determine if finer-grained tracking/marking is necessary.

[erik-kallen]

Is the pattern of calling a Helper() method to set members really common? Could we not say that if you do this, you get a false positive and have to disable the warning?

[KathleenDollard]

The great thing about doing language design in the open is that we can just tell you what happened. No need to imply we have magic wisdom. As always, I speak just for myself.

We initially agreed with having a definitive analysis of what the ctor sets. This required attributes of the type SetsMember. And then we started uncovering the niche cases. I know it can seem that we spend an inordinate amount of time on these and allow them to drive us, but that is what is required to have a coherent language. It's like a hole in a non-interlocked knit sweater. It will run and destroy the garment - unless it is protected by the way it is manufactured. Our job in maintaining C# as we expand it is to add that protection. This seemingly simple features has taken 3? years because of the way it interacts with the rest of C#, particularly your expectation of ctor not being self-contained, and type hierarchies.

We responded to niche after niche and built a dandy little DSL in attributes that still were not perfect and that did not pass internal language review. Let's just say that's an understatement. We were faced with two options: hold the feature again and try again or release the part of the feature we have confidence in and let it evolve. When looking at the first option, we sometimes ask ourselves "will we be smarter next year?" This time around we do not think so.

We have confidence that we need this feature to complete some nullable scenarios and to move forward in our object initialization theme.

We have confidence that we will always need a last-ditch escape hatch for the rare case that you just need to say "I got this." Imagine you are at a company that has many libraries, and you want to roll out required. SetsRequiredMembers is like turning nullable off for libraries to roll the change through your organization, and to avoid breaking changes, and you might not want required protection although your base class does.

SetsRequiredMembers indicates definite assignment, so you do get nullable protection. You do not get protection on value types.

It was time for us to release the feature and get feedback on where to go next. The feedback in this thread has been super useful. I love it when people push back on us - its important and I appreciate the professionalism that parallels your passion.

When @CyrusNajmabadi said "this should be an analyzer" some folks may have heard that as "just write your own analyzer". That is an important option for you to have, however it's not how LDT thinks about it. Who writes the analyzer is not the question, just whether it is part of the compiler (everyone gets it and we hope you think twice before turning it off) or an analyzer (you get a nice checkbox to set how you want this treated). The first category would be breaking in this case. Folks certainly use helper methods and base constructors that might not be available to static analysis to set values.

[KathleenDollard]

Analyzers lag behind language features. If Roslyn had an analyzer that gave you what you want today, what would it do? What would it look like? What would the attribute name be? Would it be useful if static analysis did not reach beyond the ctor's of the current class (we may not have access to the base)?

[BreyerW]

Im also concerned that this feature isnt properly enforced in all scenarios. If it is going to stay like that (and i expect them to stay that way forever due to several impossibilities outlined in that blogs comments) then required should be changed to attribute solution just like NRT is, since attributes are less restrictive about fulfilling promise in every scenario than language keywords

[CyrusNajmabadi]

then required should be changed to attribute solution just like NRT is,

NRT is both attributes and dedicated language syntax. Just like this feature is.

[hez2010]

We don't expect any form of inter-procedural analysis. But at least this should work:

var foo = new Foo(42);

class Foo 
{
    public required int Bar { get; init; }

    public Foo(int bar) 
    {
        Bar = bar;
    }
}

So that we can have the option to not use the SetsRequiredMembers. Unfortunately, we have no such option available.

The SetsRequiredMembers should only be considered as a mechanic for bypassing the definite assignment validation, which plays the same role with null forgiving operator. It should not be considered as a general solution to assigning required members in a constructor.

In other words, the current required members feature is half baked and doesn't play well with constructors at all.

[333fred]

It is very explicitly intentional that this example does not work. Required members are part of your public API, and C# never uses flow analysis to inform public API.

[John0King]

Why SetsRequiredMembers won't have a property called UnInitializedRequiredMembers eg.

class Foo 
{
     public required int Bar { get; init; }
     public required int Baz { get; init; } //Whew, that was easy

    [SetsRequiredMembers(UnInitializedRequiredMembers = new []{ "Baz" })] 
    public Foo(int bar) 
    {
        Bar = bar;
    }
}

and the information can be done automaticlly by complier

[CyrusNajmabadi]

As fred stated: Required members are part of your public API, and C# never uses flow analysis to inform public API.

[John0King]

it's using the SetsRequiredMembersAttribute now, just without known which required memeber was not initialized by the constructor.
a simple change to SetsRequiredMembersAttribute can make this feature better.

[colejohnson66]

@John0King See #6637 and #6536

[Ilchert]

I would like to add one more example of SetsRequiredMembers behavior.
If we have inheritance and base class has constructor with SetsRequiredMembers all child classes must have same constructor. So, we cannot use object initializer to initialize required properties normally.

var a = new A { PropA = "A" };
var b = new B(a) { PropB = "B" }; 
var b1 = new B(a); // no warning


class A
{
    public required string PropA { get; init; }

    [SetsRequiredMembers]
    public A(A a) => PropA = a.PropA;
    public A(){}
}

[method: SetsRequiredMembers] 
class B(A a) : A(a)
{
    public required string PropB { get; init; } // warning CS8618: Non-nullable property 'PropB' must contain a non-null value
}

Sharplab link

It would be ideal to make this code work:

var a = new A { PropA = "A" };
var b = new B(a) { PropB = "B" }; // Error CS 9035 

class A
{
    public required string PropA { get; init; }

    [SetsRequiredMembers]
    public A(A a) => PropA = a.PropA;

    public A() { }
}

class B(A a) : A(a) // Error CS 9039
{
    public required string PropB { get; init; }
}

Sharplab link