43

I've recenlty been greeted by CS8603 - Possible null reference return, which indicates that my code could possibly return null. It's a simple function that looks up an entity in a database by id - if it exists, it returns the entity. If not, it returns null.

public TEntity Get(Guid id)
{
    // Returns a TEntity on find, null on a miss
    return _entities.Find(id);
}

My gut feeling says that this makes sense. If the client says "Give me the user with ID 82739879", then giving them "nothing" makes intuitive sense, if no user with said ID exists".

However, the compiler warning caused me to re-think this approach. Should I do something else, other than returning null, if the user ID could not be found? Returning an exception is a possibility, but I don't consider a user not existing to be an "exceptional" state.

Is it "wrong" to return null in this case? Am I overthinking this?

15
  • 14
    This question might be helpful, and it also links to other questions you might find helpful: If null is a billion dollar mistake, what is the solution to represent a non-initialized object? Nov 8 at 16:24
  • 2
    @VincentSavard while indeed that link is interesting in general it does not apply to this question - OP explicitly said the method returns non null result by not specifying TEntity? and compiler simply warns them that there is some obvious mismatch between what they promised to return and what code actually does. Nov 8 at 21:58
  • 4
    @AlexeiLevenkov Back when I started C#, the TEntity? syntax didn't exit. It was implied that any object could be null.
    – MechMK1
    Nov 8 at 23:46
  • null is NOT nothing... null is a value, not the absense of a value. That's what the warning tells you.
    – jwenting
    Nov 9 at 12:30
  • 2
    I see no problem conceptually. If you request to find an object from a map with a key and if the object with a matching key doesn't exist, null is a very legitimate way to communicate that without introducing needless new and non-standard ways to communicate "nothing" as a return value. You could throw an exception or whatnot, but developers can fail to handle that too. Null is a very precise communicator of nothing. I suspect this is paradigm-specific.
    – user379844
    Nov 10 at 20:12
72

Why are you getting the warning?

You have enabled the nullable reference types (NRT) feature of C#. This requires you to explicitly specify when a null may be returned. So change the signature to:

public TEntity? Get(Guid id)
{
    // Returns a TEntity on find, null on a miss
    return _entities.Find(id);
}

And the warning will go away.

What is the use of NRTs?

Other recent changes - specifically around pattern matching - then tie in really nicely with NRT's. In the past, the way to implement the "try get pattern" in C# was to use:

public bool TryGet(Guid id, out TEntity entity)

Functional languages offer a better approach to this: the maybe (or option) type, which is a discriminated union (DU) of some value and none. Whilst C# doesn't yet support DU's, NRT's effectively provide that maybe type (or a poor man's equivalent) as TEntity? is functionally equivalent to Maybe<TEntity>:

if (Get(someId) is TEntity entity)
{
    // do something with entity as it's guaranteed not null here
}
else
{
    // handle the fact that no value was returned
}

Whilst you can use this type of pattern matching without using NRTs, the latter assists other developers as it makes clear that the method will return null to indicate no value. Change the name to TryGet and C# now provides that functional style try get pattern:

public TEntity? TryGet(Guid id) => _entities.Find(id);

And with the new match expression, we can avoid out parameters, mutating values etc and have a truly functional way of trying to get an entity and creating one if it doesn't exist:

var entity = TryGet(someId) switch {
    TEntity e => e,
    _ => Create(someId)
};

But is it wrong to return null?

There has been vast amounts written on why null was the billion dollar mistake. As a very crude rule of thumb, the existence of null likely indicates a bug. But it's only a crude rule of thumb as there are legitimate use-cases for null in the absence of Maybe<T>. NRT's bridge that gap: they provide a relatively safe way of using null to indicate no value. So I'd suggest - for those using newer versions of C# - there is nothing wrong with returning null as long as you enable the NRT feature and you stay on top of those CS8603 warnings. Enable "treat warnings as errors" and you definitely will stay on top of them.

18
  • 9
    null can cause many problems, therefore it should be avoided. But there are legitimate cases where it can be used. With the NRT feature, C# now requires you to explicitly state when a null can be returned by using TEntity?.
    – David Arno
    Nov 8 at 16:27
  • 2
    @DavidArno so what would happen in this case if the "Find" method would not find any results?
    – matkv
    Nov 8 at 17:27
  • 2
    @matkv: please do yourself a favor and take the time to read the already existing questions about the issue on this site, like this one
    – Doc Brown
    Nov 8 at 18:35
  • 4
    I don't agree that returning null likely indicates a bug. Is it a bug that a search couldn't find any matching items? No. Any function that returns a pointer to an object that doesn't necessarily exist could return null without being a bug. Nov 10 at 2:25
  • 3
    @JonathanWood The point is that the vast majority of your code shouldn't deal with possibly null values. Even in pre-NRT code, 99% of the time, you assume a reference will not be null - and a violation of that assumption is a very common cause of software errors. NRT allows you to be explicit about the parts that deal with nulls, and keep the rest of your code clean, while also getting support from static analysis to detect cases where you accidentally turn a nullable type into a non-nullable type without actually guaranteeing it's not going to be null.
    – Luaan
    Nov 10 at 19:39
19

David Arno already answered your question about the specific warning, so I'd like to address your general question:

What's wrong with returning null?

Nothing, as long as the consumer of your method is aware that null might be returned, and, thus, it is their responsibility to react appropriately.

If your language supports null annotations: Use them. If it doesn't (e.g., if you are stuck with a classic .NET Framework 4.8 project), my go-to solution is to name the method appropriately, i.e., GetOrNull, GetIfExists, etc. Often, I have both (Get throwing an exception and GetOrNull returning null), if I need to cover both use cases:

public TEntity GetOrNull(Guid id) => _entities.Find(id);

public TEntity Get(Guid id) => 
    GetOrNull(id) ?? throw new ArgumentException($"Entity {id} not found.");
14
  • 5
    The general advice here is good, but if I saw GetOrDefault, I would assume it didn't return null, but returned some default value of the relevant type - either one provided as a parameter, a "null object" implementation, or something newly generated. GetOrNull and GetIfExists make much more sense for the case where you are returning a null value.
    – IMSoP
    Nov 9 at 11:28
  • 6
    @Haukinger: The contract of a method is more than just its signature. For example, I know that String.Substring won't return null, even though its return type is not a struct. How do I know that? Because the documentation says so. Likewise, I know that GetIndexOf will never return -2 (for the same reason), even though this is not guaranteed by the type system. Unfortunately, consumers don't always check the docs (and old C# versions don't support null annotations).
    – Heinzi
    Nov 9 at 13:03
  • 3
    @Haukinger: Regarding signatures: True. In theory, the consumer should either check the docs or be prepared for anything. In practice, in my experience, consumer's don't, hence my suggestion to use a hint in the method name. It's just a pragmatic way to reduce the likelyhood of NullReferenceExceptions.
    – Heinzi
    Nov 9 at 14:10
  • 9
    @Haukinger: Regarding checking for unexpected values in critical scenarios: This is a double-bladed sword. Yes, adding an assertion that crashes the component in a controlled way if an unexpected value is returned is a good idea. However, all too often I've seen code like this, written by people who heard that "defensive programming" is a good thing without understanding it: var a = SomeMethod(); if (a != null) { /* remainder of code */ } with an implied else { /* silently do nothing */ }. This is usually worse than crashing with a NRE, since it hides the error.
    – Heinzi
    Nov 9 at 14:14
  • 4
    @Heinzi Spot on. Checking for null returned by something you call usually isn't enough; you need to be able to do something sensible when you get null (i.e. fulfill your own contract). That takes a lot more thought, and often that just flat out isn't possible. If you absolutely need the value to have methods or attributes you might as well just blindly access them and get the NRE if you're wrong. It can be worthwhile to check whether something that wasn't supposed to be null actually isn't before you pass it on to someone else though, to move the exception closer to the problem.
    – Ben
    Nov 10 at 10:39
4

So as per This answer, you can make the warning go away by explicitly specifying when a null might be returned.

Think of the caller however - they're still going to need to do a check for null.

I prefer using the Try Get pattern. You return a boolean, with the actual return value you were after as an out parameter, like so:

public bool TryGet(Guid id, out TEntity entity)
{
    entity = _entities.Find(id);

    return entity != null;
}

For the caller, this can be quite a bit more convenient. It makes it very easy to use a guard pattern such as the following:

TEntity entity;
if(!TryGet(id: idIWantRecordFor, out entity))
{
    entity = new TEntity
    {
        Id = Guid.NewGuid(),
        SomeProperty = "Some content"
    };
}

// at this point, no matter what, we've got a usable TEntity
// either one we got from the db, or a default, or a new one, etc.
Console.WriteLine(entity.SomeProperty);
7
  • 5
    This pattern makes sense when you cannot reliably say whether a null returned was an "error signalling null" or an actual value of null, because null is a valid value in that case. So for example on Dictionaries, this pattern makes sense, since null is a valid value to store in a dictionary. As a "guard pattern" it's useless, it does exactly the same in exactly the same amount of code as a null check.
    – nvoigt
    Nov 9 at 6:57
  • 3
    In fact the guard pattern now has two connected variables, a bool and an item and you have to do extra work to let the compiler and static analysis know that they belong together and that their values depend on each other. With a simple null-check any compiler or tool can trivially find out whether the local variable is null or not and give you warnings about it. Resharper did that years before any notion of nullable objects being a special thing in the C# language.
    – nvoigt
    Nov 9 at 7:03
  • 1
    And now that C# has pattern matching, you get pretty if-syntax with null-returns too. Nov 9 at 8:19
  • 2
    Five years ago, this would have been good advice, But C# has moved on hugely and these days I'd not recommend people write C# in this way. This pattern is much better served these days via T? and pattern matching.
    – David Arno
    Nov 9 at 9:15
  • 2
    And those 9 lines of code because TryGet() forces me into an if clause are supposed to be better than TEntity entity = Get(id) ?? new TEntity(Guid.NewGuid());? Nov 10 at 11:56
4

There is nothing wrong with null pointers in principle.

What's wrong with older languages is that there is no way to know whether a pointer could be null or not. So you have to check whether it's null, even though according to your code it never can be null. Wasting code and time. Of course the one time where you don't check it's Null. The other extreme is when data can be present or absent and you have a language that doesn't allow a pointer to be non-null. Then you have to produce a pointer with fake data if the data is absent, just as bad.

Newer languages have "optional" values. In Swift, for every type T there is a type optional<T>. So your TEntity CANNOT be null. But an optional<TEntity> can be null. Normal code doesn't allow you to try to access the data in an optional<TEntity>. There is a special "if" that lets you try to extract the TEntity, while at the same time reporting success or failure, and if extracting the TEntity failed, then it is not accessible.

The language doesn't allow unnecessary tests - if you have a variable of type TEntity it CANNOT be null, and the compiler doesn't allow you to test if it is null. If it is of type optional<TEntity> then you MUST test if it is null in some way. So you get 100% safety, And you can very easily report that some value isn't present.

if let result = something.get(guid: someGuid) {
    // result is a valid TEntity
} else {
    // There is no TEntity, and no variable "result"
}
4
  • 1
    I like that. My only concern is that two years later, although I was originally sure that the return value can never be null, a new use case emerges where it is possible, and I have to come up with a fake value, so I'm stuck with the worst of both worlds ;-). Nov 10 at 12:02
  • No, if it suddenly can be null, you change the lowest function that might return null to return an optional, and the compiler will tell you the rest. Obviously if it can be null (patient refuses to give their date of birth), that’s not a trivial change. The problem isn’t the null, but the fact that the date of birth is not present, and you have to handle that. Optional values make clear where changes are needed.
    – gnasher729
    Nov 11 at 7:22
  • @Peter-ReinstateMonica: Indeed. I often see code which fails with NPE when it checks something.isEmpty(), thinking that something is an Optional. And then you get double-bad code, needing to check for null first, then for isEmpty, and finally getting the value if there's one. :-( Nov 11 at 8:46
  • 1
    @EricDuminil One of my most useful functions when I programmed C# was a public static bool INoE(this IEnumerable). "INoE stands for "IsNullOrEmpty", but it had to be short because it was used all the time. Suddenly I didn't have to worry any longer whether a function author had chosen to return an empty collection or null when nothing was found. Nov 11 at 9:02
0

"In the days of yore," functions indicated error-conditions by returning particular values. Unfortunately, this put the burden upon every direct or indirect caller of those functions to "always do the right thing."

As time went on, we realized the value of "exceptions."

So: When you realize that "no user exists," instead of indicating this by a return-value (such as NULL ...) you treat it as "an exception." Instead of "returning" anything, instead you create an exception-object and throw it up into the air ... expecting someone to "catch" it.

In practice, this works quite well: "ordinary" program logic is able to proceed, simply assuming that "if I am still here, nothing went wrong." Exceptional cases are moved to an entirely different path which consists of "throwers" and "catchers."

Any point in the program which detects an exceptional condition can "throw." Meanwhile, the "catchers" don't have to care where the "throw" actually came from.

2
  • 3
    The absence of something, where absence is a valid state, is not an exceptional condition. I think (ab)using exceptions for something like that is a bad pattern.
    – Domi
    Nov 11 at 15:35
  • @Domi: that id is a guid, I would really question the idea that a record identified by a guid not existing being a valid state.
    – jmoreno
    Nov 27 at 19:40
0

What does it mean to get a record by id when that record doesn’t exist? That sounds like an error to me. I have tons of code that tries to get a record by id and then throws if the record is null, because that is the record I am supposed to be displaying or changing, and if it doesn’t exist there’s nothing else sensible I can do.

Throwing an exception is what you should do when you don’t have a way to do what you are supposed to do. Your signature says you are supposed to return a TEntity, are you returning a TEntity? No, you are sometimes returning a null value. Either your signature doesn’t say what you want it to, or you should throw.

2
  • Whether a value of type TEntity should represent an object of type TEntity, a non-null pointer to an object of type TEntity, or a possibly null pointer to an object of type TEntity is a question of language-design. Yes, the third option is an unfortunate choice. Nov 27 at 10:36
  • @Deduplicator: whether you can make the distinction is a matter of language design, and in C# you can. A signature of public TEntity? Get(Guid id) would say that it can return null, the current signature says it can’t which is why the OP is getting a warning. The OP needs to decide what the function does, and change the code/signature accordingly.
    – jmoreno
    Nov 27 at 12:59
-1

I read somewhere that you should always try to separate your valid data variables from the status variables. So using the return value for both data and status is not advised.

-4

There is absolutely nothing wrong with returning null if the business case says the data is optional. I worked as a senior Java developer in large enterprise for 10 years, and had my share of null pointer exceptions to fix. After a while I realized that the majority of the exceptions indicated there was business use case that we didn't handle correctly, if at all. For instance our mainframe database would specify that customer date of birth was a mandatory field and would never be null. But it turns out we did not collect this field for the first several years, so some records would have null value. You can't fix this without revisiting the business use case - what should we do with ancient customer data? Hiding the exception with Optional and the like fixes the technical problem but creates a subtle bug. Long live the NPE!

3
  • 16
    I disagree with this. The point of non-nullable types is this: Your business logic says that the date of birth is mandatory, so you add a non-nullable Date to your record. Then, you run into problems when importing old data. You discuss your business logic and decide that the date of birth is not mandatory after all, so you change it to Optional<Date>. And now you profit: Because the types don’t match up anymore, the compiler will tell you all the places where you are using the date of birth, so you can go through them one by one. Crucially, you don’t need to wait until you run into a NPE. Nov 9 at 8:08
  • 2
    In Swift, you cannot get a null pointer exception. The compiler doesn't let you access data referenced by a null pointer. But anyway, what if i don't want to give you my date of birth? It's none of your business (in most cases).
    – gnasher729
    Nov 9 at 14:16
  • @gnasher729: You definitely can ask my date of birth. It's usually 01-01-1900, 01-01-1970, or 01-01-(CurrentYear - 21). Nov 11 at 8:47

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.