Map candidate spans to ra_ap_syntax nodes and extract AST feature vectors (7.3.1) - #238
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 16 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
WalkthroughThis PR adds parser-independent AST lowering, deterministic feature extraction and hashing, verification harnesses, expanded lint test-context detection, pinned tooling, Cargo lock-mode propagation, documentation updates, and refreshed installer fixtures. ChangesAST extraction, validation, and build integration
Lint diagnostics and test-context recognition
Documentation and fixtures
CI and repository tooling
Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 17 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (17 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
ra_ap_syntax nodes and extract AST feature vectors (7.3.1)
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. crates/whitaker_clones_core/tests/ast_feature_extraction_behaviour.rs Comment on file //! Behaviour-driven coverage for AST feature extraction.
❌ New issue: String Heavy Function Arguments |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. crates/whitaker_clones_core/src/ast/kani.rs Comment on lines +37 to +56 fn bounded_tree(
root_kind: KindId,
left_branch_kind: KindId,
right_branch_kind: KindId,
left_leaf_kind: KindId,
right_leaf_kind: KindId,
left_first: bool,
) -> NormalisedTree {
let left_leaf = leaf(left_leaf_kind, LeafClass::Ident);
let right_leaf = leaf(right_leaf_kind, LeafClass::Literal);
let left_branch = branch(left_branch_kind, vec![left_leaf]);
let right_branch = branch(right_branch_kind, vec![right_leaf]);
let children = if left_first {
vec![left_branch, right_branch]
} else {
vec![right_branch, left_branch]
};
NormalisedTree::new(branch(root_kind, children), ast_span())
}❌ New issue: Excess Number of Function Arguments |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. crates/no_expect_outside_tests/src/driver/mod.rs Comment on lines +181 to +187 let owner_hir_id: hir::HirId = expr.hir_id.owner.into();
if has_test_like_hir_attributes(cx.tcx.hir_attrs(owner_hir_id), additional_test_attributes)
|| is_harness_marked_test_function(owner_hir_id, harness_marked_test_functions)
{
return true;
}❌ New issue: Complex Method |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
@coderabbitai Have the following failed checks now been resolved? If further work is required, please provide an AI agent prompt for the remaining work to be done to address these failures. Do not treat warnings as optional or aspirational. Where a change is out of scope for this PR, propose a GitHub issue unless one exists already. (Treat documentation and validation coverage as in scope). ❌ Failed checks (2 errors, 4 warnings)
|
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
This comment was marked as resolved.
This comment was marked as resolved.
80d7ba8 to
841b378
Compare
3df2f6b to
42d9ece
Compare
Resolve the remaining PR #238 testing, user-facing documentation, and in-scope AST observability findings. - lowering.rs: emit `tracing::error!` before every `TreeTooDeep` / `TreeTooLarge` return in both `validate_covering_node_budget` (covering- node selection) and `LoweringLimits::lower`, with distinct messages that name the failing phase. Each log carries the requested-span `start`/`end` (the `ByteSpan` is now threaded into `validate_covering_node_budget`), plus `depth`+`limit` for depth breaches and the observed `node_count`+ `limit` for node breaches. The typed errors and the `lower_span` `#[tracing::instrument]` fields are unchanged; no metrics/timing/global state is introduced (deferred to the 7.3.2 consumer boundary). - lowering_tests.rs: add a focused test asserting the selection budget surfaces `TreeTooDeep`/`TreeTooLarge`; the existing lowering-path budget tests are retained. - users-guide.md: add a top-level "Clone Detection: AST Feature Extraction" section describing the Pass B substrate — `lower_span`, deterministic feature vectors (kind counts, `2^63 >> depth` weighting zero beyond depth 63, production bigrams/trigrams, opaque hex canonical hash), `PARSER_SCHEMA_VERSION`-versioned hashes, the default `parser` feature and its `AstError::ParserUnavailable` fallback, and the 7.3.2 deferral of scoring/SARIF. The `rstest_helper_should_be_fixture` section is untouched. - artefact packaging: delete the inert `when_filename_inspected` step and its Gherkin `When` line; the scenario now runs Given -> Then directly and `then_filename_matches_artefact_name` still asserts against `ArtefactName::filename()`. Validated: cargo test -p whitaker_clones_core ast; cargo test -p whitaker-installer --test behaviour_artefact_packaging; make markdownlint; cargo clippy -p whitaker_clones_core --tests -- -D warnings. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Resolve the remaining PR #238 testing, user-facing documentation, and in-scope AST observability findings. - lowering.rs: emit `tracing::error!` before every `TreeTooDeep` / `TreeTooLarge` return in both `validate_covering_node_budget` (covering- node selection) and `LoweringLimits::lower`, with distinct messages that name the failing phase. Each log carries the requested-span `start`/`end` (the `ByteSpan` is now threaded into `validate_covering_node_budget`), plus `depth`+`limit` for depth breaches and the observed `node_count`+ `limit` for node breaches. The typed errors and the `lower_span` `#[tracing::instrument]` fields are unchanged; no metrics/timing/global state is introduced (deferred to the 7.3.2 consumer boundary). - lowering_tests.rs: add a focused test asserting the selection budget surfaces `TreeTooDeep`/`TreeTooLarge`; the existing lowering-path budget tests are retained. - users-guide.md: add a top-level "Clone Detection: AST Feature Extraction" section describing the Pass B substrate — `lower_span`, deterministic feature vectors (kind counts, `2^63 >> depth` weighting zero beyond depth 63, production bigrams/trigrams, opaque hex canonical hash), `PARSER_SCHEMA_VERSION`-versioned hashes, the default `parser` feature and its `AstError::ParserUnavailable` fallback, and the 7.3.2 deferral of scoring/SARIF. The `rstest_helper_should_be_fixture` section is untouched. - artefact packaging: delete the inert `when_filename_inspected` step and its Gherkin `When` line; the scenario now runs Given -> Then directly and `then_filename_matches_artefact_name` still asserts against `ArtefactName::filename()`. Validated: cargo test -p whitaker_clones_core ast; cargo test -p whitaker-installer --test behaviour_artefact_packaging; make markdownlint; cargo clippy -p whitaker_clones_core --tests -- -D warnings. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
3519d7b to
9cbd7c8
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/workflows/test_cargo_locked_makefile.py`:
- Around line 234-241: Update the private helper _has_locked_flag to use a
single-line docstring summarizing its behavior; move the tokenization rationale
to a regular comment only if it needs to be retained, while preserving the
exact-token matching logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0c7fa06c-3fb5-4714-afbb-07175145a519
📒 Files selected for processing (9)
crates/whitaker_clones_core/src/ast/features.rscrates/whitaker_clones_core/src/ast/lowering.rscrates/whitaker_clones_core/src/ast/lowering_tests.rscrates/whitaker_clones_core/src/hashing.rsdocs/execplans/7-3-1-map-candidate-spans-and-extract-ast-feature-vectors.mddocs/users-guide.mdinstaller/tests/behaviour_artefact_packaging.rsinstaller/tests/features/artefact_packaging.featuretests/workflows/test_cargo_locked_makefile.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
leynos/rstest-bdd(auto-detected)
💤 Files with no reviewable changes (2)
- installer/tests/features/artefact_packaging.feature
- installer/tests/behaviour_artefact_packaging.rs
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
Fix a false-positive in the shared rstest companion-module detection that
`no_unwrap_or_else_panic`, `no_expect_outside_tests`, and
`rstest_helper_should_be_fixture` all consume through
`collect_rstest_companion_test_functions`.
`module_has_inner_harness_descriptor_pairs` treated any same-named sibling
module carrying the `fn`/`const` harness-descriptor pair as an rstest
companion, exempting the same-named parent function from the lints. But
`rustc --test` synthesizes that same-name, same-span `const` descriptor for
*every* `#[test]` function, so a handwritten `mod foo { extern crate test as
t; #[test] fn bar() {} }` next to an ordinary `fn foo` has the identical HIR
shape and wrongly exempted `fn foo`.
The distinguishing rstest/rustc invariant is expansion provenance: rstest
emits the companion module through its attribute proc-macro, so the module
item comes from macro expansion, whereas a handwritten module sits at the
crate's root syntax context. `module_qualifies_as_rstest_companion_module`
now trusts the harness-pair heuristic only when
`module_item.span.from_expansion()` holds. The explicit
`RSTEST_HARNESS_DESCRIPTOR` marker remains ungated (no handwritten module
emits it), so descriptor-based regression fixtures still qualify. Genuine
`#[rstest]` recognition is unaffected: real expansions are from macro
expansion and still match.
- Replace the `pass_unwrap_in_rstest_aliased_test_crate` fixture (which
asserted the handwritten aliased-test-crate shape was exempt) with the
negative `fail_unwrap_in_hand_written_test_companion`, which reaches the
harness-pair/adjacency path yet asserts the lint fires on the parent.
- Keep `fail_unwrap_in_aliased_test_crate_non_companion` (aliased import
without a descriptor pair) as a distinct negative.
- Document the two-tier evidence rule and the expansion-provenance invariant
in the developer guide's test-context ancestry section.
Validation (pinned nightly-2026-05-28):
- cargo nextest run -p no_unwrap_or_else_panic --features dylint-driver: 25/25
- cargo nextest run -p rstest_helper_should_be_fixture --features dylint-driver: 16/16
- no_expect_outside_tests UI: 7 pre-existing environmental E0658 failures,
identical on the base revision (crate excluded from `make test`)
- make check-fmt, make lint, make test (1576), make markdownlint: pass
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Summarize the helper in a one-line docstring matching its sibling `_is_lock_relevant_invocation`, and keep the tokenization rationale as a regular comment on the return. Exact-token matching logic is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/execplans/7-3-1-map-candidate-spans-and-extract-ast-feature-vectors.md`:
- Around line 83-88: Update the bounded per-candidate cost description to
clarify that 7.3.1 avoids indiscriminate full-file lowering, while allowing a
candidate spanning the source to select SOURCE_FILE and intentionally lower the
complete file via lower_span. Preserve the existing explanation of bounded
subtree selection and AST node/depth budgets.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f2093962-56fb-463b-9c02-0ab8708719f0
📒 Files selected for processing (9)
crates/whitaker_clones_core/src/ast/features.rscrates/whitaker_clones_core/src/ast/lowering.rscrates/whitaker_clones_core/src/ast/lowering_tests.rscrates/whitaker_clones_core/src/hashing.rsdocs/execplans/7-3-1-map-candidate-spans-and-extract-ast-feature-vectors.mddocs/users-guide.mdinstaller/tests/behaviour_artefact_packaging.rsinstaller/tests/features/artefact_packaging.featuretests/workflows/test_cargo_locked_makefile.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
leynos/rstest-bdd(auto-detected)
💤 Files with no reviewable changes (2)
- installer/tests/features/artefact_packaging.feature
- installer/tests/behaviour_artefact_packaging.rs
The "does not lower whole files" claim was inaccurate: a candidate whose span covers the source legitimately selects `SOURCE_FILE` and lowers the complete file through `lower_span` (see `whole_file_span_selects_source_file`). Reword to say 7.3.1 avoids *indiscriminate* full-file lowering while allowing that intentional case, preserving the bounded-selection and node/depth budget explanation. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Summary
This PR completes roadmap item 7.3.1 — "Map candidate spans to
ra_ap_syntaxnodes and extract AST feature vectors" for clone detector PassB, first half.
It includes the execution plan and the implementation of the pure AST building
blocks that item 7.3.2 (Type-3 scoring + SARIF Run 1) will consume:
whitaker_clones_core::astmodule that maps a candidate byte span to thesmallest covering
ra_ap_syntaxnode;NormalisedTree/NormalisedNode);production bigram/trigram multisets, and canonical Merkle subtree hashes;
Scoring, SARIF Run 1 updates, latency metrics, and feature-vector emission
metrics remain out of scope for this PR and belong to 7.3.2.
Prerequisite: Stage 0 toolchain bump (
nightly-2026-05-28)The branch includes a Stage 0 bump from
nightly-2025-09-18(rustc 1.92) tonightly-2026-05-28(rustc ≥ 1.95). This unblocks the currentra_ap_syntaxsnapshot without a backwards dependency bisect and updates thesuite for Dylint 6 / newer rustc APIs.
Key design decisions
NormalisedTree) keeps parser imports confined to oneadapter file, with a boundary guard test enforcing that containment.
(KindId, depth)integer countsand applies depth weighting in pure feature-vector logic.
AstHash, typedAstError, andPARSER_SCHEMA_VERSIONprovide astable, forward-compatible surface for 7.3.2.
Kani covering-node harnesses.
Validation
make check-fmtmake testmake typecheckmake lintReferences
docs/execplans/7-3-1-map-candidate-spans-and-extract-ast-feature-vectors.mddocs/whitaker-clone-detector-design.md§"Pass B: AST engine (ra_ap_syntax)"docs/adr-003-formal-proof-strategy-for-clone-detector-pipeline.md