Select STJ member accessor based on IsDynamicCodeCompiled#130503
Open
eiriktsarpalis wants to merge 1 commit into
Open
Select STJ member accessor based on IsDynamicCodeCompiled#130503eiriktsarpalis wants to merge 1 commit into
eiriktsarpalis wants to merge 1 commit into
Conversation
MemberAccessor selected the Reflection.Emit-based accessor whenever RuntimeFeature.IsDynamicCodeSupported was true. On platforms where dynamic code is supported but only interpreted (the Mono interpreter, WASM, iOS), the emitted IL is interpreted anyway, so Reflection.Emit provides no throughput benefit while keeping the ~50KB Reflection.Emit stack un-trimmable. Gate on IsDynamicCodeCompiled instead so the trimmer removes Reflection.Emit on those platforms, matching the pattern already used by DependencyInjection and Regex. JIT-backed runtimes and NativeAOT are unaffected. This routes WASM/iOS to ReflectionMemberAccessor, which invoked user getters, setters and constructors via MethodInfo/ConstructorInfo.Invoke and wrapped their exceptions in TargetInvocationException, unlike the Reflection.Emit accessor which emits direct calls. That was already a latent bug on NativeAOT. Add InvokeNoWrapExceptions helpers (BindingFlags.DoNotWrapExceptions on .NET, an ExceptionDispatchInfo polyfill downlevel) and route all user-code invoke sites through them so exceptions propagate unwrapped on every accessor. Fixes #38693. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
eiriktsarpalis
had a problem deploying
to
copilot-pat-pool
GitHub Actions
Failure
Contributor
|
Tagging subscribers to this area: @dotnet/area-system-text-json |
Contributor
There was a problem hiding this comment.
Pull request overview
Updates System.Text.Json’s runtime accessor strategy to avoid using the Reflection.Emit-based MemberAccessor on platforms where dynamic code isn’t compiled (interpreter-only scenarios), and aligns reflection invocation behavior so user exceptions aren’t surfaced as TargetInvocationException.
Changes:
- Switch
MemberAccessorselection fromRuntimeFeature.IsDynamicCodeSupportedtoRuntimeFeature.IsDynamicCodeCompiledfor better trimming and platform-appropriate behavior. - Route reflection-based constructor/property/method invocation through
InvokeNoWrapExceptionshelpers so user-thrown exceptions propagate unwrapped. - Add regression tests verifying throwing getters/setters/constructors propagate
InvalidOperationException(including a reflection-accessor run viaRemoteExecutor).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/MemberAccessor.cs | Gates Reflection.Emit accessor usage on IsDynamicCodeCompiled instead of IsDynamicCodeSupported. |
| src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionMemberAccessor.cs | Uses no-wrap invoke helpers for reflected constructors/methods so exceptions aren’t wrapped. |
| src/libraries/System.Text.Json/src/System/ReflectionExtensions.cs | Adds shared InvokeNoWrapExceptions helpers (and refines existing polyfill behavior). |
| src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/ExceptionTests.cs | Adds regression coverage for unwrapped user exceptions under both accessor modes. |
This was referenced Jul 10, 2026
eiriktsarpalis
added a commit
to eiriktsarpalis/PolyType
that referenced
this pull request
Jul 10, 2026
* Select reflection member accessor based on IsDynamicCodeCompiled Ports the fix from dotnet/runtime#130503 to PolyType's reflection shape provider. - The default for ReflectionTypeShapeProviderOptions.UseReflectionEmit now gates on RuntimeFeature.IsDynamicCodeCompiled instead of IsDynamicCodeSupported. On runtimes where dynamic code is supported but only interpreted (Mono interpreter, WebAssembly, iOS), emitted IL is itself interpreted and offers no throughput benefit while still pulling in the un-trimmable System.Reflection.Emit stack, so plain reflection accessors are now preferred there. - The reflection member accessor now propagates user-code exceptions unwrapped (via new InvokeNoWrapExceptions helpers) instead of wrapping them in a TargetInvocationException, matching the Reflection.Emit accessor's behavior. - Adds MemberAccessorExceptionTests guarding the unwrapped-exception behavior across the reflection, reflection-emit, and source-gen providers. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Consolidate duplicated TargetInvocationException unwrapping Extract the identical downlevel catch blocks in the two InvokeNoWrapExceptions overloads into a single RethrowInnerException helper (compiled only for non-.NET TFMs, since .NET uses BindingFlags.DoNotWrapExceptions natively). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Remove now-unused Invoke(this MethodBase) helper All of its call sites were converted to InvokeNoWrapExceptions, so the exception-wrapping Invoke extension is dead code; the full solution still compiles across all TFMs without it. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Revert AGENTS.md reflection-provider description to original wording Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Inline TargetInvocationException rethrow to keep it out of stack traces Remove the shared RethrowInnerException helper and inline the ExceptionDispatchInfo rethrow directly into each downlevel catch block, so the re-thrown user exception's stack trace doesn't gain an extra helper frame. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Use PropertyInfo.SetMethod instead of GetSetMethod(nonPublic: true) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #38693.
Problem
MemberAccessorselects theReflection.Emit-based accessor wheneverRuntimeFeature.IsDynamicCodeSupportedistrue. On platforms where dynamic code is supported but only interpreted (the Mono interpreter, WASM, iOS), the emitted IL is interpreted anyway, soReflection.Emitgives no throughput benefit while keeping the (~50KB)Reflection.Emitstack un-trimmable.Fix
Gate accessor selection on
RuntimeFeature.IsDynamicCodeCompiledinstead ofIsDynamicCodeSupported. BecauseIsDynamicCodeCompiledis trim-substituted to a constant per platform (falseon WASM/iOS,trueon CoreCLR), the trimmer now removes theReflection.Emitbranch on interpreter-only platforms and keeps it on JIT-backed runtimes. This matches the pattern already used byMicrosoft.Extensions.DependencyInjection(ActivatorUtilities,ServiceProvider) andSystem.Text.RegularExpressions.The source generator remains the recommended escape hatch for throughput-sensitive WASM/iOS apps.
Coupled fix: reflection accessor was wrapping user exceptions
Routing WASM/iOS to
ReflectionMemberAccessorsurfaced a latent bug: it invoked user getters, setters and constructors viaMethodInfo/ConstructorInfo.Invoke, which wraps exceptions thrown by user code inTargetInvocationException— unlike theReflection.Emitaccessor, which emits direct calls and lets them propagate. This already affected NativeAOT (whose test suite is a subset, so it went unnoticed).Added
InvokeNoWrapExceptionshelpers (BindingFlags.DoNotWrapExceptionson .NET, anExceptionDispatchInfopolyfill downlevel) and routed all user-code invoke sites through them so exceptions propagate unwrapped on every accessor. As a drive-by, the siblingCreateInstanceNoWrapExceptionspolyfill was aligned to guard a nullInnerException.Tests
Added regression tests in
ExceptionTests.csasserting a throwing getter, setter and constructor propagate the originalInvalidOperationException(notTargetInvocationException), both under the default (Reflection.Emit) accessor and — viaRemoteExecutorforcingIsDynamicCodeSupported=false— underReflectionMemberAccessor. Verified theRemoteExecutortest fails without the fix and passes with it.Full
System.Text.Jsonsuite: 52710 passed (default) / 52457 passed, 48 skipped (reflection-forced), 0 failures.Note
This pull request was authored by GitHub Copilot.