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

[feature/dataflow] Support DynamicallyAccessedMemberKinds.RecursiveProperties #1087

Closed
eerhardt opened this issue Apr 9, 2020 · 9 comments
Closed

Comments

@eerhardt
Copy link
Member

eerhardt commented Apr 9, 2020

For (de)serialization cases, like in System.Text.Json, we need a way to tell the linker to keep the properties of type T, and also the properties of any types referenced by T's properties recursively.

Take the example in https://docs.microsoft.com/en-us/dotnet/standard/serialization/system-text-json-how-to:

public class WeatherForecast
{
    public DateTimeOffset Date { get; set; }
    public int TemperatureCelsius { get; set; }
    public string Summary { get; set; }
    public string SummaryField;
    public IList<DateTimeOffset> DatesAvailable { get; set; }
    public Dictionary<string, HighLowTemps> TemperatureRanges { get; set; }
    public string[] SummaryWords { get; set; }
}

public class HighLowTemps
{
    public int High { get; set; }
    public int Low { get; set; }
}

...

jsonString = File.ReadAllText(fileName);
weatherForecast = JsonSerializer.Deserialize<WeatherForecast>(jsonString);

Note that the properties of HighLowTemps need to be preserved even though I didn't directly pass HighLowTemps into JsonSerializer.Deserialize. Also note that when walking the properties recursively, we need to handle generic collections: Dictionary<string, HighLowTemps>.

@MichalStrehovsky
Copy link
Member

MichalStrehovsky commented Apr 9, 2020

Can we have someone who understand System.Text.Json really well look at whether they can make things work with just a capability like this? We spent a lot of time trying to make reflection-based serialization in .NET Native "just work", to no avail - serializers pretty much never operate in these naive ways, and most then have a bunch of pluggability that makes any analysis impossible.

For this example:

  • How is this going to instantiate types? I assume we need to new up the HighLowTemps somehow to deserialize, but nothing guarantees the constructor is kept.
  • How does deserialization of an IList work? I assume System.Text.Json special cases/hardcodes something for this that can actually be instantiated instead of the interface.

@eerhardt
Copy link
Member Author

eerhardt commented Apr 9, 2020

cc @steveharter @layomia @jozkee - the owners of System.Text.Json.

look at whether they can make things work with just a capability like this?

I'm sure it will take other capabilities as well, for example PreserveDependency that is used now:

https://github.com/dotnet/runtime/blob/d59f143221bfa500fe2b974827dcb47eb4ce327c/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IEnumerableConverterFactory.cs#L28-L46

How is this going to instantiate types? I assume we need to new up the HighLowTemps somehow to deserialize, but nothing guarantees the constructor is kept.

I assume we will also need a RecursiveConstructors or RecursiveDefaultConstructors option as well.

@layomia
Copy link

layomia commented Apr 16, 2020

For serializing and deserializing types passed to JsonSerializer, we'll need the following

  • for every type passed, and every type in it's object graph:

    • public properties & fields

      • we only need public getter/setters, unless [JsonInclude] is used
      • we don't need members annotated with [JsonIgnore] when the attributes's Condition property is JsonIgnoreCondition.Always.
      • we don't need properties/fields of collections, only the .GetEnumerator() & respective "Add" methods e.g. (IList.Add, Stack<T>.Push).
        • except for .Count/.Length/. IsReadOnly if available
      • we need the public parameterless for converter specified with JsonConverterAttribute on properties/fields.
      • properties/fields marked with JsonExtensionData(public only)
    • public constructors

      If none of these is present, then the type is likely not supported for deserialization. We don't need to see the other ctors:

      • Single ctor annotated with JsonConstructorAttribute, or
      • Parameterless ctor, or
      • Singlular parameterized ctor (if no other public ctor is present)

      Most interfaces and abstract types are not supported for deserialization. The exceptions here are "known" types -- mostly collections -- where we map interfaces/abstract types (we refer to these as the declared type) to concrete types (runtime type) we can instantiate e.g. IList<T> maps to List<T>. We have statically visible references to these runtime types, so presumably the linker we'll leave those alone.

      Extended polymorphic support (including introducing a way for interfaces & abstract types to be deserialized) is in the backlog for 5.0. Not sure yet how it will affect the linker work here:

    • Public parameterless for converter specified with JsonConverterAttribute on type.

  • Public parameterless ctor for every JsonNamingPolicy passed through JsonSerializerOptions.

  • Public parameterless ctor for every JsonConverter<T>/JsonConverter passed through JsonSerializerOptions.

  • Some constructors for all converters that live in System.Text.Json. We need to audit these comprehensively. There's an open issue with the ctor for JsonStringEnumConverter.

cc @steveharter, @jozkee PTAL for any gaps.

@MichalStrehovsky
Copy link
Member

Thanks @layomia.

Translating this to Linker-terms in which we would be able to avoid special-casing System.Text.Json within the linker (which is really preferable because we don't want to send a message that there's only one linker-friendly way to do serialization in .NET):

  • We need DynamicallyAccessedMemberKinds enumeration flag to cover public+private instance properties, public+private instance fields, all public constructors, recursively. (We need to capture the superset so that we don't need to do anything special for JsonInclude/JsonIgnore).
  • We need [feature/dataflow] Allow DynamicallyAccessedMembers on generic types #1086 so that we can annotate the places that find the Add/Push/Enqueue methods on collection types

Polymorphic deserialization will probably bring new challenges (especially if it's done through something like a KnownTypes collection - we don't have means to model dataflow in collections right now).

@vitek-karas
Copy link
Member

/cc @vitek-karas

@steveharter
Copy link
Member

Polymorphic deserialization will probably bring new challenges (especially if it's done through something like a KnownTypes collection

To avoid security-related concerns we do need a KnownTypes attribute and\or a mechanism to add known types at run-time.

@steveharter
Copy link
Member

FWIW another viewpoint for some users is that they may want the linker to remove unused properties. However, they likely will want the JSON to still round-trip which they can do via an extension property by using the [JsonExtensionData] attribute. With this they only have to worry about a preserving a single property.

@vitek-karas
Copy link
Member

If we were to do the "Recursive" annotations, it might be worth considering arrays - and "unfortunately" generics as well.

  • If a property is of type T[] - then for serializers we would need to propagate the annotation recursively to the T as well
  • Similarly if the type is List<T> - again, we would need to propagate to T - but this basically means it can be any generic (since we can't/should not hardcode knowledge of supported collection types/interfaces into the linker) - so even things like MySpecialType<T,U> would probably have to propagate the annotation to T and U.

@agocke
Copy link
Member

agocke commented May 26, 2021

At the moment we're not planning on doing this -- the side-effects/viralness of this annotation is too broad compared to alternatives like source generators.

We can reconsider if we find a blocking scenario and can come up with an acceptable design

@agocke agocke closed this as completed May 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants