-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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.
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
Apply a variant of the visitor pattern for reflection-based generic type instantiation. #72373
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,9 +4,9 @@ | |
using System.Collections.Generic; | ||
using System.Diagnostics; | ||
using System.Diagnostics.CodeAnalysis; | ||
using System.Reflection; | ||
using System.Runtime.CompilerServices; | ||
using System.Runtime.ExceptionServices; | ||
using System.Text.Json.Reflection; | ||
using System.Threading; | ||
|
||
namespace System.Text.Json.Serialization.Metadata | ||
|
@@ -533,11 +533,25 @@ public static JsonTypeInfo<T> CreateJsonTypeInfo<T>(JsonSerializerOptions option | |
ThrowHelper.ThrowArgumentNullException(nameof(options)); | ||
} | ||
|
||
return CreateCustomJsonTypeInfo<T>(options); | ||
} | ||
|
||
[RequiresUnreferencedCode(MetadataFactoryRequiresUnreferencedCode)] | ||
[RequiresDynamicCode(MetadataFactoryRequiresUnreferencedCode)] | ||
private static JsonTypeInfo<T> CreateCustomJsonTypeInfo<T>(JsonSerializerOptions options) | ||
{ | ||
JsonConverter converter = DefaultJsonTypeInfoResolver.GetConverterForType(typeof(T), options, resolveJsonConverterAttribute: false); | ||
return new CustomJsonTypeInfo<T>(converter, options); | ||
} | ||
|
||
private static MethodInfo? s_createJsonTypeInfo; | ||
[RequiresUnreferencedCode(MetadataFactoryRequiresUnreferencedCode)] | ||
[RequiresDynamicCode(MetadataFactoryRequiresUnreferencedCode)] | ||
private sealed class CustomJsonTypeInfoBuilder : ITypeVisitor<JsonSerializerOptions, JsonTypeInfo> | ||
{ | ||
public static CustomJsonTypeInfoBuilder Instance { get; } = new(); | ||
public JsonTypeInfo Visit<T>(JsonSerializerOptions options) | ||
=> CreateCustomJsonTypeInfo<T>(options); | ||
} | ||
|
||
/// <summary> | ||
/// Creates a blank <see cref="JsonTypeInfo"/> instance. | ||
|
@@ -576,9 +590,7 @@ public static JsonTypeInfo CreateJsonTypeInfo(Type type, JsonSerializerOptions o | |
ThrowHelper.ThrowArgumentException_CannotSerializeInvalidType(nameof(type), type, null, null); | ||
} | ||
|
||
s_createJsonTypeInfo ??= typeof(JsonTypeInfo).GetMethod(nameof(CreateJsonTypeInfo), new Type[] { typeof(JsonSerializerOptions) })!; | ||
return (JsonTypeInfo)s_createJsonTypeInfo.MakeGenericMethod(type) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just a note that this PR is changing the refactoring done in #67526. In effect, replacing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's using |
||
.Invoke(null, new object[] { options })!; | ||
return CustomJsonTypeInfoBuilder.Instance.Visit(type, options); | ||
} | ||
|
||
/// <summary> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
using System.Diagnostics.CodeAnalysis; | ||
|
||
namespace System.Text.Json.Reflection | ||
{ | ||
/// <summary> | ||
/// Encapsulates a generic type parameter that can be unpacked on-demand using a generic visitor, | ||
/// encoding an existential type. Can use reflection to instantiate type parameters that are not known at compile time. | ||
/// Uses concepts taken from https://www.microsoft.com/en-us/research/publication/generalized-algebraic-data-types-and-object-oriented-programming/ | ||
eiriktsarpalis marked this conversation as resolved.
Show resolved
Hide resolved
eiriktsarpalis marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/// </summary> | ||
internal abstract class TypeWitness | ||
{ | ||
public abstract Type Type { get; } | ||
public abstract TResult Accept<TState, TResult>(ITypeVisitor<TState, TResult> builder, TState state); | ||
|
||
[RequiresUnreferencedCode("Uses Type.MakeGenericType to instantiate a generic TypeWitness<T> using reflection.")] | ||
[RequiresDynamicCode("Uses Type.MakeGenericType to instantiate a generic TypeWitness<T> using reflection.")] | ||
public static TypeWitness Create(Type type) => (TypeWitness)Activator.CreateInstance(typeof(TypeWitness<>).MakeGenericType(type))!; | ||
} | ||
|
||
internal sealed class TypeWitness<T> : TypeWitness | ||
{ | ||
public override Type Type => typeof(T); | ||
public override TResult Accept<TState, TResult>(ITypeVisitor<TState, TResult> builder, TState state) | ||
=> builder.Visit<T>(state); | ||
} | ||
|
||
/// <summary> | ||
/// Given a <typeparamref name="TState" /> input, visits the generic parameter of a | ||
/// <see cref="TypeWitness{T}"/> and returns a <typeparamref name="TResult" /> output. | ||
/// </summary> | ||
internal interface ITypeVisitor<TState, TResult> | ||
{ | ||
TResult Visit<T>(TState state); | ||
} | ||
|
||
internal static class TypeVisitorExtensions | ||
{ | ||
[RequiresUnreferencedCode("Uses Type.MakeGenericType to instantiate a generic TypeWitness<T> using reflection.")] | ||
[RequiresDynamicCode("Uses Type.MakeGenericType to instantiate a generic TypeWitness<T> using reflection.")] | ||
public static TResult Visit<TState, TResult>(this ITypeVisitor<TState, TResult> visitor, Type type, TState state) | ||
=> TypeWitness.Create(type).Accept(visitor, state); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI there is
BindingFlags.DoNotWrapExceptions
that lets the raw exception throw.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not available in
netstandard2.0
, hence this extra code. That being said, it is evident from this PR that we haven't been applying the flag consistently.