Remove defensive copies in System.Collections.Immutable

[hamarb123]

The field being marked readonly causes 26 uses of it in this file to create a defensive copy (since it's generic, it has no such things as readonly members, meaning every access requires a defensive copy). Removing the readonly should resolve these.

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/area-system-collections
See info in area-owners.md if you want to be subscribed.

[EgorBo]

What problems this creates here exactly? are you seeing some suboptimal codegen or correctness issues? it seems that in most uses we only access .Count on top of it and if it's inlined - there should be no difference?

[hamarb123]

What problems this creates here exactly? are you seeing some suboptimal codegen or correctness issues? it seems that in most uses we only access .Count on top of it.

This one is not a correctness issue - it will cause the field to be copied to a local and then have the method be called on that - the change will result in better IL, but I'm not sure if the actual codegen of this particular issue will be affected though, since, at least out of the handful I looked through, they seem to just have 1x class field implementing the logic anyway (I'm not sure if the runtime can see through that or not - I assume you'd know).

[EgorBo]

My understanding that this doesn't create any problems for e.g. small structs or when the method we call is inlined into a field load etc. I think we have quite a lot of readonly T... fields in various places in BCL which we might need to change too if this is approved

[hamarb123]

Codegen seems to only be affected for methods that are not inlined: link.

Some of the member accesses seem to call methods that I don't imagine would be inlined (e.g., some FindItemIndex implementations).

[hamarb123]

I think we have quite a lot of readonly T... fields in various places in BCL which we might need to change too if this is approved

For generic ones, I would think they should only be potentially changed if it's an internal generic constraint, since otherwise you don't know if what's implementing it is actually readonly. Also, I don't think there really are many more of ones that I'd change (unless they're all hidden in IL compiler & nativeaot System.Private.CoreLib - I haven't look through these very thoroughly, because there's a LOT in them), since I don't recall noticing any others like this on my brief looks going through each project.

[stephentoub]

Thanks, but unless you can highlight a specific problem this is already causing, I'd prefer we close this, so as to a) not churn the code unnecessarily and b) stick with things being readonly unless there's a good reason they shouldn't be.