[NoMerge] React to StringValues

[benaadams]

dotnet/extensions#323 made a subtle change that causes string[] myArray = new StringValues(new string[0]) to implicitly become null.

/cc @Tratcher @rynowak

[Tratcher]

@dougbu See http://aspnetci/viewLog.html?buildId=425455&tab=buildResultsDiv&buildTypeId=Lite_MvcTest

[rynowak]

Is this an approved breaking change?

[dougbu]

It was my understanding that the breaking change in StringValues was going to be reversed. Is that not the case @Tratcher?

[benaadams]

Currently StringValues is ambiguous and has multiple ways to represent the low counts:

Count == 0

• null
• string[0]

Count == 1

• string
• string[1]

Count == 2

• string[2]

This means the check .Count == 0 becomes

if ((_value != null ? 1 : (_values?.Length ?? 0)) == 0)

And the cast of a .Count == 0 to an string[] can become both string[0] and null

After dotnet/extensions#323 there is a single (internal) representation for each count

Count == 0

• null

Count == 1

• string

Count == 2

• string[2]

There is a fast path for .Count == 0 of .IsNull becoming

if (_value == null && _values == null)

Obviously the inverse for .Count > 0 being !value.IsNull

And the cast of a .Count == 0 to an string[] can only return null

Though, its easy enough to have it return Array.Empty<string>() instead...

[dougbu]

We were talking about a StringValues breaking change. Now this PR seems to create a ValueProviderResult breaking change (callers can no longer iterate through the values of the empty result). I don't see how either breaking change is acceptable.

[benaadams]

callers can no longer iterate through the values of the empty result

That should still be valid

[benaadams]

callers can no longer iterate through the values of the empty result

Might need these two changes also
aspnet/HttpAbstractions#1010
dotnet/extensions#330

[benaadams]

There were two .Count == 0 before and things broke because not both of them were handled; now there is just one and everything is the same so only one of them needs to be handled.

e.g. the follow up change in StringValues dotnet/extensions#330 now tests Assert.Empty((string[])stringValues) as per MVC rather than the other path of Assert.Null((string[])stringValues) which it was doing before

[benaadams]

Added test fix to react to other PRs 4483843

Added temp fix while waiting for other PRs 91ab796

[Tratcher]

Heads up, the original change was reverted. We'll reintroduce it more cautiously by doing full universe builds before merging.

[pranavkm]

Can this be closed?

[dougbu]

Yes