Should CacheExtensions.GetOrCreate()/GetOrCreateAsync() return TItem instead of TItem??

[kevinchalet]

Take this snippet:

using Microsoft.Extensions.Caching.Memory;
using Microsoft.Extensions.DependencyInjection;

var services = new ServiceCollection();
services.AddMemoryCache();

var provider = services.BuildServiceProvider();
var cache = provider.GetRequiredService<IMemoryCache>();

var instance = cache.GetOrCreate("key", entry =>
{
    // ...
    return new
    {
        Message = "Hello World"
    };
});

Console.WriteLine(instance.Message);

On .NET 6.0, you don't get any error or warning as the caching stack was not decorated with nullable annotations.
On .NET 7.0, you get a CS8602 warning on the last line, as GetOrCreate() returns TItem? even if the delegate itself will never return null.

I guess the warning is technically correct because - in theory - nothing prevents me from inserting a null value in the cache before the delegate has a chance to be called, but I'm not sure it's a frequent case (in this case, it would be more logical that the delegate signature use TItem? instead of TItem to account for potential null values).

To make them easier to use, should CacheExtensions.GetOrCreate()/GetOrCreateAsync() return TItem instead of TItem??

[ghost]

Tagging subscribers to this area: @dotnet/area-extensions-caching
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Take this snippet:

using Microsoft.Extensions.Caching.Memory;
using Microsoft.Extensions.DependencyInjection;

var services = new ServiceCollection();
services.AddMemoryCache();

var provider = services.BuildServiceProvider();
var cache = provider.GetRequiredService<IMemoryCache>();

var instance = cache.GetOrCreate("key", entry =>
{
    // ...
    return new
    {
        Message = "Hello World"
    };
});

Console.WriteLine(instance.Message);

On .NET 6.0, you don't get any error or warning as the caching stack was not decorated with nullable annotations.
On .NET 7.0, you get a CS8602 warning on the last line, as GetOrCreate() returns TItem? even if the delegate itself will never return null.

I guess the warning is technically correct because - in theory - nothing prevents me from inserting a null value in the cache before the delegate has a chance to be called, but I'm not sure it's a frequent case (in this case, it would be more logical that the delegate signature use TItem? instead of TItem to account for potential null values).

To make them easier to use, should CacheExtensions.GetOrCreate()/GetOrCreateAsync() return TItem instead of TItem??

Author:	kevinchalet
Assignees:	-
Labels:	area-Extensions-Caching
Milestone:	-

[tarekgh]

@eerhardt what you think about this one?

[eerhardt]

This looks like the opposite of the request in #77264. In #77264, it is showing that you can cache null as a value in the IMemoryCache.

If someone cached null as the value for item, calling GetOrCreate() will return null, and not invoke your factory method.

So I believe the existing annotation is correct.

[kevinchalet]

This looks like the opposite of the request in #77264. In #77264, it is showing that you can cache null as a value in the IMemoryCache.

No, the two are actually unrelated (being able to cache null values has always been supported, otherwise I wouldn't have used it). The other issue is just an oversight affecting an extension.

If someone cached null as the value for item, calling GetOrCreate() will return null, and not invoke your factory method.

I mentioned that in my OP but my point is that GetOrCreate() is typically always used alone - as a one-shot operation - in the wild. You never do something like that:

cache.Set<object?>("key", null);

var instance = cache.GetOrCreate("key", entry =>
{
    // ...
    return new
    {
        Message = "Hello World"
    };
});

While technically correct, it semantically doesn't make much sense that the nullability of the returned type is not respected when it's used alone:

var instance = cache.GetOrCreate("key", entry =>
{
    // ...
    return new
    {
        Message = "Hello World"
    };
});

// You don't expect a null value here as the delegate doesn't return null.

[kevinchalet]

And BTW, Set() + GetOrCreate() is a terrible combination. Take this snippet, similar to the previous one, but using a delegate that returns a non-nullable value type:

using Microsoft.Extensions.Caching.Memory;
using Microsoft.Extensions.DependencyInjection;

var services = new ServiceCollection();
services.AddMemoryCache();

var provider = services.BuildServiceProvider();
var cache = provider.GetRequiredService<IMemoryCache>();

cache.Set<object?>("key", null);

// The returned type is inferred as "int" based on the delegate signature.
var instance = cache.GetOrCreate("key", entry =>
{
    // ...
    return 10;
});

Console.WriteLine(instance);

If you run this snippet, all you'll get is a null-reference exception when calling GetOrCreate().

[eerhardt]

While technically correct

The point of nullable annotations is to be able to answer questions like:

• Is it possible for this method to return null?
• Does this method accept null?

From the method signature. And to statically warn you at compile time when it is possible for you to get NullReferenceExceptions at runtime.

If this API was annotated the way you are suggesting, it would be possible for your code to get NREs at runtime, with no warnings at compile time. This goes against the spirit of the annotations.

So while it is not likely, or not often the case, it is still possible. This is why it is annotated this way.

If IMemoryCache didn't have object as its Value, and instead was generic - IMemoryCache<TValue> (see #48455 and #48567), then this API could be annotated the way you want. But since it can take any object (including null), it is possible for this API to return null.

[eerhardt]

If you run this snippet, all you'll get is a null-reference exception when calling GetOrCreate().

This is another disadvantage of MemoryCache not being generic. You are trying to use strong-typing, but everything is weakly typed underneath. So there are no guarantees your strong typing will work.

[ghost]

This issue has been marked needs-author-action and may be missing some important information.

[tarekgh]

Per @eerhardt replies, it looks preferable to not do the suggested change. @kevinchalet I am closing the issue but feel free to reply if you still have any questions.

[kevinchalet]

From the method signature. And to statically warn you at compile time when it is possible for you to get NullReferenceExceptions at runtime.

I maintain a large project that was among the very first third-party projects to adopt nullable references, so believe me, I have a bit of experience regarding how it works 😄

I don't think things are always black or white. There are tons of examples in .NET or ASP.NET Core where properties were deliberately marked non-nullable based on the context even though they can technically be set to null.

Basically most of the "options" classes work this way. For instance, https://github.com/dotnet/aspnetcore/blob/9a765c84d38d3dcc6beab7233e7908b163aa9517/src/Security/Authentication/OpenIdConnect/src/OpenIdConnectOptions.cs#L246 is not nullable and is later initialized by an IPostConfigureOptions<>, but nothing prevents me from removing that service or adding an implementation that will set that property back to null. And in this case, I'll get a null-reference exception. Yet, this property is not marked as nullable because it's not null when used the "normal way". Doing otherwise would be a source of unnecessary warnings.

It's also interesting to note that reducing noisy warning also led to adopting oblivious return types. For instance, Enumerable.Cast<TResult>(): #42215

Anyway, it's not a big deal. But I wouldn't be surprised if you had similar remarks in the future.

[stephentoub]

I don't think things are always black or white. There are tons of examples in .NET or ASP.NET Core where properties were deliberately marked non-nullable based on the context even though they can technically be set to null.

One of our overarching principles for nullable annotations in the core libraries it that they should never lie and say null isn't a possible result when in fact it is. This does lead to some cases where a method is annotated as returning a T? even if null is rare or relegated to a corner-case, e.g. Activator.CreateInstance<T> returns T? because for example it can actually be null if you specify a Nullable<T> as the type, but it means the nullability of the return value can be trusted: if it says it's non-nullable, then it won't ever be null, and callers don't need to guard against dereferencing null. The principles are outlined in https://github.com/dotnet/runtime/blob/main/docs/coding-guidelines/api-guidelines/nullability.md. One escape valve we've used in just a few places is to explicitly disable annotations when we simply lack the means to properly express the contract and what we could express would be so misleading as to be problematic (you can search the codebase for #nullable disable, in particular in ref .cs files, to see some examples).

[kevinchalet]

if it says it's non-nullable, then it won't ever be null, and callers don't need to guard against dereferencing null.

In many cases, I don't doubt it's true, but I'm still convinced there are cases where you can't guarantee that without considering the context around the API you're calling.

Take ConcurrentDictionary<TKey, TValue>.GetOrAdd(TKey key, Func<TKey, TValue> valueFactory) for instance, which is actually very close to the API discussed in my OP:

using System.Collections.Concurrent;

var dictionary = new ConcurrentDictionary<string, Record>();
dictionary.TryAdd("key", null!);

var result = dictionary.GetOrAdd("key", key =>
{
    // ...
    return new Record("Hello World");
});

Console.WriteLine(result.Message);

public record Record(string Message);

Since GetOrAdd(...) returns TValue, doing result.Message doesn't result in a warning here. Yet, since we injected a null value in the dictionary before GetOrAdd(...) was called, you'll get a null reference exception when running this snippet.

Following your logic, GetOrAdd(...) should return TValue? instead of TValue because it's actually possible for the API to return a null value.

One escape valve we've used in just a few places is to explicitly disable annotations when we simply lack the means to properly express the contract and what we could express would be so misleading as to be problematic (you can search the codebase for #nullable disable, in particular in ref .cs files, to see some examples).

I indeed mentioned one of them (Enumerable.Cast<T>) at the end of my last point 😃

[stephentoub]

Yet, since we injected a null value in the dictionary before GetOrAdd(...) was called, you'll get a null reference exception when running this snippet.

But that's because the user of the dictionary lied:

dictionary.TryAdd("key", null!);

by using null suppression. Once you do that, all bets are off, and no nullable annotation can be correct.

[kevinchalet]

But that's because the user of the dictionary lied:

dictionary.TryAdd("key", null!);

by using null suppression. Once you do that, all bets are off, and no nullable annotation can be correct.

Sure, but it can be more subtle than that, for instance when the dictionary is passed to code that isn't nullable-aware ; e.g a third-party helper that injects null in the dictionary:

var dictionary = new ConcurrentDictionary<string, Record>();
ThirdPartyAssembly.Helpers.PopulateDictionary(dictionary);

In this case, neither the author of the third-party library nor the author of this snippet is suppressing any nullable-related warning so there's no way for any of them to know that null could be returned here as the annotations are saying otherwise.

(I guess we just disagree on how strong nullability guarantees are: to me, the context matters a lot)

Anyway, I appreciate your insights 😃

[stephentoub]

Sure, but it can be more subtle than that, for instance when the dictionary is passed to code that isn't nullable-aware ; e.g a third-party helper that injects null in the dictionary:

Such code is still violating the nullability contract the developer of dictionary put in place, in which case either PopulateDictionary is buggy or the developer of dictionary used the wrong annotation and should have used ConcurrentDictionary<string, Record?>. Garbage in, garbage out.

Anyway, I appreciate your insights 😃

👍

[kevinchalet]

Such code is still violating the nullability contract the developer of dictionary put in place, in which case either PopulateDictionary is buggy or the developer of dictionary used the wrong annotation and should have used ConcurrentDictionary<string, Record?>. Garbage in, garbage out.

I agree, but the "nullability contract" you're referring to here is not something "concrete" that can be verified by the compiler and that materializes to the user as nullability warnings, so it's actually more like a "functional-mental" contract where you basically pray for everything in the chain to do the right thing.

And once you say that, it's not very far from the example in my OP, where you also expect nothing/nobody will inject a null value with the same key before you call GetOrCreate().

I get the distinction between these cases is quite subtle (and that's why I personally think we should never consider nullable annotations as super-strong guarantees).

We obviously have different perspectives but it's a very interesting conversion so thanks for that.