Collection expressions fail at runtime with ImmutableArray

[davidwengier]

Found this working in the Razor repo, but it repros simply in sharplab too:
https://sharplab.io/#v2:EYLgtghglgdgNAFxAJwK7wCYgNQB8ACATAIwCwAUPgAwAE+xAdAMID2ANmwKYDGCULMAM4MAkmDCoEEYFwDcFCjE4B3GkwAUASgYBZdQG1icGoWMBmALqb55BZTN1CamgG8KND3Qf4ALDT1iElIynACCyMgQAJ4APLAIAHw08YKaru6emfQAnOopDAAynDAA5ggAFtYZHgC+FDVAA===

Not only is the lowering bad, and producing an immutable array that would be empty, its actually worse than that and it ends up throwing at runtime. Maybe collection expressions don't support ImmutableArray yet? Though if not, I would have really loved a warning or error, because I already converted heaps of test code :(

[CyrusNajmabadi]

@RikkiGibson Perhaps a warning would be good if we see you're using ImmutableArray<T>, but ImmutableArray<T> doesn't have the collection builder attribute?

[davidwengier]

The IDE even suggests changing my code to be a form that won't work at runtime ☹️

[CyrusNajmabadi]

The ide part is odd. I'll check that tomorrow!

[RikkiGibson]

its actually worse than that and it ends up throwing at runtime.

FWIW, I think that throwing when we create the collection is a better behavior than letting an unexpected empty array propagate however far until something else in the system behaves in an unexpected way :)

@RikkiGibson Perhaps a warning would be good if we see you're using ImmutableArray<T>, but ImmutableArray<T> doesn't have the collection builder attribute?

I'm def open to adding some diagnostic here for downlevel compilation.

The problem is that ImmutableArray matches the collection-initializers pattern while not being well-behaved for that pattern. My question is: should we just special case ImmutableArray here, or can we identify something characteristic about ImmutableArray's adherence to the pattern which is the "real" problem.

My gut feeling is that we just have to special case ImmutableArray here.

[cston]

See also #69136.

[davidwengier]

FWIW, I think that throwing when we create the collection is a better behavior than letting an unexpected empty array propagate however far until something else in the system behaves in an unexpected way :)

Yeah, I guess thats fair. In my case I was in tests, so the exception was more surprising, but an empty collection would have produced a nice failure message, so I guess thats why I was leaning on that side.

[CyrusNajmabadi]

My question is: should we just special case ImmutableArray here

Yes. We shoudl do that. In the same way that we just know about that type for the purposes of calling ImmutableCollectionMarshal.AsImmutableArray, we should just know that the base ImmutableArray is just broken and will be pain for the user :)

We don't really need a generalized solution here as this pain point is the 99% case. We don't get reports of people trying other immutable-struct-collection-wrappers where they also run into this :)

[CyrusNajmabadi]

I can't repro the IDE side, and we ahve tests validating this scenario. Is it possible you're compling against a system.immutable package that has this attribute in some flavors, but not in all? note: our fixes only have compiler info, so they can't check things like multiple tfms. meaning they may offer something that doesn't work in a different tfm.

[RikkiGibson]

Is there any appetite for adding a warning for use of constructs like the following:

var arr = new ImmutableArray<int> { 1, 2, 3 };

I think we would again special case ImmutableArray, with the expectation that there will never be any changes to "collection initializer" codegen for this type. i.e. this will never be good code to write, the user needs to use something else such as collection expression or ImmutableArray.Create.

[CyrusNajmabadi]

Yes. That seems sensible (if cheap to do)