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 Reflection and SourceGen TypeInfos #67526

Merged
merged 3 commits into from
Apr 7, 2022
Merged

Split Reflection and SourceGen TypeInfos #67526

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
46a5687
Split Reflection and SourceGen TypeInfos
krwq Apr 4, 2022
08ca87b
Apply PR feedback
krwq Apr 5, 2022
a772f78
improve exception unwrapping
krwq Apr 6, 2022
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
3 changes: 2 additions & 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 @@ -46,6 +46,8 @@ static JsonTypeInfo CreateJsonTypeInfo(Type type, JsonSerializerOptions options)
JsonTypeInfo.ValidateType(type, null, null, options);
Copy link
Member

Choose a reason for hiding this comment

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

Nit: probably fine for the scope of this PR, but eventually I'd like to see this method moved out of JsonSerializerOptions responsibility.

Copy link
Member Author

Choose a reason for hiding this comment

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

most of the work this does is checking if the type is complete generic type so essentially I'd need to duplicate that logic elsewhere, we previously could have it as part of JsonTypeInfo because it wasn't generic but now we cannot instantiate it otherwise


MethodInfo methodInfo = typeof(JsonSerializerOptions).GetMethod(nameof(CreateReflectionJsonTypeInfo), BindingFlags.NonPublic | BindingFlags.Instance)!;
// Some of the validation is done during construction (i.e. validity of JsonConverter, inner types etc.)
krwq marked this conversation as resolved.
Show resolved Hide resolved
// therefore we need to unwrap TargetInvocationException for better user experience
#if NETCOREAPP
return (JsonTypeInfo)methodInfo.MakeGenericMethod(type).Invoke(options, BindingFlags.NonPublic | BindingFlags.DoNotWrapExceptions, null, null, null)!;
#else
Expand All @@ -64,7 +66,6 @@ static JsonTypeInfo CreateJsonTypeInfo(Type type, JsonSerializerOptions options)
[RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)]
private JsonTypeInfo<T> CreateReflectionJsonTypeInfo<T>()
{
// We do not use Activator.CreateInstance because it will wrap exception if constructor throws it
return new ReflectionJsonTypeInfo<T>(this);
}

Expand Down
2 changes: 1 addition & 1 deletion 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 @@ -167,7 +167,7 @@ internal JsonTypeInfo()
Debug.Assert(false, "This constructor should not be called.");
}

internal JsonTypeInfo(Type type, JsonSerializerOptions options!!, bool dummy)
internal JsonTypeInfo(Type type, JsonSerializerOptions options!!)
Copy link
Member

Choose a reason for hiding this comment

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

Can we remove this constructor altogether? IIRC all instantiation paths for JsonTypeInfo require JsonConverter one way or another.

Copy link
Member Author

Choose a reason for hiding this comment

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

technically I could do further refactoring to keep just single constructor but I prefer to keep it separate because need to make sure this will still work correctly with source gen which makes it a bit convoluted

{
Type = type;
Options = options;
Expand Down
2 changes: 1 addition & 1 deletion ...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 @@ -16,7 +16,7 @@ public abstract class JsonTypeInfo<T> : JsonTypeInfo
private Action<Utf8JsonWriter, T>? _serialize;

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

internal JsonTypeInfo(JsonConverter converter, JsonSerializerOptions options)
Expand Down
2 changes: 1 addition & 1 deletion ...System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionJsonTypeInfoOfT.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ namespace System.Text.Json.Serialization.Metadata
/// <summary>
/// Provides JSON serialization-related metadata about a type.
/// </summary>
internal class ReflectionJsonTypeInfo<T> : JsonTypeInfo<T>
internal sealed class ReflectionJsonTypeInfo<T> : JsonTypeInfo<T>
{
[RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)]
internal ReflectionJsonTypeInfo(JsonSerializerOptions options)
Expand Down