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

System.Text.Json.Serialization.JsonStringEnumConverter isn't linker-safe #34449

Closed
SteveSandersonMS opened this issue Apr 2, 2020 · 4 comments · Fixed by #38595
Closed

System.Text.Json.Serialization.JsonStringEnumConverter isn't linker-safe #34449

SteveSandersonMS opened this issue Apr 2, 2020 · 4 comments · Fixed by #38595
Assignees
Labels
area-System.Text.Json linkable-framework Issues associated with delivering a linker friendly framework
Milestone

Comments

@SteveSandersonMS
Copy link
Member

Most other converters in S.T.J. (as far as I know) are safe because their default constructors are referenced statically. However, if you use JsonStringEnumConverter like this:

[JsonConverter(typeof(JsonStringEnumConverter))]
public enum MyEnum { A, B, C }

then there is no static reference to JsonStringEnumConverter's default constructor, so the linker will strip it out. Then at runtime it will fail when JsonSerializerOptions.GetConverterFromAttribute tries to call Activator.CreateInstance(converterType) (where converterType = typeof(JsonStringEnumConverter)). This was originally reported at dotnet/aspnetcore#19086

For Blazor 3.2.0, we'll put in a workaround into Blazor's built-in linker config. However it would make sense to fix the underlying issue in S.T.J. in the longer term.

Suggested fix

Either System.Text.Json.dll should embed its own linker config file preserving the constructors for all converter types, or consider extending the set of types listed in JsonSerializerOptions.DefaultSimpleConverters to include an instantiation of JsonStringEnumConverter (or have any other static reference to its constructor).

This applies to all converter types that may be used at runtime. The only one I know of that's causing a problem is JsonStringEnumConverter, but I didn't exhaustively check that all the others are linker-safe too.

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.Text.Json untriaged New issue has not been triaged by the area owner labels Apr 2, 2020
@stephentoub
Copy link
Member

cc: @eerhardt

@eerhardt
Copy link
Member

eerhardt commented Apr 3, 2020

Another potential way to fix this is to add the correct dataflow analysis annotations (dotnet/linker#988) to JsonConverter. Using the current names in that proposal, all we would need to add [DynamicallyAccessedMembers(MemberKinds.DefaultConstructor)] to JsonConverterAttribute, which would tell the linker to keep the default constructor on any type passed into JsonConverter.

/cc @vitek-karas @MichalStrehovsky @sbomer

@MichalStrehovsky
Copy link
Member

Yes, we'll want to solve this through annotations - the fact that the other converters have their constructor rooted from elsewhere is hardly a contract one could rely on: they just happen to work right now. We are working on a linking solution that would let us guarantee things work after linking by construction (as opposed to "working thanks to luck").

@stephentoub
Copy link
Member

For reference:

[PreserveDependency(".ctor", "System.Text.Json.Serialization.Converters.ArrayConverter`2")]
[PreserveDependency(".ctor", "System.Text.Json.Serialization.Converters.ConcurrentQueueOfTConverter`2")]
[PreserveDependency(".ctor", "System.Text.Json.Serialization.Converters.ConcurrentStackOfTConverter`2")]
[PreserveDependency(".ctor", "System.Text.Json.Serialization.Converters.DefaultArrayConverter`2")]
[PreserveDependency(".ctor", "System.Text.Json.Serialization.Converters.DictionaryOfStringTValueConverter`2")]
[PreserveDependency(".ctor", "System.Text.Json.Serialization.Converters.ICollectionOfTConverter`2")]
[PreserveDependency(".ctor", "System.Text.Json.Serialization.Converters.IDictionaryOfStringTValueConverter`2")]
[PreserveDependency(".ctor", "System.Text.Json.Serialization.Converters.IEnumerableOfTConverter`2")]
[PreserveDependency(".ctor", "System.Text.Json.Serialization.Converters.IEnumerableWithAddMethodConverter`1")]
[PreserveDependency(".ctor", "System.Text.Json.Serialization.Converters.IListConverter`1")]
[PreserveDependency(".ctor", "System.Text.Json.Serialization.Converters.IListOfTConverter`2")]
[PreserveDependency(".ctor", "System.Text.Json.Serialization.Converters.ImmutableDictionaryOfStringTValueConverter`2")]
[PreserveDependency(".ctor", "System.Text.Json.Serialization.Converters.ImmutableEnumerableOfTConverter`2")]
[PreserveDependency(".ctor", "System.Text.Json.Serialization.Converters.IReadOnlyDictionaryOfStringTValueConverter`2")]
[PreserveDependency(".ctor", "System.Text.Json.Serialization.Converters.ISetOfTConverter`2")]
[PreserveDependency(".ctor", "System.Text.Json.Serialization.Converters.ListOfTConverter`2")]
[PreserveDependency(".ctor", "System.Text.Json.Serialization.Converters.QueueOfTConverter`2")]
[PreserveDependency(".ctor", "System.Text.Json.Serialization.Converters.StackOfTConverter`2")]
public override JsonConverter CreateConverter(Type typeToConvert, JsonSerializerOptions options)

[PreserveDependency(".ctor", "System.Text.Json.Serialization.Converters.ObjectDefaultConverter`1")]
[PreserveDependency(".ctor", "System.Text.Json.Serialization.Converters.LargeObjectWithParameterizedConstructorConverter`1")]
[PreserveDependency(".ctor", "System.Text.Json.Serialization.Converters.SmallObjectWithParameterizedConstructorConverter`5")]

[PreserveDependency(
".ctor(System.Text.Json.Serialization.Converters.EnumConverterOptions)",
"System.Text.Json.Serialization.Converters.EnumConverter`1")]

[PreserveDependency(".ctor()", "System.Text.Json.Serialization.Converters.KeyValuePairConverter`2")]

[PreserveDependency(
".ctor(System.Text.Json.Serialization.Converters.EnumConverterOptions, System.Text.Json.JsonNamingPolicy)",
"System.Text.Json.Serialization.Converters.EnumConverter`1")]

@sbomer sbomer closed this as completed Apr 3, 2020
@sbomer sbomer reopened this Apr 3, 2020
@layomia layomia removed the untriaged New issue has not been triaged by the area owner label Apr 6, 2020
@layomia layomia added this to the 5.0 milestone Apr 6, 2020
@layomia layomia self-assigned this Apr 6, 2020
@eerhardt eerhardt added the linkable-framework Issues associated with delivering a linker friendly framework label Jun 24, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Text.Json linkable-framework Issues associated with delivering a linker friendly framework
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants