Runtime: Consider making `Enumerable.Cast<TResult>` return `IEnumerable<TResult?>` instead of `IEnumerable<TResult>`

[stephentoub]

Technically it should probably return IEnumerable<TResult?>, as no matter what TResult is specified, the input IEnumerable could contain nulls. In practice, since the developer is supplying the TResult, this may be more annoying than helpful.

@cston, @jcouv, what do you think should be done here?

[msftbot[bot]]

Tagging subscribers to this area: @eiriktsarpalis
See info in area-owners.md if you want to be subscribed.

[stephentoub]

Related: https://github.com/dotnet/runtime/issues/40481

[huoyaoyuan]

Cast<TResult?>() is a workaround, but it doesn't have constraint to correct the usage.
With this change, IEnumerable<TBase>.Cast<TDerived> will be painful, if the source sequence is known to be non-null.

Maybe this can be resolved by a first-party analyzer to solve the known non-null scenario?

[eiriktsarpalis]

Technically it should probably return IEnumerable, as no matter what TResult is specified, the input IEnumerable could contain nulls.

But I suppose that is generally the case for non-nullable reference types? I would prefer explicitly using Cast<TResult?>() here.

[stephentoub]

But I suppose that is generally the case for non-nullable reference types?

The most common case is where the T guides both inputs and outputs, such that the output would only be violated if the developer supplied invalid input. Think List<T> for example. Here, though, any T can be supplied, and doesn't influence the inputs, and thus can be a lie for the outputs.

[eiriktsarpalis]

The arbitrariness of the T type means that there is risk of InvalidCastException, but that should be independent of whether or not the source yields null values.

Given that Cast<T>() is typically used with old APIs returning IEnumerable, where it is common for null values to be present, I can see why the proposed signature might set up users for success. But on the other hand it will be frustrating to use in cases where the source is known to produce non-null values.

[adamsitnik]

I've moved it to future as it's not 100% clear that we should do it and there is a code freeze happening on Monday.

[stephentoub]

I think we either need to do this now or close it and never do it. Changing it later from IEnumerable<T> to IEnumerable<T?> will potentially introduce warnings into nullable-enabled code.

[adamsitnik]

I think we either need to do this now or close it and never do it.

I see your point.

I am not a nullability expert, but my take on this is the following: since Cast can return nulls, while OfType filters them out, the definition of OfType should remain as it is while Cast should be changed to return ?

IEnumerable<TResult> OfType<TResult>
IEnumerable<TResult?> Cast<TResult>

so users who don't want nulls at all use OfType

static void Main(string[] args)
{
    ArrayList arrayList = new ArrayList();

    arrayList.Add("Hello");
    arrayList.Add("World");
    arrayList.Add(null);

    System.Collections.Generic.IEnumerable<string> casted = arrayList.Cast<string>();
    Console.WriteLine(casted.Count(x => x == null));
    System.Collections.Generic.IEnumerable<string> ofType = arrayList.OfType<string>();
    Console.WriteLine(ofType.Count(x => x == null));
}

1
0

@eiriktsarpalis @stephentoub if you both agree I am going to send a PR

[eiriktsarpalis]

so users who don't want nulls at all use OfType

It's not the same though. OfType<string>() filters the non-null elements at runtime, however Cast<string>() (with the current annotation) asserts at compile time that every element in the sequence is non-null. That assertion could be invalidated at runtime, but clearly this is true for any generic collection with non-null type parameters.

I'm not convinced we should make this change, but won't die on this hill either :-)

[stephentoub]

This really ends up being a question of correctness vs usability. T? is correct, but likely to be annoying; T could easily lead to nulls in things that swear they're non-null, but won't give as many warnings. And even though the nullable reference type feature isn't perfect, its purpose is to warn when things that may be null are dereferenced. So I think we need to change it.

[stephentoub]

(Note a few other APIs were also dup'd against this issue.)

[eiriktsarpalis]

@stephentoub I will try to submit a PR making the change today. What are the other APIs I would need to update as well? Presumably the ones listed in #40481?

[stephentoub]

Those are the only ones I'm aware of. I previously changed the others I knew about.

[333fred]

@stephentoub @eiriktsarpalis it might be too late to consider any changes here, but this just doesn't seem like a correct change and is introducing new warnings in roslyn. This makes Cast a very painful API to use: I _don't_ have nulls in my input list. OfType is not correct either: I want an InvalidCastException if something wasn't what I expected, not for those things to be silently filtered out. /cc @cston

[stephentoub]

this just doesn't seem like a correct change

I agree it can be painful, but it is more correct (and there are lots of ways the language forces such "pain" with nullable reference types): regardless of the type you supply in the Cast call, the output can contain nulls... that's the prototypical use case for T?. What do you propose instead? Given the facilities the language provides, I don't see how it can be done better. If the language gets more fine-grained ability to control this, the API can be updated.

[333fred]

I would propose leaving it unannotated. To me, this case is not meaningfully different than this code:

string[] s = new string[10];
s[0].ToString(); // No warning, but null reference at runtime

Yes, there can be nulls, but NRT is very pragmatic. Given that there is no way to annotate this, I believe we should do the pragmatic thing here and leave T unannotated.

[stephentoub]

Given that there is no way to annotate this, I believe we should do the pragmatic thing here and leave T unannotated.

There is a way to annotate this, as has been done. Yes, it results in null warnings when they may not be warranted; that is inherent to the NRT feature as well and happens in tons of places... just look at how many hundreds or thousands of !s we need in dotnet/runtime.

this case is not meaningfully different than this code

But it is. The language made the choice for pretending a new array didn't contain null because there was no better option. Here though we're talking about API, where the annotations serve as documentation of behavior as well as feeding into tooling like the compiler. It's more important that the APIs don't lie. Leaving it without the ? lies. If the concern is getting some more warnings that need to be !d when they're not needed, there are many bigger fish to fry.

[333fred]

There is a way to annotate this, as has been done.

A real way to annotate this would be to link the nullability of the input type parameter to that of the output type parameter. We don't have such an annotation today: further even if we did, this API exists on IEnumerable, not IEnumerable<T>, so there isn't even a T to link to. That seems quite unfortunate (though I understand that it's the case because partial type inference is not a thing that C# has), and it makes this API unfortunate as a result.

[stephentoub]

and it makes this API unfortunate as a result

I don't disagree; it's "unfortunate" either way. But the purpose of the NRT feature when it comes to API is to document where nulls are possible/allowed and to use that information to inform the compiler where to issue warnings. Leaving off the ? when the language affords the ability to use one documents "if you specify a non-nullable T, this will never yield null". We don't have a ? on the T in OfType because that's actually the case: it won't yield nulls. But Cast can.

[stephentoub]

ps If you believe it's that impactful, the compiler could special-case it (and it has more info in many cases about the source type). Just one more LINQ method the compiler knows about.

[333fred]

@jcouv, could you add this to your list of Linq methods to consider special casing?

[jcouv]

If I understand correctly, we're trying to constrain TResult so that if TInput is annotated then the type substituted for TResult must be annotated as well, in IEnumerable<TResult> IEnumerable<TInput>.Cast<TResult>().

This feels different than the current issue with Where method (nullability of returned elements should depend on logic in lambda), tracked by https://github.com/dotnet/roslyn/issues/39586

@333fred I'd suggest filing a separate issue. I like the idea of using an analyzer for this scenario.

[jcouv]

Summarized our options with Chuck. Both @cston and I dislike options in the 0 category (ie. we think the API should return TResult without annotation):

1. status quo: return TResult? (annotated)
   a. no other change
   b. compiler infers IEnumerable<TResult!> if input is IEnumerable<TSource!> (despite what the API says)
2. return TResult! (un-annotated)
   a. no other change
   b. add analyzer that gets input type from compiler (semantic model) and checks substituted TResult's annotation. ((IEnumerable<string?>)x).Cast<string>() // warning
   c. do the same as (1b) but inside compiler
3. return TResult~ (oblivious)

[RikkiGibson]

Could you please clarify what difference in practice there is between non-nullable TResult and oblivious TResult~ in IEnumerable<TResult>?

Is scenario analogous to the 'foreach on non-generic collections' issue #37918, where we chose to make object IEnumerator.Current oblivious?

[jcouv]

@RikkiGibson Using oblivious is slightly better from a documentation perspective, so that we're not returning a null from an API that claims to only produce not-null values. You're right, it's only a small difference.

[stephentoub]

To me, this is straightforward:
Even if a dev specifies a non-nullable TResult, yes or no, can the method yield null? The answer is "yes", so it should be TResult?. Otherwise, the annotation is simply wrong. That means option (1) above isn't valid.

I like (0b). There are plenty of places something is annotated as being maybe-null and the compiler can tell it's not-null; that's true even for method return types, thanks to NotNullIfNotNull. We just lack the attribute/annotation that lets us express that, hence special-casing it in the compiler.

If we don't believe it's worth the effort for 0b, then to me that says this isn't particularly important. But if we still believe it to be, then admitting defeat and making it oblivious so that the API doesn't lie is the only out.

[danmosemsft]

@stephentoub we can close this now right?

[stephentoub]

we can close this now right?

Yes. Not sure why GitHub didn't automatically close it when #42216 was merged.