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

False positive CS8618 null reference type warning when using required members and [SetsRequiredMembers] in derived class #74423

Closed
SapiensAnatis opened this issue Jul 17, 2024 · 2 comments · Fixed by #76363
Assignees
Labels
4 - In Review A fix for the issue is submitted for review. Area-Compilers Feature - Required Members Required properties and fields
Milestone

Comments

@SapiensAnatis
Copy link

SapiensAnatis commented Jul 17, 2024

Version Used:

Compiler version: '4.10.0-3.24314.14 (259e82e9)'. Language version: 12.0., but also reproduces on main (11 Jun 2024) on Sharplab.

Steps to Reproduce:

  1. Compile the following code (sharplab):
using System.Diagnostics.CodeAnalysis;

public abstract class Base
{
    [SetsRequiredMembers]
    public Base(string @string)
    {
        this.String = @string;
    }
    
    public required abstract string String { get; init; }
}

public class Derived : Base
{
    [SetsRequiredMembers]
    public Derived(string @string) : base(@string)
    {
    }

    public override required string String { get; init; }
}

Diagnostic Id:

CS8618: Non-nullable property 'String' must contain a non-null value when exiting constructor. Consider declaring the property as nullable.

Expected Behavior:

No diagnostics, because the base class sets the value.

Actual Behavior:

A diagnotic on the constructor of Derived.

Additional Information

If I remove the : Base from Derived and make it an independent class, I get the same warning. This leads me to suspect this is a case of the nullable reference type analysis not being sophisticated enough to determine I have set String in the base constructor. I am aware that NRT analysis doesn't support every pattern under the sun and there are many ways you can 'trick' it. If this is one of those cases, I apologize - but I thought it was worth mentioning all the same.

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Jul 17, 2024
@jaredpar
Copy link
Member

@333fred for comment as I'm unsure what the correct behavior is here.

@333fred
Copy link
Member

333fred commented Jul 22, 2024

I told them to open this bug. It doesn't seem like we're correctly handling the virtual property here.

@jaredpar jaredpar added the Feature - Required Members Required properties and fields label Oct 22, 2024
@jaredpar jaredpar added this to the 17.13 milestone Oct 22, 2024
@jcouv jcouv removed the untriaged Issues and PRs which have not yet been triaged by a lead label Nov 12, 2024
@arunchndr arunchndr modified the milestones: 17.13, 17.13 P2 Nov 13, 2024
@jaredpar jaredpar modified the milestones: 17.13 P2, 17.13 Dec 2, 2024
333fred added a commit to 333fred/roslyn that referenced this issue Dec 10, 2024
…ement

Fixes dotnet#74423. When we're enforcing that all members of a type must have non-null values for `SetsRequiredMembers`, we don't want to include any members of the current type that are overrides of a base type. Instead, those members should be enforced when we check the chained constructor call, since the chained constructor may itself have `SetsRequiredMembers`, and therefore already enforced that overridden properties have a valid state.
@333fred 333fred added the 4 - In Review A fix for the issue is submitted for review. label Dec 10, 2024
333fred added a commit to 333fred/roslyn that referenced this issue Dec 12, 2024
Fixes dotnet#74423. When we're getting the list of members to default initialize, we look for properties that override `abstract` properties and have nullable annotations that line up with the base ones. For these properties, we can assume that if we chain to a `SetsRequiredMembers` constructor, it took care of setting a non-null state.
333fred added a commit that referenced this issue Dec 16, 2024
Fixes #74423. When we're getting the list of members to default initialize, we look for properties that override `abstract` properties and have nullable annotations that line up with the base ones. For these properties, we can assume that if we chain to a `SetsRequiredMembers` constructor, it took care of setting a non-null state.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 - In Review A fix for the issue is submitted for review. Area-Compilers Feature - Required Members Required properties and fields
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants