Skip to content

Make Emit a registry-owned callback key - #401

Open
milyin wants to merge 2 commits into
mainfrom
fix/issue-375-emit-seal
Open

Make Emit a registry-owned callback key#401
milyin wants to merge 2 commits into
mainfrom
fix/issue-375-emit-seal

Conversation

@milyin

@milyin milyin commented Aug 13, 2026

Copy link
Copy Markdown
Owner

Summary

  • preserve the prebindgen -> prebindgen-flat -> prebindgen-registry -> adapters dependency direction
  • keep the object-safe rendering protocol in prebindgen-flat, with a registry-owned unconstructable Emit callback key
  • hide flat::emit from registry-only adapters while retaining the root prebindgen_flat::RustEmitter path for independent collectors
  • delegate through Deref<dyn RustEmitter>, keeping emit.spell(ty) ergonomic without duplicating the rendering API
  • make direct syntax accessors crate-private and remove the raw Flat::enum_item test helper

Boundary

The default registry path is compiler-checked:

  • prebindgen-registry deliberately does not re-export RustEmitter.
  • Its re-exported flat model cannot expose the trait because prebindgen-flat::flat::emit is crate-private.
  • Registry-only adapters cannot construct Emit or name the rendering protocol; they render only after a callback receives &Emit.
  • Compile-fail doctests pin both restrictions.

Independent use remains intact. Another collector can depend directly on prebindgen-flat, implement the public RustEmitter protocol for its own key, and establish a separate rendering boundary without involving the registry.

Rust cannot prevent an adapter from deliberately adding that direct dependency. That residual is explicit in both Cargo.toml and impl RustEmitter; workspace CI now rejects such implementations in prebindgen-c and prebindgen-jni.

The trait remains object-safe. The old generic shape operation becomes the unambiguous shape_struct, shape_alternative, and shape_enum_value methods. The flat testing feature is no longer needed; registry test code uses prebindgen-registry/testing.

Other API cleanup

  • Emit is a private-field tuple struct, and its compile-fail example now tests privacy directly with Emit(()).
  • Origin retains derived Debug for resolver diagnostics; Debug is documented as diagnostic output rather than structured syntax access.
  • ReturnTypeMismatch.actual now uses canonical TypeKey display, matching the already-canonical declared side, rather than captured source spelling.

Validation

  • cargo fmt --all -- --check
  • cargo test -p prebindgen-flat -p prebindgen-registry --all-features --doc
  • cargo test --workspace --all-features --quiet
  • cargo clippy --workspace --all-features --all-targets -- -D warnings
  • RUSTDOCFLAGS=-Dwarnings cargo doc -p prebindgen-flat -p prebindgen-registry --all-features --no-deps
  • CI policy guard exercised locally

Closes #375

@milyin
milyin force-pushed the fix/issue-375-emit-seal branch from a52d2d4 to a6bddec Compare August 13, 2026 21:43
@milyin milyin changed the title Restore the Emit capability seal across crates Make Emit a registry-owned callback key Aug 13, 2026
@milyin
milyin requested a balanced review from Copilot August 13, 2026 21:59

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Moves emission ownership into the registry while retaining rendering operations in the independent flat-model crate.

Changes:

  • Adds registry-owned Emit and flat-owned RustEmitter.
  • Migrates adapters, callbacks, tests, and shape rendering.
  • Restricts direct syntax access and redacts captured syntax from Debug.

Reviewed changes

Copilot reviewed 32 out of 32 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
README.md Updates crate responsibilities.
docs/crate-split.md Documents the new emission boundary.
prebindgen-flat/Cargo.toml Removes the flat testing feature.
prebindgen-flat/src/lib.rs Exports RustEmitter.
prebindgen-flat/src/flat/emit.rs Introduces the rendering protocol.
prebindgen-flat/src/flat/mod.rs Removes enum_item and updates guidance.
prebindgen-flat/src/flat/origin.rs Restricts spelling and redacts debug output.
prebindgen-flat/src/flat/spell.rs Updates shape-rendering documentation.
prebindgen-flat/src/flat/ty.rs Restricts direct type spelling.
prebindgen-flat/src/flat/tests/roundtrip.rs Migrates rendering tests.
prebindgen-registry/Cargo.toml Moves test-key support into registry.
prebindgen-registry/src/emit.rs Adds the registry-owned Emit key.
prebindgen-registry/src/lib.rs Exports registry Emit.
prebindgen-registry/src/prebindgen.rs Updates callback capability types.
prebindgen-registry/src/write.rs Mints the registry emission key.
prebindgen-registry/src/write/tests.rs Migrates write tests.
prebindgen-registry/src/test_util.rs Adds test-only spelling support.
prebindgen-registry/src/unfold.rs Uses diagnostic type display.
prebindgen-registry/src/unfold/plan.rs Updates emission documentation.
prebindgen-registry/src/unfold/tests.rs Imports test spelling support.
prebindgen-registry/src/expand/plan.rs Updates emission documentation.
prebindgen-registry/src/expand/tests.rs Imports test spelling support.
prebindgen-registry/src/registry/declare.rs Passes registry Emit to converters.
prebindgen-registry/src/registry/mod.rs Adjusts private-link documentation.
prebindgen-registry/src/registry/scan.rs Adjusts private-link documentation.
prebindgen-registry/src/registry/tests.rs Migrates registry tests and enum lookup.
prebindgen-c/src/trait_impl.rs Uses shape-specific rendering methods.
prebindgen-jni/Cargo.toml Removes flat testing feature usage.
prebindgen-jni/src/jni/fn_plan.rs Updates spelling guidance.
prebindgen-jni/src/jni/struct_plan.rs Uses diagnostic display.
prebindgen-jni/src/jni/emit/flat_input.rs Uses shape-specific rendering methods.
prebindgen-jni/src/jni/emit/sum_out.rs Uses alternative rendering.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread prebindgen-flat/src/flat/emit.rs
@milyin

milyin commented Aug 13, 2026

Copy link
Copy Markdown
Owner Author

Review

The refactor itself is good: shape → three object-safe methods, Flat::enum_item and the flat testing feature deleted, the doc comments cut down from archaeology to statements, Emit back on the crate that hands it out. Deref<Target = dyn RustEmitter> is a neat way to keep emit.spell(ty) working in adapters without them importing the trait.

But the headline claim does not hold as written.

The seal is not restored — an adapter can still forge one, with no extra dependency

RustEmitter is pub, unsealed, and every method has a default, so impl RustEmitter for MyKey {} mints full spelling authority. That is advertised in the trait's own doc example. The part that matters is that the trait is reachable from a decision-side adapter: prebindgen-flat/src/flat/mod.rs has pub mod emit;, and prebindgen-registry re-exports pub use ::prebindgen_flat::{flat, …}, so prebindgen_registry::flat::emit::RustEmitter is nameable from any crate that depends on the registry alone — which is exactly what prebindgen-c and prebindgen-jni do.

Verified against a6bddec with a crate whose only prebindgen dependency is prebindgen-registry:

use prebindgen_registry::flat::emit::RustEmitter;

pub struct Forge;
impl RustEmitter for Forge {}

// no Emit anywhere: classification code spelling captured syntax
pub fn decision_code_spells(t: &prebindgen_registry::flat::TypeRef) -> proc_macro2::TokenStream {
    Forge.spell(t)
}

Compiles. So relative to #375, the mint moved from Emit::new() to impl RustEmitter for X {} — same reachability, different incantation. The compile_fail doctests only prove the direct t.spell() route is shut, and the bypass is demonstrated 30 lines below them in the same file.

That makes these overstated:

  • docs/crate-split.md: "The accidental doors introduced by the split are closed again."
  • docs/crate-split.md: "an adapter using the registry cannot construct the registry key" — true, and irrelevant, since it does not need the registry key to spell.

One-word change that buys most of it back

Make the module private and keep the root re-export:

-pub mod emit;
+pub(crate) mod emit;

prebindgen_flat::RustEmitter (the pub use in lib.rs) stays the only public path, so the independent-collector story is untouched — a collector depends on prebindgen-flat directly and names it there. But an adapter that depends only on prebindgen-registry can no longer name the trait at all: forging now costs a prebindgen-flat = … line in its Cargo.toml, which is a reviewable diff in the manifest rather than two lines buried in a plan module.

Verified on top of the branch: cargo check --workspace --all-targets clean, RUSTDOCFLAGS=-D warnings cargo doc -p prebindgen-flat -p prebindgen-registry --no-deps clean, cargo test -p prebindgen-flat --doc 14 passed. The forge crate above then fails with E0603: module emit is private.

Consider also not re-exporting RustEmitter from prebindgen-registry — as of this PR you already don't, but it reads accidental next to pub use …::{Element, Flat} and the Emit export. Make it a deliberate, commented omission.

Then say what is actually true

Cross-crate friend visibility does not exist, so the honest end state is: the default path is compiler-checked, and the residual is a convention. That convention is now precisely greppable in a way the retired "count the doors" metric never was — one trait name, one impl form:

! git grep -n "impl .*RustEmitter for" -- prebindgen-c prebindgen-jni

Three lines of CI, and it cannot be walked around without naming the trait. Worth more than the current paragraph claiming enforcement.

Smaller things

  • prebindgen-registry/src/emit.rs: the private struct Key buys nothing over _key: () — the field's privacy is what blocks construction either way. Related, the second compile_fail doctest (Emit { _key: () }) passes for the wrong reason: it is a type mismatch, so it would still "fail" with a public field. pub struct Emit(()); plus a doctest on Emit(()) tests the thing you mean.
  • unfold.rs: actual in ReturnTypeMismatch changes from the captured spelling to Display (the TypeKey identity). That is arguably an improvement — declared is already a key, so the two sides of the message now match — but it is a user-visible diagnostic change and worth a word in the PR body. Also actual: { ret.to_string() } still has the leftover block.
  • Origin's manual Debug redacting syntax costs every dbg! on a TypeRef during resolver work, and Debug was never one of the doors the capability was about. Cheap to keep, cheap to drop; just noting the trade is debuggability for no enforcement.
  • Flat::enum_item deletion and the testing feature removal on flat: checked, nothing else in the workspace referenced either.

Net: merge the mechanical part happily. Either land the pub(crate) mod emit line and the grep, or leave the code and downgrade the prose in docs/crate-split.md from "closed again" to "convention, checked by CI" — but the PR should not close #375 while claiming a seal the compiler does not check.

@milyin

milyin commented Aug 13, 2026

Copy link
Copy Markdown
Owner Author

Addressed the full review in e85c00e:

  • made flat::emit crate-private while preserving prebindgen_flat::RustEmitter for direct independent collectors;
  • documented the deliberate omission of RustEmitter from registry exports;
  • added compile-fail coverage for the hidden registry-to-flat path and direct Emit(()) construction;
  • added the C/JNI CI grep guard against local RustEmitter implementations;
  • rewrote the boundary docs to distinguish the compiler-checked default path from the explicit direct-flat-dependency residual;
  • simplified Emit to pub struct Emit(());
  • restored derived Origin Debug and documented it as diagnostic rather than structured syntax access;
  • removed the leftover block around actual and documented the ReturnTypeMismatch change to canonical TypeKey display in the PR body.

Local validation is green: focused compile-fail doctests, full workspace tests, workspace Clippy with warnings denied, rustdoc with warnings denied, formatting, diff checks, and the new policy guard.

— Codex (GPT-5)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Restore the Emit capability seal across the crate boundary

2 participants