Skip to content

Commit 17a76a0

Browse files
eiriktsarpaliseiriktsarpalis
committed
simplify changes required to converter infrastructure
1 parent 987e340 commit 17a76a0

File tree

7 files changed

+114
-54
lines changed

7 files changed

+114
-54
lines changed

‎src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/FSharp/FSharpOptionConverter.cs‎

Lines changed: 57 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,40 +1,52 @@
11
// Licensed to the .NET Foundation under one or more agreements.
22
// The .NET Foundation licenses this file to you under the MIT license.
33

4-
using System.Diagnostics;
54
using System.Diagnostics.CodeAnalysis;
65
using System.Text.Json.Serialization.Metadata;
76

87
namespace System.Text.Json.Serialization.Converters
98
{
109
// Converter for F# optional values: https://fsharp.github.io/fsharp-core-docs/reference/fsharp-core-option-1.html
1110
// Serializes `Some(value)` using the format of `value` and `None` values as `null`.
12-
internal sealed class FSharpOptionConverter<TOption, TElement> : JsonResumableConverter<TOption>
11+
internal sealed class FSharpOptionConverter<TOption, TElement> : JsonConverter<TOption>
1312
where TOption : class
1413
{
15-
// While technically not implementing IEnumerable, F# optionals are effectively generic collections of at most one element.
16-
internal override ConverterStrategy ConverterStrategy => ConverterStrategy.Enumerable;
14+
// Reflect the converter strategy of the element type, since we use the identical contract for Some(_) values.
15+
internal override ConverterStrategy ConverterStrategy => _converterStrategy;
1716
internal override Type? ElementType => typeof(TElement);
17+
// 'None' is encoded using 'null' at runtime and serialized as 'null' in JSON.
18+
public override bool HandleNull => true;
1819

20+
private readonly JsonConverter<TElement> _elementConverter;
1921
private readonly Func<TOption, TElement> _optionValueGetter;
2022
private readonly Func<TElement?, TOption> _optionConstructor;
23+
private readonly ConverterStrategy _converterStrategy;
2124

2225
[RequiresUnreferencedCode(FSharpCoreReflectionProxy.FSharpCoreUnreferencedCodeMessage)]
2326
public FSharpOptionConverter(JsonConverter<TElement> elementConverter)
2427
{
28+
_elementConverter = elementConverter;
2529
_optionValueGetter = FSharpCoreReflectionProxy.Instance.CreateFSharpOptionValueGetter<TOption, TElement>();
2630
_optionConstructor = FSharpCoreReflectionProxy.Instance.CreateFSharpOptionSomeConstructor<TOption, TElement>();
27-
// If the element converter is value, this converter will also be writing values
28-
// Set a flag to signal this fact to the converter infrastructure.
29-
CanWriteJsonValues = elementConverter.ConverterStrategy == ConverterStrategy.Value;
31+
32+
// temporary workaround for JsonConverter base constructor needing to access
33+
// ConverterStrategy when calculating `CanUseDirectReadOrWrite`.
34+
// TODO move `CanUseDirectReadOrWrite` from JsonConverter to JsonTypeInfo.
35+
_converterStrategy = _elementConverter.ConverterStrategy;
36+
CanUseDirectReadOrWrite = _converterStrategy == ConverterStrategy.Value;
3037
}
3138

3239
internal override bool OnTryRead(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options, ref ReadStack state, out TOption? value)
3340
{
34-
state.Current.JsonPropertyInfo = state.Current.JsonTypeInfo.ElementTypeInfo!.PropertyInfoForTypeInfo;
35-
var elementConverter = (JsonConverter<TElement>)state.Current.JsonPropertyInfo.ConverterBase;
41+
// `null` values deserialize as `None`
42+
if (!state.IsContinuation && reader.TokenType == JsonTokenType.Null)
43+
{
44+
value = null;
45+
return true;
46+
}
3647

37-
if (elementConverter.TryRead(ref reader, typeof(TElement), options, ref state, out TElement? element))
48+
state.Current.JsonPropertyInfo = state.Current.JsonTypeInfo.ElementTypeInfo!.PropertyInfoForTypeInfo;
49+
if (_elementConverter.TryRead(ref reader, typeof(TElement), options, ref state, out TElement? element))
3850
{
3951
value = _optionConstructor(element);
4052
return true;
@@ -46,12 +58,43 @@ internal override bool OnTryRead(ref Utf8JsonReader reader, Type typeToConvert,
4658

4759
internal override bool OnTryWrite(Utf8JsonWriter writer, TOption value, JsonSerializerOptions options, ref WriteStack state)
4860
{
49-
Debug.Assert(value is not null); // 'None' values are encoded as null: handled by the base converter.
50-
state.Current.DeclaredJsonPropertyInfo = state.Current.JsonTypeInfo.ElementTypeInfo!.PropertyInfoForTypeInfo;
51-
var elementConverter = (JsonConverter<TElement>)state.Current.DeclaredJsonPropertyInfo.ConverterBase;
61+
if (value is null)
62+
{
63+
// Write `None` values as null
64+
writer.WriteNullValue();
65+
return true;
66+
}
5267

5368
TElement element = _optionValueGetter(value);
54-
return elementConverter.TryWrite(writer, element, options, ref state);
69+
state.Current.DeclaredJsonPropertyInfo = state.Current.JsonTypeInfo.ElementTypeInfo!.PropertyInfoForTypeInfo;
70+
return _elementConverter.TryWrite(writer, element, options, ref state);
71+
}
72+
73+
// Since this is a hybrid converter (ConverterStrategy depends on the element converter),
74+
// we need to override the value converter Write and Read methods too.
75+
76+
public override void Write(Utf8JsonWriter writer, TOption value, JsonSerializerOptions options)
77+
{
78+
if (value is null)
79+
{
80+
writer.WriteNullValue();
81+
}
82+
else
83+
{
84+
TElement element = _optionValueGetter(value);
85+
_elementConverter.Write(writer, element, options);
86+
}
87+
}
88+
89+
public override TOption? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
90+
{
91+
if (reader.TokenType == JsonTokenType.Null)
92+
{
93+
return null;
94+
}
95+
96+
TElement? element = _elementConverter.Read(ref reader, typeToConvert, options);
97+
return _optionConstructor(element);
5598
}
5699
}
57100
}

‎src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/FSharp/FSharpValueOptionConverter.cs‎

Lines changed: 46 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,48 +1,52 @@
11
// Licensed to the .NET Foundation under one or more agreements.
22
// The .NET Foundation licenses this file to you under the MIT license.
33

4-
using System.Diagnostics;
54
using System.Diagnostics.CodeAnalysis;
65
using System.Text.Json.Serialization.Metadata;
76

87
namespace System.Text.Json.Serialization.Converters
98
{
109
// Converter for F# struct optional values: https://fsharp.github.io/fsharp-core-docs/reference/fsharp-core-fsharpvalueoption-1.html
11-
internal sealed class FSharpValueOptionConverter<TValueOption, TElement> : JsonResumableConverter<TValueOption>
10+
// Serializes `ValueSome(value)` using the format of `value` and `ValueNone` values as `null`.
11+
internal sealed class FSharpValueOptionConverter<TValueOption, TElement> : JsonConverter<TValueOption>
1212
where TValueOption : struct, IEquatable<TValueOption>
1313
{
14-
// While technically not implementing IEnumerable, F# optionals are effectively generic collections of at most one element.
15-
internal override ConverterStrategy ConverterStrategy => ConverterStrategy.Enumerable;
14+
// Reflect the converter strategy of the element type, since we use the identical contract for ValueSome(_) values.
15+
internal override ConverterStrategy ConverterStrategy => _converterStrategy;
1616
internal override Type? ElementType => typeof(TElement);
17-
17+
// 'ValueNone' is encoded using 'default' at runtime and serialized as 'null' in JSON.
1818
public override bool HandleNull => true;
1919

20+
private readonly JsonConverter<TElement> _elementConverter;
2021
private readonly FSharpCoreReflectionProxy.StructGetter<TValueOption, TElement> _optionValueGetter;
2122
private readonly Func<TElement?, TValueOption> _optionConstructor;
23+
private readonly ConverterStrategy _converterStrategy;
2224

2325
[RequiresUnreferencedCode(FSharpCoreReflectionProxy.FSharpCoreUnreferencedCodeMessage)]
2426
public FSharpValueOptionConverter(JsonConverter<TElement> elementConverter)
2527
{
28+
_elementConverter = elementConverter;
2629
_optionValueGetter = FSharpCoreReflectionProxy.Instance.CreateFSharpValueOptionValueGetter<TValueOption, TElement>();
2730
_optionConstructor = FSharpCoreReflectionProxy.Instance.CreateFSharpValueOptionSomeConstructor<TValueOption, TElement>();
28-
// If the element converter is value, this converter will also be writing values
29-
// Set a flag to signal this fact to the converter infrastructure.
30-
CanWriteJsonValues = elementConverter.ConverterStrategy == ConverterStrategy.Value;
31+
32+
// temporary workaround for JsonConverter base constructor needing to access
33+
// ConverterStrategy when calculating `CanUseDirectReadOrWrite`.
34+
// TODO move `CanUseDirectReadOrWrite` from JsonConverter to JsonTypeInfo.
35+
_converterStrategy = _elementConverter.ConverterStrategy;
36+
CanUseDirectReadOrWrite = _converterStrategy == ConverterStrategy.Value;
3137
}
3238

3339
internal override bool OnTryRead(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options, ref ReadStack state, out TValueOption value)
3440
{
35-
state.Current.JsonPropertyInfo = state.Current.JsonTypeInfo.ElementTypeInfo!.PropertyInfoForTypeInfo;
36-
var elementConverter = (JsonConverter<TElement>)state.Current.JsonPropertyInfo.ConverterBase;
37-
3841
// `null` values deserialize as `ValueNone`
3942
if (!state.IsContinuation && reader.TokenType == JsonTokenType.Null)
4043
{
4144
value = default;
4245
return true;
4346
}
4447

45-
if (elementConverter.TryRead(ref reader, typeof(TElement), options, ref state, out TElement? element))
48+
state.Current.JsonPropertyInfo = state.Current.JsonTypeInfo.ElementTypeInfo!.PropertyInfoForTypeInfo;
49+
if (_elementConverter.TryRead(ref reader, typeof(TElement), options, ref state, out TElement? element))
4650
{
4751
value = _optionConstructor(element);
4852
return true;
@@ -61,11 +65,38 @@ internal override bool OnTryWrite(Utf8JsonWriter writer, TValueOption value, Jso
6165
return true;
6266
}
6367

68+
TElement element = _optionValueGetter(ref value);
69+
6470
state.Current.DeclaredJsonPropertyInfo = state.Current.JsonTypeInfo.ElementTypeInfo!.PropertyInfoForTypeInfo;
65-
var elementConverter = (JsonConverter<TElement>)state.Current.DeclaredJsonPropertyInfo.ConverterBase;
71+
return _elementConverter.TryWrite(writer, element, options, ref state);
72+
}
6673

67-
TElement element = _optionValueGetter(ref value);
68-
return elementConverter.TryWrite(writer, element, options, ref state);
74+
// Since this is a hybrid converter (ConverterStrategy depends on the element converter),
75+
// we need to override the value converter Write and Read methods too.
76+
77+
public override void Write(Utf8JsonWriter writer, TValueOption value, JsonSerializerOptions options)
78+
{
79+
if (value.Equals(default))
80+
{
81+
// Write `ValueNone` values as null
82+
writer.WriteNullValue();
83+
}
84+
else
85+
{
86+
TElement element = _optionValueGetter(ref value);
87+
_elementConverter.Write(writer, element, options);
88+
}
89+
}
90+
91+
public override TValueOption Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
92+
{
93+
if (reader.TokenType == JsonTokenType.Null)
94+
{
95+
return default;
96+
}
97+
98+
TElement? element = _elementConverter.Read(ref reader, typeToConvert, options);
99+
return _optionConstructor(element);
69100
}
70101
}
71102
}

‎src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverter.cs‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -34,11 +34,6 @@ internal JsonConverter() { }
3434

3535
internal bool CanBePolymorphic { get; set; }
3636

37-
/// <summary>
38-
/// When set, indicates a non-value converter that is capable of writing or reading simple JSON values.
39-
/// </summary>
40-
internal bool CanWriteJsonValues { get; set; }
41-
4237
/// <summary>
4338
/// Used to support JsonObject as an extension property in a loosely-typed, trimmable manner.
4439
/// </summary>

‎src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs‎

Lines changed: 8 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -249,8 +249,6 @@ internal bool TryRead(ref Utf8JsonReader reader, Type typeToConvert, JsonSeriali
249249
else
250250
#endif
251251
{
252-
long originalBytesConsumed = 0;
253-
254252
if (!wasContinuation)
255253
{
256254
// For perf and converter simplicity, handle null here instead of forwarding to the converter.
@@ -274,10 +272,6 @@ internal bool TryRead(ref Utf8JsonReader reader, Type typeToConvert, JsonSeriali
274272

275273
Debug.Assert(state.Current.OriginalDepth == 0);
276274
state.Current.OriginalDepth = reader.CurrentDepth;
277-
278-
// Do not store in the ReadStack since it will only be used if the converter
279-
// behaves like a value converter (hence no continuations are expected to occur).
280-
originalBytesConsumed = reader.BytesConsumed;
281275
}
282276

283277
success = OnTryRead(ref reader, typeToConvert, options, ref state, out value);
@@ -292,8 +286,8 @@ internal bool TryRead(ref Utf8JsonReader reader, Type typeToConvert, JsonSeriali
292286
VerifyRead(
293287
state.Current.OriginalTokenType,
294288
state.Current.OriginalDepth,
295-
bytesConsumed: originalBytesConsumed,
296-
isValueConverter: CanWriteJsonValues,
289+
bytesConsumed: 0,
290+
isValueConverter: false,
297291
ref reader);
298292

299293
// No need to clear state.Current.* since a stack pop will occur.
@@ -552,10 +546,13 @@ internal void VerifyRead(JsonTokenType tokenType, int depth, long bytesConsumed,
552546

553547
default:
554548
// A non-value converter (object or collection) should always have Start and End tokens
555-
// unless it supports handling null value reads.
556-
if (!isValueConverter && !(HandleNullOnRead && tokenType == JsonTokenType.Null))
549+
if (!isValueConverter)
557550
{
558-
ThrowHelper.ThrowJsonException_SerializationConverterRead(this);
551+
// with the exception of converters that support null value reads
552+
if (!HandleNullOnRead || tokenType != JsonTokenType.Null)
553+
{
554+
ThrowHelper.ThrowJsonException_SerializationConverterRead(this);
555+
}
559556
}
560557
// A value converter should not make any reads.
561558
else if (reader.BytesConsumed != bytesConsumed)

‎src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonResumableConverterOfT.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,6 @@ public sealed override void Write(Utf8JsonWriter writer, T value, JsonSerializer
4545
}
4646
}
4747

48-
public override bool HandleNull => false;
48+
public sealed override bool HandleNull => false;
4949
}
5050
}

‎src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfoOfT.cs‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -296,9 +296,6 @@ value is not null &&
296296

297297
if (Converter.HandleNullOnWrite)
298298
{
299-
// No object, collection, or re-entrancy converter handles null.
300-
Debug.Assert(Converter.ConverterStrategy == ConverterStrategy.Value);
301-
302299
if (state.Current.PropertyState < StackFramePropertyState.Name)
303300
{
304301
state.Current.PropertyState = StackFramePropertyState.Name;

‎src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.cs‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -49,9 +49,6 @@ internal JsonTypeInfo? ElementTypeInfo
4949
{
5050
if (_elementTypeInfo == null && ElementType != null)
5151
{
52-
Debug.Assert(PropertyInfoForTypeInfo.ConverterStrategy == ConverterStrategy.Enumerable ||
53-
PropertyInfoForTypeInfo.ConverterStrategy == ConverterStrategy.Dictionary);
54-
5552
_elementTypeInfo = Options.GetOrAddClass(ElementType);
5653
}
5754

@@ -181,6 +178,8 @@ internal JsonTypeInfo(Type type, JsonConverter converter, Type runtimeType, Json
181178

182179
PropertyInfoForTypeInfo = CreatePropertyInfoForTypeInfo(Type, runtimeType, converter, typeNumberHandling, Options);
183180

181+
ElementType = converter.ElementType;
182+
184183
switch (PropertyInfoForTypeInfo.ConverterStrategy)
185184
{
186185
case ConverterStrategy.Object:
@@ -303,14 +302,12 @@ internal JsonTypeInfo(Type type, JsonConverter converter, Type runtimeType, Json
303302
break;
304303
case ConverterStrategy.Enumerable:
305304
{
306-
ElementType = converter.ElementType;
307305
CreateObject = Options.MemberAccessorStrategy.CreateConstructor(runtimeType);
308306
}
309307
break;
310308
case ConverterStrategy.Dictionary:
311309
{
312310
KeyType = converter.KeyType;
313-
ElementType = converter.ElementType;
314311
CreateObject = Options.MemberAccessorStrategy.CreateConstructor(runtimeType);
315312
}
316313
break;

0 commit comments

Comments
 (0)