-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Implement JsonStringEnumConverter<TEnum> #87149
Implement JsonStringEnumConverter<TEnum> #87149
Conversation
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis Issue DetailsFix #79311.
|
/azp run runtime-extra-platforms |
Azure Pipelines successfully started running 1 pipeline(s). |
FYI - @stephentoub: will this will allow you to undo the workarounds in https://github.com/microsoft/semantic-kernel/pull/1284/files#diff-b2c336c6d6a74aa2671ea2afca585cdb1bf6a32ba4fdd0ddbfbaef516dd6686bR53? |
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonStringEnumConverter.cs
Outdated
Show resolved
Hide resolved
src/libraries/apicompat/ApiCompatBaseline.NetCoreAppLatestStable.xml
Outdated
Show resolved
Hide resolved
@@ -1186,6 +1187,7 @@ public static partial class JsonMetadataServices | |||
public static System.Text.Json.Serialization.JsonConverter<T> GetEnumConverter<T>(System.Text.Json.JsonSerializerOptions options) where T : struct { throw null; } | |||
public static System.Text.Json.Serialization.JsonConverter<T?> GetNullableConverter<T>(System.Text.Json.JsonSerializerOptions options) where T : struct { throw null; } | |||
public static System.Text.Json.Serialization.JsonConverter<T?> GetNullableConverter<T>(System.Text.Json.Serialization.Metadata.JsonTypeInfo<T> underlyingTypeInfo) where T : struct { throw null; } | |||
public static System.Text.Json.Serialization.JsonConverter<T> GetStringEnumConverter<T>(System.Text.Json.Serialization.JsonStringEnumConverter converterFactory, System.Text.Json.JsonSerializerOptions options) where T : struct { throw null; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why doesn't this have where T : struct, Enum
? That is what is in the approved API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would appear that the underlying converter doesn't have a : Enum
constraint. Given that this matches the existing GetEnumConverter method, we should probably just update the proposal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, actually this appears to be a case where the reference file is diverging from the implementation. Both the GetEnumConverter
method shipped in .NET 6 and GetStringEnumConverter
have an : Enum
constraint but this isn't reflected in the ref file. Surprised this was never caught by our tooling.
cc @carlossanlop @ViktorHofer is it safe to update the ref file to match the implementation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eiriktsarpalis the code for APICompat which is used to compare the contract vs the implementation is located in the sdk repo: https://github.com/dotnet/sdk/tree/main/src/ApiCompat
Please file an issue so that we can track this bug / feature request (unsure yet what it is).
@carlossanlop @ViktorHofer is it safe to update the ref file to match the implementation?
Yes, that should be fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dumb question, but did you make sure to autogenerate the ref file @eiriktsarpalis ? https://github.com/dotnet/runtime/blob/main/docs/coding-guidelines/updating-ref-source.md
I ask because we're not supposed to manually edit them. Unless of course, the tool that autogenerates it has a bug, which could be the case here, but it's not clear from the comments (at least for me) if the ref was autogenerated or not.
public partial class JsonStringEnumConverter : System.Text.Json.Serialization.JsonConverterFactory | ||
{ | ||
[System.Diagnostics.CodeAnalysis.RequiresDynamicCodeAttribute("JsonStringEnumConverter cannot be statically analyzed and requires runtime code generation. Native AOT users should only use this type indirectly via JsonConverterAttribute annotations.")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should test this out, but I think this is still going to give warnings when being used in AOT apps. JsonConverterAttribute is annotated with [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)]
:
Line 25 in 2bf8f1a
public JsonConverterAttribute([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)] Type converterType) |
which means it is going to reflect against the parameterless constructor of the Type being specified. Here we are marking that constructor as "RequiresDynamicCode". The AOT compiler is going to warn that there is reflection being done against a constructor marked as "unsafe for AOT".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're right. I'm not sure we would be able to reasonably change either the attribute or the converter annotation.
The only safe alternative I can think of is we expose a separate JsonStringEnumConverter<T>
converter that isn't a factory and have the source generator emit a diagnostic prompting users to change their annotations to the new type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW I tested this code:
using System;
using System.Reflection;
using System.Text.Json;
using System.Text.Json.Serialization;
Console.WriteLine(JsonSerializer.Serialize(new MyPoco(), MyContext.Default.MyPoco));
public class MyPoco
{
[JsonConverter(typeof(JsonStringEnumConverter))]
public BindingFlags Flags { get; set; }
}
[JsonSerializable(typeof(MyPoco))]
public partial class MyContext : JsonSerializerContext
{
}
and it appears to not be warning on the attribute annotation location. The concern though is that future revisions of the AOT compiler could be warning there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vitek-karas pointed out that the above not warning is accidental because the attribute components get trimmed away. Once I include the following line:
Console.WriteLine(new JsonConverterAttribute(typeof(int)).ConverterType);
The compiler immediately warns. So it seems to me that using JsonStringEnumConverter
(or any JsonConverterFactory
for that matter) is simply not viable in source gen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vitek-karas pointed out that the above not warning is accidental because the attribute components get trimmed away.
Is there an issue logged for this? That seems like an analysis hole to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vitek-karas pointed out that the above not warning is accidental because the attribute components get trimmed away.
Is there an issue logged for this? That seems like an analysis hole to me.
Vitek can correct me if I'm wrong for this case since he looked at it, but in general we don't analyze code that wasn't needed. So if the warning is for some piece of code that was trimmed, we'd not look at it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is NOT a bug - there is now reflection access to the Flags
property in the repro, so the AOT compiler doesn't keep any metadata about the property (or even the fact that there is a property, it only keeps the accessors), that includes not having any attributes for it. And since it's not going to keep the attributes, it doesn't run any validation on it and thus doesn't warn.
Note that if this sample is modified to use RequiresUnreferencedCode
(so that analyzer/linker will actually react to it), then both analyzer and linker would report the warning. Because they're not as precise in "trimming" as AOT compiler is.
So while this doesn't warn, I would argue that we don't want to rely on that behavior, since even defining what the behavior looks like is pretty difficult - and implementing it in illink and especially in the analyzer might be pretty tricky.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the "trick" here is that no preserved code accesses the JsonConverterAttribute.ConverterType
property? That allows the AOT compiler to not warn?
Does the ctor that is mark as RequiresDynamicCode
still get preserved? Even though no one accesses the ConverterType
property?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the trimmer and AOT, if the .ctor is preserved we will report the RDC warning on it - always. The repro above actually does warn, but from the source generated code which calls the .ctor directly.
Analyzer currently doesn't correctly implement this behavior (combination of DAM and RDC) and will not report a warning (it will for DAM and RUC - don't ask ;-))
src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs
Outdated
Show resolved
Hide resolved
@layomia @eerhardt @stephentoub I just updated the PR to include the most recently approved API shape, PTAL. |
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonStringEnumConverter.cs
Show resolved
Hide resolved
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/EnumConverterTests.cs
Show resolved
Hide resolved
/// Reading is case insensitive, writing can be customized via a <see cref="JsonNamingPolicy" />. | ||
/// </remarks> | ||
/// <typeparam name="TEnum">The enum type that this converter targets.</typeparam> | ||
public class JsonStringEnumConverter<TEnum> : JsonConverterFactory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eerhardt This was brought up in API review, do we need to annotate TEnum
with DynamicallyAccessedMembers
in case the trimmer removes enum values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trimming never removes enum values. It cannot be done safely because ToString
on enums relies on them - as soon as we see the enum getting boxed or it's typeof obtained, we keep all the fields. So this is fine without annotation - I assume there are no static analysis warnings either.
@@ -72,6 +72,14 @@ internal static class DiagnosticDescriptors | |||
category: JsonConstants.SystemTextJsonSourceGenerationName, | |||
defaultSeverity: DiagnosticSeverity.Warning, | |||
isEnabledByDefault: true); | |||
|
|||
public static DiagnosticDescriptor JsonStringEnumConverterNotSupportedInAot { get; } = new DiagnosticDescriptor( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this new diagnostic? Isn't the RequiresDynamicCode
attribute on JsonStringEnumConverter
enough to devs that JsonStringEnumConverter
is not supported in native AOT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was added in reaction to #87149 (comment) not triggering at all either at design time or at compile time. If we can get to the point that this always warns then I could remove that diagnostic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That comment will not be a problem in publish - both trimmer and AOT compiler will correctly warn if necessary.
The only "hole" is in the analyzer - which will not report this warning currently.
That said - if the specific diagnostic will always provide better UX than the more general RDC warning (even if the analyzer did report it) - so it might be worth keeping this just for the better UX.
Downside is that if we fix the analyzer this might produce two warnings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we deprecate the diagnostic in the future if the analyzer does get fixed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably - we may need to figure out some versioning stuff, but it shouldn't be too hard.
Co-authored-by: Vitek Karas <[email protected]>
Fix #79311.