Codegen for spreading a null array does not throw NullReferenceException

[jnm2]

Version Used: 17.11 preview 6

The following code should throw NullReferenceException, but it does not:

int[]? maybeNull = null;
int[] clone = [.. maybeNull];

In both https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/proposals/csharp-12.0/collection-expressions#known-length-translation and https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/proposals/csharp-12.0/collection-expressions#unknown-length-translation, the translation uses foreach over the expression being spread, which would equivalently throw:

foreach (var _ in maybeNull) // Throws (and shows NRT warning, see https://github.com/dotnet/roslyn/issues/74667)

[cston]

It looks like the collection expression is rewritten as new ReadOnlySpan<int>(maybeNull).ToArray() in LocalRewriter.CreateAndPopulateArray(), see sharplab.io.

cc @RikkiGibson

[CyrusNajmabadi]

@cston that's true here. However, when this doesn't go through the ROS path (see https://github.com/dotnet/roslyn/issues/74667) we also don't give a warning, even though it def throws at runtime :)

[RikkiGibson]

is there some straightforward way to deref the array and make the runtime throw an NRE when we are doing this? Ideally something minimal-overhead? Since it seems preferable to not directly throw NullReferenceException(), that may negatively impact inlining.

Maybe we could just access .Length and not use it?

[jaredpar]

However, when this doesn't go through the ROS path (see https://github.com/dotnet/roslyn/issues/74667) we also don't give a warning

Nullable warnings aren't fully implemented for collection expressions. That is covered by #68786. It's unfortunate cause it confuses this scenario a bit but it's the state of things.

In both https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/proposals/csharp-12.0/collection-expressions#known-length-translation and https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/proposals/csharp-12.0/collection-expressions#unknown-length-translation,

Part of the intent of collection expressions though was specifically that the compiler is free to codegen how it pleases to a large degree. Basically, we are not firmly tied to the language in the spec so that we can optimize as needed.

I agree this is a bit of a dicey case, essentially an optimizing that changes a NullReferenceException to a defined behavior. Do we want to define that as an illegal optimization?

[jnm2]

I do fully expect codegen to be optimized, but this might be a semantic that we'd want to preserve. I'm not totally opposed to optimizations being observable, e.g. for badly behaved collections that make assumptions about when GetEnumerator etc are called, so I'm not super opinionated here. Just wanted to surface it.

[CyrusNajmabadi]

My primary concern with the null-ref not happening is simply that code could easily take a dependency on that. Which would then be something we'd likely need to maintain if and when we change codegen here (which seems likely to happen given the importance of collection exprs and the varying emit strategies). That said, i don't feel strongly, and can live with this not throwing.

[CyrusNajmabadi]

I do think we should warn here though, as the high level view is htat this would throw, even if the final lowering emit makes that not happen.

[jaredpar]

Do we have a test that verifies we don't throw in this case? In the short term think we should either

1. Change this to throw
2. Accept the behavior and codify it with a test

[cston]

I do think we should warn here though

Nullable analysis for this case is added in https://github.com/dotnet/roslyn/pull/74686.