Address captures by content, not by the process that wrote them (#201) - #416
Conversation
`#[prebindgen]` named each capture `{group}_{pid}_{thread}.jsonl`, so every
rustc process created another full copy of the same records. Nothing removes
them: the build script clears the directory only when Cargo re-runs it, which
happens on a source change — while rustc and rustdoc run far more often than
that. Doctests are the extreme case, since Cargo never caches them: five
`cargo test -p example-flat` runs with no edit in between took the directory
from 2 files to 14, and it never stops (#201).
Name each capture after the record instead: `{group}/{name}_{digest}.jsonl`,
where the digest covers the record's serialized form. Any compilation that
captures an item computes the same path and writes the same bytes, so repeated
compilations rewrite one file rather than adding a copy each.
Deriving the name from the data rather than from the writer also makes the
layout loss-proof by construction: the name determines the contents, so two
writers either write identical bytes to one path or different bytes to
different paths. There is no third case where one compilation resets another's
records — which is why the digest is load-bearing and not decoration. It keeps
`#[cfg(test)] fn f` apart from the crate's own `fn f`, whose units compile in
parallel, and keeps `struct Foo` apart from `fn foo` on the case-folding
filesystems of macOS and Windows.
Records are published by writing a temporary file in the destination directory
and renaming it over the target, so a reader sees a complete file or no file,
and a lost race costs nothing. A capture that already exists is left alone.
Write failures are now reported at the item as compiler diagnostics instead of
a bare `compile_error!`.
Groups become directories, which is what lets a group name contain `_`: the
flat layout spelled the group as the file-name prefix up to the first
underscore, so `#[prebindgen("my_group")]` was discovered as group `my`. Group
names are validated at the item, since they now name a directory. `Source`
reads the group directory and still reads the flat layout, so a capture
directory written by an older macro is not orphaned.
`init_prebindgen_out_dir()` removes directories as well as files: leaving them
would keep records for items the new revision renamed or deleted.
|
Review outcome: I would not merge this revision yet because the new group-directory representation does not preserve valid group identities cross-platform. [P1] Encode group directories independently of filesystem name semantics. Please encode the group into a portable collision-free component (for example, a prefixed reversible lowercase hex encoding of the original bytes) or otherwise reject every alias/reserved case, and add coverage for case-only group pairs plus Windows device names. The existing case-fold test covers record filenames, but not the new directory level where this collision occurs. The focused proc-macro tests (4 + 4 doctests), Source tests (4), JSONL tests (6), and — Codex (GPT-5) |
`.join(group)` put `#[prebindgen("Foo")]` and `#[prebindgen("foo")]` in one
directory on macOS and Windows, merging two groups the API keeps apart:
`discover_groups` kept whichever casing was created first, and
`items_in_groups` then returned nothing for the other spelling or exposed the
merged group — generated bindings varying by host filesystem. The validator
also accepted `CON`, `PRN`, `NUL`, `COM1` and friends, which Windows refuses as
a path component outright.
Encode the group instead of restricting it: a directory is now
`{digest(group)}_{group}`, the same rule the record files already follow —
a digest carries the identity, a sanitized spelling carries the readability.
Distinct group names get distinct directories on any filesystem, and no
component can be a bare device name, a path separator, `..`, or a
normalization-dependent spelling. `validate_group` goes away with the problem
it was guarding: any string literal is a usable group name again.
The digest is not reversible, so the group's exact name is published in
`group.txt` beside its records, before the group's first record, and
`discover_groups` reads it there. A directory holding captures without one is
a damaged capture directory rather than an unnamed group, and says so instead
of dropping records silently.
Both halves of the layout now live in `prebindgen::layout`, computed by the
proc-macro when writing and by `Source` when reading, with the filesystem
hazards they defend against documented in one place. `write_record_file`
becomes `publish_file`, since the group name file goes through it too.
Covered by tests: case-only group pairs, Windows device names, separators and
traversal, non-ASCII and over-long names, and — on this checkout's APFS — a
real `structs` / `Structs` pair building side by side in example-flat.
|
Fixed in f0883a4. You were right that the directory level was unguarded — and the fix is to stop validating the group name and encode it instead, the same rule the record files already followed. A group directory is now
The digest is not reversible, so the exact name is published in Both halves of the layout moved into Coverage added at the directory level, as asked:
And end-to-end on this checkout's APFS, with a temporary Both spellings build side by side and Re-verified: |
The digest that made group directories collision-free was not reversible, so the group's exact name had to be published beside its records and read back from there. A reversible encoding carries the name in the directory itself. `g_` plus the group with only `a-z`, `0-9` and `_` verbatim and every other byte as `-XX`. Escaping to `-XX` rather than `_XX` keeps the ordinary case readable, since `_` is the character group names actually contain: `default` and `structs` stay `g_default` and `g_structs`, while `Structs` is `g_-53tructs` and `my_group/../CON` is `g_my_group-2f-2e-2e-2f-43-4f-4e`. The four hazards close exactly as before, now by construction rather than by hashing: the encoded form holds no uppercase, so case folding cannot merge two encodings; no separator or dot, so nothing escapes the capture directory or ends in one; no non-ASCII, so nothing depends on how the host normalizes; and the `g_` prefix keeps `CON` and friends off Windows' device list. Removing the name file also removes a way for a directory and the name it claims to disagree — a decoded name cannot be stale or missing. `Source` still refuses to walk past a directory that holds captures it cannot decode, rather than dropping records silently. A group name is now bounded only by what a path component can hold: one that encodes past 255 bytes is reported at the item.
|
Follow-up in 20c210d: the group directory now uses a reversible encoding, so the
The four hazards now close by construction rather than by hashing: the encoded form holds no uppercase (case folding cannot merge two encodings), no separator or dot (nothing escapes the capture directory or ends in one), no non-ASCII (nothing depends on host normalization), and the Dropping the name file also removes a way for a directory and the name it claims to disagree: a decoded name cannot be stale or missing. New coverage in Re-verified: 36 |
|
Re-review at One remaining issue: [P2] Require a canonical encoded directory before accepting it as a group. Please accept a decoded name only when Verification: all 36 — Codex (GPT-5) |
The encoding admits redundant spellings — `g_-61` decodes to `a` just as `g_a` does, and `-4A` names the same byte as `-4a` — and decoding one was worse than rejecting it. Discovery would report group `a` while `group_dir_name` sends `read_group` to `g_a`, so the records in `g_-61` were dropped with nothing to show for it: exactly the damage the capture check exists to catch, walking straight past it. Decode only when the name re-encodes to the directory it came from. A directory name that is not UTF-8 skipped that check too: discovery bailed before deciding anything, captures and all. A group directory is ASCII, so such a name is never one — but it still has to face the damage check rather than be stepped over. Regressions for both, plus a property test that every hostile group name decodes from its canonical spelling and from no other. The non-UTF-8 case is Linux-only: APFS refuses to create such a name at all, which is why macOS cannot reach it.
|
Both fixed in ed799d2. Canonical spelling required. let decoded = String::from_utf8(bytes).ok()?;
(group_dir_name(&decoded) == dir_name).then_some(decoded)That rejects every redundant spelling in one rule rather than enumerating them — Non-UTF-8 directory names. Discovery no longer Regressions, both using your suggested shape:
Re-verified: 37 |
|
Re-review at The previous P2 is fixed correctly:
I also rechecked the original cross-platform concern: case-only groups remain distinct, Windows device names are safely prefixed/encoded, separators and traversal are contained, Unicode is represented through ASCII byte escapes, and the encoding is reversible. Local verification on this head: all 37 — Codex (GPT-5) |
1 participant
Closes #201.
The leak
#[prebindgen]named each capture{group}_{pid}_{thread}.jsonl, so every rustc process wrote another full copy of the same records. Nothing removes them:init_prebindgen_out_dir()clears the directory only when Cargo re-runs the build script, which happens on a source change — and rustc/rustdoc run far more often than that.Doctests make it unbounded, because Cargo never caches them. With no edit at all:
cargo build -p example-flatcargo test -p example-flat#1(The jump to 47 is the layout change — one file per record instead of one per process. The point is the column: it stops moving.)
The fix: name the file after the data
The digest covers the record's serialized form. Any compilation that captures an item computes the same path and writes the same bytes, so repeated compilations rewrite one file instead of accumulating a copy each. The file set is a function of the source, not of build history.
This is what makes the layout loss-proof, and it is the main argument for it. The previous approach to bounding growth was to derive a key for the writer — the compilation unit — and let each writer reset its own file. That only holds while the key is exactly as fine-grained as the unit, and getting it wrong loses records rather than leaking files. Deriving the name from the data removes the question: the name determines the contents, so two writers either write identical bytes to one path or different bytes to different paths. There is no third case where one compilation resets another's records.
It also stops depending on the compiler's command line, which the unit-keyed approach had to parse (
-C metadata, and an argument filter for rustdoc). Nothing here reads argv, so a build driven by a compiler that doesn't distinguish invocations that way is not a special case.Why the digest is load-bearing
Not decoration — it is what keeps the "name determines contents" property true:
#[cfg(test)] fn fand the crate's ownfn fare different records with one name, and their units compile in parallel undercargo test.struct Fooandfn foo.The readable part is capped at 48 characters and forced to ASCII, since Rust identifiers admit characters filesystems normalize differently.
Publishing
Each record is serialized, written to a temporary file in the destination directory, and
renamed over the target. A reader sees a complete file or no file; a lost race costs nothing because the loser wrote identical bytes. An existing non-empty file at the path is left alone. Write failures are now reported at the item as compiler diagnostics rather than a barecompile_error!.Groups become directories
A group is now a subdirectory rather than a filename prefix. That fixes a latent bug: the flat layout recovered the group as the prefix up to the first
_, so#[prebindgen("my_group")]was discovered as groupmyanditems_in_groups(&["my_group"])returned nothing._,-), since they now name a directory.Sourcereads the group directory and still reads the flat layout, so a capture directory written by an older macro is not orphaned.init_prebindgen_out_dir()now removes directories as well as files. Leaving them would keep records for items the new revision renamed or deleted — the one way this layout could go stale.Scenarios
cargo check/build/testunitscargo docrustdoc#[cfg(test)])Verification
build, thentest×5,test --doc alpha,test --doc beta,clippy --all-targets,doc— the file set in each build-hash directory is byte-identical after every command (47 files: 44default+ 3structs).#[cfg(test)] #[prebindgen] fn test_only_probe()inexample-flat,cargo test --libfollowed bycargo buildleaves 48 files and the probe survives. (Under the flat layout this is the case that a unit-keyed name gets wrong when the key is too coarse.)struct Fooandfn fooproduce names that differ afterto_lowercase()._, the legacy flat layout, and non-capture entries (crate_name.txt, stray files) are covered by unit tests.cargo test --all --all-features,cargo clippy --all-targets --all-features -- --deny warnings,cargo fmt --check, on 1.85.0 (MSRV) and 1.97.1.examples/regen-check.sh: committed generated output is byte-identical, i.e. consumers produce the same bindings from the new layout.Relationship to #396 and #398
-C metadatawith an argument filter for rustdoc. It is correct as far as it goes — and it carries a reproduced record-loss regression fix for the metadata list. This PR supersedes that approach: it deletes the need for a unit key at all, so Stop leaking a JSONL file per rustc invocation (#201) #396'sunit_id, metadata parsing and runtime-option deny-list have no counterpart here. Either can land; they should not both.cargo prebindgen clean) is unaffected and still wanted: it addresses staletarget/*/build/<pkg>-<hash>/directories, which are Cargo's and which this PR deliberately does not touch. Note the counts above are per build-hash directory —clippyanddocstill mint their own.randis dropped from the workspace, since nothing generates names any more.