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

Add JsonIncludeAttribute/JsonConstructorAttribute support for private/inaccessible members in the source generator #88519

Open
eiriktsarpalis opened this issue Jul 7, 2023 · 8 comments
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

@eiriktsarpalis
Copy link
Member

The change in #88452 added internal/private member support for JsonIncludeAttribute/JsonConstructorAttribute in the reflection-based serializer. We should consider extending this to the source generator via the recent feature introduced in #81741. Note that this would only work for the latest TFMs (with some reflection-based fallback being necessary in older targets).

cc @jkotas

@ghost
Copy link

ghost commented Jul 7, 2023

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 change in #88452 added internal/private member support for JsonIncludeAttribute/JsonConstructorAttribute in the reflection-based serializer. We should consider extending this to the source generator via the recent feature introduced in #81741. Note that this would only work for the latest TFMs (with some reflection-based fallback being necessary in older targets).

cc @jkotas

Author: eiriktsarpalis
Assignees: -
Labels:

area-System.Text.Json

Milestone: 9.0.0

@eiriktsarpalis eiriktsarpalis added enhancement Product code improvement that does NOT require public API changes/additions source-generator Indicates an issue with a source generator feature labels Jul 7, 2023
@jkotas
Copy link
Member

jkotas commented Jul 7, 2023

with some reflection-based fallback being necessary in older targets

We should simply not support this feature with source generators for older targets.

Reflection-based callback in source generators for older targets would violate promise of System.Text.Json source generator producing fast code that is trimming and AOT friendly.

@ceztko
Copy link

ceztko commented May 10, 2024

I'm happy to see this enhancement is officially endorsed by the team, I hope to see it for .NET9/10. While studying this limitation, I made a strange discovery and found out that putting a serializer context partial class within the class to be serialized (see the snippet below) allows the source generator to produce code that is indeed accessing the private fields, but still the compilation produces warnings and the serialization throws at runtime. I don't know if this can cause some concern today, but I am sure it will be properly replaced with the visibility suppression mechanism in the future.

    public partial class Vehicle
    {
        [JsonInclude, JsonPropertyName(nameof(Brand))]
        private string? _Brand;

        [JsonIgnore]
        public string? Brand => _Brand;

        [JsonSerializable(typeof(Vehicle))]
        internal partial class VehicleSerializerContext : JsonSerializerContext;
    }

    /* The following code is produced by the generator, and obviously compiles correctly
     * Still it has the warning "warning SYSLIB1038: The member 'Vehicle._Model' has been
     *  annotated with the JsonIncludeAttribute but is not visible to the source generator"
     * and throws at runtime
     * 
     *  Getter = static obj => ((global::ConsoleApp1.Vehicle)obj)._Model,
     *  Setter = static (obj, value) => ((global::ConsoleApp1.Vehicle)obj)._Model = value!,
     */

@eiriktsarpalis
Copy link
Member Author

While studying this limitation, I made a strange discovery and found out that putting a serializer context partial class within the class to be serialized (see the snippet below) allows the source generator to produce code that is indeed accessing the private fields.

This is expected behavior. It's not that (opted in) private members aren't supported in general, the more accurate statement is that members inaccessible to the generated class are not supported. Per C# semantics private members are accessible to nested types, so in this (relatively rare) case the source generator happens to work.

but still the compilation produces warnings and the serialization throws at runtime.

I can't reproduce in .NET 8. Is the project using multi-targeting perhaps?

@ceztko
Copy link

ceztko commented May 10, 2024

I can't reproduce in .NET 8. Is the project using multi-targeting perhaps?

No. I can reproduce in .NET 8 (I'm using latest stable VS 17.9.6) with the following steps:

  • Create an empty console application;
  • Paste the code below and run (produces warnings and throws the exception reported below).
  • (Workaround step): As per the documentation of the warning (and the System.Text.Json documentation in general about source generators), making all the private fields in the code internals, compile and run will have everything run just fine. But you just told me private members in some contexts can be supported and that you can't reproduce...
using System.Text.Json;
using System.Text.Json.Serialization;

namespace ConsoleApp1
{
    [JsonDerivedType(typeof(MotorVehicle), "MotorVehicle")]
    [JsonDerivedType(typeof(Car), "Car")]
    public partial class Vehicle
    {
        [JsonInclude, JsonPropertyName(nameof(Model))]
        private string? _Model;

        [JsonIgnore]
        public string? Model => _Model;

        [JsonSerializable(typeof(Vehicle))]
        internal partial class VehicleSerializerContext : JsonSerializerContext;
    }

    public partial class MotorVehicle : Vehicle
    {
        [JsonInclude]
        [JsonPropertyName(nameof(TransmissionType))]
        [JsonConverter(typeof(JsonStringEnumConverter<TransmissionType>))]
        private TransmissionType _TransmissionType;

        [JsonIgnore]
        public TransmissionType TransmissionType => _TransmissionType;

        [JsonSerializable(typeof(MotorVehicle))]
        internal partial class MotorVehicleSerializerContext : JsonSerializerContext;
    }

    public partial class Car : MotorVehicle
    {
        [JsonInclude]
        [JsonPropertyName(nameof(CarClassification))]
        [JsonConverter(typeof(JsonStringEnumConverter<CarClassification>))]
        private CarClassification _CarClassification;

        [JsonIgnore]
        public CarClassification CarClassification => _CarClassification;

        [JsonSerializable(typeof(Car))]
        internal partial class CarSerializerContext : JsonSerializerContext;
    }

    public enum CarClassification
    {
        Unknown = 0,
        Small,
        Medium,
        Large,
    }

    public enum TransmissionType
    {
        Unknown = 0,
        Manual,
        Automatic
    }

    class Program
    {
        public static int Main(string[] _)
        {
            var json = JsonSerializer.Serialize(new Car(), Vehicle.VehicleSerializerContext.Default.Vehicle);
            Console.WriteLine(json);

            var result = JsonSerializer.Deserialize(json, Vehicle.VehicleSerializerContext.Default.Vehicle);
            Console.WriteLine($"result is {result!.GetType()}");

            return 0;
        }
    }
}

The compilation produces these warnings:

warning SYSLIB1038: The member 'MotorVehicle._TransmissionType' has been annotated with the JsonIncludeAttribute but is not visible to the source generator. (https://learn.microsoft.com/dotnet/fundamentals/syslib-diagnostics/syslib1038)
warning SYSLIB1038: The member 'Car._CarClassification' has been annotated with the JsonIncludeAttribute but is not visible to the source generator. (https://learn.microsoft.com/dotnet/fundamentals/syslib-diagnostics/syslib1038)
warning SYSLIB1038: The member 'Vehicle._Model' has been annotated with the JsonIncludeAttribute but is not visible to the source generator. (https://learn.microsoft.com/dotnet/fundamentals/syslib-diagnostics/syslib1038)

And the exception is:

Unhandled exception. System.InvalidOperationException: The property '_CarClassification' on type 'ConsoleApp1.Car' which is annotated with 'JsonIncludeAttribute' is not accesible by the source generator.
   at System.Text.Json.ThrowHelper.ThrowInvalidOperationException_JsonIncludeOnInaccessibleProperty(String memberName, Type declaringType)
   at System.Text.Json.Serialization.Metadata.JsonMetadataServices.PopulateProperties(JsonTypeInfo typeInfo, JsonPropertyInfoList propertyList, Func`2 propInitFunc)
   at System.Text.Json.Serialization.Metadata.JsonTypeInfo.<get_PropertyList>g__CreatePropertyList|65_0()
   at System.Text.Json.Serialization.Metadata.JsonTypeInfo.ConfigureProperties()
   at System.Text.Json.Serialization.Metadata.JsonTypeInfo.Configure()
   at System.Text.Json.Serialization.Metadata.JsonTypeInfo.<EnsureConfigured>g__ConfigureSynchronized|172_0()
   at System.Text.Json.JsonSerializerOptions.GetTypeInfoInternal(Type type, Boolean ensureConfigured, Nullable`1 ensureNotNull, Boolean resolveIfMutable, Boolean fallBackToNearestAncestorType)
   at System.Text.Json.Serialization.Metadata.PolymorphicTypeResolver.DerivedJsonTypeInfo.GetJsonTypeInfo(JsonSerializerOptions options)
   at System.Text.Json.Serialization.Metadata.PolymorphicTypeResolver.TryGetDerivedJsonTypeInfo(Type runtimeType, JsonTypeInfo& jsonTypeInfo, Object& typeDiscriminator)
   at System.Text.Json.Serialization.JsonConverter.ResolvePolymorphicConverter(Object value, JsonTypeInfo jsonTypeInfo, JsonSerializerOptions options, WriteStack& state)
   at System.Text.Json.Serialization.JsonConverter`1.TryWrite(Utf8JsonWriter writer, T& value, JsonSerializerOptions options, WriteStack& state)
   at System.Text.Json.Serialization.JsonConverter`1.WriteCore(Utf8JsonWriter writer, T& value, JsonSerializerOptions options, WriteStack& state)
   at System.Text.Json.Serialization.Metadata.JsonTypeInfo`1.Serialize(Utf8JsonWriter writer, T& rootValue, Object rootValueBoxed)
   at System.Text.Json.JsonSerializer.WriteString[TValue](TValue& value, JsonTypeInfo`1 jsonTypeInfo)
   at System.Text.Json.JsonSerializer.Serialize[TValue](TValue value, JsonTypeInfo`1 jsonTypeInfo)
   at ConsoleApp1.Program.Main(String[] _) in C:\Users\ceztko\Source\Repos\ConsoleApp1\ConsoleApp2\Program.cs:line 68

@eiriktsarpalis
Copy link
Member Author

I've been able to trim down to the following repro:

string json = JsonSerializer.Serialize(new Derived(), Base.Context.Default.Derived);
Console.WriteLine(json);

public partial class Base
{
    [JsonSerializable(typeof(Derived))]
    internal partial class Context : JsonSerializerContext;
}

public class Derived : Base
{
    [JsonInclude]
    private int Value;
}

The relevant code in the source generator uses the Compilation.IsSymbolAccessibleWithin method to determine if a member is accessible from a given symbol:

else if (IsSymbolAccessibleWithin(fieldInfo, within: contextType))
{
isAccessible = true;
canUseGetter = hasJsonInclude;
canUseSetter = hasJsonInclude && !isReadOnly;
}
else
{
hasJsonIncludeButIsInaccessible = hasJsonInclude;
}

Perhaps that method is hitting a corner case? cc @dotnet/roslyn

That being said, it seems like a super low-priority issue with a clear workaround.

@ceztko
Copy link

ceztko commented May 10, 2024

That being said, it seems like a super low-priority issue with a clear workaround.

As said, we were trying to exploit the limits of the system, and I came up with this idea of the inner class context. The workaround (making the fields internal) is acceptable for now but as you understand this lowers the quality of the encapsulation in my code and that's why I'm happy serialization of private fields will come anyway in a more general form, at some point.

Perhaps that method is hitting a corner case? cc @dotnet/roslyn

FYI, it looks like the cc failed.

@Joe4evr
Copy link
Contributor

Joe4evr commented May 10, 2024

FYI, it looks like the cc failed.

Nah, github simply doesn't render the tag if you don't have permission to see inside the group being tagged.

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

4 participants