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

Suppress nullable property/field initialization warning for required members #60243

Merged
merged 3 commits into from
Mar 25, 2022

Conversation

333fred
Copy link
Member

@333fred 333fred commented Mar 17, 2022

Test plan #57046

@333fred 333fred requested a review from a team as a code owner March 17, 2022 22:09
@333fred
Copy link
Member Author

333fred commented Mar 17, 2022

@RikkiGibson @jcouv for review.

{
}

public C(int _) {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider testing a constructor with [SetsRequiredMembers].

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fine. Let's just add the test at the appropriate time to ensure the expected behavior is recorded.

@RikkiGibson
Copy link
Contributor

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.

@RikkiGibson RikkiGibson self-assigned this Mar 18, 2022
@RikkiGibson
Copy link
Contributor

RikkiGibson commented Mar 18, 2022

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?
    }
}

@jcouv
Copy link
Member

jcouv commented Mar 24, 2022

Added Area-Compilers label (that's why I hadn't noticed this PR)


In reply to: 1077939493

@jcouv jcouv self-assigned this Mar 24, 2022
@333fred
Copy link
Member Author

333fred commented Mar 24, 2022

@RikkiGibson please take another look.

@@ -811,46 +817,43 @@ void makeNotNullMembersMaybeNull()
{
if (method.IsConstructor())
{
if (needsDefaultInitialStateForMembers())
foreach (var member in getMembersNeedingDefaultInitialState())
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may want to review this section with whitespace turned off.

@333fred
Copy link
Member Author

333fred commented Mar 24, 2022

@RikkiGibson @jcouv for another look please.

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM Thanks (iteration 3). Adding comment would be appreciated. Nits up to you.

@333fred 333fred merged commit f2d1b99 into dotnet:features/required-members Mar 25, 2022
@333fred 333fred deleted the nullability branch March 25, 2022 17:03
@333fred
Copy link
Member Author

333fred commented Mar 25, 2022

I'll address the nits in the next PR

@333fred 333fred added the New Feature - Required Members Required properties and fields label Sep 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants