Enrich panic detection for interpolated panics in tests (PanicInfo)#190
Merged
Enrich panic detection for interpolated panics in tests (PanicInfo)#190
Conversation
OverviewThis PR enriches panic detection to distinguish between plain and interpolated panics in tests, enabling selective exemption of Key ChangesCore Panic Detection (panic_detector.rs)
Policy Updates (policy.rs)
Test Harness Integration (driver.rs)
Public API Additions (src/hir.rs)Two new public functions centralise harness test-function detection:
These replace recursive helpers in Test Coverage
Documentation
Impact
Validation
WalkthroughRecord harness test HIR ids when running under Changes
Sequence Diagram(s)sequenceDiagram
participant Driver as Driver (lint driver)
participant HIR as HIR helper
participant Detector as Panic Detector
participant Policy as Policy Checker
rect rgba(200,200,255,0.5)
Driver->>HIR: On crate check if `--test`\ncollect_harness_test_functions(cx)
HIR-->>Driver: Return set of harness test HirIds
end
rect rgba(200,255,200,0.5)
Driver->>Detector: On expr: closure_panics(cx, body_id)
Detector->>Detector: Traverse closure HIR\nidentify panic/unwrap/expect sites
Detector->>Detector: Inspect panic args for\n`fmt::Arguments::new_v1/_formatted` calls
Detector-->>Driver: Return PanicInfo{panics, has_plain_panic, has_interpolated_panic}
end
rect rgba(255,200,200,0.5)
Driver->>Driver: If `--test` recompute\nContextSummary.is_test via parent traversal
Driver->>Policy: should_flag(&policy, &summary, &panic_info, is_doctest)
Policy->>Policy: If !panic_info.panics => false
Policy->>Policy: If summary.is_test && panic_info.is_interpolated_only() => false
Policy->>Policy: Apply other suppressions (doctest, allow_in_main)
Policy-->>Driver: Return decision
Driver->>Driver: Emit diagnostic if decision == true
end
Poem
🚥 Pre-merge checks | ✅ 6 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Reviewer's GuideExtends the no_unwrap_or_else_panic lint so it distinguishes between plain and interpolating panics, threads a new PanicInfo struct through the detection and policy layers, and relaxes the policy specifically for interpolating panics in test contexts while keeping production behaviour strict; also introduces harness-based test detection plus updated tests, UI tests, and docs. Sequence diagram for lint evaluation using PanicInfo and test detectionsequenceDiagram
participant CX as LateContext
participant Pass as NoUnwrapOrElsePanic
participant PD as panic_detector_closure_panics
participant Summ as summarise_context
participant Harness as collect_and_check_harness_tests
participant Policy as should_flag
participant Lint as emit_lint
CX->>Pass: check_expr(cx, expr)
Pass->>Pass: closure_body(expr)
alt no_closure_body
Pass-->>CX: return
else has_closure
Pass->>Summ: summarise_context(cx, expr.hir_id)
Summ-->>Pass: ContextSummary
Pass->>Harness: collect_harness_test_functions(cx) [once per crate]
Harness-->>Pass: HashSet hir_HirId
alt summary_is_not_test_and_is_test_harness
Pass->>Harness: is_inside_harness_test_function(cx, expr, harness_test_functions)
Harness-->>Pass: bool is_test
Pass->>Pass: summary.is_test = is_test
end
Pass->>PD: closure_panics(cx, body_id)
PD->>PD: visit_body(body)
PD->>PD: visit_expr(expr)
alt panic_call_detected
PD->>PD: panic_args_use_interpolation(expr)
PD-->>Pass: PanicInfo { panics true, uses_interpolation bool }
else unwrap_or_expect_detected
PD-->>Pass: PanicInfo { panics true, uses_interpolation false }
else no_panic
PD-->>Pass: PanicInfo { panics false, uses_interpolation false }
end
Pass->>Policy: should_flag(policy, summary, panic_info, is_doctest)
Policy-->>Pass: bool flag
alt flag_is_false
Pass-->>CX: return
else flag_is_true
Pass->>Lint: emit_lint(cx, expr)
Lint-->>Pass: ok
Pass-->>CX: return
end
end
Class diagram for updated panic detection and policy typesclassDiagram
class NoUnwrapOrElsePanic {
- LintPolicy policy
- Localizer localizer
- bool is_doctest
- bool is_test_harness
- HashSet~hir_HirId~ harness_test_functions
+ check_expr(cx, expr)
}
class PanicInfo {
+ bool panics
+ bool uses_interpolation
}
class PanicDetector {
- LateContext cx
- bool panics
- bool uses_interpolation
+ visit_body(body)
+ visit_expr(expr)
}
class RuntimeArgsFinder {
- bool found
+ visit_expr(expr)
}
class LintPolicy {
+ bool allow_in_main
+ new(allow_in_main bool) LintPolicy
+ default() LintPolicy
}
class ContextSummary {
+ bool is_test
+ bool in_main
}
class panic_detector_module {
+ closure_panics(cx, body_id) PanicInfo
+ is_unwrap_or_expect(cx, expr) bool
+ is_panic_call(cx, expr) bool
+ panic_args_use_interpolation(expr) bool
+ is_fmt_args_runtime_call(callee) bool
}
class policy_module {
+ should_flag(policy, summary, panic_info, is_doctest) bool
}
NoUnwrapOrElsePanic --> LintPolicy : uses
NoUnwrapOrElsePanic --> ContextSummary : builds
NoUnwrapOrElsePanic --> panic_detector_module : calls
NoUnwrapOrElsePanic --> policy_module : calls
panic_detector_module --> PanicDetector : constructs
PanicDetector --> PanicInfo : populates
panic_detector_module --> RuntimeArgsFinder : constructs
RuntimeArgsFinder --> PanicDetector : informs_uses_interpolation
policy_module --> LintPolicy : reads
policy_module --> ContextSummary : reads
policy_module --> PanicInfo : reads
Flow diagram for updated should_flag policy logicflowchart TD
A[Start should_flag] --> B{panic_info.panics}
B -->|false| Z[Return false]
B -->|true| C{is_doctest}
C -->|true| Z
C -->|false| D{summary.is_test}
D -->|true| E{panic_info.uses_interpolation}
E -->|true| Z
E -->|false| F{summary.in_main}
D -->|false| F
F -->|true| G{policy.allow_in_main}
G -->|true| Z
G -->|false| H[Return true]
F -->|false| H
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
PanicDetector::visit_expr,uses_interpolationis overwritten on every panic call; if a closure has both interpolating and non-interpolating panics, the final value will depend on traversal order—consider OR-ing (self.uses_interpolation |= panic_args_use_interpolation(expr)) instead of assigning so any interpolating panic is preserved. - In
is_fmt_args_runtime_call, you currently allocate aStringviaident.name.as_str()and searchFMT_ARGS_RUNTIME_METHODS; this can be simplified and made cheaper by matching directly on symbols (e.g.matches!(segment.ident.name, sym::new_v1 | sym::new_v1_formatted)) instead of using string-based membership.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `PanicDetector::visit_expr`, `uses_interpolation` is overwritten on every panic call; if a closure has both interpolating and non-interpolating panics, the final value will depend on traversal order—consider OR-ing (`self.uses_interpolation |= panic_args_use_interpolation(expr)`) instead of assigning so any interpolating panic is preserved.
- In `is_fmt_args_runtime_call`, you currently allocate a `String` via `ident.name.as_str()` and search `FMT_ARGS_RUNTIME_METHODS`; this can be simplified and made cheaper by matching directly on symbols (e.g. `matches!(segment.ident.name, sym::new_v1 | sym::new_v1_formatted)`) instead of using string-based membership.
## Individual Comments
### Comment 1
<location path="crates/no_unwrap_or_else_panic/src/driver.rs" line_range="149" />
<code_context>
+/// and source span as each test function. This function scans for those
+/// descriptors to recover test-function identity after the harness has consumed
+/// the original `#[test]` attributes.
+fn collect_harness_test_functions<'tcx>(cx: &LateContext<'tcx>) -> HashSet<hir::HirId> {
+ let root_items: Vec<_> = cx
+ .tcx
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying harness test detection by replacing the recursive item-group scan with a single linear pass and wrapping `summarise_context` in a harness-aware helper so test logic lives in one place.
The main incidental complexity here comes from (a) the recursive/O(n²) harness collection and (b) the split responsibility for “is this a test?” across `check_expr` and `summarise_context`. You can keep all behaviour but simplify both with small, focused changes.
### 1. Replace recursive `collect_in_item_group` with a single linear scan
You don’t need to recurse modules or allocate `Vec`s per module. `hir_crate_items(()).items()` already gives you all items; you can do a single pass that:
* records all `(name, span)` for harness descriptors (`const`s), then
* collects all `fn`s whose `(name, span)` appears in that set.
This removes recursion and the per-module O(n²) sibling search.
```rust
fn collect_harness_test_functions<'tcx>(cx: &LateContext<'tcx>) -> HashSet<hir::HirId> {
use rustc_data_structures::fx::FxHashSet;
use rustc_span::Span;
let mut descriptors: FxHashSet<(Symbol, Span)> = FxHashSet::default();
let mut candidate_fns: Vec<(hir::HirId, Symbol, Span)> = Vec::new();
let crate_items = cx.tcx.hir_crate_items(());
for item_id in crate_items.items() {
let item = cx.tcx.hir_item(item_id);
match &item.kind {
hir::ItemKind::Fn(..) => {
if let Some(ident) = item.kind.ident() {
candidate_fns.push((item.hir_id(), ident.name, item.span));
}
}
hir::ItemKind::Const(..) => {
if let Some(ident) = item.kind.ident() {
descriptors.insert((ident.name, item.span));
}
}
_ => {}
}
}
candidate_fns
.into_iter()
.filter_map(|(hir_id, name, span)| {
descriptors.contains(&(name, span)).then_some(hir_id)
})
.collect()
}
```
Then you can drop `collect_in_item_group` and `is_test_descriptor` entirely, reducing the number of moving parts while keeping the same “match `(name, span)` of fn to synthesized const” behaviour.
### 2. Centralise harness-awareness in a helper `summarise_context` wrapper
The current flow:
```rust
let mut summary = summarise_context(cx, expr.hir_id);
if !summary.is_test && self.is_test_harness {
summary.is_test =
is_inside_harness_test_function(cx, expr, &self.harness_test_functions);
}
```
spreads test-detection logic between `check_expr` and the summariser. You can move this into a small helper that returns a fully-derived `ContextSummary`, so `check_expr` doesn’t need to mutate it:
```rust
fn summarise_context_with_harness<'tcx>(
cx: &LateContext<'tcx>,
hir_id: hir::HirId,
is_test_harness: bool,
harness_test_functions: &HashSet<hir::HirId>,
) -> ContextSummary {
let mut summary = summarise_context(cx, hir_id);
if !summary.is_test && is_test_harness {
summary.is_test = is_inside_harness_test_function(
cx,
hir_id,
harness_test_functions,
);
}
summary
}
```
Change `is_inside_harness_test_function` to use `hir_id` instead of the full `Expr`:
```rust
fn is_inside_harness_test_function<'tcx>(
cx: &LateContext<'tcx>,
hir_id: hir::HirId,
harness_test_functions: &HashSet<hir::HirId>,
) -> bool {
cx.tcx
.hir_parent_iter(hir_id)
.any(|(_, node)| matches!(node, hir::Node::Item(item)
if matches!(item.kind, hir::ItemKind::Fn { .. })
&& harness_test_functions.contains(&item.hir_id())))
}
```
And in `check_expr`:
```rust
let summary = summarise_context_with_harness(
cx,
expr.hir_id,
self.is_test_harness,
&self.harness_test_functions,
);
let panic_info = closure_panics(cx, body_id);
if !should_flag(&self.policy, &summary, &panic_info, self.is_doctest) {
return;
}
```
This keeps all your functionality (including harness-based tests) but:
* removes the recursive item-group machinery and extra allocations,
* makes harness test detection a single linear pass over the crate, and
* centralises “is this a test?” logic so `ContextSummary` is fully constructed in one place rather than being patched post hoc in `check_expr`.
</issue_to_address>
### Comment 2
<location path="docs/adr-002-dylint-expect-attribute-macro.md" line_range="31" />
<code_context>
- but toolchains can emit `unexpected_cfgs` diagnostics for unknown cfg keys/values
- when `check-cfg` validation is enabled.
+ but toolchains can emit `unexpected_cfgs` diagnostics for unknown cfg
+ keys/values when `check-cfg` validation is enabled.
The project needs an ergonomic, consistent, and low-friction mechanism for
</code_context>
<issue_to_address>
**issue (review_instructions):** The acronym “cfg” is used here without being defined on first use, which conflicts with the requirement to define uncommon acronyms.
The acronym “cfg” (configuration) appears here without definition. To comply with the instruction, consider defining it at its first occurrence in the document, for example: “configuration (`cfg`) keys/values”.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Define uncommon acronyms on first use.
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c2ef7c00dc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/no_unwrap_or_else_panic/src/driver.rs`:
- Around line 161-195: The function collect_in_item_group currently mixes
function filtering and module recursion; extract the module-descent block (the
for item in items { let hir::ItemKind::Mod ... collect_in_item_group(...) })
into a new helper, e.g., recurse_into_submodules(cx: &LateContext<'_>, items:
&[&hir::Item<'_>], marked: &mut HashSet<hir::HirId>), move the module_items
construction and recursive call into that helper, and replace the original
recursion block in collect_in_item_group with a single call to
recurse_into_submodules to improve clarity and separation of concerns.
In `@crates/no_unwrap_or_else_panic/tests/features/policy.feature`:
- Around line 42-46: Remove the duplicate scenario titled "Scenario: Plain panic
inside a test is still flagged" from the feature file (it duplicates the earlier
test panic scenario) — either delete this entire scenario block or merge its
Given/And/When/Then steps into the existing test panic scenario so there is only
one scenario asserting the same lint trigger for a plain panic inside a test;
update or remove any redundant example data within the duplicated scenario to
avoid repetition.
🪄 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
Run ID: 5ea2beb1-1c70-4e90-9f10-20aabfcdc633
📒 Files selected for processing (14)
crates/no_unwrap_or_else_panic/src/driver.rscrates/no_unwrap_or_else_panic/src/lib.rscrates/no_unwrap_or_else_panic/src/panic_detector.rscrates/no_unwrap_or_else_panic/src/policy.rscrates/no_unwrap_or_else_panic/src/tests/behaviour.rscrates/no_unwrap_or_else_panic/tests/features/policy.featurecrates/no_unwrap_or_else_panic/ui/bad_in_test.rscrates/no_unwrap_or_else_panic/ui/bad_in_test.stderrcrates/no_unwrap_or_else_panic/ui/bad_production_with_interpolation.rscrates/no_unwrap_or_else_panic/ui/bad_production_with_interpolation.rustc-flagscrates/no_unwrap_or_else_panic/ui/bad_production_with_interpolation.stderrcrates/no_unwrap_or_else_panic/ui/ok_in_test_with_interpolation.rscrates/no_unwrap_or_else_panic/ui/ok_in_test_with_interpolation.rustc-flagsdocs/adr-002-dylint-expect-attribute-macro.md
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
devboxerhub
bot
force-pushed
the
allow-unwrap-else-panic-in-tests-j92i13
branch
from
a714ea9 to
7094433
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/no_unwrap_or_else_panic/src/panic_detector.rs (1)
95-104:⚠️ Potential issue | 🟠 MajorStop classifying a closure from the first panic site only.
Keep walking until every panic/unwrap site in the closure has been seen. The current early exit makes
uses_interpolationdepend on HIR visitation order, so a closure containing bothpanic!("static")andpanic!("failed: {e}")can be exempted in tests even though it still has a plain panic path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/no_unwrap_or_else_panic/src/panic_detector.rs` around lines 95 - 104, The current visit_expr in PanicDetector stops traversal after the first panic (the early "if self.panics { return; }"), causing uses_interpolation to depend on HIR order; remove that early return and instead, when is_panic_call(expr) is true, set self.panics = true and update self.uses_interpolation = self.uses_interpolation || panic_args_use_interpolation(expr) (i.e., OR in any interpolation seen) but do not return—allow traversal to continue so every panic/unwrap site is seen; update visit_expr in PanicDetector accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/no_unwrap_or_else_panic/src/driver.rs`:
- Around line 149-211: Duplicate logic for detecting harness-generated test
descriptors (functions collect_harness_test_functions and is_test_descriptor)
should be factored into shared infrastructure and reused instead of kept in two
places; extract these functions from
crates/no_unwrap_or_else_panic/src/driver.rs into the common helper module used
by the other lint (the module containing the existing descriptor-matching rule
in crates/no_expect_outside_tests/src/driver/mod.rs), make their signatures and
visibility usable across crates (e.g., pub(crate) or pub as appropriate),
replace the local definitions in no_unwrap_or_else_panic::driver with calls to
the shared helpers (keeping the same parameters: LateContext, HirId/Symbol/Span,
sibling Item), update imports/usages (replace direct calls to
collect_harness_test_functions and is_test_descriptor with the shared module
path), and run cargo build to fix any visibility/type mismatches.
In `@crates/no_unwrap_or_else_panic/src/panic_detector.rs`:
- Around line 149-190: The current RuntimeArgsFinder marks any nested
fmt::Arguments constructor (new_v1/new_v1_formatted) anywhere inside a panic
payload as an interpolated message; instead, restrict detection to the actual
panic message expression itself by only considering fmt::Arguments constructors
that are the message argument of a panic invocation. Update
panic_args_use_interpolation / RuntimeArgsFinder so you walk the HIR to find
panic invocations (the call representing the panic entry point) and then inspect
only that invocation's message argument for is_fmt_args_runtime_call; ensure
is_fmt_args_runtime_call is still used to match a TypeRelative path segment
(new_v1/new_v1_formatted) but only when that expr is exactly the panic message
expression, not an arbitrary nested call like Foo::new_v1().
In `@crates/no_unwrap_or_else_panic/src/policy.rs`:
- Around line 73-83: Several near-duplicate tests that exercise combinations of
panicking/plain and interpolated/plain cases should be collapsed into a single
parameterised test using rstest; replace the multiple one-case tests (the cases
built from PanicInfo via the panicking(...) helper and the SAFE const) with a
single #[rstest] table that enumerates (PanicInfo, uses_interpolation,
expected_result) rows. Update the test module to import rstest::rstest, use the
panicking(uses_interpolation: bool) -> PanicInfo constructor and SAFE constant
as inputs inside the table, and assert the same expectations per row so the
matrix (panics yes/no × interpolated yes/no) is covered compactly; ensure you
keep existing assertion logic but move it into the single parameterised test
function to remove duplicated tests across lines ~85–163.
- Around line 22-25: The doc comment incorrectly describes the heuristic as
detecting `_fmt` or `_display`; update it to state that the detector keys off
construction of runtime-formatted arguments via
fmt::Arguments::new_v1(_formatted) — i.e., we permit `.unwrap_or_else(||
panic!(...))` in tests only when the panic invocation receives a runtime
`fmt::Arguments` (created via fmt::Arguments::new_v1(_formatted)), not merely
calls to `_fmt`/`_display`; change the wording in policy.rs to mention
fmt::Arguments::new_v1(_formatted) and that the check looks for that constructed
`fmt::Arguments` at the panic site for interpolation detection.
---
Outside diff comments:
In `@crates/no_unwrap_or_else_panic/src/panic_detector.rs`:
- Around line 95-104: The current visit_expr in PanicDetector stops traversal
after the first panic (the early "if self.panics { return; }"), causing
uses_interpolation to depend on HIR order; remove that early return and instead,
when is_panic_call(expr) is true, set self.panics = true and update
self.uses_interpolation = self.uses_interpolation ||
panic_args_use_interpolation(expr) (i.e., OR in any interpolation seen) but do
not return—allow traversal to continue so every panic/unwrap site is seen;
update visit_expr in PanicDetector accordingly.
🪄 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
Run ID: 9b53211f-c883-4956-93ce-c6a6302f68cb
📒 Files selected for processing (14)
crates/no_unwrap_or_else_panic/src/driver.rscrates/no_unwrap_or_else_panic/src/lib.rscrates/no_unwrap_or_else_panic/src/panic_detector.rscrates/no_unwrap_or_else_panic/src/policy.rscrates/no_unwrap_or_else_panic/src/tests/behaviour.rscrates/no_unwrap_or_else_panic/tests/features/policy.featurecrates/no_unwrap_or_else_panic/ui/bad_in_test.rscrates/no_unwrap_or_else_panic/ui/bad_in_test.stderrcrates/no_unwrap_or_else_panic/ui/bad_production_with_interpolation.rscrates/no_unwrap_or_else_panic/ui/bad_production_with_interpolation.rustc-flagscrates/no_unwrap_or_else_panic/ui/bad_production_with_interpolation.stderrcrates/no_unwrap_or_else_panic/ui/ok_in_test_with_interpolation.rscrates/no_unwrap_or_else_panic/ui/ok_in_test_with_interpolation.rustc-flagsdocs/adr-002-dylint-expect-attribute-macro.md
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/no_unwrap_or_else_panic/src/panic_detector.rs (1)
141-182:⚠️ Potential issue | 🟠 MajorRestrict interpolation detection to the panic message expression.
Walking the full panic subtree still treats any type-relative
new_v1(_formatted)call as interpolation, so payload-only cases such aspanic_any(Foo::new_v1())get exempted in tests even though no formatted panic message exists. Inspect only the actual panic message operand before settinguses_interpolation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/no_unwrap_or_else_panic/src/panic_detector.rs` around lines 141 - 182, The current panic_args_use_interpolation walks the entire panic subtree (panic_args_use_interpolation, RuntimeArgsFinder, is_fmt_args_runtime_call), which causes type-relative runtime format calls anywhere in the payload to be treated as interpolation; change it to only inspect the actual panic message operand. Modify panic_args_use_interpolation to locate the panic call's message argument (the format string / first formatting argument) and run rustc_hir::intravisit::walk_expr(&mut finder, message_expr) instead of walking the whole `expr`; keep RuntimeArgsFinder and is_fmt_args_runtime_call unchanged but ensure you pass only the message Expr to the visitor so payload-only calls like panic_any(Foo::new_v1()) are not considered interpolation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/no_unwrap_or_else_panic/src/policy.rs`:
- Around line 87-103: The test's rstest matrix passes six separate parameters to
policy_evaluation which trips the arity lint; define a single struct (e.g.,
PolicyCase) that groups allow_in_main, is_test, in_main, panic_info: PanicInfo,
is_doctest, expected: bool, update each #[case::...] entry to pass a single
PolicyCase instance, and change the test function signature to accept #[case]
case: PolicyCase (then destructure case to local variables or access fields
inside policy_evaluation). Modify references to the original parameters inside
policy_evaluation to use the PolicyCase fields and keep the test table-driven
while reducing parameter arity.
---
Duplicate comments:
In `@crates/no_unwrap_or_else_panic/src/panic_detector.rs`:
- Around line 141-182: The current panic_args_use_interpolation walks the entire
panic subtree (panic_args_use_interpolation, RuntimeArgsFinder,
is_fmt_args_runtime_call), which causes type-relative runtime format calls
anywhere in the payload to be treated as interpolation; change it to only
inspect the actual panic message operand. Modify panic_args_use_interpolation to
locate the panic call's message argument (the format string / first formatting
argument) and run rustc_hir::intravisit::walk_expr(&mut finder, message_expr)
instead of walking the whole `expr`; keep RuntimeArgsFinder and
is_fmt_args_runtime_call unchanged but ensure you pass only the message Expr to
the visitor so payload-only calls like panic_any(Foo::new_v1()) are not
considered interpolation.
🪄 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
Run ID: 1b02d652-7b21-4eb2-b0db-b3b6984686e8
📒 Files selected for processing (5)
crates/no_expect_outside_tests/src/driver/mod.rscrates/no_unwrap_or_else_panic/src/driver.rscrates/no_unwrap_or_else_panic/src/panic_detector.rscrates/no_unwrap_or_else_panic/src/policy.rssrc/hir.rs
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
crates/no_unwrap_or_else_panic/src/policy.rs (1)
67-103:⚠️ Potential issue | 🟠 MajorRemove the module-wide Clippy suppression and collapse test inputs into one case object.
Fix the root cause at Lines 97-103 and delete the blanket suppression at Line 67. Stop masking lint failures across the entire test module.
♻️ Proposed fix
#[cfg(test)] -#[allow(clippy::too_many_arguments)] mod tests { use super::*; use rstest::rstest; + + #[derive(Clone, Copy, Debug)] + struct PolicyCase { + policy: LintPolicy, + context: ContextSummary, + panic_info: PanicInfo, + is_doctest: bool, + should_flag: bool, + } @@ #[rstest] - #[case::safe_closure(LintPolicy::new(false), summary(false, false), SAFE, false, false)] + #[case::safe_closure(PolicyCase { + policy: LintPolicy::new(false), + context: summary(false, false), + panic_info: SAFE, + is_doctest: false, + should_flag: false, + })] @@ - fn policy_evaluation( - #[case] policy: LintPolicy, - #[case] context: ContextSummary, - #[case] panic_info: PanicInfo, - #[case] is_doctest: bool, - #[case] expected: bool, - ) { + fn policy_evaluation(#[case] case: PolicyCase) { assert_eq!( - should_flag(&policy, &context, &panic_info, is_doctest), - expected + should_flag( + &case.policy, + &case.context, + &case.panic_info, + case.is_doctest + ), + case.should_flag ); } }#!/bin/bash set -euo pipefail # Verify no blanket suppression remains in this module. rg -n --type=rust '#\[allow\(clippy::too_many_arguments\)\]' crates/no_unwrap_or_else_panic/src/policy.rs # Verify policy test uses a single case parameter. rg -nP --type=rust 'fn\s+policy_evaluation\s*\(\s*#\[case\]\s*case\s*:\s*PolicyCase\s*\)' crates/no_unwrap_or_else_panic/src/policy.rs # Verify current Clippy arity threshold config in repo (if present). fd -H 'clippy.toml|.clippy.toml' --exec sed -n '1,160p' {}As per coding guidelines, "
#[allow]is forbidden, suppressions must be narrowly scoped, and grouped inputs should reduce excessive function parameters in Rust tests."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/no_unwrap_or_else_panic/src/policy.rs` around lines 67 - 103, The module-level Clippy suppression #[allow(clippy::too_many_arguments)] should be removed and the many #[case] parameters collapsed into a single case object to avoid excessive test-arity; introduce a small struct PolicyCase (derive Clone, Copy, Debug) holding policy: LintPolicy, context: ContextSummary, panic_info: PanicInfo, is_doctest: bool, should_flag: bool, update each rstest case to pass a PolicyCase instance (use existing SAFE and summary helpers), and change the test function policy_evaluation to take #[case] case: PolicyCase and call should_flag(&case.policy, &case.context, &case.panic_info, case.is_doctest) comparing to case.should_flag; remove the module-wide #[allow(...)] so no blanket suppression remains.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/no_unwrap_or_else_panic/src/policy.rs`:
- Around line 89-96: The file's parameterized test attributes (the
#[case::safe_closure ...] .. #[case::flags_main_when_not_allowed ...] lines
referencing LintPolicy::new and panicking/summary helpers are not rustfmt'ed,
causing CI formatting failure; run rustfmt (cargo fmt) on
crates/no_unwrap_or_else_panic/src/policy.rs to reflow/wrap the #[case::...]
attributes, stage the reformatted file, and commit the changes so the CI
formatting check passes.
In `@src/hir.rs`:
- Around line 92-100: The current seed uses
cx.tcx.hir_crate_items(()).free_items() which gathers free items across the
whole crate and breaks the "same module" invariant; change
collect_harness_test_functions to start from the crate root module instead (use
cx.tcx.hir().krate() or the appropriate API to obtain the root module's items)
and pass that root module's item list into collect_in_item_group so recursion
proceeds module-by-module rather than comparing functions against unrelated
free_items; keep the rest of the logic (collect_in_item_group, marked HashSet)
unchanged.
---
Duplicate comments:
In `@crates/no_unwrap_or_else_panic/src/policy.rs`:
- Around line 67-103: The module-level Clippy suppression
#[allow(clippy::too_many_arguments)] should be removed and the many #[case]
parameters collapsed into a single case object to avoid excessive test-arity;
introduce a small struct PolicyCase (derive Clone, Copy, Debug) holding policy:
LintPolicy, context: ContextSummary, panic_info: PanicInfo, is_doctest: bool,
should_flag: bool, update each rstest case to pass a PolicyCase instance (use
existing SAFE and summary helpers), and change the test function
policy_evaluation to take #[case] case: PolicyCase and call
should_flag(&case.policy, &case.context, &case.panic_info, case.is_doctest)
comparing to case.should_flag; remove the module-wide #[allow(...)] so no
blanket suppression remains.
🪄 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
Run ID: cfbf1bf7-7336-4482-9ce2-ae518b92549a
📒 Files selected for processing (2)
crates/no_unwrap_or_else_panic/src/policy.rssrc/hir.rs
Owner
Author
|
@coderabbitai Have the following now been resolved? Annul any requirements that violate the en-GB-oxendict spelling (-ize / -yse / -our) conventions (for example a request to replace "normalize" with "normalise" or "artefact" with "artifact"), or where the requirement unnecessarily increases cyclomatic complexity. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/hir.rs`:
- Around line 117-139: The nested sibling scan in collect_in_item_group causes
O(n²) behavior; change it to a two-pass approach: first iterate items to build a
lookup (e.g., HashMap<(Symbol, Span), hir::HirId> or HashSet of keys) of
descriptor consts by detecting descriptor nodes the same way
is_harness_test_descriptor identifies them, then in the second pass iterate
function items (the existing filter matches) and perform O(1) lookups using
(ident.name, item.span) to decide whether to marked.insert(item.hir_id()); keep
recurse_into_submodules(cx, items, marked) as-is and reuse
is_harness_test_descriptor logic for key construction so behavior is unchanged.
🪄 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
Run ID: 9f76a2d3-6d10-4884-8fdb-88274b48da25
📒 Files selected for processing (2)
crates/no_unwrap_or_else_panic/src/policy.rssrc/hir.rs
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/hir.rs (1)
117-173:⚠️ Potential issue | 🟠 MajorReplace recursive module scans with a crate-wide linear two-pass collector.
Line 117 through Line 173 still recurse per module and still do per-function descriptor scans. Implement the agreed flat two-pass scan over crate items and centralize matching in one pass-built index, then one pass lookup. Remove recursive descent for this path to eliminate repeated module-level scans.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hir.rs` around lines 117 - 173, collect_in_item_group and recurse_into_submodules currently recurse per-module and perform per-function descriptor scans; replace this with a crate-wide flat two-pass collector: first traverse all crate items (not per-module recursion) to build a single index of harness descriptors (e.g. map from (name, source-equal span) or name -> Vec<(HirId, Span)>), then do a single second pass over all functions and mark matches by looking them up in that index (update the existing marked: HashSet<hir::HirId>); remove recurse_into_submodules and all recursive per-module logic, and update collect_in_item_group to accept or obtain the full crate item list (via the LateContext/tcx APIs) so matching is centralized and O(items+descriptors) instead of repeated module scans.crates/no_unwrap_or_else_panic/src/panic_detector.rs (1)
138-142:⚠️ Potential issue | 🟠 MajorRestrict interpolation detection to the panic message argument.
Fix the detector scope. Line 138 traverses the whole panic call subtree, and Line 155/Line 177 treat any
TypeRelative::*::new_v1(_)/new_v1_formatted(_)call as interpolation. This incorrectly exempts non-formatted payload paths (for example,panic_any(Foo::new_v1())) in tests.Gate interpolation checks by panic entry-point kind and inspect only the actual panic message expression.
Run this read-only verification. Expect output showing full-expression traversal and symbol-only matching without message-argument scoping.
#!/bin/bash set -euo pipefail echo "1) Show interpolation traversal scope:" rg -n -C3 'fn panic_args_use_interpolation|walk_expr\(&mut finder, expr\)' crates/no_unwrap_or_else_panic/src/panic_detector.rs echo echo "2) Show runtime constructor matcher details:" rg -n -C3 'fn is_fmt_args_runtime_call|QPath::TypeRelative|new_v1_formatted|new_v1' crates/no_unwrap_or_else_panic/src/panic_detector.rs echo echo "3) Show panic entry points including panic_any:" rg -n -C2 'panic_any|panic_fmt|panic_display|begin_panic_fmt' crates/no_unwrap_or_else_panic/src/panic_detector.rsAlso applies to: 154-161, 173-178
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/no_unwrap_or_else_panic/src/panic_detector.rs` around lines 138 - 142, The current interpolation detection walks the entire panic call subtree (panic_args_use_interpolation + RuntimeArgsFinder) and thus treats any TypeRelative::new_v1/_formatted call anywhere in the panic invocation as formatted; change it to first determine the panic entry-point kind (handle panic_any, panic_fmt, panic_display, begin_panic_fmt separately) and then only run the interpolation finder against the actual panic message expression (pass the message Expr to panic_args_use_interpolation or add a new helper that takes that Expr), and ensure is_fmt_args_runtime_call / QPath::TypeRelative checks are applied only within that scoped message-expression traversal so calls like panic_any(Foo::new_v1()) are not misclassified.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/no_unwrap_or_else_panic/src/panic_detector.rs`:
- Around line 138-142: The current interpolation detection walks the entire
panic call subtree (panic_args_use_interpolation + RuntimeArgsFinder) and thus
treats any TypeRelative::new_v1/_formatted call anywhere in the panic invocation
as formatted; change it to first determine the panic entry-point kind (handle
panic_any, panic_fmt, panic_display, begin_panic_fmt separately) and then only
run the interpolation finder against the actual panic message expression (pass
the message Expr to panic_args_use_interpolation or add a new helper that takes
that Expr), and ensure is_fmt_args_runtime_call / QPath::TypeRelative checks are
applied only within that scoped message-expression traversal so calls like
panic_any(Foo::new_v1()) are not misclassified.
In `@src/hir.rs`:
- Around line 117-173: collect_in_item_group and recurse_into_submodules
currently recurse per-module and perform per-function descriptor scans; replace
this with a crate-wide flat two-pass collector: first traverse all crate items
(not per-module recursion) to build a single index of harness descriptors (e.g.
map from (name, source-equal span) or name -> Vec<(HirId, Span)>), then do a
single second pass over all functions and mark matches by looking them up in
that index (update the existing marked: HashSet<hir::HirId>); remove
recurse_into_submodules and all recursive per-module logic, and update
collect_in_item_group to accept or obtain the full crate item list (via the
LateContext/tcx APIs) so matching is centralized and O(items+descriptors)
instead of repeated module scans.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ff9367be-efcf-449f-b5ed-296ad49002e7
📒 Files selected for processing (2)
crates/no_unwrap_or_else_panic/src/panic_detector.rssrc/hir.rs
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 outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
crates/no_unwrap_or_else_panic/src/panic_detector.rs (1)
144-197:⚠️ Potential issue | 🟠 MajorStop classifying arbitrary
*_::new_v1()payloads as interpolated panic messages.Line 175 still treats any type-relative
new_v1/new_v1_formattedcall under the first panic argument as runtime formatting. That misclassifies cases likepanic_any(MyType::new_v1())andpanic!(MyType::new_v1())as interpolated, so test closures using those payloads are exempted even though nofmt::Argumentsmessage was built. Match the compiler-generatedformat_args!constructor itself, not just the segment name.In rustc HIR for Rust 2021/2024, how can compiler-generated `fmt::Arguments::new_v1` / `new_v1_formatted` calls from `format_args!` be distinguished from arbitrary user-defined associated functions with the same segment name inside `panic!` arguments?src/hir.rs (1)
95-115: 🧹 Nitpick | 🔵 TrivialSeed traversal from the root module directly.
Remove the crate-wide
free_items()pre-scan at Line 96 and start from root-moduleitem_ids. Keep module-scoped recursion, and deleteis_crate_root_itemafterwards.♻️ Proposed refactor
pub fn collect_harness_test_functions(cx: &LateContext<'_>) -> HashSet<hir::HirId> { - let root_items: Vec<_> = cx - .tcx - .hir_crate_items(()) - .free_items() - .map(|id| cx.tcx.hir_item(id)) - .filter(|item| is_crate_root_item(cx, item)) - .collect(); + let root_module = cx.tcx.hir_root_module(); + let root_items: Vec<_> = root_module + .item_ids + .iter() + .map(|id| cx.tcx.hir_item(*id)) + .collect(); let mut marked = HashSet::new(); collect_in_item_group(cx, &root_items, &mut marked); marked } - -/// Returns `true` when the item's immediate parent is the crate root rather -/// than a nested module. -fn is_crate_root_item(cx: &LateContext<'_>, item: &hir::Item<'_>) -> bool { - cx.tcx - .hir_parent_iter(item.hir_id()) - .next() - .is_some_and(|(_, node)| matches!(node, hir::Node::Crate(_))) -}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hir.rs` around lines 95 - 115, The current collect_harness_test_functions starts by pre-scanning crate-wide free_items and filtering with is_crate_root_item; instead, seed traversal from the root module's item_ids and recurse module-scoped via collect_in_item_group, removing the free_items() pre-scan and deleting is_crate_root_item. Update collect_harness_test_functions to obtain the root module's Vec<hir::Item<'_>> via the crate root item_ids (using cx.tcx.hir_crate_items(...) or the appropriate root module API), call collect_in_item_group(cx, &root_items, &mut marked) the same as before, and remove the now-unused is_crate_root_item function and any filtering logic that depended on it so traversal originates from the root module only.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/no_unwrap_or_else_panic/src/panic_detector.rs`:
- Around line 39-41: PanicInfo currently collapses interpolation state into
uses_interpolation, which hides mixed panic paths; change PanicInfo to track two
booleans (e.g., has_plain_panic and has_interpolated_panic) instead of
uses_interpolation, update all code that sets/merges PanicInfo (including the
logic around the other occurrence at the 94-99 area) to set the appropriate
field when encountering interpolated vs plain panic sites, and modify
should_flag (and any test-suppression check) to only suppress flagging when
summary.is_test && has_interpolated_panic && !has_plain_panic so a closure with
any plain panic is still flagged.
In `@src/hir.rs`:
- Around line 87-90: Update the Rustdoc spelling: in the doc comment that starts
"The test harness synthesises a sibling `const` descriptor..." replace the
British-form "synthesises" with "synthesizes" so the comment conforms to the
project's en-GB-oxendict (-ize/-yse/-our) rule; locate the comment above the
function that scans for sibling `const` descriptors (the docblock describing
recovery of test-function identity) and make the single-word replacement.
---
Duplicate comments:
In `@src/hir.rs`:
- Around line 95-115: The current collect_harness_test_functions starts by
pre-scanning crate-wide free_items and filtering with is_crate_root_item;
instead, seed traversal from the root module's item_ids and recurse
module-scoped via collect_in_item_group, removing the free_items() pre-scan and
deleting is_crate_root_item. Update collect_harness_test_functions to obtain the
root module's Vec<hir::Item<'_>> via the crate root item_ids (using
cx.tcx.hir_crate_items(...) or the appropriate root module API), call
collect_in_item_group(cx, &root_items, &mut marked) the same as before, and
remove the now-unused is_crate_root_item function and any filtering logic that
depended on it so traversal originates from the root module only.
🪄 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
Run ID: 598798e8-6d4e-40c4-9449-e2876ac90346
📒 Files selected for processing (2)
crates/no_unwrap_or_else_panic/src/panic_detector.rssrc/hir.rs
Owner
Author
|
@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. Comment on file fn collect_in_item_group<'tcx>(
cx: &LateContext<'tcx>,
items: &[&'tcx hir::Item<'tcx>],
marked: &mut HashSet<hir::HirId>,
) {
// First pass: build a lookup of harness test descriptors by name.
// Descriptors are const items synthesised by the --test harness with the
// same name and source-equal span as their corresponding test functions.
let mut descriptors_by_name: HashMap<rustc_span::Symbol, Vec<(hir::HirId, Span)>> =
HashMap::new();
for item in items.iter().copied() {
if !matches!(item.kind, hir::ItemKind::Const(..)) {
continue;
}
let Some(ident) = item.kind.ident() else {
continue;
};
descriptors_by_name
.entry(ident.name)
.or_default()
.push((item.hir_id(), item.span));
}
// Second pass: check each function against the descriptor lookup.
// This is O(functions + descriptors × name_collisions) which in practice
// is much faster than O(functions × all_descriptors) since most names are
// unique.
for item in items
.iter()
.copied()
.filter(|item| matches!(item.kind, hir::ItemKind::Fn { .. }))
{
let Some(ident) = item.kind.ident() else {
continue;
};
let Some(candidates) = descriptors_by_name.get(&ident.name) else {
continue;
};
for &(desc_id, desc_span) in candidates {
if desc_id != item.hir_id() && desc_span.source_equal(item.span) {
marked.insert(item.hir_id());
break;
}
}
}
recurse_into_submodules(cx, items, marked);
}❌ New issue: Bumpy Road Ahead |
This comment was marked as resolved.
This comment was marked as resolved.
…review findings PanicInfo now distinguishes between plain and interpolated panic sites instead of collapsing them into a single uses_interpolation flag. This prevents false negatives when a test closure contains both diagnostic (interpolated) and unconditional (plain) panics - the lint now correctly flags closures with any plain panic even if interpolated panics are also present. Changes to panic detection: - PanicInfo: replaced `uses_interpolation: bool` with `has_plain_panic: bool` and `has_interpolated_panic: bool` - PanicDetector: sets appropriate field based on panic_args_use_interpolation result instead of using |= to collapse all panics - unwrap/expect calls are now correctly classified as plain panics - should_flag: only suppresses in tests when has_interpolated_panic && !has_plain_panic (interpolation-only), not when any interpolation exists Additional fixes: - Corrected British spelling "synthesises" → "synthesizes" (en-GB-oxendict) - Removed free_items() pre-scan and is_crate_root_item filtering in collect_harness_test_functions; now seeds traversal directly from the crate root module via hir_owner_nodes(CRATE_OWNER_ID), matching the pattern used in recurse_into_submodules All test cases and documentation updated to reflect the new PanicInfo structure. Verified with full test suite: all 1244 tests pass. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Issue 1: Flatten test harness collection to single pass - Replaced recursive module traversal with flat iteration over all_items.free_items() - Keyed descriptors by (name, parent_module) to maintain sibling semantics without recursion - Eliminated collect_in_item_group and recurse_into_submodules helper functions - Reduced complexity from recursive tree walk to two flat passes over all items Issue 2: Centralized context summarization with harness detection - Introduced summarise_context_with_harness helper that returns immutable ContextSummary - Merged attribute-based and harness-based test detection in single function - Eliminated post-hoc mutation of summary.is_test in check_expr - Removed now-unused is_inside_harness_test_function helper - Driver now receives fully-derived summary instead of mutating it Benefits: - Simpler control flow (flat iteration vs recursive descent) - Single source of truth for context summary derivation - Immutable summary passed to policy layer (no mutation) - Fewer helper functions and clearer separation of concerns Verified with full test suite: all 1244 tests pass. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Applies targeted fixes identified in code review: 1. **Spelling consistency (driver.rs:115):** Changed "Summarises" to "Summarizes" in doc comment to adhere to project's en-GB-oxendict spelling convention (-ize/-yse/-our rule). 2. **Test coverage (policy.rs:92-148):** Added new rstest case `mixed_plain_and_interpolated_in_tests` to verify that closures containing both plain and interpolated panics in test contexts are correctly flagged. This ensures the policy logic properly handles the edge case where `has_plain_panic` and `has_interpolated_panic` are both true. 3. **BDD step clarity (behaviour.rs:53-59):** Added new additive step `given_also_interpolating` that sets only `has_interpolated_panic` without clearing `has_plain_panic`. This preserves the existing `given_interpolating` step's semantics (which replaces plain with interpolated) while providing a composable alternative for testing mixed-panic scenarios. All changes verified with: - cargo fmt --check (formatting) - cargo clippy --all-targets --all-features (linting) - cargo test --features dylint-driver (1245 tests pass) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Extracts nested loop bodies into two helper functions to eliminate "Bumpy Road Ahead" code health warning from CodeScene. **Changes:** 1. Added `const_descriptor` helper that extracts harness-descriptor key/value pairs from const items, replacing the first loop's nested if/match/if-let chain. 2. Added `harness_matched_fn` helper that checks if a function item matches a harness descriptor, replacing the second loop's nested if/match/if-let/for/if chain. 3. Refactored `collect_harness_test_functions` to delegate to these helpers, eliminating all nested conditionals from the top-level function body. **Preserved invariants:** - (name, parent_module) keying strategy retained to guard against cross-module false positives - source_equal span comparison (not ==) preserved - HirId inequality check (desc_id != fn_hir_id) preserved - All 1245 tests pass without modification The refactoring reduces cyclomatic complexity while maintaining identical semantics and performance characteristics. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…raction Eliminates nested conditionals in collect_harness_test_functions to resolve CodeScene "Bumpy Road Ahead" warning (3 blocks at nesting depth ≥ 2). **Changes:** 1. Renamed `const_descriptor` to `descriptor_entry` and replaced chained `.then_some(())?` with early-return `if !matches!` guard for clarity. 2. Replaced `harness_matched_fn` with `fn_candidate` that returns a 4-tuple `(fn_id, name, parent, span)` instead of performing the descriptor lookup internally. This moves the matching logic into the caller's `filter_map`, creating a flat pipeline. 3. Refactored main function to use iterator-chain style with inline `filter_map` closure that performs descriptor matching, eliminating all nested conditionals from both passes. **Preserved invariants:** - source_equal span comparison (not ==) preserved - HirId inequality check (desc_id != fn_id) preserved - (name, parent_module) keying strategy retained - All 1245 tests pass without modification The refactoring reduces cyclomatic complexity while maintaining identical semantics. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Reduces conditional complexity in should_flag by moving the
interpolation-only check into a PanicInfo method, addressing CodeScene
"Complex Conditional" warning.
**Changes:**
1. Added PanicInfo::is_interpolated_only() method that encapsulates the
logic "has_interpolated_panic && !has_plain_panic", clarifying the
semantic intent.
2. Updated should_flag to use panic_info.is_interpolated_only() instead
of directly checking the two boolean fields, reducing the conditional
expression from 3 terms to 2.
**Rationale:**
The sub-expression "has_interpolated_panic && !has_plain_panic"
represents a domain property of PanicInfo ("contains only interpolated
panics") and belongs on that type. This improves readability and
encapsulation while reducing cyclomatic complexity in the policy
function.
**Preserved invariants:**
- Behaviour of should_flag identical for all inputs
- All 1245 tests pass without modification
- Only panic_detector.rs and policy.rs modified
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…tion check Fixes false positive in is_fmt_args_runtime_call where any type's new_v1 or new_v1_formatted method would be treated as format interpolation, incorrectly suppressing the lint for code like panic_any(MyType::new_v1()) in test contexts. **Changes:** 1. Thread LateContext through panic_args_use_interpolation and RuntimeArgsFinder to enable type checking. 2. Updated is_fmt_args_runtime_call to verify the receiver type is core::fmt::Arguments or std::fmt::Arguments via def_path_str check, not just checking the method name. **Rationale:** The previous implementation only checked method names (new_v1, new_v1_formatted), which could match user-defined types. This caused false negatives where panics with custom types would be incorrectly classified as interpolated, suppressing lint warnings in tests when they should have fired. **Testing:** - All 1245 tests pass - Verified with cargo clippy and make lint Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Apply cargo fmt's preferred formatting for struct initialization, collapsing the multi-line RuntimeArgsFinder initialization into a single line for consistency with project formatting standards. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…on after rebase Mergiraf auto-resolved a conflict during the rebase onto main by removing the local `is_matching_harness_test_descriptor` function (which our branch had moved to the shared `whitaker::hir` module), but this also orphaned the rstest companion module detection that main added in #195. Restructure the local harness detection to use the shared `collect_harness_test_functions` for standard const-descriptor matching, then augment with a focused `collect_rstest_companion_ test_functions` pass that finds functions whose same-named sibling module contains a harness descriptor (the pattern rstest case expansions produce). This preserves both our branch's shared harness scan and main's rstest companion module support. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
devboxerhub
bot
force-pushed
the
allow-unwrap-else-panic-in-tests-j92i13
branch
from
32e0923 to
23b6ff1
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…ation CodeScene flagged `descriptor_entry` and `fn_candidate` as duplicated structure. Both functions guard on an ItemKind variant, extract an ident, compute a parent HirId, and return a tuple built from (hir_id, name, parent, span). Extract the shared logic into `item_components`, a private helper parameterised on a kind-predicate closure. This leaves each existing function as a thin, purpose-named wrapper that transforms the canonical tuple into its domain-specific return shape. The signatures and call sites remain unchanged, preserving all existing behaviour while eliminating the structural duplication. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/hir.rs (1)
85-107: 🧹 Nitpick | 🔵 TrivialExtract shared item decomposition into one helper.
Remove duplicated extraction flow in
descriptor_entryandfn_candidateby centralizing ident/parent/span harvesting once, then mapping per kind.♻️ Proposed refactor
+fn item_components<'tcx>( + cx: &LateContext<'tcx>, + item: &hir::Item<'tcx>, +) -> Option<(hir::HirId, rustc_span::Symbol, hir::HirId, Span)> { + let ident = item.kind.ident()?; + let parent: hir::HirId = cx.tcx.hir_get_parent_item(item.hir_id()).into(); + Some((item.hir_id(), ident.name, parent, item.span)) +} + fn descriptor_entry<'tcx>( cx: &LateContext<'tcx>, item: &hir::Item<'tcx>, ) -> Option<((rustc_span::Symbol, hir::HirId), (hir::HirId, Span))> { if !matches!(item.kind, hir::ItemKind::Const(..)) { return None; } - let ident = item.kind.ident()?; - let parent: hir::HirId = cx.tcx.hir_get_parent_item(item.hir_id()).into(); - Some(((ident.name, parent), (item.hir_id(), item.span))) + let (id, name, parent, span) = item_components(cx, item)?; + Some(((name, parent), (id, span))) } fn fn_candidate<'tcx>( cx: &LateContext<'tcx>, item: &hir::Item<'tcx>, ) -> Option<(hir::HirId, rustc_span::Symbol, hir::HirId, Span)> { if !matches!(item.kind, hir::ItemKind::Fn { .. }) { return None; } - let ident = item.kind.ident()?; - let parent: hir::HirId = cx.tcx.hir_get_parent_item(item.hir_id()).into(); - Some((item.hir_id(), ident.name, parent, item.span)) + item_components(cx, item) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hir.rs` around lines 85 - 107, Extract the duplicated ident/parent/span extraction into a single helper (e.g., fn decompose_item<'tcx>(item: &hir::Item<'tcx>, cx: &LateContext<'tcx>) -> Option<(rustc_span::Symbol, hir::HirId, hir::HirId, Span)>), then have descriptor_entry and fn_candidate call that helper and map its returned tuple into their respective return shapes; update descriptor_entry to map (name, parent, id, span) -> Some(((name, parent), (id, span))) and fn_candidate to map -> Some((id, name, parent, span)), preserving the same Option semantics and using the same ident() and hir_get_parent_item calls only in the helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/no_expect_outside_tests/src/driver/mod.rs`:
- Around line 263-269: The crate-wide flat collection in root_items (built from
cx.tcx.hir_crate_items(()).free_items()) allows items from different modules to
be treated as siblings; modify collect_companion_in_group invocation so
companions are matched only within the same module scope — either (A) seed the
walk per-module (iterate modules and use module.item_ids when building the items
passed into collect_companion_in_group) or (B) when matching siblings (the logic
currently applied around the root_items sibling check), filter by the
parent/module id (compare parent scope/module of the candidate to the current
module) so only items from the same module are considered; use the same
module-scoped approach as the recursive path that uses module.item_ids and
mirror the pattern from whitaker::hir::collect_harness_test_functions.
In `@crates/no_unwrap_or_else_panic/src/driver.rs`:
- Around line 64-69: When initializing harness_test_functions, extend the
current whitaker::hir::collect_harness_test_functions(cx) result with the same
rstest companion-module augmentation used in the other driver: build the HashSet
by collecting collect_harness_test_functions(cx) and then merge/extend it with
the rstest companion-module-derived test function IDs (i.e. call the same helper
used to augment rstest-generated cases and union those results into
harness_test_functions), keeping the is_test_harness conditional and HashSet
fallback for non-test builds.
In `@crates/no_unwrap_or_else_panic/src/panic_detector.rs`:
- Around line 111-123: The visitor stops traversing into a panic! call once it
classifies it, so nested plain panics or unwrap/expect inside the panic
arguments are missed; modify the branch handling in panic_detector so that after
you detect is_panic_call(self.cx, expr) and set self.panics plus either
self.has_interpolated_panic or self.has_plain_panic (via
panic_args_use_interpolation), you still continue traversal into the expression
(i.e., call rustc_hir::intravisit::walk_expr(self, expr)); similarly ensure
is_unwrap_or_expect branch still sets flags and then walks the expression; in
short, do not short-circuit traversal—always call
rustc_hir::intravisit::walk_expr(self, expr) after classifying so nested panic
sites are recorded.
---
Duplicate comments:
In `@src/hir.rs`:
- Around line 85-107: Extract the duplicated ident/parent/span extraction into a
single helper (e.g., fn decompose_item<'tcx>(item: &hir::Item<'tcx>, cx:
&LateContext<'tcx>) -> Option<(rustc_span::Symbol, hir::HirId, hir::HirId,
Span)>), then have descriptor_entry and fn_candidate call that helper and map
its returned tuple into their respective return shapes; update descriptor_entry
to map (name, parent, id, span) -> Some(((name, parent), (id, span))) and
fn_candidate to map -> Some((id, name, parent, span)), preserving the same
Option semantics and using the same ident() and hir_get_parent_item calls only
in the helper.
🪄 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
Run ID: 95b9180c-ae75-4ca3-9364-97587bd132da
📒 Files selected for processing (16)
crates/no_expect_outside_tests/src/driver/mod.rscrates/no_unwrap_or_else_panic/src/driver.rscrates/no_unwrap_or_else_panic/src/lib.rscrates/no_unwrap_or_else_panic/src/panic_detector.rscrates/no_unwrap_or_else_panic/src/policy.rscrates/no_unwrap_or_else_panic/src/tests/behaviour.rscrates/no_unwrap_or_else_panic/tests/features/policy.featurecrates/no_unwrap_or_else_panic/ui/bad_in_test.rscrates/no_unwrap_or_else_panic/ui/bad_in_test.stderrcrates/no_unwrap_or_else_panic/ui/bad_production_with_interpolation.rscrates/no_unwrap_or_else_panic/ui/bad_production_with_interpolation.rustc-flagscrates/no_unwrap_or_else_panic/ui/bad_production_with_interpolation.stderrcrates/no_unwrap_or_else_panic/ui/ok_in_test_with_interpolation.rscrates/no_unwrap_or_else_panic/ui/ok_in_test_with_interpolation.rustc-flagsdocs/adr-002-dylint-expect-attribute-macro.mdsrc/hir.rs
…on after rebase Move rstest companion test function detection from the no_expect_outside_tests driver into the shared whitaker::hir module, fixing a scoping bug: the original used hir_crate_items().free_items() which flattens items across all modules, allowing false sibling matches between items in different modules. The shared version starts from hir_root_module().item_ids and recurses per-module. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Augment the harness test function set with rstest companion module detection, matching the pattern already used by no_expect_outside_tests. Without this, rstest-parametrised test functions were not recognised as tests and unwrap_or_else closures inside them would trigger false positives. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…classification Make walk_expr unconditional in PanicDetector::visit_expr so that nested panic sites within arguments of already-classified calls are not missed. Previously walk_expr was in an else branch, meaning child expressions of panic!/unwrap/expect calls were never visited. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/no_unwrap_or_else_panic/src/panic_detector.rs (1)
18-18:⚠️ Potential issue | 🟠 MajorExclude
panic_anyfrom interpolation detection.
panic_anyaccepts an arbitrary payload, not a formatted panic message. The current logic inpanic_args_use_interpolationcannot distinguish betweenpanic!("{x}")(a formatted diagnostic) andpanic_any(format_args!("{x}"))(arbitrary payload), causing the latter to be misclassified as "interpolated-only" and incorrectly exempted in tests. Removepanic_anyfrom the interpolation check or treat allpanic_anycalls as plain panics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/no_unwrap_or_else_panic/src/panic_detector.rs` at line 18, The interpolation detection wrongly treats panic_any as a formatted panic; update the logic in panic_args_use_interpolation to exclude or special-case panic_any so it is not considered for interpolation detection: remove "panic_any" from the &["core","panicking","panic_any"] target list (or explicitly treat calls to panic_any as plain panics) and ensure any matching/inspection paths in panic_args_use_interpolation and related match arms treat panic_any as non-interpolated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@crates/no_unwrap_or_else_panic/src/panic_detector.rs`:
- Line 18: The interpolation detection wrongly treats panic_any as a formatted
panic; update the logic in panic_args_use_interpolation to exclude or
special-case panic_any so it is not considered for interpolation detection:
remove "panic_any" from the &["core","panicking","panic_any"] target list (or
explicitly treat calls to panic_any as plain panics) and ensure any
matching/inspection paths in panic_args_use_interpolation and related match arms
treat panic_any as non-interpolated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e5f734e5-c40f-4dad-861e-05c065ce855b
📒 Files selected for processing (4)
crates/no_expect_outside_tests/src/driver/mod.rscrates/no_unwrap_or_else_panic/src/driver.rscrates/no_unwrap_or_else_panic/src/panic_detector.rssrc/hir.rs
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 22028
Overall Comment 2 — Symbol matching in
|
…nts lang item Replace string-based def_path_str comparison against "core::fmt::Arguments" / "std::fmt::Arguments" with a lang-item check via cx.tcx.lang_items().format_arguments(). This eliminates the fragile string match and ensures the receiver type is verified against the canonical compiler definition. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace nested conditional logic with a self-contained two-pass flat scan in `collect_harness_test_functions`. The previous implementation delegated to `collect_in_item_group` and `recurse_into_submodules`, producing nested conditionals flagged by CodeScene. The new implementation directly iterates over all free items twice: once to collect descriptors (const items), and once to collect candidate functions. Filtering then matches functions against descriptors using the same invariants (parent equality, span source_equal, and HirId inequality). Remove `HashMap`, `descriptor_entry`, `fn_candidate`, `is_crate_root_item`, `collect_in_item_group`, and `recurse_into_submodules`, which are no longer needed. In `no_unwrap_or_else_panic/src/driver.rs`, centralize context derivation by extracting `is_inside_harness_test_function` as a separate helper accepting `hir_id` directly. The helper is called from `summarise_context_with_harness`, consolidating test detection logic into a single call site rather than post-hoc mutation. All 1293 tests pass without modification. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/no_unwrap_or_else_panic/src/panic_detector.rs (1)
18-18:⚠️ Potential issue | 🟠 MajorExclude
panic_anyfrom interpolation detection; treat all invocations as plain panics.The first argument to
panic_anyis an arbitrary payload (M: 'static + Any + Send), not a formatted panic message. Classify allpanic_anycalls as plain (non-interpolated) panics regardless of the payload type. Currently,panic_any(format_args!("{value}"))is misclassified as interpolated-only and incorrectly exempted from test linting, even though it falls outside thepanic!(...)exception.Exclude
panic_anyfrom the call graph inpanic_args_use_interpolation()before inspecting the first argument. Add an early return that detectspanic_anycalls (matching pathscore::panicking::panic_any,std::panicking::panic_any, andstd::panic::panic_any) and returnsfalseto classify them as plain panics.🛠️ Proposed fix
fn panic_args_use_interpolation<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>) -> bool { - // Extract the panic message argument (first argument to the panic call). - let ExprKind::Call(_, args) = expr.kind else { + let ExprKind::Call(callee, args) = expr.kind else { return false; }; + if is_panic_any_call(cx, callee) { + return false; + } let Some(message_arg) = args.first() else { return false; }; @@ rustc_hir::intravisit::walk_expr(&mut finder, message_arg); finder.found } + +fn is_panic_any_call(cx: &LateContext<'_>, callee: &Expr<'_>) -> bool { + let Some(def_id) = def_id_of_callee(cx, callee) else { + return false; + }; + + let path = SimplePath::from(cx.tcx.def_path_str(def_id).as_str()); + whitaker_common::is_path_to(&path, ["core", "panicking", "panic_any"].into_iter()) + || whitaker_common::is_path_to(&path, ["std", "panicking", "panic_any"].into_iter()) + || whitaker_common::is_path_to(&path, ["std", "panic", "panic_any"].into_iter()) +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/no_unwrap_or_else_panic/src/panic_detector.rs` at line 18, The function panic_args_use_interpolation() currently treats calls to panic_any like other panics and may inspect their first argument; change it to detect panic_any early by checking the call path for "core::panicking::panic_any", "std::panicking::panic_any", or "std::panic::panic_any" and immediately return false so panic_any is classified as a plain (non-interpolated) panic; update the call-graph/pre-check logic in panic_args_use_interpolation() to exclude panic_any before any argument inspection.
♻️ Duplicate comments (1)
src/hir.rs (1)
107-135:⚠️ Potential issue | 🟠 MajorMatch harness descriptors by parent module.
Guard the flat scan with the parent HIR id as well as name and
source_equalspan. The current key can cross-match unrelated macro-generated items in different modules, which marks non-test functions as harness tests and suppresses diagnostics under--test.🛠️ Proposed fix
- let mut descriptors: Vec<(rustc_span::Symbol, Span, hir::HirId)> = Vec::new(); - let mut candidate_fns: Vec<(hir::HirId, rustc_span::Symbol, Span)> = Vec::new(); + let mut descriptors: Vec<(rustc_span::Symbol, Span, hir::HirId, hir::HirId)> = Vec::new(); + let mut candidate_fns: Vec<(hir::HirId, rustc_span::Symbol, Span, hir::HirId)> = Vec::new(); for item_id in cx.tcx.hir_crate_items(()).free_items() { let item = cx.tcx.hir_item(item_id); - let Some(ident) = item.kind.ident() else { - continue; - }; - - match item.kind { - hir::ItemKind::Const(..) => { - descriptors.push((ident.name, item.span, item.hir_id())); - } - hir::ItemKind::Fn { .. } => { - candidate_fns.push((item.hir_id(), ident.name, item.span)); - } - _ => {} - } + if let Some((hir_id, name, parent, span)) = + item_components(cx, item, |kind| matches!(kind, hir::ItemKind::Const(..))) + { + descriptors.push((name, span, hir_id, parent)); + } else if let Some((hir_id, name, parent, span)) = + item_components(cx, item, |kind| matches!(kind, hir::ItemKind::Fn { .. })) + { + candidate_fns.push((hir_id, name, span, parent)); + } } candidate_fns .into_iter() - .filter(|(fn_id, name, span)| { - descriptors.iter().any(|(desc_name, desc_span, desc_id)| { - desc_id != fn_id && *desc_name == *name && desc_span.source_equal(*span) + .filter(|(fn_id, name, span, parent)| { + descriptors.iter().any(|(desc_name, desc_span, desc_id, desc_parent)| { + desc_id != fn_id + && *desc_parent == *parent + && *desc_name == *name + && desc_span.source_equal(*span) }) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hir.rs` around lines 107 - 135, The matching needs to also require the parent HIR id to avoid cross-module/macro collisions: when building descriptors and candidate_fns, capture each item's parent HIR id (in addition to ident.name, span and hir_id) and then change the filter to require desc_parent == fn_parent && desc_id != fn_id && desc_name == name && desc_span.source_equal(*span); update the tuples in descriptors and candidate_fns to include the parent id and use those fields (e.g., replace descriptors: Vec<(rustc_span::Symbol, Span, hir::HirId)> and candidate_fns: Vec<(hir::HirId, rustc_span::Symbol, Span)> with versions that include the parent id, and adjust the closure to compare desc_parent with fn_parent).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/hir.rs`:
- Around line 195-208: The helper has_companion_test_module currently treats any
sibling mod with the same name as the function as a test module regardless of
which test inside it matches; update it to require the module actually contains
a harness descriptor for the same function_name by passing function_name into
module_has_harness_descriptor (change module_has_harness_descriptor signature to
accept the function_name/target symbol and update all call sites, including the
other occurrences noted around lines 210-241) and make has_companion_test_module
call module_has_harness_descriptor(cx, sibling, function_name) so sibling mods
are accepted only when they prove a harness for that exact function name.
---
Outside diff comments:
In `@crates/no_unwrap_or_else_panic/src/panic_detector.rs`:
- Line 18: The function panic_args_use_interpolation() currently treats calls to
panic_any like other panics and may inspect their first argument; change it to
detect panic_any early by checking the call path for
"core::panicking::panic_any", "std::panicking::panic_any", or
"std::panic::panic_any" and immediately return false so panic_any is classified
as a plain (non-interpolated) panic; update the call-graph/pre-check logic in
panic_args_use_interpolation() to exclude panic_any before any argument
inspection.
---
Duplicate comments:
In `@src/hir.rs`:
- Around line 107-135: The matching needs to also require the parent HIR id to
avoid cross-module/macro collisions: when building descriptors and
candidate_fns, capture each item's parent HIR id (in addition to ident.name,
span and hir_id) and then change the filter to require desc_parent == fn_parent
&& desc_id != fn_id && desc_name == name && desc_span.source_equal(*span);
update the tuples in descriptors and candidate_fns to include the parent id and
use those fields (e.g., replace descriptors: Vec<(rustc_span::Symbol, Span,
hir::HirId)> and candidate_fns: Vec<(hir::HirId, rustc_span::Symbol, Span)> with
versions that include the parent id, and adjust the closure to compare
desc_parent with fn_parent).
🪄 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
Run ID: 082296ff-d800-41ca-9c24-53a991d7c5c0
📒 Files selected for processing (3)
crates/no_unwrap_or_else_panic/src/driver.rscrates/no_unwrap_or_else_panic/src/panic_detector.rssrc/hir.rs
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.
Summary
Changes
Core logic
Policy
API & internal wiring
Tests
UI tests
Documentation & ADR
Test plan
Notes
◳ Generated by DevBoxer ◰
ℹ️ Tag @devboxerhub to ask questions and address PR feedback
📎 Task: https://www.devboxer.com/task/ac994937-e677-4025-abc5-f36329d6f327
📝 Closes #188
Summary by Sourcery
Allow interpolating panicking unwrap_or_else fallbacks in test contexts while preserving strict behavior in production, based on richer panic detection.
New Features:
Enhancements:
Documentation:
Tests: