Suppress nullable property/field initialization warning for required members#60243
Conversation
|
@RikkiGibson @jcouv for review. |
| { | ||
| } | ||
|
|
||
| public C(int _) {} |
There was a problem hiding this comment.
Consider testing a constructor with [SetsRequiredMembers].
There was a problem hiding this comment.
This isn't implemented yet. That's next PR. However, SetsRequiredMembers will not affect behavior here: LDM made the decision that SetsRequiredMembers does not ensure that required members are set in the constructor, so no nullable warnings should be given either.
There was a problem hiding this comment.
That's fine. Let's just add the test at the appropriate time to ensure the expected behavior is recorded.
|
Was it considered, instead of skipping checking the flow state on exit, to behave as though the property is being assigned with something "valid" at the exit point? Curious if there's appetite to support scenarios like the following: class C
{
private string _field;
public required string Field { get => _field; [MemberNotNull(nameof(_field))] set => _field = value; }
public C() { } // no warnings
}It's fine if this isn't of interest to the language team, or if it's a "stretch goal" so to speak that might get added later as time permits, but thought I would check. |
|
Also curious about what should happen in this scenario: public class C
{
public required string Prop { get; set; }
public C(bool unused) { }
public C() : this(true)
{
Prop.ToString(); // warning?
}
} |
|
Added Area-Compilers label (that's why I hadn't noticed this PR) In reply to: 1077939493 |
|
@RikkiGibson please take another look. |
| if (method.IsConstructor()) | ||
| { | ||
| if (needsDefaultInitialStateForMembers()) | ||
| foreach (var member in getMembersNeedingDefaultInitialState()) |
There was a problem hiding this comment.
You may want to review this section with whitespace turned off.
src/Compilers/CSharp/Test/Symbol/Symbols/RequiredMembersTests.cs
Outdated
Show resolved
Hide resolved
|
@RikkiGibson @jcouv for another look please. |
jcouv
left a comment
There was a problem hiding this comment.
LGTM Thanks (iteration 3). Adding comment would be appreciated. Nits up to you.
|
I'll address the nits in the next PR |
Test plan #57046