Nullable Reference Types annotations. Fixed by team through multiple PRs.

[jnm2]

tl;dr Now that C# 8 is out, people will be using the Nullable Reference Types language feature. We should consider putting the compiler annotations on our library. That would require moving the codebase to C# 8. To reap the benefits in our own codebase, we could also use https://github.com/tunnelvisionlabs/ReferenceAssemblyAnnotator.

What it is

People who put <Nullable>enable</Nullable> in their projects or #nullable enable in their source files are then allowed to use the syntax ReferenceType? where nulls are permitted. In typical software, nulls end up being actually intended in only a small percentage of the usages of reference types.

The compiler then produces warnings (we have /warnaserror) when a thing that might be null is treated as though it can't be null, like when dotting off which could cause an NRE or when passing to a method which could cause an ArgumentNullException, or when setting a field or property which only causes a problem at a later point in time.

This goes a long way toward solving the billion-dollar problem of nullability blindness in the type system.

https://docs.microsoft.com/en-us/dotnet/csharp/nullable-references
https://devblogs.microsoft.com/dotnet/nullable-reference-types-in-csharp/
https://docs.microsoft.com/en-us/dotnet/csharp/tutorials/nullable-reference-types

Things like string.IsNullOrEmpty are annotated with new compiler attributes such as [NotNullWhen(false)] on its parameter so that the compiler can recognize that it's a null check. These attributes can be defined by anyone as internal so that they don't conflict with other assemblies. .NET Core 3.0 is the first version of the BCL that adds these attributes as public classes. Referencing https://github.com/tunnelvisionlabs/ReferenceAssemblyAnnotator would define these internally for us without having to maintain the internal attribute definitions ourselves.

Reasons to do it

• It would be nice to annotate our library so that people who use NUnit are warned if they forget to check things like TestContext.CurrentContext for null or if they pass things that could be null to our APIs. It's being up-front about the types that each API accepts in the same way that declaring a parameter IFoo foo instead of object foo is being more up-front and helpful.

• This would aid the mission of maintaining high-quality internal code written by many different people. We've had our share of null reference exceptions that nothing had forced us to notice at compile time.

• It's not a breaking change.

Reasons not to do it

• It requires <LangVersion>8<LangVersion>. While Visual Studio wouldn't be required in order to compile the codebase, 2019 Update 3 would become the minimum version if you were using Visual Studio. You could also compile using the .NET Core 3.0 SDK using the command line or using VS Code and other IDEs.

• It's likely to require us to think about things we never had to reconcile before. Once we start shining around this black light, so to speak, we might not like some of the things we see. (Or is this good, as NUnit 4 begins to take shape?)

• It would touch a decent percentage of our files.

Proposal

I think we should do it in v3 (because this is my function in life, apparently?) and it's work I enjoy and would prioritize. However, I see these downsides and I'm not going to be overly disappointed if we don't want to do it in v3.

I feel more strongly that we should do this by v4. Otherwise, as one of the most-frequently used libraries, I think we'll end up standing out in an unappealing way as time goes on. I could easily be wrong. But if we do this by v4, the easiest way might be to start now in v3 since it can all be done incrementally.

[ChrisMaddock]

I would love to see this in place in v3 also. I personally think the benefits of doing this outweigh the unfortunate cost of limiting the IDEs here.

[Dreamescaper]

Could you clarify what is ReferenceAssemblyAnnotator? According to description, it is much more than simple defining attributes, right? Since we will simply add netstandard2.1 as target framework, probably that's not needed.
Just suggesting an alternative - we could use Nullable package, it seems to be much more lightweight.

Another thing I'd like to ask. Apart from annotating actual nullable properties, should we think about nullability attributes on assertions somehow? E.g.

string? str = GetNullable();
Assert.That(str, Is.Not.Null);
Assert.That(str.Length, Is.Not.Zero);  // <----- should we care that no warning here?

[jnm2]

@Dreamescaper https://github.com/manuelroemer/Nullable only defines internal attributes in your source. ReferenceAssemblyAnnotator does that too, but more importantly it rewrites all the .NET Framework or .NET Standard reference assemblies that you compile against to retroactively add nullability attributes by copying them from .NET Core.

Another thing I'd like to ask. Apart from annotating actual nullable properties, should we think about nullability attributes on assertions somehow? E.g.

The current annotations are too limited for the compiler to be able to tell that Assert.That(str, Is.Not.Null); is a null check. We could approach it from the NUnit.Analyzers angle, though.

[Dreamescaper]

@jnm2

ReferenceAssemblyAnnotator does that too, but more importantly it rewrites all the .NET Framework or .NET Standard reference assemblies that you compile against to retroactively add nullability attributes by copying them from .NET Core.

Yeah. So it makes sense if you cannot update your project for whatever reason. Why is that needed, however, if we can simply add netcoreapp3.0 target, where it would simply work?

The current annotations are too limited for the compiler to be able to tell that Assert.That(str, Is.Not.Null); is a null check. We could approach it from the NUnit.Analyzers angle, though.

We technically could introduce some That overloads for NullConstraint and NotNullConstraint, and add corresponding attributes there.
But I agree - it makes more sense to do from analyzer perspective.
Probably via DiagnosticSuppressor ?

[jnm2]

Yeah. So it makes sense if you cannot update your project for whatever reason. Why is that needed, however, if we can simply add netcoreapp3.0 target, where it would simply work?

I don't remember if we are planning to add a netcoreapp3.0 target, but I know we intend to keep netstandard2.0 and net472 targets in addition to netcoreapp2.1. Also, if we're doing this in v3, we're keeping the net35 target.

Without ReferenceAssemblyAnnotator, this means that the NUnit codebase would get warnings in the code editor around our usage of the BCL if we select netcoreapp3.0 in the project context dropdown in the top left of the code editor, but not if we select netcoreapp2.1, netstandard2.0, or net472. And that's if we even add a netcoreapp3.0 target.

[jnm2]

@Dreamescaper Did I answer what you were asking? I might be misunderstanding, sorry. If we use ReferenceAssemblyAnnotator, it will be impossible to tell downstream. It doesn't go in the nuspec or add an extra assembly. It just makes the compiler smarter when this codebase compiles.

[jnm2]

@nunit/framework-team I played around with this since I have my head in the space at work and other places. While trying to make ends meet, I found instances where we have bad assumptions by not checking for null when we really should have been the whole time, or by having complex timing dependencies between methods that make it hard to reason about the state that the class fields are in.

Should I start submitting standalone fixes using C# 7 in the meantime?

[mikkelbu]

But if we do this by v4, the easiest way might be to start now in v3 since it can all be done incrementally.

My initial feeling about this was to do something similar to this. E.g. go for the v4 release, but do it incrementally with parts of the work in v3, as I think that this is a larger change, mostly in the build process. But this also depends on the timeline for v4. At least the v4 label would make it clear that this is "something completely different".

Should I start submitting standalone fixes using C# 7 in the meantime?

Yes. Please do 👍 . This will perhaps also give us an idea of how far we are from the target. Would it make sense to create a "Nullable" branch or build taget, so that more people can work on this, or will this complicate the work.

[jnm2]

Okay, thanks for the input.

My instinct is against a long-lived branch because merging is always painful and people will have to be aware of both threads. For now, I'm so close to having NUnit fully annotated that I think I'll tackle opening the issues/PRs. I'll have questions and PRs that I'll need help with.

[woodstoke]

We need not forget specify [DoesNotReturn] attributes.
Now, I'm writing code like this to prevent warnings:

if (!OK())
{
    Assert.Fail("my error message");
    throw new Exception();
}

[jnm2]

We can't use [DoesNotReturn] because Assert.Fail does not always throw.

The difference is whether you are inside an Assert.Multiple block.

nunit/src/NUnitFramework/framework/Assert.cs

Lines 385 to 395 in 1eb2459

	private static void ReportFailure(string message)
	{
	// Record the failure in an <assertion> element
	var result = TestExecutionContext.CurrentContext.CurrentResult;
	result.RecordAssertion(AssertionStatus.Failed, message, GetStackTrace());
	result.RecordTestCompletion();
	// If we are outside any multiple assert block, then throw
	if (TestExecutionContext.CurrentContext.MultipleAssertLevel == 0)
	throw new AssertionException(result.Message);
	}

[jnm2]

Also, even when [DoesNotReturn] is applied, C# still requires the throw afterwards. The C# language design team decided to make the nullability attributes affect only the nullability state of variables, not flow control. So this would be the only difference:

nullableThing = a?.B;
if (nullableThing is null) Assert.Fail("Some message");

// Warns that `nullableThing` may be null unless Assert.Fail has [DoesNotReturn]
nullableThing.Foo();