Aot annotation on System.Text.Json by LakshanF · Pull Request #67582 · dotnet/runtime

LakshanF · 2022-04-05T12:37:52Z

The annotation falls into two categories

DynamicMethod calls: All DynamicMethod calls are behind the runtime feature switch and propagated up to the switch method and suppressed
Copy the annotation that was used for MakeGenericXXX in the trimmer but by using RequiresDynamicCode (same message as trimmer)

This PR takes a dependency on a pending API request and hence is in a draft form.

ghost · 2022-04-05T12:37:58Z

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.

ghost · 2022-04-05T12:38:03Z

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

DynamicMethod calls: All DynamicMethod calls are behind the runtime feature switch and propagated up to the switch method and suppressed
Copy the annotation that was used for MakeGenericXXX in the trimmer but by using RequiresDynamicCode (same message as trimmer)

This PR takes a dependency on a pending API request and hence is in a draft form.

Author:	LakshanF
Assignees:	LakshanF
Labels:	`area-System.Text.Json`, `new-api-needs-documentation`
Milestone:	-

eerhardt · 2022-04-05T15:33:53Z

        ///   The object to be added to the end of the <see cref="JsonArray"/>.
        /// </param>
        [RequiresUnreferencedCode(JsonValue.CreateUnreferencedCodeMessage)]
+        [RequiresDynamicCode(JsonValue.CreateUnreferencedCodeMessage)]


I believe you need to update the ref code as well for all public changes.

eerhardt · 2022-04-05T15:37:59Z


        [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode",
                Justification = "Workaround for https://github.com/mono/linker/issues/1416. All usages are marked as unsafe.")]
+        [UnconditionalSuppressMessage("AOT", "IL3050:Calling members annotated with 'RequiresDynamicCodeAttribute' may break functionality when AOT compiling.", Justification = "<Pending>")]


The strings in the second string, IL3050 are really long. Can they just be simplified to `IL3050:RequiresDynamicCodeAttribute".

All justification should be filled out.

Is the justification the same as the existing suppress message - working around Linker warns about generic type flown into lambda method linker#1416? That doesn't seem right. Maybe this method should just have [RequiresDynamicCode] on it.

eerhardt · 2022-04-05T15:39:09Z

        /// for <typeparamref name="TValue"/> or its serializable members.
        /// </exception>
        [RequiresUnreferencedCode(SerializationUnreferencedCodeMessage)]
+        [RequiresDynamicCode(SerializationUnreferencedCodeMessage)]


I'm not sure the existing SerializationUnreferencedCodeMessage is the right message for RequiresDynamicCode. It says:

"JSON serialization and deserialization might require types that cannot be statically analyzed. Use the overload that takes a JsonTypeInfo or JsonSerializerContext, or make sure all of the required types are preserved."

I'm not sure that advice works correctly for requiring dynamic code.

krwq · 2022-04-05T16:08:22Z


        [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode",
            Justification = "The ctor is marked with RequiresUnreferencedCode.")]
+        [UnconditionalSuppressMessage("AOT", "IL3050:Calling members annotated with 'RequiresDynamicCodeAttribute' may break functionality when AOT compiling.", Justification = "<Pending>")]


could they re-use same attributes rather than doubling them?

MichalStrehovsky · 2022-04-06T02:10:54Z

    {
        public override bool CanConvert(Type typeToConvert) => GetAsyncEnumerableInterface(typeToConvert) is not null;

+        [UnconditionalSuppressMessage("AOT", "IL3050:Calling members annotated with 'RequiresDynamicCodeAttribute' may break functionality when AOT compiling.", Justification = "<Pending>")]


Unless we have guarantees that the typeToConvert and elementType are always reference type, or the code got compiled through other means, this is not safe to suppress.

MichalStrehovsky · 2022-04-06T02:12:51Z

            return converter;
        }

+        [UnconditionalSuppressMessage("AOT", "IL3050:Calling members annotated with 'RequiresDynamicCodeAttribute' may break functionality when AOT compiling.", Justification = "<Pending>")]


Unless the key type and value type are reference types or code for that instantiation is available through other means, this is not safe to suppress.

MichalStrehovsky · 2022-04-06T02:13:41Z

        [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2055:MakeGenericType",
            Justification = "'EnumConverter<T> where T : struct' implies 'T : new()', so the trimmer is warning calling MakeGenericType here because enumType's constructors are not annotated. " +
            "But EnumConverter doesn't call new T(), so this is safe.")]
+        [UnconditionalSuppressMessage("AOT", "IL3050:Calling members annotated with 'RequiresDynamicCodeAttribute' may break functionality when AOT compiling.", Justification = "<Pending>")]


Enum types are valuetypes, the MakeGenericType will likely throw 100% of time.

You might consider suppressing the warning here and add an annotation in the converter factory constructor, as is done here:

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IEnumerableConverterFactory.cs

Lines 31 to 32 in 765ecde

[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode",

Justification = "The ctor is marked RequiresUnreferencedCode.")]

The factory constructors are only invoked in the context of reflection serialization code paths, with the notable exception of JsonStringEnumConverter which is a public class. It would likely require an annotation on its constructors.

MichalStrehovsky · 2022-04-06T02:14:33Z

        [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2055:MakeGenericType",
            Justification = "'NullableConverter<T> where T : struct' implies 'T : new()', so the trimmer is warning calling MakeGenericType here because valueTypeToConvert's constructors are not annotated. " +
            "But NullableConverter doesn't call new T(), so this is safe.")]
+        [UnconditionalSuppressMessage("AOT", "IL3050:Calling members annotated with 'RequiresDynamicCodeAttribute' may break functionality when AOT compiling.", Justification = "<Pending>")]


Same as for the EnumConverter above.

MichalStrehovsky · 2022-04-06T02:15:04Z

 #endif
        }

+        [UnconditionalSuppressMessage("AOT", "IL3050:Calling members annotated with 'RequiresDynamicCodeAttribute' may break functionality when AOT compiling.", Justification = "<Pending>")]


Same concern as the other MakeGenericType comments

MichalStrehovsky · 2022-04-06T02:16:58Z

 {
+#if NETCOREAPP
+    [RequiresDynamicCode("Runtime code generation is not available for Aot form factor")]
+    [UnconditionalSuppressMessage("AotAnalysis", "IL3051:RequiresDynamicCode",


It doesn't look right that we need this suppression if the type is RDC.

MichalStrehovsky · 2022-04-06T02:18:59Z

 {
+#if NETCOREAPP
+    [RequiresDynamicCode("Runtime code generation is not available for Aot form factor")]
+    [UnconditionalSuppressMessage("AotAnalysis", "IL3051:RequiresDynamicCode",


Same - we shouldn't need this. If this type is RDC, we can't construct an instance of it without a warning. It should be okay to have mismatched virtual override annotations after that because we'd be warned at the time the instance is constructed.

eiriktsarpalis · 2022-04-27T11:06:59Z

I'm guessing we should we close this in favor of #68464?

Aot annotation on System.Text.Json

fcb68df

LakshanF requested a review from eerhardt April 5, 2022 12:37

ghost assigned LakshanF Apr 5, 2022

ghost added area-System.Text.Json new-api-needs-documentation labels Apr 5, 2022

LakshanF requested a review from MichalStrehovsky April 5, 2022 12:38

LakshanF marked this pull request as draft April 5, 2022 12:39

eerhardt reviewed Apr 5, 2022

View reviewed changes

krwq reviewed Apr 5, 2022

View reviewed changes

MichalStrehovsky reviewed Apr 6, 2022

View reviewed changes

eiriktsarpalis mentioned this pull request Apr 19, 2022

Add missing RUC annotation to IAsyncEnumerableConverterFactory #68233

Merged

LakshanF mentioned this pull request Apr 24, 2022

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

Merged

LakshanF closed this May 10, 2022

LakshanF deleted the AotJsonAnnotation branch May 10, 2022 13:20

ghost locked as resolved and limited conversation to collaborators Jun 9, 2022

	[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode",
	Justification = "The ctor is marked RequiresUnreferencedCode.")]

Conversation

LakshanF commented Apr 5, 2022

Uh oh!

ghost commented Apr 5, 2022

Uh oh!

ghost commented Apr 5, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eiriktsarpalis commented Apr 27, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants