IDE0028 changes runtime semantics of read-only properties/members

[TsengSR]

Version Used:
From #error version: : '5.6.0-2.26180.1 (1068905)'. Language version: latest (14.0).

Steps to Reproduce:

1. public class Example
   {
       private readonly IEnumerable<Foo> bar = new List<Fo>();
   
       public IEnumerable<Foo> Foo { get; } = new List<Fo>();
   }

2. Suggestion will appear to change it to [] on both, the field and the property.
3. When using specific libraries which inspect the concrete type, they fail, because the collection is read-only.

AutoMapper is one such example, but others too like EF Core where a collection can be declared as IEnumerable<T> , but initialized with a mutable collection for populating.

This allows both libraries to populate the underlying list during mapping (AutoMapper) or materialization (EF Core).

In EF Core a popular pattern is

public class Foo
{
    public IEnumerable<Bar> Bar { get; } = new List<Bar>();
}

to ensure read-only semantics but still allow EF Core to populate it

Ef Core: Collection types

Diagnostic Id:

IDE0028 : Collection initialization can be simplified

Expected Behavior:

Not receive a suggestion. If the collection is NOT assigable (i.e. being read-only property with only a getter and no setter or a readonly member), do not show the suggestion.

Even going further I'd suggest adding a new Diagnostic for obsolete/assigned but never changing field when [] or immutable empty collection is assigend to a readonly field/property.

Actual Behavior:

Suggestion is shown and when applied, breaks existing behavior depending on the collection being mutable.

[MihuBot]

I'm a bot. Here is a possible related and/or duplicate issue (I may be wrong):

• IDE0301 wrongly alters the collection type of ICollection<T> property. #76092

[TsengSR]

The linked issue is unrelated btw. Its about changing the type when its mutable collection. This changes the semantics of mutable collection to immutable, breaking other applications functionality.

[CyrusNajmabadi]

This is intentional behavior. The fixer even says semantics may change:

[CyrusNajmabadi]

Note: you want the "when_types_exactly_match" value to ensure this doesn't happen. The default is "when_types_loosely_match", as the normal case for most code is that the static type definition is what matters, not the runtime type.

[TsengSR]

It still suggests and appears in the messages tab of error list

with dotnet_style_collection_initializer = when_types_exactly_match.

Anyways, that wouldn't work for all other cases of IEnumerable<T> etc. since it a concrete type would never match an interface which isn't really what I like. Having the [] on interface defined properties is fine, if they are assignable, we'd then have a non-allocatable array and can reassign it when we have concrete values.

However, readonly properties and fields lack this abillities. In above case, AutoMapper has no issue when it has a private setter, i.e. public IEnumerable<Foo> Foo { get; private set; } = []; as it will just reassign it rather than interacting with the underlying type. It only falls back to the underlying object type when it cant assign the property.

And to be fair, when you are fixing 200 messages and analyzer warnings after a project migration, no one reads the Fix tips anymore, only the one showing in Visual Studios Error List ;)

Also logically it makes no sense to even allow a "Fix" for the above OP example of read only property, because the collection will always be empty AND immutable.

If one were to implement an interface with IEnumerable<T> property then implementing it with Array.Empty<T>() would make more sense as its explicit instead of [] which is less explicit. , but having it on a class makes little sense, especially if its not virtual. But overriding a getter is different semantic than initializing it with a value.

The proposal for fixing it is not to remove the Hint/Warning on all loose type, but only to remove it on read-only members and properties, which statistically can never change/be reassigned.

[CyrusNajmabadi]

Also logically it makes no sense to even allow a "Fix" for the above OP example of read only property, because the collection will always be empty AND immutable.

That makes perfect sense in many cases. For example, a value that may change in the future, but for now is just empty.

but only to remove it on read-only members and properties, which statistically can never change/be reassigned.

This doesn't change the equation.

[CyrusNajmabadi]

And to be fair, when you are fixing 200 messages and analyzer warnings after a project migration, no one reads the Fix tips anymore, only the one showing in Visual Studios Error List ;)

Seems problematic.

That said, if when_types_exactly_match isn't working here, that needs to be looked into.

[TsengSR]

Also logically it makes no sense to even allow a "Fix" for the above OP example of read only property, because the collection will always be empty AND immutable.

That makes perfect sense in many cases. For example, a value that may change in the future, but for now is just empty.

In which case can assigned type of a read-only property/field be later changed when its initialized immutable?

The moment one would add a setter (private, internal, init) or the readonly keyword removed, the diagnosic would kick in normaly

[CyrusNajmabadi]

No. I mean: changed in a future code change. Not at runtime.

It's totally normal and acceptable for. Type to expose a read-only value assigned something empty and unchanging.

[bzd3y]

@TsengSR I see where you are coming from, but at the same time, I'm not sure your examples are the most compelling.

In the readonly examples, if you are expecting a third-party API to manage the object then you cannot, or at least should not, use readonly. The readonly keyword is for you to tell the compiler not to let you assign new values to a member. In a way, you're promising it that you won't. Handing the object off to a third-party breaks that idea.

In the case of the non-readonly examples, then that is arguably an issue with those (and similar) libraries. For example, it might be worth creating an issue with EFCore or AutoMapper about replacing a read-only collection if it needs to modify it (I'm pretty sure I remember AutoMapper being configurable there, but I haven't used it in years. EFCore might be as well).

As for EFCore specifically, is that really a popular pattern? I've been using it for a long time and I have never done that. In fact, I would never do that. From what I recall, in past versions, maybe only before EFCore, you couldn't and it had to be an ICollection<T>.

I would argue that it is bad practice to use IEnumerable<T> on your entities (it might arguably be bad practice to use it for public members in general anyway...). The entities are supposed to model your database and an IEnumerable<T> does not really represent anything in a database. A database, or its tables, is mutable, by definition and default, unless it isn't, as determined by permissions, which AFAIK EFCore does not concern itself with as far as its data model goes.

As evidence for it being bad practice, there is actually a rule related to this. See CA2104. It is deprecated, but I think that is only due to the complexity of implementing or enforcing it.

There's also CA1859 which is arguably related.

I might be worth noting that even a database object like a view, that might conceptually seem like it would be read-only, is not necessarily read-only.

If you want to use IEnumerable<T> in your data model (and, again, IMO you probably shouldn't), then you'd use it on your "DTO"s. And in that case, I think EFCore will project to the IEnumerable<T> just fine. Though, with that being said, you'll do that with ToList() or one of the other similar methods, which probably further illustrates my point.