Add support for more concrete types & derived types during deserialization.

[CodeBlanch]

Adds support for ReadOnlyDictionary<,> concrete type and derived types. Examples that will now work fine:

        public class ConcreteDictionaryClass
        {
            public Dictionary<string, string> Dictionary1 { get; set; }
            public DerivedDictionary Dictionary2 { get; set; }
            public DerivedReadOnlyDictionary Dictionary3 { get; set; }
        }

        public class DerivedDictionary : Dictionary<string, string> { }

        public class DerivedReadOnlyDictionary : ReadOnlyDictionary<string, string>
        {
            public DerivedReadOnlyDictionary(IDictionary<string, string> dictionary) : base(dictionary) { }
        }

        public class ReadOnlyDictionaryClass
        {
            public IReadOnlyDictionary<string, string> Dictionary1 { get; set; }
            public ReadOnlyDictionary<string, string> Dictionary2 { get; set; }
            public DerivedReadOnlyDictionaryClass Dictionary3 { get; set; }
        }

        public class DerivedReadOnlyDictionaryClass : ReadOnlyDictionary<string, string>
        {
            public DerivedReadOnlyDictionaryClass(IDictionary<string, string> dictionary) : base(dictionary) { }
        }

Right now you will get this nasty NullRef Exception:

System.NullReferenceException
  HResult=0x80004003
  Message=Object reference not set to an instance of an object.
  Source=System.Text.Json
  StackTrace:
   at System.Text.Json.JsonPropertyInfoCommon`4.CreateDerivedDictionaryInstance(JsonPropertyInfo collectionPropertyInfo, IDictionary sourceDictionary, String jsonPath, JsonSerializerOptions options) in C:\Source\Repos\corefx_CodeBlanch\src\System.Text.Json\src\System\Text\Json\Serialization\JsonPropertyInfoCommon.cs:line 177
   at System.Text.Json.Serialization.Converters.DefaultDerivedDictionaryConverter.CreateFromDictionary(ReadStack& state, IDictionary sourceDictionary, JsonSerializerOptions options) in C:\Source\Repos\corefx_CodeBlanch\src\System.Text.Json\src\System\Text\Json\Serialization\Converters\DefaultDerivedDictionaryConverter.cs:line 15
   at System.Text.Json.JsonSerializer.HandleEndDictionary(JsonSerializerOptions options, Utf8JsonReader& reader, ReadStack& state) in C:\Source\Repos\corefx_CodeBlanch\src\System.Text.Json\src\System\Text\Json\Serialization\JsonSerializer.Read.HandleDictionary.cs:line 114
   at System.Text.Json.JsonSerializer.ReadCore(JsonSerializerOptions options, Utf8JsonReader& reader, ReadStack& readStack) in C:\Source\Repos\corefx_CodeBlanch\src\System.Text.Json\src\System\Text\Json\Serialization\JsonSerializer.Read.cs:line 92
   at System.Text.Json.JsonSerializer.ReadCore(Type returnType, JsonSerializerOptions options, Utf8JsonReader& reader) in C:\Source\Repos\corefx_CodeBlanch\src\System.Text.Json\src\System\Text\Json\Serialization\JsonSerializer.Read.Helpers.cs:line 22
   at System.Text.Json.JsonSerializer.ParseCore(String json, Type returnType, JsonSerializerOptions options) in C:\Source\Repos\corefx_CodeBlanch\src\System.Text.Json\src\System\Text\Json\Serialization\JsonSerializer.Read.String.cs:line 74
   at System.Text.Json.JsonSerializer.Deserialize[TValue](String json, JsonSerializerOptions options) in C:\Source\Repos\corefx_CodeBlanch\src\System.Text.Json\src\System\Text\Json\Serialization\JsonSerializer.Read.String.cs:line 31
   at System.Text.Json.Serialization.Tests.DictionaryTests.ConcreteDictionaryDeserializes() in C:\Source\Repos\corefx_CodeBlanch\src\System.Text.Json\tests\Serialization\DictionaryTests.cs:line 1407

Adds supports for Collection<>, ObservableCollection<>, & ReadOnlyCollection<> concrete types and derived types. Examples that will now work fine:

        public class ConcreteCollectionClass
        {
            public Collection<int> Collection1 { get; set; }
            public DerivedCollection Collection2 { get; set; }
            public ObservableCollection<int> Collection3 { get; set; }
        }

        public class DerivedCollection : Collection<int> { }

        public class ReadOnlyCollectionClass
        {
            public IReadOnlyCollection<int> Collection1 { get; set; }
            public ReadOnlyCollection<int> Collection2 { get; set; }
            public DerivedReadOnlyCollection Collection3 { get; set; }
            public IReadOnlyList<int> List1 { get; set; }
        }

        public class DerivedReadOnlyCollection : ReadOnlyCollection<int>
        {
            public DerivedReadOnlyCollection(IList<int> items) : base(items) { }
        }

Also enables a bunch of TODO scenarios that were sprinkled around the code. Queue, Stack, HashTable, etc. Some tests that failed with a note to get working in the future now work.

Implementation

It was simple to get this working for Dictionaries. Array code was a totally different story! Way more code paths and mixed up logic. What I did was refactor the Array code to work like the (much cleaner) Dictionary code. IMO it is a bit more maintainable now, has more features, and might even be a smidge faster.

Issues

Resolves https://github.com/dotnet/corefx/issues/40597
Resolves https://github.com/dotnet/corefx/issues/40479
Starts https://github.com/dotnet/corefx/issues/38552

[CodeBlanch]

FYI @layomia has another PR out there (#40654) also for https://github.com/dotnet/corefx/issues/40597. My approach was to add support for more of the common stuff.

[layomia]

@CodeBlanch, thanks for submitting this PR! I think it should build atop #40654, and subsequent PRs to address https://github.com/dotnet/corefx/issues/40370 and https://github.com/dotnet/corefx/issues/40657.

These changes will ensure that all collections in the System.Collections* namespaces are either supported for (de)serialization (if the implementation can fit into the existing (de)serialization logic), or throw a clear NotSupportedException message; that is, no leaked exceptions.
For example, #40654 fixes the NullReferenceException you brought up, adds (de)serialization support for System.Collections.ObjectModel collections where applicable, and ensures that we throw NotSupportedException where we don't have support.

One concern that we have with the current (de)serialization logic is that it manages a lot of state which can become unwieldy. This PR adds more state (ClassType.ICollectionConstructible, ReadStackFrame.ICollectionConstructible(Property), ReadStackFrame.IsProcessingICollectionConstructible).

The plan for post-3.0 is to refactor collection to support to leverage the converter mechanism where possible and have a cleaner implementation that supports System.Collections* collections where possible: https://github.com/dotnet/corefx/issues/38552.

[CodeBlanch]

@layomia Thanks for the link! Some great insight and context I was missing. Agree greatly there's a lot of room for improvement. The array paths were a mess. Adding the new state/flags allowed me to direct a lot of the logic into the existing converters. IMO it gets things into a much nicer state which can then be improved upon further.

Pushed some changes...

• Added support for ReadOnlyObservableCollection which was very simple now that everything is routed to the converter.
• I merged in your latest PR. Updated test cases for stuff that now passes.
• I restored the explicit list of instantiated types in CreateIEnumerableInstance & CreateIDictionaryInstance because the discussion indicated they were there for performance. In order to make the list complete I had to add references to System.Collections.NonGeneric & System.ObjectModel. Is the performance gain worth it?

One thing my initial change does is open up support for derived types that need to be passed an IList or IDictionary during construction. The derived types need only expose a constructor to accept the temp list being built up. I added an exception with an explicit message for those...

            catch (MissingMethodException)
            {
                throw ThrowHelper.ThrowNotSupportedException_DeserializeInstanceConstructorNotFound(parentType, sourceList.GetType());
            }

            catch (MissingMethodException)
            {
                throw ThrowHelper.ThrowNotSupportedException_DeserializeInstanceConstructorNotFound(parentType, sourceDictionary.GetType());
            }

What are your thoughts on that?

[CodeBlanch]

@steveharter @layomia I refactored all of the dictionary/collection deserialization code into JsonEnumerableConverters & JsonDictionaryConverters. Should be much faster now, especially on cold startup. A few methods before like GetImplementedCollectionType would be called constantly. I was able to support basically every built-in collection and dictionary type (directly, inherited, or derived) that I could think of, lots of the tests that threw NotSupported now pass. Eliminated IDictionaryConstructible completely. Overall less code paths and complexity to worry about.

Anyway if you think this is worthwhile let me know and I'll run performance benchmarks and resolve conflicts with the latest improvements.

[maryamariyan]

Thank you for your contribution. As announced in dotnet/coreclr#27549 this repository will be moving to dotnet/runtime on November 13. If you would like to continue working on this PR after this date, the easiest way to move the change to dotnet/runtime is:

1. In your corefx repository clone, create patch by running git format-patch origin
2. In your runtime repository clone, apply the patch by running git apply --directory src/corefx <path to the patch created in step 1>

[layomia]

@CodeBlanch thanks for the PR.

Most of the issues it addresses, including extended collection support (Queue, ObservableCollection), the removal of ClassType.IDictionaryConstructible, various null refs, and the repeated calls to GetImplementedCollectionType, have been fixed for 5.0, mainly in #41482. Null ref related fixes have been ported to 3.1.

The only thing left is the creation of collections using ctors. that take IDictionary[<TKey, TValue>] or IEnumerable<T>, e.g. the readonly collections you point out. I think this should be addressed separately (tracked by https://github.com/dotnet/corefx/issues/42498) in a new/clean PR in the dotnet/runtime repo when it opens up.
The implementation would have to consider any overlap with custom (parameterless) constructor support: https://github.com/dotnet/corefx/issues/40399.

[layomia]

Closing this PR in light of comments above.