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

Fully annotate JsonNode for trimmability #53184

Merged
merged 1 commit into from
Jun 2, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Fully annotate JsonNode for trimmability
Follow up to #52934.

- Using JsonNode in dynamic statements is not trim compatible. Add a LibraryBuild warning since there isn't a direct API to put the warning on.
- Mark JsonValueNotTrimmable's ctor as unsafe
- Fix up a few warning messages
- minor doc fixup

Contributes to #45623
  • Loading branch information
eerhardt committed Jun 1, 2021
commit c37f4c1bb80a6b283b91819e20134e10a571264d
4 changes: 2 additions & 2 deletions src/libraries/System.Text.Json/ref/System.Text.Json.cs
Original file line number Diff line number Diff line change
Expand Up @@ -518,7 +518,7 @@ public JsonArray(params System.Text.Json.Nodes.JsonNode?[] items) { }
public int Count { get { throw null; } }
bool System.Collections.Generic.ICollection<System.Text.Json.Nodes.JsonNode?>.IsReadOnly { get { throw null; } }
public void Add(System.Text.Json.Nodes.JsonNode? item) { }
[System.Diagnostics.CodeAnalysis.RequiresUnreferencedCodeAttribute("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.")]
[System.Diagnostics.CodeAnalysis.RequiresUnreferencedCodeAttribute("Creating JsonValue instances with non-primitive types is not compatible with trimming. It can result in non-primitive types being serialized, which may have their members trimmed.")]
public void Add<T>(T? value) { }
public void Clear() { }
public bool Contains(System.Text.Json.Nodes.JsonNode? item) { throw null; }
Expand Down Expand Up @@ -711,7 +711,7 @@ public abstract partial class JsonValue : System.Text.Json.Nodes.JsonNode
[System.CLSCompliantAttribute(false)]
public static System.Text.Json.Nodes.JsonValue Create(ulong value, System.Text.Json.Nodes.JsonNodeOptions? options = default(System.Text.Json.Nodes.JsonNodeOptions?)) { throw null; }
public static System.Text.Json.Nodes.JsonValue? Create<T>(T? value, System.Text.Json.Serialization.Metadata.JsonTypeInfo<T> jsonTypeInfo, System.Text.Json.Nodes.JsonNodeOptions? options = default(System.Text.Json.Nodes.JsonNodeOptions?)) { throw null; }
[System.Diagnostics.CodeAnalysis.RequiresUnreferencedCodeAttribute("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.")]
[System.Diagnostics.CodeAnalysis.RequiresUnreferencedCodeAttribute("Creating JsonValue instances with non-primitive types is not compatible with trimming. It can result in non-primitive types being serialized, which may have their members trimmed. Use the overload that takes a JsonTypeInfo, or make sure all of the required types are preserved.")]
public static System.Text.Json.Nodes.JsonValue? Create<T>(T? value, System.Text.Json.Nodes.JsonNodeOptions? options = default(System.Text.Json.Nodes.JsonNodeOptions?)) { throw null; }
public abstract bool TryGetValue<T>([System.Diagnostics.CodeAnalysis.NotNullWhenAttribute(true)] out T? value);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
<?xml version="1.0" encoding="utf-8"?>
<linker>
<assembly fullname="System.Text.Json">
<attribute fullname="System.Diagnostics.CodeAnalysis.UnconditionalSuppressMessageAttribute">
<argument>ILLink</argument>
<argument>IL2026</argument>
<property name="Scope">member</property>
<property name="Target">M:System.Text.Json.Nodes.JsonNode.System#Dynamic#IDynamicMetaObjectProvider#GetMetaObject(System.Linq.Expressions.Expression)</property>
<property name="Justification">System.Text.Json's integration with dynamic is not trim compatible. However, there isn't a direct API developers call. Instead they use the 'dynamic' keyword, which gets compiled into calls to Microsoft.CSharp. Microsoft.CSharp looks for IDynamicMetaObjectProvider implementations. Leaving this warning in the product so developers get a warning stating that using dynamic JsonValue code may be broken in trimmed apps.</property>
Copy link
Member

Choose a reason for hiding this comment

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

Does usage of dynamic itself causes a warning (without STJ)?

Copy link
Member Author

@eerhardt eerhardt Jun 1, 2021

Choose a reason for hiding this comment

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

Yes. We marked basically all APIs in Microsoft.CSharp as RequiresUnreferencedCode.

public static partial class Binder
{
[System.Diagnostics.CodeAnalysis.RequiresUnreferencedCodeAttribute("Using dynamic types might cause types or members to be removed by trimmer.")]
public static System.Runtime.CompilerServices.CallSiteBinder BinaryOperation(Microsoft.CSharp.RuntimeBinder.CSharpBinderFlags flags, System.Linq.Expressions.ExpressionType operation, System.Type? context, System.Collections.Generic.IEnumerable<Microsoft.CSharp.RuntimeBinder.CSharpArgumentInfo>? argumentInfo) { throw null; }
[System.Diagnostics.CodeAnalysis.RequiresUnreferencedCodeAttribute("Using dynamic types might cause types or members to be removed by trimmer.")]
public static System.Runtime.CompilerServices.CallSiteBinder Convert(Microsoft.CSharp.RuntimeBinder.CSharpBinderFlags flags, System.Type type, System.Type? context) { throw null; }
[System.Diagnostics.CodeAnalysis.RequiresUnreferencedCodeAttribute("Using dynamic types might cause types or members to be removed by trimmer.")]
public static System.Runtime.CompilerServices.CallSiteBinder GetIndex(Microsoft.CSharp.RuntimeBinder.CSharpBinderFlags flags, System.Type? context, System.Collections.Generic.IEnumerable<Microsoft.CSharp.RuntimeBinder.CSharpArgumentInfo>? argumentInfo) { throw null; }
[System.Diagnostics.CodeAnalysis.RequiresUnreferencedCodeAttribute("Using dynamic types might cause types or members to be removed by trimmer.")]
public static System.Runtime.CompilerServices.CallSiteBinder GetMember(Microsoft.CSharp.RuntimeBinder.CSharpBinderFlags flags, string name, System.Type? context, System.Collections.Generic.IEnumerable<Microsoft.CSharp.RuntimeBinder.CSharpArgumentInfo>? argumentInfo) { throw null; }
[System.Diagnostics.CodeAnalysis.RequiresUnreferencedCodeAttribute("Using dynamic types might cause types or members to be removed by trimmer.")]
public static System.Runtime.CompilerServices.CallSiteBinder Invoke(Microsoft.CSharp.RuntimeBinder.CSharpBinderFlags flags, System.Type? context, System.Collections.Generic.IEnumerable<Microsoft.CSharp.RuntimeBinder.CSharpArgumentInfo>? argumentInfo) { throw null; }
[System.Diagnostics.CodeAnalysis.RequiresUnreferencedCodeAttribute("Using dynamic types might cause types or members to be removed by trimmer.")]
public static System.Runtime.CompilerServices.CallSiteBinder InvokeConstructor(Microsoft.CSharp.RuntimeBinder.CSharpBinderFlags flags, System.Type? context, System.Collections.Generic.IEnumerable<Microsoft.CSharp.RuntimeBinder.CSharpArgumentInfo>? argumentInfo) { throw null; }
[System.Diagnostics.CodeAnalysis.RequiresUnreferencedCodeAttribute("Using dynamic types might cause types or members to be removed by trimmer.")]
public static System.Runtime.CompilerServices.CallSiteBinder InvokeMember(Microsoft.CSharp.RuntimeBinder.CSharpBinderFlags flags, string name, System.Collections.Generic.IEnumerable<System.Type>? typeArguments, System.Type? context, System.Collections.Generic.IEnumerable<Microsoft.CSharp.RuntimeBinder.CSharpArgumentInfo>? argumentInfo) { throw null; }
[System.Diagnostics.CodeAnalysis.RequiresUnreferencedCodeAttribute("Using dynamic types might cause types or members to be removed by trimmer.")]
public static System.Runtime.CompilerServices.CallSiteBinder IsEvent(Microsoft.CSharp.RuntimeBinder.CSharpBinderFlags flags, string name, System.Type? context) { throw null; }
[System.Diagnostics.CodeAnalysis.RequiresUnreferencedCodeAttribute("Using dynamic types might cause types or members to be removed by trimmer.")]
public static System.Runtime.CompilerServices.CallSiteBinder SetIndex(Microsoft.CSharp.RuntimeBinder.CSharpBinderFlags flags, System.Type? context, System.Collections.Generic.IEnumerable<Microsoft.CSharp.RuntimeBinder.CSharpArgumentInfo>? argumentInfo) { throw null; }
[System.Diagnostics.CodeAnalysis.RequiresUnreferencedCodeAttribute("Using dynamic types might cause types or members to be removed by trimmer.")]
public static System.Runtime.CompilerServices.CallSiteBinder SetMember(Microsoft.CSharp.RuntimeBinder.CSharpBinderFlags flags, string name, System.Type? context, System.Collections.Generic.IEnumerable<Microsoft.CSharp.RuntimeBinder.CSharpArgumentInfo>? argumentInfo) { throw null; }
[System.Diagnostics.CodeAnalysis.RequiresUnreferencedCodeAttribute("Using dynamic types might cause types or members to be removed by trimmer.")]
public static System.Runtime.CompilerServices.CallSiteBinder UnaryOperation(Microsoft.CSharp.RuntimeBinder.CSharpBinderFlags flags, System.Linq.Expressions.ExpressionType operation, System.Type? context, System.Collections.Generic.IEnumerable<Microsoft.CSharp.RuntimeBinder.CSharpArgumentInfo>? argumentInfo) { throw null; }
}

However, I didn't want to rely on those warnings here, because someone could cast a JsonNode to IDynamicMetaObjectProvider and call GetMetaObject on it, and thus travelling down the path to creating a JsonValueNotTrimmable object.

</attribute>
</assembly>
</linker>
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ internal JsonArray (JsonElement element, JsonNodeOptions? options = null) : base
/// <param name="value">
/// The object to be added to the end of the <see cref="JsonArray"/>.
/// </param>
[RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)]
[RequiresUnreferencedCode(JsonValue.CreateUnreferencedCodeMessage)]
Copy link
Member

Choose a reason for hiding this comment

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

Due to trimming this was discussed as a method that we may want to remove, and require usage of the IList<T>.Add<T>(T value) implementation. However since that requires an extra call to JsonValue.Create(), it affects usability.

Have there been other non-JSON areas that we've discouraged API use from in this manner in favor of forcing or encouraging a trimmable API?

Copy link
Member Author

Choose a reason for hiding this comment

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

Have there been other non-JSON areas that we've discouraged API use from in this manner in favor of forcing or encouraging a trimmable API?

Yes. A main difference between JSON and those APIs is that we are actively adding features to JSON, so that's why it seems JSON is trying to be "trim-compatible" from the start. And also because any API we've already shipped we can't change. But since we are defining these JSON APIs at the same time as making the BCL trimmable, we might as well at least consider it in the API design.

Other examples include:

discouraged API use

Note though, this only matters if you care about trimmability and AOT'ing your code. If a developer is only concerned about the "current deployment models" of their code, they can easily use these APIs and their code will work. They only get warnings once they attempt to trim unused code away.

public void Add<T>(T? value)
{
if (value == null)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Diagnostics.CodeAnalysis;
using System.Dynamic;
using System.Linq.Expressions;
using System.Reflection;
Expand All @@ -10,9 +11,17 @@ namespace System.Text.Json.Nodes
public partial class JsonNode : IDynamicMetaObjectProvider
{
internal virtual MethodInfo? TryGetMemberMethodInfo => null;
internal virtual MethodInfo? TrySetMemberMethodInfo => null;
internal virtual MethodInfo? TrySetMemberMethodInfo
{
[RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)]
get => null;
}

DynamicMetaObject IDynamicMetaObjectProvider.GetMetaObject(Expression parameter) =>
new MetaDynamic(parameter, this);
CreateDynamicObject(parameter, this);

[RequiresUnreferencedCode("Using JsonNode instances as dynamic types is not compatible with trimming. It can result in non-primitive types being serialized, which may have their members trimmed.")]
private static DynamicMetaObject CreateDynamicObject(Expression parameter, JsonNode node) =>
new MetaDynamic(parameter, node);
}
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Diagnostics.CodeAnalysis;
using System.Dynamic;
using System.Reflection;

Expand All @@ -21,6 +22,7 @@ private bool TryGetMemberCallback(GetMemberBinder binder, out object? result)
return true;
}

[RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)]
private bool TrySetMemberCallback(SetMemberBinder binder, object? value)
{
JsonNode? node = null;
Expand All @@ -37,17 +39,17 @@ private bool TrySetMemberCallback(SetMemberBinder binder, object? value)
return true;
}

private static MethodInfo GetMethod(string name) => typeof(JsonObject).GetMethod(
name, BindingFlags.Instance | BindingFlags.NonPublic)!;
private const BindingFlags MemberInfoBindingFlags = BindingFlags.Instance | BindingFlags.NonPublic;

private static MethodInfo? s_TryGetMember;
internal override MethodInfo? TryGetMemberMethodInfo =>
s_TryGetMember ??
(s_TryGetMember = GetMethod(nameof(TryGetMemberCallback)));
s_TryGetMember ??= typeof(JsonObject).GetMethod(nameof(TryGetMemberCallback), MemberInfoBindingFlags);

private static MethodInfo? s_TrySetMember;
internal override MethodInfo? TrySetMemberMethodInfo =>
s_TrySetMember ??
(s_TrySetMember = GetMethod(nameof(TrySetMemberCallback)));
internal override MethodInfo? TrySetMemberMethodInfo
{
[RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)]
get => s_TrySetMember ??= typeof(JsonObject).GetMethod(nameof(TrySetMemberCallback), MemberInfoBindingFlags);
}
}
}
Loading