Skip to content

Replace Unsafe.Unbox with StrongBox for JSON source-generated struct accessors #133997

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

Open
jkoritzinsky wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Replace Unsafe.Unbox with StrongBox for JSON source-generated struct accessors #133997

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d609dfd
Replace Unsafe.Unbox with StrongBox for JSON source-generated struct …
jkoritzinsky Sep 15, 2026
a46a31c
Address PR feedback on StrongBox struct deserialization and fix CI co…
jkoritzinsky Sep 15, 2026
8a9c9c0
Preserve legacy Get/Set contract for StrongBox-wrapped struct instances
jkoritzinsky Sep 16, 2026
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
97 changes: 67 additions & 30 deletions src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ private sealed partial class Emitter
private const string InvalidOperationExceptionTypeRef = "global::System.InvalidOperationException";
private const string JsonExceptionTypeRef = "global::System.Text.Json.JsonException";
private const string TypeTypeRef = "global::System.Type";
private const string UnsafeTypeRef = "global::System.Runtime.CompilerServices.Unsafe";
private const string StrongBoxTypeRef = "global::System.Runtime.CompilerServices.StrongBox";
private const string EqualityComparerTypeRef = "global::System.Collections.Generic.EqualityComparer";
private const string KeyValuePairTypeRef = "global::System.Collections.Generic.KeyValuePair";
private const string UnsafeAccessorAttributeTypeRef = "global::System.Runtime.CompilerServices.UnsafeAccessorAttribute";
Expand Down Expand Up @@ -643,7 +643,7 @@ private SourceText GenerateForObject(ContextGenerationSpec contextSpec, TypeGene
if (propInitMethodName != null)
{
writer.WriteLine();
GeneratePropMetadataInitFunc(writer, contextSpec, propInitMethodName, typeMetadata);
GeneratePropMetadataInitFunc(writer, propInitMethodName, typeMetadata);
}

if (serializeMethodName != null)
Expand Down Expand Up @@ -826,7 +826,7 @@ private static string FormatNullCast(UnionCaseSpec caseSpec)
: $"({fqn}?)";
}

private void GeneratePropMetadataInitFunc(SourceWriter writer, ContextGenerationSpec contextSpec, string propInitMethodName, TypeGenerationSpec typeGenerationSpec)
private void GeneratePropMetadataInitFunc(SourceWriter writer, string propInitMethodName, TypeGenerationSpec typeGenerationSpec)
{
ImmutableEquatableArray<PropertyGenerationSpec> properties = typeGenerationSpec.PropertyGenSpecs;
HashSet<string> duplicateMemberNames = GetDuplicateMemberNames(properties);
Expand Down Expand Up @@ -855,8 +855,8 @@ property.DefaultIgnoreCondition is JsonIgnoreCondition.Always &&

string propertyTypeFQN = isIgnoredPropertyOfUnusedType ? "object" : property.PropertyType.FullyQualifiedName;

string getterValue = GetPropertyGetterValue(contextSpec, property, typeGenerationSpec, propertyName, declaringTypeFQN, i, duplicateMemberNames.Contains(property.MemberName));
string setterValue = GetPropertySetterValue(contextSpec, property, typeGenerationSpec, propertyName, declaringTypeFQN, i, duplicateMemberNames.Contains(property.MemberName));
string getterValue = GetPropertyGetterValue(property, typeGenerationSpec, propertyName, declaringTypeFQN, i, duplicateMemberNames.Contains(property.MemberName));
string setterValue = GetPropertySetterValue(property, typeGenerationSpec, propertyName, declaringTypeFQN, i, duplicateMemberNames.Contains(property.MemberName));

string ignoreConditionNamedArg = property.DefaultIgnoreCondition.HasValue
? $"{JsonIgnoreConditionTypeRef}.{property.DefaultIgnoreCondition.Value}"
Expand Down Expand Up @@ -969,14 +969,7 @@ private static bool NeedsAccessorForSetter(PropertyGenerationSpec property)
return false;
}

private static string GetUnboxExpression(ContextGenerationSpec contextSpec, string declaringTypeFQN)
{
string expression = $"{UnsafeTypeRef}.Unbox<{declaringTypeFQN}>(obj)";
return contextSpec.UseUpdatedMemorySafetyRules ? $"unsafe({expression})" : expression;
}

private static string GetPropertyGetterValue(
ContextGenerationSpec contextSpec,
PropertyGenerationSpec property,
TypeGenerationSpec typeGenerationSpec,
string propertyName,
Expand All @@ -991,7 +984,11 @@ private static string GetPropertyGetterValue(

if (property.CanUseGetter)
{
return $"static obj => (({declaringTypeFQN})obj).{propertyName}";
// For value types, the getter may receive a StrongBox<T> during deserialization (e.g. for populated properties or callbacks)
// or a boxed T during serialization.
return typeGenerationSpec.TypeRef.IsValueType
? $"static obj => (obj is {StrongBoxTypeRef}<{declaringTypeFQN}> box ? box.Value : ({declaringTypeFQN})obj).{propertyName}"
: $"static obj => (({declaringTypeFQN})obj).{propertyName}";
}

if (NeedsAccessorForGetter(property))
Expand All @@ -1000,33 +997,39 @@ private static string GetPropertyGetterValue(

if (property.CanUseUnsafeAccessors)
{
// UnsafeAccessor externs for value types take 'ref T'.
string castExpr = typeGenerationSpec.TypeRef.IsValueType
? $"ref {GetUnboxExpression(contextSpec, declaringTypeFQN)}"
: $"({declaringTypeFQN})obj";

string accessorName = property.IsProperty
? GetQualifiedAccessorName(property, typeFriendlyName, "get", property.MemberName, propertyIndex, needsDisambiguation)
: GetQualifiedAccessorName(property, typeFriendlyName, "field", property.MemberName, propertyIndex, needsDisambiguation);

return $"static obj => {accessorName}({castExpr})";
// Value types pass ref StrongBox<T>.Value during deserialization or ref temp during serialization.
if (typeGenerationSpec.TypeRef.IsValueType)
{
return $"static obj => {{ if (obj is {StrongBoxTypeRef}<{declaringTypeFQN}> box) return {accessorName}(ref box.Value); var temp = ({declaringTypeFQN})obj; return {accessorName}(ref temp); }}";
}

return $"static obj => {accessorName}(({declaringTypeFQN})obj)";
}

string getterName = GetAccessorName(typeFriendlyName, "get", property.MemberName, propertyIndex, needsDisambiguation);
if (!property.IsProperty)
{
return $"static obj => {getterName}(obj)";
// Value types can be passed as StrongBox<T> during deserialization or boxed T during serialization.
return typeGenerationSpec.TypeRef.IsValueType
? $"static obj => {getterName}(obj is {StrongBoxTypeRef}<{declaringTypeFQN}> box ? box.Value : obj)"
: $"static obj => {getterName}(obj)";
}

// Reflection fallback property wrappers are strongly typed; cast in the delegate.
return $"static obj => {getterName}(({declaringTypeFQN})obj)";
// Value types can be passed as StrongBox<T> during deserialization or boxed T during serialization.
return typeGenerationSpec.TypeRef.IsValueType
? $"static obj => {getterName}(obj is {StrongBoxTypeRef}<{declaringTypeFQN}> box ? box.Value : ({declaringTypeFQN})obj)"
: $"static obj => {getterName}(({declaringTypeFQN})obj)";
}

return "null";
}

private static string GetPropertySetterValue(
ContextGenerationSpec contextSpec,
PropertyGenerationSpec property,
TypeGenerationSpec typeGenerationSpec,
string propertyName,
Expand All @@ -1041,19 +1044,19 @@ private static string GetPropertySetterValue(

if (property is { CanUseSetter: true, IsInitOnlySetter: true })
{
return GetAccessorBasedSetterDelegate(contextSpec, property, typeGenerationSpec, declaringTypeFQN, propertyIndex, needsDisambiguation);
return GetAccessorBasedSetterDelegate(property, typeGenerationSpec, declaringTypeFQN, propertyIndex, needsDisambiguation);
}

if (property.CanUseSetter)
{
return typeGenerationSpec.TypeRef.IsValueType
? $"""static (obj, value) => {GetUnboxExpression(contextSpec, declaringTypeFQN)}.{propertyName} = value!"""
? $"""static (obj, value) => (({StrongBoxTypeRef}<{declaringTypeFQN}>)obj).Value.{propertyName} = value!"""

@eiriktsarpalis eiriktsarpalis Sep 16, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I concur, this is a breaking change unfortunately. Users can access, modify, or wrap the delegate directly via the JsonPropertyInfo.Set property and anybody doing so today is relying on the current implicit contract.

@jkoritzinsky jkoritzinsky Sep 16, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Addressed in 8a9c9c0 without changing JsonPropertyInfo.Get/.Set's existing contract or adding new public API.

JsonPropertyInfo<T> now lazily detects, per property, whether the assigned Get/Set delegate understands a StrongBox<TDeclaringType>-wrapped obj: it tries the delegate directly first, and on InvalidCastException falls back to a legacy unbox/invoke/rebox path (using the pre-existing public System.Runtime.CompilerServices.IStrongBox interface, which StrongBox<T> implements). The result is cached per property so the fallback only triggers once. This transparently covers:

  • source-gen assemblies built before StrongBox-based struct accessors existed, and
  • any resolver-modifier-supplied delegate written against the historical "obj is a boxed TDeclaringType" contract (the exact scenario you flagged) - including one captured/wrapped from JsonPropertyInfo.Get/.Set and reassigned.

Since the public untyped Get/Set properties must keep returning the exact delegate instance a caller assigned (there's an existing test enforcing that), the compatibility handling lives behind the internal typed accessor instead, and the two internal call sites that previously invoked the untyped Get/Set directly (TryGetPrePopulatedValue, extension-data population) now go through new GetValueAsObject/SetValueAsObject methods so they get the same handling.

No emitter changes were needed - new-gen's StrongBox-aware delegates always succeed on the fast path.

Note

This reply was generated with AI assistance.

Auto-replied by the GitHub Copilot app

@eiriktsarpalis eiriktsarpalis Sep 17, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This still breaks callers of the JsonPropertyInfo.Set delegate populated by the source generator. For example, this console app uses a contract modifier to trim string properties after deserialization. It invokes the generated Get/Set delegates without replacing them:

using System.Text.Json;
using System.Text.Json.Serialization;
using System.Text.Json.Serialization.Metadata;

JsonSerializerOptions options = new()
{
    TypeInfoResolver = AppJsonContext.Default.WithAddedModifier(TrimStringProperties)
};

Contact contact = JsonSerializer.Deserialize<Contact>(
    """{"Name":"  Alice  ","Email":"  alice@example.com  "}""", options);

Console.WriteLine($"Name: '{contact.Name}'");
Console.WriteLine($"Email: '{contact.Email}'");

static void TrimStringProperties(JsonTypeInfo typeInfo)
{
    if (typeInfo.Kind is not JsonTypeInfoKind.Object)
    {
        return;
    }

    JsonPropertyInfo[] properties = typeInfo.Properties
        .Where(p => p.PropertyType == typeof(string) && p.Get is not null && p.Set is not null)
        .ToArray();

    Action<object>? originalCallback = typeInfo.OnDeserialized;
    typeInfo.OnDeserialized = obj =>
    {
        originalCallback?.Invoke(obj);

        foreach (JsonPropertyInfo property in properties)
        {
            if (property.Get!(obj) is string value)
            {
                property.Set!(obj, value.Trim());
            }
        }
    };
}

public struct Contact
{
    public string? Name { get; set; }
    public string? Email { get; set; }
}

[JsonSerializable(typeof(Contact))]
internal partial class AppJsonContext : JsonSerializerContext;

Before the change, this prints:

Name: 'Alice'
Email: 'alice@example.com'

After rebuilding with the generator and library from 8a9c9c0, it throws at property.Set!(obj, value.Trim()):

System.InvalidCastException: Unable to cast object of type 'Contact'
to type 'System.Runtime.CompilerServices.StrongBox`1[Contact]'.

I don't think this would be possible to fix without keeping our dependency on Unsafe.Unbox.

: $"""static (obj, value) => (({declaringTypeFQN})obj).{propertyName} = value!""";
}

if (NeedsAccessorForSetter(property))
{
return GetAccessorBasedSetterDelegate(contextSpec, property, typeGenerationSpec, declaringTypeFQN, propertyIndex, needsDisambiguation);
return GetAccessorBasedSetterDelegate(property, typeGenerationSpec, declaringTypeFQN, propertyIndex, needsDisambiguation);
}

return "null";
Expand All @@ -1064,7 +1067,6 @@ private static string GetPropertySetterValue(
/// or the strongly typed reflection wrapper.
/// </summary>
private static string GetAccessorBasedSetterDelegate(
ContextGenerationSpec contextSpec,
PropertyGenerationSpec property,
TypeGenerationSpec typeGenerationSpec,
string declaringTypeFQN,
Expand All @@ -1076,7 +1078,7 @@ private static string GetAccessorBasedSetterDelegate(
if (property.CanUseUnsafeAccessors)
{
string castExpr = typeGenerationSpec.TypeRef.IsValueType
? $"ref {GetUnboxExpression(contextSpec, declaringTypeFQN)}"
? $"ref (({StrongBoxTypeRef}<{declaringTypeFQN}>)obj).Value"
: $"({declaringTypeFQN})obj";

if (property.IsProperty)
Expand All @@ -1097,7 +1099,7 @@ private static string GetAccessorBasedSetterDelegate(

// Reflection fallback property wrappers are strongly typed; cast in the delegate like UnsafeAccessor.
string setterCastExpr = typeGenerationSpec.TypeRef.IsValueType
? $"ref {GetUnboxExpression(contextSpec, declaringTypeFQN)}"
? $"ref (({StrongBoxTypeRef}<{declaringTypeFQN}>)obj).Value"
: $"({declaringTypeFQN})obj";

return $"static (obj, value) => {setterName}({setterCastExpr}, value!)";
Expand Down Expand Up @@ -1239,13 +1241,48 @@ private static bool GeneratePropertyAccessors(SourceWriter writer, ContextGenera
if (needsGetterAccessor)
{
string wrapperName = GetAccessorName(typeFriendlyName, "get", property.MemberName, i, disambiguate);
writer.WriteLine($"private static {propertyTypeFQN} {wrapperName}(object obj) => ({propertyTypeFQN})({fieldCacheName} ??= {fieldExpr}).GetValue(obj)!;");
if (typeGenerationSpec.TypeRef.IsValueType)
{
// Value types can be passed as StrongBox<T> during deserialization or boxed T during serialization.
writer.WriteLine($"private static {propertyTypeFQN} {wrapperName}(object obj) => ({propertyTypeFQN})({fieldCacheName} ??= {fieldExpr}).GetValue(obj is {StrongBoxTypeRef}<{declaringTypeFQN}> box ? box.Value : obj)!;");
}
else
{
writer.WriteLine($"private static {propertyTypeFQN} {wrapperName}(object obj) => ({propertyTypeFQN})({fieldCacheName} ??= {fieldExpr}).GetValue(obj)!;");
}
}

if (needsSetterAccessor)
{
string wrapperName = GetAccessorName(typeFriendlyName, "set", property.MemberName, i, disambiguate);
writer.WriteLine($"private static void {wrapperName}(object obj, {propertyTypeFQN} value) => ({fieldCacheName} ??= {fieldExpr}).SetValue(obj, value);");
if (typeGenerationSpec.TypeRef.IsValueType)
{
writer.WriteLine($"private static void {wrapperName}(object obj, {propertyTypeFQN} value)");
writer.WriteLine('{');
writer.Indentation++;
// Value types are wrapped in StrongBox<T> during deserialization so mutating box.Value persists.
// If called on an unboxed or directly boxed struct instance, set directly on obj.
writer.WriteLine($"if (obj is {StrongBoxTypeRef}<{declaringTypeFQN}> box)");
writer.WriteLine('{');
writer.Indentation++;
writer.WriteLine("object boxed = box.Value;");
writer.WriteLine($"({fieldCacheName} ??= {fieldExpr}).SetValue(boxed, value);");
writer.WriteLine($"box.Value = ({declaringTypeFQN})boxed;");
writer.Indentation--;
writer.WriteLine('}');
writer.WriteLine("else");
writer.WriteLine('{');
writer.Indentation++;
writer.WriteLine($"({fieldCacheName} ??= {fieldExpr}).SetValue(obj, value);");
writer.Indentation--;
writer.WriteLine('}');
writer.Indentation--;
writer.WriteLine('}');
}
else
{
writer.WriteLine($"private static void {wrapperName}(object obj, {propertyTypeFQN} value) => ({fieldCacheName} ??= {fieldExpr}).SetValue(obj, value);");
}
}
}
}
Expand Down
Loading
Loading