StringValues.ToArray does not return a copy if the StringValues contains an array internally.

[marcelltoth]

Describe the bug

The StringValues class is an IEnumerable<string>, but it has its own ToArray() method that shadows the well-known extension method with the same name. This would not be a problem by itself, but this ToArray works surprisingly differently than the extension: If the StringValues contains an array internally it simply returns that array. This greatly violates the principle of least astonishment and causes hard-to-localize bugs in user code such as: dotnet/aspnetcore#11016

To Reproduce

Steps to reproduce the behavior:

1. Using the latest master version of assembly Microsoft.Extensions.Primitives (the problem existed for a long time though)
2. Run these two tests:

[Fact]
public void ToArray_ConstructedFromString_CopiesValues()
{
	var singleStringValue = new StringValues("abc");

	var array = singleStringValue.ToArray();
	array[0] = "bcd";

	Assert.Equal("abc", singleStringValue[0]);
}

[Fact]
public void ToArray_ConstructedFromArray_CopiesValues()
{
	var singleStringValue = new StringValues(new [] {"abc"});

	var array = singleStringValue.ToArray();
	array[0] = "bcd";

	Assert.Equal("abc", singleStringValue[0]);
}

3. The first one succeeds, as expected.
4. The second one fails, it allows the mutation of singleStringValue.

Expected behavior

Both tests succeed.

Rationale

1. Seeing a method called ToArray, I can't blame anyone that assumes that it creates a copy. I definitely would.
2. This struct is defined as read only at every possible place (ICollection<string>.IsReadOnly, all the NotSupportedException-s, etc.), it should be immutable as much as practical.
3. Because of the shadowing, the current situation makes this innocent change a breaking chage:

   IEnumerable<string> values = someStringValues;
   var arrayOfValues = values.ToArray();
   Array.Sort(arrayOfValues, StringComparer.Ordinal);
   

   versus:

   var values = someStringValues;
   var arrayOfValues = values.ToArray();
   Array.Sort(arrayOfValues, StringComparer.Ordinal);
   

   The first one copies, the second one mutates. Very hard to catch.

Disclaimer

The proposed fix is a change of behavior in the public-facing api of the StringValues class.
However I do not think anyone has used this behavior intentionally because:

• StringValues is an IList by itself, if someone were to attempt to mutate it they just used that interface (and realized that it does not support mutations intentionally).
• If you really really need a mutable array out of the struct, it has an implicit string[] conversion operator which does exactly that.

Further considerations

It should be worth considering whether the implicit operator string[] conversion operator and string[]-based constructor should create a copy, too.

I am assuming that these two are implemented without copies for performance reasons, and I left them alone for now in the fix I prepared for the ToArray problem. But still, these two points on the API allow simple (and perhaps unintentional) mutation of a struct that is otherwise acts perfectly immutable. It might be worth considering whether the performance gain is worth the integrity risk.

As part of the migration of components from dotnet/extensions to dotnet/runtime (aspnet/Announcements#411) we will be bulk closing some of the older issues. If you are still interested in having this issue addressed, just comment and the issue will be automatically reactivated (even if you aren't the author). When you do that, I'll page the team to come take a look. If you've moved on or workaround the issue and no longer need this change, just ignore this and the issue will be closed in 7 days.

If you know that the issue affects a package that has moved to a different repo, please consider re-opening the issue in that repo. If you're unsure, that's OK, someone from the team can help!

[marcelltoth]

I haven't checked recently but AFAIK this is still causing the bug in ASP.NET Core: dotnet/aspnetcore#11016

Paging @dotnet/extensions-migration ! This issue has been revived from staleness. Please take a look and route to the appropriate repository.