fix: repair snapshot IDs, three inert detectors and dead_code's cubic scan - #39
fix: repair snapshot IDs, three inert detectors and dead_code's cubic scan#39fixcik wants to merge 5 commits into
Conversation
Three smell types produced colliding or unstable snapshot IDs, which broke `archlint diff`: - PackageCycle carries no files, so its ID fell through to the generic hash of an empty file list: every package cycle in a project got the same `packagecycle:e3b0c442`. Two cycles were enough to make `diff` fail outright with "Duplicate smell ID" — even when comparing a snapshot against itself. It is now keyed by the sorted package names. - LayerViolation was keyed by source file and target layer only, so N illegal imports from one file into one layer collapsed onto one ID. Fixing 2 of 3 violations produced no improvement in the diff. The import line now disambiguates them, as it already does for side-effect imports. - SdpViolation had the same collision (keyed by the source file alone) and gets the same treatment. FeatureEnvy picked the envied module with `max_by_key` over a HashMap, so a tie was resolved by iteration order and the winner changed between runs. That module is part of the ID, so `diff` reported a phantom regression plus a phantom fix on unchanged code — 20 runs of the same project split 9/11 between two modules. Ties are now broken by source path. Finally, `write_snapshot` validates before writing. Until now `archlint snapshot` happily produced files that `archlint diff` then refused to read, which made all of the above much harder to diagnose. Note: the ID formulas for PackageCycle, LayerViolation and SdpViolation have changed, so existing baseline snapshots will report those smells once as new.
Three detectors could never report anything in a real run. circular_type_deps resolved every type-only import by walking the dependency graph. The graph only holds files with runtime code, so a cycle between pure type declarations — the very case the detector exists for — had no node on either end and was invisible. It now resolves through the already-resolved import specifier and `file_symbols`, which also retires a hand-rolled path-segment matching heuristic. shotgun_surgery keyed its co-change statistics by the paths git reports, which are relative to the repository root, then looked them up in `file_symbols`, which is keyed by absolute path. Nothing ever matched; even with both thresholds lowered to 1 the detector returned nothing. Paths are now joined onto the repository workdir, the same convention `GitHistoryCache::get_churn_map` already uses. cyclomatic_complexity ignored both `exclude` and path `overrides`. The legacy `complexity` alias was consulted exactly when the modern rule resolved to None — that is, when the user had switched the rule off or excluded the file — and resurrected it with default settings. The alias is now consulted only when `cyclomatic_complexity` is configured nowhere, which also repairs the alias itself: configuring `complexity` previously had no effect either, because the modern id resolved to a permissive default first and the fallback was never reached. The new tests drive the CLI rather than the shared `analyze_fixture` harness. That harness puts every scanned file into the graph while the production runner does not, which is why this whole class of bug could stay green.
Whole-project dead-code analysis grew cubically. `find_dead_files`
iterated every file; for each one `check_default_imports`,
`check_reexports` and `check_local_usages` scanned every other file,
and `check_direct_reexports` ran a nested scan inside that. On a
release build: 200 files 0.36s, 400 files 3.2s, 800 files 24.8s — a
factor of eight per doubling. 1600 files had not finished after twelve
minutes.
The cost came from `matches_source`, a fuzzy comparison
(`source == path || source.ends_with(file_name) || ...`) that cannot be
indexed, so every question had to be answered by a scan. It is replaced
by `SourceIndex`, built once per run and keyed on the resolved absolute
path. Specifiers that failed to resolve — unconfigured aliases, virtual
modules — keep their raw form and are indexed by their trailing
segment, so projects whose aliases archlint cannot resolve do not light
up with false reports.
Same measurements after: 200 files 0.03s, 800 files 0.11s, 3200 files
0.41s — linear.
Because the fuzzy comparison matched on basename, an unused file was
considered alive whenever any other file with the same basename was
imported somewhere. In a codebase full of `index.ts`, `types.ts` and
`utils.ts` that suppressed a large share of real findings. Exact keys
fix that too.
`is_reexported` turned out to be unreachable: `has_used_exports` ran
first and already returned true on the weaker condition that something
re-exports the file, so the extra "and is the re-exporter itself used"
test never applied. Observable behaviour is kept, the dead branch is
gone.
Also in this file: `is_entry_point` parsed patterns by hand, and its
`strip_prefix('*')` branch reduced any pattern starting with a star to
a literal `ends_with`. `entry_points: ["**/bin/**"]` was silently
ignored, as were the built-in `**/test/**`, `**/tests/**`,
`**/__fixtures__/**` and `**/*.mock.ts`. Patterns are now compiled as
globs and matched against both the project-relative path and the file
name.
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Warning Review limit reached
Next review available in: 43 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 (6)
📝 WalkthroughSummary
Testing
CompatibilityExisting snapshots report DocumentationThe detector documentation exists in English, Russian, Spanish, Japanese, Portuguese, and Chinese. The new legacy alias, snapshot validation, deterministic IDs, and WalkthroughThe PR updates detector path resolution, dead-code indexing, rule selection, smell ID generation, snapshot validation, and CLI integration coverage. ChangesDetector correctness
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
Bot
added
bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@crates/archlint/src/detectors/hygiene/dead_code.rs`:
- Around line 264-266: The flat name-keyed usage index incorrectly lets a file’s
own identifiers and same-named identifiers from unrelated files keep exports
alive. In crates/archlint/src/detectors/hygiene/dead_code.rs lines 264-266,
update build_usage_index and check_local_usages to retain usages per file and
exclude the candidate path when checking an export name. In
crates/archlint/tests/cli_detectors.rs lines 191-208, rename the export so it
differs from the imported binding and exercises SourceIndex’s trailing-segment
fallback.
- Around line 61-66: Update the filename key generation in the dead-code
detector to derive the extension-less key from the first dot after any leading
dot, rather than using rsplit_once('.') in the target file-name handling.
Preserve the full filename key and ensure names such as widget.test.ts also
produce widget for unresolved specifier matching.
In `@crates/archlint/src/snapshot/id.rs`:
- Around line 184-188: Update id_for_package_cycle to encode each sorted package
name unambiguously before hashing, using length-prefixing or an equivalent
delimiter-safe representation instead of sorted.join("|"). Preserve
deterministic sorting and ensure distinct package lists cannot produce the same
encoded input.
In `@crates/archlint/tests/cli_detectors.rs`:
- Around line 80-126: Add a positive-control test alongside the complexity tests
using the same complex_function fixture with cyclomatic_complexity enabled and
no exclude or path override, and assert that has_smell reports
high_cyclomatic_complexity before the suppression cases. Keep the existing
exclude, override, and legacy-name tests unchanged, using the JSON smell
identifier high_cyclomatic_complexity rather than the snapshot type string.
🪄 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: 6e0dc7c3-8099-4a2c-b04f-b746ffb7e8c9
📒 Files selected for processing (8)
crates/archlint/src/detectors/dependency/circular_type_deps.rscrates/archlint/src/detectors/design/feature_envy.rscrates/archlint/src/detectors/design/shotgun_surgery.rscrates/archlint/src/detectors/hygiene/dead_code.rscrates/archlint/src/detectors/metrics/mod.rscrates/archlint/src/snapshot/id.rscrates/archlint/src/snapshot/io.rscrates/archlint/tests/cli_detectors.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Test Rust
- GitHub Check: Lint
🔇 Additional comments (13)
crates/archlint/src/detectors/design/feature_envy.rs (2)
77-84: LGTM!
141-182: LGTM!crates/archlint/src/detectors/metrics/mod.rs (1)
59-76: 🎯 Functional CorrectnessNo scope change needed.
resolve_complexity_rulealready passespathto bothcyclomatic_complexityandcomplexity, andget_rule_for_fileresolves overrides throughmatches_path.> Likely an incorrect or invalid review comment.crates/archlint/src/detectors/dependency/circular_type_deps.rs (2)
6-6: LGTM!Also applies to: 37-37
84-95: 🩺 Stability & AvailabilityNo path-key mismatch remains.
circular_type_depsnow resolves type imports through the runtime graph and the normal resolver path, so the removedctx.file_symbols.contains_key(import.source)lookup no longer applies.crates/archlint/src/detectors/design/shotgun_surgery.rs (1)
26-35: LGTM!Also applies to: 49-49, 85-97, 107-110
crates/archlint/src/detectors/hygiene/dead_code.rs (1)
13-19: LGTM!Also applies to: 21-59, 71-109, 197-199, 272-307, 362-394, 396-410, 476-484
crates/archlint/tests/cli_detectors.rs (1)
1-78: LGTM!Also applies to: 128-189, 210-257, 259-329
crates/archlint/src/snapshot/id.rs (3)
21-30: LGTM on the line-based disambiguation approach.The
LayerViolationandSdpViolationID formulas now incorporate the location line viawith_line_hash_fallback, and the accompanying tests confirm distinct offending imports produce distinct IDs. This resolves the reported collision.
118-134: LGTM!Also applies to: 166-178
234-319: LGTM!crates/archlint/src/snapshot/io.rs (2)
6-14: LGTM!
34-89: LGTM!
| if let Some(file_name) = target.file_name().and_then(|name| name.to_str()) { | ||
| keys.push(file_name.to_string()); | ||
| if let Some((base, _)) = file_name.rsplit_once('.') { | ||
| keys.push(base.to_string()); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
Extension-less lookup key misses multi-dot filenames.
rsplit_once('.') splits at the last dot. For widget.test.ts the extension-less key becomes widget.test. An unresolved specifier ./widget produces the key widget, so the two never match and the file is reported as dead. Split at the first dot after the leading dot to cover compound extensions.
♻️ Proposed change
keys.push(file_name.to_string());
- if let Some((base, _)) = file_name.rsplit_once('.') {
- keys.push(base.to_string());
- }
+ if let Some((base, _)) = file_name.rsplit_once('.') {
+ keys.push(base.to_string());
+ }
+ // Compound extensions (`widget.test.ts`) also answer to the leading segment.
+ if let Some((stem, _)) = file_name.trim_start_matches('.').split_once('.') {
+ keys.push(stem.to_string());
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if let Some(file_name) = target.file_name().and_then(|name| name.to_str()) { | |
| keys.push(file_name.to_string()); | |
| if let Some((base, _)) = file_name.rsplit_once('.') { | |
| keys.push(base.to_string()); | |
| } | |
| } | |
| if let Some(file_name) = target.file_name().and_then(|name| name.to_str()) { | |
| keys.push(file_name.to_string()); | |
| if let Some((base, _)) = file_name.rsplit_once('.') { | |
| keys.push(base.to_string()); | |
| } | |
| // Compound extensions (`widget.test.ts`) also answer to the leading segment. | |
| if let Some((stem, _)) = file_name.trim_start_matches('.').split_once('.') { | |
| keys.push(stem.to_string()); | |
| } | |
| } |
🤖 Prompt for 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.
In `@crates/archlint/src/detectors/hygiene/dead_code.rs` around lines 61 - 66,
Update the filename key generation in the dead-code detector to derive the
extension-less key from the first dot after any leading dot, rather than using
rsplit_once('.') in the target file-name handling. Preserve the full filename
key and ensure names such as widget.test.ts also produce widget for unresolved
specifier matching.
| index | ||
| .local_usages | ||
| .extend(symbols.local_usages.iter().map(ToString::to_string)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Flat, name-keyed local_usages weakens dead-code detection and masks the test it should cover. build_usage_index merges every non-excluded file's identifiers into one set keyed by bare name, with no record of which file used which identifier. A file's own identifiers therefore keep its exports alive, and identical names in unrelated files do the same.
crates/archlint/src/detectors/hygiene/dead_code.rs#L264-L266: index local usages per file, then exclude the candidate path whencheck_local_usagestests an export name.crates/archlint/tests/cli_detectors.rs#L191-L208: rename the export so it differs from the imported binding, so the test exercises theSourceIndextrailing-segment fallback instead of the identifier-name match.
📍 Affects 2 files
crates/archlint/src/detectors/hygiene/dead_code.rs#L264-L266(this comment)crates/archlint/tests/cli_detectors.rs#L191-L208
🤖 Prompt for 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.
In `@crates/archlint/src/detectors/hygiene/dead_code.rs` around lines 264 - 266,
The flat name-keyed usage index incorrectly lets a file’s own identifiers and
same-named identifiers from unrelated files keep exports alive. In
crates/archlint/src/detectors/hygiene/dead_code.rs lines 264-266, update
build_usage_index and check_local_usages to retain usages per file and exclude
the candidate path when checking an export name. In
crates/archlint/tests/cli_detectors.rs lines 191-208, rename the export so it
differs from the imported binding and exercises SourceIndex’s trailing-segment
fallback.
The Lint job runs `dtolnay/rust-toolchain@stable`, which has moved ahead of the toolchain these were written against and now rejects the build: code_clone/engine.rs:324 consider using `sort_by_key` engine/runner.rs:347 called `map(<f>).unwrap_or(false)` on a Result utils/package.rs:26 called `map(<f>).unwrap_or(false)` on a Result `context7.json` is missing its trailing newline, which prettier rejects. It arrived in 351f919, pushed straight to main — the Lint job only runs on pull requests and tags, so nothing checked it on the way in. None of these files are otherwise touched by this branch; all four predate it and would block any PR opened today. Mechanical fixes, no behaviour change.
9f81287 to
eb659e8
Compare
Two of the new tests did not test what they claimed. `test_unresolved_import_still_keeps_a_file_alive` used an export whose name matched the imported binding, so `check_local_usages` kept the file alive on an identifier-name match and the unresolved-specifier fallback was never exercised — the test passed with or without it. It now uses a default export and a differently named local binding, so the trailing segment of the specifier is the only thing left to match on. Verified by disabling the fallback: the test fails, and passes again once restored. The cyclomatic_complexity tests all assert an absence, so they would have kept passing if the detector stopped reporting entirely. Added the same fixture with nothing silencing it as a positive control. Also hardened `id_for_package_cycle`: joining names with "|" let ["a|b", "c"] and ["a", "b|c"] hash to the same ID. NUL is the one byte a path component cannot contain. Reported by CodeRabbit on #39.
|
Thanks — two of these were real, and one of them caught a test of mine that was passing for the wrong reason. Addressed in 3efe5bf. Applied
Positive control for the complexity tests — right, all three asserted an absence and would have survived the detector going silent. Added the same fixture with nothing silencing it. Ambiguous separator in Not applied Index Extension-less key from the first dot — declining, for two reasons. It matches the old |
1 participant
Fixes a set of detector and snapshot bugs found while auditing all 30 detectors. The
test suite was green before this work, so none of these were caught by existing tests —
several were actively hidden by a gap between the test harness and the production path.
Every bug below was reproduced on the real CLI before the fix and re-checked after.
1.
archlint diffwas unusable for three smell typespackage_cyclesbrokediffoutright.PackageCyclecarries no files, so its IDfell through to the generic hash of an empty file list — every package cycle in a project
got the same
packagecycle:e3b0c442. Two cycles were enough to makedifffail withDuplicate smell ID, even when comparing a snapshot against itself:layer_violationhid fixes. Keyed by source file and target layer only, so N illegalimports from one file into one layer collapsed onto one ID. Fixing 2 of 3 violations
produced no improvement in the diff. Now disambiguated by import line, as side-effect
imports already were.
SdpViolationhad the same collision and gets the same treatment.feature_envyproduced phantom regressions. The envied module was picked withmax_by_keyover aHashMap, so ties were resolved by iteration order. That module ispart of the smell ID, so on unchanged code
diffreported a regression plus a fix.20 runs of the same project split 9/11 between two modules; now 25/25 identical.
Finally,
write_snapshotnow validates before writing — until nowarchlint snapshothappily produced files that
archlint diffthen refused to read.2. Three detectors could never fire
circular_type_depsresolved type-only imports by walking the dependency graph.The graph only holds files with runtime code, so a cycle between pure type
declarations — the case the detector exists for — had no node on either end. Its own
fixture
type_cycles/basicreports nothing in production; adding oneexport const x = 1to both files makes it fire.shotgun_surgerykeyed co-change stats by git's repository-relative paths, thenlooked them up in
file_symbols, which is keyed by absolute path. Nothing evermatched — a repo with 8 commits × 4 files returned zero even with both thresholds
lowered to 1.
GitHistoryCache::get_churn_mapalready had the right convention.cyclomatic_complexityignored bothexcludeand pathoverrides. The legacycomplexityalias was consulted exactly when the modern rule resolved toNone— thatis, when the user had switched the rule off or excluded the file — and resurrected it
with default settings. Identical override, same file:
cognitive_complexitycyclomatic_complexityThis also repairs the alias itself: configuring
complexitypreviously had no effecteither, because the modern id resolved to a permissive default first.
3.
dead_code: cubic runtime and a large false-negative classfind_dead_filesiterated every file; for each, three helpers scanned every other file,and
check_direct_reexportsran a nested scan inside that.The cost came from
matches_source, a fuzzy comparison that cannot be indexed. Replacedwith a
SourceIndexbuilt once per run, keyed on the resolved absolute path; specifiersthat fail to resolve keep their raw form and are indexed by trailing segment, so projects
with unresolvable aliases don't light up with false reports (guarded by a test).
Because that comparison matched on basename, an unused file was considered alive whenever
any file with the same basename was imported anywhere — in a codebase full of
index.ts,types.tsandutils.tsthat suppressed a large share of real findings.Also here:
is_entry_pointparsed patterns by hand and itsstrip_prefix('*')branchreduced any star-leading pattern to a literal
ends_with.entry_points: ["**/bin/**"]was silently ignored, as were the built-in
**/test/**,**/tests/**,**/__fixtures__/**and**/*.mock.ts. Now compiled as real globs.Heads-up: snapshot IDs change
The ID formulas for
PackageCycle,LayerViolationandSdpViolationchanged, soexisting baseline snapshots will report those smells once as new. I did not add a
BREAKING CHANGE:footer, since on 0.x that would push semantic-release to 1.0.0 — thatrelease call is yours.
Testing
clippy --all-featuresand--no-default-featuresclean.crates/archlint/tests/cli_detectors.rsdrives the actual binary. The sharedanalyze_fixtureharness puts every scanned file into the graph while the productionrunner does not, which is why this whole class of bug could stay green.
(
docs/,packages/,scripts/).Known and left alone
Non-deterministic ordering of report/snapshot output (
builder.rsiterates aHashSet,cycles.rssorts by aHashMapvalue), RustDebugoutput leaking into the JSONtypefield (
report/json.rs:65), cluster severity taken from an arbitraryscc.first(),order-insensitive
matches_glob_patterninlayer_violation, and repeated file reads inlarge_file/update_cache.