Skip to content
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

STJ Source Generator fails to do polymorphic serialization for built-in types #58134

Closed
Tracked by #77019
aromaa opened this issue Aug 25, 2021 · 9 comments
Closed
Tracked by #77019
Labels
area-System.Text.Json enhancement Product code improvement that does NOT require public API changes/additions source-generator Indicates an issue with a source generator feature
Milestone

Comments

@aromaa
Copy link
Contributor

aromaa commented Aug 25, 2021

Description

The System.Text.Json source generator fails to serialize list of objects (List<object>) that contains only built-in types like string. This can be worked around by adding the types to the JsonSerializerContext with the JsonSerializable attribute but this feels counterintuitive and doesn't look great and bloats the class. This scenario works just fine without source generators.

Repo

internal static class Program
{
    private static void Main()
    {
        JsonSerializer.Serialize(new List<object>
        {
            "hey"
        }, typeof(List<object>), JsonContext.Default);
    }
}

[JsonSerializable(typeof(List<object>))]
[JsonSerializable(typeof(string))] //Commenting this line breaks the program
internal sealed partial class JsonContext : JsonSerializerContext
{
}
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Text.Json untriaged New issue has not been triaged by the area owner labels Aug 25, 2021
@ghost
Copy link

ghost commented Aug 25, 2021

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

Issue Details

Description

The System.Text.Json source generator fails to serialize list of objects (List<object>) that contains only built-in types like string. This can be worked around by adding the types to the JsonSerializerContext with the JsonSerializable attribute but this feels counterintuitive and doesn't look great and bloats the class. This scenario works just fine without source generators.

Repo

internal static class Program
{
    private static void Main()
    {
        JsonSerializer.Serialize(new List<object>
        {
            "hey"
        }, typeof(List<object>), JsonContext.Default);
    }
}

[JsonSerializable(typeof(List<object>))]
[JsonSerializable(typeof(string))] //Commenting this line breaks the program
internal sealed partial class JsonContext : JsonSerializerContext
{
}
Author: aromaa
Assignees: -
Labels:

area-System.Text.Json, untriaged

Milestone: -

@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Aug 26, 2021

Agreed that the user experience could be improved here, however the error message:

System.NotSupportedException: Metadata for type 'System.Int32' was not provided to the serializer. The serializer method used does not support reflection-based creation of serialization-related type metadata. If using source generation, ensure that all root types passed to the serializer have been indicated with 'JsonSerializableAttribute', along with any types that might be serialized polymorphically. The unsupported member type is located on type 'System.Object'. Path: $.

indicates that this is clearly by design. Given that we have a workaround, recommend moving this to 7.0.0. cc @layomia @steveharter

@eiriktsarpalis eiriktsarpalis added enhancement Product code improvement that does NOT require public API changes/additions and removed untriaged New issue has not been triaged by the area owner labels Aug 26, 2021
@eiriktsarpalis eiriktsarpalis added this to the 7.0.0 milestone Aug 26, 2021
@eiriktsarpalis
Copy link
Member

Related to #30083.

@layomia
Copy link
Contributor

layomia commented Sep 20, 2021

Yes it is by design - for polymorphic serialization scenarios, we need to know all possible runtime types ahead of time via JsonSerializableAttribute.

What sort of improved user experience would we like to see here - a diagnostic warning when compiling and object is found as a top-level type for source generation? I think that could be a nuisance in the case where a user has already provided the runtime type.

@aromaa
Copy link
Contributor Author

aromaa commented Sep 21, 2021

It feels unintuitive to need to add the basic primitives and commonly used built-in types with the JsonSerializableAttribute that support serialization out of the box.

It would make sense that these are pre-included or that theres a switch to generate them without the need to manually include them all (which there are dozens of)

@layomia layomia added the source-generator Indicates an issue with a source generator feature label Sep 22, 2021
@eiriktsarpalis
Copy link
Member

It feels unintuitive to need to add the basic primitives and commonly used built-in types with the JsonSerializableAttribute that support serialization out of the box.

I guess it depends on what "commonly used built-in types" means. We might consider adding common primitives and string OOTB but want to avoid rooting all of BCL by baking in support for things like DateTimeOffset or Guid. It is also impossible to do it for user-defined POCOs or generic types like Dictionary.

It makes me think that users will encounter this issue one way or another, so there doesn't seem to be a lot of value in baking in support for what might amount to 20% of commonly used runtime types for object.

@aromaa
Copy link
Contributor Author

aromaa commented Apr 27, 2022

I guess it depends on what "commonly used built-in types" means. We might consider adding common primitives and string OOTB but want to avoid rooting all of BCL

Considering the rooting problem, I think it would relieve the pain point mostly by just including the primitives and string. It would be nice if there was a trimming attribute to mark things as "weak" where they need to be rooted elsewhere to not get trimmed.

It is also impossible to do it for user-defined POCOs or generic types like Dictionary.

I'm not sure I follow? As long as the underlying type for object is found from the GetTypeInfo it will generate JSON just fine for types like List<object>. What I'm asking is to fallback to some BCL code to fill in the blanks for primitives or just include them by default in the generator.

It makes me think that users will encounter this issue one way or another, so there doesn't seem to be a lot of value in baking in support for what might amount to 20% of commonly used runtime types for object.

The primitives and string are the most common types of them all so including at least them seems to make sense to me. If we end up needing to explicitly specify types like DateTimeOffset or Guid it seems okay-ish to me but having to do it for primitives doesn't feel right to me.

@eiriktsarpalis
Copy link
Member

We won't have time to work on this for .NET 7, moving to Future.

@layomia
Copy link
Contributor

layomia commented Nov 8, 2022

Closing as by-design: implicit handling by the source generator is avoided. It is beneficial to pay attention to polymorphism occurring in serialization routines.

Including primitives by default might have an impact on size-related issues - #77897.

There's a tracking issue to consider issuing a diagnostic warning when typeof(object) is encountered in the generator's input type graph - #53935.

@layomia layomia closed this as completed Nov 8, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Text.Json enhancement Product code improvement that does NOT require public API changes/additions source-generator Indicates an issue with a source generator feature
Projects
None yet
Development

No branches or pull requests

3 participants