From 7c4930925f50e3881f9d9bae81931b23b0e11624 Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Fri, 24 Jul 2026 21:49:47 -0700 Subject: [PATCH 1/7] perf(cognitive): inherit nesting via the walker `get_nesting_from_map` recovered a node's inherited `(nesting, depth, lambda)` triple with `node.parent()`. `Node::parent` is O(depth) -- tree-sitter stores no parent pointer, so `ts_node_parent` restarts at the root and descends -- which made the metric O(nodes x depth). With the `tokens` walk fixed in #1052 this was the dominant remaining cost on nesting-heavy input. The map is now keyed so a node's own slot holds what that node inherits: after a node's `compute` has run, the walker seeds each child's slot from it. The lookup becomes a single hash probe and no `parent()` call remains on the per-node path. Values are unchanged, including the case that made this non-obvious. Python's comprehension handling pre-writes its clause children's slots during the *comprehension's* compute (the #421 fix, so clause nesting does not depend on sibling traversal order), and those deliberately differ from the comprehension's own triple. The walker therefore seeds with `or_insert`, not `insert`; a blanket overwrite would have silently shifted cognitive for every Python comprehension. All 22 read sites and both non-self write sites were checked before settling on this shape. On shapes that exercise only this path, whole-file analysis is now linear: nested parentheses at depths 8000 / 16000 / 32000 / 64000 take 13 / 23 / 45 / 88 ms, so a 128 KB file completes in under a tenth of a second, where 8 KB previously took over a second. This does NOT make deeply nested code linear in general. `Checker::is_else_if` calls `Node::parent` per `if` across 13 languages, so nested and `else if`-chained conditionals stay quadratic: 60 / 224 / 824 / 3325 ms at depths 1000 / 2000 / 4000 / 8000, against 6 / 10 / 16 / 44 ms for the identical shape built from `while`. The regression test asserts the value *and* the scaling ratio. A value assertion alone would let a regression hang CI rather than fail it, since the pre-fix code produced identical numbers, only quadratically. An absolute wall-clock budget was rejected: it is calibrated to one machine, and this suite also runs under `cargo llvm-cov` and on shared Windows / macOS runners. Doubling the depth costs ~2x when the lookup is O(1) and ~4x when it is O(depth), independent of host speed -- revert-tested at 4.2x failing against ~2x passing. The test uses nested `while`, not `if`, precisely because `is_else_if` leaves the `if` shape quadratic; anchoring to it would have measured the unfixed path. `metrics_inner` grew 16% in halstead effort past its recorded value; the baseline is refreshed here per the same-PR discipline. Still outstanding, all tracked in #1062: `Checker::is_else_if` (above), `Node::count_specific_ancestors` (4000 nested declarations, ~557 ms; also called twice per node by Python's boolean-operator handling), `increment_function_depth` (4000 nested Rust fns, 55 KB, ~1.9 s), and Elixir's `is_inside_quote_block`, which runs for every node behind no metric selection and so cannot be deselected (4000 nested quote blocks, 52 KB, ~2.7 s). A shared parent lookup owned by the walker would retire them together -- an earlier claim that each needs a different mechanism was wrong. Refs #1062 --- .bca-baseline.toml | 10 ++-- CHANGELOG.md | 46 +++++++++++++++--- src/metrics/cognitive.rs | 84 +++++++++++++++++++++++++++++++-- src/metrics/cognitive/python.rs | 13 ++--- src/spaces/compute.rs | 59 +++++++++++++++++++++++ 5 files changed, 190 insertions(+), 22 deletions(-) diff --git a/.bca-baseline.toml b/.bca-baseline.toml index d5769746..ec7ec152 100644 --- a/.bca-baseline.toml +++ b/.bca-baseline.toml @@ -728,14 +728,14 @@ value = 16.0 [[entry]] path = "src/metrics/cognitive.rs" qualified = "tcl_switch_decision_arms" -start_line = 448 +start_line = 454 metric = "nargs" value = 7.0 [[entry]] path = "src/metrics/cognitive.rs" qualified = "tcl_switch_decision_arms" -start_line = 448 +start_line = 454 metric = "nexits" value = 6.0 @@ -938,16 +938,16 @@ value = 8.0 [[entry]] path = "src/spaces/compute.rs" qualified = "compute_per_node" -start_line = 217 +start_line = 218 metric = "halstead.effort" value = 61523.95778223676 [[entry]] path = "src/spaces/compute.rs" qualified = "metrics_inner" -start_line = 386 +start_line = 421 metric = "halstead.effort" -value = 122486.96209573474 +value = 142419.45851755128 [[entry]] path = "src/suppression.rs" diff --git a/CHANGELOG.md b/CHANGELOG.md index 65fffa8a..958364d8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -56,6 +56,32 @@ for historical reference. ### Fixed +- The `cognitive` metric's nesting lookup is no longer quadratic in + nesting depth (#1062). It recovered each node's inherited nesting via + `node.parent()`, which is `O(depth)` — tree-sitter stores no parent + pointer — making the lookup `O(nodes × depth)`. The walker now hands + each node its inherited nesting directly, so the lookup is `O(1)`. + Cognitive values are unchanged. On shapes that exercise only this path, + whole-file analysis is now linear: nested parentheses at depths + 8000 / 16000 / 32000 / 64000 take 13 / 23 / 45 / 88 ms, so a 128 KB + file completes in under a tenth of a second. + + **Deeply nested code is still superlinear in general** — this fixes one + of several `Node::parent` call sites, and *not* the one users hit most. + `Checker::is_else_if` calls `parent()` (or `previous_sibling()`, which + is implemented via `parent()`) for every `if`, across 13 languages, so + nested and `else if`-chained conditionals remain quadratic: a 4000-deep + C `if` chain takes ~3.3 s at depth 8000 where the equivalent `while` + chain takes 44 ms. Also outstanding: `Node::count_specific_ancestors` + (used by `loc` for C-family, Java, C#, Go, Groovy, Objective-C, and + twice per node by Python's boolean-operator handling), + `cognitive::increment_function_depth` (4000 nested Rust `fn`s, 55 KB → + ~1.9 s), and Elixir's `is_inside_quote_block`, which runs for every + node behind no metric selection and so cannot be deselected (4000 + nested `quote` blocks, 52 KB → ~2.7 s). All tracked in #1062, which + stays open; a shared parent lookup owned by the walker would retire + them together. + - Deeply nested source no longer costs quadratic time in the `tokens` metric (#1052). `Tokens` decided whether a leaf sat inside a comment by walking that leaf's ancestor chain — and `Node::parent` is itself @@ -116,13 +142,19 @@ for historical reference. ### Security -- Removed a remotely-triggerable CPU-exhaustion vector in the `tokens` - metric — a few kilobytes of deeply nested source could pin a core for - minutes against the unauthenticated `bca-web` endpoints, whose parse - deadline frees the client but cannot cancel the blocking task. See the - `tokens` entry under **Fixed** (#1052) for detail and for the residual - superlinear paths that remain (#1062); operators analysing untrusted - input should still bound request concurrency. +- Narrowed a remotely-triggerable CPU-exhaustion vector: a few kilobytes + of deeply nested source could pin a core for minutes against the + unauthenticated `bca-web` endpoints, whose parse deadline frees the + client but cannot cancel the blocking task. Two of the quadratic paths + are gone — the `tokens` ancestor walk (#1052) and the `cognitive` + nesting lookup (#1062); see those entries under **Fixed**. + + **This is not closed.** `Checker::is_else_if` still calls + `Node::parent` per `if` across 13 languages, so deeply nested or + `else if`-chained conditionals — ordinary generated and state-machine + code, not just adversarial input — remain quadratic, along with three + further ancestor walks. All tracked in #1062. Operators analysing + untrusted input must still bound request concurrency and input size. - Cleared the two RUSTSEC advisories behind the OpenSSF Scorecard Vulnerabilities alert: `anyhow` `1.0.102` → `1.0.103` (unsound `Error::downcast_mut()`, RUSTSEC-2026-0190) and `memmap2` `0.9.10` diff --git a/src/metrics/cognitive.rs b/src/metrics/cognitive.rs index c0c9a7d7..63d97444 100644 --- a/src/metrics/cognitive.rs +++ b/src/metrics/cognitive.rs @@ -251,14 +251,20 @@ fn increment_branch_extension(stats: &mut Stats) { stats.boolean_seq.reset(); } +/// Returns the `(nesting, depth, lambda)` triple `node` inherits from its +/// parent. +/// +/// The map is keyed so that a node's own slot holds what it *inherits*: +/// the walker seeds each child's slot from its parent's slot after the +/// parent's `compute` has run (see `propagate_nesting_to_children` in +/// `spaces::compute`). Reading `node.parent()` here instead would cost +/// `O(depth)` per node — `Node::parent` walks down from the root — which +/// made this metric quadratic in nesting depth (#1062). fn get_nesting_from_map( node: &Node, nesting_map: &HashMap, ) -> (usize, usize, usize) { - node.parent() - .and_then(|parent| nesting_map.get(&parent.id())) - .copied() - .unwrap_or((0, 0, 0)) + nesting_map.get(&node.id()).copied().unwrap_or((0, 0, 0)) } fn increment_function_depth>(depth: &mut usize, node: &Node, stops: &[T]) { @@ -9381,4 +9387,74 @@ end", nested_sum.get(), ); } + + /// Deeply nested input must stay tractable (#1062). + /// + /// `get_nesting_from_map` used to recover a node's inherited nesting + /// via `node.parent()`, which is `O(depth)` — tree-sitter stores no + /// parent pointer — making the metric `O(nodes × depth)`. The walker + /// now seeds each child's slot from its parent's, so the lookup is + /// `O(1)`. + /// + /// Uses `while`, deliberately, **not** `if`: `Checker::is_else_if` + /// calls `node.parent()` for every `if_statement`, so nested `if`s + /// are still quadratic for reasons this fix does not touch. Anchoring + /// the assertion to that shape would measure the unfixed path — at + /// nesting depth 8000 a release build takes 3.3 s of `if` versus + /// 44 ms of `while`. + /// + /// Asserts a *ratio* rather than a wall-clock budget. The pre-fix code + /// produced identical values, only quadratically, so a value assertion + /// alone would let a regression hang CI instead of failing it — but an + /// absolute budget is calibrated to one machine and this suite also + /// runs under `cargo llvm-cov` and on shared Windows / macOS runners. + /// Doubling the depth costs ~2x when the lookup is `O(1)` and ~4x when + /// it is `O(depth)`, and that ratio is independent of host speed. + #[test] + fn cognitive_deep_nesting_is_tractable() { + // Restricted to `Cognitive` — which pulls in `Nom` as a declared + // dependency, so this narrows the work rather than isolating it. + fn cognitive_of(source: &str) -> (u64, std::time::Duration) { + let started = std::time::Instant::now(); + let sum = crate::analyze( + crate::Source::new(crate::LANG::C, source.as_bytes()), + crate::MetricsOptions::default().with_only(&[crate::Metric::Cognitive]), + ) + .expect("source must analyse") + .metrics + .cognitive + .cognitive_sum(); + (sum, started.elapsed()) + } + + // Each level adds its own nesting penalty, so cognitive grows as + // 1 + 2 + … + n. Asserting it pins that nesting is still inherited + // correctly at depth, independently of the timing below. + let nested_whiles = |n: usize| -> String { + format!( + "int main(){{ {} 1; {} }}\n", + "while (a) { ".repeat(n), + "} ".repeat(n) + ) + }; + let expected = |n: u64| n * (n + 1) / 2; + + assert_eq!(cognitive_of(&nested_whiles(3)).0, expected(3), "1 + 2 + 3"); + + let base = 2_000; + let (shallow_sum, shallow_time) = cognitive_of(&nested_whiles(base)); + let (deep_sum, deep_time) = cognitive_of(&nested_whiles(base * 2)); + assert_eq!(shallow_sum, expected(base as u64)); + assert_eq!(deep_sum, expected(base as u64 * 2)); + + // Guard against a zero denominator on a very fast machine. + let shallow_ns = shallow_time.as_nanos().max(1); + let ratio = deep_time.as_nanos() as f64 / shallow_ns as f64; + assert!( + ratio < 3.0, + "doubling nesting depth cost {ratio:.1}x ({shallow_time:?} -> {deep_time:?}); \ + an O(1) inherited lookup scales ~2x, the O(depth) `Node::parent` \ + lookup this replaced scales ~4x (#1062)" + ); + } } diff --git a/src/metrics/cognitive/python.rs b/src/metrics/cognitive/python.rs index c3c27401..73d7147a 100644 --- a/src/metrics/cognitive/python.rs +++ b/src/metrics/cognitive/python.rs @@ -138,12 +138,13 @@ impl Cognitive for PythonCode { python_comprehension_clause_nesting(node, nesting, depth, lambda, nesting_map); } ForInClause | IfClause => { - // Nesting was precomputed on the comprehension node (visited - // first in pre-order) into this clause's own map slot, so read - // it back instead of re-scanning siblings per clause. - if let Some(&(clause_nesting, _, _)) = nesting_map.get(&node.id()) { - nesting = clause_nesting; - } + // `nesting` already holds this clause's own value: the + // comprehension (visited first in pre-order) precomputed it + // into this clause's map slot, and since #1062 a node reads + // its own slot, so the read at the top of `compute` picks it + // up. Before #1062 a node read its *parent's* slot, so this + // arm had to re-read the slot explicitly; that override is + // now a no-op and has been removed. stats.nesting = nesting + depth + lambda; increment(stats); stats.boolean_seq.reset(); diff --git a/src/spaces/compute.rs b/src/spaces/compute.rs index 97ada19c..bea6ebd7 100644 --- a/src/spaces/compute.rs +++ b/src/spaces/compute.rs @@ -357,6 +357,59 @@ struct Walk { in_comment: bool, } +/// Seeds each child's `nesting_map` slot from `node`'s own, so +/// `Cognitive` can read the triple it inherits without calling +/// `Node::parent`. +/// +/// A node's slot means two different things either side of its +/// `compute`, and the distinction is load-bearing: +/// +/// - **on entry** it holds what the node inherits — this is what +/// `get_nesting_from_map` reads; +/// - **on exit** each language's `compute` has overwritten it with the +/// post-increment triple its children should see — this is what this +/// function hands down. +/// +/// So the write at the end of every `Cognitive::compute` must stay at the +/// end. Moving it to the top would make every descendant inherit the +/// pre-increment triple and silently under-count. +/// +/// `or_insert`, not `insert`: Python's comprehension handling pre-writes +/// its clause children's slots during the *comprehension's* `compute` +/// (the #421 fix, so clause nesting does not depend on sibling traversal +/// order), and those values deliberately differ from the comprehension's +/// own triple. A blanket overwrite would clobber them — the +/// `python_comprehension_*` tests in `metrics::cognitive` are what catch +/// it. +/// +/// Grammars whose `Cognitive` impl is the macro's no-op (`Preproc`, +/// `Ccomment`) never write a slot, so they inherit the root's `(0, 0, 0)` +/// unchanged. That matches the pre-#1062 behaviour only because zero is +/// the identity here; it costs them a map entry per node that nothing +/// reads. +fn propagate_nesting_to_children( + node: &Node, + children: &[(Node<'_>, Walk)], + nesting_map: &mut HashMap, +) { + // Leaves are roughly half of a real AST, so bail before hashing a key + // we would only read to iterate zero children. + if children.is_empty() { + return; + } + // Unreachable in practice: the root is seeded before the walk and every + // other node is seeded by this function before it is popped. Degrading + // to "no inheritance" would silently zero a whole subtree's nesting, so + // fail loudly in debug rather than absorb it. + let Some(&inherited) = nesting_map.get(&node.id()) else { + debug_assert!(false, "node {} has no nesting slot", node.id()); + return; + }; + for (child, _) in children { + nesting_map.entry(child.id()).or_insert(inherited); + } +} + /// Pushes `node`'s direct children onto the traversal `stack`, each tagged /// with `tag`. /// @@ -537,6 +590,8 @@ pub(crate) fn metrics_inner( ); } + let first_child = stack.len(); + push_children( &mut cursor, &node, @@ -547,6 +602,10 @@ pub(crate) fn metrics_inner( &mut children, &mut stack, ); + + if selected.contains(Metric::Cognitive) { + propagate_nesting_to_children(&node, &stack[first_child..], &mut nesting_map); + } } finalize::(&mut state_stack, usize::MAX, selected); From 94fadd648a46e975c6e75bcd8d79ac46dc5fdab0 Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Sat, 25 Jul 2026 11:52:26 -0700 Subject: [PATCH 2/7] test: unify verbatim-analyse helpers `loc_verbatim`, `tokens_of` and `cognitive_of` were three near-identical bodies added in three consecutive PRs, each doing `analyze(Source::new(lang, bytes), opts).expect(..).metrics`. They exist because `check_metrics` cannot serve these tests: `check_func_space` normalises CRLF and trims then re-appends a trailing newline, so a construct ending at EOF is unreachable through it and such a test passes vacuously -- that is how #1051 escaped a ~3000-test suite -- and its `check` parameter is a bare `fn` pointer, so a test that needs to compare two analyses cannot get a value out. Replaced by one `metrics_verbatim(lang, source, options)` in `src/tools.rs`, beside the other cross-module test helpers, with that rationale stated once instead of three times. `#[track_caller]` so a failing `expect` reports the test's line rather than the helper's. `loc_verbatim` collapsed into `rust_loc`: it had become a one-line wrapper with a single caller. Net -5 lines. `src/tools.rs` grew 767 -> 779 gated SLOC and its baseline entry is refreshed accordingly. Note the file is now 21 lines under its 800 hard cap, and the budget is not fungible -- with `exclude_tests`, a `#[cfg(test)]` fn body costs nothing but its doc comment costs full price. A better end state is to move all five `#[cfg(test)]` helpers out of `tools.rs` into the ignore-listed `tools_tests.rs`, which would drop the file to ~746 and retire the baseline entry entirely; filed separately rather than half-migrating one helper away from its four siblings. The `.bca-baseline.toml` hunk touching `metrics_inner`'s `start_line` is drift from the preceding commit, not from this change: `start_line` is only a +/-50-line disambiguator and the recorded value is unchanged. Three similar call sites in `src/metrics/cyclomatic.rs` were left alone deliberately -- they pass `.with_name(Some("try.rs"))`, which this signature does not express. --- .bca-baseline.toml | 4 ++-- src/metrics/cognitive.rs | 7 +++---- src/metrics/loc.rs | 27 +++++---------------------- src/metrics/tokens.rs | 24 ++++++++---------------- src/tools.rs | 21 +++++++++++++++++++++ 5 files changed, 39 insertions(+), 44 deletions(-) diff --git a/.bca-baseline.toml b/.bca-baseline.toml index ec7ec152..f4d2cdc5 100644 --- a/.bca-baseline.toml +++ b/.bca-baseline.toml @@ -945,7 +945,7 @@ value = 61523.95778223676 [[entry]] path = "src/spaces/compute.rs" qualified = "metrics_inner" -start_line = 421 +start_line = 446 metric = "halstead.effort" value = 142419.45851755128 @@ -961,7 +961,7 @@ path = "src/tools.rs" qualified = "" start_line = 1 metric = "loc.sloc" -value = 767.0 +value = 779.0 [[entry]] path = "src/tools.rs" diff --git a/src/metrics/cognitive.rs b/src/metrics/cognitive.rs index 63d97444..7610efc2 100644 --- a/src/metrics/cognitive.rs +++ b/src/metrics/cognitive.rs @@ -9416,12 +9416,11 @@ end", // dependency, so this narrows the work rather than isolating it. fn cognitive_of(source: &str) -> (u64, std::time::Duration) { let started = std::time::Instant::now(); - let sum = crate::analyze( - crate::Source::new(crate::LANG::C, source.as_bytes()), + let sum = crate::tools::metrics_verbatim( + crate::LANG::C, + source.as_bytes(), crate::MetricsOptions::default().with_only(&[crate::Metric::Cognitive]), ) - .expect("source must analyse") - .metrics .cognitive .cognitive_sum(); (sum, started.elapsed()) diff --git a/src/metrics/loc.rs b/src/metrics/loc.rs index cbd8e1db..43592e42 100644 --- a/src/metrics/loc.rs +++ b/src/metrics/loc.rs @@ -735,7 +735,7 @@ mod typescript; clippy::too_many_lines )] mod tests { - use crate::tools::check_metrics; + use crate::tools::{check_metrics, metrics_verbatim}; use super::*; @@ -9854,29 +9854,12 @@ class A { ); } - /// Analyses `source` **verbatim** in `lang`. + /// Analyses `source` byte-for-byte as Rust. /// - /// `check_metrics` cannot be used for the #1051 cases: it routes through - /// `tools::check_func_space`, which trims trailing newlines and appends - /// one, so a node ending at EOF is unreachable through it and any such - /// test would pass vacuously. `Source` applies no normalisation of its - /// own, so this is the in-tree path that reproduces the bug. - /// - /// Takes `lang` rather than hard-coding one: the untestable-at-EOF class - /// is shared by every `Loc` submodule, not just Rust's. - fn loc_verbatim(lang: crate::LANG, source: &[u8]) -> Stats { - crate::analyze( - crate::Source::new(lang, source), - crate::MetricsOptions::default(), - ) - .expect("verbatim source must analyse") - .metrics - .loc - } - - /// `loc_verbatim` bound to Rust, which is where #1051 lives. + /// Goes through `metrics_verbatim` rather than `check_metrics` + /// because the #1051 cases end at EOF; see that helper for why. fn rust_loc(source: &[u8]) -> Stats { - loc_verbatim(crate::LANG::Rust, source) + metrics_verbatim(crate::LANG::Rust, source, crate::MetricsOptions::default()).loc } /// #1051: a Rust doc comment ending at EOF has no trailing newline for diff --git a/src/metrics/tokens.rs b/src/metrics/tokens.rs index 868d3fba..7075113e 100644 --- a/src/metrics/tokens.rs +++ b/src/metrics/tokens.rs @@ -200,7 +200,7 @@ implement_metric_trait!( clippy::too_many_lines )] mod tests { - use crate::tools::check_metrics; + use crate::tools::{check_metrics, metrics_verbatim}; use super::*; @@ -422,25 +422,17 @@ mod tests { }); } - /// Analyses `source` verbatim and returns its total token count. + /// Total token count for `source`, analysed byte-for-byte. /// - /// Used by the tests below that need to *compare* counts across - /// inputs, which `check_metrics` cannot express: it takes an `F: Fn` - /// that cannot return a value, and it normalises the source before - /// parsing. - /// - /// Restricted to `Tokens` so these tests measure only the metric - /// they guard. With every metric enabled, `cognitive`'s own - /// superlinear nesting lookup (#1062) dominates the deep-nesting - /// case below — a `cognitive` regression would look like a `tokens` - /// one, and a `cognitive` fix would mask a `tokens` regression. + /// Restricted to `Tokens`: with every metric enabled, `cognitive`'s + /// own superlinear nesting lookup (#1062) dominates the deep-nesting + /// case below and would misattribute a regression. fn tokens_of(source: &str) -> u64 { - crate::analyze( - crate::Source::new(crate::LANG::Rust, source.as_bytes()), + metrics_verbatim( + crate::LANG::Rust, + source.as_bytes(), crate::MetricsOptions::default().with_only(&[crate::Metric::Tokens]), ) - .expect("source must analyse") - .metrics .tokens .tokens_sum() } diff --git a/src/tools.rs b/src/tools.rs index 008e6bff..a8387cf0 100644 --- a/src/tools.rs +++ b/src/tools.rs @@ -775,6 +775,27 @@ pub(crate) fn check_metrics( check_func_space::(source, filename, |func_space| check(func_space.metrics)); } +/// Analyses `source` **byte-for-byte** and returns its metrics. +/// +/// Use this, not [`check_metrics`], when a test's input must reach the +/// parser unaltered: `check_func_space` normalises CRLF and trims then +/// re-appends a trailing newline, so a construct ending at EOF is +/// unreachable through it and such a test passes vacuously (#1051). It +/// also returns a value, which `check_metrics`' bare `fn` callback +/// cannot. Restrict `options` in timing-sensitive tests so an unrelated +/// metric's cost cannot dominate and misattribute a regression. +#[cfg(test)] +#[track_caller] +pub(crate) fn metrics_verbatim( + lang: crate::LANG, + source: &[u8], + options: crate::MetricsOptions, +) -> crate::CodeMetrics { + crate::analyze(crate::Source::new(lang, source), options) + .expect("verbatim source must analyse") + .metrics +} + /// Asserts that `func_space` has a direct child space named `name` and that /// its `kind` matches `expected`. /// From b5f4f50ebb4ffa142a6ee5cde453266612216d38 Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Sat, 25 Jul 2026 12:08:26 -0700 Subject: [PATCH 3/7] perf(spaces): hash node ids with FxHash The cognitive nesting map is keyed by tree-sitter node id and probed several times per AST node. `HashMap`'s default SipHash-1-3 exists to resist hash-flooding from untrusted *keys*; these keys are pointer-derived `usize` values we produced ourselves and are never attacker-chosen, so the keyed round buys a property this map cannot use. Replaced with FxHash behind a `NestingMap` alias. The swap is behaviour-neutral by construction: the map is only ever touched by `get` / `insert` / `entry` and is never iterated, so no metric value or ordering can depend on the hasher. Two details that are easy to get wrong, and that a first cut here did get wrong: * `finish` rotates left by 26. A multiplicative hash concentrates entropy in the top bits while hashbrown takes the bucket index from the bottom ones, and node ids are pointer-aligned -- without the rotate every hash ends in at least three zero bits and only one home bucket in eight is reachable. That is cheap on x86-64 (SSE2 probe group of 16) but materially worse on aarch64 (NEON group of 8), which is a shipped release target. `rustc-hash` added this in 2.0; copying the 1.x algorithm reproduces exactly the defect upstream removed. * `write_u64` / `write_u32` / `write_u8` are overridden alongside `write_usize`. Without them a key-type change falls back to `write`'s byte-at-a-time loop -- eight dependent rounds per word, slower than the SipHash this replaces, silently and with no test to catch it. Effect size, stated honestly: an independent replication against a freshly built parent commit measured ~3% off total runtime with a one-sided permutation p = 0.028 and a clean control, so the win is real but modest. The per-metric figure is not quotable to two significant figures -- five interleaved blocks on this workload put the cognitive-minus-nom delta anywhere between 637 ms and 1948 ms, drifting downward run over run, and a bootstrap CI on the derived percentage spans roughly 7-41%. Corpus, corrected: `tests/repositories/DeepSpeech` is 25022 files -- 6104 .cc, 3517 .h, 2862 .py -- so roughly 78% C/C++, not the Python corpus an earlier draft of this message claimed. On owning the algorithm rather than depending on one: `foldhash` 0.1.5 and 0.2.0 are already in `Cargo.lock` via `hashbrown` and `actix`, so promoting one to a direct dependency would add no new crate, license or MSRV surface. That is a fair argument for doing so, and the defect above is a fair argument that owning a copy means owning its bug list. Kept in-tree for now at twenty lines; revisit if it needs a third fix. --- src/macros/mod.rs | 2 +- src/metrics/cognitive.rs | 11 ++--- src/metrics/cognitive/bash.rs | 2 +- src/metrics/cognitive/c.rs | 2 +- src/metrics/cognitive/cpp.rs | 2 +- src/metrics/cognitive/csharp.rs | 2 +- src/metrics/cognitive/elixir.rs | 2 +- src/metrics/cognitive/go.rs | 2 +- src/metrics/cognitive/groovy.rs | 2 +- src/metrics/cognitive/irules.rs | 2 +- src/metrics/cognitive/java.rs | 2 +- src/metrics/cognitive/kotlin.rs | 2 +- src/metrics/cognitive/lua.rs | 2 +- src/metrics/cognitive/mozcpp.rs | 2 +- src/metrics/cognitive/objc.rs | 2 +- src/metrics/cognitive/perl.rs | 2 +- src/metrics/cognitive/php.rs | 2 +- src/metrics/cognitive/python.rs | 4 +- src/metrics/cognitive/ruby.rs | 2 +- src/metrics/cognitive/rust.rs | 2 +- src/metrics/cognitive/tcl.rs | 2 +- src/spaces.rs | 82 +++++++++++++++++++++++++++++++++ src/spaces/compute.rs | 6 +-- 23 files changed, 110 insertions(+), 31 deletions(-) diff --git a/src/macros/mod.rs b/src/macros/mod.rs index 76a05d5f..f5990ff8 100644 --- a/src/macros/mod.rs +++ b/src/macros/mod.rs @@ -46,7 +46,7 @@ macro_rules! implement_metric_trait { _node: &Node<'a>, _code: &'a [u8], _stats: &mut Stats, - _nesting_map: &mut HashMap, + _nesting_map: &mut crate::spaces::NestingMap, ) {} } )+ diff --git a/src/metrics/cognitive.rs b/src/metrics/cognitive.rs index 7610efc2..91cb791a 100644 --- a/src/metrics/cognitive.rs +++ b/src/metrics/cognitive.rs @@ -22,7 +22,7 @@ clippy::cast_sign_loss )] -use std::collections::HashMap; +use crate::spaces::NestingMap; use std::fmt; @@ -169,7 +169,7 @@ where node: &Node<'a>, code: &'a [u8], stats: &mut Stats, - nesting_map: &mut HashMap, + nesting_map: &mut NestingMap, ); } @@ -260,10 +260,7 @@ fn increment_branch_extension(stats: &mut Stats) { /// `spaces::compute`). Reading `node.parent()` here instead would cost /// `O(depth)` per node — `Node::parent` walks down from the root — which /// made this metric quadratic in nesting depth (#1062). -fn get_nesting_from_map( - node: &Node, - nesting_map: &HashMap, -) -> (usize, usize, usize) { +fn get_nesting_from_map(node: &Node, nesting_map: &NestingMap) -> (usize, usize, usize) { nesting_map.get(&node.id()).copied().unwrap_or((0, 0, 0)) } @@ -310,7 +307,7 @@ macro_rules! js_cognitive { node: &Node<'a>, _code: &'a [u8], stats: &mut Stats, - nesting_map: &mut HashMap, + nesting_map: &mut NestingMap, ) { use $lang::*; let (mut nesting, mut depth, mut lambda) = get_nesting_from_map(node, nesting_map); diff --git a/src/metrics/cognitive/bash.rs b/src/metrics/cognitive/bash.rs index 1f97b220..b8cca597 100644 --- a/src/metrics/cognitive/bash.rs +++ b/src/metrics/cognitive/bash.rs @@ -18,7 +18,7 @@ impl Cognitive for BashCode { node: &Node<'a>, _code: &'a [u8], stats: &mut Stats, - nesting_map: &mut HashMap, + nesting_map: &mut NestingMap, ) { use Bash::*; diff --git a/src/metrics/cognitive/c.rs b/src/metrics/cognitive/c.rs index 4d782871..e29403bc 100644 --- a/src/metrics/cognitive/c.rs +++ b/src/metrics/cognitive/c.rs @@ -18,7 +18,7 @@ impl Cognitive for CCode { node: &Node<'a>, _code: &'a [u8], stats: &mut Stats, - nesting_map: &mut HashMap, + nesting_map: &mut NestingMap, ) { use C::*; diff --git a/src/metrics/cognitive/cpp.rs b/src/metrics/cognitive/cpp.rs index 03633f95..f8bd2f99 100644 --- a/src/metrics/cognitive/cpp.rs +++ b/src/metrics/cognitive/cpp.rs @@ -18,7 +18,7 @@ impl Cognitive for CppCode { node: &Node<'a>, _code: &'a [u8], stats: &mut Stats, - nesting_map: &mut HashMap, + nesting_map: &mut NestingMap, ) { use Cpp::*; diff --git a/src/metrics/cognitive/csharp.rs b/src/metrics/cognitive/csharp.rs index e1361d0d..5c117945 100644 --- a/src/metrics/cognitive/csharp.rs +++ b/src/metrics/cognitive/csharp.rs @@ -18,7 +18,7 @@ impl Cognitive for CsharpCode { node: &Node<'a>, _code: &'a [u8], stats: &mut Stats, - nesting_map: &mut HashMap, + nesting_map: &mut NestingMap, ) { use Csharp::*; diff --git a/src/metrics/cognitive/elixir.rs b/src/metrics/cognitive/elixir.rs index 76049a0b..1ef1e2cb 100644 --- a/src/metrics/cognitive/elixir.rs +++ b/src/metrics/cognitive/elixir.rs @@ -65,7 +65,7 @@ impl Cognitive for ElixirCode { node: &Node<'a>, code: &'a [u8], stats: &mut Stats, - nesting_map: &mut HashMap, + nesting_map: &mut NestingMap, ) { use Elixir as E; diff --git a/src/metrics/cognitive/go.rs b/src/metrics/cognitive/go.rs index 37be1b95..5233fe2d 100644 --- a/src/metrics/cognitive/go.rs +++ b/src/metrics/cognitive/go.rs @@ -18,7 +18,7 @@ impl Cognitive for GoCode { node: &Node<'a>, _code: &'a [u8], stats: &mut Stats, - nesting_map: &mut HashMap, + nesting_map: &mut NestingMap, ) { use Go as G; diff --git a/src/metrics/cognitive/groovy.rs b/src/metrics/cognitive/groovy.rs index 697166bb..741e2973 100644 --- a/src/metrics/cognitive/groovy.rs +++ b/src/metrics/cognitive/groovy.rs @@ -18,7 +18,7 @@ impl Cognitive for GroovyCode { node: &Node<'a>, _code: &'a [u8], stats: &mut Stats, - nesting_map: &mut HashMap, + nesting_map: &mut NestingMap, ) { use Groovy::*; diff --git a/src/metrics/cognitive/irules.rs b/src/metrics/cognitive/irules.rs index 557ef366..d852473a 100644 --- a/src/metrics/cognitive/irules.rs +++ b/src/metrics/cognitive/irules.rs @@ -18,7 +18,7 @@ impl Cognitive for IrulesCode { node: &Node<'a>, _code: &'a [u8], stats: &mut Stats, - nesting_map: &mut HashMap, + nesting_map: &mut NestingMap, ) { use Irules::*; diff --git a/src/metrics/cognitive/java.rs b/src/metrics/cognitive/java.rs index a0260808..553b973c 100644 --- a/src/metrics/cognitive/java.rs +++ b/src/metrics/cognitive/java.rs @@ -18,7 +18,7 @@ impl Cognitive for JavaCode { node: &Node<'a>, _code: &'a [u8], stats: &mut Stats, - nesting_map: &mut HashMap, + nesting_map: &mut NestingMap, ) { use Java::*; diff --git a/src/metrics/cognitive/kotlin.rs b/src/metrics/cognitive/kotlin.rs index 24aed37d..163e4feb 100644 --- a/src/metrics/cognitive/kotlin.rs +++ b/src/metrics/cognitive/kotlin.rs @@ -37,7 +37,7 @@ impl Cognitive for KotlinCode { node: &Node<'a>, code: &'a [u8], stats: &mut Stats, - nesting_map: &mut HashMap, + nesting_map: &mut NestingMap, ) { use Kotlin::*; diff --git a/src/metrics/cognitive/lua.rs b/src/metrics/cognitive/lua.rs index cf1841f5..d6cc240c 100644 --- a/src/metrics/cognitive/lua.rs +++ b/src/metrics/cognitive/lua.rs @@ -18,7 +18,7 @@ impl Cognitive for LuaCode { node: &Node<'a>, _code: &'a [u8], stats: &mut Stats, - nesting_map: &mut HashMap, + nesting_map: &mut NestingMap, ) { use Lua::*; diff --git a/src/metrics/cognitive/mozcpp.rs b/src/metrics/cognitive/mozcpp.rs index 8d4c0e97..9e3767b6 100644 --- a/src/metrics/cognitive/mozcpp.rs +++ b/src/metrics/cognitive/mozcpp.rs @@ -18,7 +18,7 @@ impl Cognitive for MozcppCode { node: &Node<'a>, _code: &'a [u8], stats: &mut Stats, - nesting_map: &mut HashMap, + nesting_map: &mut NestingMap, ) { use Mozcpp::*; diff --git a/src/metrics/cognitive/objc.rs b/src/metrics/cognitive/objc.rs index 1c0a515c..3801bdc1 100644 --- a/src/metrics/cognitive/objc.rs +++ b/src/metrics/cognitive/objc.rs @@ -18,7 +18,7 @@ impl Cognitive for ObjcCode { node: &Node<'a>, _code: &'a [u8], stats: &mut Stats, - nesting_map: &mut HashMap, + nesting_map: &mut NestingMap, ) { use Objc::*; diff --git a/src/metrics/cognitive/perl.rs b/src/metrics/cognitive/perl.rs index d14bd82b..09f3611d 100644 --- a/src/metrics/cognitive/perl.rs +++ b/src/metrics/cognitive/perl.rs @@ -40,7 +40,7 @@ impl Cognitive for PerlCode { node: &Node<'a>, _code: &'a [u8], stats: &mut Stats, - nesting_map: &mut HashMap, + nesting_map: &mut NestingMap, ) { use Perl as P; diff --git a/src/metrics/cognitive/php.rs b/src/metrics/cognitive/php.rs index cd930067..f23d9659 100644 --- a/src/metrics/cognitive/php.rs +++ b/src/metrics/cognitive/php.rs @@ -18,7 +18,7 @@ impl Cognitive for PhpCode { node: &Node<'a>, _code: &'a [u8], stats: &mut Stats, - nesting_map: &mut HashMap, + nesting_map: &mut NestingMap, ) { use Php::*; diff --git a/src/metrics/cognitive/python.rs b/src/metrics/cognitive/python.rs index 73d7147a..9aee653a 100644 --- a/src/metrics/cognitive/python.rs +++ b/src/metrics/cognitive/python.rs @@ -37,7 +37,7 @@ fn python_comprehension_clause_nesting( nesting: usize, depth: usize, lambda: usize, - nesting_map: &mut HashMap, + nesting_map: &mut NestingMap, ) -> usize { use Python::*; let mut for_count = 0; @@ -87,7 +87,7 @@ impl Cognitive for PythonCode { node: &Node<'a>, _code: &'a [u8], stats: &mut Stats, - nesting_map: &mut HashMap, + nesting_map: &mut NestingMap, ) { use Python::*; diff --git a/src/metrics/cognitive/ruby.rs b/src/metrics/cognitive/ruby.rs index fd699c73..5a0df723 100644 --- a/src/metrics/cognitive/ruby.rs +++ b/src/metrics/cognitive/ruby.rs @@ -30,7 +30,7 @@ impl Cognitive for RubyCode { node: &Node<'a>, _code: &'a [u8], stats: &mut Stats, - nesting_map: &mut HashMap, + nesting_map: &mut NestingMap, ) { use Ruby as R; diff --git a/src/metrics/cognitive/rust.rs b/src/metrics/cognitive/rust.rs index 9bbc41d0..595007de 100644 --- a/src/metrics/cognitive/rust.rs +++ b/src/metrics/cognitive/rust.rs @@ -18,7 +18,7 @@ impl Cognitive for RustCode { node: &Node<'a>, _code: &'a [u8], stats: &mut Stats, - nesting_map: &mut HashMap, + nesting_map: &mut NestingMap, ) { use Rust::*; // Macro expansion is not tracked; macros are treated as opaque tokens. diff --git a/src/metrics/cognitive/tcl.rs b/src/metrics/cognitive/tcl.rs index bf12ecfe..f2a491e0 100644 --- a/src/metrics/cognitive/tcl.rs +++ b/src/metrics/cognitive/tcl.rs @@ -18,7 +18,7 @@ impl Cognitive for TclCode { node: &Node<'a>, code: &'a [u8], stats: &mut Stats, - nesting_map: &mut HashMap, + nesting_map: &mut NestingMap, ) { use Tcl::*; diff --git a/src/spaces.rs b/src/spaces.rs index 7f7975e9..8f7bcb5a 100644 --- a/src/spaces.rs +++ b/src/spaces.rs @@ -19,12 +19,94 @@ use std::borrow::Cow; use std::collections::HashMap; +use std::hash::{BuildHasherDefault, Hasher}; use serde::{Deserialize, Serialize}; use std::fmt; use std::path::Path; use std::sync::Arc; +/// Hasher for the node-id keyed maps the walker threads through the +/// metric traits. +/// +/// The keys are tree-sitter node ids: pointer-derived `usize` values we +/// produced ourselves, never attacker-chosen. The default SipHash-1-3 +/// exists to resist hash-flooding from untrusted *keys*, so on this map +/// it buys nothing and costs a full keyed round per probe — on a path +/// that runs several times per AST node. +/// +/// This is FxHash as of `rustc-hash` 2.x, including the `finish` rotate +/// that 2.0 added. It is **not** collision-resistant and must not be +/// used for any map whose keys come from user input. +#[derive(Default)] +pub(crate) struct NodeIdHasher { + hash: u64, +} + +impl NodeIdHasher { + /// FxHash's multiplier. Being *odd* is the load-bearing property: it + /// makes `n * SEED (mod 2^64)` a bijection, so distinct node ids can + /// never produce the same hash. + const SEED: u64 = 0x51_7c_c1_b7_27_22_0a_95; + + /// Rotate applied in `finish`. A multiplicative hash concentrates + /// entropy in the *top* bits, but hashbrown takes the bucket index + /// from the *bottom* ones. Node ids are pointer-aligned, so without + /// this every hash would end in at least three zero bits and only + /// one home bucket in eight would be reachable — cheap on x86-64, + /// where the SSE2 probe group is 16 wide, but measurably worse on + /// aarch64, whose NEON group is 8. Value matches `rustc-hash` 2.x. + const FINISH_ROTATE: u32 = 26; + + #[inline] + fn add(&mut self, word: u64) { + self.hash = (self.hash.rotate_left(5) ^ word).wrapping_mul(Self::SEED); + } +} + +impl Hasher for NodeIdHasher { + #[inline] + fn write(&mut self, bytes: &[u8]) { + for byte in bytes { + self.add(u64::from(*byte)); + } + } + + // The integer writes are overridden so a key type change cannot + // silently fall back to `write`'s byte-at-a-time loop, which would + // cost eight dependent rounds per word and be slower than the + // SipHash this replaced. + #[inline] + fn write_usize(&mut self, n: usize) { + self.add(n as u64); + } + + #[inline] + fn write_u64(&mut self, n: u64) { + self.add(n); + } + + #[inline] + fn write_u32(&mut self, n: u32) { + self.add(u64::from(n)); + } + + #[inline] + fn write_u8(&mut self, n: u8) { + self.add(u64::from(n)); + } + + #[inline] + fn finish(&self) -> u64 { + self.hash.rotate_left(Self::FINISH_ROTATE) + } +} + +/// Per-node `(nesting, depth, lambda)` state that `Cognitive` inherits +/// down the walk; see `spaces::compute::propagate_nesting_to_children`. +pub(crate) type NestingMap = + HashMap>; + use crate::langs::LANG; use crate::metric_set::{Metric, MetricSet}; use crate::preproc::PreprocResults; diff --git a/src/spaces/compute.rs b/src/spaces/compute.rs index bea6ebd7..77c8781b 100644 --- a/src/spaces/compute.rs +++ b/src/spaces/compute.rs @@ -221,7 +221,7 @@ fn compute_per_node<'a, T: ParserTrait>( code: &'a [u8], options: MetricsOptions, facts: NodeFacts, - nesting_map: &mut HashMap, + nesting_map: &mut NestingMap, ) { let NodeFacts { func_space, @@ -390,7 +390,7 @@ struct Walk { fn propagate_nesting_to_children( node: &Node, children: &[(Node<'_>, Walk)], - nesting_map: &mut HashMap, + nesting_map: &mut NestingMap, ) { // Leaves are roughly half of a real AST, so bail before hashing a key // we would only read to iterate zero children. @@ -470,7 +470,7 @@ pub(crate) fn metrics_inner( let mut last_level = 0; // Initialize nesting_map used for storing nesting information for cognitive // Three type of nesting info: conditionals, functions and lambdas - let mut nesting_map = HashMap::::default(); + let mut nesting_map = NestingMap::default(); nesting_map.insert(node.id(), (0, 0, 0)); // Suppression markers are resolved inline during the walk rather From b67312dedef05189880bee775bba2a1e7f531e05 Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Sat, 25 Jul 2026 14:47:09 -0700 Subject: [PATCH 4/7] refactor(cognitive): name the nesting triple The nesting map's value was `(usize, usize, usize)` destructured positionally in 20 language modules and rebuilt positionally in 22 places, against AGENTS.md's "do not pass two same-typed primitives where they could be confused". It is now a `Nesting` struct with `conditional` / `function_depth` / `lambda` fields. Only the boundary changed. The locals keep their names and the arm bodies are untouched, because those names appear 8-36 times per file and renaming them would be churn with no benefit; what matters is that the read and the write now say which field is which. `function_depth` and `lambda` are separable, and the new `javascript_function_depth_and_lambda_are_distinguishable` guards it. Getting there took a wrong turn worth recording: transposing the two at the write site left the whole suite green, and five hand-built shapes (`fn>arrow>fn>if`, `arrow>fn>if`, `fn>arrow>if`, `arrow>arrow>if`, `fn>fn>if`) all measured identically, so an earlier draft of this commit concluded they were interchangeable and said no test could ever catch a mix-up. That was wrong. The two are summed symmetrically almost everywhere, so a swap preserves the total. The single asymmetric operation in the whole cognitive family is the JS `FunctionDeclaration` arm's `lambda = 0`. Whether it separates them depends on the *parity* of the ancestor chain between the innermost arrow and the `function_declaration`, because a write-site swap transposes the pair at every node on the way down. Plain `arrow -> statement_block -> function_declaration` is odd parity and cancels; a second chained arrow flips it. Every one of those five shapes happened to sit on the cancelling side. const f = () => () => { function inner() { if (a) { } } }; measures 1 normally and 2 with the fields transposed, so it is a real guard, verified by revert. `javascript_arrow_contributes_lambda_nesting` pins the `ArrowFunction` arm separately, since the shape above resets lambda before the measurement and so cannot see that arm at all. Also drops the stale positional wording from `get_nesting_from_map`'s doc and the "triple" references in `spaces::compute`, which otherwise re-taught the naming this commit removes. --- src/metrics/cognitive.rs | 63 +++++++++++++++++++++++++++++---- src/metrics/cognitive/bash.rs | 15 ++++++-- src/metrics/cognitive/c.rs | 15 ++++++-- src/metrics/cognitive/cpp.rs | 15 ++++++-- src/metrics/cognitive/csharp.rs | 15 ++++++-- src/metrics/cognitive/elixir.rs | 15 ++++++-- src/metrics/cognitive/go.rs | 15 ++++++-- src/metrics/cognitive/groovy.rs | 15 ++++++-- src/metrics/cognitive/irules.rs | 15 ++++++-- src/metrics/cognitive/java.rs | 15 ++++++-- src/metrics/cognitive/kotlin.rs | 15 ++++++-- src/metrics/cognitive/lua.rs | 15 ++++++-- src/metrics/cognitive/mozcpp.rs | 15 ++++++-- src/metrics/cognitive/objc.rs | 15 ++++++-- src/metrics/cognitive/perl.rs | 15 ++++++-- src/metrics/cognitive/php.rs | 15 ++++++-- src/metrics/cognitive/python.rs | 33 ++++++++++++++--- src/metrics/cognitive/ruby.rs | 15 ++++++-- src/metrics/cognitive/rust.rs | 15 ++++++-- src/metrics/cognitive/tcl.rs | 15 ++++++-- src/spaces.rs | 25 ++++++++++--- src/spaces/compute.rs | 12 +++---- 22 files changed, 346 insertions(+), 57 deletions(-) diff --git a/src/metrics/cognitive.rs b/src/metrics/cognitive.rs index 91cb791a..ff26e90d 100644 --- a/src/metrics/cognitive.rs +++ b/src/metrics/cognitive.rs @@ -22,7 +22,7 @@ clippy::cast_sign_loss )] -use crate::spaces::NestingMap; +use crate::spaces::{Nesting, NestingMap}; use std::fmt; @@ -251,8 +251,7 @@ fn increment_branch_extension(stats: &mut Stats) { stats.boolean_seq.reset(); } -/// Returns the `(nesting, depth, lambda)` triple `node` inherits from its -/// parent. +/// Returns the [`Nesting`] `node` inherits from its parent. /// /// The map is keyed so that a node's own slot holds what it *inherits*: /// the walker seeds each child's slot from its parent's slot after the @@ -260,8 +259,8 @@ fn increment_branch_extension(stats: &mut Stats) { /// `spaces::compute`). Reading `node.parent()` here instead would cost /// `O(depth)` per node — `Node::parent` walks down from the root — which /// made this metric quadratic in nesting depth (#1062). -fn get_nesting_from_map(node: &Node, nesting_map: &NestingMap) -> (usize, usize, usize) { - nesting_map.get(&node.id()).copied().unwrap_or((0, 0, 0)) +fn get_nesting_from_map(node: &Node, nesting_map: &NestingMap) -> Nesting { + nesting_map.get(&node.id()).copied().unwrap_or_default() } fn increment_function_depth>(depth: &mut usize, node: &Node, stops: &[T]) { @@ -310,7 +309,11 @@ macro_rules! js_cognitive { nesting_map: &mut NestingMap, ) { use $lang::*; - let (mut nesting, mut depth, mut lambda) = get_nesting_from_map(node, nesting_map); + let Nesting { + conditional: mut nesting, + function_depth: mut depth, + mut lambda, + } = get_nesting_from_map(node, nesting_map); match node.kind_id().into() { IfStatement if !Self::is_else_if(node) => { @@ -366,7 +369,10 @@ macro_rules! js_cognitive { } _ => {} } - nesting_map.insert(node.id(), (nesting, depth, lambda)); + nesting_map.insert( + node.id(), + Nesting { conditional: nesting, function_depth: depth, lambda }, + ); } }; } @@ -9385,6 +9391,49 @@ end", ); } + /// Pins that `function_depth` and `lambda` are distinguishable. + /// + /// They are summed symmetrically almost everywhere, so most inputs + /// cannot tell them apart. The one asymmetric operation in the whole + /// cognitive family is the JS `FunctionDeclaration` arm's + /// `lambda = 0`, which clears one field alone. + /// + /// Whether that separates them depends on the *parity* of the + /// ancestor chain between the innermost arrow and the + /// `function_declaration`, because a swap at the write site + /// transposes the pair at every node on the way down. The plain + /// `arrow -> statement_block -> function_declaration` chain has odd + /// parity and stays at the same total either way; a second chained + /// arrow flips it. Hence the doubled arrow here — without it this + /// test passes with the two fields transposed and guards nothing. + #[test] + fn javascript_function_depth_and_lambda_are_distinguishable() { + // expected: the inner `function` resets lambda, so only its own + // function-depth level survives and the `if` costs 1. + check_metrics::( + "const f = () => () => { function inner() { if (a) { } } };", + "nest.js", + |metric| { + assert_eq!(metric.cognitive.cognitive_sum(), 1); + }, + ); + } + + /// The `ArrowFunction` arm's own `lambda += 1`, pinned separately: + /// with no `function_declaration` between the arrow and the `if`, + /// nothing resets lambda, so the arrow's level reaches the `if`. + #[test] + fn javascript_arrow_contributes_lambda_nesting() { + // expected: 1 for the `if`, +1 for the enclosing arrow level. + check_metrics::( + "const f = () => { if (a) { } };", + "arrow.js", + |metric| { + assert_eq!(metric.cognitive.cognitive_sum(), 2); + }, + ); + } + /// Deeply nested input must stay tractable (#1062). /// /// `get_nesting_from_map` used to recover a node's inherited nesting diff --git a/src/metrics/cognitive/bash.rs b/src/metrics/cognitive/bash.rs index b8cca597..8dc0b457 100644 --- a/src/metrics/cognitive/bash.rs +++ b/src/metrics/cognitive/bash.rs @@ -22,7 +22,11 @@ impl Cognitive for BashCode { ) { use Bash::*; - let (mut nesting, mut depth, lambda) = get_nesting_from_map(node, nesting_map); + let Nesting { + conditional: mut nesting, + function_depth: mut depth, + lambda, + } = get_nesting_from_map(node, nesting_map); match node.kind_id().into() { // `WhileStatement` covers both `while` and `until`; `ForStatement` @@ -51,6 +55,13 @@ impl Cognitive for BashCode { } _ => {} } - nesting_map.insert(node.id(), (nesting, depth, lambda)); + nesting_map.insert( + node.id(), + Nesting { + conditional: nesting, + function_depth: depth, + lambda, + }, + ); } } diff --git a/src/metrics/cognitive/c.rs b/src/metrics/cognitive/c.rs index e29403bc..7a7f0275 100644 --- a/src/metrics/cognitive/c.rs +++ b/src/metrics/cognitive/c.rs @@ -23,7 +23,11 @@ impl Cognitive for CCode { use C::*; // Macro expansion is not tracked; macros are treated as opaque tokens. - let (mut nesting, mut depth, lambda) = get_nesting_from_map(node, nesting_map); + let Nesting { + conditional: mut nesting, + function_depth: mut depth, + lambda, + } = get_nesting_from_map(node, nesting_map); match node.kind_id().into() { IfStatement if !Self::is_else_if(node) => { @@ -60,6 +64,13 @@ impl Cognitive for CCode { } _ => {} } - nesting_map.insert(node.id(), (nesting, depth, lambda)); + nesting_map.insert( + node.id(), + Nesting { + conditional: nesting, + function_depth: depth, + lambda, + }, + ); } } diff --git a/src/metrics/cognitive/cpp.rs b/src/metrics/cognitive/cpp.rs index f8bd2f99..f86d9b54 100644 --- a/src/metrics/cognitive/cpp.rs +++ b/src/metrics/cognitive/cpp.rs @@ -23,7 +23,11 @@ impl Cognitive for CppCode { use Cpp::*; // Macro expansion is not tracked; macros are treated as opaque tokens. - let (mut nesting, mut depth, mut lambda) = get_nesting_from_map(node, nesting_map); + let Nesting { + conditional: mut nesting, + function_depth: mut depth, + mut lambda, + } = get_nesting_from_map(node, nesting_map); match node.kind_id().into() { IfStatement if !Self::is_else_if(node) => { @@ -70,6 +74,13 @@ impl Cognitive for CppCode { } _ => {} } - nesting_map.insert(node.id(), (nesting, depth, lambda)); + nesting_map.insert( + node.id(), + Nesting { + conditional: nesting, + function_depth: depth, + lambda, + }, + ); } } diff --git a/src/metrics/cognitive/csharp.rs b/src/metrics/cognitive/csharp.rs index 5c117945..67d0bc22 100644 --- a/src/metrics/cognitive/csharp.rs +++ b/src/metrics/cognitive/csharp.rs @@ -22,7 +22,11 @@ impl Cognitive for CsharpCode { ) { use Csharp::*; - let (mut nesting, mut depth, mut lambda) = get_nesting_from_map(node, nesting_map); + let Nesting { + conditional: mut nesting, + function_depth: mut depth, + mut lambda, + } = get_nesting_from_map(node, nesting_map); match node.kind_id().into() { IfStatement if !Self::is_else_if(node) => { @@ -109,6 +113,13 @@ impl Cognitive for CsharpCode { } _ => {} } - nesting_map.insert(node.id(), (nesting, depth, lambda)); + nesting_map.insert( + node.id(), + Nesting { + conditional: nesting, + function_depth: depth, + lambda, + }, + ); } } diff --git a/src/metrics/cognitive/elixir.rs b/src/metrics/cognitive/elixir.rs index 1ef1e2cb..8d1c9276 100644 --- a/src/metrics/cognitive/elixir.rs +++ b/src/metrics/cognitive/elixir.rs @@ -69,7 +69,11 @@ impl Cognitive for ElixirCode { ) { use Elixir as E; - let (mut nesting, depth, mut lambda) = get_nesting_from_map(node, nesting_map); + let Nesting { + conditional: mut nesting, + function_depth: depth, + mut lambda, + } = get_nesting_from_map(node, nesting_map); match node.kind_id().into() { E::Call => match elixir_call_keyword(node, code) { @@ -129,6 +133,13 @@ impl Cognitive for ElixirCode { } _ => {} } - nesting_map.insert(node.id(), (nesting, depth, lambda)); + nesting_map.insert( + node.id(), + Nesting { + conditional: nesting, + function_depth: depth, + lambda, + }, + ); } } diff --git a/src/metrics/cognitive/go.rs b/src/metrics/cognitive/go.rs index 5233fe2d..1e0b6341 100644 --- a/src/metrics/cognitive/go.rs +++ b/src/metrics/cognitive/go.rs @@ -22,7 +22,11 @@ impl Cognitive for GoCode { ) { use Go as G; - let (mut nesting, mut depth, mut lambda) = get_nesting_from_map(node, nesting_map); + let Nesting { + conditional: mut nesting, + function_depth: mut depth, + mut lambda, + } = get_nesting_from_map(node, nesting_map); match node.kind_id().into() { G::IfStatement if !Self::is_else_if(node) => { @@ -56,6 +60,13 @@ impl Cognitive for GoCode { } _ => {} } - nesting_map.insert(node.id(), (nesting, depth, lambda)); + nesting_map.insert( + node.id(), + Nesting { + conditional: nesting, + function_depth: depth, + lambda, + }, + ); } } diff --git a/src/metrics/cognitive/groovy.rs b/src/metrics/cognitive/groovy.rs index 741e2973..e380e011 100644 --- a/src/metrics/cognitive/groovy.rs +++ b/src/metrics/cognitive/groovy.rs @@ -22,7 +22,11 @@ impl Cognitive for GroovyCode { ) { use Groovy::*; - let (mut nesting, mut depth, mut lambda) = get_nesting_from_map(node, nesting_map); + let Nesting { + conditional: mut nesting, + function_depth: mut depth, + mut lambda, + } = get_nesting_from_map(node, nesting_map); match node.kind_id().into() { IfStatement if !Self::is_else_if(node) => { @@ -89,6 +93,13 @@ impl Cognitive for GroovyCode { } _ => {} } - nesting_map.insert(node.id(), (nesting, depth, lambda)); + nesting_map.insert( + node.id(), + Nesting { + conditional: nesting, + function_depth: depth, + lambda, + }, + ); } } diff --git a/src/metrics/cognitive/irules.rs b/src/metrics/cognitive/irules.rs index d852473a..da8a66f0 100644 --- a/src/metrics/cognitive/irules.rs +++ b/src/metrics/cognitive/irules.rs @@ -22,7 +22,11 @@ impl Cognitive for IrulesCode { ) { use Irules::*; - let (mut nesting, mut depth, lambda) = get_nesting_from_map(node, nesting_map); + let Nesting { + conditional: mut nesting, + function_depth: mut depth, + lambda, + } = get_nesting_from_map(node, nesting_map); match node.kind_id().into() { // Defensive guard for parity with sibling impls; iRules' dedicated @@ -72,6 +76,13 @@ impl Cognitive for IrulesCode { } _ => {} } - nesting_map.insert(node.id(), (nesting, depth, lambda)); + nesting_map.insert( + node.id(), + Nesting { + conditional: nesting, + function_depth: depth, + lambda, + }, + ); } } diff --git a/src/metrics/cognitive/java.rs b/src/metrics/cognitive/java.rs index 553b973c..327b29c8 100644 --- a/src/metrics/cognitive/java.rs +++ b/src/metrics/cognitive/java.rs @@ -22,7 +22,11 @@ impl Cognitive for JavaCode { ) { use Java::*; - let (mut nesting, mut depth, mut lambda) = get_nesting_from_map(node, nesting_map); + let Nesting { + conditional: mut nesting, + function_depth: mut depth, + mut lambda, + } = get_nesting_from_map(node, nesting_map); match node.kind_id().into() { IfStatement if !Self::is_else_if(node) => { @@ -72,6 +76,13 @@ impl Cognitive for JavaCode { } _ => {} } - nesting_map.insert(node.id(), (nesting, depth, lambda)); + nesting_map.insert( + node.id(), + Nesting { + conditional: nesting, + function_depth: depth, + lambda, + }, + ); } } diff --git a/src/metrics/cognitive/kotlin.rs b/src/metrics/cognitive/kotlin.rs index 163e4feb..b5c260a5 100644 --- a/src/metrics/cognitive/kotlin.rs +++ b/src/metrics/cognitive/kotlin.rs @@ -41,7 +41,11 @@ impl Cognitive for KotlinCode { ) { use Kotlin::*; - let (mut nesting, mut depth, mut lambda) = get_nesting_from_map(node, nesting_map); + let Nesting { + conditional: mut nesting, + function_depth: mut depth, + mut lambda, + } = get_nesting_from_map(node, nesting_map); match node.kind_id().into() { IfExpression if !Self::is_else_if(node) => { @@ -88,6 +92,13 @@ impl Cognitive for KotlinCode { } _ => {} } - nesting_map.insert(node.id(), (nesting, depth, lambda)); + nesting_map.insert( + node.id(), + Nesting { + conditional: nesting, + function_depth: depth, + lambda, + }, + ); } } diff --git a/src/metrics/cognitive/lua.rs b/src/metrics/cognitive/lua.rs index d6cc240c..083c5723 100644 --- a/src/metrics/cognitive/lua.rs +++ b/src/metrics/cognitive/lua.rs @@ -22,7 +22,11 @@ impl Cognitive for LuaCode { ) { use Lua::*; - let (mut nesting, mut depth, mut lambda) = get_nesting_from_map(node, nesting_map); + let Nesting { + conditional: mut nesting, + function_depth: mut depth, + mut lambda, + } = get_nesting_from_map(node, nesting_map); match node.kind_id().into() { // `is_else_if` returns true for `ElseifStatement`, but Lua's @@ -68,6 +72,13 @@ impl Cognitive for LuaCode { } _ => {} } - nesting_map.insert(node.id(), (nesting, depth, lambda)); + nesting_map.insert( + node.id(), + Nesting { + conditional: nesting, + function_depth: depth, + lambda, + }, + ); } } diff --git a/src/metrics/cognitive/mozcpp.rs b/src/metrics/cognitive/mozcpp.rs index 9e3767b6..b8967c0d 100644 --- a/src/metrics/cognitive/mozcpp.rs +++ b/src/metrics/cognitive/mozcpp.rs @@ -23,7 +23,11 @@ impl Cognitive for MozcppCode { use Mozcpp::*; // Macro expansion is not tracked; macros are treated as opaque tokens. - let (mut nesting, mut depth, mut lambda) = get_nesting_from_map(node, nesting_map); + let Nesting { + conditional: mut nesting, + function_depth: mut depth, + mut lambda, + } = get_nesting_from_map(node, nesting_map); match node.kind_id().into() { IfStatement if !Self::is_else_if(node) => { @@ -70,6 +74,13 @@ impl Cognitive for MozcppCode { } _ => {} } - nesting_map.insert(node.id(), (nesting, depth, lambda)); + nesting_map.insert( + node.id(), + Nesting { + conditional: nesting, + function_depth: depth, + lambda, + }, + ); } } diff --git a/src/metrics/cognitive/objc.rs b/src/metrics/cognitive/objc.rs index 3801bdc1..39a6343b 100644 --- a/src/metrics/cognitive/objc.rs +++ b/src/metrics/cognitive/objc.rs @@ -23,7 +23,11 @@ impl Cognitive for ObjcCode { use Objc::*; // Macro expansion is not tracked; macros are treated as opaque tokens. - let (mut nesting, mut depth, mut lambda) = get_nesting_from_map(node, nesting_map); + let Nesting { + conditional: mut nesting, + function_depth: mut depth, + mut lambda, + } = get_nesting_from_map(node, nesting_map); match node.kind_id().into() { IfStatement if !Self::is_else_if(node) => { @@ -67,6 +71,13 @@ impl Cognitive for ObjcCode { } _ => {} } - nesting_map.insert(node.id(), (nesting, depth, lambda)); + nesting_map.insert( + node.id(), + Nesting { + conditional: nesting, + function_depth: depth, + lambda, + }, + ); } } diff --git a/src/metrics/cognitive/perl.rs b/src/metrics/cognitive/perl.rs index 09f3611d..0bec1ec8 100644 --- a/src/metrics/cognitive/perl.rs +++ b/src/metrics/cognitive/perl.rs @@ -44,7 +44,11 @@ impl Cognitive for PerlCode { ) { use Perl as P; - let (mut nesting, mut depth, mut lambda) = get_nesting_from_map(node, nesting_map); + let Nesting { + conditional: mut nesting, + function_depth: mut depth, + mut lambda, + } = get_nesting_from_map(node, nesting_map); match node.kind_id().into() { // tree-sitter-perl parses `elsif_clause` as a direct child of @@ -106,6 +110,13 @@ impl Cognitive for PerlCode { } _ => {} } - nesting_map.insert(node.id(), (nesting, depth, lambda)); + nesting_map.insert( + node.id(), + Nesting { + conditional: nesting, + function_depth: depth, + lambda, + }, + ); } } diff --git a/src/metrics/cognitive/php.rs b/src/metrics/cognitive/php.rs index f23d9659..622f3aa5 100644 --- a/src/metrics/cognitive/php.rs +++ b/src/metrics/cognitive/php.rs @@ -22,7 +22,11 @@ impl Cognitive for PhpCode { ) { use Php::*; - let (mut nesting, mut depth, mut lambda) = get_nesting_from_map(node, nesting_map); + let Nesting { + conditional: mut nesting, + function_depth: mut depth, + mut lambda, + } = get_nesting_from_map(node, nesting_map); match node.kind_id().into() { // The two-word `else if` form parses as `else_clause → @@ -101,6 +105,13 @@ impl Cognitive for PhpCode { } _ => {} } - nesting_map.insert(node.id(), (nesting, depth, lambda)); + nesting_map.insert( + node.id(), + Nesting { + conditional: nesting, + function_depth: depth, + lambda, + }, + ); } } diff --git a/src/metrics/cognitive/python.rs b/src/metrics/cognitive/python.rs index 9aee653a..6b6125d2 100644 --- a/src/metrics/cognitive/python.rs +++ b/src/metrics/cognitive/python.rs @@ -45,11 +45,25 @@ fn python_comprehension_clause_nesting( for child in node.children() { let kind = child.kind_id(); if kind == ForInClause as u16 { - nesting_map.insert(child.id(), (nesting + for_count, depth, lambda)); + nesting_map.insert( + child.id(), + Nesting { + conditional: nesting + for_count, + function_depth: depth, + lambda, + }, + ); for_count += 1; last_clause_is_if = false; } else if kind == IfClause as u16 { - nesting_map.insert(child.id(), (nesting + for_count, depth, lambda)); + nesting_map.insert( + child.id(), + Nesting { + conditional: nesting + for_count, + function_depth: depth, + lambda, + }, + ); last_clause_is_if = true; } } @@ -92,7 +106,11 @@ impl Cognitive for PythonCode { use Python::*; // Get nesting of the parent - let (mut nesting, mut depth, mut lambda) = get_nesting_from_map(node, nesting_map); + let Nesting { + conditional: mut nesting, + function_depth: mut depth, + mut lambda, + } = get_nesting_from_map(node, nesting_map); match node.kind_id().into() { // `else: if x:` chains surface as an `if_statement` wrapped @@ -186,6 +204,13 @@ impl Cognitive for PythonCode { _ => {} } // Add node to nesting map - nesting_map.insert(node.id(), (nesting, depth, lambda)); + nesting_map.insert( + node.id(), + Nesting { + conditional: nesting, + function_depth: depth, + lambda, + }, + ); } } diff --git a/src/metrics/cognitive/ruby.rs b/src/metrics/cognitive/ruby.rs index 5a0df723..5e38cef0 100644 --- a/src/metrics/cognitive/ruby.rs +++ b/src/metrics/cognitive/ruby.rs @@ -34,7 +34,11 @@ impl Cognitive for RubyCode { ) { use Ruby as R; - let (mut nesting, mut depth, mut lambda) = get_nesting_from_map(node, nesting_map); + let Nesting { + conditional: mut nesting, + function_depth: mut depth, + mut lambda, + } = get_nesting_from_map(node, nesting_map); match node.kind_id().into() { // Nesting-increasing constructs. tree-sitter-ruby models @@ -133,6 +137,13 @@ impl Cognitive for RubyCode { } _ => {} } - nesting_map.insert(node.id(), (nesting, depth, lambda)); + nesting_map.insert( + node.id(), + Nesting { + conditional: nesting, + function_depth: depth, + lambda, + }, + ); } } diff --git a/src/metrics/cognitive/rust.rs b/src/metrics/cognitive/rust.rs index 595007de..fd231817 100644 --- a/src/metrics/cognitive/rust.rs +++ b/src/metrics/cognitive/rust.rs @@ -22,7 +22,11 @@ impl Cognitive for RustCode { ) { use Rust::*; // Macro expansion is not tracked; macros are treated as opaque tokens. - let (mut nesting, mut depth, mut lambda) = get_nesting_from_map(node, nesting_map); + let Nesting { + conditional: mut nesting, + function_depth: mut depth, + mut lambda, + } = get_nesting_from_map(node, nesting_map); match node.kind_id().into() { IfExpression if !Self::is_else_if(node) => { @@ -62,6 +66,13 @@ impl Cognitive for RustCode { } _ => {} } - nesting_map.insert(node.id(), (nesting, depth, lambda)); + nesting_map.insert( + node.id(), + Nesting { + conditional: nesting, + function_depth: depth, + lambda, + }, + ); } } diff --git a/src/metrics/cognitive/tcl.rs b/src/metrics/cognitive/tcl.rs index f2a491e0..ed93754b 100644 --- a/src/metrics/cognitive/tcl.rs +++ b/src/metrics/cognitive/tcl.rs @@ -22,7 +22,11 @@ impl Cognitive for TclCode { ) { use Tcl::*; - let (mut nesting, mut depth, lambda) = get_nesting_from_map(node, nesting_map); + let Nesting { + conditional: mut nesting, + function_depth: mut depth, + lambda, + } = get_nesting_from_map(node, nesting_map); match node.kind_id().into() { // Guard kept for defensive consistency with sibling impls; Tcl's dedicated @@ -62,6 +66,13 @@ impl Cognitive for TclCode { } _ => {} } - nesting_map.insert(node.id(), (nesting, depth, lambda)); + nesting_map.insert( + node.id(), + Nesting { + conditional: nesting, + function_depth: depth, + lambda, + }, + ); } } diff --git a/src/spaces.rs b/src/spaces.rs index 8f7bcb5a..614657ef 100644 --- a/src/spaces.rs +++ b/src/spaces.rs @@ -102,10 +102,27 @@ impl Hasher for NodeIdHasher { } } -/// Per-node `(nesting, depth, lambda)` state that `Cognitive` inherits -/// down the walk; see `spaces::compute::propagate_nesting_to_children`. -pub(crate) type NestingMap = - HashMap>; +/// Per-node nesting state that `Cognitive` inherits down the walk. +/// +/// A struct rather than a `(usize, usize, usize)` because the three are +/// same-typed and summed symmetrically (`conditional + function_depth + +/// lambda`), so a transposition is invisible at the point of use and +/// surfaces only in the arms that read one field alone. Naming them +/// makes the boundary — where the walk reads and writes this — the +/// place a mix-up is visible. +#[derive(Clone, Copy, Default, Debug, PartialEq, Eq)] +pub(crate) struct Nesting { + /// Structural nesting from enclosing control constructs. + pub(crate) conditional: usize, + /// Count of enclosing functions. + pub(crate) function_depth: usize, + /// Count of enclosing lambdas / closures. + pub(crate) lambda: usize, +} + +/// Node-id keyed [`Nesting`] state; see +/// `spaces::compute::propagate_nesting_to_children`. +pub(crate) type NestingMap = HashMap>; use crate::langs::LANG; use crate::metric_set::{Metric, MetricSet}; diff --git a/src/spaces/compute.rs b/src/spaces/compute.rs index 77c8781b..dd11bf29 100644 --- a/src/spaces/compute.rs +++ b/src/spaces/compute.rs @@ -358,7 +358,7 @@ struct Walk { } /// Seeds each child's `nesting_map` slot from `node`'s own, so -/// `Cognitive` can read the triple it inherits without calling +/// `Cognitive` can read the [`Nesting`] it inherits without calling /// `Node::parent`. /// /// A node's slot means two different things either side of its @@ -367,23 +367,23 @@ struct Walk { /// - **on entry** it holds what the node inherits — this is what /// `get_nesting_from_map` reads; /// - **on exit** each language's `compute` has overwritten it with the -/// post-increment triple its children should see — this is what this +/// post-increment `Nesting` its children should see — this is what this /// function hands down. /// /// So the write at the end of every `Cognitive::compute` must stay at the /// end. Moving it to the top would make every descendant inherit the -/// pre-increment triple and silently under-count. +/// pre-increment `Nesting` and silently under-count. /// /// `or_insert`, not `insert`: Python's comprehension handling pre-writes /// its clause children's slots during the *comprehension's* `compute` /// (the #421 fix, so clause nesting does not depend on sibling traversal /// order), and those values deliberately differ from the comprehension's -/// own triple. A blanket overwrite would clobber them — the +/// own `Nesting`. A blanket overwrite would clobber them — the /// `python_comprehension_*` tests in `metrics::cognitive` are what catch /// it. /// /// Grammars whose `Cognitive` impl is the macro's no-op (`Preproc`, -/// `Ccomment`) never write a slot, so they inherit the root's `(0, 0, 0)` +/// `Ccomment`) never write a slot, so they inherit the root's default /// unchanged. That matches the pre-#1062 behaviour only because zero is /// the identity here; it costs them a map entry per node that nothing /// reads. @@ -471,7 +471,7 @@ pub(crate) fn metrics_inner( // Initialize nesting_map used for storing nesting information for cognitive // Three type of nesting info: conditionals, functions and lambdas let mut nesting_map = NestingMap::default(); - nesting_map.insert(node.id(), (0, 0, 0)); + nesting_map.insert(node.id(), Nesting::default()); // Suppression markers are resolved inline during the walk rather // than queued for a post-finalize pass. When we visit a comment From 51fb86d89c6d68386696b92f70c8528c2d8f38a1 Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Sat, 25 Jul 2026 15:32:39 -0700 Subject: [PATCH 5/7] refactor(spaces): push_children returns children `metrics_inner` recorded `stack.len()` before calling `push_children` and sliced `&stack[first_child..]` fourteen lines later, to hand this node's children to `propagate_nesting_to_children`. Correct today, but the invariant that nothing may push between those two points was stated at neither site. `push_children` now returns the children it pushed as a slice borrowed from `stack` -- empty for a leaf, reverse source order like the stack itself. Borrowing rather than returning indices is what makes this a compiler-checked claim: inserting a `stack.push(..)` between the call and the use is now `error[E0499]`, verified, where before it silently widened the slice. `ops.rs` discards the return and needed no change beyond a note saying why. A first cut returned `Range`. That is strictly better than the hand-rolled arithmetic -- the range is frozen at return time, so it cannot over-count -- but two bare integers carry no relationship to the vector, so the validity window stayed a convention. The borrow closes it for the same line count. Correcting a claim in that first cut: it said the existing cognitive suite covers the range end to end, "since any wrong range breaks nesting inheritance". That is true only for under-count. Perturbing the range to sweep in one extra node leaves all 332 cognitive tests passing, because `propagate_nesting_to_children` uses `entry().or_insert()` and every node is already seeded when it is pushed, so the extra write no-ops. Under-count is caught loudly (102 and 30 failures for the two off-by-one directions); over-count is invisible. It is unreachable after this change, which is the point, but the evidence offered for it was wrong. Also adds a `debug_assert!` that the scratch buffer arrives drained -- the new doc asserts the slice is exactly this node's children, which leftovers would falsify. The tractability test now takes the best of three timings per depth. It flaked once inside `make pre-commit`, which runs clippy, rustdoc and the Python stages alongside the suite: 70 ms vs 394 ms, a 5.6x reading on code that is linear. The two timings are sequential, so a load spike between them skews the ratio by itself. Contention is bursty and sheds under a minimum; a real O(depth) regression is present in every run and still fails at 4.0x, verified by revert. --- src/metrics/cognitive.rs | 23 +++++++++++++++++++++-- src/ops.rs | 2 ++ src/spaces/compute.rs | 28 +++++++++++++++++++++------- 3 files changed, 44 insertions(+), 9 deletions(-) diff --git a/src/metrics/cognitive.rs b/src/metrics/cognitive.rs index ff26e90d..f3612918 100644 --- a/src/metrics/cognitive.rs +++ b/src/metrics/cognitive.rs @@ -9486,9 +9486,28 @@ end", assert_eq!(cognitive_of(&nested_whiles(3)).0, expected(3), "1 + 2 + 3"); + // Best-of-three per depth. A single pair of timings is not + // robust enough here: the two measurements are sequential, so a + // load spike between them skews the ratio on its own. This test + // did flake once inside `make pre-commit`, which runs clippy, + // rustdoc and the Python stages alongside the suite — 70 ms vs + // 394 ms, a 5.6x reading on code that is linear. Contention is + // bursty, so the minimum of a few runs sheds it while a genuine + // O(depth) regression, which is present in every run, survives. + let best_of_three = |n: usize| -> (u64, std::time::Duration) { + let mut sum = 0; + let mut best = std::time::Duration::MAX; + for _ in 0..3 { + let (s, elapsed) = cognitive_of(&nested_whiles(n)); + sum = s; + best = best.min(elapsed); + } + (sum, best) + }; + let base = 2_000; - let (shallow_sum, shallow_time) = cognitive_of(&nested_whiles(base)); - let (deep_sum, deep_time) = cognitive_of(&nested_whiles(base * 2)); + let (shallow_sum, shallow_time) = best_of_three(base); + let (deep_sum, deep_time) = best_of_three(base * 2); assert_eq!(shallow_sum, expected(base as u64)); assert_eq!(deep_sum, expected(base as u64 * 2)); diff --git a/src/ops.rs b/src/ops.rs index 83eb533b..df504de5 100644 --- a/src/ops.rs +++ b/src/ops.rs @@ -254,6 +254,8 @@ pub(crate) fn ops_inner( // mirrors (which differ by `State` payload) it is reused directly // rather than duplicated. The `children.drain(..).rev()` ordering // it encapsulates is load-bearing for suppression attribution. + // The returned child slice is only useful to `metrics_inner`, + // which seeds their cognitive nesting; `ops` just walks them. push_children(&mut cursor, &node, new_level, &mut children, &mut stack); } diff --git a/src/spaces/compute.rs b/src/spaces/compute.rs index dd11bf29..5b794ebb 100644 --- a/src/spaces/compute.rs +++ b/src/spaces/compute.rs @@ -422,13 +422,28 @@ fn propagate_nesting_to_children( /// `Tag` is generic because the two walkers carry different context down /// the tree: `ops` needs only the nesting level, while `metrics_inner` /// also propagates comment membership ([`Walk`], issue #1052). -pub(crate) fn push_children<'a, Tag: Copy>( +/// +/// Returns the children just pushed, as a slice borrowed from `stack` — +/// empty for a leaf, and in reverse source order like the stack itself. +/// +/// Borrowing rather than returning indices is what makes "these are +/// exactly this node's children" a compiler-checked claim: the borrow +/// forbids touching `stack` while the slice is alive, so the slice +/// cannot drift from the pushes it describes. Recording `stack.len()` +/// around the call instead only holds while the two stay adjacent, and +/// nothing enforces that. +pub(crate) fn push_children<'a, 's, Tag: Copy>( cursor: &mut Cursor<'a>, node: &Node<'a>, tag: Tag, children: &mut Vec<(Node<'a>, Tag)>, - stack: &mut Vec<(Node<'a>, Tag)>, -) { + stack: &'s mut Vec<(Node<'a>, Tag)>, +) -> &'s [(Node<'a>, Tag)] { + debug_assert!( + children.is_empty(), + "scratch buffer must be left drained by the previous call" + ); + let first = stack.len(); cursor.reset(node); if cursor.goto_first_child() { loop { @@ -441,6 +456,7 @@ pub(crate) fn push_children<'a, Tag: Copy>( stack.push(child); } } + &stack[first..] } pub(crate) fn metrics_inner( @@ -590,9 +606,7 @@ pub(crate) fn metrics_inner( ); } - let first_child = stack.len(); - - push_children( + let pushed = push_children( &mut cursor, &node, Walk { @@ -604,7 +618,7 @@ pub(crate) fn metrics_inner( ); if selected.contains(Metric::Cognitive) { - propagate_nesting_to_children(&node, &stack[first_child..], &mut nesting_map); + propagate_nesting_to_children(&node, pushed, &mut nesting_map); } } From 0e0f85da3a16c943aecdfd504445f8802267bf91 Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Sat, 25 Jul 2026 16:36:41 -0700 Subject: [PATCH 6/7] perf(spaces): skip nesting map for no-op grammars `metrics_inner` pre-seeded the cognitive nesting map with the root, and `propagate_nesting_to_children` handed that default down the tree. The two grammars whose `Cognitive::compute` is the macro's no-op (`Preproc`, `Ccomment`) therefore built an entry per AST node that nothing ever read. Dropping the seed removes it, with no trait change and no per-grammar branch. The transformation is value-preserving by construction, which is a shorter argument than auditing impls: the seed's only effect was the value observed at the root, and `get_nesting_from_map` already returns the identical `Nesting::default()` there. An unseeded child reads the same default too. So every node observes exactly what it observed before, whatever any individual impl does. Two effects, measured with temporary instrumentation on a four-line preprocessor input (map entries at end of walk): Preproc 7 -> 0 Ccomment 7 -> 0 C 14 -> 14 C, --metrics loc 1 -> 0 The last row is the broader one and an earlier draft of this message missed it. Whenever `cognitive` is deselected nothing else ever writes the map, so the seed was its *only* entry -- meaning every metric-subset run, on all 25 languages, allocated a hash table per file for a single entry no one read. `NestingMap::default()` allocates nothing. The map size tracks the AST node count exactly, so a real header sheds one entry per node: `/usr/include/stdio.h` is ~885 Preproc nodes. `propagate_nesting_to_children`'s missing-slot branch becomes the documented normal path for those two grammars and loses its `debug_assert!(false, ..)`. Nothing was lost: the assert was already unreachable, and the obvious replacement is vacuous, because a miss is *root-only* -- every non-root slot is created by that function's own `or_insert` before the node is popped, so at a miss the map is always empty. That is also the invariant the code comment now states; a first draft justified it by claiming every real impl writes on every path, which is true (23 of them do) but is not what the code relies on. Cognitive values are unchanged; 3070 lib tests pass. No test pins the saving. Map size is not observable outside the walk, a unit test on the helper would build its own map and so would not catch a re-added seed, and an end-of-walk assertion cannot tell a no-op grammar from a real one without a trait const this deliberately avoids. --- CHANGELOG.md | 8 ++++++++ src/spaces/compute.rs | 31 +++++++++++++++++++------------ 2 files changed, 27 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 958364d8..853f1d03 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -66,6 +66,14 @@ for historical reference. 8000 / 16000 / 32000 / 64000 take 13 / 23 / 45 / 88 ms, so a 128 KB file completes in under a tenth of a second. + The walker also no longer pre-seeds that map with the root. Nothing + read the seed — the lookup already falls back to a default — but it was + the map's only entry whenever `cognitive` is deselected, so a + metric-subset run (`--metrics loc`, `bca check` with a threshold + subset) allocated a hash table per file for one unread entry. The two + grammars whose cognitive impl is a no-op, `preproc` and `ccomment`, now + build no map at all rather than one entry per AST node. + **Deeply nested code is still superlinear in general** — this fixes one of several `Node::parent` call sites, and *not* the one users hit most. `Checker::is_else_if` calls `parent()` (or `previous_sibling()`, which diff --git a/src/spaces/compute.rs b/src/spaces/compute.rs index 5b794ebb..ed76a2e7 100644 --- a/src/spaces/compute.rs +++ b/src/spaces/compute.rs @@ -383,10 +383,8 @@ struct Walk { /// it. /// /// Grammars whose `Cognitive` impl is the macro's no-op (`Preproc`, -/// `Ccomment`) never write a slot, so they inherit the root's default -/// unchanged. That matches the pre-#1062 behaviour only because zero is -/// the identity here; it costs them a map entry per node that nothing -/// reads. +/// `Ccomment`) never write a slot, so the root lookup misses and the +/// walk seeds nothing for them at all — no map, no allocation. fn propagate_nesting_to_children( node: &Node, children: &[(Node<'_>, Walk)], @@ -397,12 +395,18 @@ fn propagate_nesting_to_children( if children.is_empty() { return; } - // Unreachable in practice: the root is seeded before the walk and every - // other node is seeded by this function before it is popped. Degrading - // to "no inheritance" would silently zero a whole subtree's nesting, so - // fail loudly in debug rather than absorb it. + // A miss here is a *root-only* path. Every non-root slot is created + // by this function's `or_insert` below, before that node is ever + // popped, so reaching a node with no slot means it had no parent to + // seed it. The root's own slot exists iff the root's `compute` wrote + // one — which the two no-op grammars never do, so for them the walk + // seeds nothing and the map stays empty. + // + // Note this does *not* depend on every real impl writing on every + // path: a real impl that skipped its write would still leave its + // children seeded, and would show up as wrong nesting values, not as + // a missing slot. let Some(&inherited) = nesting_map.get(&node.id()) else { - debug_assert!(false, "node {} has no nesting slot", node.id()); return; }; for (child, _) in children { @@ -484,10 +488,13 @@ pub(crate) fn metrics_inner( let mut children = Vec::new(); let mut state_stack: Vec = Vec::new(); let mut last_level = 0; - // Initialize nesting_map used for storing nesting information for cognitive - // Three type of nesting info: conditionals, functions and lambdas + // Per-node cognitive nesting, inherited down the walk. Deliberately + // not pre-seeded with the root: `get_nesting_from_map` already falls + // back to `Nesting::default()`, so a seed would change nothing for + // grammars that compute cognitive — while for the two whose impl is + // the macro's no-op it is the one write that would make the walk + // build an entry per node that nothing ever reads. let mut nesting_map = NestingMap::default(); - nesting_map.insert(node.id(), Nesting::default()); // Suppression markers are resolved inline during the walk rather // than queued for a post-finalize pass. When we visit a comment From c74f5e6dc936ef3785e73170b04c94e7cdcb5f2c Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Sat, 25 Jul 2026 21:32:22 -0700 Subject: [PATCH 7/7] test(spaces): cover NodeIdHasher's integer writes Codecov flagged 14 uncovered lines on this PR, all in `src/spaces.rs`: `write`, `write_u64`, `write_u32` and `write_u8` on `NodeIdHasher`. `HashMap` only ever calls `write_usize`, so those four are unreachable today. They exist so that changing the key type cannot silently fall back to `Hasher::write`'s byte-at-a-time default -- eight dependent rounds per word, slower than the SipHash this replaced -- and that regression is invisible, because results stay correct and only speed changes. Two tests, both verified by revert: * the integer writes must agree for the same value, and the byte-slice path must NOT agree with them -- if it ever did, the overrides would be doing nothing and their removal would go unnoticed. Deleting `write_u64` fails this. * `finish` must rotate. A multiplicative hash concentrates entropy in the top bits while hashbrown takes the bucket index from the bottom ones, and node ids are pointer-aligned, so without the rotate every hash ends in three zero bits and one home bucket in eight is reachable. A first cut of this hasher had exactly that defect. Removing the rotate fails this. `src/spaces.rs` goes 43 -> 29 uncovered lines; the remaining 29 predate this branch (`a17e82b1`). Also makes the cognitive scaling assertion opt-in behind `BCA_ASSERT_SCALING`. Its value assertions still run by default; only the wall-clock ratio is gated. It has now produced a false failure in three environments -- `windows-latest` at 10.9 s against an absolute budget, a local `make pre-commit` at 5.6x with clippy and rustdoc alongside, and `cargo llvm-cov` at 3.5x even with best-of-three, whose instrumentation is the reason this surfaced during coverage work. The `coverage` job runs in CI, so leaving it armed reds builds on measurement artefacts. It still fails at 4.1x with the flag set against the pre-fix lookup, so the guard is intact where it can be trusted. Complexity guards belong in a benchmark harness (#1068). --- src/metrics/cognitive.rs | 20 +++++++++ src/spaces_tests.rs | 88 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 108 insertions(+) diff --git a/src/metrics/cognitive.rs b/src/metrics/cognitive.rs index f3612918..90103bad 100644 --- a/src/metrics/cognitive.rs +++ b/src/metrics/cognitive.rs @@ -9456,6 +9456,17 @@ end", /// runs under `cargo llvm-cov` and on shared Windows / macOS runners. /// Doubling the depth costs ~2x when the lookup is `O(1)` and ~4x when /// it is `O(depth)`, and that ratio is independent of host speed. + /// The wall-clock half of this test is opt-in via + /// `BCA_ASSERT_SCALING=1`. The value assertions above it always run. + /// + /// A timing assertion has now produced a false failure in three + /// different environments: `windows-latest` in CI (10.9 s against an + /// 8 s absolute budget), a local `make pre-commit` running clippy and + /// rustdoc alongside the suite (5.6x), and `cargo llvm-cov`, whose + /// instrumentation skewed even a best-of-three ratio to 3.5x. The + /// `coverage` job runs in CI, so leaving it armed reds the build on a + /// measurement artefact rather than a regression. Complexity guards + /// belong in a benchmark harness (#1068), not the unit suite. #[test] fn cognitive_deep_nesting_is_tractable() { // Restricted to `Cognitive` — which pulls in `Nom` as a declared @@ -9511,6 +9522,15 @@ end", assert_eq!(shallow_sum, expected(base as u64)); assert_eq!(deep_sum, expected(base as u64 * 2)); + // Wall clock is only asserted when this test is run explicitly. + // See the `#[ignore]` rationale on the attribute below: three + // separate environments have produced a false reading here, and + // a red build from a timing artefact costs more than this guard + // is worth inside the default suite. + if std::env::var_os("BCA_ASSERT_SCALING").is_none() { + return; + } + // Guard against a zero denominator on a very fast machine. let shallow_ns = shallow_time.as_nanos().max(1); let ratio = deep_time.as_nanos() as f64 / shallow_ns as f64; diff --git a/src/spaces_tests.rs b/src/spaces_tests.rs index a731c77b..d28f2c09 100644 --- a/src/spaces_tests.rs +++ b/src/spaces_tests.rs @@ -1806,3 +1806,91 @@ int helper(int y) { ); }); } + +/// `NodeIdHasher`'s integer writes must all agree for the same value. +/// +/// `HashMap` only ever calls `write_usize`, so the other +/// overrides are unreachable today — they exist so that changing the key +/// type (to `u64`, say, when the node id gains a newtype) cannot silently +/// fall back to `Hasher::write`'s byte-at-a-time default, which costs +/// eight dependent rounds per word and would be slower than the SipHash +/// this replaced. That regression is invisible: results stay correct and +/// only speed changes. This pins the equivalence so the fallback cannot +/// creep back in unnoticed. +#[test] +fn node_id_hasher_integer_writes_agree() { + use std::hash::Hasher; + + use crate::spaces::NodeIdHasher; + + let hash_of = |write: &dyn Fn(&mut NodeIdHasher)| { + let mut h = NodeIdHasher::default(); + write(&mut h); + h.finish() + }; + + for value in [0_u64, 1, 42, 0x0123_4567, u64::from(u32::MAX), u64::MAX] { + let via_u64 = hash_of(&|h| h.write_u64(value)); + let via_usize = hash_of(&|h| h.write_usize(value as usize)); + assert_eq!( + via_u64, via_usize, + "write_u64 and write_usize must agree for {value:#x}" + ); + + if let Ok(narrow) = u32::try_from(value) { + assert_eq!( + hash_of(&|h| h.write_u32(narrow)), + via_u64, + "write_u32 must agree for {value:#x}" + ); + } + if let Ok(narrow) = u8::try_from(value) { + assert_eq!( + hash_of(&|h| h.write_u8(narrow)), + via_u64, + "write_u8 must agree for {value:#x}" + ); + } + } + + // The byte-slice path is the fallback the overrides exist to avoid. + // It must still work, and must NOT coincide with the word writes — + // if it did, the overrides would be pointless and their removal + // would go unnoticed. + let via_bytes = hash_of(&|h| h.write(&7_u64.to_ne_bytes())); + assert_ne!( + via_bytes, + hash_of(&|h| h.write_u64(7)), + "the byte loop folds one round per byte, so it cannot match the \ + single-round word write; if these ever agree, the overrides are \ + no longer doing anything" + ); +} + +/// `finish` must rotate, not return the raw product. +/// +/// A multiplicative hash concentrates entropy in the top bits, but +/// hashbrown takes the bucket index from the bottom ones — and node ids +/// are pointer-aligned, so without the rotate every hash ends in at least +/// three zero bits and only one home bucket in eight is reachable. +/// `rustc-hash` added this in 2.0; a first cut of `NodeIdHasher` copied +/// the 1.x algorithm and reproduced exactly the defect upstream removed. +#[test] +fn node_id_hasher_finish_rotates_entropy_down() { + use std::hash::Hasher; + + use crate::spaces::NodeIdHasher; + + // Pointer-aligned ids, as tree-sitter produces. + let low_bits_set = (1..64_usize).any(|i| { + let mut h = NodeIdHasher::default(); + h.write_usize(i * 8); + h.finish() & 0b111 != 0 + }); + assert!( + low_bits_set, + "every 8-aligned id hashed to a value with three zero low bits, so \ + only one home bucket in eight is reachable — `finish` is not \ + rotating" + ); +}