Repository navigation
(PATCH): Drop the Roslyn patch class builder's own cache - #154
Merged
Merged
Conversation
Lets the Roslyn builder drop its own cache without the shared suite compiling a new class on every call. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
GetPatchClassFor now compiles a new class on every call; caching is CachingPatchClassBuilder's job. Replaces the cache tests with one that checks each call builds a new class. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
PaulTrampert
commented
Oct 4, 2026
| public Type GetPatchClassFor(Type type) | ||
| { | ||
| return OptionalsClasses.GetOrAdd(type, t => new Lazy<Type>(() => CreatePatchClass(t))).Value; | ||
| return CreatePatchClass(type); |
Owner
Author
There was a problem hiding this comment.
@claude Since this is just a call through to the private method, we can do away with the private method.
Owner
Author
There was a problem hiding this comment.
Done in 77b70dd: the body of CreatePatchClass now lives in GetPatchClassFor, and the private method is gone. Build warnings unchanged; all tests pass (131/12/18).
PaulTrampert
added a commit
that referenced
this pull request
Oct 4, 2026
…ilder (#155) ## What changes `PatchClassBuilder.Instance` now holds one `CachingPatchClassBuilder` around `EmitPatchClassBuilder.Instance`. The Emit builder no longer has a static cache, so it emits a new class in a new dynamic assembly on every call. The two changes go in one PR, as #149 says: either one alone would leave the runtime path with no cache, or with two. Things the diff doesn't make obvious: - **`Instance` is `{ get; } = new ...`, not `=> new ...`.** An expression body would create a decorator with an empty cache on every call, so every call would emit a new assembly. `Instance_IsTheSameBuilderEachTime` guards this. - **`CachingPatchClassBuilder` gets an `Inner` property** (the class is internal, so the public API doesn't change). It lets `PatchClassBuilderDelegationTest` check that `Instance` wraps the Emit builder, as the issue asks. A behavioural check, such as looking for the `PTrampert.SimplePatch.Emitted` assembly name, was the other option. I rejected it because it wouldn't tell "wraps the Emit builder" apart from "is the Emit builder". The decorator's lambda now reads `Inner` instead of the captured parameter, so the class holds a single field rather than two copies of the inner builder. - **The Emit cache tests moved.** These are same type on repeat, a single build under concurrent first use, and the count of emitted dynamic assemblies. They now run through `PatchClassBuilder.Instance` in `PatchClassBuilderDelegationTest`. `EmitPatchClassBuilderTest` keeps only the tests that are specific to Emit. - The comment in `EmitPatchClassBuilder` that mentioned `RoslynPatchClassBuilder`'s cache is gone along with the dictionary, so it doesn't conflict with #154 (#150). This PR doesn't touch the Roslyn builder or `PatchClassBuilders.cs`. - The "How it works" bullet in `AGENTS.md` now describes the decorator. No public API change. ## Tests - Regression: before the fix, `Instance_IsACachingBuilderAroundTheEmitBuilder` failed (1 failed, 132 passed). It passes after the fix. - `dotnet build`: the warning set is the same as on `main` (21 unique warnings, none new). - `dotnet test`: core 133/133, OpenApi 12/12, Swashbuckle 18/18. Closes #149 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
With the cache gone, GetPatchClassFor only forwarded to the private method. 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
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.
RoslynPatchClassBuilderisn't used at runtime (it's kept pending #144). Now thatCachingPatchClassBuilderexists (#148), the Roslyn builder no longer keeps a cache of its own.Changes
RoslynPatchClassBuilder: removed the staticOptionalsClassesdictionary and the comments about it.GetPatchClassFornow compiles and loads a new assembly on every call, and its doc comment says so and points toCachingPatchClassBuilder.Instancestays, and its summary now says the builder holds no state, rather than mentioning a static cache.PatchClassBuilders(the fixture source for the sharedPatchClassBuilderTestsuite) wraps each builder in aCachingPatchClassBuilder. Without that, the Roslyn fixture would compile a class on everyGetPatchClassForcall and the suite would slow down. The suite still tests what each builder produces, because the cache only returns what the inner builder built. The wrappers are static fields, so every time NUnit enumeratesAll()it gets the same two caches. The Emit builder is wrapped too, which is harmless while it still has its own cache (Route PatchClassBuilder.Instance through CachingPatchClassBuilder and drop the Emit builder's cache #149 removes that).RoslynPatchClassBuilderTest: removed the two cache tests (same type on repeat calls, one compile under concurrent first use).CachingPatchClassBuilderTestcovers that behaviour. AddedGetPatchClassFor_BuildsANewClassOnEachCall, which fails before the fix and passes after it.ConcurrentFirstUseTestObjectstays becauseEmitPatchClassBuilderTeststill uses it. A comment inEmitPatchClassBuilderstill mentions "RoslynPatchClassBuilder's cache". I left it alone so this PR doesn't touch the Emit builder, since #149 rewrites that part.No public API change.
Test results
dotnet build: succeeds, with 21 unique warnings both before and after this change.dotnet test: all passing. Core: 131. OpenApi: 12. Swashbuckle: 18.main, because the second call returned the same cached type.Closes #150
🤖 Generated with Claude Code