Improved collection deserialization in System.Text.Json

[YohDeadfall]

Fixes #38435. I will write some tests to cover the second commit (read only and fixed size collections), but otherwise it's ready to review.

/cc @ahsonkhan @steveharter @layomia @buyaa-n

[ahsonkhan]

cc @JeremyKuhne

[YohDeadfall]

I found what tends the test to fail:

corefx/src/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.HandleArray.cs

Lines 190 to 198 in 219b671

	IList list = (IList)state.Current.JsonPropertyInfo.GetValueAsObject(state.Current.ReturnValue);
	if (list == null)
	{
	state.Current.JsonPropertyInfo.SetValueAsObject(state.Current.ReturnValue, value);
	}
	else
	{
	list.Add(value);
	}

The code should be changed to fall into the first block and not the second one, but I can't understand what it is for. In which case the Add method should be called on the list?

[steveharter]

The code should be changed to fall into the first block and not the second one, but I can't understand what it is for. In which case the Add method should be called on the list?

IIRC, the first branch is when the property should be set explicitly and Add() not called. This is for arrays (the final set, not the temporary list to build up the values) and perhaps other collection types that aren't an IList.

[YohDeadfall]

I see. It's just strange how it's implemented and behaves now. The first solution is to set a null value, but I don't like it. So will research how to solve it another way. On the weekend I will have enouhg time to finish it.

[YohDeadfall]

@steveharter I haven't found a better way than this to fix the bug. Take a look, please.

[steveharter]

@steveharter I haven't found a better way than this to fix the bug. Take a look, please.

@YohDeadfall this is more involved than I thought. I'll take over the issue and close the PR. Thanks for your research into this.

The best approach is to do this early during warm-up so we don't have to call the property getter in order to inspect every collection instance that doesn't have a setter.

[YohDeadfall]

@steveharter Did you mean that you want to store the current collection in ReturnValue and not to access the setter each time? I thought about it too, but haven't touched yet. This will improve performance and make the code more observable and clear.