F5: cbindgen's struct-field classification reads the model, not the projection (#211) - #225
Conversation
…rojection #212 made the struct-field path read `SourceType` from `Registry::source_struct`, but then threw the model away: every field was projected back through `to_syn()` and re-classified with `is_string` / `is_bool` / `is_scalar` / `is_scalar_array`. The frontend had already decided all of that, so the projection was a second authority on one fact -- the drift #211 exists to stop. `c_field_wire`, `is_scalar_array`, `data_field_wire`, `data_field_owns`, `restricted_validity_field`, and the `in_data_struct` / `out_data_struct` field loops now take a `SourceType` and match it. The rule this establishes, for the stages after it: SHAPE FROM THE MODEL, IDENTITY FROM THE REGISTRY. "Is this field a `String`, a `bool`, an array of scalars?" is a source fact the frontend decided; asking syntax again is the duplicate. "Is this the type the binding declared as a `tagged_union`?" is a registry lookup that happens to be keyed by `TypeKey`, i.e. by a `syn::Type` -- rekeying that is F4's job, not this one, so those sites project and say why. `mirror_field_wire` deliberately stays on syntax. It is ONE policy serving both the `repr_c_struct` mirror path, which has a model, and the tagged-union payload path, whose variant fields the frontend does not model yet. Splitting it in two would duplicate the policy, which is the thing being deleted here. It waits on F2 modeling enums. The ledger caught the win on its first real use: `api/lang/cbindgen/mod.rs` 6 -> 5, total 196 -> 195, because `is_scalar_array` no longer matches `syn::Type::Array`. Note what that number does NOT capture -- `is_string`, `is_bool` and `is_scalar` were never in the ledger at all, since they classify by ident name, one of the blind spots its header lists. The count moved by one; the duplication deleted was larger. That is the ledger working as designed, not a measure of the change. Generated C artifacts byte-identical (`regen-check.sh` clean); 428 lib tests and the full workspace suite green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
milyin
left a comment
There was a problem hiding this comment.
Review of 7e8802e: the implementation is coherent. The migrated data-struct,
ownership, restricted-validity, and converter branches now take their shape
from SourceType; the remaining projections are identity lookups against
TypeKey-backed declarations. The nested scalar-array rule is behaviorally
equivalent for canonical frontend types and now correctly avoids treating
foreign paths ending in String, bool, or a scalar name as built-ins.
I found no code correctness issue, but one contract comment still describes the
state before this PR:
struct_fields rustdoc says field classification still uses syntax
prebindgen/src/api/lang/cbindgen/emit.rs:318 currently says:
The per-field classification ... still runs on SourceType::to_syn();
migrating that off syn is stage F5 ... not this change.
That is now the opposite of the implementation and of
docs/source-frontend.md: this commit is the F5 struct-field migration, and
the listed shape consumers classify SourceType directly. Please update this
paragraph to state the new split—shape comes from the model, while TypeKey-backed
identity queries (and the deliberately deferred shared mirror_field_wire
policy) may still project to syntax. In a boundary-migration series, leaving
this comment stale risks directing the next change back toward the duplicate
authority this PR removes.
Verification:
cargo test -p prebindgen --all-features: 429 passed; doctests passed./examples/regen-check.sh: generated output byte-identicalgit diff --check: clean- all four GitHub checks: green
The paragraph still said per-field classification runs on `to_syn()` and that migrating it was future work -- the opposite of what this branch does, and of docs/source-frontend.md. In a boundary-migration series a stale contract comment is worse than none: the next change reads it and heads back toward the duplicate authority this PR removed. Replaced with the rule itself -- shape from the model, identity may still project through TypeKey until F4 -- plus the one deliberate exception, `mirror_field_wire`, and why splitting it would duplicate a policy rather than delete one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…211) (#226) * F2: model enums, and move cbindgen's whole enum path onto the model Everything still open in #211 was blocked on one fact: the model covered struct fields and nothing else. This lands enums, which is what unblocks F5's `mirror_field_wire` and is a precondition for F6. Not a new model. `types_util` already described an enum language-neutrally -- `SumSpec` / `SumVariant` / `SumField`, tag plus one field group per variant, with jnigen already consuming it. Writing a second enum model beside it would have been exactly the duplicate authority #211 exists to remove, so this MOVES them into the frontend and completes them: * payload types are lowered through `lower_type` instead of being carried as raw `syn::Type` -- the change that lets cbindgen ask the model; * `EnumShape` / `enum_shape` / `first_payload_variant` fold in as `SourceEnum::is_unit` / `first_payload_variant` / `SourceVariant::shape`, so the shape question stops being a match on `syn::Fields` at each use site; * the discriminant is modeled. THE DISCRIMINANT IS AN `ArrayExtent` PROBLEM AGAIN. The two backends need different halves of one fact: cbindgen re-emits the SPELLING verbatim into its `#[repr(C)]` mirror, deliberately, so a `const`- or `cfg`-driven value keeps working and `= 0x07` stays `0x07`; jnigen needs the NUMBER, for a Kotlin `NAME(n)` entry and the `jint -> variant` decode. So `Discriminant { value: Option<i64>, source: DiscriminantSource }`, the same split `ArrayExtent { value, source }` already makes for an extent. `value: None` turns a panic reachable from `enum_class!` into a refusal naming the variant. `DiscriminantSource::Explicit(syn::Expr)` is the model's one carrier of open syntax -- deliberate, since narrowing it would delete C support that exists today, and now listed under F7. With variant payloads modeled, cbindgen's whole enum path follows the rule #225 set -- shape from the model, identity from the registry: `mirror_field_wire`, `payload_field_wire`, `payload_wire_owns`, `payload_needs_converter`, `enum_variants`, `variant_pattern`, `variant_ctor`, `payload_wire_of`. `mirror_field_wire` is the bullet #225 had to defer, because it is ONE policy serving both the modeled mirror path and the then-unmodeled payload path; splitting it would have duplicated the policy rather than deleted it. Modeling enums let it move as one piece. Ledger: `api/core/types_util.rs` 41 -> 39, total 195 -> 193. Generated C, Rust and Kotlin byte-identical (`regen-check.sh` clean); 426 lib tests and the full workspace suite green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * F2 review: keep the written variant shape, and stop overflowing discriminants Two blockers from the review of the new enum model. P1: `fields.is_empty()` is not `syn::Fields::Unit`. `B()` and `C {}` carry no payload and still must be spelled `E::B()` / `E::C {}` wherever Rust names them, so deriving the shape from the payload group emitted `E::B` — a constructor function, not a value. `VariantShape` is now taken from `syn::Fields` at lowering and stored on `SourceVariant`; `is_unit()` keeps answering the separate group question. `VariantShape::spell` is the one place the delimiters are chosen, replacing five hand-rolled derivations across cbindgen and jnigen (two of which read `syn::Fields` directly). P2: `next = value.map(|n| n + 1)` panicked during ingest for valid Rust such as `#[repr(u64)] enum E { A = i64::MAX, B }`, taking down every adapter including the C one that only re-emits the spelling. `checked_add` ends the numeric chain the way an unevaluable spelling does, leaving `DiscriminantSource` intact. Covered in the model (both empty forms, shape vs group, top-of-range discriminant) and in the generated Rust for both adapters and both directions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Stage F5 of #215 (umbrella for #211) — the struct-field half.
The duplicate
#212 made cbindgen's struct-field path read
SourceTypefromRegistry::source_struct. It then threw the model away:The frontend had already decided that this field is a
String, abool, or an array of scalars. Projecting back tosynand asking again made the projection a second authority on one fact — the drift #211 exists to stop, and the exact shape #210 took.c_field_wire,is_scalar_array,data_field_wire,data_field_owns,restricted_validity_field, and thein_data_struct/out_data_structfield loops now take aSourceTypeand match it:The rule this establishes
Shape from the model, identity from the registry.
"Is this field a
String, abool, an array of scalars?" is a source fact the frontend decided. Asking syntax again is the duplicate."Is this the type the binding declared as a
tagged_union?" is a registry lookup that happens to be keyed byTypeKey, i.e. by asyn::Type. Rekeying the registry is F4's job, not this stage's, so those call sites project deliberately and say so in a comment rather than pretending to be migrated.Later stages should follow the same split; without it, "migrate to the model" degenerates into churning every lookup that touches a type.
What stays on syntax, and why
mirror_field_wire(is_scalar/box_inner/is_option) is not migrated. It is one policy serving two callers: therepr_c_structmirror path, which has a model, and the tagged-union payload path, whose variant fields the frontend does not model yet (F2 covers structs only). Splitting it into a model version and a syntax version would duplicate the policy — precisely the thing this PR deletes. It waits on F2 modeling enums.Same for parameters and returns: functions are still
synitems, so that F5 bullet is blocked on F2 too. Both are recorded as such indocs/source-frontend.mdrather than left as open boxes with no stated blocker.The ledger caught it
First real use of #224's boundary check, and it fired on the decrease:
is_scalar_arrayno longer matchessyn::Type::Array. Total 196 → 195, and the regenerated ledger is in the diff — which is the point of requiring an edit in both directions.Note what that number does not capture.
is_string,is_boolandis_scalarwere never in the ledger: they classify by ident name (type_path_tail(ty) == "String"), one of the blind spots the ledger header lists. The count moved by one; the duplication deleted was larger. That is the ledger working exactly as documented — a tripwire against growth, not a measure of progress — and it is worth seeing on the first stage that uses it rather than discovering later.Verification
cargo test --all --all-features— green (428 lib tests + the workspace suite)../examples/regen-check.sh— byte-identical. Per the branch's review protocol this stage is must not move: this is a pure re-sourcing of the same decisions, so any generated diff would be a bug.--deny warnings, fmt — clean.🤖 Generated with Claude Code