Conversation
Move the System.Text.Json source generator's accessor-emission machinery (UnsafeAccessor-based get/set/field accessors, constructor accessors, generic wrappers, and the reflection fallback) into a new shared helper, Common/src/SourceGenerators/UnsafeAccessorEmitter.cs, so other source generators (e.g. the Microsoft.Extensions.Configuration.Binder generator) can reuse it. The helper works over neutral, primitive-only spec types (UnsafeAccessorMemberSpec, UnsafeAccessorConstructorSpec, UnsafeAccessorParameterSpec) and owns accessor naming using STJ's existing scheme, parameterized by a type-friendly name. STJ's GenerateMemberAccessors and GenerateConstructorAccessor become thin adapters that build the neutral specs and delegate to the helper. This is a pure refactoring with no behavior change: the generated output is byte-identical, as verified by the SourceGeneratedOutputTests baselines for both the netcoreapp (UnsafeAccessor) and net462 (reflection) code paths. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1a0d7ac2-f2a3-4526-93ab-36bf1a23933f
|
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. |
Contributor
|
Tagging subscribers to this area: @dotnet/area-system-text-json |
This was referenced Sep 7, 2026
Member
|
Is this change a precondition for a #133369 fix? If the answer is yes, it may complicate the backporting process to .NET 11. I would suggest making a minimal fix for the regression first that can be backported to 11 before branching off to new infra. |
The reflection fallback emitted by the shared helper references an InstanceMemberBindingFlags const and a ValueTypeSetter<,> delegate that the consuming generator must declare. Their exact name and signature are a contract with the fallback code, so expose them as InstanceMemberBindingFlagsDeclaration and ValueTypeSetterDelegateDeclaration constants on the helper and have STJ write those instead of hardcoding the text. The constants hold the identical text STJ emitted, so the generated output is byte-identical (output baselines unchanged). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1a0d7ac2-f2a3-4526-93ab-36bf1a23933f
InstanceMemberBindingFlagsDeclaration now holds only the declaration, not surrounding blank lines, so callers own separation. STJ reproduces its existing indented blank lines via WriteLine of an empty string (output byte-identical, baselines unchanged); other consumers can add clean blanks. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1a0d7ac2-f2a3-4526-93ab-36bf1a23933f
Reconciles the shared UnsafeAccessorEmitter extraction with #133599 (fix STJ source-generated unsafe accessors). The only conflict was JsonSourceGenerator.Emitter.cs; it was resolved by taking main's post-#133599 accessor code and re-applying the extraction on top. The #133599 improvements are folded into the shared helper so STJ output stays byte-identical: - safe/unsafe extern modifier, threaded as a useUpdatedMemorySafetyRules bool parameter on EmitMemberAccessors/EmitConstructorAccessor (STJ computes the flag at parse time and stores it on ContextGenerationSpec); - DeclaringTypeIndex disambiguator in the generic wrapper class name; - generic constructor accessor emitted inside a partial __GenericAccessors_<name>_0 wrapper shared with the member accessors; - ref/out/in constructor parameters (including the reflection fallback that writes back ref/out args); - field reflection-fallback and open-type fallbacks. STJ's GenerateMemberAccessors/GenerateConstructorAccessor remain thin adapters that build the neutral specs and delegate to the helper; the moved name/dup helpers and the GenericAccessorEntry alias are removed. Both Roslyn versions build clean and the SourceGeneratedOutputTests baselines are byte-identical.
This was referenced Sep 16, 2026
svick
marked this pull request as ready for review
September 18, 2026 10:04
|
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. |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Constructor accessor qualification can reference an un emitted wrapper, and fallback declarations are not using the shared constants.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Extracts STJ’s unsafe-accessor and reflection-fallback generation into a reusable shared source-generator helper while preserving generated output.
Changes:
- Adds neutral accessor specifications and shared emission logic.
- Adapts STJ to delegate accessor generation to the helper.
- Includes the helper in the STJ source-generator project.
File summaries
| File | Summary |
|---|---|
src/libraries/System.Text.Json/gen/System.Text.Json.SourceGeneration.targets |
Includes the shared emitter. |
src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs |
Uses shared accessor specs and adapters; fallback declarations remain inline. |
src/libraries/Common/src/SourceGenerators/UnsafeAccessorEmitter.cs |
Adds shared accessor emission, with an inconsistency between constructor wrapper qualification and emission conditions. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Lite
Comment on lines
+178
to
+181
| if (canUseUnsafeAccessor && declaringTypeParameterNames is not null) | ||
| { | ||
| string typeArgsList = typeFQN.Substring(typeFQN.IndexOf('<')); | ||
| return $"__GenericAccessors_{typeFriendlyName}_0{typeArgsList}.{accessorName}"; |
Comment on lines
+40
to
+45
| public const string InstanceMemberBindingFlagsDeclaration = """ | ||
| private const global::System.Reflection.BindingFlags InstanceMemberBindingFlags = | ||
| global::System.Reflection.BindingFlags.Instance | | ||
| global::System.Reflection.BindingFlags.Public | | ||
| global::System.Reflection.BindingFlags.NonPublic; | ||
| """; |
This was referenced Sep 18, 2026
This branch has not been deployed
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.
Summary
Extracts the System.Text.Json source generator's accessor-emission machinery into a new shared helper,
src/libraries/Common/src/SourceGenerators/UnsafeAccessorEmitter.cs, so that other source generators can reuse it. This is a refactoring with no change to STJ's generated output — a preparatory step for giving theMicrosoft.Extensions.Configuration.Bindersource generator the same[UnsafeAccessor]-based accessor support, done in the stacked PR #131597.What the helper owns
The helper emits, over neutral primitive-only spec types (
UnsafeAccessorMemberSpec,UnsafeAccessorConstructorSpec,UnsafeAccessorParameterSpec) rather than STJ's model types:[UnsafeAccessor]property/field get/set externs, and the inaccessible-constructor extern.partial __GenericAccessors_<TypeFriendlyName>_<index>wrapper class holding both the member and constructor externs, keyed by the declaring type's position in the hierarchy.safeextern modifier when the compilation uses the updated memory-safety rules (passed in as a flag the caller computes).ref/out/inref-kinds, including the reflection fallback that writes backref/outargs.net462): cached delegates /FieldInfo/ConstructorInfowrappers.The reflection fallback references an
InstanceMemberBindingFlagsconst and aValueTypeSetter<,>delegate; the helper owns their declaration text (InstanceMemberBindingFlagsDeclaration,ValueTypeSetterDelegateDeclaration) so their names and signatures stay in sync with the code that consumes them, and each consuming generator writes them once into its own scope.STJ's
GenerateMemberAccessorsandGenerateConstructorAccessorare thin adapters that build the neutral specs and delegate to the helper.Validation
STJ's generated output is byte-identical, verified by the
SourceGeneratedOutputTestsbaselines (both thenetcoreappUnsafeAccessor andnet462reflection code paths); both Roslyn-versioned source generator projects build clean.Note
This PR description was generated by GitHub Copilot.