Skip to content

(MAJOR): Make Reflection.Emit the only runtime patch class builder - #142

Merged
PaulTrampert merged 6 commits into
release/2.0from
feature/126-remove-roslyn-builder
Oct 4, 2026
Merged

PaulTrampert merged 6 commits into
release/2.0from
feature/126-remove-roslyn-builder

Conversation

@PaulTrampert

@PaulTrampert PaulTrampert commented Oct 4, 2026 •

Copy link
Copy Markdown
Owner

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 Generate patch classes at compile time with a source generator #144. That issue decides whether it becomes a compile-time source generator. This departs from Remove the Roslyn-based patch class builder in favour of Reflection.Emit #126's scope, which asked for the Roslyn path to be deleted, because the maintainer wants it kept until Generate patch classes at compile time with a source generator #144 is decided.
    • The class remarks say it isn't used at runtime and is kept pending Generate patch classes at compile time with a source generator #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 Generate patch classes at compile time with a source generator #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 Generate patch classes at compile time with a source generator #144 is decided.

Alternatives rejected

Coordination with parallel 2.0 PRs

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

@PaulTrampert PaulTrampert changed the title (MAJOR): Remove the Roslyn patch class builder in favour of Reflection.Emit (MAJOR): Make Reflection.Emit the only runtime patch class builder Oct 4, 2026
PaulTrampert added a commit that referenced this pull request Oct 4, 2026
…flow (#146)

Closes #145

## Cause

Breaking changes for 2.0 are staged on `release/2.0`, but
`dotnet-library.yml` only triggered `pull_request` runs for `main`, so
#139–#142 had no CI build.

## Fix

- Add `release/**` to the `pull_request` branches. Publishing still runs
only for `main`: the `push` trigger is unchanged, and the shared
workflow publishes only when `github.ref` is `main`.
- Document the release-branch flow in `AGENTS.md`. PRs into
`release/<major>` are squash-merged. The release PR into `main` is
rebase-merged, because release notes are now built from commit subjects
(PaulTrampert/github-workflows#6), and a squash would collapse them to
one line.

A `pull_request` run uses the workflow file from the base branch, so
`release/2.0` needs this commit too. It has nothing `main` lacks, so it
can be fast-forwarded to `main` once this merges.

## Rejected alternatives

- Shipping releases by pushing `release/2.0:main` directly, with a
merge-gate check and an admin bypass. That kept commit SHAs for GitHub's
generated release notes, which notes built from commit subjects no
longer need.

## Outside this PR

Already applied through the API: rebase merging enabled for the
repository, `rebase` allowed in both `main` rulesets, and a new
`release/**` ruleset (squash only, build and PR-title checks required).

## Tests

No code changes. YAML change verified by inspection; this PR's own build
exercises the `main` trigger.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
PaulTrampert and others added 2 commits October 4, 2026 17:30
PatchClassBuilder now always delegates to EmitPatchClassBuilder. The
CodeDom/Roslyn builder, its Microsoft.CodeAnalysis.CSharp and
System.CodeDom references, and PatchClassBuilder.UseExperimentalDynamicClassBuilder
are removed. Internal write models are supported when their assembly grants
InternalsVisibleTo("PTrampert.SimplePatch.Emitted"), and single-file
publishing now works.

Closes #126

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The maintainer wants RoslynPatchClassBuilder kept until #144 decides
whether it becomes a source generator. It is restored, unused at runtime,
with its package references and its tests, which call the internal
builders directly. PatchClassBuilder still delegates only to the Emit
builder, and UseExperimentalDynamicClassBuilder stays removed. The
public entry point tests move to PatchClassBuilderDelegationTest.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@PaulTrampert
PaulTrampert force-pushed the feature/126-remove-roslyn-builder branch from 05d90e5 to ec335f3 Compare October 4, 2026 21:30
PaulTrampert and others added 3 commits October 4, 2026 17:39
# Conflicts:
#	AGENTS.md
#	PTrampert.SimplePatch.Test/RoslynPatchClassBuilderTest.cs
#	PTrampert.SimplePatch/PatchClassBuilder.cs
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
PaulTrampert added a commit that referenced this pull request Oct 4, 2026
…make PatchClassBuilder static (#141)

## Cause

#133 made `PatchClassBuilder` a facade over the internal
`RoslynPatchClassBuilder`, and #95 added
`UseExperimentalDynamicClassBuilder`, which the facade's
`GetPatchClassFor` read on every call. Both kept
`PatchClassBuilder.Instance` typed as `PatchClassBuilder`, because
changing a property's type breaks binary compatibility. This is the 2.0
change that #135 deferred.

Once `Instance` hands out the selected internal builder, nothing hands
out a `PatchClassBuilder` any more. Its instance surface (the obsolete
constructor, instance `GetPatchClassFor`, and its `IPatchClassBuilder`
implementation) only forwards to `Instance` and is dead weight (#143).
The two changes are one PR because a static class can't be a property
type, so #143 can't compile without #135.

## Fix

**Retype `Instance` (#135)**
- `public static IPatchClassBuilder Instance` returns the selected
builder itself: `EmitPatchClassBuilder.Instance` when the flag is on,
otherwise `RoslynPatchClassBuilder.Instance`. Both internal builders
already had static `Instance` singletons.
- `Instance` is a computed property (`=>`), not an initialised one, so
it honours the flag at read time. The flag is a settable static, and the
tests flip it back and forth.
- Call sites (`PatchJsonConverterFactory`, the Swashbuckle filter, the
OpenAPI transformer) already call
`PatchClassBuilder.Instance.GetPatchClassFor(...)` on each use, so they
compile and behave unchanged against the new type. None of them caches
the builder.

**Make `PatchClassBuilder` static (#143)**
- `public static class PatchClassBuilder` now holds only `Instance` and
`UseExperimentalDynamicClassBuilder`.
- The constructor, instance `GetPatchClassFor` and the
`IPatchClassBuilder` implementation are removed.
- The XML docs worth keeping moved. The description of the generated
class (its properties, its `Patch` method, sealed and public, the
`.Optionals` namespace) is now in the class remarks. The
`NotSupportedException` conditions are now in `Instance`'s remarks.
`IPatchClassBuilder.GetPatchClassFor` keeps its builder-neutral
contract. `RoslynPatchClassBuilder.GetPatchClassFor` had `<inheritdoc
cref="PatchClassBuilder.GetPatchClassFor"/>` and now has its own summary
and exception docs.
-
`RoslynPatchClassBuilderTest.GetPatchClassFor_SharesGeneratedTypesAcrossBuilders`
constructed `PatchClassBuilder` twice. It is now
`GetPatchClassFor_CachesTheGeneratedType`, which calls
`RoslynPatchClassBuilder.Instance` twice and still checks that
`PatchClassBuilder.Instance` agrees with it while the flag is off. The
internal builders have private constructors, so a second instance can't
be constructed.

**Docs:** `README.md` and `docs/getting-started.md` note that `Instance`
returns the selected builder, so you should read it where you use it and
not keep it. `AGENTS.md` describes the static class.

**Breaking:**
- Binaries compiled against `PatchClassBuilder
PatchClassBuilder.Instance { get; }` fail with `MissingMethodException`.
- `new PatchClassBuilder()` (obsolete since 1.x) and instance
`GetPatchClassFor` are gone.
- `PatchClassBuilder` no longer implements `IPatchClassBuilder`, and
can't be used as a variable, parameter or generic argument type.

## Coordination with sibling PRs into `release/2.0`

- **#139 (#75) makes the constructor internal.** This PR deletes the
constructor, which supersedes that change. When the two meet, resolve
the conflict by deleting the constructor.
- **#142 (#126)** makes Emit the only runtime builder and removes the
flag. Once it lands, `Instance` collapses to
`EmitPatchClassBuilder.Instance`.
- **#140 (#76)** changes TFMs. They are unchanged here.

## Alternatives rejected

- **Initialise `Instance` once (`{ get; } = ...`).** This would freeze
whatever builder the flag selected at type initialisation and ignore
later changes to the flag.
- **Keep returning the facade, typed as the interface.** This would
avoid the read-time caveat, but it keeps the per-call forwarding that
#135 asks to remove. It also leaves `Instance` as the one thing that
hands out a `PatchClassBuilder`.
- **Keep `PatchClassBuilder` non-static, with the constructor internal
(#75 alone).** Nothing would construct it, so its instance members would
be unreachable dead code.
- **Ship #143 as a separate PR stacked on this one.** The maintainer
chose to fold it in, because #143 can't compile without the retype.

## Tests

New tests in `UseExperimentalDynamicClassBuilderTest`:
- `Instance_IsTheRoslynBuilderWhenOff`
- `Instance_IsTheEmitBuilderWhenOn`
- `Instance_FollowsTheFlagWhenItIsTurnedBackOff`

Against the old `PatchClassBuilder.cs`, the NUnit analyzer rejects all
three with NUnit2020 (a `SameAs` that always fails because the types are
mutually exclusive), so they fail before the fix.

CI's build pipeline doesn't run on PRs that target a branch other than
`main`, so these results are local only:
- `dotnet build`: 0 errors, 21 warnings, the same count as the base
commit.
- `dotnet test`: all passed. Core 132/132, Swashbuckle 18/18, OpenApi
12/12. The net8.0 test hosts ran with `DOTNET_ROLL_FORWARD=Major`,
because only the .NET 10 runtime is installed locally.

Closes #135
Closes #143

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
# Conflicts:
#	AGENTS.md
#	PTrampert.SimplePatch.Test/RoslynPatchClassBuilderTest.cs
#	PTrampert.SimplePatch.Test/UseExperimentalDynamicClassBuilderTest.cs
#	PTrampert.SimplePatch/PatchClassBuilder.cs
#	PTrampert.SimplePatch/RoslynPatchClassBuilder.cs
#	README.md
#	docs/getting-started.md
@github-actions

github-actions Bot commented Oct 4, 2026

Copy link
Copy Markdown

✅ PR Title Formatted Correctly

The title of this PR has been updated to match the correct format. Thank you!

@PaulTrampert
PaulTrampert merged commit 9beb9ea into release/2.0 Oct 4, 2026
5 checks passed
@PaulTrampert
PaulTrampert deleted the feature/126-remove-roslyn-builder branch October 4, 2026 21:46
@PaulTrampert PaulTrampert mentioned this pull request Oct 4, 2026
PaulTrampert added a commit that referenced this pull request Oct 4, 2026
Ships the 2.0 major release from `release/2.0`.

> [!IMPORTANT]
> Merge with **Rebase and merge**, never squash. Release notes are built
from the commit subjects since `v1.5.0`, so a rebase merge gives each PR
below its own line. A squash would collapse them into one.

## Included

- #139 Make `PatchClassBuilder`'s constructor internal (closes #75)
- #140 Target net10.0 only (closes #76)
- #141 Retype `PatchClassBuilder.Instance` to `IPatchClassBuilder` and
make `PatchClassBuilder` static (closes #135, closes #143)
- #142 Make Reflection.Emit the only runtime patch class builder (closes
#126)

## Breaking changes

- **.NET 10 or later only.** .NET 8 and 9 consumers stay on 1.x.
- **`PatchClassBuilder` is a static class.**
`PatchClassBuilder.Instance` is typed `IPatchClassBuilder`, and the
constructor is gone.
- **`UseExperimentalDynamicClassBuilder` is removed.** Reflection.Emit
is the only runtime builder. Internal write models are supported when
their assembly declares
`[InternalsVisibleTo("PTrampert.SimplePatch.Emitted")]`. The Roslyn
builder is kept but unused at runtime, pending #144.

## Branch state

`release/2.0` has no commits behind `main` and no merge commits. All
four commits are `(MAJOR)`, so the version calculation produces 2.0.0.

Postponed until after 2.0: #96 and #102, with open questions noted on
each.

## Tests

Each included PR passed the CI build and tests against `release/2.0`.
This PR's build runs the full suite on the combined branch.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant