refactor(ast): split the parse and classification layer into big-code-analysis-ast - #1404
Merged
Merged
Conversation
ParserTrait now carries only the Checker / Getter classifiers and the tree accessors; the thirteen per-metric associated types move to a new MetricSuite supertrait with a blanket impl over Parser<T>. The metric walks bound on MetricSuite, everything that only classifies nodes stays on ParserTrait, so the parse layer no longer depends on the metric modules. The language-dispatched carrier behind Ast becomes AnyParser, whose variants exist regardless of the feature set (only the constructors are feature-gated), and a hand-written with_any_parser! macro replaces the per-operation run_* methods. Exhaustive matching makes a forgotten language a compile error and keeps the macro free of cfg attributes. Preparatory step for #1376: no behaviour change, no metric moves.
The Elixir keyword-Call, Tcl command-name and Python alias helpers were defined in metrics::cognitive / metrics::npa yet imported by the Checker and Getter impls, so the parse layer depended on the metric layer. They now live in a lang_helpers module the metrics import instead. SpaceKind and HalsteadType are Getter return types, so they move out of spaces / metrics::halstead into their own modules; both stay re-exported at their public paths. The two termcolor helpers leave tools.rs for output::color, and FromPathError leaves error.rs so that file holds only the parse-layer MetricsError. Preparatory step for #1376: no public path changes, no metric moves.
The generated kind enums, LANG and language detection, Node and the tree-sitter wrappers, the Checker / Getter / Alterator classifiers and their per-language impls, the C-family preprocessor pass, comment stripping, the AST dump, and node counting / finding now live in a published sub-crate the root pins at an exact version. The root keeps the metric walk, output formats, suppression and VCS metrics, and re-exports everything it re-exported before at the same paths, so no caller of big-code-analysis changes. The dependency runs one way only: the sub-crate builds and tests with this crate absent from its graph. Every per-language Cargo feature forwards to it under the same name, so a --features rust build still links exactly one grammar, and its own feature-matrix legs resolve what they name rather than unifying every language back on. Two types crossed with Getter because they are its return types. SpaceKind keeps its name and its public paths. HalsteadType is renamed TokenRole: it answers whether a node acts as an operator or an operand, which the grammar decides and any structural consumer can ask, so naming it after the single metric that reads it made the parse layer look like it carried metric vocabulary. The old name survives as a deprecated alias at metrics::halstead::HalsteadType until 3.0, pinned by a guard that fails to compile if it is dropped or re-pointed. Node's accessors that the walk uses become public and documented, and MetricsError moves with LANG::tree_sitter_language, which returns it. Gates, workflows, baselines and the enums codegen path follow the move. No metric value changes. Fixes #1376
`SpaceKind::is_member_scope` answered whether `wmc`, `npm` and `npa` roll up on a space kind — a question about three metrics, asked from a crate that is meant to know nothing about metrics. Its only callers were those three. It becomes a `pub(crate)` `MemberScopeExt` extension trait beside `average`, the other helper the metric modules share. `SpaceKind` keeps its name, its variants and its public paths; the parse layer now carries no metric vocabulary at all. Doing this before the release matters: the split had widened the predicate to `pub` so the walk could reach it across the crate boundary, and that widening would have been a public item this crate was then stuck with until 3.0. It never ships. The method takes `&self` because a trait cannot tell clippy::wrong_self_convention that every implementor is `Copy`, and a carve-out for a by-value `Copy` receiver would buy nothing.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1404 +/- ##
==========================================
- Coverage 98.49% 97.96% -0.54%
==========================================
Files 278 358 +80
Lines 77200 91516 +14316
Branches 76769 91085 +14316
==========================================
+ Hits 76037 89650 +13613
- Misses 752 1201 +449
- Partials 411 665 +254
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
AGENTS.md's layout bullet now names `space_kind.rs` and `token_role.rs` and states the rule they follow: nothing under the parse crate names a metric, which is why `HalsteadType` became `TokenRole` and the member-scope predicate lives in the root. A baseline example still pointed at `src/count.rs`, which moved. The book gains three corrections. Its feature page explains that the grammar crates now arrive through `big-code-analysis-ast`, so they sit one level down in `cargo tree` while nothing changes for a consumer. The developer guide lists the new member among the per-crate builds. The stability summary picks up a bullet saying the parse crate is not a stability surface. That page also carried a claim that predates this work: it said `Node` exposes its `tree_sitter::Node` "through `.0`", which #556 made private in favour of `as_tree_sitter()`. Corrected, and extended to cover the walk accessors this branch made public — shape-stable signatures over values that follow the grammar pin.
The clippy job aborted with "binary `clippy-sarif` already exists in destination", taking the aggregate `ci` gate down with it, and it did so on three consecutive attempts. Two cache layers own `~/.cargo/bin`. `Swatinem/rust-cache` runs with `cache-bin: true` and restores both binaries; the dedicated `actions/cache` step keyed on `sarif-tools-<os>-0.8.0` can still report a miss, because its key is scoped separately and a branch that has never populated it misses while rust-cache restores from the default branch. The install step then runs against a directory that already has the binaries and refuses to overwrite. `--force` makes it idempotent in both states. It is still reached only on a cache miss, so a warm run skips the build as before. Not part of #1376 — the failure surfaced on that branch because it is new, but the conflict is repo-wide and independent of it.
The unconditional variants make every generic walk monomorphise for all 25 languages under any feature subset — measured at 25 instantiations of metrics_inner and ops_inner in a --features rust build, against one before the split. That is now written next to the enum along with why it stays: the fix needs the dispatch macro in the root crate, which buys an eighth site enumerating every language, and a visitor trait cannot substitute because one defined here can only bound its type parameter by traits this crate can name. Nothing in the workspace pays the cost, and what it costs is compile time rather than shipped binary size. Written as a DECIDED note, matching .rustfmt-bail-baseline.txt's, so the next reader prices it from the record instead of re-deriving it.
`cargo llvm-cov report` scopes its output to the current package even when the run that produced the profile data was `--workspace`: it builds an `--ignore-filename-regex` naming every other member directory. The run and the report had drifted apart, so the report only ever described the root crate. That was invisible while the root crate held everything. Moving the parse layer out in #1376 made it visible: 103 files and ~15k lines left the report, which Codecov read as the project shrinking. Those files are not untested — measured at 97.99% — they were simply no longer being described. Repeating `--workspace` on the report also restores two crates that had never been measured at all: the CLI (62 files, 97.26%) and the web server (12 files, 92.30%). Measured lines go from 61,720 to 91,075. The reported percentage moves 99.29% -> 98.67%, which is the denominator growing rather than anything getting worse — the covered count rises from 61,280 to 89,866. Compare covered counts, not the percent column. The benchmark harness joins xtask in codecov's ignore list, and so does the PyO3 crate's Rust layer: pytest drives it and `cargo llvm-cov nextest` cannot see that, so it measures ~52% while being far better tested than that implies. codecov.yml already documented that gap and warned against papering over it with redundant cargo tests; reporting the number would have invited exactly those.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Splits the parse and classification layer out of the root crate into a
new published sub-crate,
big-code-analysis-ast, per the investigationin #1376. The cut is where Finding 2 of that issue proposed it: at the
Checker/Getterline, not at the bare parser.No caller of
big-code-analysischanges, and no metric value moves —the corpus snapshot suites pass byte-identical, which is the acceptance
test for this branch.
What moved
The generated per-grammar kind enums,
LANGand language detection,Nodeand the tree-sitter wrappers, theChecker/Getter/Alteratorclassifiers and their per-language impls, the C-familypreprocessor pass, comment stripping, the AST dump, and node counting
and finding. The root keeps the metric walk, the output formats,
suppression and the VCS metrics.
The dependency runs one way. The sub-crate builds and tests with the
root absent from its graph, and every per-language Cargo feature
forwards to it under the same name, so a
--features rustbuild stilllinks exactly one grammar.
What the issue got wrong
Three things had to cross the seam that #1376 did not anticipate:
SpaceKindand the operator/operand classification areGetterreturn types, so they moved with it. Both keep their public paths.
by the checkers, the exact inversion this split removes. They now sit
in
lang_helpers/and the metrics import them.tools.rsmoved because the preprocessor depends on its includeresolver.
Two of the issue's four "what does not get cheaper" items turned out
cheaper than expected:
mk_langs!needed no splitting, and theParserTraitinversion is a singleMetricSuitesupertrait with oneblanket impl. Item 4, the PyO3 safety-doc pin, was moot — that gate
reads the workspace pin, which did not move.
Two renames worth reviewing
HalsteadTypeis nowTokenRole. It answers whether a node actsas an operator or an operand, which the grammar decides and any
structural consumer can ask; naming it after the one metric that reads
it made the parse layer look like it carried metric vocabulary. The
old name survives as a deprecated alias until
3.0, pinned by a guardthat fails to compile if it is dropped or re-pointed.
SpaceKind::is_member_scopebecame apub(crate)MemberScopeExttrait in the root. It asks whether
wmc/npm/nparoll up on akind, which only the metric crate can ask. Doing this before the
release matters: the split had widened it to
pub, and that wouldhave been a public item stuck until
3.0.Review history
A fresh-context review of the whole branch found 15 findings on a tree
whose
make pre-commitwas already green. The one that mattered most:the root's dev-dependency on the sub-crate omitted
default-features = false, so cargo unifiedall-languagesback on inevery test build. Two
LanguageDisabledtests were passing vacuouslyand the
no-default-features/minimal-langsCI legs were linking all25 grammars rather than the ones they name. Both are fixed and verified
by perturbation.
Verification
make pre-commit:BCA_GATE: passmake chain-auditacross both crates,make bench-scaling,cargo deny check,cargo +nightly udeps, the codegen-drift,publish-metadata and lockstep-version gates
(ast)legs.snap.newundertests/repositories/Known, deliberate, and decided
AnyParser's 25 variants are unconditional so the exportedwith_any_parser!can be free ofcfgattributes — a#[cfg]insidean exported macro is evaluated in the invoking crate. The cost,
measured: a
--features rustbuild instantiates the metric walk for all25 languages instead of one.
This stays, and the reasoning is recorded next to the enum. Undoing
it means moving the dispatch macro into the root crate, which buys a
permanent eighth site enumerating every language; a visitor trait cannot
substitute, because one defined in the parse crate can only bound its
type parameter by traits that crate can name, and the metric walks need
the root's
MetricSuite. Nothing in this workspace pays the cost — theCLI, the server and the Python bindings all build
all-languages— andwhat it costs is compile time and rlib size, not shipped binary size,
since unreferenced instantiations are dropped at link time.
Separately,
cargo test --no-default-features --features rust,typescriptfails, and that is pre-existing on
main— the per-metric testmodules carry no feature gates, and CI only ever
cargo checks thatconfiguration. This branch neither caused it nor fixes it.