Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
244 changes: 96 additions & 148 deletions src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -1465,11 +1465,11 @@ private static bool PropertyAccessorCanBeReferenced(MethodInfo? accessor)

if (forType)
{
return $"{Emitter.GetConverterFromFactoryMethodName}(typeof({type.GetCompilableName()}), new {converterType.GetCompilableName()}())";
return $"{Emitter.GetConverterFromFactoryMethodName}({OptionsLocalVariableName}, typeof({type.GetCompilableName()}), new {converterType.GetCompilableName()}())";
}
else
{
return $"{Emitter.JsonContextVarName}.{Emitter.GetConverterFromFactoryMethodName}<{type.GetCompilableName()}>(new {converterType.GetCompilableName()}())";
return $"{Emitter.GetConverterFromFactoryMethodName}<{type.GetCompilableName()}>({OptionsLocalVariableName}, new {converterType.GetCompilableName()}())";
}
}

Expand Down
5 changes: 5 additions & 0 deletions src/libraries/System.Text.Json/gen/TypeGenerationSpec.cs
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,11 @@ internal sealed class TypeGenerationSpec
/// </summary>
public string TypeInfoPropertyName { get; set; }

/// <summary>
/// Method used to generate JsonTypeInfo given options instance
/// </summary>
public string CreateTypeInfoMethodName => $"Create_{TypeInfoPropertyName}";

public JsonSourceGenerationMode GenerationMode { get; set; }

public bool GenerateMetadata => GenerationModeIsSpecified(JsonSourceGenerationMode.Metadata);
Expand Down
4 changes: 3 additions & 1 deletion src/libraries/System.Text.Json/ref/System.Text.Json.cs
Original file line number Diff line number Diff line change
Expand Up @@ -960,7 +960,6 @@ public JsonSerializableAttribute(System.Type type) { }
public abstract partial class JsonSerializerContext : System.Text.Json.Serialization.Metadata.IJsonTypeInfoResolver
{
protected JsonSerializerContext(System.Text.Json.JsonSerializerOptions? options) { }
protected JsonSerializerContext(System.Text.Json.JsonSerializerOptions? options, bool bindOptionsToContext) { }
protected abstract System.Text.Json.JsonSerializerOptions? GeneratedSerializerOptions { get; }
public System.Text.Json.JsonSerializerOptions Options { get { throw null; } }
public abstract System.Text.Json.Serialization.Metadata.JsonTypeInfo? GetTypeInfo(System.Type type);
Expand Down Expand Up @@ -1139,6 +1138,7 @@ public static partial class JsonMetadataServices
public static System.Text.Json.Serialization.JsonConverter<T> GetUnsupportedTypeConverter<T>() { throw null; }
public static System.Text.Json.Serialization.JsonConverter<T> GetEnumConverter<T>(System.Text.Json.JsonSerializerOptions options) where T : struct { throw null; }
public static System.Text.Json.Serialization.JsonConverter<T?> GetNullableConverter<T>(System.Text.Json.Serialization.Metadata.JsonTypeInfo<T> underlyingTypeInfo) where T : struct { throw null; }
public static System.Text.Json.Serialization.JsonConverter<T?> GetNullableConverter<T>(System.Text.Json.JsonSerializerOptions options) where T : struct { throw null; }
}
[System.ComponentModel.EditorBrowsableAttribute(System.ComponentModel.EditorBrowsableState.Never)]
public sealed partial class JsonObjectInfoValues<T>
Expand Down Expand Up @@ -1206,6 +1206,8 @@ internal JsonTypeInfo() { }
public System.Func<object>? CreateObject { get { throw null; } set { } }
public System.Text.Json.Serialization.Metadata.JsonTypeInfoKind Kind { get { throw null; } }
public System.Text.Json.Serialization.JsonNumberHandling? NumberHandling { get { throw null; } set { } }
[System.Diagnostics.CodeAnalysis.RequiresUnreferencedCode("JSON serialization and deserialization might require types that cannot be statically analyzed and might need runtime code generation. Use generic overload or System.Text.Json source generation for native AOT applications.")]
[System.Diagnostics.CodeAnalysis.RequiresDynamicCode("JSON serialization and deserialization might require types that cannot be statically analyzed and might need runtime code generation. Use generic overload or System.Text.Json source generation for native AOT applications.")]
public static System.Text.Json.Serialization.Metadata.JsonTypeInfo<T> CreateJsonTypeInfo<T>(System.Text.Json.JsonSerializerOptions options) { throw null; }
[System.Diagnostics.CodeAnalysis.RequiresUnreferencedCode("JSON serialization and deserialization might require types that cannot be statically analyzed and might need runtime code generation. Use generic overload or System.Text.Json source generation for native AOT applications.")]
[System.Diagnostics.CodeAnalysis.RequiresDynamicCode("JSON serialization and deserialization might require types that cannot be statically analyzed and might need runtime code generation. Use generic overload or System.Text.Json source generation for native AOT applications.")]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,12 +37,12 @@ public override bool CanConvert(Type typeToConvert) =>
case FSharpKind.Option:
elementType = typeToConvert.GetGenericArguments()[0];
converterFactoryType = typeof(FSharpOptionConverter<,>).MakeGenericType(typeToConvert, elementType);
constructorArguments = new object[] { options.GetConverterInternal(elementType) };
constructorArguments = new object[] { options.GetConverterFromTypeInfo(elementType) };
break;
case FSharpKind.ValueOption:
elementType = typeToConvert.GetGenericArguments()[0];
converterFactoryType = typeof(FSharpValueOptionConverter<,>).MakeGenericType(typeToConvert, elementType);
constructorArguments = new object[] { options.GetConverterInternal(elementType) };
constructorArguments = new object[] { options.GetConverterFromTypeInfo(elementType) };
break;
case FSharpKind.List:
elementType = typeToConvert.GetGenericArguments()[0];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ namespace System.Text.Json.Serialization.Converters
/// <typeparam name="T">The type to converter</typeparam>
internal sealed class JsonMetadataServicesConverter<T> : JsonResumableConverter<T>
{
private readonly Func<JsonConverter<T>> _converterCreator;
private readonly Func<JsonConverter<T>>? _converterCreator;

private readonly ConverterStrategy _converterStrategy;

Expand All @@ -26,7 +26,7 @@ internal JsonConverter<T> Converter
{
get
{
_converter ??= _converterCreator();
_converter ??= _converterCreator!();
Debug.Assert(_converter != null);
Debug.Assert(_converter.ConverterStrategy == _converterStrategy);
return _converter;
Expand Down Expand Up @@ -54,6 +54,12 @@ public JsonMetadataServicesConverter(Func<JsonConverter<T>> converterCreator, Co
_converterStrategy = converterStrategy;
}

public JsonMetadataServicesConverter(JsonConverter<T> converter)
{
_converter = converter;
_converterStrategy = converter.ConverterStrategy;
}

internal override bool OnTryRead(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options, ref ReadStack state, out T? value)
=> Converter.OnTryRead(ref reader, typeToConvert, options, ref state, out value);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ internal override void WriteAsPropertyNameCore(Utf8JsonWriter writer, object? va
Debug.Assert(value != null);

Type runtimeType = value.GetType();
JsonConverter runtimeConverter = options.GetConverterInternal(runtimeType);
JsonConverter runtimeConverter = options.GetConverterFromTypeInfo(runtimeType);
if (runtimeConverter == this)
{
ThrowHelper.ThrowNotSupportedException_DictionaryKeyTypeNotSupported(runtimeType, this);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ public override JsonConverter CreateConverter(Type typeToConvert, JsonSerializer

Type valueTypeToConvert = typeToConvert.GetGenericArguments()[0];

JsonConverter valueConverter = options.GetConverterInternal(valueTypeToConvert);
JsonConverter valueConverter = options.GetConverterFromTypeInfo(valueTypeToConvert);
Debug.Assert(valueConverter != null);

// If the value type has an interface or object converter, just return that converter directly.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,32 +81,11 @@ internal bool CanUseSerializationLogic
/// or until <see cref="Options"/> is called, where a new options instance is created and bound.
/// </remarks>
protected JsonSerializerContext(JsonSerializerOptions? options)
: this(options, bindOptionsToContext: true)
{
}

/// <summary>
/// Creates an instance of <see cref="JsonSerializerContext"/> and optionally binds it with the indicated <see cref="JsonSerializerOptions"/>.
/// </summary>
/// <param name="options">The run time provided options for the context instance.</param>
/// <param name="bindOptionsToContext">Specify whether the options <paramref name="options"/> instance should be bound to the new context.</param>
/// <remarks>
/// If no instance options are passed, then no options are set until the context is bound using <see cref="JsonSerializerOptions.AddContext{TContext}"/>,
/// or until <see cref="Options"/> is called, where a new options instance is created and bound.
/// </remarks>
protected JsonSerializerContext(JsonSerializerOptions? options, bool bindOptionsToContext)
{
if (options != null)
{
if (bindOptionsToContext)
{
options.TypeInfoResolver = this;
Debug.Assert(_options == options, "options.TypeInfoResolver setter did not assign options");
}
else
{
_options = options;
}
options.TypeInfoResolver = this;
Debug.Assert(_options == options, "options.TypeInfoResolver setter did not assign options");
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,14 +23,27 @@ public sealed partial class JsonSerializerOptions
// Simple LRU cache for the public (de)serialize entry points that avoid some lookups in _cachingContext.
private volatile JsonTypeInfo? _lastTypeInfo;

/// <summary>
/// This method returns configured non-null JsonTypeInfo
/// </summary>
internal JsonTypeInfo GetOrAddJsonTypeInfo(Type type)
{
if (_cachingContext == null)
{
InitializeCachingContext();
}

return _cachingContext.GetOrAddJsonTypeInfo(type);
JsonTypeInfo? typeInfo = _cachingContext.GetOrAddJsonTypeInfo(type);

if (typeInfo == null)
{
ThrowHelper.ThrowNotSupportedException_NoMetadataForType(type);
return null;
}

typeInfo.EnsureConfigured();

return typeInfo;
}

internal bool TryGetJsonTypeInfo(Type type, [NotNullWhen(true)] out JsonTypeInfo? typeInfo)
Expand Down Expand Up @@ -85,7 +98,6 @@ private void InitializeCachingContext()
/// </summary>
internal sealed class CachingContext
{
private readonly ConcurrentDictionary<Type, JsonConverter> _converterCache = new();
private readonly ConcurrentDictionary<Type, JsonTypeInfo> _jsonTypeInfoCache = new();

public CachingContext(JsonSerializerOptions options)
Expand All @@ -96,15 +108,29 @@ public CachingContext(JsonSerializerOptions options)
public JsonSerializerOptions Options { get; }
// Property only accessed by reflection in testing -- do not remove.
// If changing please ensure that src/ILLink.Descriptors.LibraryBuild.xml is up-to-date.
public int Count => _converterCache.Count + _jsonTypeInfoCache.Count;
public JsonConverter GetOrAddConverter(Type type) => _converterCache.GetOrAdd(type, Options.GetConverterFromType);
public JsonTypeInfo GetOrAddJsonTypeInfo(Type type) => _jsonTypeInfoCache.GetOrAdd(type, Options.GetJsonTypeInfoFromContextOrCreate);
public int Count => _jsonTypeInfoCache.Count;

public JsonTypeInfo? GetOrAddJsonTypeInfo(Type type)
{
if (_jsonTypeInfoCache.TryGetValue(type, out JsonTypeInfo? typeInfo))
{
return typeInfo;
}

typeInfo = Options.GetTypeInfoInternal(type);
if (typeInfo != null)
{
return _jsonTypeInfoCache.GetOrAdd(type, _ => typeInfo);
}

return null;
}

public bool TryGetJsonTypeInfo(Type type, [NotNullWhen(true)] out JsonTypeInfo? typeInfo) => _jsonTypeInfoCache.TryGetValue(type, out typeInfo);
public bool IsJsonTypeInfoCached(Type type) => _jsonTypeInfoCache.ContainsKey(type);

public void Clear()
{
_converterCache.Clear();
_jsonTypeInfoCache.Clear();
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,9 +55,12 @@ public sealed partial class JsonSerializerOptions
return converter;
}

/// <summary>
/// Gets converter for type but does not use TypeInfoResolver
/// </summary>
internal JsonConverter GetConverterForType(Type typeToConvert)
{
JsonConverter converter = GetConverterInternal(typeToConvert);
JsonConverter converter = GetConverterFromOptionsOrReflectionConverter(typeToConvert);
Debug.Assert(converter != null);

converter = ExpandFactoryConverter(converter, typeToConvert);
Expand Down Expand Up @@ -122,39 +125,67 @@ public JsonConverter GetConverter(Type typeToConvert)
}

DefaultJsonTypeInfoResolver.RootDefaultInstance();
return GetConverterInternal(typeToConvert);
return GetConverterFromTypeInfo(typeToConvert);
}

internal JsonConverter GetConverterInternal(Type typeToConvert)
/// <summary>
/// Same as GetConverter but does not root converters
/// </summary>
internal JsonConverter GetConverterFromTypeInfo(Type typeToConvert)
{
// Only cache the value once (de)serialization has occurred since new converters can be added that may change the result.
if (_cachingContext != null)
if (_cachingContext == null)
{
return _cachingContext.GetOrAddConverter(typeToConvert);
if (_isLockedInstance)
{
InitializeCachingContext();
}
else
{
// We do not want to lock options instance here but we need to return correct answer
// which means we need to go through TypeInfoResolver but without caching because that's the
// only place which will have correct converter for JsonSerializerContext and reflection
// based resolver. It will also work correctly for combined resolvers.
return GetTypeInfoInternal(typeToConvert)?.Converter
?? GetConverterFromOptionsOrReflectionConverter(typeToConvert);

}
}

return GetConverterFromType(typeToConvert);
}
JsonConverter? converter = _cachingContext.GetOrAddJsonTypeInfo(typeToConvert)?.Converter;

internal JsonConverter GetConverterFromType(Type typeToConvert)
{
Debug.Assert(typeToConvert != null);
// we can get here if resolver returned null but converter was added for the type
converter ??= GetConverterFromOptions(typeToConvert);

// Priority 1: If there is a JsonSerializerContext, fetch the converter from there.
JsonConverter? converter = SerializerContext?.GetTypeInfo(typeToConvert)?.Converter;
if (converter == null)
{
ThrowHelper.ThrowNotSupportedException_BuiltInConvertersNotRooted(typeToConvert);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems wrong. Why would we throw this error if JsonSerializerOptions.Converters doesn't contain a compatible converter? As the name suggests the error is meant to be thrown if the built-in converters have not been rooted.

Copy link
Owner Author

Choose a reason for hiding this comment

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

As I mentioned in other places, we don't throw here unless:

  • rooting logic was never exercised
  • we somehow ended up using resolver which source gen did not generate type info code for (that's what we also did before this change) - same applies if multiple contexts are combined

return null!;
}

return converter;
}

// Priority 2: Attempt to get custom converter added at runtime.
// Currently there is not a way at runtime to override the [JsonConverter] when applied to a property.
private JsonConverter? GetConverterFromOptions(Type typeToConvert)
Copy link
Collaborator

Choose a reason for hiding this comment

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

In the current behaviour we want to return the built-in converters (or throw an exception if they haven't been rooted yet). This seems like a breaking change?

Copy link
Owner Author

Choose a reason for hiding this comment

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

it's not breaking change. If you call GetConverter directly it will root converter first and then call this method will behave as it was.

On the other hand if this is called when context is bound but converters are not rooted (it may be called implicitly) you will get an exception same way as you used to.

{
foreach (JsonConverter item in _converters)
{
if (item.CanConvert(typeToConvert))
{
converter = item;
break;
return item;
}
}

// Priority 3: Attempt to get converter from [JsonConverter] on the type being converted.
return null;
}

private JsonConverter GetConverterFromOptionsOrReflectionConverter(Type typeToConvert)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Calling it ResolverConverter over GetConverter might be better at emphasizing this is not caching

Suggested change
private JsonConverter GetConverterFromOptionsOrReflectionConverter(Type typeToConvert)
private JsonConverter ResolveConverter(Type typeToConvert)

Copy link
Owner Author

Choose a reason for hiding this comment

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

this actually isn't resolving converter - this is technically speaking reflection converter logic. From user perspective the order is following:

  • type info converter (i.e. serializer context)
  • options
  • reflection converter (if rooted, if GetConverter called explicitly then this is always rooted)

this method does the last two and it's used by: reflection json type info and as a fallback for GetConverter (the fallback is arguably wrong but since we already shipped like that this logic is preserved here)

Copy link
Collaborator

Choose a reason for hiding this comment

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

this actually isn't resolving converter

Arguably it's resolving a converter based on the Converters list and the built-in stuff. My only concern is that we have accumulated too many JsonSerializer.GetConverter* method names and it might be in our interest to adopt a separate naming convention for the converter resolution logic that doesn't involve the cache in any way.

Copy link
Owner Author

@krwq krwq Jun 14, 2022

Choose a reason for hiding this comment

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

There are only two case which must not use cache and first one could also cause stack overflow: reflection/custom type info resolver which needs to get converter; calling GetConverter before options are locked (caching cannot happen because converters/type infos can still be changed). Everything else is should go through caching

{
Debug.Assert(typeToConvert != null);

// Priority 1: Attempt to get custom converter from the Converters list.
JsonConverter? converter = GetConverterFromOptions(typeToConvert);

// Priority 2: Attempt to get converter from [JsonConverter] on the type being converted.
if (converter == null)
{
JsonConverterAttribute? converterAttribute = (JsonConverterAttribute?)
Expand All @@ -166,7 +197,7 @@ internal JsonConverter GetConverterFromType(Type typeToConvert)
}
}

// Priority 4: Attempt to get built-in converter.
// Priority 3: Attempt to get built-in converter.
if (converter == null)
{
converter = DefaultJsonTypeInfoResolver.GetDefaultConverter(typeToConvert);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -652,28 +652,24 @@ internal void InitializeForReflectionSerializer()
IsInitializedForReflectionSerializer = true;
}

private JsonTypeInfo GetJsonTypeInfoFromContextOrCreate(Type type)
private JsonTypeInfo? GetTypeInfoInternal(Type type)
{
IJsonTypeInfoResolver? resolver = _effectiveJsonTypeInfoResolver ?? _typeInfoResolver;
JsonTypeInfo? info = resolver?.GetTypeInfo(type, this);

if (info == null)
if (info != null)
{
ThrowHelper.ThrowNotSupportedException_NoMetadataForType(type);
return null!;
}

if (info.Type != type)
{
ThrowHelper.ThrowInvalidOperationException_ResolverTypeNotCompatible(type, info.Type);
}
if (info.Type != type)
{
ThrowHelper.ThrowInvalidOperationException_ResolverTypeNotCompatible(type, info.Type);
}

if (info.Options != this)
{
ThrowHelper.ThrowInvalidOperationException_ResolverTypeInfoOptionsNotCompatible();
if (info.Options != this)
{
ThrowHelper.ThrowInvalidOperationException_ResolverTypeInfoOptionsNotCompatible();
}
}

info.EnsureConfigured();
return info;
}

Expand Down
Loading