Repository navigation
(PATCH): Add an internal Reflection.Emit patch class builder - #130
Merged
Merged
Conversation
EmitPatchClassBuilder builds the same patch class as PatchClassBuilder from PatchClassModel, but emits it as IL into a dynamic assembly per source type. The assembly grants itself [IgnoresAccessChecksTo] for every assembly the patch class names, so it isn't limited to public types. Nothing uses it yet. PatchClassBuilderTest now runs against both builders. The Roslyn-only cases (its cache and its public-only restriction) move to their own fixture. A new case checks that the patch doesn't read target.P when it sets P. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Covers an internal class (explicit null, [JsonPropertyName], init accessor, [Range] validation, an internal [JsonConverter]), a private nested class, a private positional record, an internal primary-constructor class, an internal struct, and an internal property type from a second assembly. The second assembly is a new, unpacked test-helper project, PTrampert.SimplePatch.Test.External. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
PaulTrampert
commented
Oct 2, 2026
…ode comment Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
IgnoresAccessChecksToAttribute isn't officially supported (dotnet/runtime#37875), so take Castle DynamicProxy's approach: every emitted assembly has the fixed name PTrampert.SimplePatch.Emitted, and an assembly whose internal types are patched grants it InternalsVisibleTo. Before emitting, check that every type and getter the patch class uses is accessible, and throw NotSupportedException naming the assembly that needs the grant. Private nested types and private getters are no longer supported, because InternalsVisibleTo doesn't reach them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
✅ PR Title Formatted CorrectlyThe title of this PR has been updated to match the correct format. Thank you! |
PaulTrampert
added a commit
that referenced
this pull request
Oct 4, 2026
) Closes #126 ## Cause Since #130 and #136 the library has had two builders for the same patch class. One is the original builder, which generates C# with CodeDom and compiles it with Roslyn. The other is a Reflection.Emit builder, behind the experimental `PatchClassBuilder.UseExperimentalDynamicClassBuilder` flag that shipped in v1.5.0. The process-wide switch adds runtime complexity, and the Roslyn path has limits that the Emit path doesn't have. It can't patch internal write models, and it breaks under single-file publishing because it builds metadata references from `Assembly.Location`. ## Fix - `PatchClassBuilder.GetPatchClassFor` now always delegates to `EmitPatchClassBuilder.Instance`. Emit is the only builder used at runtime. - `PatchClassBuilder.UseExperimentalDynamicClassBuilder` is removed outright, and so are its tests. Its `GetPatchClassFor` `<exception>` docs now describe when Emit throws. - **`RoslynPatchClassBuilder` is kept as internal, unused, tested code, pending #144.** That issue decides whether it becomes a compile-time source generator. This departs from #126's scope, which asked for the Roslyn path to be deleted, because the maintainer wants it kept until #144 is decided. - The class remarks say it isn't used at runtime and is kept pending #144. - Its "not public" error no longer mentions the removed flag. - **The `Microsoft.CodeAnalysis.CSharp` and `System.CodeDom` package references remain, so consumers still get them transitively.** The `.csproj` has a comment explaining why. - Tests: - `PatchClassBuilderTest` is still parameterized over both builders through `PatchClassBuilders`, calling the internal builders directly, so the two can't drift apart. - `RoslynPatchClassBuilderTest` is kept. Its shared-cache test, which asserted that `PatchClassBuilder` hands out Roslyn types, is now a plain Roslyn cache test. - New `PatchClassBuilderDelegationTest` covers the public entry point: - `PatchClassBuilder` (including the obsolete constructor) hands out the Emit builder's types. - An internal model round-trips through `IPatchObject<T>` with validation. - A private nested type throws `NotSupportedException`. - `EmitPatchClassBuilderTest` gains a concurrent-first-use test. - The Swashbuckle and OpenApi `ExperimentalDynamicClassBuilderTest` files are renamed to `InternalWriteModelTest`, without the flag, `TearDown` or `[NonParallelizable]`. - Docs: - README: "Non-public write models (experimental)" is now "Internal write models", and there is a new "Deployment" section. - `docs/getting-started.md` matches the README. - `AGENTS.md` "How it works" describes Emit as the runtime builder, and the Roslyn builder as unused, tested code pending #144. ### Single-file publishing (investigated, as #126 asks) I published `PTrampert.SimplePatch.Sample` with `-r linux-x64 --self-contained false -p:PublishSingleFile=true` and sent a PATCH to `/People/1`: - **`release/2.0` (Roslyn):** HTTP 500. `CS0518: Predefined type 'System.Object' is not defined or imported` and `CS0234` for `IPatchObject<>`. Roslyn gets no metadata references because the bundled assemblies have no `Location`. - **This branch (Emit):** HTTP 200 with the patched person. I re-checked this after restoring the Roslyn builder. The Roslyn assemblies are still bundled, but nothing on the runtime path calls into them. The README's new Deployment section says single-file publishing is supported, and that Native AOT and trimming aren't. **I didn't add an automated single-file test or sample.** It would need a publish-and-run step in CI, which is a larger change than this PR should carry. ## Observable behaviour changes for consumers - `PatchClassBuilder.UseExperimentalDynamicClassBuilder` is gone. Code that sets it no longer compiles, and existing binaries fail with `MissingMethodException`. - Generated types change: - They are named `<Namespace>.Optionals.<Type>_Optionals`, without the random suffix. - They live in a dynamic assembly named `PTrampert.SimplePatch.Emitted`, so `Assembly.IsDynamic` is true and there is no `Location`. - Internal write models now work when their assembly declares `[InternalsVisibleTo("PTrampert.SimplePatch.Emitted")]`. Without that grant, they still throw `NotSupportedException`, and the message names the grant and the assembly that needs it. - Private and protected nested types still throw `NotSupportedException`. - If generation fails, the error surfaces from Reflection.Emit (`TypeLoadException` / `InvalidProgramException`), not as Roslyn diagnostics. - Single-file published apps now work. - **Unchanged:** `Microsoft.CodeAnalysis.*` and `System.CodeDom` still arrive transitively, until #144 is decided. ## Alternatives rejected - **Delete the Roslyn builder and its dependencies now, as #126 originally scoped.** I did this in this PR's first commit, then reverted it, because the maintainer wants the code kept until #144 decides whether it becomes a source generator. - **Keep `UseExperimentalDynamicClassBuilder` as an `[Obsolete]` no-op.** #126 rules this out. The removal ships in the same major as the other breaking changes, and a compile error is the clearest signal. - **Keep the flag so the Roslyn builder stays selectable.** That would keep the process-wide switch and the unsupported single-file path reachable. The dead code is exercised by tests instead. ## Coordination with parallel 2.0 PRs - **#135** (retype `PatchClassBuilder.Instance` to `IPatchClassBuilder`): this PR leaves `Instance` and its type alone. When both land, #135's `Instance` getter will need to resolve to `EmitPatchClassBuilder.Instance`. - **#75** (constructor visibility): this PR doesn't touch the obsolete public constructor. `PatchClassBuilderDelegationTest` calls it under `#pragma warning disable CS0618`, and will keep compiling if the constructor becomes internal, because the test project has `InternalsVisibleTo`. ## Test results - `dotnet build`: 0 errors, 21 warnings, the same count as `release/2.0`. - `dotnet test`: - `PTrampert.SimplePatch.Test` (net8.0): 128 passed. That is 129 on `release/2.0`, minus 5 flag tests, plus 4 new ones. - `PTrampert.SimplePatch.Swashbuckle.Test` (net8.0): 18 passed. - `PTrampert.SimplePatch.OpenApi.Test` (net10.0): 12 passed. - Only the .NET 10 runtime is installed locally, so I ran the net8.0 test hosts with `DOTNET_ROLL_FORWARD=Major`. - **CI won't run these tests on this PR.** The main pipeline only runs on PRs to `main`, so PRs into `release/2.0` get only the title check. The net8.0 suites will first run in CI when `release/2.0` is merged to `main`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
PaulTrampert
pushed a commit
to PaulTrampert/PacmanManager
that referenced
this pull request
Oct 6, 2026
Updated [PTrampert.SimplePatch](https://github.com/PaulTrampert/PTrampert.SimplePatch) from 1.3.6 to 2.0.3. <details> <summary>Release notes</summary> _Sourced from [PTrampert.SimplePatch's releases](https://github.com/PaulTrampert/PTrampert.SimplePatch/releases)._ ## 2.0.3 ## Changes - (PATCH): Drop the Roslyn patch class builder's own cache ([#154](PaulTrampert/PTrampert.SimplePatch#154)) ## 2.0.2 ## Changes - (PATCH): Route PatchClassBuilder.Instance through CachingPatchClassBuilder ([#155](PaulTrampert/PTrampert.SimplePatch#155)) ## 2.0.1 ## Changes - (PATCH): Add an internal CachingPatchClassBuilder decorator ([#153](PaulTrampert/PTrampert.SimplePatch#153)) - (PATCH): Document the release PR format for staged release branches ([#152](PaulTrampert/PTrampert.SimplePatch#152)) ## 2.0.0 ## Changes - (MAJOR): Release 2.0 ([#147](PaulTrampert/PTrampert.SimplePatch#147)) - (PATCH): Build PRs into release/** branches and document the release flow ([#146](PaulTrampert/PTrampert.SimplePatch#146)) ## 1.5.0 ## What's Changed * (MINOR): Add PatchClassBuilder.UseExperimentalDynamicClassBuilder by @PaulTrampert in PaulTrampert/PTrampert.SimplePatch#136 **Full Changelog**: PaulTrampert/PTrampert.SimplePatch@v1.4.0...v1.5.0 ## 1.4.0 ## What's Changed * (MINOR): Add IPatchClassBuilder, with PatchClassBuilder delegating to an internal RoslynPatchClassBuilder by @PaulTrampert in PaulTrampert/PTrampert.SimplePatch#133 **Full Changelog**: PaulTrampert/PTrampert.SimplePatch@v1.3.8...v1.4.0 ## 1.3.8 ## What's Changed * (PATCH): Add an internal Reflection.Emit patch class builder by @PaulTrampert in PaulTrampert/PTrampert.SimplePatch#130 **Full Changelog**: PaulTrampert/PTrampert.SimplePatch@v1.3.7...v1.3.8 ## 1.3.7 ## What's Changed * (PATCH): Bump the csharp-deps group with 6 updates by @dependabot[bot] in PaulTrampert/PTrampert.SimplePatch#81 * (PATCH): Add AGENTS.md, CLAUDE.md symlink and /implement-unblocked by @PaulTrampert in PaulTrampert/PTrampert.SimplePatch#125 * (PATCH): Extract the patch class model from PatchClassBuilder by @PaulTrampert in PaulTrampert/PTrampert.SimplePatch#129 **Full Changelog**: PaulTrampert/PTrampert.SimplePatch@v1.3.6...v1.3.7 Commits viewable in [compare view](PaulTrampert/PTrampert.SimplePatch@v1.3.6...v2.0.3). </details> Updated [PTrampert.SimplePatch.Swashbuckle](https://github.com/PaulTrampert/PTrampert.SimplePatch) from 1.3.6 to 2.0.3. <details> <summary>Release notes</summary> _Sourced from [PTrampert.SimplePatch.Swashbuckle's releases](https://github.com/PaulTrampert/PTrampert.SimplePatch/releases)._ ## 2.0.3 ## Changes - (PATCH): Drop the Roslyn patch class builder's own cache ([#154](PaulTrampert/PTrampert.SimplePatch#154)) ## 2.0.2 ## Changes - (PATCH): Route PatchClassBuilder.Instance through CachingPatchClassBuilder ([#155](PaulTrampert/PTrampert.SimplePatch#155)) ## 2.0.1 ## Changes - (PATCH): Add an internal CachingPatchClassBuilder decorator ([#153](PaulTrampert/PTrampert.SimplePatch#153)) - (PATCH): Document the release PR format for staged release branches ([#152](PaulTrampert/PTrampert.SimplePatch#152)) ## 2.0.0 ## Changes - (MAJOR): Release 2.0 ([#147](PaulTrampert/PTrampert.SimplePatch#147)) - (PATCH): Build PRs into release/** branches and document the release flow ([#146](PaulTrampert/PTrampert.SimplePatch#146)) ## 1.5.0 ## What's Changed * (MINOR): Add PatchClassBuilder.UseExperimentalDynamicClassBuilder by @PaulTrampert in PaulTrampert/PTrampert.SimplePatch#136 **Full Changelog**: PaulTrampert/PTrampert.SimplePatch@v1.4.0...v1.5.0 ## 1.4.0 ## What's Changed * (MINOR): Add IPatchClassBuilder, with PatchClassBuilder delegating to an internal RoslynPatchClassBuilder by @PaulTrampert in PaulTrampert/PTrampert.SimplePatch#133 **Full Changelog**: PaulTrampert/PTrampert.SimplePatch@v1.3.8...v1.4.0 ## 1.3.8 ## What's Changed * (PATCH): Add an internal Reflection.Emit patch class builder by @PaulTrampert in PaulTrampert/PTrampert.SimplePatch#130 **Full Changelog**: PaulTrampert/PTrampert.SimplePatch@v1.3.7...v1.3.8 ## 1.3.7 ## What's Changed * (PATCH): Bump the csharp-deps group with 6 updates by @dependabot[bot] in PaulTrampert/PTrampert.SimplePatch#81 * (PATCH): Add AGENTS.md, CLAUDE.md symlink and /implement-unblocked by @PaulTrampert in PaulTrampert/PTrampert.SimplePatch#125 * (PATCH): Extract the patch class model from PatchClassBuilder by @PaulTrampert in PaulTrampert/PTrampert.SimplePatch#129 **Full Changelog**: PaulTrampert/PTrampert.SimplePatch@v1.3.6...v1.3.7 Commits viewable in [compare view](PaulTrampert/PTrampert.SimplePatch@v1.3.6...v2.0.3). </details> Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore <dependency name> major version` will close this group update PR and stop Dependabot creating any more for the specific dependency's major version (unless you unignore this specific dependency's major version or upgrade to it yourself) - `@dependabot ignore <dependency name> minor version` will close this group update PR and stop Dependabot creating any more for the specific dependency's minor version (unless you unignore this specific dependency's minor version or upgrade to it yourself) - `@dependabot ignore <dependency name>` will close this group update PR and stop Dependabot creating any more for the specific dependency (unless you unignore this specific dependency or upgrade to it yourself) - `@dependabot unignore <dependency name>` will remove all of the ignore conditions of the specified dependency - `@dependabot unignore <dependency name> <ignore condition>` will remove the ignore condition of the specified dependency and ignore conditions </details> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@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.
Closes #128
What
EmitPatchClassBuilderis a new internal builder. From the sharedPatchClassModel, it emits the same patch class thatPatchClassBuildercompiles with Roslyn, and it also works for internal source types whose assembly declares[InternalsVisibleTo("PTrampert.SimplePatch.Emitted")]. Nothing in the library calls it yet.PatchClassBuilder.Instancestill uses Roslyn, and the opt-in is #95. The public API doesn't change, so this is a PATCH.Points the diff doesn't make obvious
AssemblyBuilderAccess.Run), created insideAssemblyLoadContext.EnterContextualReflection(type.Assembly)so it lands in the source type's ALC. Every one of them is namedPTrampert.SimplePatch.Emitted(EmitPatchClassBuilder.AssemblyName), so a consuming assembly grants access with[InternalsVisibleTo].InternalsVisibleTodoesn't reach private or protected members, so private nested source types and private getters aren't supported.EnsureAccessiblewalks every type the class names: the source type and its declaring types and generic arguments, plus each patched property's type and declaring type. It also checks every getter thatPatchreads. If one isn't reachable, it throwsNotSupportedExceptionnaming the assembly that needs the grant. Otherwise the runtime would fail at type load with an error that doesn't say how to fix it.PatchIL. Each value is_P.HasValue ? _P.Value : target.P, andtarget.Pis read only on the false branch. A record iscallvirt <Clone>$+castclass, thendup/callvirton each init accessor. A class isnewobjwith the bound values, thendup/callvirton each setter. A struct is built in a local (newobj+stloc, orinitobj), set withldloca/call, andtargetis read throughldarga. Ignored properties are copied only for non-records, as in the Roslyn builder.{Namespace}.Optionals.{type.Name}_Optionals. Each type has its own assembly, so the name needs no random suffix and no identifier cleanup.NotSupportedExceptionwith a clear message. Roslyn gave a compile error for the same case.Tests
PatchClassBuilderTestis now a[TestFixtureSource]over both builders (Roslyn,Emit), so every existing regression case also covers the Emit path. Those cases now deserialize straight into the fixture's patch type. Going throughIPatchObject<T>would hitPatchJsonConverterFactory, which always usesInstance.RoslynPatchClassBuilderTest: the cross-builder cache, concurrent first use, and the "must be public" exceptions.target.Pis not read when the patch setsP(ReadTrackingTestObject).EmitPatchClassBuilderTestcases. The test project grantsInternalsVisibleTo("PTrampert.SimplePatch.Emitted").[JsonPropertyName],init, an internal[JsonConverter])[Range]validation of an internal classList<InternalStruct>)NotSupportedException:PTrampert.SimplePatch.Test.External. It is unpacked and givesInternalsVisibleToto the core test project, but not to the emitted assemblies.InternalsVisibleTofails the 8 tests on supported internal types.EnsureAccessiblefails the 4NotSupportedExceptiontests.Alternatives rejected
[IgnoresAccessChecksTo], which needs no setup from users and covers private nested types. It is undocumented and not officially supported (Document IgnoresAccessChecksToAttribute dotnet/runtime#37875), so review rejected it.Microsoft.CodeAnalysisupdate.Not covered
Results
dotnet build: 0 errors. The warning set is unchanged: the same set of warnings as onmain.dotnet test: all passing. Core 124/124 (was 86), Swashbuckle 17/17, OpenApi 11/11. This machine has no .NET 8 runtime, so the net8.0 test hosts ran withDOTNET_ROLL_FORWARD=Majoron .NET 10.🤖 Generated with Claude Code