Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Split TryRead implementation into TryPopulate and TryCreateObject #79659

Closed
wants to merge 2 commits into from
Closed

Split TryRead implementation into TryPopulate and TryCreateObject #79659

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c8981ac
Split TryRead implementation into TryPopulate and TryCreateObject
krwq Dec 14, 2022
9635dca
CreateObject for collections/dictionaries
krwq Jan 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 5 additions & 10 deletions src/libraries/System.Text.Json/src/System.Text.Json.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,8 @@ The System.Text.Json library is built-in as part of the shared framework in .NET
<Compile Include="System\Text\Json\Serialization\Attributes\JsonRequiredAttribute.cs" />
<Compile Include="System\Text\Json\Serialization\Attributes\JsonPropertyOrderAttribute.cs" />
<Compile Include="System\Text\Json\Serialization\Attributes\JsonUnmappedMemberHandlingAttribute.cs" />
<Compile Include="System\Text\Json\Serialization\Converters\Collection\IEnumerableConverterBase.cs" />
<Compile Include="System\Text\Json\Serialization\Converters\JsonAdvancedConverter.cs" />
<Compile Include="System\Text\Json\Serialization\Converters\CastingConverter.cs" />
<Compile Include="System\Text\Json\Serialization\Converters\Collection\ImmutableDictionaryOfTKeyTValueConverterWithReflection.cs" />
<Compile Include="System\Text\Json\Serialization\Converters\Collection\ImmutableEnumerableOfTConverterWithReflection.cs" />
Expand Down Expand Up @@ -388,15 +390,8 @@ The System.Text.Json library is built-in as part of the shared framework in .NET
</ItemGroup>

<ItemGroup>
<ProjectReference Include="..\gen\System.Text.Json.SourceGeneration.Roslyn3.11.csproj"
ReferenceOutputAssembly="false"
PackAsAnalyzer="true"
Condition="'$(DotNetBuildFromSource)' != 'true'" />
<ProjectReference Include="..\gen\System.Text.Json.SourceGeneration.Roslyn4.0.csproj"
ReferenceOutputAssembly="false"
PackAsAnalyzer="true" />
<ProjectReference Include="..\gen\System.Text.Json.SourceGeneration.Roslyn4.4.csproj"
ReferenceOutputAssembly="false"
PackAsAnalyzer="true" />
<ProjectReference Include="..\gen\System.Text.Json.SourceGeneration.Roslyn3.11.csproj" ReferenceOutputAssembly="false" PackAsAnalyzer="true" Condition="'$(DotNetBuildFromSource)' != 'true'" />
<ProjectReference Include="..\gen\System.Text.Json.SourceGeneration.Roslyn4.0.csproj" ReferenceOutputAssembly="false" PackAsAnalyzer="true" />
<ProjectReference Include="..\gen\System.Text.Json.SourceGeneration.Roslyn4.4.csproj" ReferenceOutputAssembly="false" PackAsAnalyzer="true" />
</ItemGroup>
</Project>
1 change: 0 additions & 1 deletion ...raries/System.Text.Json/src/System/Text/Json/Serialization/Converters/CastingConverter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ internal sealed class CastingConverter<T> : JsonConverter<T>
internal override Type? ElementType => _sourceConverter.ElementType;

public override bool HandleNull { get; }
internal override bool SupportsCreateObjectDelegate => _sourceConverter.SupportsCreateObjectDelegate;

internal CastingConverter(JsonConverter sourceConverter, bool handleNull, bool handleNullOnRead, bool handleNullOnWrite)
{
Expand Down
22 changes: 12 additions & 10 deletions ...stem.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/ArrayConverter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,34 +3,36 @@

using System.Collections;
using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
using System.Text.Json.Serialization.Metadata;

namespace System.Text.Json.Serialization.Converters
{
/// <summary>
/// Converter for <cref>System.Array</cref>.
/// </summary>
internal sealed class ArrayConverter<TCollection, TElement> : IEnumerableDefaultConverter<TElement[], TElement>
internal sealed class ArrayConverter<TCollection, TElement> : IEnumerableDefaultConverter<TElement[], TElement, List<TElement>>
eiriktsarpalis marked this conversation as resolved.
Show resolved Hide resolved
{
internal override bool CanHaveMetadata => false;

protected override void Add(in TElement value, ref ReadStack state)
private protected override void Add(ref List<TElement> collection, in TElement value, JsonTypeInfo collectionTypeInfo)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In any case, we don't stand to win much by making it strongly typed -- I don't think any of the intermediate collections we're using are structs so we aren't boxing anything redundant right now.

Copy link
Member Author

@krwq krwq Jan 30, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually ReadOnlyArray is a struct and uses intermediate type - that's why I added that in the first place

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ReadOnlyArray? Was this added in this PR?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, there are existing tests though which implement read only collections as structs and those are failing unless this is ref previously we changed state.Current.ReturnValue here and reassigning if this was value type

{
((List<TElement>)state.Current.ReturnValue!).Add(value);
collection.Add(value);
}

internal override bool SupportsCreateObjectDelegate => false;
protected override void CreateCollection(ref Utf8JsonReader reader, scoped ref ReadStack state, JsonSerializerOptions options)
internal override void ConfigureJsonTypeInfo(JsonTypeInfo jsonTypeInfo, JsonSerializerOptions options)
{
state.Current.ReturnValue = new List<TElement>();
jsonTypeInfo.CreateObject ??= () => new List<TElement>();
}

protected override void ConvertCollection(ref ReadStack state, JsonSerializerOptions options)
private protected override bool TryConvert(ref Utf8JsonReader reader, JsonTypeInfo jsonTypeInfo, scoped ref ReadStack state, List<TElement> obj, out TElement[] value)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does the bool result in TryConvert signify?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

honestly I added it because I think we can implement parametrized constructors in terms of these 3 and in that case it will need the bool result. In general it means same as in other places: "I did not finish reading, continuation will need to happen"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't convert something meant to run after serialization has completed to materialize the final instance from the intermediate one? If so, there's not async reading necessary that might necessitate producing partial results.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that is certainly possible it's not needed and I added it initially because I wasn't initially sure if we do that or not. I will reiterate on that

{
List<TElement> list = (List<TElement>)state.Current.ReturnValue!;
state.Current.ReturnValue = list.ToArray();
List<TElement> list = obj;
value = list.ToArray();
return true;
}

protected override bool OnWriteResume(Utf8JsonWriter writer, TElement[] array, JsonSerializerOptions options, ref WriteStack state)
internal override bool OnWriteResume(Utf8JsonWriter writer, TElement[] array, JsonSerializerOptions options, ref WriteStack state)
{
int index = state.Current.EnumeratorIndex;

Expand Down
7 changes: 4 additions & 3 deletions ...n/src/System/Text/Json/Serialization/Converters/Collection/ConcurrentQueueOfTConverter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,17 @@
// The .NET Foundation licenses this file to you under the MIT license.

using System.Collections.Concurrent;
using System.Text.Json.Serialization.Metadata;

namespace System.Text.Json.Serialization.Converters
{
internal sealed class ConcurrentQueueOfTConverter<TCollection, TElement>
: IEnumerableDefaultConverter<TCollection, TElement>
: IEnumerableDefaultConverter<TCollection, TElement, TCollection>
where TCollection : ConcurrentQueue<TElement>
{
protected override void Add(in TElement value, ref ReadStack state)
private protected override void Add(ref TCollection collection, in TElement value, JsonTypeInfo collectionTypeInfo)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does it need to be private protected? The type itself is internal so it shouldn't matter much.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no preference

{
((TCollection)state.Current.ReturnValue!).Enqueue(value);
collection.Enqueue(value);
}
}
}
7 changes: 4 additions & 3 deletions ...n/src/System/Text/Json/Serialization/Converters/Collection/ConcurrentStackOfTConverter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,17 @@
// The .NET Foundation licenses this file to you under the MIT license.

using System.Collections.Concurrent;
using System.Text.Json.Serialization.Metadata;

namespace System.Text.Json.Serialization.Converters
{
internal sealed class ConcurrentStackOfTConverter<TCollection, TElement>
: IEnumerableDefaultConverter<TCollection, TElement>
: IEnumerableDefaultConverter<TCollection, TElement, TCollection>
where TCollection : ConcurrentStack<TElement>
{
protected override void Add(in TElement value, ref ReadStack state)
private protected override void Add(ref TCollection collection, in TElement value, JsonTypeInfo collectionTypeInfo)
{
((TCollection)state.Current.ReturnValue!).Push(value);
collection.Push(value);
}
}
}
8 changes: 3 additions & 5 deletions ...on/src/System/Text/Json/Serialization/Converters/Collection/DictionaryDefaultConverter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,23 +2,21 @@
// The .NET Foundation licenses this file to you under the MIT license.

using System.Collections.Generic;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Text.Json.Serialization.Metadata;

namespace System.Text.Json.Serialization.Converters
{
/// <summary>
/// Default base class implementation of <cref>JsonDictionaryConverter{TCollection}</cref> .
/// </summary>
internal abstract class DictionaryDefaultConverter<TDictionary, TKey, TValue>
: JsonDictionaryConverter<TDictionary, TKey, TValue>
internal abstract class DictionaryDefaultConverter<TDictionary, TKey, TValue, IntermediateType>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my earlier feedback. I think this could regress application sizes without much tangible impact.

: JsonDictionaryConverter<TDictionary, TKey, TValue, IntermediateType>
where TDictionary : IEnumerable<KeyValuePair<TKey, TValue>>
where TKey : notnull
{
internal override bool CanHaveMetadata => true;

protected internal override bool OnWriteResume(
private protected override bool OnWriteResumeCore(
Utf8JsonWriter writer,
TDictionary value,
JsonSerializerOptions options,
Expand Down
8 changes: 4 additions & 4 deletions ...c/System/Text/Json/Serialization/Converters/Collection/DictionaryOfTKeyTValueConverter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,16 +11,16 @@ namespace System.Text.Json.Serialization.Converters
/// representing the dictionary element key and value.
/// </summary>
internal sealed class DictionaryOfTKeyTValueConverter<TCollection, TKey, TValue>
: DictionaryDefaultConverter<TCollection, TKey, TValue>
: DictionaryDefaultConverter<TCollection, TKey, TValue, TCollection>
where TCollection : Dictionary<TKey, TValue>
where TKey : notnull
{
protected override void Add(TKey key, in TValue value, JsonSerializerOptions options, ref ReadStack state)
protected override void Add(ref TCollection collection, TKey key, in TValue value, JsonSerializerOptions options)
{
((TCollection)state.Current.ReturnValue!)[key] = value;
collection[key] = value;
}

protected internal override bool OnWriteResume(
private protected override bool OnWriteResumeCore(
Utf8JsonWriter writer,
TCollection value,
JsonSerializerOptions options,
Expand Down
15 changes: 8 additions & 7 deletions .../src/System/Text/Json/Serialization/Converters/Collection/IAsyncEnumerableOfTConverter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,15 @@

using System.Collections.Generic;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Text.Json.Serialization.Metadata;
using System.Threading;
using System.Threading.Tasks;

namespace System.Text.Json.Serialization.Converters
{
internal sealed class IAsyncEnumerableOfTConverter<TAsyncEnumerable, TElement>
: JsonCollectionConverter<TAsyncEnumerable, TElement>
: JsonCollectionConverter<TAsyncEnumerable, TElement, IAsyncEnumerable<TElement>>
where TAsyncEnumerable : IAsyncEnumerable<TElement>
{
internal override bool OnTryRead(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options, scoped ref ReadStack state, out TAsyncEnumerable value)
Expand All @@ -22,15 +24,14 @@ internal override bool OnTryRead(ref Utf8JsonReader reader, Type typeToConvert,
return base.OnTryRead(ref reader, typeToConvert, options, ref state, out value!);
}

protected override void Add(in TElement value, ref ReadStack state)
private protected override void Add(ref IAsyncEnumerable<TElement> collection, in TElement value, JsonTypeInfo collectionTypeInfo)
{
((BufferedAsyncEnumerable)state.Current.ReturnValue!)._buffer.Add(value);
((BufferedAsyncEnumerable)collection)._buffer.Add(value);
}

internal override bool SupportsCreateObjectDelegate => false;
protected override void CreateCollection(ref Utf8JsonReader reader, scoped ref ReadStack state, JsonSerializerOptions options)
internal override void ConfigureJsonTypeInfo(JsonTypeInfo jsonTypeInfo, JsonSerializerOptions options)
{
state.Current.ReturnValue = new BufferedAsyncEnumerable();
jsonTypeInfo.CreateObject ??= () => new BufferedAsyncEnumerable();
}

internal override bool OnTryWrite(Utf8JsonWriter writer, TAsyncEnumerable value, JsonSerializerOptions options, ref WriteStack state)
Expand All @@ -44,7 +45,7 @@ internal override bool OnTryWrite(Utf8JsonWriter writer, TAsyncEnumerable value,
}

[Diagnostics.CodeAnalysis.SuppressMessage("Reliability", "CA2012:Use ValueTasks correctly", Justification = "Converter needs to consume ValueTask's in a non-async context")]
protected override bool OnWriteResume(Utf8JsonWriter writer, TAsyncEnumerable value, JsonSerializerOptions options, ref WriteStack state)
internal override bool OnWriteResume(Utf8JsonWriter writer, TAsyncEnumerable value, JsonSerializerOptions options, ref WriteStack state)
{
IAsyncEnumerator<TElement> enumerator;
ValueTask<bool> moveNextTask;
Expand Down
24 changes: 6 additions & 18 deletions ....Json/src/System/Text/Json/Serialization/Converters/Collection/ICollectionOfTConverter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System.Collections.Generic;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Text.Json.Serialization.Metadata;

namespace System.Text.Json.Serialization.Converters
Expand All @@ -11,28 +12,15 @@ namespace System.Text.Json.Serialization.Converters
/// Converter for <cref>System.Collections.Generic.ICollection{TElement}</cref>.
/// </summary>
internal sealed class ICollectionOfTConverter<TCollection, TElement>
: IEnumerableDefaultConverter<TCollection, TElement>
: IEnumerableDefaultConverter<TCollection, TElement, TCollection>
where TCollection : ICollection<TElement>
{
protected override void Add(in TElement value, ref ReadStack state)
{
TCollection collection = (TCollection)state.Current.ReturnValue!;
collection.Add(value);
if (IsValueType)
{
state.Current.ReturnValue = collection;
};
}
private protected sealed override bool IsReadOnly(object obj)
=> ((TCollection)obj).IsReadOnly;

protected override void CreateCollection(ref Utf8JsonReader reader, scoped ref ReadStack state, JsonSerializerOptions options)
private protected override void Add(ref TCollection collection, in TElement value, JsonTypeInfo collectionTypeInfo)
{
base.CreateCollection(ref reader, ref state, options);
TCollection returnValue = (TCollection)state.Current.ReturnValue!;
if (returnValue.IsReadOnly)
{
state.Current.ReturnValue = null; // clear out for more accurate JsonPath reporting.
ThrowHelper.ThrowNotSupportedException_CannotPopulateCollection(TypeToConvert, ref reader, ref state);
}
collection.Add(value);
}

internal override void ConfigureJsonTypeInfo(JsonTypeInfo jsonTypeInfo, JsonSerializerOptions options)
Expand Down
26 changes: 7 additions & 19 deletions ...ext.Json/src/System/Text/Json/Serialization/Converters/Collection/IDictionaryConverter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System.Collections;
using System.Collections.Generic;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Text.Json.Serialization.Metadata;

namespace System.Text.Json.Serialization.Converters
Expand All @@ -13,31 +14,18 @@ namespace System.Text.Json.Serialization.Converters
/// representing the dictionary element key and value.
/// </summary>
internal sealed class IDictionaryConverter<TDictionary>
: JsonDictionaryConverter<TDictionary, string, object?>
: JsonDictionaryConverter<TDictionary, string, object?, TDictionary>
where TDictionary : IDictionary
{
protected override void Add(string key, in object? value, JsonSerializerOptions options, ref ReadStack state)
{
TDictionary collection = (TDictionary)state.Current.ReturnValue!;
collection[key] = value;
if (IsValueType)
{
state.Current.ReturnValue = collection;
}
}
private protected override bool IsReadOnly(object obj)
=> ((TDictionary)obj).IsReadOnly;

protected override void CreateCollection(ref Utf8JsonReader reader, scoped ref ReadStack state)
protected override void Add(ref TDictionary collection, string key, in object? value, JsonSerializerOptions options)
{
base.CreateCollection(ref reader, ref state);
TDictionary returnValue = (TDictionary)state.Current.ReturnValue!;
if (returnValue.IsReadOnly)
{
state.Current.ReturnValue = null; // clear out for more accurate JsonPath reporting.
ThrowHelper.ThrowNotSupportedException_CannotPopulateCollection(TypeToConvert, ref reader, ref state);
}
collection[key] = value;
}

protected internal override bool OnWriteResume(Utf8JsonWriter writer, TDictionary value, JsonSerializerOptions options, ref WriteStack state)
private protected override bool OnWriteResumeCore(Utf8JsonWriter writer, TDictionary value, JsonSerializerOptions options, ref WriteStack state)
{
IDictionaryEnumerator enumerator;
if (state.Current.CollectionEnumerator == null)
Expand Down
24 changes: 6 additions & 18 deletions .../System/Text/Json/Serialization/Converters/Collection/IDictionaryOfTKeyTValueConverter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System.Collections.Generic;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Text.Json.Serialization.Metadata;

namespace System.Text.Json.Serialization.Converters
Expand All @@ -12,29 +13,16 @@ namespace System.Text.Json.Serialization.Converters
/// (de)serializes as a JSON object with properties representing the dictionary element key and value.
/// </summary>
internal sealed class IDictionaryOfTKeyTValueConverter<TDictionary, TKey, TValue>
: DictionaryDefaultConverter<TDictionary, TKey, TValue>
: DictionaryDefaultConverter<TDictionary, TKey, TValue, TDictionary>
where TDictionary : IDictionary<TKey, TValue>
where TKey : notnull
{
protected override void Add(TKey key, in TValue value, JsonSerializerOptions options, ref ReadStack state)
{
TDictionary collection = (TDictionary)state.Current.ReturnValue!;
collection[key] = value;
if (IsValueType)
{
state.Current.ReturnValue = collection;
};
}
private protected sealed override bool IsReadOnly(object obj)
=> ((TDictionary)obj).IsReadOnly;

protected override void CreateCollection(ref Utf8JsonReader reader, scoped ref ReadStack state)
protected override void Add(ref TDictionary collection, TKey key, in TValue value, JsonSerializerOptions options)
{
base.CreateCollection(ref reader, ref state);
TDictionary returnValue = (TDictionary)state.Current.ReturnValue!;
if (returnValue.IsReadOnly)
{
state.Current.ReturnValue = null; // clear out for more accurate JsonPath reporting.
ThrowHelper.ThrowNotSupportedException_CannotPopulateCollection(TypeToConvert, ref reader, ref state);
}
collection[key] = value;
}

internal override void ConfigureJsonTypeInfo(JsonTypeInfo jsonTypeInfo, JsonSerializerOptions options)
Expand Down