Skip to content

Simplify JsonTypeInfo construction #67700

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

Merged
krwq merged 1 commit into from
Apr 7, 2022
Merged

Simplify JsonTypeInfo construction #67700

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...s/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ static JsonTypeInfo CreateJsonTypeInfo(Type type, JsonSerializerOptions options)
// Some of the validation is done during construction (i.e. validity of JsonConverter, inner types etc.)
// therefore we need to unwrap TargetInvocationException for better user experience
ExceptionDispatchInfo.Capture(ex.InnerException).Throw();
throw ex.InnerException;
throw null!;
}
#endif
}
Expand Down
12 changes: 0 additions & 12 deletions ...ries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonMetadataServices.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,19 +81,7 @@ public static JsonTypeInfo<T> CreateObjectInfo<T>(JsonSerializerOptions options!
public static JsonTypeInfo<T> CreateValueInfo<T>(JsonSerializerOptions options, JsonConverter converter)
{
JsonTypeInfo<T> info = new SourceGenJsonTypeInfo<T>(converter, options);
info.PropertyInfoForTypeInfo = CreateJsonPropertyInfoForClassInfo(typeof(T), info, converter, options);
return info;
}

internal static JsonPropertyInfo CreateJsonPropertyInfoForClassInfo(
Type type,
JsonTypeInfo typeInfo,
JsonConverter converter,
JsonSerializerOptions options)
{
JsonPropertyInfo propertyInfo = converter.CreateJsonPropertyInfo();
propertyInfo.InitializeForTypeInfo(type, typeInfo, converter, options);
return propertyInfo;
}
}
}
52 changes: 16 additions & 36 deletions ...ibraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,24 +90,21 @@ internal void Configure()
_isConfigured = true;
}

internal virtual void GetPolicies(JsonIgnoreCondition? ignoreCondition)
internal void GetPolicies(JsonIgnoreCondition? ignoreCondition)
{
if (!IsForTypeInfo)
{
Debug.Assert(MemberInfo != null);
DetermineSerializationCapabilities(ignoreCondition);
DeterminePropertyName();
DetermineIgnoreCondition(ignoreCondition);

JsonPropertyOrderAttribute? orderAttr = GetAttribute<JsonPropertyOrderAttribute>(MemberInfo);
if (orderAttr != null)
{
Order = orderAttr.Order;
}
Debug.Assert(MemberInfo != null);
DetermineSerializationCapabilities(ignoreCondition);
DeterminePropertyName();
DetermineIgnoreCondition(ignoreCondition);

JsonNumberHandlingAttribute? attribute = GetAttribute<JsonNumberHandlingAttribute>(MemberInfo);
NumberHandling = attribute?.Handling;
JsonPropertyOrderAttribute? orderAttr = GetAttribute<JsonPropertyOrderAttribute>(MemberInfo);
if (orderAttr != null)
{
Order = orderAttr.Order;
}

JsonNumberHandlingAttribute? attribute = GetAttribute<JsonNumberHandlingAttribute>(MemberInfo);
NumberHandling = attribute?.Handling;
}

private void DeterminePropertyName()
Expand Down Expand Up @@ -330,33 +327,16 @@ private bool NumberHandingIsApplicable()
internal bool HasGetter { get; set; }
internal bool HasSetter { get; set; }

internal virtual void Initialize(
internal abstract void Initialize(
Type parentClassType,
Type declaredPropertyType,
ConverterStrategy converterStrategy,
MemberInfo? memberInfo,
bool isVirtual,
JsonConverter converter,
JsonIgnoreCondition? ignoreCondition,
JsonNumberHandling? parentTypeNumberHandling,
JsonSerializerOptions options)
{
Debug.Assert(converter != null);

DeclaringType = parentClassType;
PropertyType = declaredPropertyType;
ConverterStrategy = converterStrategy;
MemberInfo = memberInfo;
IsVirtual = isVirtual;
ConverterBase = converter;
Options = options;
}

internal abstract void InitializeForTypeInfo(
Type declaredType,
JsonTypeInfo runtimeTypeInfo,
JsonConverter converter,
JsonSerializerOptions options);
JsonSerializerOptions options,
JsonTypeInfo? jsonTypeInfo = null);

internal bool IgnoreDefaultValuesOnRead { get; private set; }
internal bool IgnoreDefaultValuesOnWrite { get; private set; }
Expand Down Expand Up @@ -483,7 +463,7 @@ internal bool ReadJsonExtensionDataValue(ref ReadStack state, ref Utf8JsonReader

internal Type DeclaringType { get; set; } = null!;

internal MemberInfo? MemberInfo { get; private set; }
internal MemberInfo? MemberInfo { get; set; }

internal JsonTypeInfo JsonTypeInfo
{
Expand Down
145 changes: 66 additions & 79 deletions ...aries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfoOfT.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,78 +43,89 @@ internal override void Initialize(
bool isVirtual,
JsonConverter converter,
JsonIgnoreCondition? ignoreCondition,
JsonNumberHandling? parentTypeNumberHandling,

@eiriktsarpalis eiriktsarpalis Apr 7, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Am I right to understand that this parameter was not being used anywhere?

@krwq krwq Apr 7, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

yes, we assign it in the Configure() as it could get out of sync once we get into contract resolving which will allow to change this (also I think it might have not been used before my previous PR as well)

JsonSerializerOptions options)
JsonSerializerOptions options,
JsonTypeInfo? jsonTypeInfo = null)
{
base.Initialize(
parentClassType,
declaredPropertyType,
converterStrategy,
memberInfo,
isVirtual,
converter,
ignoreCondition,
parentTypeNumberHandling,
options);

switch (memberInfo)
Debug.Assert(converter != null);

PropertyType = declaredPropertyType;
ConverterStrategy = converterStrategy;
if (jsonTypeInfo != null)
{
case PropertyInfo propertyInfo:
{
bool useNonPublicAccessors = GetAttribute<JsonIncludeAttribute>(propertyInfo) != null;
JsonTypeInfo = jsonTypeInfo;
}

MethodInfo? getMethod = propertyInfo.GetMethod;
if (getMethod != null && (getMethod.IsPublic || useNonPublicAccessors))
{
HasGetter = true;
Get = options.MemberAccessorStrategy.CreatePropertyGetter<T>(propertyInfo);
}
ConverterBase = converter;
Options = options;
DeclaringType = parentClassType;
MemberInfo = memberInfo;
IsVirtual = isVirtual;

MethodInfo? setMethod = propertyInfo.SetMethod;
if (setMethod != null && (setMethod.IsPublic || useNonPublicAccessors))
if (memberInfo != null)

@eiriktsarpalis eiriktsarpalis Apr 7, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You can actually embed the null check in the switch statement by doing something like

switch (memberInfo)
{
     case null:
         IsForTypeInfo = true;
         HasGetter = true;
         HasSetter = true;
         break;
     case PropertyInfo propertyInfo:
         ...

which should save you a tab of indentation :-)

@krwq krwq Apr 7, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

but we also need to do:

                // TODO (perf): can we pre-compute some of these values during source gen?
                _converterIsExternalAndPolymorphic = !converter.IsInternalConverter && PropertyType != converter.TypeToConvert;
                PropertyTypeCanBeNull = PropertyType.CanBeNull();
                _propertyTypeEqualsTypeToConvert = typeof(T) == PropertyType;


            GetPolicies(ignoreCondition);
                GetPolicies(ignoreCondition);

for memberInfo != null case, I'll leave this as is for now

{
switch (memberInfo)
{
case PropertyInfo propertyInfo:
{
HasSetter = true;
Set = options.MemberAccessorStrategy.CreatePropertySetter<T>(propertyInfo);
}
bool useNonPublicAccessors = GetAttribute<JsonIncludeAttribute>(propertyInfo) != null;

MemberType = MemberTypes.Property;
MethodInfo? getMethod = propertyInfo.GetMethod;
if (getMethod != null && (getMethod.IsPublic || useNonPublicAccessors))
{
HasGetter = true;
Get = options.MemberAccessorStrategy.CreatePropertyGetter<T>(propertyInfo);
}

break;
}
MethodInfo? setMethod = propertyInfo.SetMethod;
if (setMethod != null && (setMethod.IsPublic || useNonPublicAccessors))
{
HasSetter = true;
Set = options.MemberAccessorStrategy.CreatePropertySetter<T>(propertyInfo);
}

case FieldInfo fieldInfo:
{
Debug.Assert(fieldInfo.IsPublic);
MemberType = MemberTypes.Property;

HasGetter = true;
Get = options.MemberAccessorStrategy.CreateFieldGetter<T>(fieldInfo);
break;
}

if (!fieldInfo.IsInitOnly)
case FieldInfo fieldInfo:
{
HasSetter = true;
Set = options.MemberAccessorStrategy.CreateFieldSetter<T>(fieldInfo);
}
Debug.Assert(fieldInfo.IsPublic);

MemberType = MemberTypes.Field;
HasGetter = true;
Get = options.MemberAccessorStrategy.CreateFieldGetter<T>(fieldInfo);

break;
}
if (!fieldInfo.IsInitOnly)
{
HasSetter = true;
Set = options.MemberAccessorStrategy.CreateFieldSetter<T>(fieldInfo);
}

default:
{
IsForTypeInfo = true;
HasGetter = true;
HasSetter = true;
MemberType = MemberTypes.Field;

break;
}
}
break;
}

_converterIsExternalAndPolymorphic = !converter.IsInternalConverter && PropertyType != converter.TypeToConvert;
PropertyTypeCanBeNull = PropertyType.CanBeNull();
_propertyTypeEqualsTypeToConvert = typeof(T) == PropertyType;
default:
{
Debug.Fail($"Invalid memberInfo type: {memberInfo.GetType().FullName}");
break;
}
}

// TODO (perf): can we pre-compute some of these values during source gen?
_converterIsExternalAndPolymorphic = !converter.IsInternalConverter && PropertyType != converter.TypeToConvert;
PropertyTypeCanBeNull = PropertyType.CanBeNull();
_propertyTypeEqualsTypeToConvert = typeof(T) == PropertyType;

GetPolicies(ignoreCondition);
GetPolicies(ignoreCondition);
}
else
{
IsForTypeInfo = true;
HasGetter = true;
HasSetter = true;
}
}

internal void InitializeForSourceGen(JsonSerializerOptions options, JsonPropertyInfoValues<T> propertyInfo)
Expand Down Expand Up @@ -189,30 +200,6 @@ internal void InitializeForSourceGen(JsonSerializerOptions options, JsonProperty
}
}

/// <summary>
/// Create a <see cref="JsonPropertyInfo"/> for a given Type.
/// See <seealso cref="JsonTypeInfo.PropertyInfoForTypeInfo"/>.
/// </summary>
internal override void InitializeForTypeInfo(
Type declaredType,
JsonTypeInfo runtimeTypeInfo,
JsonConverter converter,
JsonSerializerOptions options)
{
PropertyType = declaredType;
ConverterStrategy = converter.ConverterStrategy;
JsonTypeInfo = runtimeTypeInfo;
ConverterBase = converter;
Options = options;
IsForTypeInfo = true;
HasGetter = true;
HasSetter = true;
// TODO (perf): can we pre-compute some of these values during source gen?
_converterIsExternalAndPolymorphic = !converter.IsInternalConverter && declaredType != converter.TypeToConvert;
PropertyTypeCanBeNull = declaredType.CanBeNull();
_propertyTypeEqualsTypeToConvert = typeof(T) == declaredType;
}

internal override JsonConverter ConverterBase
{
get
Expand Down
18 changes: 9 additions & 9 deletions ...raries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.Cache.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,8 @@ internal static JsonPropertyInfo CreateProperty(
bool isVirtual,
JsonConverter converter,
JsonSerializerOptions options,
JsonNumberHandling? parentTypeNumberHandling = null,
JsonIgnoreCondition? ignoreCondition = null)
JsonIgnoreCondition? ignoreCondition = null,
JsonTypeInfo? jsonTypeInfo = null)
{
// Create the JsonPropertyInfo instance.
JsonPropertyInfo jsonPropertyInfo = converter.CreateJsonPropertyInfo();
Expand All @@ -72,8 +72,8 @@ internal static JsonPropertyInfo CreateProperty(
isVirtual,
converter,
ignoreCondition,
parentTypeNumberHandling,
options);
options,
jsonTypeInfo);

return jsonPropertyInfo;
}
Expand All @@ -85,17 +85,17 @@ internal static JsonPropertyInfo CreateProperty(
private static JsonPropertyInfo CreatePropertyInfoForTypeInfo(
Type declaredPropertyType,
JsonConverter converter,
JsonNumberHandling? numberHandling,
JsonSerializerOptions options)
JsonSerializerOptions options,
JsonTypeInfo? jsonTypeInfo = null)
{
JsonPropertyInfo jsonPropertyInfo = CreateProperty(
declaredPropertyType: declaredPropertyType,
memberInfo: null, // Not a real property so this is null.
parentClassType: ObjectType, // a dummy value (not used)
isVirtual: false,
converter,
options,
parentTypeNumberHandling: numberHandling);
converter: converter,
options: options,
jsonTypeInfo: jsonTypeInfo);

Debug.Assert(jsonPropertyInfo.IsForTypeInfo);

Expand Down
17 changes: 2 additions & 15 deletions src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ internal JsonTypeInfo? KeyTypeInfo
/// TypeInfo (for the cases mentioned above). In addition, methods that have a JsonPropertyInfo argument would also likely

@eiriktsarpalis eiriktsarpalis Apr 7, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

FWIW I suspect the justification presented in the comments is obsolete:

The JsonPropertyInfo for this JsonTypeInfo. It is used to obtain the converter for the TypeInfo.

Since all construction paths now require JsonConverter it seems like this could be replaced with a JsonConverter property.

@krwq krwq Apr 7, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

it's not that simple. Currently Read/WriteState heavly depend on converter being on PropertyInfo and that needs heavy refactoring in there. It's a huge mess as I've already gave it a quick attempt.

/// need to add an argument for JsonTypeInfo.
/// </remarks>
internal JsonPropertyInfo PropertyInfoForTypeInfo { get; set; }
internal JsonPropertyInfo PropertyInfoForTypeInfo { get; private set; }

/// <summary>
/// Returns a helper class used for computing the default value.
Expand All @@ -162,24 +162,11 @@ internal JsonTypeInfo? KeyTypeInfo

internal JsonNumberHandling? NumberHandling { get; set; }

internal JsonTypeInfo()
{
Debug.Assert(false, "This constructor should not be called.");
}

internal JsonTypeInfo(Type type, JsonSerializerOptions options!!)
{
Type = type;
Options = options;
// Setting this option is deferred to the initialization methods of the various metadada info types.
PropertyInfoForTypeInfo = null!;
}

internal JsonTypeInfo(Type type, JsonConverter converter, JsonSerializerOptions options)
{
Type = type;
Options = options;
PropertyInfoForTypeInfo = CreatePropertyInfoForTypeInfo(Type, converter, NumberHandling, Options);
PropertyInfoForTypeInfo = CreatePropertyInfoForTypeInfo(Type, converter, Options, this);
ElementType = converter.ElementType;

switch (PropertyInfoForTypeInfo.ConverterStrategy)
Expand Down
9 changes: 0 additions & 9 deletions ...libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfoOfT.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,19 +15,10 @@ public abstract class JsonTypeInfo<T> : JsonTypeInfo
{
private Action<Utf8JsonWriter, T>? _serialize;

internal JsonTypeInfo(Type type, JsonSerializerOptions options) :
base(type, options)
{ }

internal JsonTypeInfo(JsonConverter converter, JsonSerializerOptions options)
: base(typeof(T), converter, options)
{ }

internal JsonTypeInfo()
{
Debug.Assert(false, "This constructor should not be called.");
}

/// <summary>
/// Serializes an instance of <typeparamref name="T"/> using
/// <see cref="JsonSourceGenerationOptionsAttribute"/> values specified at design time.
Expand Down
Loading