Fix SG evolver construction for required-member aggregates (marten#4542)#362
Closed
jeremydmiller wants to merge 1 commit into
Closed
Fix SG evolver construction for required-member aggregates (marten#4542)#362jeremydmiller wants to merge 1 commit into
jeremydmiller wants to merge 1 commit into
Conversation
…tes (marten#4542)
The Apply-only / null-snapshot branch synthesized `new T { Required = default! }`
for any aggregate with `required` members. For a primary-constructor record
(`record DiagnostiekActiviteit(string Id) { required ... }`) that object
initializer needs a parameterless ctor the record doesn't have, so the
generated evolver failed to compile with CS7036 (or CS9035 when the id was
supplied positionally).
Guard the `default!` object initializer on the type actually having an
accessible public parameterless constructor (or being a struct). When there
isn't one — a primary-ctor record — fall back to
`RuntimeHelpers.GetUninitializedObject(typeof(T))` instead, which allocates
without invoking a ctor or enforcing required members; Apply then populates
them. `default!` is now only ever emitted when there is a public no-arg ctor
for the initializer to call.
Verified end-to-end against Marten release/9.0.0: a partial
SingleStreamProjection over a required-member primary-ctor record fails to
build with the released generator and both builds and runs correctly with
this fix.
Overlaps with #359 (which converges on the same construction rule and also
addresses marten#4543 plus the redundant standalone-evolver emission); kept
here as a focused, regression-tested fix for the marten#4542 construction bug.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Member
Author
|
Closing in favor of #359, which is the more complete fix. I verified empirically that this focused PR resolves only a narrower slice of marten#4542 (the partial-projection construction expression). When I added #359's own test cases to this branch, both failed:
Conversely, #359 passes its own 18 tests and the 3 construction-focused tests from this PR (21/21), so it's a strict superset and converges on the same construction rule (emit Thanks @erdtsieck — #359 is the one to land. Happy to contribute the 3 extra construction-focused regression tests from here if useful. |
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.
What changed
The source-generated evolver's Apply-only / null-snapshot branch synthesized
new T { Required = default! }for any aggregate withrequiredmembers. For a primary-constructor record — e.g.record DiagnostiekActiviteit(string Id) { required Guid? SubContractorId; ... }— that object initializer needs a parameterless constructor the record doesn't have, so the generated*.Evolver.g.csfailed to compile with CS7036 (or CS9035 when the id was supplied positionally).This guards the
default!object initializer on the type actually having an accessible public parameterless constructor (or being a struct). When there isn't one (a primary-ctor record), it falls back toRuntimeHelpers.GetUninitializedObject(typeof(T)), which allocates without invoking a ctor or enforcing required members;Applythen populates them.default!is now only ever emitted when there's a public no-arg ctor for the initializer to call.Why
Addresses JasperFx/marten#4542. A required-member record aggregate projected by a partial
SingleStreamProjectionwith conventionalCreate/Applyproduced uncompilable generated code.Validation
dotnet test src/JasperFx.Events.SourceGenerator.Tests— green (added regression tests: record-with-Create/Applycompiles; recordApply-only synthesizes viaGetUninitializedObject, neverdefault!; required-member plain class still synthesizes via thedefault!object initializer — Marten'ssample_external-account-linkpattern).release/9.0.0: a partialSingleStreamProjectionover a required-member primary-ctor record fails to build with the released generator (CS7036) and both builds and runs correctly with this fix (event stream →Create→Apply→ loaded document has every required member set).Relationship to #359
This converges on the same construction rule as #359 (emit
default!only with a public parameterless ctor; otherwiseGetUninitializedObject). #359 is broader — it also fixes marten#4543 (the nullable hint-nameAddSourcecrash) and removes the redundant standalone-evolver emission when a partial projection already covers the aggregate.This PR is a focused, regression-tested fix for the marten#4542 construction bug. Maintainer's call whether to land this and bring #359's marten#4543 + duplicate-evolver fixes separately, or land #359 as the superset.