Report diagnostic for collection expression with collection initializer target type where Add() returns a value

[cston]

Version Used:
c5ec55b

Steps to Reproduce:
Compile and run:

using System.Collections.Immutable;

ImmutableArray<int> a = [1, 2, 3];

Expected Behavior:
Compiler warning or error that ImmutableArray<int>.Add(int) returns a value which might indicate the collection type is immutable.

Actual Behavior:
No compiler diagnostics; NullReferenceException in ImmutableArray<int>.Add(int) at runtime.

Relates to test plan #66418 (collection expressions)

[cston]

If we report a similar diagnostic for classic collection initializer expressions as well, the diagnostic may need to be a warning wave warning since it is a breaking change.

using System.Collections.Immutable;

ImmutableArray<int> a = new() { 1, 2, 3 }; // warning?

[CyrusNajmabadi]

Note: Add returns a value for HashSet.

Perhaps only if the return value is the same type as the instance?

[alrz]

Perhaps only if the return value is the same type as the instance?

That could be a fluent-friendly API just to chain Add methods.

I think itd make sense if void was a requirement here, at least for collection literals. Anything like HashSet would need to use custom create methods or something similar.

[CyrusNajmabadi]

It's a strong principle that a type that works correctly with collection initializers should just work with collection expressions.

[CyrusNajmabadi]

 That could be a fluent-friendly API just to chain Add methods.

I'm unfamiliar with any apis that work like that. I'm not saying they don't exist, just that I don't know if any. Can you point me at any? It would be useful for seeing what sorts of patterns already exist.

Note: I'm also ok with this warning being just out of Roslyn-analyzers

[alrz]

It would be useful for seeing what sorts of patterns already exist.

I mean it's just a convenient choice to return this, it doesn't indicate whether or not the method is mutating or anything.. actually that could be the reason the return type is ignored in the fisrt place.

Definitely agree on this being an analyzer, imo faulty usage patterns could get too narrow for a compiler warning.

[jnm2]

If it helps, it could start out being limited to types implementing System.Collections.Immutable.IImmutableXyz interfaces.

[RikkiGibson]

To reiterate my thoughts in #70351, I would feel most comfortable with special-casing ImmutableArray to begin with. I want to enable this warning by default in the platform, so I would want to have 99.5% confidence that the warning we are giving is really catching buggy user code. Not just usage of a user collection type where they decided to have a fluent Add.

Also, I would want to figure out what can downlevel users do, including ourselves, to actually make ImmutableArray usable with collection-expressions. Even if we don't consider any solution there to be officially supported, it's something that we ourselves will want. For example, can we declare our own ImmutableCollectionsMarshal.AsImmutableArray internally and have things just light up.

[jnm2]

I would want to figure out what can downlevel users do, including ourselves, to actually make ImmutableArray usable with collection-expressions.

We're using collection expressions for ImmutableArray downlevel by referencing the System.Collections.Immutable 8.0.0 NuGet package.