Address the three nits from the #152 review - #175
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses three follow-up nits from the #152 review, focused on making sum-type return planning less adapter-coupled, clarifying a Kotlin/JNI comment to reflect current behavior, and normalizing an example config file’s formatting.
Changes:
- Ensure
wire_fixed_returnsdrops the required-output scan registration for the peeledVecelement (core) when sums have no whole-value converter. - Update the
kt_handle_targetexplanation in JNI/Kotlin rendering to describe the current (defensive) rationale and non-reachability for projection payloads. - Add a focused test for the
Vec<sum>-only fixture and add the missing trailing newline tocbindgen.toml.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| prebindgen/src/api/lang/jnigen/jni/render.rs | Clarifies comment explaining why handle lock targets use the leaf’s access template. |
| prebindgen/src/api/core/unfold/tests.rs | Adds a test covering Vec<Reading>-only sum return invariants. |
| prebindgen/src/api/core/unfold.rs | Extends no_converter unrequire logic to include the peeled core type. |
| examples/example-cbindgen/cbindgen.toml | Adds trailing newline at EOF. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let mut reg = reg_with(&["fn read_all(n: i32) -> Vec<Reading> { todo!() }"]); | ||
| let declared: std::collections::HashSet<syn::Ident> = | ||
| ["read_all"].iter().map(|s| ident(s)).collect(); | ||
| apply_sum_returns(&mut reg, vec![reading_sum_decon()], &declared).expect("apply_sum_returns"); |
There was a problem hiding this comment.
Correct, and fixed in 9b4699e — this names the caveat the test's own doc comment admitted, which was a sign the test should have been better rather than better-documented.
Seeding is exactly the right shape: require_output(Reading) before apply_sum_returns reproduces what an adapter that does require the peeled element leaves behind, which is the state unrequire_output(&core) exists for. A precondition assert pins that the fixture starts out in it, so the seeding cannot silently stop working either.
let bare: syn::Type = syn::parse_quote!(Reading);
reg.require_output(&bare, &crate::SourceLocation::default());
assert!(
reg.required_outputs_scan.contains(&TypeKey::from_type(&bare)),
"fixture precondition: the bare element starts out required"
);Verified to discriminate, which it previously did not:
wire_fixed_returns |
Result |
|---|---|
with unrequire_output(&core) |
pass |
| with that line removed | FAILED |
The doc comment no longer hedges about pinning-rather-than-catching, because it now catches.
- `wire_fixed_returns` unrequires the peeled `Vec` element too. The shape fold peels three layers here and unrequired two, so a `Vec<sum>`-only declaration left the bare `E` requirement to the adapter's `boundary_only_types` — true for JniGen today, which makes it a coupling rather than a property. A test states the invariant for a `Vec`-only fixture; the test above cannot, because its `Option<Reading>` fixture unrequires the bare type through a different layer. - `render.rs:classify_params`: the `kt_handle_target` comment read as if it fixed a live bug. `build_flat_sum_field` returns `None` for any projection payload, so a tag-gated handle leaf cannot reach that code today. The comment now says what the change actually is — the leaf's own answer to "where did this come from", rather than a second derivation that can drift. - `examples/example-cbindgen/cbindgen.toml` gets its trailing newline. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ac05089 to
4a043b9
Compare
…review) Copilot's review, and it is right — it names the caveat the doc comment itself admitted. A scan registers only the top-level return as required, so `Reading` was never in `required_outputs_scan` in this fixture and the assertion held whether or not `wire_fixed_returns` unrequired the peeled element. The test passed while testing nothing. Seeding `require_output(Reading)` reproduces what an adapter that DOES require the element leaves behind — the state the unrequire exists for — and a precondition assert pins that the fixture starts out in it. Verified to discriminate: passes with `unrequire_output(&core)`, fails without it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* Ask rustc whether the emitted Rust compiles (#198, step 2 in part) `plan` said the generator produced a file. That is a weak claim: an emission can be well-formed, contain every substring a unit test looks for, and still not type-check — `examples/emitcheck` exists because that happened once with 41 of 41 tests green over it. Every cell that produced Rust is now compiled. The state is a **receipt**, not a claim. Each cell is written to `<id>.rs`, the whole crate is checked in one pass, and each diagnostic is attributed back by the file rustc names. Nothing maps a cell to a fixture by hand — that mapping is what let #175's test pass without creating its own precondition. Verified to discriminate: `the_compile_check_separates_good_from_bad` feeds one compiling and one non-compiling unit through the real path and requires them separated. Compiler messages stay OUT of the committed report — they vary by toolchain, and `cargo test --all` runs on both 1.85 and stable, so a message in the file would make the report disagree with itself across jobs. Failing cells print their diagnostics on stderr. The check crate's dependencies are pinned exactly for the same reason: it has its own lockfile, so a caret range would let an upstream release move a cell with nothing in this repo having changed. **Exit: answers move.** 138 of 288 cells, in four classes: * 120 `plan` -> `rustc` — the new evidence. * 10 `plan` -> `bad rust` — emitted Rust that does not compile. See below. * 8 -> n/a — `Option<&T>` and `Vec<&T>` in a field or payload. Those fixtures were never legal Rust; the borrow rule only excused a spelling *starting* with `&`, so a borrow nested inside one was being measured against a struct that needs a lifetime parameter to exist. * no cell moved between `rejected` and `panic`. Three harness defects found by turning the compiler on, each of which had been producing a confident wrong answer: * the source crate was mounted as `mod probe` beside a generated `pub fn probe` wrapper — two different things sharing a name, so it is `flat` now; * a returned borrow has no lifetime to elide from, so `-> &Handle` is not Rust. Every borrow-returning cell had reported `plan` for a fixture that could not compile; the fixture writes `'static` now; * `impl Display for ZError` was being fed to the *model*, which correctly refuses an item kind the flat language does not have — failing 32 cells for a reason unrelated to their shape. The model now sees the four item kinds a `#[prebindgen]` surface declares, the same filter `emitcheck` applies. What the 10 findings are, all confirmed in the generator's own output rather than in fixture scaffolding: * JNI emits `Cow<'static, str>` **unqualified** into the consumer's scope, so a consumer that has not imported `Cow` cannot compile the file (3 cells); * C emits `flat::Option<Handle>` — a std type qualified into the source module; * C moves out of a value behind a raw pointer for `Option<data struct>` and `Option<sum>` parameters; * C calls the source function with one argument too many for a `&[T]` return, and builds a `map`/`collect` over a `Vec<&T>` return whose closure is a function item of the wrong signature; * JNI mismatches types for a `&mut T` parameter and for `&mut MaybeUninit<T>`. Part of #198, tracked by #399. * docs: step 2's rustc half has landed Splits the receipts step in two: rustc accepts the emitted Rust (done), and the rest of the toolchain plus the runtime states (not started).
Finding 6 of the review on #152 — three small items, plus a follow-up fixing the test Copilot caught.
1.
wire_fixed_returnsunrequires the peeledVecelementThe shape fold peels three layers (
Option,Vec,&) and theno_converterbranch unrequired two. With only aVec<E>-returning function declared, the bareErequirement was left to the adapter'sboundary_only_types— which covers it for JniGen today, making it a coupling rather than a property. The peel happens here, so the unrequire belongs here.a_vec_only_sum_return_drops_the_bare_requirementcovers it. The existingsum_return_layers_ride_the_shape_foldcannot: itsOption<Reading>fixture unrequires the bare type through a different layer, so it would keep passing if theVecelement were left required.The test seeds the requirement explicitly (Copilot caught that the first version did not). A scan registers only the top-level return, so
Readingwas never inrequired_outputs_scanand the assertion held whether or not the fix was present — it passed while testing nothing.require_output(Reading)beforeapply_sum_returnsreproduces what an adapter that does require the element leaves behind, and a precondition assert pins that the fixture starts out in that state.Verified to discriminate:
wire_fixed_returnsunrequire_output(&core)2. The
kt_handle_targetcomment inrender.rsIt read as if it fixed a live bug ("so a tag-gated variant handle locks the expression it actually came from"). It does not:
build_flat_sum_fieldreturnsNonefor any projection payload, so a tag-gated handle leaf cannot reach that code today. The change is correct and defensive, and the comment now says that — the access template is the leaf's own answer to "where did this come from", and deriving it twice is how the two drift apart.3.
cbindgen.tomltrailing newlinePre-existing, now with added content. Added.
Verified
cargo test --all --all-features(392 lib),cargo fmt --check,clippy --all-targets --no-default-features --all-features -D warnings,examples/regen-check.sh— all clean.🤖 Generated with Claude Code