Non-nullable reference types could introduce a new kind of bugs

[savahmel]

The non-nullable reference types feature does two things:

1. Introduces the concept of the non-nullability that allows to express programmer’s intent.
2. Provides a check mechanism for a subset of cases when this intent is violated.

The intent of non-nullability leads us to omit null checks, otherwise what’s the point of non-nullability as a concept?

This might introduce a new kind of bug, that originates from the omission of the null-check that otherwise would have been performed.

On legacy code once turned on, this feature might have two consequences:

1. Bugs found on dereference of a nullable reference or assignment to a non-nullable reference.
2. Bugs introduced upon removal of null-checks where non-nullable types are now used.

Let’s look at the following function:

public static void PrintSize(string text) 
{ 
    Console.WriteLine(text.Length); 
}

Should we null-check the “text” argument? It actually depends on the usage of the PrintSize function.

1. For a library function, we should put a null check. Since a for now, CLR ignores non-nullability whoever will use this function might pass a null value.

2. Suppose this is an internal function, will compiler make sure that I do not pass a null value from an uninitialized static field for example? As for now, at least in certain cases does not.
   Non-Nullable reference types of static fields #2088

3. What if I pass as an argument a result of a call to an external library function? As for now, I am not getting any warnings for oblivious types by design.
   No nullability warning for a FirstOrDefault() method roslyn#31997

To my mind in order to be on the safe side the feature lacks an optional “Strict Mode” that I would personally prefer for new projects:

1. Until non-nullability is supported on the CLR level, provide a warning for every non-nullable argument of a public library method, field or property.

2. Provide a warning message for dereference of non-nullable types whenever the compiler is not able to prove 100% non-nullability.

[HaloFour]

Non-nullability is not a feature coming to the CLR, nor is it 100% enforceable by the C# compiler. Being perfect is not a goal of this feature. The goal is to make the compiler aware of the cases in which null could throw when passed to a method so that the compiler can provide that feedback to the developer, who currently does not have this information outside of documentation.

There are separate proposals for having the compiler actually emit the null checks and to throw, either when parameters are explicitly annotated or by default based on preprocessor directives like DEBUG.

[savahmel]

@HaloFour

It is not 100% enforceable, and it would be great in my opinion to be pointed out at the cases where the compiler is not able to enforce the non-nullability. This way I would be urged to investigate these cases to see whether I am able to prove otherwise or maybe change my decision and make the type nullable.

[YairHalberstadt]

@savahmel
I would suggest that could be better achieved using a Roslyn analyser. You may want to suggest this there.

[CyrusNajmabadi]

It is not 100% enforceable, and it would be a great in my opinion to be pointed out at the cases where the compiler is not able to enforce the non-nullability. This way I would be urged to investigate these cases to see whether I am able to prove otherwise or maybe change my decision and make the type nullable.

You can write an analyzer for this purpose. It may be something the team decides to provide as well. But it is out of scope of the explicit intentions of hte core nullable-ref-types feature that is being delivered.

[gafter]

The intent of non-nullability leads us to omit null checks, otherwise what’s the point of non-nullability as a concept?

No.

The point of non-nullability is to warn you when you left off a null check that is needed. The compiler doesn't complain about null checks even if it thinks it might not be needed.

[alexk8]

NRTs at this point are not just useless, they are evil.
If method declares that it accepts only non-nullable, it must be guaranted by the compiler (adding null-checks for public methods, which are not needed for internal)
This comment is quite good - #2145 (comment)

If I could appeal to developers, I would say "Please do not introduce semi-non-nullable into release"
Some people propose "strict non-nullable" like SomeMethod(object x!) so it will be a nightmare.

void SomeMethod(object? x){} // nullable
void SomeMethod(object x){}  // not guaranted non-nullable (=nullable!)
void SomeMethod(object x!){} // strict non-nullable

So one of them are evil. If you provide object x! it will be used everywhere in public methods,
so there is no need to introduce this syntax, just make (object x) strict non-nullable.
If you make such a great feature like NRT, please do it right, with all efforts

[alexk8]

another option is to track context switching while compiling

#nullable disable
        public void main()
        {
            object arg = null;

            //SomeMethod is declared in NRT-context (has some attribute), 
            //so compiler generates code: 
            //if (arg == null) throw new ArgumentNullException(nameof(arg));
            SomeMethod(arg);
        }
#nullable enable
        public void SomeMethod(object x) {
            //x guaranted to be not null
        } 

[savahmel]

@alexk8

In my humble opinion an attempt to make this feature work for the legacy code, was the wrong design decision. It lead to compromise on language elegance, making the feature counter intuitive.
(However, leaving space for possible future improvements)

I think a better choice would have been, once NRT is opt-in, to give errors and not warnings in every case where it is not possible to prove non-nullablility. Thus requiring the usage of nullable types, for instance on every public library method, property, field.

This way all the knowledge about all the corner cases would be concentrated in the compiler, and we could learn from its wisdom. Currently this knowledge should be ours, and unless we know all the cases, we cannot safely use the non-nullable types, which is pretty insane.

As to introduction of ! operator, it tires to fix the problem originating from the obscure concept of non-nullable type that might be passed a null value. I would rather, there was additional option to be explicitly enabled, allowing non-nullable types to have null-check-throw injected on public method arguments and properties.

[svick]

@alexk8 Are you saying that changing the access modifier on a method would also change its behavior? I don't think that would be acceptable, it's much better to be explicit about that behavior change.

And if you think only nullable and "strict non-nullable" parameters should be allowed on public methods in your codebase, it shouldn't be hard to write an analyzer for that.

[CyrusNajmabadi]

I think a better choice would have been, once NRT is opt-in, to give errors and not warnings in every case where it is not possible to prove non-nullablility.

This would be too onerous and would make the feature effectively unusable for the vast majority of users.

NOte: if you want that behavior, you can get that once NRTs ship. Simply write an analyzer that is as strict as you want it to be. NRT forms the basis in the language and compiler to make it possible to do that sort of thing.

[CyrusNajmabadi]

If you need to put null checks anyway regardless of whether the type is nullable or non-nullable, then what is the point of non-nullability?

To help you catch potential mistakes. Already, it's found several in libraries i work with.

The compiler won't suggest to remove a null-check but in what cases would you put a null-check in the first place?

I put in the null checks in cases where the compiler has given a good indication that a null ref is possible.

I believe it could become a source of new bugs,

This is trivially solvable: write an analyzer for those cases. The NRT feature was not designed to prevent every single possible null ref from happening. It was designed to help catch many common ones, with a reasonable balance toward understanding common user patterns, without being too onerous in terms of the rules.

If you don't plan to omit the null checks then what is the point of the feature and what value does it bring with it?

To help you catch a larger number of null-ref exceptions than today.

[CyrusNajmabadi]

@savahmel You might want to consider coming to gitter.im/dotnet/csharplang to discuss the topic. It might be more conducive to this sort of back-and-forth.

Note: to me, this feature draws a lot of inspiration from TypeScript (with the same positives/negatives of that language). Specifically, this is an attempt to take a complex space and give the user some level of control over trying to inform the compiler what is going on in their domain. The compiler can then use that to help drive information back to the user about potential issues. In both cases, there is no belief that it will fix or prevent all issues from happening. But it puts the user in a better place than from where they started, where there was no information at all, and all you could do was run things and hope they didn't blow up.

[alexk8]

Are you saying that changing the access modifier on a method would also change its behavior? I don't think that would be acceptable, it's much better to be explicit about that behavior change.

Ok, let's go from starting point. "Non-nullable" should be guaranted somehow. It's possible if compiler adds null-check automatically. In that way huge bunch of auto-generated null-checks would be everywhere, which is huge performance impact. Next let's optimize this - do not add null checks where compiler can prove non-nullability (call from known NTR-context) so null-checks must be added on the caller's side by compiler only when nullable-context switching is happening.
That's what I'm calling "smart" language. Why it's not acceptable?

And if you think only nullable and "strict non-nullable" parameters should be allowed on public methods in your codebase, it shouldn't be hard to write an analyzer for that.

I'm not an IDE developer to white analyzers, I just want cool language from the box )

[alexk8]

In my humble opinion an attempt to make this feature work for the legacy code, was the wrong design decision.

Legacy code anyway would have to be recompiled on new 3.0 compiler, so auto-generated caller's-side null-checks would solve this problem for a while and give time to adopt NRTs

[CyrusNajmabadi]

Ok, let's go from starting point. "Non-nullable" should be guaranted somehow.

If you want that, you should work on an analyzer that enforces that for your code. That's not what the NRT feature is.

It's possible if compiler adds null-check automatically.

What does this mean? What sort of null-check? The only thing i can think you mean is that the compiler would insert code that throws an exception. But that's not any better than today. With nulls today, you already get an exception. That's what NRT is trying to help you avoid in a bunch of a cases that you can run into today.

[CyrusNajmabadi]

Yes, but the LDM decided against this.

The LDM can't dictate what other people do. If someone wants an ultra strict analyzer and wants to document existing shipped APIs, they can go do this.

[yaakov-h]

Oh, you mean out-of-band info for just that analyzer? gotcha.

[CyrusNajmabadi]

Oh, you mean out-of-band info for just that analyzer? gotcha.

Yup yup. I'm totally in support of an effort for people to annotate and write analyzers for other libraries that seriously dial up the pain, but potentially catch a few issues that they really care about.

[alexk8]

if (a.b != null)
{
    Foo(a.b)
}

compiler should show a warning or just a message, it's up to you whether refactor it or not, but I would.
Annoying messages can be disabled in your project if you wish. But I thing this warning is a good option as long as that code is somewhat incorrect. correct way is something like

if (a.b is {} x)
{
    Foo(x);
}

[CyrusNajmabadi]

compiler should show a warning or just a message,

And you can get that if you want to write an analyzer. This is what analyzers are really good at, looking for patterns you think are bad and giving warnings/messages about them.

But I thing this warning is a good option as long as that code is somewhat incorrect.

That's a broad statement to be making. If it's correct 99.9% of the time, and somewhat incorrect 0.1% of the time, then you have a situation where the number of false positives drowns out the number of true positives and people won't use your feature/product.

correct way is something like

Again, if you want to code that way, you're welcome to. And you can write features that force you down that path. This came about from the statement of:

in case it would be widely adopted and supported to be painful to use. Can you provide an example of a use case that would be painful?

It would be extremely painful to give users the message that that sort of check was problematic when for nearly all cases for nearly all users that sort of check is not problematic at all. This is the very essence of the perfect being the enemy of the good. Having a perfectly accurate system that can have no null checks isn't helpful if it ends up being so impactful on users that they won't adopt it. There needs to be an acceptable migratory path forward that can give users valuable messages from the start that they should take action on, instead of having those important messages lost in the avalanche of warnings/messages tehy should not have to care about.

If, later on, they want to get to perfection, they can use some analyzer that doesn't let them do any single read/write of anything unless it can be proven totally that it's not null.

[CyrusNajmabadi]

Annoying messages can be disabled in your project if you wish.

If users are so annoyed that they disable these messages, then the feature doesn't help them. The point is to give them actionable highly valuable notifications about problems with their code.

Note: i just had direct experience with this today enabling checks for some existing code. There were a couple dozen issues reported, nearly all of which were real problems that needed addressing. That was a great experience for me because the messages directly led to positive changes, and the amount of false positives i had to deal with was minimal. If i wanted to make my system this clean to the level of pedantry you want here, i would lkely have had to change thousands of lines of code. I wouldn't even bother, and i'd stick with not having hte checks, defeating the entire purpose of going down this path in the first place.

[yaakov-h]

That seems confusing and counter-intuitive, and I'm not sure why it's needed when this is/will-be valid syntax:

if (a?.b?.c is {} x) {
    Foo(x);
}

[CyrusNajmabadi]

@CyrusNajmabadi
I agree that your example needs a syntactic sugar.

I think you're missing the point unfortunately. This is likely the most common pattern out there for doing null checks. If we introduce the feature, can't understand that pattern, and have warnings for this (or make update all their code like this), then the feature will not get adopted. It would be equivalent to thrusting an entirely new language onto people, and that goes against the actual goals of NRT.

Again, if you want to code like this, then go do that. And go write an analyzer to force you to do this. I'll help you write it. But it's not going to happen for the language because it would be awful for people to have to have this experience 1 second after trying to move to NRT.

[savahmel]

@CyrusNajmabadi
I was too hasty writing the previous comment, sorry. Deleted it.
Indeed

if (a?.b is {} x) {
    Foo(x);
}

is uglier and few more characters to type, than

if (a.b != null) {
    Foo(a.b);
}

What you are actually saying, that the community would not be willing to give up an old habit and a few more keystrokes for more robust and less buggy code?

[CyrusNajmabadi]

What you are actually saying, that the community would not be willing to give up an old habit and a few more keystrokes for more robust and less buggy code?

It's not less buggy for them. that's the perspective you seem to be lacking. In many domains that sort of check is 100% reasonable and safe. So yes, this will introduce a lot of friction without improving things for people.

Again, consider if someone said: i'll give you this feature. it will find a real bug in one out of every 10,000 lines of code. but, to use it, you'll have to update thousands and thousands of lines of code to stop using patterns that have been familiar and natural to you since c# 1.0.

99%+ of your code will need to be updated to find an issue in 0.01% of it. That's not a palatable tradeoff for people. it's an enormous amount of work, and it's risky. it also risks alarm fatigue and all sorts of other problems.

[savahmel]

@CyrusNajmabadi

Again, consider if someone said: i'll give you this feature. it will find a real bug in one out of every 10,000 lines of code. but, to use it, you'll have to update thousands and thousands of lines of code to stop using patterns that have been familiar and natural to you since c# 1.0.

I hate being annoying, but I am talking about new code. Not legacy code, which I totally agree that the taken approach makes perfect sense for.

The non-nullable feature (as you know, by far the most wanted on the UserVoice for C#), talks about enforcement. "Enforcement" makes non-nullability compiler's problem. That's my understanding of what is anticipated. I might be wrong of course.

I appreciate you taking the time to respond to all my comments. Thanks!

[CyrusNajmabadi]

I hate being annoying, but I am talking about new code. Not legacy code, which I totally agree that the taken approach makes perfect sense for.

For new code, feel free to use analyzers to be as strict as you want. As i mentioned before, i'd be willing to help you write such an analyzer.

[M-Pixel]

@CyrusNajmabadi has this happened? I would like to avoid this situation: #3752

I tried searching around but couldn't find anything other than these discussions. Nothing comes up on NuGet when I search "default struct null warning" (is there anywhere else one might find additional analyzers?).

As much as I would love to learn how to write custom analyzers, I wouldn't want to waste time re-writing something that has already been written by someone else. This is a general enough problem - evidently a lot of people have wanted solved since the introduction of nullability annotations - so I'd be surprised if nobody has gotten around to writing one. But if that is indeed the case, I'd take you up on that offer for help writing one.

[CyrusNajmabadi]

I appreciate you taking the time to respond to all my comments. Thanks!

You're welcome!

[AndreyMikhailov]

Did somebody actually wrote an analyzer for that? 🙂

[M-Pixel]

That's what I would like to know. The fact that it can be added in a custom analyzer isn't a legitimate reason for not including it among the analyzers that are bundled with the language. By that logic, C# shouldn't provide any warnings whatsoever - you can just write them yourself!

[nathanpovo]

@CyrusNajmabadi any idea how to tackle the following scenarios in a custom analyser?

#nullable enable

class ClassUnderTest
{
    ClassUnderTest()
    {
        int length;

        // A string from a null-oblivious context assigned to a nullable string
        string? nullableString = NullObliviousClass.NullObliviousProp;
        // The NRT flow analysis thinks that this object will never be null
        // SHOULD show a warning
        length = nullableString.Length;

        // A string from a null-oblivious context assigned to a var
        var varString = NullObliviousClass.NullObliviousProp;
        // The NRT flow analysis thinks that this object will never be null
        // SHOULD show a warning
        length = varString.Length;

        // A null from a null-enabled context assigned to a nullable variable
        string? nullableString2 = null;
        // The code below gives a warning (as it should)
        length = nullableString2.Length;
    }
}

#nullable disable

static class NullObliviousClass
{
    public static string NullObliviousProp { get; set; } = null;
}

When NullObliviousClass.NullObliviousProp is assigned to nullableString or varString, the NRT flow analysis thinks that the value will never be null and does not show any warnings when that variable is used in a way that might throw a NullReferenceException exception.

I am trying to figure out how to get a custom analyser to solve/improve this situation.

Assigning NullObliviousClass.NullObliviousProp to a string? or var is not a problem since those are the correct types for a null-oblivious value. So a custom analyser must not show any warnings at that location.

How would I inform the NRT flow analysis that the value (NullObliviousClass.NullObliviousProp) that had been assigned to a string? or a var can in fact be null?

These are the only 2 options I could come up with to handle this scenario (both of which I do not like):

1. Have the custom analyser show a diagnostic around the entire assignment of a null-oblivious value to a nullable variable (a variable with a type like string?) or a var variable. This diagnostic would inform the user that they should be cautious using the variable because the variable might be null even though the NRT flow analysis thinks it will never be null.
2. Implement a custom NRT flow analysis in the custom analyser to be able to show a diagnostic whenever the variable is used.

[nathanpovo]

Thanks for the response.

Wouldn't that involve a crazy amount of work to basically re-implement the entire built-in NRT flow analysis from scratch? 😅

Is there really no way of informing the built-in NRT flow analysis that a particular variable should be considered as "nullable" even though the NRT flow analysis already determined that the variable is "definitely not null"? I tried to do some research on this but couldn't find much on such a niche topic.

[CyrusNajmabadi]

Wouldn't that involve a crazy amount of work to basically re-implement the entire built-in NRT flow analysis from scratch?

Not really. Just add cases for the simple things you care about.

Is there really no way of informing the built-in NRT flow analysis that a particular variable should be considered as "nullable"

One way would be to fork the compilation, adding in something like an explicit = null assignment to that variable, then see what diagnostics you get in the new code. Then map those back to the original code (really simple as you cna just subtract the positions in teh new code, minus the span-length of the inserted assignment.

[CyrusNajmabadi]

You could even be sneaky, and introduce the nodes with empty spans (really ugly hack, but technically doable). Then redo the analysis and take the new diagnostics and report those.

[nathanpovo]

Just add cases for the simple things you care about.

This is always the plan but scope creep gets us all in the end.

On top of that, I'd rather not have to re-implement as complex as NRT flow analysis.

One way would be to fork the compilation, adding in something like an explicit = null assignment to that variable, then see what diagnostics you get in the new code.

This sounds interesting. Thanks for bringing it up. I'll check a bit on this.

If I can get that working, I would end up getting all the built-in NRT flow analysis for "free" without have to implement a million different scenarios related to assignments.

[nathanpovo]

Your suggestion worked beautifully

I swapped out the null-oblivious fields/properties/methods/etc and compiled the result. From that, I just get the diagnostics and show them to the user.

You can take a look at the code here if you are interested https://github.com/nathanpovo/NullableReferenceTypes.StrictMode