Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.Sign up
Should ToArray methods be allowed to return Array.Empty<T>()? #14848
We've been trying to use
The question is: should we relax that and instead use
Several months back, before the corefx libraries were on GitHub, I'd done a sweep through them doing hundreds of empty array replacements. In general I stayed away from changing anything that looked like it was risky, e.g. ToArray methods, places where the arrays were obviously used for reference equality, etc. I apparently was a little over aggressive with regards to my own guidelines, and ended up changing some of the non-generic collection's ToArray methods to use
Personally I'm tempted to say that ToArray methods should be able to use
I was half-way to writing this exact issue last night when a crying baby took priority, and I say yes.
I don't think reference uniqueness is something that should be inferred from any API method's result unless:
In any case of an immutable reference type, I think using a pre-existing object is not just a reasonable optimisation to take, but a reasonable optimisation to expect that other code may or may not take. (And by extension therefore, it is unreasonable to expect the same policy from version to version and build a dependency on it).
I'd note that there have been some changes about just what returns
I'm highly in favor of doing this. I've always needed to have a
In particular it makes it much cheaper to have something like an immutable tree data structure where each node can have a list of attributes or children, but where many nodes have 0 of those. Instead of scattering null checks everywhere (or some other non-terse pattern), just have a constructor that takes your usual
By baking this into a core extension method, it will propagate and encourage good habits and performance. If someone really needs unique object identity then they can do their own
It may also be worth taking this pattern further and ensuring that methods such as
I'd also like to say that this seems like a very good place to test out a policy of "if no one screams then ship it." This is a trivial change to make, and also a trivial one to undo if it really becomes necessary.
I do think that method names like
Edit: If this passes, I'm wondering if the same logic could be applied to other
if (source is T) return (T)source;
@James-Ko, you can't just return the same array back to the caller if it has 1+ items in it because one of the most important use cases for
What you're thinking of could be articulated as something like
Definitely not. The optimisation of replacing a
Non-empty arrays, lists, and most other collections are not immutable, so such a use would always be a bug. (Internal
More generally, it's worth noting that replacing
There are some cases though where the use is more of a question of optimisation for one quality over another, or for performance in one case over another.
An optimisation would be one that introduced a
I think that
We discussed this offline and concluded that we should do the following: