refactor(ohno_macros_impl): let the shape place the core and the grammar split a #[from] entry - #715
refactor(ohno_macros_impl): let the shape place the core and the grammar split a #[from] entry#715Evgenii (Vaiz) wants to merge 1 commit into
#[from] entry#715Conversation
b1f7763 to
0d4b21f
Compare
Codecov Report❌ Patch coverage is
❌ Your project check has failed because the head coverage (99.9%) is below the target coverage (100.0%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #715 +/- ##
========================================
- Coverage 100.0% 99.9% -0.1%
========================================
Files 563 563
Lines 61068 61071 +3
========================================
+ Hits 61068 61070 +2
- Misses 0 1 +1
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:
|
0d4b21f to
fa3a6c3
Compare
…mar split a `#[from]` entry The `#[from(...)]` entry parser told a parenthesized type apart from an override list with a hand-written look-ahead over `syn::buffer::Cursor`, matching a punct character and its `Spacing` so the `::` of a path would not read as a member followed by a colon. That reimplements, by hand, a question the two grammars already answer: an override list is what the override parser accepts. Trying it on a forked stream decides the same case without knowing how far a path reaches, and drops the `Delimiter`/`Spacing`/`Cursor` imports with it. An empty pair of parentheses is excluded from that trial, so `()` decodes as the unit type, and validate then rejects it against the `()` itself. It was accepted before, but never usably: `OhnoCore::from` needs `T: Into<Box<dyn Error + Send + Sync>>`, which `()` does not satisfy, so the generated `From<()>` failed with E0277 inside code the user never wrote — the diagnostic R4 forbids. Reading `()` as an empty override list instead would report a decoding failure, and since the entries of one attribute parse as a single list, that would discard every other entry alongside it. Rejecting it in validate keeps the rest of the list intact. `Shape` is split around the field holding the core precisely so nothing has to search for it, but both generators searched anyway: each walked the full field list and compared every member against the core's to decide how to initialize it, and the conversion generator threaded a counter alongside that walk to index its tuple, guarded by an `expect` — a panic path in the phase that is required not to have one. `Shape` now yields declaration order with each field carrying a `Position`, and the shared builder takes the core's initializer apart from the rest, so neither caller compares members and neither carries an alignment of its own. The builder pairs each member with its value in one walk, rather than zipping two iterators that agree only because they share a source. `Conversion` compared `#[from(...)]` keys against field members by formatting both to `String`, and built every initializer before checking whether the keys named anything. `syn::Member` is already `PartialEq` under `extra-traits`, which the crate enables and `constructors.rs` already relied on; the keys are now checked first, so a rejected conversion builds nothing. `validate` no longer clones each override into a `(Member, Expr)` pair to hand it over. `member_name` moves out of the parse phase, which the generate phase was reaching into for it, and `is_ohno_core` moves to the one phase that uses it. Rejecting `()` is the one behavior change, and it turns code that failed to compile into code that fails to compile with a message naming the cause; every expansion snapshot and every pre-existing `.stderr` snapshot is unchanged. A new `tests/ui` fixture pins the rejection against the real compiler, including that a rejected `()` beside a second faulty entry still reports both. Seven cases are added to the expansion snapshots, all previously uncovered: the three readings of a leading parenthesis the entry parser has to tell apart, a tuple struct whose core sits between two data fields, `()` and `(())` as source types, and the two-fault accumulation case. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
fa3a6c3 to
29b213e
Compare
|
|
🤖 Clawpilot here! Automated review comment from an AI agent -- not a human reviewer. Please verify before acting. AI reviewer: Claude Opus 5
The paragraph above it already justifies the rule without appealing to R4: The alternative is to widen |
|
|
||
| /// Renders a member the way a diagnostic spells it, and the way `Debug` labels it: `path`, or `0`. | ||
| /// | ||
| /// Shared by all three phases: a member reaches the user as text in a diagnostic, in a template |
There was a problem hiding this comment.
🤖 Clawpilot here! Automated review comment from an AI agent -- not a human reviewer. Please verify before acting.
AI reviewer: Claude Opus 5
member_name has no call site in parse.rs, and had none before this change either. The callers are model.rs:203, model.rs:215, display/mod.rs:41, display/mod.rs:50 and generate/traits.rs:138. The display module is imported and called only from validate.rs, and unknown_key is a validation diagnostic, so the users are validate and generate — two of the three phases the module doc names above.
The count is load-bearing for a later reader deciding whether this helper can move back down into one module, so make it say two: "Shared by validate and generate: a member reaches the user as text in a diagnostic, in a template resolution and in a Debug label, and the three have to agree on how it is spelled." The list of three uses is correct; only the phase count is not.
| @@ -91,6 +112,15 @@ impl Shape { | |||
| self.before.iter().chain(self.after.iter()) | |||
There was a problem hiding this comment.
🤖 Clawpilot here! Automated review comment from an AI agent -- not a human reviewer. Please verify before acting.
AI reviewer: Claude Opus 5
Position::Data is documented as "numbered as Shape::data yields it", and the generated conversions depend on that: Conversion::new lays out initializers by walking data(), while construct emits __ohno_fields.#index from the Position::Data(index) that positions() hands out. Those two orders come from two independent iterator expressions — positions() walks before/core/after with its own offset, this line walks before/after.
They agree today. If data() later gains a filter or a different chain order, positions() keeps numbering the old way and every generated From<T> assigns initializers to the wrong fields, with no compile error — only the snapshots would catch it. That is the second alignment the positions() doc argues against a few lines up. all() was rewritten in this PR to go through positions(); data() can do the same, and then the invariant stops being a comment:
pub(crate) fn data(&self) -> impl Iterator<Item = &ModelField> {
self.positions()
.filter_map(|(field, position)| matches!(position, Position::Data(_)).then_some(field))
}Low priority as written, since the two definitions sit a few lines apart today.
| /// of the shape. A generator that recovered it by comparing members would be re-deciding, at | ||
| /// every use, something the split around the core already settled. | ||
| pub(crate) fn positions(&self) -> impl Iterator<Item = (&ModelField, Position)> { | ||
| let after = self.before.len(); |
There was a problem hiding this comment.
🤖 Clawpilot here! Automated review comment from an AI agent -- not a human reviewer. Please verify before acting.
AI reviewer: Claude Opus 5
This binds the count of the fields before the core to after, which is also the name of a field on this struct. Eleven lines down it reads Position::Data(after + index) inside the closure that walks self.after, where it looks like "the number of fields after the core, plus this one's index" — the wrong quantity, and one that would give the wrong answer.
The value is correct: the after fields do start at before.len() in the data numbering. But this is the one function the generated conversions' field ordering depends on, so the name should not point the reader the other way. Rename it: let data_before_core = self.before.len();, used as Position::Data(data_before_core + index).
1 participant
🤖 Clawpilot here! Posted automatically by Clawpilot (an AI agent), not by a human. Please verify before acting.
Mostly an internal refactor of
ohno_macros_impl. One input that used to be accepted is now rejected, deliberately. No code that compiles today stops compiling — see Compatibility below.The corner this is really about
A
#[from(...)]entry is a type, optionally followed by(field: expression, ...)overrides. That makes a(in the leading position genuinely ambiguous, and these all mean different things:Telling them apart is the parser's whole job here. Before this PR, exactly one of them had a test. All of them do now.
What you write, and what you get
produces this, byte-identically before and after the change:
so ordinary usage is untouched:
Field overrides, several source types in one attribute, generic sources whose commas belong to the type rather than to the list, tuple structs, and a core field placed anywhere in the struct all produce identical output too. Every pre-existing expansion snapshot is unchanged.
()is now rejected#[from(())]used to be accepted by the macro. It was never usable:OhnoCore::fromrequiresT: Into<Box<dyn Error + Send + Sync>>, which()does not satisfy, so the generatedFrom<()>failed to compile — pointing into code the user never wrote, whichdesign.mdR4 forbids.Before, on
main:After, pointing at what you actually wrote:
#[from((()))]is the same type and gets the same message.((),)is a genuine one-element tuple and is left alone.The rejection is a rule, checked in the validation phase, and that placement matters.
#[from(...)]parses its entries as a single list, so had the parser instead treated()as an empty override list and failed at decode time, one()would have discarded every other entry in the attribute. A newtests/uifixture pins that it does not:#[from((), std::io::Error(missing: 1))]reports both the
()and the unknown field, rather than losing the second behind the first.Compatibility
Not a breaking change, in the sense that matters: no source that compiles against
mainfails to compile after this.#[from(())]— verified empirically, not assumed.OhnoCorehas exactly oneFromimpl, bounded onInto<Box<dyn StdError + Send + Sync>>, andOhnoCore::from(())is rejected byrustcwithE0277. So any crate containing#[from(())]already failed to build. Code that did not compile still does not compile; only the diagnostic improves.ohno_macros_implexports the same three functions with the same signatures, and no type inohnois touched, socargo-semver-checkshas nothing to report..stderrsnapshot covered it.#[from(())]E0277in generated code()#[from(std::io::Error(path: ))]expected a typeunexpected token, expected ")"Diagnostics you see when you get it wrong
Otherwise unchanged, message for message and span for span — the compile-fail tests that pin them are untouched:
Why touch it at all
Three things were re-derived by hand that a type or a grammar already held. The entry parser restated part of Rust's path grammar as raw token peeking, so that
(std::io::Error)would not read as a field namedstd; it now settles the question by trying to parse an override list and seeing whether that succeeds. Both code generators re-found the core field by comparing member names, and one threaded its own index alongside that walk guarded by anexpect— a panic path in the generation phase, whichdesign.mdrequires not to have one; the shape now reports each field's position directly. Finally, override keys were compared by formatting both sides toString, wheresyn::Memberalready implementsPartialEq.Implementation source shrinks by roughly 20 lines. The rest of the diff is test coverage, plus the
design.mdandrequirements.mdupdates for the()rejection.Validation
ohno_macros_implandohnoboth pass on the pinned MSRV, including thetrybuildcompile-fail tests that pin the diagnostics. Clippy is clean under-D warnings, and both formatting gates pass on the pinned nightly. The parser change was checked against the previous implementation with a differential probe over 68 adversarial#[from(...)]entries;()is the only input whose outcome moves.darlingwas evaluated first and rejected: its derives parse each attribute throughNestedMeta, which cannot represent#[display("{a}", self.x())], a#[from(...)]entry carrying generic arguments orkey: exprpairs, or bare markers such as#[no_debug]. Adopting it would have added a dependency without removing any of the code above.