Should we remove DynamicallyAccessedMembers attributes on JsonSerializer?

[eerhardt]

In 5.0, we annotated the JsonSerializer APIs with DynamicallyAccessedMembers attributes in #38595.

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.String.cs

Lines 43 to 44 in 300c39d

	[RequiresUnreferencedCode(SerializationUnreferencedCodeMessage)]
	public static TValue? Deserialize<[DynamicallyAccessedMembers(JsonHelpers.MembersAccessedOnRead)] TValue>(string json, JsonSerializerOptions? options = null)

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.String.cs

Lines 26 to 27 in 300c39d

	[RequiresUnreferencedCode(SerializationUnreferencedCodeMessage)]
	public static string Serialize<[DynamicallyAccessedMembers(MembersAccessedOnWrite)] TValue>(TValue value, JsonSerializerOptions? options = null)

In 6.0, we resolved all the ILLink warnings in System.Text.Json in #51886, which caused us to also add the RequiresUnreferencedCode attributes. This is because the trim annotations can't model everything that needs to be preserved to make these APIs trim compatible. For example, see dotnet/linker#1087.

Having both sets of attributes on these APIs isn't really ideal. The DynamicallyAccessedMembers attributes will only preserve members on the top-level type. So having the DynamicallyAccessedMembers gives a false sense of hope that this API will work with trimming, when in reality it won't.

The small benefit it does provide is in the case where the user is not serializing a graph, but just a class with primitive properties. However, even in this case, we'd like the user to move to use the source generator instead. So keeping these attributes for this tiny case doesn't seem to outweigh the benefit of the encouragement to move to using the source generator.

We also didn't annotate the DataContractSerializer and XmlSerializer APIs like this. We just marked the unsafe APIs with RequiresUnreferencedCode.

However, if we have plans for making the non-source generation Json serializer APIs work with trimming in 6.0, it might not be worth the effort of removing these attributes now, just to add them back in the near future.

cc @vitek-karas @agocke @layomia @steveharter

[ghost]

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

Issue Details

---

In 5.0, we annotated the JsonSerializer APIs with DynamicallyAccessedMembers attributes in #38595.

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.String.cs

Lines 43 to 44 in 300c39d

	[RequiresUnreferencedCode(SerializationUnreferencedCodeMessage)]
	public static TValue? Deserialize<[DynamicallyAccessedMembers(JsonHelpers.MembersAccessedOnRead)] TValue>(string json, JsonSerializerOptions? options = null)

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.String.cs

Lines 26 to 27 in 300c39d

	[RequiresUnreferencedCode(SerializationUnreferencedCodeMessage)]
	public static string Serialize<[DynamicallyAccessedMembers(MembersAccessedOnWrite)] TValue>(TValue value, JsonSerializerOptions? options = null)

In 6.0, we resolved all the ILLink warnings in System.Text.Json in #51886, which caused us to also add the RequiresUnreferencedCode attributes. This is because the trim annotations can't model everything that needs to be preserved to make these APIs trim compatible. For example, see dotnet/linker#1087.

Having both sets of attributes on these APIs isn't really ideal. The DynamicallyAccessedMembers attributes will only preserve members on the top-level type. So having the DynamicallyAccessedMembers gives a false sense of hope that this API will work with trimming, when in reality it won't.

The small benefit it does provide is in the case where the user is not serializing a graph, but just a class with primitive properties. However, even in this case, we'd like the user to move to use the source generator instead. So keeping these attributes for this tiny case doesn't seem to outweigh the benefit of the encouragement to move to using the source generator.

We also didn't annotate the DataContractSerializer and XmlSerializer APIs like this. We just marked the unsafe APIs with RequiresUnreferencedCode.

However, if we have plans for making the non-source generation Json serializer APIs work with trimming in 6.0, it might not be worth the effort of removing these attributes now, just to add them back in the near future.

cc @vitek-karas @agocke @layomia @steveharter

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

[vitek-karas]

I think we should remove them - as mentioned they are not enough and won't help in most cases anyway.

However, if we have plans for making the non-source generation Json serializer APIs work with trimming in 6.0, it might not be worth the effort of removing these attributes now, just to add them back in the near future.

If we do come up with a solution, I doubt it will be using the exact same annotations, so we would have to change this anyway. I don't mind revisiting this code again, if we have a reason to do so.

[eerhardt]

Just an FYI - we have trimming tests for these attributes:

runtime/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/TrimmingTests/SerializerEntryPoint/SerializeAsync.ToStream.BoxedObject.cs

Lines 31 to 39 in c9c954b

	using (var stream = new MemoryStream())
	{
	MyStruct obj = default;
	await JsonSerializer.SerializeAsync(stream, obj, typeof(MyStruct));
	string actual = Encoding.UTF8.GetString(stream.ToArray());
	if (!TestHelper.JsonEqual(@"{""X"":0,""Y"":0}", actual))
	{
	return -1;
	}

I assume as part of this work, we would be deleting those tests as well.

[eerhardt]

@steveharter @layomia - I plan on working on this. Just ensuring you don't have any objections to removing these attributes.

[davidfowl]

Didn't we decide that if we had a special [DAM] we could propagate these from user code to preserve type the call sites?

[eerhardt]

Yes, when the linker supports making a custom [DAM], then we can mark these methods with that. But we don’t have that feature yet.