fix(ohno_macros): correct the syntax context of a rewritten unit struct and the #[automatically_derived] placements - #723
Open
Evgenii (Vaiz) wants to merge 6 commits into
Open
Conversation
…nit struct `#[ohno::error]` turns a unit struct into a tuple struct so it has room for the `OhnoCore` field. The parentheses and the semicolon it synthesises for that came from `Paren::default()` and `<Token![;]>::default()`, both of which carry `Span::call_site()`. The rewritten item therefore ended up in the macro's syntax context rather than the caller's. `rustc` reports `dead_code` on a struct at its identifier span re-tagged with the item's syntax context, and cancels any lint whose primary span sits in an external macro expansion. So an error type declared as a unit struct was never reported as dead, and a caller's `#[expect(dead_code)]` on it could never be fulfilled — it surfaced instead as `unfulfilled_lint_expectations`, which fails a `-D warnings` gate with no hint that ohno was involved. Taking the spans from the declaration's own identifier keeps the item in the caller's context. Only the unit shape was affected; the named and tuple shapes push a field onto delimiters the author wrote, and `#[derive(ohno::Error)]` never rebuilds the item at all. Generated tokens are unchanged, so the expansion snapshots are untouched. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The change is narrowly scoped to span construction for unit-struct rewriting and aligns with the stated diagnostic/context issue without introducing broader behavioral changes.
Pull request overview
This PR adjusts #[ohno::error]’s unit-struct rewrite so that newly synthesized delimiters (the tuple parens and trailing semicolon) inherit the caller’s syntax context, avoiding dead_code diagnostics being treated as “external macro” and therefore not fulfilling a caller’s #[expect(dead_code)].
Changes:
- For unit structs, construct
syn::token::Parenandsyn::Token![;]using the struct identifier’s span instead ofSpan::call_site(). - Add an inline comment documenting the lint-context rationale for future maintainers.
File summaries
| File | Description |
|---|---|
| crates/ohno_macros_impl/src/error_attr/mod.rs | Preserves caller syntax context when rewriting unit structs by sourcing delimiter spans from item.ident.span(). |
Review details
- Files reviewed: 1/1 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 #723 +/- ##
=======================================
Coverage 100.0% 100.0%
=======================================
Files 587 587
Lines 63002 63007 +5
=======================================
+ Hits 63002 63007 +5
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:
|
…rs impl
`#[automatically_derived]` is accepted only on a trait `impl`. The derive put
it on the inherent `impl` holding `new` and `caused_by`, where `rustc` rejects
it with "this was previously accepted by the compiler but is being phased out;
it will become a hard error in a future release".
Nobody sees that warning today, because lints raised inside an external macro
expansion are discarded — including after the unit-struct span fix, since these
tokens come from the derive rather than from the caller's declaration. So it is
latent: it costs nothing now and breaks every `#[ohno::error]` and
`#[derive(ohno::Error)]` user at once on the release that promotes it.
Removing it changes no behaviour. The attribute marks an impl as machine-written
so `dead_code` skips field reads within it, which is why `Debug` deliberately
goes without one; the constructors carry their own `#[allow(dead_code)]` and
never relied on it. The five trait impls the derive emits keep theirs, as those
are the placements the attribute is for.
Snapshots re-recorded: 35 deleted lines, every one of them the attribute above
an inherent `impl T {`.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Evgenii (Vaiz)
changed the title
#[automatically_derived]
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The changes are small, targeted, and align generated code with correct span hygiene and valid attribute placement without introducing new API surface.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Lite
… applies The previous commit left the design describing the attribute twice, once per exception, with each paragraph defined in terms of the other — "one of the two generated items", "the other item". That reads as a retrofit and puts a rule that governs every generated item inside the entries for two of them. State it once in the section preamble, beside the generics rule it resembles: the attribute marks an `impl` as machine-written so dead-code analysis skips the field reads inside it, and rustc accepts it only on a trait `impl`. Both exceptions then follow from the rule instead of being asserted next to it. `Debug` keeps its own paragraph, because omitting the attribute there is a deliberate trade rather than a consequence of where it is allowed, and a reader who does not know that would eventually "fix" it. The constructors need no paragraph of their own now that the preamble covers them. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The unit-struct rewrite constructs syn::token::Paren in a way that does not correctly populate its delimiter span field in syn 3.x, undermining the intended caller-context preservation.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 1
- Review effort level: Lite
Comment on lines
109
to
111
| let mut unnamed = FieldsUnnamed { | ||
| paren_token: syn::token::Paren::default(), | ||
| paren_token: syn::token::Paren(span), | ||
| unnamed: syn::punctuated::Punctuated::new(), |
The comments and design text added here carried prose that only makes sense to
a reader who knows what the code used to do, and that expires on its own: a
compiler roadmap note ("rustc warns there today and states it will become a
hard error"), and a module doc whose whole subject was the absence of an
attribute rather than anything the module does.
Neither survives its own change. Once the attribute is a hard error, "warns
today" is wrong; and a note explaining why an invalid attribute is missing
tells a reader nothing they could have acted on, since the compiler rejects it
without help. The rule that matters — the attribute is accepted only on a trait
`impl` — is already stated where the generated items are described, so the
absence needs no commentary of its own.
The span comment stays, reworded as a constraint rather than a contrast: it
guards a real trap, since `Paren::default()` is the obvious spelling and
silently puts the item in the wrong syntax context.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The changes are narrowly scoped, address concrete macro/codegen correctness issues, and the accompanying documentation and snapshot updates are consistent with the new generated output.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Lite
The design spent a paragraph on `#[automatically_derived]`, half of it on the constructors' inherent `impl`. That half is not a design decision: the attribute is only accepted on a trait `impl`, so an inherent one could never carry it, and a reader can act on none of it. What is worth stating is that generated trait `impl`s are annotated, and that `Debug` is the exception. One line covers the first; the `Debug` entry already covered the second and now reads as a single sentence, so the rule and its reason are not split across two. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The changes are narrowly scoped to span hygiene and removing an invalid attribute, with documentation and snapshot updates aligned to the new behavior.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
…ved]`
`Debug` carries `#[rustc_trivial_field_reads]`, so an `#[automatically_derived]`
`Debug` impl has its field reads discarded by dead-code analysis. The generated
impl went without the attribute to avoid that, on the grounds that it would make
a field only `Debug` reads look unused.
It does, and that is the right answer. A field nothing but `Debug` reads is
unused, `#[derive(Debug)]` reports it, and the remedy is the author's:
`#[allow(dead_code)]`, an accessor, or a place in the `#[display]` message.
Withholding the attribute suppressed that report for every error type in every
consumer crate, so a field left behind by a refactor was never flagged.
The suppression was also broader than the reasoning assumed. `Display`, `Error`,
`Enrichable` and `ErrorExt` are not `#[rustc_trivial_field_reads]`, so their
reads still count: the core stays live through all four, and a field named by
the `#[display]` template stays live through `Display`. Measured on a crate
carrying all four cases, only the `Debug`-only field is reported.
Nine fields in this crate's own examples and tests were relying on the
suppression. Each is handled on its merits rather than silenced:
- `default_constructors` and `derive_with_fields` gain the `#[display]`
message they should always have had, which reads the fields and makes the
examples print something other than the type name.
- `constructor_integration` asserts the values the constructor stored, which
the sibling test already did.
- the rest carry `#[expect(dead_code)]` with a reason, being fields that exist
to demonstrate a struct shape or to be compared through `Debug`.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Evgenii (Vaiz)
changed the title
#[automatically_derived]#[automatically_derived] placements
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
It changes proc-macro hygiene and consumer-visible lint behavior, which warrants final human review despite the updates being internally consistent.
Review details
- Files reviewed: 19/19 changed files
- Comments generated: 0 new
- Review effort level: Lite
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.
🤖 Clawpilot here! Posted automatically by Clawpilot (an AI agent), not by a human. Please verify before acting.
Three fixes to
#[ohno::error]/#[derive(ohno::Error)]. The third changes what consumers see — please read that part first.1. A unit struct is emitted in the macro's syntax context
An error type declared as a unit struct is never reported by
dead_code, even when nothing in the crate constructs or names it. A caller who writes#[expect(dead_code)]on such a type getsunfulfilled_lint_expectationsinstead, which fails a-D warningsgate with nothing pointing at ohno as the cause.Cause
#[ohno::error]gives a unit struct room for theOhnoCorefield by turning it into a tuple struct. The parentheses and the semicolon it synthesises for that came fromParen::default()and<Token![;]>::default(), both of which carrySpan::call_site(), so the rewritten item landed in the macro's syntax context rather than the caller's.rustc_passes::deadreports a struct atident_span.with_ctxt(def_span.ctxt())— the identifier's span re-tagged with the item's context — andlint_levelcancels any lint whose primary span sits in an external macro expansion. Thedead_codediagnostic was therefore dropped before it could fulfil the expectation.Only the unit shape is affected. The named and tuple shapes push a field onto delimiters the author wrote, and
#[derive(ohno::Error)]never rebuilds the item at all.Fix
Take the synthesised spans from the declaration's own identifier. Measured against a consumer crate declaring all four forms with
#[expect(dead_code)], on rustc 1.97 (ok= the expectation is fulfilled):#[ohno::error] struct E;#[ohno::error] struct E(u32);#[ohno::error] struct E { n: u32 }#[derive(ohno::Error)] struct E(#[error] OhnoCore);Why there is no regression test
A test would have to assert that a
#[expect(dead_code)]is fulfilled, which means denyingunfulfilled_lint_expectations. That cannot pass on the repo's MSRV: rustc kept an error type alive through the#[allow(dead_code)]on its generated constructors until rust-lang/rust#154377 landed in 1.97, andRUST_MSRVis1.95. Such a test becomes possible once the MSRV reaches 1.97.That rustc bug is a separate, second cause of the same symptom: on 1.93–1.96 every shape is affected, including the derive form, and no change to ohno avoids it. This PR fixes the part that is ohno's, and is the part still reproducible on 1.97 and later.
2.
#[automatically_derived]on the inherent constructors implThat attribute is accepted only on a trait
impl. The derive put it on the inherentimplholdingnewandcaused_by, whererustcanswers:No consumer sees that warning, because lints raised inside an external macro expansion are discarded. The unit-struct fix above does not change that — those tokens come from the derive rather than from the caller's declaration, which was verified by reverting the attribute independently and re-running a consumer crate. The defect is latent: it costs nothing until the release that promotes it to an error, at which point it breaks every
#[ohno::error]and#[derive(ohno::Error)]user at once.Removing it changes no behaviour. The constructors carry their own
#[allow(dead_code)]and never relied on it.3. The generated
Debugnow carries#[automatically_derived]This one changes what consumers see.
Debugcarries#[rustc_trivial_field_reads], so an#[automatically_derived]Debugimpl has its field reads discarded by dead-code analysis. The generated impl went without the attribute deliberately, to avoid making a field that onlyDebugreads look unused.It does make it look unused — and that is the correct answer. Such a field is unused,
#[derive(Debug)]reports it, and the remedy belongs to the author:#[allow(dead_code)], an accessor, or a place in the#[display]message. Withholding the attribute suppressed that report for every error type in every consumer crate, so a field left behind by a refactor was never flagged.The suppression was also wider than the original reasoning assumed.
Display,Error,EnrichableandErrorExtare not#[rustc_trivial_field_reads], so their reads still count. Measured on one crate carrying all four cases, rustc 1.95:#[display("cannot open {path}")]DebugonlySo the core stays live through the other four impls, and a field named by the
#[display]template stays live throughDisplay. Only a genuinely unused field is reported.Effect on this repo
A workspace check surfaced 9 such fields, all in
ohno's own examples and tests, none in any other crate. Each is handled on its merits rather than silenced:default_constructorsandderive_with_fieldsgain the#[display]message they should always have had, which reads the fields and makes the examples print something other than the type name.constructor_integrationnow asserts the values the constructor stored, which the sibling test already did.#[expect(dead_code)]with a reason — fields that exist to demonstrate a struct shape, or to be compared throughDebug.Effect on consumers
A consumer with an error field that nothing reads but
Debugwill get a newdead_codewarning, and a build failure under-D warnings. That is the point of the change, but it is a visible break and worth a release note.Validation
cargo test --all-featuresforohno,ohno_macrosandohno_macros_implpasses on MSRV 1.95 — 49 + 53 unit tests, 12 trybuild UI tests, all insta snapshots — as dojust anvil-fmt,just anvil-clippy, the nightlyunstable-rustfmt.tomlcheck, and a cleancargo check --workspace --all-targets.Related
Reported internally as AB#7829355, whose original diagnosis blamed the constructors'
#[allow(dead_code)]; that work item now carries the corrected analysis. Real-world instance: AssistantsOxide PR 5605707 deleted an#[expect(dead_code)]purely to work around part 1. That removal remains necessary there until the repo moves offms-prod-1.95, because the rustc half applies regardless of this PR.