JsonValue<TValue> is not compatible with trimming

[eerhardt]

In the current design, JsonValue<TValue> can take any TValue type, and it passes the value into JsonSerializer.Serialize. However, it doesn't allow for a source generated JsonTypeInfo to be passed into Serialize.

This is a problem because using JsonSerializer.Serialize without a JsonTypeInfo is not trim-compatible. This is because of the recursive use of Reflection that the trimming tool can't reason about.

In most cases, JsonValue is for primitive types, which are intrinsically handled by the serializer without needing reflection. However, because we don't limit the types that can be passed into JsonValue<T>, we need to mark the JsonValue<TValue>.WriteTo method as RequiresUnreferencedCode:

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Node/JsonValueOfT.cs

Lines 85 to 92 in f372f6b

	if (_value is JsonElement jsonElement)
	{
	jsonElement.WriteTo(writer);
	}
	else
	{
	JsonSerializer.Serialize(writer, _value, _value!.GetType(), options);
	}

But we can't just mark WriteTo as unsafe, because this method is called from the ToString():

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Node/JsonNode.To.cs

Lines 30 to 56 in f372f6b

	public override string ToString()
	{
	// Special case for string; don't quote it.
	if (this is JsonValue)
	{
	if (this is JsonValue<string> jsonString)
	{
	return jsonString.Value;
	}
	if (this is JsonValue<JsonElement> jsonElement &&
	jsonElement.Value.ValueKind == JsonValueKind.String)
	{
	return jsonElement.Value.GetString()!;
	}
	}
	using (var output = new PooledByteBufferWriter(JsonSerializerOptions.BufferSizeDefault))
	{
	using (var writer = new Utf8JsonWriter(output, new JsonWriterOptions { Indented = true }))
	{
	WriteTo(writer);
	}
	return JsonHelpers.Utf8GetString(output.WrittenMemory.ToArray());
	}
	}

Which we can't mark ToString() as RequiresUnreferencedCode, since that would mean the base Object.ToString() and every overridden ToString() method would need to be annotated the same.

To resolve this, I can see the following options:

1. Limit JsonValue<TValue> to only primitive types. Don't allow for custom defined types. This would allow us to suppress the warning, since primitive types can be serialized safely in a trimmed application.
2. We make constructing JsonValue<TValue> with a custom defined type as RequiresUnreferencedCode. This way, it lets callers who create JsonValue instances know that they are using a type that is not trim compatible. Then, we can specific APIs for primitive types, and also add an API to create a JsonValue<TValue> that takes a JsonTypeInfo to use when serializing the value. These will be considered "trim compatible", since it doesn't use the reflection code path. The only thing that will be RequiresUnreferencedCode is creating a JsonValue<TValue> with a custom type, without passing in a JsonTypeInfo. This mirrors the JsonSerializer APIs.

cc @steveharter @layomia

Tagging subscribers to this area: @eiriktsarpalis, @layomia
See info in area-owners.md if you want to be subscribed.

Issue Details

---

In the current design, JsonValue<TValue> can take any TValue type, and it passes the value into JsonSerializer.Serialize. However, it doesn't allow for a source generated JsonTypeInfo to be passed into Serialize.

This is a problem because using JsonSerializer.Serialize without a JsonTypeInfo is not trim-compatible. This is because of the recursive use of Reflection that the trimming tool can't reason about.

In most cases, JsonValue is for primitive types, which are intrinsically handled by the serializer without needing reflection. However, because we don't limit the types that can be passed into JsonValue<T>, we need to mark the JsonValue<TValue>.WriteTo method as RequiresUnreferencedCode:

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Node/JsonValueOfT.cs

Lines 85 to 92 in f372f6b

	if (_value is JsonElement jsonElement)
	{
	jsonElement.WriteTo(writer);
	}
	else
	{
	JsonSerializer.Serialize(writer, _value, _value!.GetType(), options);
	}

But we can't just mark WriteTo as unsafe, because this method is called from the ToString():

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Node/JsonNode.To.cs

Lines 30 to 56 in f372f6b

	public override string ToString()
	{
	// Special case for string; don't quote it.
	if (this is JsonValue)
	{
	if (this is JsonValue<string> jsonString)
	{
	return jsonString.Value;
	}
	if (this is JsonValue<JsonElement> jsonElement &&
	jsonElement.Value.ValueKind == JsonValueKind.String)
	{
	return jsonElement.Value.GetString()!;
	}
	}
	using (var output = new PooledByteBufferWriter(JsonSerializerOptions.BufferSizeDefault))
	{
	using (var writer = new Utf8JsonWriter(output, new JsonWriterOptions { Indented = true }))
	{
	WriteTo(writer);
	}
	return JsonHelpers.Utf8GetString(output.WrittenMemory.ToArray());
	}
	}

Which we can't mark ToString() as RequiresUnreferencedCode, since that would mean the base Object.ToString() and every overridden ToString() method would need to be annotated the same.

To resolve this, I can see the following options:

1. Limit JsonValue<TValue> to only primitive types. Don't allow for custom defined types. This would allow us to suppress the warning, since primitive types can be serialized safely in a trimmed application.
2. We make constructing JsonValue<TValue> with a custom defined type as RequiresUnreferencedCode. This way, it lets callers who create JsonValue instances know that they are using a type that is not trim compatible. Then, we can specific APIs for primitive types, and also add an API to create a JsonValue<TValue> that takes a JsonTypeInfo to use when serializing the value. These will be considered "trim compatible", since it doesn't use the reflection code path. The only thing that will be RequiresUnreferencedCode is creating a JsonValue<TValue> with a custom type, without passing in a JsonTypeInfo. This mirrors the JsonSerializer APIs.

cc @steveharter @layomia

Author:	eerhardt
Assignees:	-
Labels:	area-System.Text.Json
Milestone:	-

Tagging subscribers to 'linkable-framework': @eerhardt, @vitek-karas, @LakshanF, @sbomer
See info in area-owners.md if you want to be subscribed.

Issue Details

---

In the current design, JsonValue<TValue> can take any TValue type, and it passes the value into JsonSerializer.Serialize. However, it doesn't allow for a source generated JsonTypeInfo to be passed into Serialize.

This is a problem because using JsonSerializer.Serialize without a JsonTypeInfo is not trim-compatible. This is because of the recursive use of Reflection that the trimming tool can't reason about.

In most cases, JsonValue is for primitive types, which are intrinsically handled by the serializer without needing reflection. However, because we don't limit the types that can be passed into JsonValue<T>, we need to mark the JsonValue<TValue>.WriteTo method as RequiresUnreferencedCode:

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Node/JsonValueOfT.cs

Lines 85 to 92 in f372f6b

	if (_value is JsonElement jsonElement)
	{
	jsonElement.WriteTo(writer);
	}
	else
	{
	JsonSerializer.Serialize(writer, _value, _value!.GetType(), options);
	}

But we can't just mark WriteTo as unsafe, because this method is called from the ToString():

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Node/JsonNode.To.cs

Lines 30 to 56 in f372f6b

	public override string ToString()
	{
	// Special case for string; don't quote it.
	if (this is JsonValue)
	{
	if (this is JsonValue<string> jsonString)
	{
	return jsonString.Value;
	}
	if (this is JsonValue<JsonElement> jsonElement &&
	jsonElement.Value.ValueKind == JsonValueKind.String)
	{
	return jsonElement.Value.GetString()!;
	}
	}
	using (var output = new PooledByteBufferWriter(JsonSerializerOptions.BufferSizeDefault))
	{
	using (var writer = new Utf8JsonWriter(output, new JsonWriterOptions { Indented = true }))
	{
	WriteTo(writer);
	}
	return JsonHelpers.Utf8GetString(output.WrittenMemory.ToArray());
	}
	}

Which we can't mark ToString() as RequiresUnreferencedCode, since that would mean the base Object.ToString() and every overridden ToString() method would need to be annotated the same.

To resolve this, I can see the following options:

1. Limit JsonValue<TValue> to only primitive types. Don't allow for custom defined types. This would allow us to suppress the warning, since primitive types can be serialized safely in a trimmed application.
2. We make constructing JsonValue<TValue> with a custom defined type as RequiresUnreferencedCode. This way, it lets callers who create JsonValue instances know that they are using a type that is not trim compatible. Then, we can specific APIs for primitive types, and also add an API to create a JsonValue<TValue> that takes a JsonTypeInfo to use when serializing the value. These will be considered "trim compatible", since it doesn't use the reflection code path. The only thing that will be RequiresUnreferencedCode is creating a JsonValue<TValue> with a custom type, without passing in a JsonTypeInfo. This mirrors the JsonSerializer APIs.

cc @steveharter @layomia

Author:	eerhardt
Assignees:	-
Labels:	area-System.Text.Json, linkable-framework, untriaged
Milestone:	-

[steveharter]

Option (2) sounds reasonable. Option (1) would prevent many documented scenarios from working including anonymous types, pre-existing POCO and collections instances and custom data types.