Supress (even more) 'Non-nullable field must contain a non-null value when exiting constructor (CS8618)'

[smkanadl]

I started to migrate a larger codebase towards nullable reference types and the Analyzers are of great use. Many thanks for the effort so far!

The analyzer is already capable of suppressing simple cases where the variable is assigned within the SetUp() method explicitly (Case someVariable).

[TestFixture]
public class TestClass
{
    private string someVariable; // CS8618 very nicely suppressed!
    private string someVariableThatGetSetInACodeAnalysisDecoratedFunction; // CS8618 does not get (currently) suppressed

    [SetUp]
    public void SetUp()
    {
        someVariable = "value";
        SetValue();
    }

    [MemberNotNull("someVariableThatGetSetInACodeAnalysisDecoratedFunction")]
    private void SetValue()
    {
        someVariableThatGetSetInACodeAnalysisDecoratedFunction = "also-value";
    }
}

I was wondering if it is possible to detect some more complex cases (Case someVariableThatGetSetInACodeAnalysisDecoratedFunction)? Of course it may not be a good idea to do extensive travelling of the AST to find an assignment deep down in some function called by SetUp(),

Instead, I thought it might be possible to take advantage of the System.Diagnostics.CodeAnalysis attributes. In this case e.g. MemberNotNull. In such a case it would only the functions that are called directly called by SetUp() needed to be analyzed and checked for the MemberNotNull attribute. Then the attributes argument has to be matched against the member variables that are causing warnings to find one that could be suppressed.

What are your thoughts on this?

[manfred-brands]

@smkanadl Thanks for the compliment.

I'm not in favour of the attribute, for two reasons:

1. It requires extra work by the developer.
2. It can easily get out of sync.

In your case nothing calls the SetValue method, it is not called from SetUp.
If the method is not called before every Test, there is a chance that one test doesn't call it and uses a null value without getting a warning.

I updated the suppressor to traverse into instance methods called from SetUp methods to detect field assignment. This should not be that deep as it would be limited to methods defined in the Fixture itself as others can't set the fields.

[smkanadl]

Hi, of course SetValue should have been called by SetUp(). I just forgot that and updated m example. Sorry for being that stupid.

I am not a big fan of the attributes either, so I completely agree/understand your arguments.

Is there a pre-release version to try out?

[manfred-brands]

Is there a pre-release version to try out?

You can find the NuGet packages in the build artifacts.
Copy the appropriate .nupkg file into a local nuget folder and add that folder as a nuget package source with:
dotnet nuget source add <folder>

I just tried that out and it works for a test project I made.

[smkanadl]

Hi @manfred-brands,

I gave that version a try and it works(*). The suppression of the nullable warnings works perfect in the IDE, but in the build I still get the warnings and I even get warnings for direct variable assignments in the SetUp() (which worked before).

I have the strong assumption, that it is related to the warning that I also get in the output:
AD0001 Analyzer 'NUnit.Analyzers.DiagnosticSuppressors.NonNullableFieldOrPropertyIsUninitializedSuppressor' threw an exception of type 'System.ArgumentException' with message 'An item with the same key has already been added.'.

I looks like when actually running the build, the supressor crashes and none of the potential warnings are surpressed. I already did a run with diagnostic output, but it didn't give any more specific details or context from where the exception is thrown. It happens in a larger test project with ~80 fixtures and ~800 test cases.

Is there any more data that I can provide or how can I help you to go forward?

[manfred-brands]

@smkanadl it is a pity that the exception doesn't actually say what the key is as that would give some indication where to look.

I managed to reproduce your problem.

The only Dictionary created in the Suppressor is the one of methods in the class. I assumed that the method names would be unique, but that is not the case when using parameter overloads.

public void SomeMethod(int i);
public void SomeMethod(double d);

This raises the complexity; I not only have to check the method name, but also its parameter types. I don't even think those are available at the level the suppressor runs as it requires potential conversions, e.g. when passing an int to a method expecting a double.

I will have a look what I can do.

[smkanadl]

@smkanadl it is a pity that the exception doesn't actually say what the key is as that would give some indication where to look.

Yeah, thats one of my typical problems when it comes to standard dotnet exceptions.

I managed to reproduce your problem.

Sounds good, although the details themself not :-/. I really appreciate the effort you put in and hope you will find a solution!

[manfred-brands]

@smkanadl I have updated the code to recognized method overloads. Updated NuGet package here

[smkanadl]

That one is working beautifully. And the overload detection code doesn't look too bad for me :-).

Again, many thanks!

PS: Is there a release date scheduled for 3.2?

[manfred-brands]

PS: Is there a release date scheduled for 3.2?

That depends on @mikkelbu

[mikkelbu]

Hi @smkanadl
I'm very busy at the moment, but I think I'll have time to do a release in the weekend.

[smkanadl]

That would be a perfect fit into m time schedule! Thank you!

[manfred-brands]

@smkanadl Can you verify the latest works for you?
I no longer check the parameters myself, but check the semantic model (aka compiler) for the match.

[smkanadl]

@manfred-brands I tried the latest and it still works like a charm :-)

[mikkelbu]

I've just merged the PR. I'll try to make a release tonight CET aka in 4-5 hours.