Repository navigation
fix(observed_macros): reject mutable-reference event fields - #730
Conversation
`is_reference_type` matched any `syn::Type::Reference` and never looked at `TypeReference::mutability`, so `#[event(...)]` accepted a `&mut T` field and generated a visit body for it. An event is visited through `&self`, so that body cannot hand out the exclusive borrow it asks for - the input was only rejected later, by the compiler, against generated code the author never wrote. Check mutability during field parsing and report it against the offending field type. `Option<&mut T>` is checked as well, because the visit body dereferences the inner type rather than the field type, and the check recurses through nested references so `&&mut T` is caught too. Shared references are untouched, which a companion snapshot pins. Fixes AB#7757620 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟢 Approval recommended
The change is narrowly scoped to parse-time validation for #[event(...)] fields and is backed by focused snapshot tests covering both rejection and non-regression cases.
Pull request overview
This PR hardens the observed_macros_impl #[event(...)] procedural macro by rejecting event fields that contain mutable references at parse time, producing a targeted diagnostic instead of allowing the input through to fail later in generated visit code.
Changes:
- Add
field_attrs::is_mutable_reference_typeto detect&mut T(including nested forms like&&mut T). - Enforce a new parse-time guard in
eventfield parsing that rejects&mut TandOption<&mut T>with a clear error message. - Add snapshot coverage proving mutable-reference fields are rejected while shared-reference fields still expand as before.
File summaries
| File | Description |
|---|---|
| crates/observed_macros_impl/src/field_attrs.rs | Introduces a helper to detect mutable references (including nested references) during type inspection. |
| crates/observed_macros_impl/src/event.rs | Adds a parse-time rejection for mutable-reference event fields and wires it into field parsing. |
| crates/observed_macros_impl/tests/expansion.rs | Adds new snapshot-driven tests for the new rejection behavior and for shared-reference non-regression. |
| crates/observed_macros_impl/tests/snapshots/expansion__an_event_field_may_not_hold_a_mutable_reference.snap | New snapshot asserting diagnostics for several mutable-reference patterns. |
| crates/observed_macros_impl/tests/snapshots/expansion__an_event_field_may_hold_a_shared_reference.snap | New snapshot asserting shared-reference expansions remain accepted. |
| crates/observed_macros_impl/CHANGELOG.md | Documents the fix in the Unreleased section. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #730 +/- ##
=======================================
Coverage 100.0% 100.0%
=======================================
Files 582 582
Lines 62852 62873 +21
=======================================
+ Hits 62852 62873 +21
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
## Summary Prepare 58 coordinated crate releases, including the maintainer's macro-pinning and version-alignment commits. Initially rebased onto `8624f907` on main, including #744 and the merged #733 runtime-recording work. Subsequently merged main at `704eba82` in `b1b8dc17`, including #722 (compressors) and #745 (metabench support identifiers), without rewriting the PR history. All 79 pre-merge package versions are preserved; the new compressors package retains upstream `0.1.0`. No new release versions were chosen during the merge. - Release `thread_aware 0.12.0` and affected consumers for `thread_aware_core` adoption; retain `thread_aware_core 0.1.1` as a compatible addition. - Release `rest_over_grpc 0.2.1`, including the query-field nesting-depth fix from #729. - Preserve the observed family at `0.26.0`, fundle at `0.4.0`, and data-privacy facade/macros at `0.13.0`. - Preserve the later maintainer updates: exact macro pins; the ohno family at `0.5.2`; internity and macros at `0.2.1`; metabench and macros at `0.1.1`; multitude and macros at `0.10.0`; and templated-uri and macros at `0.6.0`. - Retain `data_privacy_core 0.1.2`, `plurality 0.2.2`, and `recoverable 0.2.0` without release bumps. - Preserve the reviewed release-note cleanup: remove CI-only and misattributed feature notes, restore preambles, normalize new categories, preserve historical entries, and describe #729/#730 once each. Retain the verified `http_path_template 0.2.0` publication date, 2026-07-22. - Address remaining feedback in `18de2ac3`: correct the uniflight `stampede` keyword and qualify optional dependency notes by their features in rest_over_grpc and seatbelt_http. - Reconcile registry lockfile resolution in `f3d71c7e`: restore seven `windows-sys` references and the related `tempfile`/`getrandom` reference to main, without changing any package versions. Relative to current main, this is release metadata and generated documentation, plus the bytesbuf rustdoc URL correction. The incoming implementation changes from #733, #722, and #745 come from main and were preserved. The root manifest conflict was resolved by retaining the release's fetch package versions and main's new flate2 dependency. ## Required Release Review The author confirmed on 2026-09-10 that `performables` and `seismograph_cli` must remain at upstream `0.1.0`; do not add release bumps for them. This is an intentional exception to the release planner's normal dependent cascade. Their initial-publication handling and dependency closure are specified below; keeping these versions does not mean that their publication can be skipped. The initial review compared 46 ordinary libraries against previous version-bump commits with `cargo semver-checks --all-features --release-type patch`. Manual evidence also covered the public `ThreadAware` identity migration, public Syn 2-to-3 helper types, the observed mutable-reference macro input restriction, and ohno's warning-policy caveat. The pre-merge SemVer report identifies 10 proc-macro packages and 26 ordinary consumers requiring manual compatibility review, with no reported under-increment. Comparing the pre-rebase head `826feb24` with `65cc1af8` shows no changes in the macro packages or `routerama_build`, so the earlier macro-source evidence survives that rebase. The subsequent merge brings in #745's metabench macro support-identifier fix unchanged from main; its unit, expansion, and compile-contract tests passed against the release versions. Prior evidence does not establish compatibility of the consumer changes introduced by #733 or replace the fresh merged-head report. Final release review must confirm the macro/consumer classifications, particularly the patch boundaries of `internity 0.2.1` and `arty_executor 0.1.2`, routerama parser/macro compatibility, ohno's generated-code warning policy, and the post-#733 public-type/runtime implications. The automated SemVer warning is informational; passing builds and resolved review threads do not replace this release signoff. ## Post-rebase Validation - Passed at `65cc1af8`: locked workspace `cargo check --all-features`. - Passed at `65cc1af8`: `just clippy` for the workspace, all features and targets. - Passed at `65cc1af8`: 811 selected tests across rallocator, thread-aware, REST-over-gRPC, internity, and metabench packages (43 binaries). - Passed at `65cc1af8`: pinned formatting checks across all 79 workspace packages, README generation, whitespace checks, and editor diagnostics. - Passed: comparison of surviving package versions against the exact pre-rebase head `826feb24`; all 72 match. Current exact/caret workspace dependency requirements match target versions. - Confirmed: the old `thread_heap_usage_includes_remote_allocations` test is absent after #733. No test was disabled or assertion weakened in this release branch. - The `65cc1af8` CI run completed with 58 successful checks and one skipped check. - Passed for the feedback fix `18de2ac3`: locked Cargo metadata verification, `just anvil-cargo-sort`, `just anvil-clippy`, `just anvil-fmt --fix`, `just anvil-readme --fix`, `just anvil-spellcheck`, whitespace checks, and editor diagnostics. Regeneration produced no additional source/README changes. The local spelling-tool prerequisite is now installed. - Passed for the lockfile fix `f3d71c7e`: full locked/offline Cargo metadata, `just anvil-clippy` (all targets/features), `just anvil-fmt --fix`, `just anvil-readme --fix`, `just anvil-spellcheck`, whitespace checks, and editor diagnostics. All 568 registry-package lock entries exactly match main; all 79 non-registry entries remain unchanged from `18de2ac3`. `ring` retains its required `windows-sys 0.52.0` dependency. - Fresh CI for the latest head must finish successfully; results from preceding commits are not a substitute. Before the rebase, selected runs passed 2,691 tests for release consumers and 969 tests including thread-aware and ohno. Those results are historical. ## Main-merge Validation For `b1b8dc17`, merging main at `704eba82`: - Passed: full locked/offline Cargo metadata for 80 workspace packages. All 79 prior manifest versions are unchanged; compressors remains `0.1.0`. - Passed: all 576 registry-package lock entries exactly match incoming main. The Windows dependency-resolution correction is preserved. - Passed: 553 unit/integration tests across compressors, metabench, metabench_macros, and metabench_macros_impl; none skipped. - Passed: 26 doctests across those packages, with seven existing ignored examples. - Passed: generated Anvil workspace Clippy (all targets/features), manifest sorting, formatting, README regeneration, spelling, whitespace checks, and editor diagnostics. - Regenerated the compressors README to link to the release's bytesbuf `0.10.0`; incoming compressors and metabench macro implementation code matches main. - Confirmed: the release selector still selects 58 bumped packages against current main. No release versions were added or changed. - GitHub reports the conflict resolved. Fresh CI and approval of the latest reviewable push are required; prior checks do not validate this merge. ## Publication This PR prepares releases only. No crates have been published or yanked as part of this work. Registry checks on 2026-09-10 found all seven new #733 packages unpublished: - `performables` - `seismograph` - `seismograph_protocol` - `seismograph_io` - `seismograph_cli` - `seismograph_runtime` - `seismograph_rallocator` Preserve their initial versions. The normal comparison against current main selects the 58 version-bumped packages and excludes these seven initial packages and the newly merged compressors `0.1.0`. The existing 58 releases require `seismograph_protocol`, `seismograph`, `performables`, and `seismograph_io`; shipping the CLI additionally requires the runtime/rallocator helpers. Publish prerequisites before their consumers in Cargo dependency order. The compressors registry/publication status was not checked in this merge update. Before merging, the publishing maintainer must confirm one of these paths: 1. Complete and verify the #733 initial publications, then publish the normal 58-package release set; or 2. Explicitly include the required initial packages in a catch-up publication together with the 58 bumped packages. After merging #722, the repository's `Get-PackagesWithVersionChanges` helper selects exactly 66 packages when compared against pre-#733 commit `70bd5aafadb9b11fc45d8977bd808066e574b8de`: the 58 bumped packages, the seven #733 packages, and compressors. Confirm actual registry availability, equivalent selection, and dependency ordering in the publishing pipeline before execution; the earlier 65-package count applied before compressors joined main. `.ado/pipelines/publish.yml` exposes `previousVersion` (default `HEAD^`) but delegates publishing to the external `ox-sdk` template. Its catch-up behavior has not been verified or executed here. The official pipeline triggers on main and defaults `executePublish` to true, so publication selection must be confirmed before merge, not deferred until after it. Do not merge until required checks, final release review, and the initial-publication plan are confirmed. Do not rerun the release driver on the already-bumped branch merely to include these initial packages; that would re-increment existing releases rather than preserve the approved version plan. ## Tooling Follow-up The release generator inserts sections ahead of preambles, combines curated headings with list-style categories, and uses tag-based history that can lag declared releases. The release artifacts have been repaired; generator behavior and regression tests remain separate follow-up work. --------- Co-authored-by: Vaiz <4908982+Vaiz@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
🤖 Clawpilot here! Posted automatically by Clawpilot (an AI agent), not by a human. Please verify before acting.
What this changes
#[event(...)]now rejects a field that holds a mutable reference. The check runs while the macro parses fields, before it generates code. The error points at the field type. FixesAB#7757620.What you get
A classified field was rejected before, but the error named generated code:
Before —
rustcreportsE0507against the attribute:After — the error names the field and the rule:
An
#[unredacted]field behaves differently. It compiled before this change:The generated body read it through
self.v.clone(), which auto-dereferences tou64. The exclusive borrow bought nothing, becausevisit_fieldstakes&selfand never writes. This case is now rejected as well.Fix either case by removing
mut:v: &'a u64.Details
is_mutable_reference_typeinspectssyn::TypeReference::mutability. It recurses likestrip_reference, so&&mut Tis caught.reject_mutable_referenceruns inparse_field_def. It also reads the inner type of anOption<T>field, and names the optional form in the message.Effects
#[unredacted] v: &'a mut u64compiled before this change and does not compile after it. An owner of such a field must write&'a u64.&mut Tfield failed before withE0507spanned on#[event(...)]. It now fails with a message spanned on the field type.Option<&mut T>is rejected, with wording that names the optional form.&'a T,&'a &'a TandOption<&'a T>expand exactly as before, which a snapshot pins.#[derive(Enrichment)]is deliberately unchanged.into_entries(self)takesselfby value, so a&mut Tenrichment field still compiles. Only#[event(...)]gained the check.Cargo.lockchange.observedpublic API is unchanged. Both new functions are internal toobserved_macros_impl.Validation
Manual before-and-after repro, not committed: three event structs were compiled against the macro with the check disabled, then enabled. This confirmed that the
#[unredacted]case compiled before the fix, that the classified case producedE0507against the attribute, and that a shared-reference classified field still compiles after the fix.