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 #32358 (comment): 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" :)

[SoftCircuits]

As the functionality is now, it's just plain wrong. My init() method initializes the member and is called from the constructor. I shouldn't have to deal with this warning or rewrite my code.

[CyrusNajmabadi]

As Andy said:

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.

[RikkiGibson]

It feels like a lot of what's bothering people is the need to repeat yourself by e.g. applying MemberNotNullAttribute. I do wonder if it would be possible to, for example, analyze flow state interprocedurally in methods with a hypothetical init modifier, but it feels like there are many risks:

1. have a weird cliff for when the analysis "cuts off" that annoys users. The compiler figured out that this init method assigns X and Y, but I assigned Z by calling this other non-init method, what gives!
2. have effects that are difficult to reason about. A user removes a random line of code in a member method somewhere and the constructor starts getting warnings.
3. resource usage by the compiler. I believe Roslyn is pretty geared around the idea that member bodies can be bound/analyzed independently of each other, and that binding a body only requires knowing the signatures of anything used in it. If it turns out that binding one constructor may require binding the whole closure of members used by the constructor, especially if that spans across different types, that might be too expensive to permit.

[joelmdev]

Personally I hate looking at attributes and avoid them in domain/business logic as much as I possibly can. As others have said, being forced to use MemberNotNullAttribute is a kind of clunky solution.

Regarding point 1, I don't think an init modifier would cut it. I call member methods in ctors that are either called by other member methods or are public regularly; they're not dedicated initialization methods. So this just moves the problem and I still have to repeat myself.

Point 2 is actually exactly what I want- if I modify a line of code in a method that is called by my ctor and it violates the non-nullability that I've defined, I want to get yelled at.

Point 3 I can't speak to other than to say that plenty of people use Resharper, and no operation is too expensive for R# :)

[nehio]

What about EntityFramework Core entity classes ? We have no constructors are we supposed to mark virtual navigation properties as nullable ? even when the relation always exists ?

Or is this fixed in the next version ?

[DennisCorvers]

Running into the same issue as well. Would be nice if there was at least an attribute that you could mark the "Initialize" method with, rather than having to include every single member via the aforementioned attribute.

[IGkougkousis]

Just chiming in to say that I encounter this problem frequently and there aren't any good solutions. The best compromises I've found is to create several init methods that either 1. return tuples and the values are directly assigned in the ctor or 2. use out parameters (as @skarllot suggested).

The nullable reference types feature is quite handy and I like having it enabled, but this is definitely one area that could be improved. Thank you for your efforts.

[robert-io]

For those of you looking for a slightly better workaround to the lack of initialization interprocedurally tracking you can use the ! (null-forgiving) operator in the variable declaration. Using this method you can make the compiler think the field has been set so you won't get the warning for the constructor.

https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/operators/null-forgiving

I use it with a massive comment so i always know why i did it:

class Example {
    /* Note: The ! (null-forgiving) operator is used here to eliminate the warning
     *       that these fields are not set in the constructor. The fields are set
     *       in a method called by the constructor but the compilers static analysis
     *       can't determine that.
     * */
    private string _data1 = null!;
    private string _data2 = null!;

    public Example() {
        FooBar();
    }

    private void FooBar() {
        _data1 = "Foo";
        _data2 = "Bar";
    }
}

Obviously i would still prefer the compiler to track calls made during the constructor but this is a pragmatic solution.

Note: This solution is available from C# 8.

[SuuperW]

I am running into the problem as well. Further, I am unable to use the MemberNotNull attribute because my project targets .NET Standard.
I don't understand how this works under the hood, but it seems like really bad design to have code analysis depend in this way on attributes defined by the target framework instead of by the code analyzer itself. This is a very frustrating problem, and I think the workaround posted by robert-io is the best option here.

[manfred-brands]

I had similar issues in the nunit.analyzer for variable initialized in the Setup method and adding support for locally defined methods wasn't that hard: https://github.com/nunit/nunit.analyzers/pull/386/files

[Entiat]

Here to say, "Me too" and add another scenario that breaks because of this.

We have created a C# scripting engine in our Windows app (via "dotnet build" and loading the dll in a weakly referenced AssemblyLoadContext, etc. but that's not important right now (and quit calling me Shirley)).

One of the scripting features is that it can display general Form-derived classes for the users from these otherwise tightly controlled scripts (we generate wrappers for classes, methods, csproj file behind the scenes, etc. - we don't want end-users monkeying around with that stuff)

Anyways, the architecture means we need to include VS designer modified code in one script blob (under the covers: one cs file). Thus, the "standard" winforms InitializeComponent() is called from the constructor from within the same cs file as the constructor.

The compiler somehow knows that myform.cs + myform.designer.cs (with embedded InitializeComponent) should not throw CS8618, but it doesn't know that the same exact thing wedged into one file should not throw the error. Not sure what drives the former (perhaps the default "partial" class keyword that we've now removed?)

Main Point: this error is quite a challenge, with very limited workarounds at present, especially if I'm counting on only-slightly-technical scripter customers to...uh...wrap their VS-designer-created UI members with...uh...pragmas or attributes? That's absolutely not something they can cope with.

I'd rather not turn off nullability checking overall or perhaps just for the constructor - it is an incredible, powerful language feature - but I'm having trouble coming up with a solution other than that.

[RikkiGibson]

Partial keyword shouldn't affect analysis here. It's possible your .designer.cs file is being treated as generated code and analysis is not actually running on it. If you add #nullable enable to the top of your designer file you should see warnings in there start to show up.

[danielwagstaff]

Please fix this. Nullables are amazing and avoid so many needless null checks and unexpected problems, but not being able to consolidate the setter code in one place (without adding Attributes) is another little annoyance that makes it feel like dotnet has got it right, but only 90% of the time.

[danipen]

Same here! Nullables are great, and we're happy using them, but in some scenarios, it's annoying to use. For example, we extensively use this pattern in our codebase:

internal class SomeUIComponent
{
    internal SomeUIComponent()
    {
        BuildComponents();
    }

    void BuildComponents()
    {
        // we initialize all the sub-components at this point, to reduce noise in the constructor
        _header = new DetailsHeader();
       ...
    }

    DetailsHeader _header;
}

And the compiler complains:

CS8618: Non-nullable field '_header' is uninitialized. Consider declaring the field as nullable.

Which obviously is not null, and we shouldn't make it nullable. So please, consider tracking initialization interprocedurally.

[MisinformedDNA]

Closed issues are often ignored. If this is important to you, please create a new issue.

[RikkiGibson]

If there's interest in continuing the discussion, please do so by opening a discussion in csharplang.

[Eli-Black-Work]

(And if you do open a discussion, please link to it here! 🙂)