📝 CodeRabbit Chat: Implement requested code changes#237
📝 CodeRabbit Chat: Implement requested code changes#237coderabbitai[bot] wants to merge 7 commits into
Conversation
Add a draft execution plan for roadmap item 8.2.2, covering call-site collection inside `#[rstest]` tests, fixture-local classification, and constant-aware argument fingerprinting. The plan stays diagnostic-silent and defers threshold evaluation, diagnostic emission, and UI pass/fail fixtures to roadmap items 8.2.3 and 8.2.4. It records the in-crate adapter placement decision, dedup policy for `#[case]` companions, and the validation, review, and rollout sequence required before the roadmap tick lands.
Add a passive call-site collector for local helper invocations inside strict `#[rstest]` tests. Lower positional arguments into the shared argument fingerprint model, deduplicate recovered source spans, and log collected evidence at crate-post time without emitting diagnostics. Keep the implementation scoped to the lint crate and record the first milestone validation and CodeRabbit review outcome in the ExecPlan.
Add behavioural, property, and trybuild pass coverage for the rstest helper call-site collector. Record the CodeRabbit fixes and validation results in the 8.2.2 execplan.
Add an explicit regression proving macro-only HIR spans do not recover to user-editable source. Record the validation and CodeRabbit review outcome in the 8.2.2 execplan.
Update user, developer, design, roadmap, and execplan docs for the diagnostic-silent rstest helper call-site collection phase. Record the validation and review outcomes for 8.2.2.
Mark the 8.2.2 execplan PR step complete after pushing the branch and updating draft PR #235.
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. CodeRabbit does not review PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Reviewer's GuideRefactors the rstest helper call collection behavior to reuse logic, parameterize tests around second-call behavior, and split rstest function collection from the LateLintPass implementation while keeping behavior identical and adding some configurability in tests and BDD steps. Sequence diagram for refactored rstest function collectionsequenceDiagram
participant LintDriver
participant RstestHelperShouldBeFixture as Helper
participant LateContext as Cx
participant CallSiteVisitor as Visitor
LintDriver->>Helper: check_fn(cx, kind, decl, body, span, def_id)
Helper->>Helper: collect_rstest_fn(cx, body, def_id)
Helper->>Cx: local_def_id_to_hir_id(def_id)
Helper->>Cx: typeck(def_id)
Helper->>Visitor: new(cx, collector, def_id, fixture_locals)
Helper->>Visitor: visit_expr(body.value)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
check_crate_post, the innerfor record in recordsloop was removed but the body still appears to userecord, so either reintroduce the iteration or adjust the debug logic to avoid referencing an undefined variable and to ensure all records are still processed.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `check_crate_post`, the inner `for record in records` loop was removed but the body still appears to use `record`, so either reintroduce the iteration or adjust the debug logic to avoid referencing an undefined variable and to ensure all records are still processed.
## Individual Comments
### Comment 1
<location path="crates/rstest_helper_should_be_fixture/src/driver.rs" line_range="203" />
<code_context>
for (callee, records) in self.collector.iter() {
- for record in records {
+
debug!(
target: LINT_NAME,
"collected rstest helper call: callee={}, callee_def_id={:?}, \
</code_context>
<issue_to_address>
**issue (bug_risk):** The loop over `records` appears to have been removed, breaking iteration and likely compilation.
`check_crate_post` used to bind `record` via `for record in records { ... }`, but that loop is now gone while `record` is still used in the body. This will fail to compile (undefined `record`) and also prevents iterating over all `records`. Reintroduce a loop over `records` or otherwise adjust the logic to process each record correctly.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| for (callee, records) in self.collector.iter() { | ||
| for record in records { | ||
|
|
||
| debug!( |
There was a problem hiding this comment.
issue (bug_risk): The loop over records appears to have been removed, breaking iteration and likely compilation.
check_crate_post used to bind record via for record in records { ... }, but that loop is now gone while record is still used in the body. This will fail to compile (undefined record) and also prevents iterating over all records. Reintroduce a loop over records or otherwise adjust the logic to process each record correctly.
e4830a2 to
a6ad47c
Compare
b81b889 to
a37bc88
Compare
Code changes was requested by @leynos.
The following files were modified:
crates/rstest_helper_should_be_fixture/src/collector_behaviour.rscrates/rstest_helper_should_be_fixture/src/collector_tests.rscrates/rstest_helper_should_be_fixture/src/driver.rsSummary by Sourcery
Refine rstest helper call collection behaviour and testing for duplicate and distinct call sites.
Bug Fixes:
Enhancements:
Tests: