From a934b8ee4044f0db9a06e9371d4cd58d3926986b Mon Sep 17 00:00:00 2001 From: Nexlab-One <86677687+Nexlab-One@users.noreply.github.com> Date: Wed, 29 Jul 2026 10:53:44 +0200 Subject: [PATCH 1/3] fix(fn-dsa): forward `shake256x4` to -comm; the feature could not be built MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `cargo check -p lib-q-fn-dsa-kgen --features shake256x4` failed with E0433 and `-p lib-q-fn-dsa-sign` with E0425: the feature flips both crates' call sites onto `fn_dsa_comm::shake::SHAKE256x4`, but they declared `shake256x4 = []`, so -comm — where that type is itself `#[cfg(feature = "shake256x4")]` — was never compiled with the feature. rustc said so explicitly ("found an item that was configured out ... gated behind the `shake256x4` feature"). The type was never missing; the manifest wiring was. WHY IT SURVIVED SINCE IMPORT The only feature coverage in CI is workspace-wide (`--all-features`, ci.yml:76 / cd.yml:78). There the `lib-q-fn-dsa` facade — which does forward — enables the flag on -comm, and feature unification hands that same -comm build to every leaf, so a leaf that forwards nothing still compiles. Only per-crate selection (`-p --features shake256x4`, `-p --all-features`) exposes it, and no CI row ever did that. The defect is inherited: upstream fn-dsa 0.3.0 declares the same empty features in fn-dsa-kgen/fn-dsa-sign and only works because its facade enables all three. A future re-vendor must not "restore" the empty form. WHY FIX THE WIRING RATHER THAN REMOVE THE FEATURE `SHAKE256x4` is present and complete (portable + dedicated AVX2 backend), and it is verified, not assumed: comm's own oracle (fn-dsa-comm/src/shake.rs) compares it against four 64-bit-interleaved SHAKE256 streams over all 301 seed lengths 0..=300 and passes on BOTH dispatch paths (`--features shake256x4` and `--features shake256x4,no_avx2`), and fn-dsa-kgen's `gauss::sample_tv` shake256x4 vectors are byte-identical to upstream 0.3.0's. Deleting a working, upstream-faithful, test-covered feature to fix a two-line manifest bug would be the wrong trade. Enabling the forward cannot regress any build that compiles today: such a build already had comm/shake256x4 on, or it would not have compiled. REGRESSION COVERAGE (both were proven to fail on the reverted manifest) - lib-q-fn-dsa/fn-dsa-kgen/tests/feature_forwarding.rs reads the manifests as data, so `cargo test -p lib-q-fn-dsa-kgen` — which the CI workspace pass runs, the crate is a member and is not in the --exclude list — catches a missing forward without having to build the broken configuration. It cannot pass vacuously: the crate pairs it must evaluate are asserted, so a manifest reshape that blinds the parser fails the test instead of silently skipping it. - .github/actions/test-fn-dsa/action.yml gains the compile-level counterpart: per-crate `cargo check --features shake256x4` / `--all-features`, plus the SHAKE256x4 equivalence oracle on both dispatch paths and the feature-on keygen and signing tests. One `-p` per invocation — merging them re-unifies the features and tests nothing. NOT FIXED HERE `no_avx2` has the same missing forward in -kgen/-sign/-vrfy. It is not build-breaking (it gates internal dispatch, not a cross-crate item); adding the forward would change which SHAKE code path existing per-crate builds select, which is a behaviour change that belongs with the portable-keygen work, not a build fix. The guard test is deliberately scoped to `shake256x4` and documents this. Verified on x86_64 Windows with the pinned toolchain (rustc 1.99.0-nightly 89c61a754 2026-07-23). No non-x86_64 target was built; the change is manifest-only and target-independent by construction, and the portable dispatch path was exercised on x86_64 via `no_avx2`. --- .github/actions/test-fn-dsa/action.yml | 32 +++ lib-q-fn-dsa/fn-dsa-kgen/Cargo.toml | 7 +- .../fn-dsa-kgen/tests/feature_forwarding.rs | 269 ++++++++++++++++++ lib-q-fn-dsa/fn-dsa-sign/Cargo.toml | 7 +- 4 files changed, 313 insertions(+), 2 deletions(-) create mode 100644 lib-q-fn-dsa/fn-dsa-kgen/tests/feature_forwarding.rs diff --git a/.github/actions/test-fn-dsa/action.yml b/.github/actions/test-fn-dsa/action.yml index a289c1de..47bc287a 100644 --- a/.github/actions/test-fn-dsa/action.yml +++ b/.github/actions/test-fn-dsa/action.yml @@ -109,6 +109,38 @@ runs: cargo check --no-default-features --features "alloc,rand" --target thumbv7em-none-eabi echo "✅ no_std check completed" + # Per-crate feature selection. Every other feature gate in CI is workspace-wide + # (`--all-features`), where the `lib-q-fn-dsa` facade enables `shake256x4` on `-comm` and + # feature unification hands that same `-comm` build to every leaf — so a leaf whose own + # `shake256x4` forwards nothing still compiles and the missing forward is invisible. + # `-kgen`/`-sign` shipped exactly that (`shake256x4 = []`, as upstream fn-dsa 0.3.0 does): + # the feature flipped their call sites onto `fn_dsa_comm::shake::SHAKE256x4`, which `-comm` + # only compiles under the same feature, so `cargo check -p lib-q-fn-dsa-kgen --features + # shake256x4` failed with E0433 for every consumer that selected the crate directly. + # Keep one `-p` per invocation: merging them re-unifies the features and tests nothing. + - name: Per-crate feature wiring (shake256x4) + shell: bash + run: | + echo "🔗 Checking per-crate feature wiring (no cross-crate feature unification)..." + cd "${{ github.workspace }}" + for p in lib-q-fn-dsa-comm lib-q-fn-dsa-kgen lib-q-fn-dsa-sign; do + cargo check -p "$p" --features shake256x4 + done + for p in lib-q-fn-dsa-comm lib-q-fn-dsa-kgen lib-q-fn-dsa-sign lib-q-fn-dsa-vrfy; do + cargo check -p "$p" --all-features + done + # Manifest-level guard for the same invariant (fails without building the broken config). + cargo test -p lib-q-fn-dsa-kgen --test feature_forwarding + # SHAKE256x4 must equal four 64-bit-interleaved SHAKE256 streams on BOTH dispatch + # paths; x86_64 takes the AVX2 branch unless no_avx2 forces the portable one. + cargo test -p lib-q-fn-dsa-comm --features shake256x4 shake256x4 + cargo test -p lib-q-fn-dsa-comm --features shake256x4,no_avx2 shake256x4 + # Exercises the feature through keygen/signing (gauss::sample_tv carries its own + # shake256x4 test-vector set). + cargo test -p lib-q-fn-dsa-kgen --features shake256x4 + cargo test -p lib-q-fn-dsa-sign --features shake256x4 + echo "✅ Per-crate feature wiring verified" + - name: Run quick FN-DSA smoke tests shell: bash run: | diff --git a/lib-q-fn-dsa/fn-dsa-kgen/Cargo.toml b/lib-q-fn-dsa/fn-dsa-kgen/Cargo.toml index bc1bdf16..6b6087f7 100644 --- a/lib-q-fn-dsa/fn-dsa-kgen/Cargo.toml +++ b/lib-q-fn-dsa/fn-dsa-kgen/Cargo.toml @@ -27,4 +27,9 @@ num-traits = { workspace = true } [features] default = [] no_avx2 = [] -shake256x4 = [] +# MUST forward to `-comm`: the type this crate switches to under the feature +# (`fn_dsa_comm::shake::SHAKE256x4`) is itself `#[cfg(feature = "shake256x4")]` inside +# `-comm`. Declared empty (upstream fn-dsa 0.3.0 does the same), `cargo check -p +# lib-q-fn-dsa-kgen --features shake256x4` fails with E0433 — the call sites flip to a type +# that was never compiled. Guarded by tests/feature_forwarding.rs. +shake256x4 = ["lib-q-fn-dsa-comm/shake256x4"] diff --git a/lib-q-fn-dsa/fn-dsa-kgen/tests/feature_forwarding.rs b/lib-q-fn-dsa/fn-dsa-kgen/tests/feature_forwarding.rs new file mode 100644 index 00000000..30d6f0cf --- /dev/null +++ b/lib-q-fn-dsa/fn-dsa-kgen/tests/feature_forwarding.rs @@ -0,0 +1,269 @@ +//! Guard: every FN-DSA crate that declares `shake256x4` must forward it to the dependency +//! that *defines* the gated type. +//! +//! WHY THIS EXISTS +//! +//! `fn_dsa_comm::shake::SHAKE256x4` — the PRNG that `-kgen` and `-sign` switch to under +//! `--features shake256x4` — is itself `#[cfg(feature = "shake256x4")]` inside `-comm`. +//! `-kgen` and `-sign` declared the feature as an empty list (`shake256x4 = []`), exactly as +//! upstream fn-dsa 0.3.0 does, so enabling it flipped their own call sites onto a type that +//! was never compiled: +//! +//! ```text +//! $ cargo check -p lib-q-fn-dsa-kgen --features shake256x4 +//! error[E0433]: cannot find `SHAKE256x4` in `shake` +//! --> lib-q-fn-dsa/fn-dsa-kgen/src/lib.rs:328:26 +//! note: found an item that was configured out +//! --> lib-q-fn-dsa/fn-dsa-comm/src/shake.rs:809:12 +//! ``` +//! +//! Nothing caught it because feature coverage in CI is workspace-wide (`--all-features`): +//! there, the `lib-q-fn-dsa` facade — which *does* forward — turns the flag on for `-comm`, +//! feature unification hands that same `-comm` build to `-kgen`/`-sign`, and the missing +//! forward becomes invisible. Only per-crate selection (`-p --features shake256x4`, +//! or `-p --all-features`) exposes it, and no CI row did that. +//! +//! This test reads the manifests as data, so it fails on the broken wiring *without* having +//! to build the broken configuration — a plain `cargo test -p lib-q-fn-dsa-kgen` catches it. +//! The compile-level counterpart (`cargo check -p --features shake256x4`) lives in +//! `.github/actions/test-fn-dsa/action.yml`. +//! +//! SCOPE: deliberately limited to `shake256x4`. `no_avx2` has the same missing forward in +//! `-kgen`/`-sign`/`-vrfy`, but it gates only internal dispatch, not a cross-crate item, so +//! it builds today; adding the forward there would change which SHAKE code path existing +//! builds select and is tracked separately. + +use std::collections::BTreeSet; +use std::fs; +use std::path::{ + Path, + PathBuf, +}; + +/// The feature under guard. +const FEATURE: &str = "shake256x4"; + +/// Pairs (`dependent`, `dependency`) that MUST be evaluated by this test. If a manifest is +/// reshaped so the parser stops seeing them, the test fails as loudly as a real regression +/// instead of silently passing on an empty set. +const REQUIRED_PAIRS: [(&str, &str); 5] = [ + ("lib-q-fn-dsa-kgen", "lib-q-fn-dsa-comm"), + ("lib-q-fn-dsa-sign", "lib-q-fn-dsa-comm"), + ("lib-q-fn-dsa-alg", "lib-q-fn-dsa-comm"), + ("lib-q-fn-dsa-alg", "lib-q-fn-dsa-kgen"), + ("lib-q-fn-dsa-alg", "lib-q-fn-dsa-sign"), +]; + +#[derive(Debug, Default)] +struct Manifest { + name: String, + dir: PathBuf, + /// (dependency key as written in the manifest, resolved directory) for path deps only. + path_deps: Vec<(String, PathBuf)>, + /// (feature name, the list it enables) + features: Vec<(String, Vec)>, +} + +impl Manifest { + fn feature(&self, name: &str) -> Option<&[String]> { + self.features + .iter() + .find(|(f, _)| f == name) + .map(|(_, v)| v.as_slice()) + } +} + +#[test] +fn shake256x4_is_forwarded_to_the_crate_that_defines_it() { + let mut problems: Vec = Vec::new(); + + // `.../lib-q-fn-dsa/fn-dsa-kgen` -> `.../lib-q-fn-dsa` + let kgen_dir = PathBuf::from(env!("CARGO_MANIFEST_DIR")); + let family_root = match kgen_dir.parent() { + Some(p) => p.to_path_buf(), + None => { + problems.push(format!("no parent directory for {}", kgen_dir.display())); + PathBuf::new() + } + }; + + let manifests = collect_manifests(&family_root, &mut problems); + + let mut evaluated: BTreeSet<(String, String)> = BTreeSet::new(); + for m in &manifests { + let Some(entries) = m.feature(FEATURE) else { + continue; + }; + for (dep_key, dep_dir) in &m.path_deps { + let Some(dep) = manifests.iter().find(|d| same_dir(&d.dir, dep_dir)) else { + continue; + }; + if dep.feature(FEATURE).is_none() { + continue; // dependency does not gate anything on this feature + } + evaluated.insert((m.name.clone(), dep.name.clone())); + + // Cargo's two spellings: unconditional and "only if the optional dep is enabled". + let want = format!("{dep_key}/{FEATURE}"); + let want_weak = format!("{dep_key}?/{FEATURE}"); + if !entries.iter().any(|e| *e == want || *e == want_weak) { + problems.push(format!( + "{}: feature `{FEATURE} = {entries:?}` does not enable `{want}`, but \ + `{}` gates items on `{FEATURE}`. `cargo check -p {} --features {FEATURE}` \ + will fail to compile. Fix: add \"{want}\" to that feature list in {}.", + m.name, + dep.name, + m.name, + m.dir.join("Cargo.toml").display(), + )); + } + } + } + + for (dependent, dependency) in REQUIRED_PAIRS { + let pair = (dependent.to_string(), dependency.to_string()); + if !evaluated.contains(&pair) { + problems.push(format!( + "guard is not wired up: expected to check that `{dependent}` forwards \ + `{FEATURE}` to `{dependency}`, but that pair was never evaluated \ + (manifest not found, feature no longer declared, or the parser did not \ + recognise the manifest layout). Evaluated pairs: {evaluated:?}" + )); + } + } + + assert!( + problems.is_empty(), + "`{FEATURE}` feature wiring is broken:\n - {}", + problems.join("\n - ") + ); +} + +/// Read `/Cargo.toml` plus every `/*/Cargo.toml`. +fn collect_manifests(root: &Path, problems: &mut Vec) -> Vec { + let mut out = Vec::new(); + let mut dirs = vec![root.to_path_buf()]; + + match fs::read_dir(root) { + Ok(entries) => { + for entry in entries.flatten() { + let p = entry.path(); + if p.is_dir() && p.join("Cargo.toml").is_file() { + dirs.push(p); + } + } + } + Err(e) => problems.push(format!("cannot list {}: {e}", root.display())), + } + + for dir in dirs { + let manifest_path = dir.join("Cargo.toml"); + match fs::read_to_string(&manifest_path) { + Ok(text) => out.push(parse_manifest(&dir, &text)), + Err(e) => problems.push(format!("cannot read {}: {e}", manifest_path.display())), + } + } + out +} + +fn parse_manifest(dir: &Path, text: &str) -> Manifest { + let mut m = Manifest { + dir: dir.to_path_buf(), + ..Manifest::default() + }; + let mut section = String::new(); + let mut features_src = String::new(); + + for raw in text.lines() { + let line = strip_comment(raw).trim().to_string(); + if line.is_empty() { + continue; + } + if line.starts_with('[') && line.ends_with(']') { + section = line[1..line.len() - 1].to_string(); + continue; + } + if section == "package" && line.starts_with("name") { + if let Some(v) = quoted_after(&line, "name") { + m.name = v; + } + } else if is_dependency_section(§ion) { + if let (Some(key), Some(rel)) = (key_of(&line), quoted_after(&line, "path")) { + m.path_deps.push((key, dir.join(rel))); + } + } else if section == "features" { + features_src.push_str(&line); + features_src.push('\n'); + } + } + m.features = parse_features(&features_src); + m +} + +/// `[dependencies]`, `[build-dependencies]`, `[target.'cfg(..)'.dependencies]` — but not +/// dev-dependencies, which cannot carry feature forwarding for downstream builds. +fn is_dependency_section(section: &str) -> bool { + section.ends_with("dependencies") && !section.contains("dev-dependencies") +} + +fn strip_comment(line: &str) -> &str { + match line.find('#') { + Some(i) => &line[..i], + None => line, + } +} + +/// `foo = { .. }` -> `foo` +fn key_of(line: &str) -> Option { + let eq = line.find('=')?; + let key = line[..eq].trim(); + if key.is_empty() || key.contains('.') { + return None; + } + Some(key.trim_matches('"').to_string()) +} + +/// The first quoted value after `key` on the line: `path = "../x"` -> `../x` +fn quoted_after(line: &str, key: &str) -> Option { + let i = line.find(key)?; + let rest = &line[i + key.len()..]; + let j = rest.find('"')?; + let rest = &rest[j + 1..]; + let k = rest.find('"')?; + Some(rest[..k].to_string()) +} + +/// Parse a `[features]` section body into (name, entries). Feature values are always arrays +/// of strings, so a bracket scan is sufficient and needs no TOML dependency. +fn parse_features(section: &str) -> Vec<(String, Vec)> { + let mut out = Vec::new(); + let mut rest = section; + while let Some(eq) = rest.find('=') { + let name = rest[..eq].trim().trim_matches('"').to_string(); + let after = &rest[eq + 1..]; + let Some(open) = after.find('[') else { break }; + let Some(close_rel) = after[open..].find(']') else { + break; + }; + let close = open + close_rel; + let entries = after[open + 1..close] + .split(',') + .map(|s| s.trim().trim_matches('"').to_string()) + .filter(|s| !s.is_empty()) + .collect(); + if !name.is_empty() { + out.push((name, entries)); + } + rest = &after[close + 1..]; + } + out +} + +/// Compare directories through the filesystem so `../fn-dsa-comm` matches the scanned +/// `/fn-dsa-comm` (and `\\?\` prefixes on Windows do not defeat the match). +fn same_dir(a: &Path, b: &Path) -> bool { + match (fs::canonicalize(a), fs::canonicalize(b)) { + (Ok(a), Ok(b)) => a == b, + _ => a == b, + } +} diff --git a/lib-q-fn-dsa/fn-dsa-sign/Cargo.toml b/lib-q-fn-dsa/fn-dsa-sign/Cargo.toml index ba59b29b..3f202228 100644 --- a/lib-q-fn-dsa/fn-dsa-sign/Cargo.toml +++ b/lib-q-fn-dsa/fn-dsa-sign/Cargo.toml @@ -27,4 +27,9 @@ small_context = [] no_avx2 = [] div_emu = [] sqrt_emu = [] -shake256x4 = [] +# MUST forward to `-comm`: the type this crate switches to under the feature +# (`fn_dsa_comm::shake::SHAKE256x4`) is itself `#[cfg(feature = "shake256x4")]` inside +# `-comm`. Declared empty (upstream fn-dsa 0.3.0 does the same), `cargo check -p +# lib-q-fn-dsa-sign --features shake256x4` fails with E0425 — the signatures flip to a type +# that was never compiled. Guarded by ../fn-dsa-kgen/tests/feature_forwarding.rs. +shake256x4 = ["lib-q-fn-dsa-comm/shake256x4"] From 15fd2102c1c59d40377b47a4c029f493e60500a4 Mon Sep 17 00:00:00 2001 From: Nexlab-One <86677687+Nexlab-One@users.noreply.github.com> Date: Wed, 29 Jul 2026 12:12:45 +0200 Subject: [PATCH 2/3] fix(fn-dsa): guard false-positived on transitive forwarding, and said so untruthfully MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The shake256x4 guard added in fe7cfbd inspected only DIRECT entries of the `shake256x4` feature list. Cargo does not: a feature entry with no `/` names another feature of the same crate, which cargo expands in turn. So this wiring in fn-dsa-kgen/Cargo.toml _comm_x4 = ["lib-q-fn-dsa-comm/shake256x4"] shake256x4 = ["_comm_x4"] is correct — `cargo check -p lib-q-fn-dsa-kgen --features shake256x4` exits 0 — yet the guard failed it (exit 101), and the failure text read: `cargo check -p lib-q-fn-dsa-kgen --features shake256x4` will fail to compile. That sentence was asserted flatly and was false in that state. The test runs no compiler; it was predicting an outcome it had never observed, and predicting it wrong. That is the worse half of the bug: a false positive costs one confused afternoon, but a guard that states a falsehood about WHY it failed teaches the next person to distrust it and delete it — taking the real defect's only detector with it. WHAT CHANGED Both available remedies, because they fix different faults and the second costs nothing once the first is in place: 1. `resolve_feature` walks the feature graph within a manifest transitively (worklist + `seen` set), so indirect wiring is accepted exactly as cargo accepts it. Entries naming another crate (`dep/feat`, `dep?/feat`) or an optional dep (`dep:name`) are terminal. `seen` makes a cyclic manifest terminate instead of hanging; cargo diagnoses cycles itself. 2. The failure message now states only what was inspected — the resolved entry set, and that none of it forwards — then names what it did NOT do ("NOT checked: whether anything compiles. This test runs no build"), points at the `cargo check` counterpart in .github/actions/test-fn-dsa/action.yml, and gives the cross-crate cfg reasoning as rationale rather than as a prediction about this particular build. Transitive resolution alone would have left the message free to lie the next time it fired; an honest message alone would have left the false positive. NOT WEAKENED The defect that was live on main is still caught. Verified in both directions on the real manifest, restored afterwards: shake256x4 = ["lib-q-fn-dsa-comm/shake256x4"] guard 0 check 0 _comm_x4 = [...]; shake256x4 = ["_comm_x4"] guard 0 check 0 (was 101) shake256x4 = [] guard 101 check 101 (E0433) The guard's verdict now matches the compiler's on all three. Eight unit tests over synthetic manifests pin both directions permanently, so the next person does not have to hand-edit and hand-restore Cargo.toml to trust it: empty list, wrong target feature and dangling indirection are rejected; direct, indirect, multi-hop and weak (`dep?/feat`) forwards are accepted; cycles terminate. REQUIRED_PAIRS gains ("lib-q-fn-dsa", "lib-q-fn-dsa-alg"), the outermost link in the chain, so every crate in the `cargo check -p --features shake256x4` sweep is now pinned as must-be-evaluated. Confirmed non-vacuous: breaking that forward to `[]` fails the guard with the forwarding message. Re-confirmed green on the pinned toolchain (nightly-2026-07-24), x86_64 Windows: `cargo check -p --features shake256x4` exits 0 for -comm, -kgen, -sign, -alg and lib-q-fn-dsa. That sweep is not vacuous — cargo errors on an undeclared feature ("the package 'lib-q-fn-dsa' does not contain this feature"), so exit 0 means the feature really was selected. `cargo clippy -p lib-q-fn-dsa-kgen --all-targets` and `cargo fmt --check` are clean. --- .../fn-dsa-kgen/tests/feature_forwarding.rs | 166 ++++++++++++++++-- 1 file changed, 156 insertions(+), 10 deletions(-) diff --git a/lib-q-fn-dsa/fn-dsa-kgen/tests/feature_forwarding.rs b/lib-q-fn-dsa/fn-dsa-kgen/tests/feature_forwarding.rs index 30d6f0cf..b1ce839e 100644 --- a/lib-q-fn-dsa/fn-dsa-kgen/tests/feature_forwarding.rs +++ b/lib-q-fn-dsa/fn-dsa-kgen/tests/feature_forwarding.rs @@ -28,6 +28,15 @@ //! The compile-level counterpart (`cargo check -p --features shake256x4`) lives in //! `.github/actions/test-fn-dsa/action.yml`. //! +//! WHAT THIS TEST DOES AND DOES NOT ESTABLISH +//! +//! It resolves feature entries the way cargo does — transitively within each manifest, since a +//! bare entry names another feature of the *same* crate — and asserts that the forward is +//! reachable from `shake256x4`. Indirect wiring (`shake256x4 = ["_x"]`, `_x = ["dep/x4"]`) is +//! therefore accepted, because cargo accepts it. It runs no compiler, so it must never claim a +//! build outcome: on failure it reports the resolved entry set it actually inspected and names +//! the compile check it did *not* perform. A guard that misstates why it failed gets disabled. +//! //! SCOPE: deliberately limited to `shake256x4`. `no_avx2` has the same missing forward in //! `-kgen`/`-sign`/`-vrfy`, but it gates only internal dispatch, not a cross-crate item, so //! it builds today; adding the forward there would change which SHAKE code path existing @@ -46,12 +55,15 @@ const FEATURE: &str = "shake256x4"; /// Pairs (`dependent`, `dependency`) that MUST be evaluated by this test. If a manifest is /// reshaped so the parser stops seeing them, the test fails as loudly as a real regression /// instead of silently passing on an empty set. -const REQUIRED_PAIRS: [(&str, &str); 5] = [ +const REQUIRED_PAIRS: [(&str, &str); 6] = [ ("lib-q-fn-dsa-kgen", "lib-q-fn-dsa-comm"), ("lib-q-fn-dsa-sign", "lib-q-fn-dsa-comm"), ("lib-q-fn-dsa-alg", "lib-q-fn-dsa-comm"), ("lib-q-fn-dsa-alg", "lib-q-fn-dsa-kgen"), ("lib-q-fn-dsa-alg", "lib-q-fn-dsa-sign"), + // The outermost link: `lib-q-fn-dsa` (the published facade) -> `-alg`. Every crate named + // in the `cargo check -p --features shake256x4` sweep is now pinned here. + ("lib-q-fn-dsa", "lib-q-fn-dsa-alg"), ]; #[derive(Debug, Default)] @@ -91,7 +103,7 @@ fn shake256x4_is_forwarded_to_the_crate_that_defines_it() { let mut evaluated: BTreeSet<(String, String)> = BTreeSet::new(); for m in &manifests { - let Some(entries) = m.feature(FEATURE) else { + let Some(enabled) = resolve_feature(m, FEATURE) else { continue; }; for (dep_key, dep_dir) in &m.path_deps { @@ -106,15 +118,27 @@ fn shake256x4_is_forwarded_to_the_crate_that_defines_it() { // Cargo's two spellings: unconditional and "only if the optional dep is enabled". let want = format!("{dep_key}/{FEATURE}"); let want_weak = format!("{dep_key}?/{FEATURE}"); - if !entries.iter().any(|e| *e == want || *e == want_weak) { + if !enabled.contains(&want) && !enabled.contains(&want_weak) { + // Register discipline: state only what was inspected. No compiler ran here, + // so this must not predict a build outcome — see the module header. problems.push(format!( - "{}: feature `{FEATURE} = {entries:?}` does not enable `{want}`, but \ - `{}` gates items on `{FEATURE}`. `cargo check -p {} --features {FEATURE}` \ - will fail to compile. Fix: add \"{want}\" to that feature list in {}.", - m.name, - dep.name, - m.name, - m.dir.join("Cargo.toml").display(), + "{dependent}: `{FEATURE}` does not forward to `{dep_name}`.\n \ + Checked: the entries reachable from `{FEATURE}` in {manifest} — direct \ + entries plus the same-crate features they enable, resolved transitively \ + — are {enabled:?}; none of them is `{want}` or `{want_weak}`.\n \ + NOT checked: whether anything compiles. This test runs no build; the \ + `cargo check -p {dependent} --features {FEATURE}` counterpart lives in \ + .github/actions/test-fn-dsa/action.yml.\n \ + Why the forward is required: `{dep_name}` declares `{FEATURE}` and gates \ + items on it, so `--features {FEATURE}` on `{dependent}` alone turns on \ + this crate's own `#[cfg(feature = \"{FEATURE}\")]` code while `{dep_name}` \ + is still built without the feature — anything that code reaches for \ + behind that gate is configured out (E0433/E0425).\n \ + Fix: add \"{want}\" to `{FEATURE}`, or to a feature `{FEATURE}` enables, \ + in {manifest}.", + dependent = m.name, + dep_name = dep.name, + manifest = m.dir.join("Cargo.toml").display(), )); } } @@ -139,6 +163,41 @@ fn shake256x4_is_forwarded_to_the_crate_that_defines_it() { ); } +/// Everything `--features ` turns on for this crate, resolved the way cargo resolves +/// it: an entry with no `/` and no `dep:` prefix names ANOTHER feature of the SAME crate, which +/// cargo expands in turn. So `shake256x4 = ["_x"]` + `_x = ["lib-q-fn-dsa-comm/shake256x4"]` +/// forwards exactly as the directly-written form does, and a guard that inspected only direct +/// entries would report that manifest as broken when `cargo check` proves it is not. +/// +/// Entries naming another crate (`dep/feat`, `dep?/feat`) or an optional dependency (`dep:name`) +/// are collected but not followed — they are not features of this crate. `seen` makes a cyclic +/// or self-referential manifest terminate rather than hang; cargo rejects feature cycles itself, +/// so diagnosing one is not this guard's job. +/// +/// `None` when the crate does not declare `feature` at all. +fn resolve_feature(m: &Manifest, feature: &str) -> Option> { + let direct = m.feature(feature)?; + + let mut enabled = BTreeSet::new(); + let mut seen: BTreeSet = BTreeSet::from([feature.to_string()]); + let mut queue: Vec = direct.to_vec(); + + while let Some(entry) = queue.pop() { + if entry.contains('/') || entry.starts_with("dep:") { + enabled.insert(entry); + continue; + } + if !seen.insert(entry.clone()) { + continue; + } + if let Some(next) = m.feature(&entry) { + queue.extend(next.iter().cloned()); + } + enabled.insert(entry); + } + Some(enabled) +} + /// Read `/Cargo.toml` plus every `/*/Cargo.toml`. fn collect_manifests(root: &Path, problems: &mut Vec) -> Vec { let mut out = Vec::new(); @@ -267,3 +326,90 @@ fn same_dir(a: &Path, b: &Path) -> bool { _ => a == b, } } + +/// Both directions of the guard, pinned against synthetic manifests so they hold without +/// hand-editing (and hand-restoring) the real ones. +/// +/// The guard's value is entirely in the gap between these two: it must fail on the wiring that +/// really was broken on main, and pass on wiring cargo really does accept. A regression in +/// either direction — a guard that stops catching the defect, or one that cries wolf until +/// someone deletes it — is a guard that is not doing its job. +#[cfg(test)] +mod resolution { + use super::*; + + /// Minimal manifest text; `$X4` is the `[features]` body under test. + fn manifest(features: &str) -> Manifest { + let text = format!( + "[package]\nname = \"probe\"\n\n\ + [dependencies]\ncomm = {{ version = \"0.0.9\", path = \"../comm\" }}\n\n\ + [features]\ndefault = []\n{features}\n" + ); + parse_manifest(Path::new("."), &text) + } + + fn forwards(features: &str) -> bool { + let m = manifest(features); + let enabled = resolve_feature(&m, FEATURE).expect("feature is declared"); + enabled.contains(&format!("comm/{FEATURE}")) || + enabled.contains(&format!("comm?/{FEATURE}")) + } + + /// The defect that was live on main: declared, forwards nothing. MUST still be caught. + #[test] + fn empty_feature_list_is_rejected() { + assert!(!forwards("shake256x4 = []")); + } + + /// A forward to some *other* feature of the dependency is not the forward we need. + #[test] + fn wrong_target_feature_is_rejected() { + assert!(!forwards("shake256x4 = [\"comm/no_avx2\"]")); + } + + /// A same-crate feature that does not exist resolves to nothing useful — still rejected. + #[test] + fn dangling_indirection_is_rejected() { + assert!(!forwards("shake256x4 = [\"_typo\"]")); + } + + #[test] + fn direct_forward_is_accepted() { + assert!(forwards("shake256x4 = [\"comm/shake256x4\"]")); + } + + /// The false positive this module exists to prevent: `cargo check` accepts this wiring + /// (verified out of band, exit 0), so the guard must accept it too. + #[test] + fn indirect_forward_is_accepted() { + assert!(forwards( + "_comm_x4 = [\"comm/shake256x4\"]\nshake256x4 = [\"_comm_x4\"]" + )); + } + + #[test] + fn multi_hop_and_weak_forward_are_accepted() { + assert!(forwards( + "shake256x4 = [\"a\"]\na = [\"b\"]\nb = [\"comm/shake256x4\"]" + )); + assert!(forwards("shake256x4 = [\"a\"]\na = [\"comm?/shake256x4\"]")); + } + + /// A cyclic manifest must terminate rather than hang the test suite. Cargo rejects cycles + /// itself; the guard only has to not spin. + #[test] + fn cycles_terminate() { + assert!(!forwards( + "shake256x4 = [\"a\"]\na = [\"b\"]\nb = [\"a\", \"shake256x4\"]" + )); + assert!(forwards( + "shake256x4 = [\"a\"]\na = [\"shake256x4\", \"comm/shake256x4\"]" + )); + } + + /// An undeclared feature is not a violation — nothing to forward. + #[test] + fn undeclared_feature_resolves_to_none() { + assert!(resolve_feature(&manifest("no_avx2 = []"), FEATURE).is_none()); + } +} From 3011382aa0ecf68b1599449f4b85d7e510ff8344 Mon Sep 17 00:00:00 2001 From: Nexlab-One <86677687+Nexlab-One@users.noreply.github.com> Date: Wed, 29 Jul 2026 13:17:54 +0200 Subject: [PATCH 3/3] =?UTF-8?q?fix(fn-dsa):=20correct=20the=20feature=5Ffo?= =?UTF-8?q?rwarding=20rationale=20=E2=80=94=20not=20every=20pair=20errors?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "Why the forward is required" claimed every missing shake256x4 forward gets "configured out" as E0433/E0425. That's only true for pairs where the dependent's own code directly names an item the dependency gates behind the same flag (kgen/sign naming -comm's SHAKE256x4). For alg->kgen, alg->sign, alg->comm, and lib-q-fn-dsa->alg it isn't: the build succeeds silently and the crate keeps un-forwarded behavior instead — a correctness bug with no compiler diagnostic. The lib-q-fn-dsa->alg case is sharper still: that crate has zero shake256x4 cfg sites of its own, so the old text's "turns on this crate's own #[cfg(...)] code" premise didn't even apply. Verified: emptying alg's forward to -kgen or -sign compiles clean but fails `cargo test -p lib-q-fn-dsa-alg --features shake256x4 test_kat` on a byte mismatch, not a compile error. Emptying alg->comm alone produces no failure at all in isolation (kgen/sign's own forwards already turn comm's feature on transitively). Emptying the top facade's forward compiles clean and passes every existing test — lib-q-fn-dsa/src has no shake256x4 cfg sites and no KAT test exists at that layer to catch the divergence. The guard logic and REQUIRED_PAIRS list are unchanged; only the inline rationale text changes. --- .../fn-dsa-kgen/tests/feature_forwarding.rs | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/lib-q-fn-dsa/fn-dsa-kgen/tests/feature_forwarding.rs b/lib-q-fn-dsa/fn-dsa-kgen/tests/feature_forwarding.rs index b1ce839e..6950ab7b 100644 --- a/lib-q-fn-dsa/fn-dsa-kgen/tests/feature_forwarding.rs +++ b/lib-q-fn-dsa/fn-dsa-kgen/tests/feature_forwarding.rs @@ -130,10 +130,18 @@ fn shake256x4_is_forwarded_to_the_crate_that_defines_it() { `cargo check -p {dependent} --features {FEATURE}` counterpart lives in \ .github/actions/test-fn-dsa/action.yml.\n \ Why the forward is required: `{dep_name}` declares `{FEATURE}` and gates \ - items on it, so `--features {FEATURE}` on `{dependent}` alone turns on \ - this crate's own `#[cfg(feature = \"{FEATURE}\")]` code while `{dep_name}` \ - is still built without the feature — anything that code reaches for \ - behind that gate is configured out (E0433/E0425).\n \ + items on it. Without the forward, `--features {FEATURE}` on `{dependent}` \ + does not turn it on in `{dep_name}`, which stays built without it — and \ + this test cannot tell you what breaks as a result, because that depends on \ + `{dependent}`'s own code, not on manifest wiring. Where `{dependent}` \ + itself names an item that only exists under this same flag (e.g. `-kgen`/ \ + `-sign` naming `-comm`'s gated `SHAKE256x4`), that reference fails to \ + resolve and the build breaks: E0433/E0425. Where it does not — including \ + `{dependent}` having no `{FEATURE}`-gated code of its own — the build \ + succeeds and `{dependent}` silently keeps the un-forwarded behavior \ + instead: a correctness bug with no compiler diagnostic, caught only if \ + some downstream check happens to compare actual output, e.g. a KAT byte \ + comparison.\n \ Fix: add \"{want}\" to `{FEATURE}`, or to a feature `{FEATURE}` enables, \ in {manifest}.", dependent = m.name,