Use UnsafeAccessor/reflection for inaccessible members in source-generated JsonTypeInfo#124650
Use UnsafeAccessor/reflection for inaccessible members in source-generated JsonTypeInfo#124650
Conversation
…TypeInfo Remove non-required init-only properties from the constructor delegate's object initializer expression. Instead, generate real setters using UnsafeAccessor (NET8+) or cached reflection (older TFMs) so that C# default values are preserved when properties are absent from JSON. Co-authored-by: eiriktsarpalis <[email protected]>
Co-authored-by: eiriktsarpalis <[email protected]>
eiriktsarpalis
left a comment
There was a problem hiding this comment.
@copilot Let's try to broaden the scope of this PR a little bit. We can also use unsafe accessors (or the reflection fall-back) to finally enable JsonIncludeAttribute support in the source generator. If a member is marked JsonInclude but is not accessible by the current generation context, we instruct the source generator to emit getter and setter delegates using unsafe accessors.
src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs
Outdated
Show resolved
Hide resolved
…flection, JsonInclude support, rename to requiredMembers - Per-property unsafe accessor check: generic types can't use UnsafeAccessor - Lazy PropertyInfo initialization via Lazy<T> for reflection fallback - Enable JsonInclude for inaccessible members via unsafe accessors/reflection - Rename memberInitializerNames to requiredMemberNames - Add IsGenericType to TypeRef for per-property checks - Generate getter accessors for inaccessible JsonInclude properties Co-authored-by: eiriktsarpalis <[email protected]>
Broadened scope — |
…cType from TypeRef Move the per-property unsafe accessor eligibility check from the emitter to the property model. The parser now computes CanUseUnsafeAccessors by checking both UnsafeAccessorAttribute availability and whether the declaring type is generic, using the type symbol directly. This removes IsGenericType from TypeRef, IsUnsafeAccessorsSupported from ContextGenerationSpec, and the _isUnsafeAccessorsSupported field from the Emitter. Co-authored-by: eiriktsarpalis <[email protected]>
…reflection fallback Use a nullable PropertyInfo? static field with ??= lazy initialization in a helper method, following the PolyType pattern. This avoids introducing a Lazy<T> type dependency. Co-authored-by: eiriktsarpalis <[email protected]>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs
Outdated
Show resolved
Hide resolved
Replace PropertyInfo caching with Func/Action delegate caching for the reflection fallback path. Instead of calling PropertyInfo.GetValue/SetValue on each access, we now cache PropertyInfo.GetValue and PropertyInfo.SetValue as Func<object?, object?> and Action<object?, object?> delegates respectively, which provides faster subsequent access. Co-authored-by: eiriktsarpalis <[email protected]>
The parser no longer emits SYSLIB1222 for inaccessible constructors since they are now supported via UnsafeAccessor or reflection fallback. Update TypesWithJsonConstructorAnnotations_WarnAsExpected to expect no diagnostics, matching the pattern used for inaccessible [JsonInclude] members. Co-authored-by: Copilot <[email protected]>
These diagnostics are no longer emitted since the source generator now supports inaccessible members via UnsafeAccessor or reflection fallback: - SYSLIB1037: Init-only property deserialization (already removed, orphaned resources) - SYSLIB1038: Inaccessible [JsonInclude] members - SYSLIB1222: Inaccessible [JsonConstructor] constructors Removes diagnostic descriptors, exception message constants, resx strings, XLF translations, and test warning suppressions. Co-authored-by: Copilot <[email protected]>
|
@copilot please update the OP to include samples of source generated unsafe accessors in action. Include samples both for unsafe accessors and the reflection-based fallback. Include getters, setters, and constructors. |
Updated the PR description with detailed generated code samples showing:
|
Adds TypeWithInitOnlyAndRequiredMembers_OnlyRequiredHasAssociatedParameterInfo test that checks init-only properties do not get an AssociatedParameter while required properties do in source-gen mode. Co-authored-by: Copilot <[email protected]>
UnsafeAccessor externs are now invoked directly from delegate lambdas instead of going through an intermediate wrapper that upcasts/downcasts between object? and the concrete type. Reflection fallback wrappers are now strongly typed to match. Also removes the redundant (PropertyType) cast on the setter value parameter since JsonPropertyInfoValues<T>.Setter is Action<object, T?>, meaning value is already T. Co-authored-by: Copilot <[email protected]>
f4ee211 to
81bdd14
Compare
Only append the property index suffix (_0, _1, etc.) to accessor method names when there are duplicate member names in the property list, which happens when derived types shadow base members via 'new'. Co-authored-by: Copilot <[email protected]>
For the reflection fallback path (generic types where UnsafeAccessor is not available), use Delegate.CreateDelegate on the property's MethodInfo instead of caching PropertyInfo.GetValue/SetValue. This avoids per-call reflection dispatch overhead. For value type setters, a bespoke ValueTypeSetter<TDeclaringType, TValue> delegate with ref parameter is defined once in the context class, used with Unsafe.Unbox to mutate the boxed value in-place. Fields still use FieldInfo.GetValue/SetValue since they don't have MethodInfo. Co-authored-by: Copilot <[email protected]>
1227f49 to
29319c5
Compare
|
@copilot apply the code review skill against the changes. |
There was a problem hiding this comment.
Pull request overview
This pull request enables the source generator to support inaccessible members by using [UnsafeAccessor] (NET8+ for non-generic types) or cached reflection delegates as fallback. The PR addresses three previously unsupported scenarios:
Changes:
- Init-only properties now preserve default values when not present in JSON, by setting them post-construction via UnsafeAccessor/reflection instead of including them in constructor object initializers
[JsonInclude]properties with inaccessible getters/setters are now fully supported instead of throwingInvalidOperationException[JsonConstructor]constructors that are inaccessible are now invoked via UnsafeAccessor/reflection instead of being ignored with SYSLIB1222 warning
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| JsonSourceGeneratorDiagnosticsTests.cs | Updated tests to expect no diagnostics for inaccessible [JsonInclude] and [JsonConstructor] members |
| System.Text.Json.SourceGeneration.Tests.targets | Removed SYSLIB1037, SYSLIB1038, SYSLIB1222 from NoWarn list as these diagnostics no longer apply |
| PropertyVisibilityTests.cs (source gen) | Changed tests to call base implementations instead of expecting exceptions for inaccessible members; added new test method for NonPublicProperty_JsonInclude_WorksAsExpected |
| PropertyVisibilityTests.NonPublicAccessors.cs | Added extensive new test types and methods for private/protected/internal [JsonInclude] properties and fields, init-only properties with defaults, and struct scenarios |
| PropertyVisibilityTests.InitOnly.cs | Added tests for init-only property default value preservation |
| MetadataTests.cs | Split test into two - one for required members (which have AssociatedParameter) and one for init-only members (which no longer have AssociatedParameter); added test for mixed init-only and required members |
| ConstructorTests.AttributePresence.cs | Removed isAccessibleBySourceGen parameter - all inaccessible constructors with [JsonConstructor] now work |
| JsonMetadataServices.Helpers.cs | Updated validation to allow non-public [JsonInclude] properties when getter/setter delegates are provided by the generator |
| Strings.*.xlf (all languages) | Removed localized strings for SYSLIB1037, SYSLIB1038, SYSLIB1222 diagnostics |
| Strings.resx | Removed diagnostic resource strings for init-only and inaccessible [JsonInclude] and [JsonConstructor] warnings |
| TypeGenerationSpec.cs | Added ConstructorIsInaccessible and CanUseUnsafeAccessorForConstructor properties |
| PropertyGenerationSpec.cs | Added CanUseUnsafeAccessors property; updated ShouldIncludePropertyForFastPath to include inaccessible [JsonInclude] properties |
| JsonSourceGenerator.Parser.cs | Changed constructor handling to track inaccessibility instead of nulling out inaccessible constructors; renamed memberInitializerNames to requiredMemberNames and excluded init-only properties; removed fast-path invalidation for inaccessible [JsonInclude] |
| JsonSourceGenerator.Emitter.cs | CRITICAL BUG: Field reflection fallback implementation (lines 973-987) is broken and won't compile - attempts to assign instance methods to variables. Property accessors (UnsafeAccessor and reflection) are correctly implemented. Constructor accessors are correctly implemented. |
| JsonSourceGenerator.Emitter.ExceptionMessages.cs | Removed obsolete exception messages |
| JsonSourceGenerator.DiagnosticDescriptors.cs | Removed SYSLIB1038 and SYSLIB1222 diagnostic descriptors |
| KnownTypeSymbols.cs | Added detection for UnsafeAccessorAttribute availability |
| writer.WriteLine($"private static global::System.Func<object?, object?>? {cacheName};"); | ||
| writer.WriteLine($"private static {propertyTypeFQN} {wrapperName}(object obj) => ({propertyTypeFQN})({cacheName} ??= {fieldExpr}.GetValue)(obj)!;"); | ||
| } | ||
|
|
||
| if (needsSetterAccessor) | ||
| { | ||
| string cacheName = GetReflectionCacheName(typeFriendlyName, "set", property.MemberName, i, disambiguate); | ||
| string wrapperName = GetAccessorName(typeFriendlyName, "set", property.MemberName, i, disambiguate); | ||
| writer.WriteLine($"private static global::System.Action<object?, object?>? {cacheName};"); | ||
| writer.WriteLine($"private static void {wrapperName}(object obj, {propertyTypeFQN} value) => ({cacheName} ??= {fieldExpr}.SetValue)(obj, value);"); |
There was a problem hiding this comment.
The reflection fallback code for field accessors is incorrect and will not compile.
At lines 977-978 (field getter):
writer.WriteLine($"private static global::System.Func<object?, object?>? {cacheName};");
writer.WriteLine($"private static {propertyTypeFQN} {wrapperName}(object obj) => ({propertyTypeFQN})({cacheName} ??= {fieldExpr}.GetValue)(obj)!;");This generates code like:
private static Func<object?, object?>? s_get_TypeName_fieldName;
private static PropertyType __get_TypeName_fieldName(object obj) =>
(PropertyType)(s_get_TypeName_fieldName ??= typeof(DeclaringType).GetField("fieldName", ...).GetValue)(obj)!;The problem is that FieldInfo.GetValue is an instance method, not a delegate that can be assigned to a variable. The expression {cacheName} ??= {fieldExpr}.GetValue attempts to assign the GetValue method directly, which is invalid C# syntax and won't compile.
Unlike PropertyInfo.GetMethod() which returns a MethodInfo that can be wrapped in a delegate via Delegate.CreateDelegate, FieldInfo doesn't have a GetMethod() equivalent. Fields must be accessed via FieldInfo.GetValue(object) and FieldInfo.SetValue(object, object) directly.
The correct approach is to cache the FieldInfo itself and call its methods:
private static FieldInfo? s_field_TypeName_fieldName;
private static PropertyType __get_TypeName_fieldName(object obj) =>
(PropertyType)(s_field_TypeName_fieldName ??= typeof(DeclaringType).GetField("fieldName", ...))!.GetValue(obj)!;
private static void __set_TypeName_fieldName(object obj, PropertyType value) =>
(s_field_TypeName_fieldName ??= typeof(DeclaringType).GetField("fieldName", ...))!.SetValue(obj, value);The same issue exists for field setters at lines 985-986.
| writer.WriteLine($"private static global::System.Func<object?, object?>? {cacheName};"); | |
| writer.WriteLine($"private static {propertyTypeFQN} {wrapperName}(object obj) => ({propertyTypeFQN})({cacheName} ??= {fieldExpr}.GetValue)(obj)!;"); | |
| } | |
| if (needsSetterAccessor) | |
| { | |
| string cacheName = GetReflectionCacheName(typeFriendlyName, "set", property.MemberName, i, disambiguate); | |
| string wrapperName = GetAccessorName(typeFriendlyName, "set", property.MemberName, i, disambiguate); | |
| writer.WriteLine($"private static global::System.Action<object?, object?>? {cacheName};"); | |
| writer.WriteLine($"private static void {wrapperName}(object obj, {propertyTypeFQN} value) => ({cacheName} ??= {fieldExpr}.SetValue)(obj, value);"); | |
| writer.WriteLine($"private static global::System.Reflection.FieldInfo? {cacheName};"); | |
| writer.WriteLine($"private static {propertyTypeFQN} {wrapperName}(object obj) => ({propertyTypeFQN})(({cacheName} ??= {fieldExpr}).GetValue(obj))!;"); | |
| } | |
| if (needsSetterAccessor) | |
| { | |
| string cacheName = GetReflectionCacheName(typeFriendlyName, "set", property.MemberName, i, disambiguate); | |
| string wrapperName = GetAccessorName(typeFriendlyName, "set", property.MemberName, i, disambiguate); | |
| writer.WriteLine($"private static global::System.Reflection.FieldInfo? {cacheName};"); | |
| writer.WriteLine($"private static void {wrapperName}(object obj, {propertyTypeFQN} value) => ({cacheName} ??= {fieldExpr}).SetValue(obj, value);"); |
Code review complete. Summary: Key findings:
No blocking issues found. The inaccessible constructor + required properties edge case is advisory. |
Description
The source generator uses
[UnsafeAccessor](NET8+ non-generic types) or cached reflection delegates (older targets/generic types) to access inaccessible members, enabling three previously unsupported scenarios:1. Preserve default values for init-only properties
The source generator previously set init-only properties via the constructor delegate's object initializer expression, even when absent from JSON. This replaced C# property initializer defaults with
default(T):Init-only properties are now removed from the constructor delegate and set post-construction via UnsafeAccessor or reflection, only when present in JSON.
2. Enable
[JsonInclude]for inaccessible propertiesPreviously,
[JsonInclude]properties with inaccessible getters or setters generated throwing delegates. They are now fully supported:3. Enable
[JsonConstructor]for inaccessible constructorsPreviously, inaccessible constructors marked with
[JsonConstructor]were ignored with a SYSLIB1222 warning. They are now invoked via[UnsafeAccessor(UnsafeAccessorKind.Constructor)]or cachedConstructorInfo.Invokereflection fallback:Generated Code Samples
UnsafeAccessor path (NET8+ non-generic types)
For a type with a private
[JsonInclude]property and an init-only property:The source generator emits
[UnsafeAccessor]extern methods that are invoked directly from the getter/setter delegates — no intermediate wrapper methods.JsonPropertyInfoValues<T>is generic on the property type, so theSetterdelegate isAction<object, T?>and theGetterisFunc<object, T?>. The only cast in each delegate is on theobjectparameter to the declaring type:Property getter (inaccessible
[JsonInclude])Property setter (inaccessible
[JsonInclude])Init-only property setter
Constructor (inaccessible
[JsonConstructor])Value types (structs)
For struct types, the extern uses
refand the delegate passesref Unsafe.Unbox<T>(obj):Reflection fallback (older targets or generic types)
When
[UnsafeAccessor]is not available (e.g., netstandard2.0 targets) or the declaring type is generic (not supported by UnsafeAccessor), the generator falls back to cached reflection with strongly typed wrappers:Property getter (reflection)
Property setter (reflection)
Value type setter (reflection, struct)
For value type property setters, a bespoke
ValueTypeSetter<TDeclaringType, TValue>delegate withrefparameter is emitted once in the context class and used withUnsafe.Unboxto mutate the boxed value in-place:Constructor (reflection)
Note: Unlike
PropertyInfo,ConstructorInfocannot be wrapped in aFunc<>delegate, so theConstructorInfoitself is cached:Changes
Parser: Non-required init-only properties removed from
PropertyInitializerSpecs(renamedmemberInitializerNamestorequiredMemberNames). Inaccessible[JsonInclude]properties no longer flagged asHasInvalidConfigurationForFastPath. Inaccessible[JsonConstructor]constructors no longer nulled out — tracked as inaccessible for the emitter to handle.PropertyGenerationSpec: Added
CanUseUnsafeAccessors(per-property, checksUnsafeAccessorAttributeavailability and non-generic declaring type),NeedsAccessorForGetter, andNeedsAccessorForSetter.TypeGenerationSpec: Added
ConstructorIsInaccessibleandCanUseUnsafeAccessorForConstructorfor inaccessible constructor support.Emitter: Unified accessor pattern — both UnsafeAccessor and reflection fallback generate wrapper methods with identical names and strongly typed signatures, so call sites don't need to differentiate between the two paths. UnsafeAccessor externs are implementation details wrapped by the unified method.
[UnsafeAccessor(UnsafeAccessorKind.Method, Name = "get_X")]/"set_X"withref Unsafe.Unbox<T>(obj)for value types, or cachedFunc<TDeclaringType, TValue>/Action<TDeclaringType, TValue>delegates viaDelegate.CreateDelegateonMethodInfo. For struct setters, a bespokeValueTypeSetter<TDeclaringType, TValue>delegate withrefparameter is emitted.[UnsafeAccessor(UnsafeAccessorKind.Constructor)]or cachedConstructorInfo.Invoke(note:ConstructorInfocannot be wrapped in a delegate unlikeMethodInfo, so theConstructorInfoitself is cached).[JsonInclude]properties included in the fast path using UnsafeAccessor-generated getters.KnownTypeSymbols: Detect
UnsafeAccessorAttributeavailability in the target compilation.Runtime validation (
JsonMetadataServices.Helpers.cs):PopulatePropertiesnow allows non-public[JsonInclude]properties when getter or setter delegates are provided. Only throws when both are null (backward-compatible with older generators).Diagnostics:
InaccessibleJsonIncludePropertiesNotSupported(SYSLIB1038) no longer emitted. Inaccessible[JsonConstructor]no longer triggers SYSLIB1222.Tests:
InvalidOperationExceptionfor private/protected[JsonInclude]members and inaccessible[JsonConstructor]constructors.ClassWithPrivateJsonIncludeProperties_Roundtrip,ClassWithProtectedJsonIncludeProperties_Roundtrip,ClassWithMixedAccessibilityJsonIncludeProperties,ClassWithJsonIncludePrivateInitOnlyProperties,ClassWithJsonIncludePrivateGetterProperties,StructWithJsonIncludePrivateProperties.MetadataTeststo reflect that init-only properties no longer have anAssociatedParameter.💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.