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.

Sign up for GitHub

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

Jump to bottom

Annotating System.Text.Json library for aot form factor #68464

Merged
merged 10 commits into from
May 5, 2022
Merged

Annotating System.Text.Json library for aot form factor #68464

merged 10 commits into from
May 5, 2022

Conversation

LakshanF
Copy link
Contributor

The annotation falls into two categories and incorporates fb from the old PR, #67582.

  1. DynamicMethod calls: All DynamicMethod calls are behind the runtime feature switch and propagated up to the switch method and suppressed
  2. Copy the annotation that was used for MakeGenericXXX in the trimmer but by using RequiresDynamicCode

New PR instead of continuing from previous one, #67582, since there has been significant external changes that impact this library, including getting #67368 approved. The previous PR feedback is reflected in this change, specifically,

  1. Updating the reference API as suggested by this fb.
  2. Added a warning instead of suppressing the warning as suggested here.
  3. Changed the warning message to match aot requirements as suggested.
  4. Given factory converters are used only in serialization reflection paths as mentioned by @eiriktsarpalis to the converter suppression feedback, kept the suppression on. Added a warning to JsonStringEnumConverter. The impacted types are;

EnumConverterFactory,

NullableConverterFactory

UnsupportedTypeConverterFactory

  1. IAsyncEnumerableConverterFactory and ObjectConverterFactory.cs have constructors that have the warning and left the suppression on the impacted method.
  2. Removed the IL3051 suppression from ReflectionEmitMemberAccessor and ReflectionEmitCachingMemberAccessor as suggested.

@ghost ghost assigned LakshanF Apr 24, 2022
@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to 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.

@LakshanF LakshanF requested a review from eerhardt April 24, 2022 13:51
@ghost
Copy link

ghost commented Apr 24, 2022

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 annotation falls into two categories and incorporates fb from the old PR, #67582.

  1. DynamicMethod calls: All DynamicMethod calls are behind the runtime feature switch and propagated up to the switch method and suppressed
  2. Copy the annotation that was used for MakeGenericXXX in the trimmer but by using RequiresDynamicCode

New PR instead of continuing from previous one, #67582, since there has been significant external changes that impact this library, including getting #67368 approved. The previous PR feedback is reflected in this change, specifically,

  1. Updating the reference API as suggested by this fb.
  2. Added a warning instead of suppressing the warning as suggested here.
  3. Changed the warning message to match aot requirements as suggested.
  4. Given factory converters are used only in serialization reflection paths as mentioned by @eiriktsarpalis to the converter suppression feedback, kept the suppression on. Added a warning to JsonStringEnumConverter. The impacted types are;

EnumConverterFactory,

NullableConverterFactory

UnsupportedTypeConverterFactory

  1. IAsyncEnumerableConverterFactory and ObjectConverterFactory.cs have constructors that have the warning and left the suppression on the impacted method.
  2. Removed the IL3051 suppression from ReflectionEmitMemberAccessor and ReflectionEmitCachingMemberAccessor as suggested.
Author: LakshanF
Assignees: LakshanF
Labels:

area-System.Text.Json, new-api-needs-documentation
Milestone: -

@LakshanF LakshanF requested review from eiriktsarpalis, eerhardt, krwq and jkotas and removed request for eerhardt April 24, 2022 13:51
eerhardt
eerhardt reviewed Apr 26, 2022
View reviewed changes
Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like I didn't submit these comments yesterday. (Sorry)

src/libraries/System.Text.Json/src/System/Text/Json/Nodes/JsonArray.cs Outdated Show resolved Hide resolved
...System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/EnumConverterFactory.cs Outdated Show resolved Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs Outdated Show resolved Hide resolved
...ries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs Outdated Show resolved Hide resolved
...es/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptionsUpdateHandler.cs Outdated Show resolved Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonStringEnumConverter.cs Outdated Show resolved Hide resolved
@layomia layomia mentioned this pull request Apr 26, 2022
Closed
@eiriktsarpalis eiriktsarpalis mentioned this pull request Apr 27, 2022
Closed
@eerhardt
Copy link
Member

How are these annotations being checked? Is the analyzer turned on during the build? If so, can you point me to where it is turned on?

eerhardt
eerhardt reviewed Apr 27, 2022
View reviewed changes
src/libraries/System.Text.Json/ref/System.Text.Json.cs Outdated
public JsonStringEnumConverter() { }
[System.Diagnostics.CodeAnalysis.RequiresDynamicCode("JSON serialization and deserialization might require types that cannot be statically analyzed and might need runtime code generation. Use System.Text.Json source generation for native AOT applications.")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is rather unfortunate that we can't support converting enums to and from strings without requiring dynamic code. It feels like we should be able to make something work here for NativeAOT.

I don't think you need to solve this in this PR, so maybe we can open an issue for it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is rather unfortunate that we can't support converting enums to and from strings without requiring dynamic code.

Runtime allows you to convert enums to and from strings with NativeAOT. The problem is that the EnumConverter implementation creates new generic types dynamically: typeof(EnumConverter<>).MakeGenericType.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. I'm thinking (in a separate PR) we add a switch to JsonStringEnumConverter on RuntimeFeature.IsDynamicCodeSupported, and fallback to some other implementation when dynamic code isn't supported. Then we can remove this attribute and support this API in NativeAOT.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the JsonStringEnumConverter used with serialization source generators?

If yes, I think there should be a static path that is both fast and that does not require dynamical code generation. For example, can EnumConverter<T> be public and source generators changed to use it?

If not, I do not think we need to bother with making it AOT friendly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eiriktsarpalis @layomia @krwq - can one of you answer this? Can the JsonStringEnumConverter be used outside of the reflection-based serialization? i.e. with source generation.

@jkotas
Copy link
Member

jkotas commented Apr 27, 2022

runtime-dev-innerloop build is failing with 2022-04-27T15:08:30.1359969Z /_/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionEmitCachingMemberAccessor.cs(17): AOT analysis error IL3050: System.Text.Json.Serialization.Metadata.ReflectionEmitCachingMemberAccessor..cctor(): Using member 'System.Text.Json.Serialization.Metadata.ReflectionEmitMemberAccessor.ReflectionEmitMemberAccessor()' which has 'RequiresDynamicCodeAttribute' can break functionality when AOT compiling. JSON serialization and deserialization might require types that cannot be statically analyzed and might need runtime code generation. Use System.Text.Json source generation for native AOT applications. [/__w/1/s/src/coreclr/tools/aot/crossgen2/crossgen2.csproj]

@LakshanF
Copy link
Contributor Author

How are these annotations being checked? Is the analyzer turned on during the build? If so, can you point me to where it is turned on?

Its opt-in right now with /p:EnableAOTAnalyzer=true.

For native AOT apps, its automatically turned on with this SDK PR. The ILC compiler should also separately give similar warnings for the IL.

@eerhardt
Copy link
Member

Its opt-in right now with /p:EnableAOTAnalyzer=true.

Should we be turning that property on for this library with this change? There are new features and bug fixes happening in this library, and I think we would want to ensure the library's annotations stay up to date.

@eiriktsarpalis
Copy link
Member

Its opt-in right now with /p:EnableAOTAnalyzer=true.

Should we be turning that property on for this library with this change? There are new features and bug fixes happening in this library, and I think we would want to ensure the library's annotations stay up to date.

Yes please, we will definitely be refactoring some of the code being touched by these changes. We should try to avoid inadvertently breaking AOT annotations in the future.

eerhardt
eerhardt reviewed Apr 29, 2022
View reviewed changes
...em.Text.Json/src/System/Text/Json/Serialization/Converters/Value/NullableConverterFactory.cs Outdated Show resolved Hide resolved
....Json/src/System/Text/Json/Serialization/Converters/Value/UnsupportedTypeConverterFactory.cs Outdated Show resolved Hide resolved
...Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionEmitCachingMemberAccessor.cs Outdated Show resolved Hide resolved
...System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionEmitMemberAccessor.cs Outdated Show resolved Hide resolved
...ries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs
Comment on lines +326 to +327
[UnconditionalSuppressMessage("AotAnalysis", "IL3050:RequiresDynamicCode", Justification = "The factory constructors are only invoked in the context of reflection serialization code paths " +
"and are marked RequiresDynamicCode")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why isn't this marked with RequiresDynamicCode?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This API is a gateway to a much larger dependency graph. I need to understand the potential impact on Json source generated paths better to correctly annotate this and need help from @eiriktsarpalis @layomia @krwq first

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is somewhat concerning. If this means the source generation path might not work in NativeAot, we will need to understand it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this suppression be replaced by proper annotations instead?

For example, I see the following callchain:

[RequiresDynamicCode] public GetConverter
->
GetConverterInternal (not annotated)
->
GetConverterFromType (not annotated)
->
[UnconditionalSuppressMessage] GetConverterFromAttribute

Should GetConverterInternal and GetConverterFromType be annotated as RequiresDynamicCode on this call-chain? (And similar to other call-chains.)

Copy link
Contributor Author

@LakshanF LakshanF May 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are 2 concerns in the call graph that I left open to get FB;

WriteAsPropertyNameCore -> GetConverterInternal -> GetConverterFromType -> GetConverterFromAttribute

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened #68878 to track the status since there needs to be converter changes that should be done by domain experts in this space.

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs Outdated Show resolved Hide resolved
@LakshanF LakshanF mentioned this pull request May 4, 2022
Closed
@LakshanF LakshanF force-pushed the AotJsonAnnotation3 branch from c857681 to 3eff9c3 Compare May 5, 2022 00:23
eerhardt
eerhardt approved these changes May 5, 2022
View reviewed changes
Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this work, @LakshanF. This looks good to me. Hopefully that one remaining suppression won't be too much work to solve in a follow up.

@LakshanF LakshanF merged commit 8a31a32 into dotnet:main May 5, 2022
@LakshanF LakshanF deleted the AotJsonAnnotation3 branch May 5, 2022 16:01
@layomia layomia mentioned this pull request May 17, 2022
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Jun 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jkotas jkotas jkotas left review comments

@eerhardt eerhardt eerhardt approved these changes

@eiriktsarpalis eiriktsarpalis Awaiting requested review from eiriktsarpalis

@krwq krwq Awaiting requested review from krwq

Assignees

@LakshanF LakshanF

Labels
area-System.Text.Json new-api-needs-documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@LakshanF @eerhardt @jkotas @eiriktsarpalis