Roslyn: C#8 with nullable compiler only checks code in constructors before producing CS8618 warning

[vsfeedback]

The following code snippet initializes both member variables, but, when compiled with C# 8 with nullable enabled, produces the warning:

warning CS8618: Non-nullable field 'm_str1' is uninitialized.

    // Demonstrates CS8618 doesn't check methods called from the constructor for initialisation
    class MyClass
    {
        private string m_str1;
        private string m_str2;

        public MyClass()    // warning CS8618: Non-nullable field 'm_str1' is uninitialized.
        {
            Init();
            m_str2 = string.Empty;  // No warning for m_str2
        }

        private void Init()
        {
            m_str1 = string.Empty;
        }
    }

To repro, just paste the snippet into a C# console project and set it to build with the V8 compiler with #nullable enable set.

_This issue has been moved from https://developercommunity.visualstudio.com/content/problem/412341/c8-with-nullable-compiler-only-checks-code-in-cons.html
VSTS ticketId: 754367_
_These are the original issue comments:_
(no comments)
_These are the original issue solutions:_
(no solutions)

[agocke]

We've decided not to track initialization interprocedurally for now, but may reconsider later if it turns out to be extremely common and we think there's a tractable implementation.

[DjArt]

Same warning with inherited constructor:

    public sealed class SpecialMethod<T>
        where T : Delegate
    {
        internal const string M_OWNER = nameof(Owner);
        internal const string M_METHOD = nameof(Method);
        internal const string M_METHODINVOKED = nameof(MethodInvoked);

        private T? _Delegate;
        private readonly T _Method;

        public event MethodInvokeHandler<T> MethodInvoked;

        public T Method
        {
            get => _Method;
            set => _Delegate = value;
        }

        public SpecialMethod()
        {
            _Method = DelegateHelper.CreateDelegate<T>(DelegateHandler);
        }
        //here
        public SpecialMethod(T @delegate) : base()
        {
            _Delegate = @delegate;
        }

        private object? DelegateHandler(object[]? parameters)
        {
            object? result = _Delegate?.DynamicInvoke(parameters);
            MethodInvoked?.Invoke(this, result, parameters);
            return result;
        }
    }

[jtbrower]

I am running into this issue frequently. We like to break up constructor initialization into functions with a descriptive name. We are forced to ignore the error throughout code yet honestly feel wrongfully compelled to place all of the code as one chunk into the constructors just to avoid the warnings.

By not tracking procedural initialization you are indirectly encouraging an opinionated coding style that lumps all of the code directly into the constructor. I should note that the initialization routines are being called directly from the constructor itself, its not as if we are asking for tracking through another call chain. Ending on a good note, I appreciate all you guys do.

[ddshpak]

Just chiming in to agree with @jtbrower -- my organization loves nullable reference feature, and enabling warnings has already revealed a couple subtle design flaws that we've corrected before they turned into defects. But this specific issue is requiring us to decide between:

• Changing our code design to something we like less, occasionally resulting in code duplication
• Accepting these warnings during compilation, possibly masking new ones when they appear
• Disabling this warning entirely

...and we don't really like any of them :(

We're going to keep using nullable references _anyway_ because we see so much value in this feature, but if this could be addressed it would be even better :)

[skarllot]

There are two solutions to mitigate this issue:

• The method return the value instead of directly set field:

class MyClass
{
    private string m_str1;
    private string m_str2;

    public MyClass()
    {
        m_str1 = InitStr1();
        m_str2 = string.Empty;
    }

    private string InitStr1()
    {
        return string.Empty;
    }
}

• The method initialize fields using out parameters:

class MyClass
{
    private string m_str1;
    private string m_str2;

    public MyClass()
    {
        Init(out m_str1, out m_str2);
    }

    private void Init(out string str1, out string str2)
    {
        str1 = string.Empty;
        str2 = string.Empty;
    }
}

[MichaelPeter]

This is also one of the most annoying things with nullables. So far my workarround is this:

#pragma warning disable CS8618 // Disable constructor variable init initalized warnings.
Ctor1
Ctor2
#pragma warning restore CS8618

[RikkiGibson]

MemberNotNullAttribute can now handle some of these scenarios. See dotnet/runtime#31877

[Ramobo]

As it becomes clearer that I'll need this, if cross-method analysis doesn't happen, how about this compromise: The compiler automatically applies [MemberNotNull] in nullable contexts. If necessary, make it a compiler option like nullable itself. For example:

public class MyClass
{
    private string _string1;
    private string _string2;
    private string? _string3;

    public MyClass()
    {
        Initialize();

        _string2 = string.Empty;
    }

    [MemberNotNull(nameof(_string1))] // Automatically applied by the compiler.
    private void Initialize()
    {
        _string1 = string.Empty;
        _string3 = string.Empty; // No point in applying the attribute for this one since it's nullable.
    }
}

[avo32768]

I ended up here due to the same problem. I think it'd be great to have an attribute for that, e.g.:

#nullable enable

class Test 
{
    object _nonNullableRef;

    // I wish this SuppressMessage would work!   
    [System.Diagnostics.CodeAnalysis.SuppressMessage("Compiler", "CS8618")] 
    public Test() { Initialize(); }

    void Initialize() { _nonNullableRef = new Object(); }
}

IMHO, it's much better than #pragma warning disable CS8618.

[RikkiGibson]

Regarding https://github.com/dotnet/roslyn/issues/32358#issuecomment-653699452: There is a use case for applying [MemberNotNull(nameof(_string3))]. Consider the following:

using System;
using System.Diagnostics.CodeAnalysis;

class Widget
{
    string? _string3;

    [MemberNotNull(nameof(_string3))]
    void Init() { _string3 ??= "default"; }

    void M()
    {
        Init();
        Console.Write(_string3.GetHashCode()); // the attribute on Init() prevents a warning here
    }
}

I do think that we should consider how we can improve the "Init() method" scenario, where multiple constructors all call into the same Init member method.

[myblindy]

Or when a constructor calls a get-or-reallocate-larger-buffer-and-get scenario, where otherwise the get portion would have to be duplicated.

MemberNotNull works here too, but it's at best clunky, at worst error prone -- the point of code analysis is that if someone ever removes the initialization from the inner method I automatically get a notification, not that it won't trigger because at some point it was there.

[RikkiGibson]

Could you please clarify how MemberNotNull is error prone? The compiler does analyze and warn about the implementations of methods marked with the attribute.

[MisinformedDNA]

@RikkiGibson I think @myblindy means something like this:

Initial code

    [MemberNotNull(nameof(_string3))]
    void Init() { _string3 ??= "default"; }

Altered code

    [MemberNotNull(nameof(_string3))]
    void Init() { }

Now the attribute is incorrect.

Even so, the .NET team did release MethodNotNull in C# 9.

This issue can probably be closed now.

[RikkiGibson]

The compiler warns when the method implementation doesn't satisfy the attribute:

https://sharplab.io/#v2:EYLgtghgzgLgpgJwD4GIB2BXANliwtwAEcaeBAsAFBUACADITQIwB0AIgJYQDmaA9rA4BjKCwDCfACZwAgqSwBPKBygBuKrQDMjAEyExhAN5VCpxkzoB+QgH1YCDmm7aAvIUw511SmcIBtAFk4MGBEADk+GDDsLAAKUjA4PgAzWLsYBydNAEpsgF0TMxoAFkIASTQOGCZY7KNbe0dnQktLNwAiaWSIbBh21UIAX0LTEf8gkPDI6Jx4iESUtMas3IKfItKKqp1a+uHKfaoEuCgABwghImZ2Ll4BGGFRCWk5CEVlKCpjddMabRo9BNQggIlEYjIYBkOMAMPBCCBCBCoTD4GNvr5fH9CECpmCcEiHCi4LFmAxTgg+Kc6oYhmN9oMgA=

edit: I realize that I just restated the same comment I made in July, but perhaps linking the example helps :P

[RikkiGibson]

We may end up further improving some of these scenarios in future language versions, but I will close this one as being resolved "well enough" :)