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

JSON source generator emitting source for types unexpectedly #108682

Open
stephentoub opened this issue Oct 8, 2024 · 7 comments
Open

JSON source generator emitting source for types unexpectedly #108682

stephentoub opened this issue Oct 8, 2024 · 7 comments
Assignees
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

@stephentoub
Copy link
Member

stephentoub commented Oct 8, 2024

Description

In the repo below, Exception should not be serialized as it's defined, but rather only via the ExceptionConverter that treats it as a string. However, the source generator is emitting code to handle all of the properties on Exception. While this is unnecessary code, it also results in trimmer warnings about members of Exception that aren't safe to serialized.

Reproduction Steps

using System.Text.Json;
using System.Text.Json.Serialization;

Console.WriteLine(JsonSerializer.Serialize(new MyClass { Error = new Exception("error") } ));

sealed class MyClass
{
    [JsonConverter(typeof(ExceptionConverter))]
    public Exception? Error { get; set; }
}

sealed class ExceptionConverter : JsonConverter<Exception>
{
    public override Exception Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) =>
        new Exception(reader.GetString());

    public override void Write(Utf8JsonWriter writer, Exception? value, JsonSerializerOptions options) =>
        writer.WriteStringValue(value?.Message ?? string.Empty);
}

[JsonSerializable(typeof(MyClass))]
partial class MyContext : JsonSerializerContext;

Expected behavior

Code emitted for Exception related to its converter.

Actual behavior

Emits this:

// <auto-generated/>

#nullable enable annotations
#nullable disable warnings

// Suppress warnings about [Obsolete] member usage in generated code.
#pragma warning disable CS0612, CS0618

partial class MyContext
{
    private global::System.Text.Json.Serialization.Metadata.JsonTypeInfo<global::System.Exception>? _Exception;
    
    /// <summary>
    /// Defines the source generated JSON serialization contract metadata for a given type.
    /// </summary>
    #nullable disable annotations // Marking the property type as nullable-oblivious.
    public global::System.Text.Json.Serialization.Metadata.JsonTypeInfo<global::System.Exception> Exception
    #nullable enable annotations
    {
        get => _Exception ??= (global::System.Text.Json.Serialization.Metadata.JsonTypeInfo<global::System.Exception>)Options.GetTypeInfo(typeof(global::System.Exception));
    }
    
    private global::System.Text.Json.Serialization.Metadata.JsonTypeInfo<global::System.Exception> Create_Exception(global::System.Text.Json.JsonSerializerOptions options)
    {
        if (!TryGetTypeInfoForRuntimeCustomConverter<global::System.Exception>(options, out global::System.Text.Json.Serialization.Metadata.JsonTypeInfo<global::System.Exception> jsonTypeInfo))
        {
            var objectInfo = new global::System.Text.Json.Serialization.Metadata.JsonObjectInfoValues<global::System.Exception>
            {
                ObjectCreator = () => new global::System.Exception(),
                ObjectWithParameterizedConstructorCreator = null,
                PropertyMetadataInitializer = _ => ExceptionPropInit(options),
                ConstructorParameterMetadataInitializer = null,
                ConstructorAttributeProviderFactory = static () => typeof(global::System.Exception).GetConstructor(InstanceMemberBindingFlags, binder: null, global::System.Array.Empty<global::System.Type>(), modifiers: null),
                SerializeHandler = ExceptionSerializeHandler,
            };
            
            jsonTypeInfo = global::System.Text.Json.Serialization.Metadata.JsonMetadataServices.CreateObjectInfo<global::System.Exception>(options, objectInfo);
            jsonTypeInfo.NumberHandling = null;
        }
    
        jsonTypeInfo.OriginatingResolver = this;
        return jsonTypeInfo;
    }

    private static global::System.Text.Json.Serialization.Metadata.JsonPropertyInfo[] ExceptionPropInit(global::System.Text.Json.JsonSerializerOptions options)
    {
        var properties = new global::System.Text.Json.Serialization.Metadata.JsonPropertyInfo[8];

        var info0 = new global::System.Text.Json.Serialization.Metadata.JsonPropertyInfoValues<global::System.Collections.IDictionary>
        {
            IsProperty = true,
            IsPublic = true,
            IsVirtual = true,
            DeclaringType = typeof(global::System.Exception),
            Converter = null,
            Getter = static obj => ((global::System.Exception)obj).Data,
            Setter = null,
            IgnoreCondition = null,
            HasJsonInclude = false,
            IsExtensionData = false,
            NumberHandling = null,
            PropertyName = "Data",
            JsonPropertyName = null,
            AttributeProviderFactory = static () => typeof(global::System.Exception).GetProperty("Data", InstanceMemberBindingFlags, null, typeof(global::System.Collections.IDictionary), global::System.Array.Empty<global::System.Type>(), null),
        };
        
        properties[0] = global::System.Text.Json.Serialization.Metadata.JsonMetadataServices.CreatePropertyInfo<global::System.Collections.IDictionary>(options, info0);
        properties[0].IsGetNullable = false;

        var info1 = new global::System.Text.Json.Serialization.Metadata.JsonPropertyInfoValues<string>
        {
            IsProperty = true,
            IsPublic = true,
            IsVirtual = true,
            DeclaringType = typeof(global::System.Exception),
            Converter = null,
            Getter = static obj => ((global::System.Exception)obj).HelpLink,
            Setter = static (obj, value) => ((global::System.Exception)obj).HelpLink = value!,
            IgnoreCondition = null,
            HasJsonInclude = false,
            IsExtensionData = false,
            NumberHandling = null,
            PropertyName = "HelpLink",
            JsonPropertyName = null,
            AttributeProviderFactory = static () => typeof(global::System.Exception).GetProperty("HelpLink", InstanceMemberBindingFlags, null, typeof(string), global::System.Array.Empty<global::System.Type>(), null),
        };
        
        properties[1] = global::System.Text.Json.Serialization.Metadata.JsonMetadataServices.CreatePropertyInfo<string>(options, info1);

        var info2 = new global::System.Text.Json.Serialization.Metadata.JsonPropertyInfoValues<int>
        {
            IsProperty = true,
            IsPublic = true,
            IsVirtual = false,
            DeclaringType = typeof(global::System.Exception),
            Converter = null,
            Getter = static obj => ((global::System.Exception)obj).HResult,
            Setter = static (obj, value) => ((global::System.Exception)obj).HResult = value!,
            IgnoreCondition = null,
            HasJsonInclude = false,
            IsExtensionData = false,
            NumberHandling = null,
            PropertyName = "HResult",
            JsonPropertyName = null,
            AttributeProviderFactory = static () => typeof(global::System.Exception).GetProperty("HResult", InstanceMemberBindingFlags, null, typeof(int), global::System.Array.Empty<global::System.Type>(), null),
        };
        
        properties[2] = global::System.Text.Json.Serialization.Metadata.JsonMetadataServices.CreatePropertyInfo<int>(options, info2);

        var info3 = new global::System.Text.Json.Serialization.Metadata.JsonPropertyInfoValues<global::System.Exception>
        {
            IsProperty = true,
            IsPublic = true,
            IsVirtual = false,
            DeclaringType = typeof(global::System.Exception),
            Converter = null,
            Getter = static obj => ((global::System.Exception)obj).InnerException,
            Setter = null,
            IgnoreCondition = null,
            HasJsonInclude = false,
            IsExtensionData = false,
            NumberHandling = null,
            PropertyName = "InnerException",
            JsonPropertyName = null,
            AttributeProviderFactory = static () => typeof(global::System.Exception).GetProperty("InnerException", InstanceMemberBindingFlags, null, typeof(global::System.Exception), global::System.Array.Empty<global::System.Type>(), null),
        };
        
        properties[3] = global::System.Text.Json.Serialization.Metadata.JsonMetadataServices.CreatePropertyInfo<global::System.Exception>(options, info3);

        var info4 = new global::System.Text.Json.Serialization.Metadata.JsonPropertyInfoValues<string>
        {
            IsProperty = true,
            IsPublic = true,
            IsVirtual = true,
            DeclaringType = typeof(global::System.Exception),
            Converter = null,
            Getter = static obj => ((global::System.Exception)obj).Message,
            Setter = null,
            IgnoreCondition = null,
            HasJsonInclude = false,
            IsExtensionData = false,
            NumberHandling = null,
            PropertyName = "Message",
            JsonPropertyName = null,
            AttributeProviderFactory = static () => typeof(global::System.Exception).GetProperty("Message", InstanceMemberBindingFlags, null, typeof(string), global::System.Array.Empty<global::System.Type>(), null),
        };
        
        properties[4] = global::System.Text.Json.Serialization.Metadata.JsonMetadataServices.CreatePropertyInfo<string>(options, info4);
        properties[4].IsGetNullable = false;

        var info5 = new global::System.Text.Json.Serialization.Metadata.JsonPropertyInfoValues<string>
        {
            IsProperty = true,
            IsPublic = true,
            IsVirtual = true,
            DeclaringType = typeof(global::System.Exception),
            Converter = null,
            Getter = static obj => ((global::System.Exception)obj).Source,
            Setter = static (obj, value) => ((global::System.Exception)obj).Source = value!,
            IgnoreCondition = null,
            HasJsonInclude = false,
            IsExtensionData = false,
            NumberHandling = null,
            PropertyName = "Source",
            JsonPropertyName = null,
            AttributeProviderFactory = static () => typeof(global::System.Exception).GetProperty("Source", InstanceMemberBindingFlags, null, typeof(string), global::System.Array.Empty<global::System.Type>(), null),
        };
        
        properties[5] = global::System.Text.Json.Serialization.Metadata.JsonMetadataServices.CreatePropertyInfo<string>(options, info5);

        var info6 = new global::System.Text.Json.Serialization.Metadata.JsonPropertyInfoValues<string>
        {
            IsProperty = true,
            IsPublic = true,
            IsVirtual = true,
            DeclaringType = typeof(global::System.Exception),
            Converter = null,
            Getter = static obj => ((global::System.Exception)obj).StackTrace,
            Setter = null,
            IgnoreCondition = null,
            HasJsonInclude = false,
            IsExtensionData = false,
            NumberHandling = null,
            PropertyName = "StackTrace",
            JsonPropertyName = null,
            AttributeProviderFactory = static () => typeof(global::System.Exception).GetProperty("StackTrace", InstanceMemberBindingFlags, null, typeof(string), global::System.Array.Empty<global::System.Type>(), null),
        };
        
        properties[6] = global::System.Text.Json.Serialization.Metadata.JsonMetadataServices.CreatePropertyInfo<string>(options, info6);

        var info7 = new global::System.Text.Json.Serialization.Metadata.JsonPropertyInfoValues<global::System.Reflection.MethodBase>
        {
            IsProperty = true,
            IsPublic = true,
            IsVirtual = false,
            DeclaringType = typeof(global::System.Exception),
            Converter = null,
            Getter = static obj => ((global::System.Exception)obj).TargetSite,
            Setter = null,
            IgnoreCondition = null,
            HasJsonInclude = false,
            IsExtensionData = false,
            NumberHandling = null,
            PropertyName = "TargetSite",
            JsonPropertyName = null,
            AttributeProviderFactory = static () => typeof(global::System.Exception).GetProperty("TargetSite", InstanceMemberBindingFlags, null, typeof(global::System.Reflection.MethodBase), global::System.Array.Empty<global::System.Type>(), null),
        };
        
        properties[7] = global::System.Text.Json.Serialization.Metadata.JsonMetadataServices.CreatePropertyInfo<global::System.Reflection.MethodBase>(options, info7);

        return properties;
    }

    // Intentionally not a static method because we create a delegate to it. Invoking delegates to instance
    // methods is almost as fast as virtual calls. Static methods need to go through a shuffle thunk.
    private void ExceptionSerializeHandler(global::System.Text.Json.Utf8JsonWriter writer, global::System.Exception? value)
    {
        if (value is null)
        {
            writer.WriteNullValue();
            return;
        }
        
        writer.WriteStartObject();

        writer.WritePropertyName(PropName_Data);
        global::System.Text.Json.JsonSerializer.Serialize(writer, ((global::System.Exception)value).Data, IDictionary);
        writer.WriteString(PropName_HelpLink, ((global::System.Exception)value).HelpLink);
        writer.WriteNumber(PropName_HResult, ((global::System.Exception)value).HResult);
        writer.WritePropertyName(PropName_InnerException);
        ExceptionSerializeHandler(writer, ((global::System.Exception)value).InnerException);
        writer.WriteString(PropName_Message, ((global::System.Exception)value).Message);
        writer.WriteString(PropName_Source, ((global::System.Exception)value).Source);
        writer.WriteString(PropName_StackTrace, ((global::System.Exception)value).StackTrace);
        writer.WritePropertyName(PropName_TargetSite);
        global::System.Text.Json.JsonSerializer.Serialize(writer, ((global::System.Exception)value).TargetSite, MethodBase);

        writer.WriteEndObject();
    }
}

Regression?

n/a

Known Workarounds

n/a

Configuration

.NET 9

Other information

No response

Copy link
Contributor

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

@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Oct 8, 2024
@eiriktsarpalis eiriktsarpalis self-assigned this Oct 8, 2024
@eiriktsarpalis eiriktsarpalis added bug source-generator Indicates an issue with a source generator feature and removed untriaged New issue has not been triaged by the area owner labels Oct 8, 2024
@eiriktsarpalis eiriktsarpalis added this to the 9.0.0 milestone Oct 8, 2024
@eiriktsarpalis eiriktsarpalis removed the bug label Oct 8, 2024
@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Oct 8, 2024

This happens because the source generator will attempt to source generate for every type that is found within the transitive closure of JsonSerializable types, even if those types are only defined on properties that define custom converters:

// A JsonTypeInfo<Bar> has been generated even if it isn't being used by Foo
Console.WriteLine(JsonSerializer.Serialize(new Bar("Value"), MyContext.Default.Bar)); // {"Value":"Value"}

class Foo
{
    [JsonConverter(typeof(CustomBarConverter))]
    public Bar? PropertyWithCustomConverter { get; set; }

    // Serializes Bar as a plain old string
    public class CustomBarConverter : JsonConverter<Bar>
    {
        public override Bar Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
            => new Bar(reader.GetString()!);

        public override void Write(Utf8JsonWriter writer, Bar value, JsonSerializerOptions options) =>
            writer.WriteStringValue(value.Value);
    }
}

record Bar(string Value);

[JsonSerializable(typeof(Foo))]
partial class MyContext : JsonSerializerContext;

I don't think we could change this today without incurring breaking changes.

@eiriktsarpalis eiriktsarpalis modified the milestones: 9.0.0, Future Oct 9, 2024
@eiriktsarpalis eiriktsarpalis added the enhancement Product code improvement that does NOT require public API changes/additions label Oct 9, 2024
@stephentoub
Copy link
Member Author

How can trimmer warnings be suppressed just in the generated files? Should the source generator be suppressing them with pragmas? Otherwise, it seems you need to suppress them project-wide.

@eiriktsarpalis
Copy link
Member

Pragma suppressions won't prevent the linker from emitting warnings when it encounters unsafe APIs. The generated code must be annotated with attributes.

@stephentoub
Copy link
Member Author

Then what do you recommend? You moved this to future, but that's effectively saying such warnings need to be suppressed at the project level, which then defeats the purpose of such warnings and makes it near impossible to guarantee linker safety.

@eiriktsarpalis
Copy link
Member

but that's effectively saying such warnings need to be suppressed at the project level

I don't believe either pragma suppressions or project level suppressions at the library will prevent the same warnings from being surfaced when running dotnet publish. Suppressions must be made using attributes. @vitek-karas should be able to confirm this.

Then what do you recommend?

I can't think of anything that doesn't involve introducing new API. An ad hoc fix would involve just disabling generation for exception -- the code that it does generate today by default won't work since it has a number of already unsupported property types.

@vitek-karas
Copy link
Member

Pragma suppressions only work on analyzer. That said, if the generated code is never used, then trimmer or AOT will not produce warnings on it since they'll get rid of it first. So if the discussion is only around the case where the source generator produces code which is never used, pragma suppressions should be enough.

There's nothing really like "project level suppressions" - even if you suppress them via NoWarn, if it's in the library, the final app will show them again (because trimmer runs on all IL globally). What is in theory possible is injecting suppression attributes via XML trimmer files, but that's not something we want to do if at all possible.

You can suppress the warnings on the app level via NoWarn, just like any other warning, and that will work for all sources (analyzer, trimmer, AOT).

@sbomer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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