nullable checking doesn't go far enough to check non-nullable variables being set to null

[vsfeedback]

The following code snippet shows a situation where a non-nullable string gets inadvertently assigned null.
Limitations like this mean that we can't trust the nullable warnings - which makes the feature less useful as people will then not trust it and will abandon trying to fix their code.

To reproduce, paste the following code into a C# console project, and set the project to use the C#8 compiler with #nullable enable set.

Note that there's no warning produced, but the string gets set to null.

		string str;
		{
			using ( RegistryKey rk = Registry.CurrentUser.OpenSubKey( "Software" ) )
			{
				// No warning, but the cast from a null object will result in null
				str = (string) rk.GetValue( "WontExist" );
			}
		}
		if ( str == null )
		{
			Console.WriteLine( "It is null" );
		}

This issue has been moved from https://developercommunity.visualstudio.com/content/problem/412355/nullable-checking-doesnt-go-far-enough-to-check-no.html
VSTS ticketId: 754373
These are the original issue comments:
(no comments)
These are the original issue solutions:
(no solutions)

[jmarolf]

Note it will warn on as

string str;
using (var rk = Registry.CurrentUser.OpenSubKey("Software"))
{
    // Warning	CS8600	Converting null literal or possible null value to non-nullable type.
    str = rk.GetValue("WontExist") as string;
}
if (str == null)
{
    Console.WriteLine("It is null");
}

I am torn.
I feel like a direct cast is basically telling the type system "I know this will do the right thing".
On the other hand there are a lot of APIs in .NET that return objects and you are expected to cast them to the underlying type.

Seems like if I get a type from a null oblivious library I should assume that the returned object can be null and require a null check before assignment.

[Dave-Lowndes]

I am torn.
I feel like a direct cast is basically telling the type system "I know this will do the right thing".

I had the same thought too with my example, but it was a concise example of a real issue I hit when testing. Using 'as' didn't seem quite the same situation as it can inherently result in null.

On the other hand there are a lot of APIs in .NET that return objects and you are expected to cast them to the underlying type.

Yes.

Seems like if I get a type from a null oblivious library I should assume that the returned object can be null and require a null check before assignment.

Yes, I feel that it should assume the object is nullable - which is the opposite of what's currently assumed. I presume the C# team have a good reason for doing that?

[CyrusNajmabadi]

I presume the C# team have a good reason for doing that?

Yes. because you casted directly to a type. So it seems odd to say: well, it's not that type. Furthermore, it would make it much more verbose to then have to have some other mechnaism that says "no, damnit, it's the non-nullable version of this type".

As it stands, tehre are two supported forms:

1. (Type). Casts to the type, expects the user knows what they're doing.
2. as Type. Casts to the type, but may be null.

This gives hte user two good ways of handling this situation. if (Type) was the same as as Type we'd both have redundancy and we'd need an entirely different way to express this very desirable operation. That is far worse than just having two separate forms to support to the two common desired ops.

[Dave-Lowndes]

Yes. because you casted directly to a type.

Ignore the fact the example shows a cast. I was trying to keep things simple. Assume instead that it's an external method that is defined to return a string, and in some circumstance it returns a null.

For example, if you have a class library project built with C#7 with this code:

namespace ClassLibrary
{
    public class Class1
    {
		public static string IReturnNull()
		{
			return null;
		}
    }
}

Then in the C#8 nullable enabled project you call it like this:

string myString = ClassLibrary.Class1.IReturnNull();

Your C#8 project then has a non-nullable string which is null, and there's no warning :(

[jmarolf]

Assume instead that it's an external method that is defined to return a string, and in some circumstance it returns a null.

This is a general critique of how null obliviousness is handled today. The fundamental assumption of the compiler it that:

• T implies the intention is the type cannot be null
• T? implies the intention is the type can be null

For libraries that are null oblivious when given a T the compiler can assume:

1. That the type is not null
2. That the type is allowed to be null
3. That the type is some third state (not either 1 or 2 until proven otherwise, forcing the user to disambiguate)

Of these options, 3 cannot work in my opinion. Users are not in the position to really reason about the intent of an arbitrary method they are calling in all but the most trivial use case. In addition if we went with 3 what would the concrete type of var be below? T or T?

var x = NullObliviousMethodReturningT();

Option 2 feels like it might be correct as we are saying "I am unable to prove this is non-null so you need to do something" but that goes against the fundamental assumption that nullable reference types is based on. Nullable reference types says "Since runtime behavior assumes that references are not null (as it is a runtime exception for them to be otherwise) the type system can also assume that they are non-null". The hope is that for the 90% case nulls are not allowed on reference types and this assumption is correct for the users code. If instead we had all null-oblivious types treated as T? this would quickly get confusing. Users would see type declarations of T and say "I can't know what the semantics are for this type without also knowing the compiler options and pragmas for this file". You then effectively have two languages, each with completely different semantics.

This leaves us with option 1. But I still think we need to do something to make this better. I think an analyzer that I turn on that warns me when a null-oblivious type is being used as a non-null type would be useful. I could then suppress this warning using the normal analyzer suppression mechanisms in my code for things that I feel are safe oblivious to non-null conversions or add nullability annotations for types that I am unsure about. I could also just not opt-into this analyzer at all if I didn't want to be bothered.

[ewlloyd]

Just spitballing from the cheap seats here...

Would it be possible for the #nullable enable pragma to decorate the output assembly with a NullableRefAttribute, then have the compiler use it to handle null-oblivious assemblies differently (assume T == T?)?

[jmarolf]

@ewlloyd The design today is that the compiler will emit an attribute for assemblies that are compiled with the nullable option. This is how null-obliviousness is expected to be determined. We are still left with the question of what the compiler is supposed to do with this information.

Using the sematics you have suggested if GetValue() is defined in a null-oblivious library then the below code will result in a warning.

#nullable enable
T a = GetValue();

However if the user goes to the definition of the method they may see something like this

T GetValue => new T();

Where T is clearly not T? even though it is being treated that way. I worry that this would make it very difficult for newcomers to reason about which types are expected to be nullable and which are not.

[ewlloyd]

@ewlloyd The design today is that the compiler will emit an attribute for assemblies that are compiled with the nullable option. This is how null-obliviousness is expected to be determined. We are still left with the question of what the compiler is supposed to do with this information.

I had a feeling it was already there, but I just wanted to level-set from my own naive perspective -- I haven't spent a lot of time exploring the intricacies of Nullable Reference Types (more's the pity).

Using the sematics you have suggested if GetValue() is defined in a null-oblivious library then the below code will result in a warning.

#nullable enable
T a = GetValue();

However if the user goes to the definition of the method they may see something like this

T GetValue => new T();

Where T is clearly not T? even though it is being treated that way. I worry that this would make it very difficult for newcomers to reason about which types are expected to be nullable and which are not.

I definitely see your point here, but hear me out. Let's consider adding another pragma that implements foreign T == T? semantics (let's call it #nullable strict for now). It's okay to do without this if you're writing end-user-facing code, and can still write defensively against the odd NullReferenceException.

However, if you use #nullable enable without #nullable strict in a class-library assembly, then you really can't make any guarantees of null-safety to your callers, either, and really have no business advertising yourself as null-safe. Otherwise, the whole Nullable Reference Types feature takes on a decidedly screen-door-on-a-submarine vibe.

In fact, when you really think about it, the #nullable strict isn't such a hot idea after all, because users who are unaware of it will get a false sense of security (as noted by the OP). Perhaps the better solution is to just have the compiler force the strict semantics by default, and issue more explicit messages. Even if you don't have the room to spell it out completely in the message, the user's just a Google (okay, Bing) search away from true enlightenment.

[Dave-Lowndes]

Using the sematics you have suggested if GetValue() is defined in a null-oblivious library then the below code will result in a warning.

#nullable enable
T a = GetValue();

However if the user goes to the definition of the method they may see something like this

T GetValue => new T();

Where T is clearly not T?

You should have no knowledge of the implementation of an external library, and the library implementation may change anyway!

The only safe way that I can see is that tools should assume class types not marked with a nullable attribute should be assumed nullable.

[jmarolf]

We have now officially steered into csharplang territory. @gafter can you move this issue over there please?

[Arshia001]

I'm just trying out C# 8, and I've already found at least 2 different ways to get an NRE with null checks enabled. The subject of this issue is the more serious one. One can never assume external libraries to be correctly implemented. Unless null-oblivious types are assumed to be nullable, this entire null checking feature is rather pointless in any mid-sized project. When was the last time anyone made anything with no dependencies on any external libraries?

I'm in favor of having at least #nullable strict as @ewlloyd put it, though I also fail to see how null checks would be beneficious in non-strict mode. Sure, most libraries probably don't return nulls where you expect a value, but then again, most code within a module also doesn't.

To me it looks like null checks were created specifically for the rarer cases where somebody makes an unintentional mistake. Library authors aren't necessarily better than other programmers, they also make mistakes. Besides, once C# 8 is released, the more heavily used libraries' authors would probably update their code with nullability checks. You'd then be left with the more esoteric libraries, which are also more likely to contain mistakes. Voila, NRE's in your C# 8 code.

[Joe4evr]

To me it looks like null checks were created specifically for the rarer most common cases where somebody makes an unintentional mistake.

Nail, meet head. (Also, FTFY.)

The mantra of NRTs is "We can protect you from Murphy, not from Machiavelli." You can subvert the null-warnings in dozens of different ways, but you'd have to go through the trouble of writing really convoluted/non-conventional code to do so.

[Arshia001]

most common cases

@Joe4evr Most common cases that happen to result in errors rarely. Otherwise, you'd have a hard time passing tests and making it to production.

you'd have to go through the trouble of writing really convoluted/non-conventional code to do so.

Or, just wait for null checks to catch on before using them. Better to have absolute safety in two years than have 90% safety now IMO. And then there's the opt-in thing.

[ewlloyd]

I'd thrown the #nullable strict in as a thought experiment, to point out the ultimate folly of an opt-in approach. We need the compiler to assume that all null-oblivious assemblies should be coerced to yield nullable types, even if the type declarations receive magic question marks that don't appear in their source code. Anything less is null-safety theater, and an exercise in futility.

You've got one shot at getting this right, or NRTs will be widely UNused, and eventually slink off into the night, say, around C#9 or 10.

[theunrepentantgeek]

Or, just wait for null checks to catch on before using them. Better to have absolute safety in two years than have 90% safety now IMO. And then there's the opt-in thing.

If everybody waits for two years before adopting, then in two years time we'll be in exactly the same place we are right now.

You can be sure the feature will be improved over time.

Since 100% safety is actually unachievable, I'll take 80% safety now and look forward to 95% safety as things evolve.

[Arshia001]

If everybody waits for two years before adopting, then in two years time we'll be in exactly the same place we are right now.

T = 0: C# 8 is released.
T = 3 months: People start picking it up.
T = 6 months: Most heavily used libraries are starting to add null safety.
T = 12 month: The libraries you depend on now probably have null safety built in. You can wait some more for that last library author, or just add a bunch of null checks and be done with it.

In return, you now have 100% null safety in the F#/Kotlin sense: You're probably 99% safe as long as nobody's intentionally messing stuff up. Yes, you can never be 100% safe. Who's going to stop you from modifying a non-nullable property with reflection (or breaking your PC apart with an axe)? But you're safe as long as you stick to the rules. The language and the compiler and the code analyzers are there to back you up.

Unfortunately, the same cannot be said for the current design though.

I'd thrown the #nullable strict in as a thought experiment, to point out the ultimate folly of an opt-in approach.

NRT's are an opt-in feature themselves. I'm all for forcing null-oblivious types to be nullable, but I guess some people could live with the compromise. They'll get their fair share of NRE's and hopefully start opting in.

[ewlloyd]

NRT's are an opt-in feature themselves. I'm all for forcing null-oblivious types to be nullable, but I guess some people could live with the compromise. They'll get their fair share of NRE's and hopefully start opting in.

Exactly. But if you opt into NRTs, you should expect to get the full Monty, and not have to take on the cognitive load of remembering whether a given type is null-oblivious or not. If someone wants to exempt a given type or assembly from the NRT world, they can opt it out -- and on their own overworked head be it.

[marksmeltzer]

I am still a huge fan of creating a C# successor language that merges the best of C#, TypeScript and F# into a new language... Hell, probably just making TypeScript a .NET language would be amazing. Basic idea there is to allow calling all of the legacy APIs, and the legacy APIs could even be doctored with additional metadata to assist the editor.

Unless we have a clean break from the everything is nullable past, the language will have all of this messy compromise.

I love all of the the changes that C# 8 and on are bringing, but we really need a way of allowing breaking changes into C# itself to really move it forward to break free of past design decisions.

Other than the nullable strict pragma, I would love to see the C# compiler have a version range capability: add the ability to set the minimum language feature version.

So, I should be able to set a project to C# latest minor, I might also set the C# minimum version to 8.1 (hypothetical) wherein nullablility is strictly required for compilation - i.e. same as TypeScipt.

If I have some older projects that are too much work to port entirely, I can make progress over time and eventually move up the minimum C# version.

Adding the minimum version capability would allow each C# release to come with a list of things that are obsolete, but will still compile (as warnings?), and a list of things removed completely. So the rule could be to mark language features as obsolete one release, and removed the next.

Adding a minimum version capability to the project settings and compiler at least provides a way out of the legacy language features that the community agrees should be removed.

Regards,

[Arshia001]

Hell, probably just making TypeScript a .NET language would be amazing.

Do not go putting JavaScript in my .Net please.

[Arshia001]

So... NRT is 'With the release of .NET Core 3.0 Preview 7, ... considered "feature complete"' and we never got any special treatment for null-oblivious libraries, meaning with the final version of C# 8 we can still get NREs at runtime. Am I correct?

[CyrusNajmabadi]

meaning with the final version of C# 8 we can still get NREs at runtime. Am I correct?

Correct. Indeed, NRT has zero runtime impact.

[Arshia001]

@CyrusNajmabadi Yes, I was talking about the treatment of null-oblivious libraries, which is (to the best of my knowledge) to treat them as non-nullable nonetheless, which means we can get NRE's the compiler doesn't catch.

[CyrusNajmabadi]

which means we can get NRE's the compiler doesn't catch.

Yes. I'm just being clear that in 8.0 there will already be many NREs the compiler doesn't catch. So this doesn't change much there.

[Arshia001]

I'm just being clear that in 8.0 there will already be many NREs the compiler doesn't catch.

Oh, I was under the impression that this was supposed to be a complete solution to the problem. Good to know that!

[CyrusNajmabadi]

Oh, I was under the impression that this was supposed to be a complete solution to the problem.

It is not. It is a set of checks for common types of mistakes to help find and eliminate very probable bugs. To be 'complete' it would have to flip things around and likely end up with a system that gave tons of false positives, which would likely cause an enormous around of developer fatigue around adoption.

So, the system opts now to surface highly suspect issues, at the cost of masking some real issues.

[ewlloyd]

So null-oblivious libraries can still pollute our NRT-checked code, and we'll have no obvious way of flushing them out. Is there anything on the backlog to at least give us an opt-in with #nullable strict or somesuch?

[CyrusNajmabadi]

@ewlloyd You're welcome to open an issue asking for this so it can be appropriate reviewed. You could also just write your own analyzer for this sort of thing if you wanted :)

[ewlloyd]

Fair enough -- I just might! :-)