[teo-tsirpanis]

This PR updates System.Text.Json's reflection member accesor, to use use the MethodInvoker and ConstructorInvoker APIs on supported frameworks. This is faster, and does not wrap exceptions in TargetInvocationException.

I also replaced one use of checking whether an attribute exists with IsDefined.

[azure-pipelines]

Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/area-system-text-json
See info in area-owners.md if you want to be subscribed.

[Copilot]

Pull request overview

This PR updates System.Text.Json’s reflection-based MemberAccessor implementation to use MethodInvoker/ConstructorInvoker when targeting NET, and adjusts the internal parameterized-constructor delegate signature to Func<object?[], T>. It also switches a JsonConstructorAttribute presence check to IsDefined to avoid allocating the attribute instance.

Changes:

• Use ConstructorInvoker/MethodInvoker in ReflectionMemberAccessor (#if NET) for faster invocation paths.
• Update MemberAccessor.CreateParameterizedConstructor<T> and related emit/caching implementations to return Func<object?[], T>.
• Replace GetCustomAttribute<JsonConstructorAttribute>() != null with IsDefined(...).

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionMemberAccessor.cs	Switches reflection invocations to invokers on NET; adjusts constructor/property/add delegates.
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionEmitMemberAccessor.cs	Updates parameterized-constructor delegate signature to Func<object?[], T>.
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionEmitCachingMemberAccessor.cs	Updates cached parameterized-constructor delegate signature to Func<object?[], T>.
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/MemberAccessor.cs	Changes abstract API to Func<object?[], T> for parameterized constructors.
src/libraries/System.Text.Json/src/System/ReflectionExtensions.cs	Uses IsDefined for JsonConstructorAttribute presence check.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

[azure-pipelines]

Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionMemberAccessor.cs:56

• CreateParameterizedConstructor currently slices/copies parameterCount elements from the pooled arguments array and passes them directly to ConstructorInvoker/ConstructorInfo.Invoke. However, System.Text.Json deliberately skips out parameters when building the ctor-argument array (see DefaultJsonTypeInfoResolver.Helpers.cs:672-714), and the Reflection.Emit accessor compensates via an explicit ctor-param-index -> args-index mapping (ReflectionEmitMemberAccessor.cs:94-113). On platforms that use ReflectionMemberAccessor (e.g., !IsDynamicCodeCompiled), parameterCount includes out parameters, so this will read uninitialized pooled elements (or even go out of range) and then fail in invoker argument checking / parameter-count validation.

Please expand the packed non-out argument array into a full-length argument list (inserting null placeholders for out params) before invoking when any out parameters are present, keeping the existing no-out fast path.

            int parameterCount = constructor.GetParameters().Length;

#if NET
            ConstructorInvoker invoker = ConstructorInvoker.Create(constructor);
            return arguments => (T)invoker.Invoke(arguments.AsSpan(0, parameterCount));

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
  "version": 5,
  "last_dispatched_commit": "e94afd467676736c5cd4bdd64d1ce2fcf6d2b23f",
  "last_dispatched_base_ref": "main",
  "last_dispatched_base_sha": "13c03a4a9a854291f77636b1e15038e65919e68b",
  "last_reviewed_commit": "e94afd467676736c5cd4bdd64d1ce2fcf6d2b23f",
  "last_reviewed_base_ref": "main",
  "last_reviewed_base_sha": "13c03a4a9a854291f77636b1e15038e65919e68b",
  "last_recorded_worker_run_id": "29689457976",
  "review_attempt_commit": "",
  "review_attempt_base_ref": "",
  "review_attempt_count": 0,
  "max_review_attempts": 5,
  "review_history_format": "holistic-review-disclosure-v1",
  "review_history": [
    {
      "commit": "e94afd467676736c5cd4bdd64d1ce2fcf6d2b23f",
      "review_id": 4730875547
    }
  ]
}

[github-actions]

Holistic Review

Motivation: The PR is well justified. System.Text.Json's reflection-based member accessor previously routed every constructor/method/property access through MethodInfo.Invoke/ConstructorInfo.InvokeNoWrapExceptions, which allocate argument arrays and are slower than the modern MethodInvoker/ConstructorInvoker APIs. Using these on supported frameworks is a real, measurable improvement.

Approach: Sound and idiomatic. Fast paths are guarded with #if NET and fall back to the existing InvokeNoWrapExceptions path on netstandard2.0/netfx, preserving the important "no TargetInvocationException wrapping" semantics (both ConstructorInvoker.Invoke and MethodInvoker.Invoke propagate the original exception, matching InvokeNoWrapExceptions). The value-type property getter now binds an open-instance ref-receiver delegate instead of boxing through Invoke, and the IsDefined swap avoids materializing an attribute instance. Scope is tight and internal-only — no public API surface is added.

Summary: ✅ LGTM. The refactor preserves observable behavior (exception propagation, parameter-count handling, null argument passing) while reducing allocations and invocation overhead. I verified the only caller of the two-generic CreatePropertyGetter<TDeclaringType, TProperty> and the #if NET framework floor; no correctness concerns found. See the minor observation below.

---

Detailed Findings

✅ Exception semantics preserved

The legacy path used InvokeNoWrapExceptions specifically so exceptions (including the eight-element tuple ArgumentException) surface unwrapped. ConstructorInvoker.Invoke/MethodInvoker.Invoke also do not wrap in TargetInvocationException, so behavior is preserved on the #if NET path. Good.

✅ Parameterized constructor slicing is correct

In CreateParameterizedConstructor<T>, the input array is rented from a shared pool and may be larger than the parameter count. arguments.AsSpan(0, parameterCount) correctly bounds the span to exactly the constructor's parameters, matching the old explicit copy loop. The goto case fall-through rewrite in the 4-arg overload's #else branch is equivalent to the original per-index assignment.

✅ Value-type property getter binding

CreatePropertyGetter<TDeclaringType, TProperty> now binds an open-instance delegate. For value types it uses a ref TDeclaringType receiver delegate (ValueTypePropertyGetter) and wraps it, which is the correct way to bind an instance getter on a struct without boxing; for reference types it binds Func<TDeclaringType, TProperty> directly. This is the standard runtime pattern and matches the emit-based accessor's intent.

💡 Latent constraint on the two-generic getter (non-blocking)

Switching to CreateDelegate makes CreatePropertyGetter<TDeclaringType, TProperty> stricter than the old Invoke+cast: Delegate.CreateDelegate does not permit value-type-to-object boxing return conversions. This is safe today because the sole caller (DefaultJsonTypeInfoResolver.Union.CreatePropertyGetter<TUnion, object?>) is gated on PropertyType == typeof(object), so no boxing conversion is ever required. Worth keeping in mind if a future caller passes a TProperty that differs from the property's actual return type — the emit accessor would handle it via IL conversion, but this reflection accessor would throw at delegate creation.

✅ Test coverage

No tests are added, which is reasonable for a behavior-preserving internal refactor: the reflection member accessor is exercised extensively by existing (non-source-gen) serialization/deserialization tests across both #if NET and downlevel TFMs. CI results should confirm no regressions.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 137 AIC · ⌖ 14.9 AIC · ⊞ 10K

[eiriktsarpalis]

Thanks

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionMemberAccessor.cs:57

• CreateParameterizedConstructor assumes the incoming arguments span includes all ctor parameters (arguments.AsSpan(0, parameterCount) / Array.Copy(..., parameterCount)). However, System.Text.Json packs constructor arguments excluding out parameters (see DefaultJsonTypeInfoResolver.Helpers.PopulateParameterInfoValues which skips IsOut and uses a packed Position). For ctors with one or more out parameters, this will throw (slice/copy out of range) or pass the wrong argument mapping. ReflectionEmitMemberAccessor already has explicit out-parameter mapping; ReflectionMemberAccessor should mirror that by expanding packed args to the full parameter list (inserting placeholders for out) before calling ConstructorInvoker/InvokeNoWrapExceptions.

        public override Func<object?[], T> CreateParameterizedConstructor<T>(ConstructorInfo constructor)
        {
            Type type = typeof(T);

            Debug.Assert(!type.IsAbstract);
            Debug.Assert(constructor.DeclaringType == type && !constructor.IsStatic);

            int parameterCount = constructor.GetParameters().Length;

#if NET
            ConstructorInvoker invoker = ConstructorInvoker.Create(constructor);
            return arguments => (T)invoker.Invoke(arguments.AsSpan(0, parameterCount));
#else

@MichalPetryka it appears your review feedback has been addressed, dismissing your initial request-changes review.