The JsonSerializerOptions.TypeInfoResolver property should be nullable

[eiriktsarpalis]

The current iteration of JsonSerializerOptions.TypeInfoResolver

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs

Lines 168 to 181 in 52d9d0c

	public IJsonTypeInfoResolver TypeInfoResolver
	{
	[RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)]
	[RequiresDynamicCode(JsonSerializer.SerializationRequiresDynamicCodeMessage)]
	get
	{
	return _typeInfoResolver ??= DefaultJsonTypeInfoResolver.RootDefaultInstance();
	}
	set
	{
	VerifyMutable();
	_typeInfoResolver = value;
	}
	}

is implemented as non-nullable and marked as RequiredUnreferencedCode since by default it will return the reflection-based contract resolver. This has been informed by the existing semantics of the JsonSerializer methods accepting JsonSerializerOptions as an argument: unless a contract resolver is explicitly set the method will revert to using reflection-based serialization.

I believe however that this approach is problematic, for a number of reasons:

• Because the property is marked RUC, it can trigger linker warnings in otherwise innocuous operations, such as copying resolver configuration to a new options instance.
• The introduction of contract customization has effectively decoupled reflection-based serialization from JsonSerializerOptions. While we cannot change the semantics of existing JsonSerializer methods it is conceivable that future serialization APIs can make use of it in a linker-safe manner. Assuming we ship the TypeInfoResolver property in the current manifestation, we forever couple JsonSerializerOptions to the reflection-based serialization and trimmability concerns.

I propose we make the following changes to the API:

public partial class JsonSerializerOptions
{
-       [RequiresUnreferenceCode]
-       [RequiresDynamicCode]
-       public IJsonTypeInfoResolver TypeInfoResolver { get; set; }
+       public IJsonTypeInfoResolver? TypeInfoResolver { get; set; }

+       [RequiresUnreferenceCode]
+       [RequiresDynamicCode]
        public static JsonSerializerOptions Default { get; } // default instance comes populated with reflection resolver
}

[ghost]

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis
See info in area-owners.md if you want to be subscribed.

Issue Details

---

The current iteration of JsonSerializerOptions.TypeInfoResolver

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs

Lines 168 to 181 in 52d9d0c

	public IJsonTypeInfoResolver TypeInfoResolver
	{
	[RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)]
	[RequiresDynamicCode(JsonSerializer.SerializationRequiresDynamicCodeMessage)]
	get
	{
	return _typeInfoResolver ??= DefaultJsonTypeInfoResolver.RootDefaultInstance();
	}
	set
	{
	VerifyMutable();
	_typeInfoResolver = value;
	}
	}

is implemented as non-nullable and marked as RequiredUnreferencedCode since by default it will return the reflection-based contract resolver. This has been informed by the existing semantics of the JsonSerializer methods accepting JsonSerializerOptions as an argument: unless a contract resolver is explicitly set the method will revert to using reflection-based serialization.

I believe however that this approach is problematic, for a number of reasons:

• Because the property is marked RUC, it can trigger linker warnings in otherwise innocuous operations, such as copying resolver configuration to a new options instance.
• The introduction of contract customization has effectively decoupled reflection-based serialization from JsonSerializerOptions. While we cannot change the semantics of existing JsonSerializer methods it is conceivable that future serialization APIs can make use of it in a linker-safe manner. Assuming we ship the TypeInfoResolver property in the current manifestation, we forever couple JsonSerializerOptions to the reflection-based serialization and trimmability concerns.

I propose we make the following changes to the API:

public partial class JsonSerializerOptions
{
-       [RequiresUnreferenceCode]
-       [RequiresDynamicCode]
-       public IJsonTypeInfoResolver TypeInfoResolver { get; set; }
+       public IJsonTypeInfoResolver? TypeInfoResolver { get; set; }

+       [RequiresUnreferenceCode]
+       [RequiresDynamicCode]
        public static JsonSerializerOptions Default { get; } // default instance comes populated with reflection resolver
}

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

[krwq]

IMO following behavior which is proposed here feels like designing APIs around linker. In both cases null gets translated tot he same instance the only difference is that in this proposal it happens in some seemingly magic moment in time (we as devs know that moment but I doubt trying to explain that to anyone will make sense why we would've picked that behavior)

// proposed:
options.TypeInfoResolver = null;
// options.TypeInfoResolver == null
JsonSerializer(..., options);
// options.TypeInfoResolver.GetType()  == typeof(DefaultJsonTypeInfoResolver)

current behavior:

// current behavior:
options.TypeInfoResolver = null;
// options.TypeInfoResolver.GetType()  == typeof(DefaultJsonTypeInfoResolver) 
JsonSerializer(..., options);
// options.TypeInfoResolver.GetType()  == typeof(DefaultJsonTypeInfoResolver) 

it just feels more consistent the way it is right now

[eiriktsarpalis]

cc @jkotas @eerhardt who might have opinions on the matter.

I don't think "designing APIs around the linker" is the right way to look at this. It is about designing a type that works for scenaria beyond the narrow scope of reflection-based serialization. If we do ship JsonSerializerOptions with the current semantics/annotation, the type will be forever coupled to reflection-based serialization: in effect it will be impossible to ship serialization APIs accepting JsonSerializerOptions parameters in the future that do not need to be marked "RequiresUnreferenceCode". I think this is a problem, particularly given the fact that we expect source generators to be the future of serialization in .NET.

For the record, I don't believe reflection-based JsonSerializer methods populating null TypeInfoResolvers to be an important issue. This is in line with System.Text.Json semantics since the APIs got shipped originally (these methods also lock the options values so a degree of mutation is not entirely unexpected). We need to document this, of course, but I doubt this might be considered a source of confusion.

[jkotas]

@eiriktsarpalis 's proposal makes sense to me.