Repository navigation
Preserve readonly struct fields when calling mutating members - #592
Merged
Merged
Conversation
Owner
|
@jogibear9988 thank you |
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.
Calling a mutating struct method on a readonly instance field currently changes the original field. Load its value into the existing temporary-address path before calling the member, matching System.Linq.Expressions. Writable fields and explicit assignment/ref paths retain their address handling.
The regression fixture checks repeated calls, writable controls, static readonly fields, nested readonly receivers, and receiver evaluation exactly once, using strict CompileFast and System comparison.
Allocation tradeoff: compiling the measured readonly struct receiver costs 8 additional bytes per compilation (944 to 952 B/op) in both Standard and LightExpression, on .NET 8 and 10. The defensive copy requires a local. This is an accepted correctness cost; it is not allocation-neutral. Runtime allocation remains zero. Other measured working paths retain their allocation counts and emitted IL.
Validation
Baseline: fetched
dadhi/masterate2c78de038244228f978f93ed89f40f30daec680. The regression fixture fails on master and passes with this change, for both input APIs.Release solution build: zero warnings/errors. Full runner results:
Performance measurements
Windows ARM64, .NET 8.0.28 and 10.0.9, Release, tiering disabled. Prebuilt trees; Light conversion occurs outside timing. Separate assembly contexts, one CPU, nine alternating samples per phase, two fresh process rounds with reversed load order, roughly 100 ms pair calibration, and GC allocation counters. Tables show the median of the 18 samples for compilation and delegate invocation; ns/op and allocated B/op are master → patch. All measured expressions already produce correct results on master.
Timing limitation: this machine shows substantial variation even comparing master with itself (individual process medians ranged from -43.5% to +16.8% for compilation and -46.9% to +55.5% for invocation). These results do not establish a speedup or prove CPU-time neutrality. Slower observations are included below. Allocation and IL comparisons are more stable.
All measured working cases, including controls
add_controlnested_field_readreadonly_readwritable_readadd_controlnested_field_readreadonly_readwritable_readadd_controlnested_field_readreadonly_readwritable_readadd_controlnested_field_readreadonly_readwritable_read