feat(dead_code,dead_symbols): count_test_imports, barrel tracing & root entry points#36
feat(dead_code,dead_symbols): count_test_imports, barrel tracing & root entry points#36
Conversation
Add a `count_test_imports` option (default: true) to the dead_code detector. When set to false, imports from test files (*.test.ts, *.spec.ts, etc.) are not counted as usage, so code used only in tests is flagged as dead code. Closes #35 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When set to false, test file imports and usages are excluded from dead symbol analysis, allowing detection of test-only code.
Replace is_symbol_imported() with is_symbol_consumed() that recursively traces export * chains. A symbol re-exported through a barrel is only considered alive if actually imported downstream. Closes #37
- Change count_test_imports default from true to false in both dead_code and dead_symbols detectors, so test-only code is flagged by default - Fix Config.ignore serde default: use default_ignore_patterns() instead of Vec::new() so test file patterns are preserved when loading from YAML - Update schema and tests accordingly Closes #37 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds centralized test-file glob patterns and a serde default, makes hygiene detectors optionally count test imports (new Changes
Sequence DiagramsequenceDiagram
participant Analyzer as Analyzer
participant Detector as DeadSymbolsDetector
participant Matcher as TestPatternMatcher
participant Collector as UsageCollector
participant Barrel as BarrelResolver
Analyzer->>Detector: start dead_symbols analysis (with config)
Detector->>Matcher: request test file set (TEST_FILE_PATTERNS)
Matcher-->>Detector: return test file globs/set
Detector->>Collector: collect usages (pass excluded test files)
Collector->>Barrel: resolve export * / barrel chains (recursive)
Barrel-->>Collector: resolved re-export mappings
Collector-->>Detector: provide usage map & consumption info
Detector->>Collector: is_symbol_consumed(symbol)
Collector->>Barrel: traverse re-exports to find real consumers
Barrel-->>Collector: consumption result
Collector-->>Detector: consumed? (true|false)
Detector-->>Analyzer: emit dead vs alive symbols
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
- Extract helper functions in dead_code_test_only_test.rs to reduce duplication: - detect_smells() for detector creation and analysis - assert_file_flagged() and assert_file_not_flagged() for common assertions - Remove obsolete test_barrel_default_counts_test_imports from dead_symbols_barrel_test.rs (no longer relevant after default behavior change in 337ac0c) - Reduces code duplication from ~43% to near zero in test file Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/archlint/src/detectors/hygiene/dead_code.rs (1)
236-272: 🧹 Nitpick | 🔵 TrivialPotential logic conflict: test files are both entry points and excluded when
count_test_imports=false.When
count_test_imports=false, test file patterns are added tocombined_exclude. Butis_entry_point(line 550) callsis_path_excludedfirst and returnsfalsefor excluded paths. This means test files won't be recognized as entry points — they'll simply be skipped infind_dead_filesbecauseis_path_excludedfilters them out at line 191. The net result is correct (test files aren't flagged), but the interaction is subtle and fragile. Iffind_dead_fileschanges iteration logic, test files could start being flagged.Consider adding an explicit comment at the
is_entry_pointcheck documenting this dependency on exclusion order, or add a dedicated test that asserts test files are never in the output regardless of the code path taken.crates/archlint/src/detectors/hygiene/dead_symbols.rs (1)
250-304:⚠️ Potential issue | 🟡 MinorNamed re-exports (
export { foo } from '...') are not traced through, unlike wildcard re-exports.The
is_symbol_consumed_recursivefunction correctly tracesexport * frombarrel chains (Check 2), but Check 1 (lines 262–268) treats any named importer as a real consumer — including a barrel that doesexport { foo } from './module'. This means a symbol that is only passed through via a named re-export but never actually consumed downstream will incorrectly appear "alive."This is an improvement over the prior baseline (which didn't trace wildcards either), but it's worth noting as a known limitation. A follow-up could extend the barrel-tracing logic to named re-exports as well.
🤖 Fix all issues with AI agents
In `@crates/archlint/src/config/types.rs`:
- Around line 10-14: The serde attribute on the ignore field uses default =
"default_ignore_patterns" together with skip_serializing_if = "Vec::is_empty",
which makes an explicit ignore: [] impossible to persist; change the field to
either Option<Vec<String>> (e.g., pub ignore: Option<Vec<String>>) and implement
semantics where None = use default_ignore_patterns() and Some(vec![]) = no
ignores, or remove skip_serializing_if so an explicit empty Vec is serialized
and preserved; update the serde attribute (remove skip_serializing_if or replace
default with custom deserialization logic) and adjust any code that reads
`ignore` to handle the new Option-based or preserved-empty semantics (refer to
the ignore field and default_ignore_patterns function in types.rs).
In `@crates/archlint/src/detectors/hygiene/dead_code.rs`:
- Around line 69-74: The change introduced a breaking behavioral default by
setting count_test_imports to false which will flag test-only imports as dead;
update the default to preserve prior behavior by using true as the fallback
(i.e. change the unwrap_or(false) call used when reading
rule.get_option("count_test_imports") to unwrap_or(true)), so TEST_FILE_PATTERNS
is only appended to combined_exclude when a user explicitly opts into excluding
test imports; ensure the check that extends combined_exclude (the if
!count_test_imports branch referencing Self::TEST_FILE_PATTERNS and
combined_exclude) remains correct after this default change.
- Around line 108-126: The three separate test/config glob lists are duplicated
and drifting: unify them by extracting a single shared constant (e.g., rename
TEST_FILE_PATTERNS to SHARED_TEST_AND_CONFIG_PATTERNS or move it to a central
module) and have default_ignore_patterns(), the constructor new() entry
patterns, and the existing TEST_FILE_PATTERNS all reference that single
constant; update new() to include the additional config/setup globs currently
only present there (e.g., "*.config.*" and "*.setup.*") and normalize glob
styles (use double-star patterns consistently) so all callers
(TEST_FILE_PATTERNS, default_ignore_patterns(), new()) read from the same
canonical pattern list.
In `@crates/archlint/src/detectors/hygiene/dead_symbols.rs`:
- Around line 83-90: The default for count_test_imports is currently set to
false causing test imports to be excluded and breaking backward-compat tests;
change the default to true by using
rule.get_option("count_test_imports").unwrap_or(true) (or otherwise ensure rule
option defaults to true) so that the logic around test_excluded (which calls
Self::find_test_files) will count test imports by default and satisfy the
expectations in dead_symbols_barrel_test.rs; update any related comments if
needed.
- Around line 28-46: The default behavior currently excludes test files because
the code calls unwrap_or(false) when reading count_test_imports; change that
default to true (e.g., use unwrap_or(true) or equivalent) so count_test_imports
defaults to counting test imports to satisfy
test_barrel_default_counts_test_imports and preserve backward compatibility;
also make TEST_FILE_PATTERNS configurable (extract it from the hardcoded const
into a configurable field or read-from-config) and document the
count_test_imports option so users can override or narrow patterns (refer to
symbols TEST_FILE_PATTERNS and count_test_imports and the unwrap_or(false) usage
when locating the change).
In `@crates/archlint/tests/dead_code_test_only_test.rs`:
- Around line 9-33: The helper create_config_with_count_test_imports sets
ignore: Vec::new() which intentionally overrides default_ignore_patterns() to
prevent test files from being globally excluded; add a concise inline comment
next to the ignore field explaining that the empty Vec is required so fixtures
include test files (otherwise default_ignore_patterns() would filter them out
and break this test), referencing the rationale in
create_config_with_count_test_imports and mentioning default_ignore_patterns for
clarity.
In `@crates/archlint/tests/dead_symbols_barrel_test.rs`:
- Around line 78-92: The comment in the test function
test_barrel_with_test_imports_counted is misleading because count_test_imports
is not the default (detect_symbols uses unwrap_or(false)); update the comment to
remove or correct "(default)" and clearly state that this test explicitly sets
count_test_imports to true via create_dead_symbols_config(true) so testHelper
should not be dead when the detector
(archlint::detectors::dead_symbols::DeadSymbolsDetector) runs; reference
detect_symbols' unwrap_or(false) behavior if you want to justify the corrected
wording.
- Around line 94-119: The test test_barrel_default_counts_test_imports currently
relies on test imports being counted by default, but the dead_symbols detector
option "n" defaults to false (unwrap_or(false)), so update the test's
dead_symbols RuleConfig to explicitly enable test-file counting by setting
option "n" to true in the RuleFullConfig/options for the "dead_symbols" rule
(or, alternatively, change the detector's fallback in dead_symbols.rs from
unwrap_or(false) to unwrap_or(true) if you intend to change the global default);
reference test_barrel_default_counts_test_imports and the "n" option in
dead_symbols to locate where to apply the fix.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (15)
crates/archlint/src/config/types.rscrates/archlint/src/detectors/hygiene/dead_code.rscrates/archlint/src/detectors/hygiene/dead_symbols.rscrates/archlint/test_data/dead_code_test_only/main.tscrates/archlint/test_data/dead_code_test_only/prod_util.tscrates/archlint/test_data/dead_code_test_only/test_only_util.test.tscrates/archlint/test_data/dead_code_test_only/test_only_util.tscrates/archlint/test_data/dead_symbols_barrel/index.tscrates/archlint/test_data/dead_symbols_barrel/main.tscrates/archlint/test_data/dead_symbols_barrel/prod_util.tscrates/archlint/test_data/dead_symbols_barrel/test_only_util.spec.tscrates/archlint/test_data/dead_symbols_barrel/test_only_util.tscrates/archlint/tests/dead_code_test_only_test.rscrates/archlint/tests/dead_symbols_barrel_test.rsresources/archlint.schema.json
🧰 Additional context used
🧬 Code graph analysis (7)
crates/archlint/tests/dead_code_test_only_test.rs (2)
crates/archlint/tests/common/mod.rs (1)
analyze_fixture_with_config(23-104)crates/archlint/src/detectors/hygiene/dead_code.rs (1)
new(229-292)
crates/archlint/tests/dead_symbols_barrel_test.rs (1)
crates/archlint/tests/common/mod.rs (1)
analyze_fixture_with_config(23-104)
crates/archlint/test_data/dead_code_test_only/main.ts (1)
crates/archlint/test_data/dead_code_test_only/prod_util.ts (1)
prodHelper(1-1)
crates/archlint/test_data/dead_symbols_barrel/test_only_util.spec.ts (1)
crates/archlint/test_data/dead_symbols_barrel/test_only_util.ts (1)
testHelper(1-1)
crates/archlint/src/detectors/hygiene/dead_code.rs (1)
crates/archlint/src/detectors/hygiene/dead_symbols.rs (2)
rule(368-368)rule(377-377)
crates/archlint/test_data/dead_code_test_only/test_only_util.test.ts (1)
crates/archlint/test_data/dead_code_test_only/test_only_util.ts (1)
testHelper(1-1)
crates/archlint/test_data/dead_symbols_barrel/main.ts (1)
crates/archlint/test_data/dead_symbols_barrel/prod_util.ts (1)
prodHelper(1-1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Cursor Bugbot
- GitHub Check: SonarQube
- GitHub Check: Lint
🔇 Additional comments (19)
crates/archlint/test_data/dead_code_test_only/test_only_util.ts (1)
1-1: LGTM!Simple, well-defined test fixture for verifying the
count_test_importsfeature.crates/archlint/test_data/dead_code_test_only/test_only_util.test.ts (1)
1-2: LGTM!Clean fixture establishing a test-only import of
testHelperfor integration test validation.crates/archlint/test_data/dead_symbols_barrel/test_only_util.ts (1)
1-1: LGTM!Test fixture for barrel re-export scenarios.
crates/archlint/test_data/dead_symbols_barrel/test_only_util.spec.ts (1)
1-2: LGTM!Spec fixture for barrel re-export dead-symbol tests.
resources/archlint.schema.json (1)
8-20: Schemaignoredefault is consistent withdefault_ignore_patterns()intypes.rs.Both list the same 11 patterns. The previously noted inconsistency with
TEST_FILE_PATTERNS(17 patterns indead_code.rs) applies here as well.crates/archlint/tests/dead_code_test_only_test.rs (2)
35-81: LGTM — solid coverage of thecount_test_imports=falsescenario.Assertions cover all four key cases: test-only utility flagged, production utility not flagged, entry point not flagged, and test file itself not flagged.
83-141: LGTM —count_test_imports=trueand default-behavior tests are well-structured.The default test at line 107 explicitly confirms the new default (
false) matches the expected behavior. Good.crates/archlint/src/detectors/hygiene/dead_symbols.rs (4)
54-75: LGTM —find_test_fileslogic is sound.Path normalization for Windows (
replace('\\', "/")) and thestrip_prefixfallback are good defensive practices. Since patterns are hardcoded and known-valid, the.ok()filter on glob compilation is acceptable.
196-229: LGTM — consistent test-file exclusion across both usage collection paths.Both
collect_all_usagesandbuild_symbol_imports_mapcorrectly filter out the same set of excluded test file paths, ensuring coherent behavior across local-usage checks and import-based checks.
635-661: LGTM —check_export_usagecorrectly integrates consumption tracing.The transition from a simple import check to
is_symbol_consumedproperly handles barrel re-export chains. The fallbackis_used_by_namecheck viaall_project_usagespreserves existing behavior for name-based usage detection.
728-1207: Existing unit tests look good.No issues with the existing test suite. The tests correctly exercise private methods, name collisions, namespace inheritance, polymorphism with barrels, contract methods, and config merging.
crates/archlint/test_data/dead_symbols_barrel/prod_util.ts (1)
1-1: LGTM — simple test fixture.crates/archlint/test_data/dead_symbols_barrel/main.ts (1)
1-2: LGTM — correctly exercises barrel consumption path.Imports
prodHelperthrough the barrel./indexand uses it, establishing production consumption for the test scenario.crates/archlint/test_data/dead_code_test_only/main.ts (1)
1-2: LGTM — straightforward test fixture for direct import scenario.crates/archlint/test_data/dead_code_test_only/prod_util.ts (1)
1-1: LGTM — simple test fixture.crates/archlint/test_data/dead_symbols_barrel/index.ts (1)
1-2: LGTM — barrel fixture correctly establishes the re-export chain.Re-exports both
prod_util(consumed bymain.ts) andtest_only_util(consumed only by test files), enabling the barrel consumption-tracking tests.crates/archlint/tests/dead_symbols_barrel_test.rs (3)
8-32: LGTM — clean helper for building test configs.
create_dead_symbols_configis well-structured and reusable across the test cases.
34-42: LGTM — concise helper for checking dead symbol presence.
44-76: LGTM — core barrel behavior tests are clear and well-documented.Tests for
count_test_imports=falsecorrectly validate both the positive case (testHelper is dead) and negative case (prodHelper is alive through barrel consumption).
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/archlint/tests/dead_code_test_only_test.rs`:
- Around line 36-45: The test helper detect_smells currently constructs
DeadCodeDetector with empty collections, discarding ctx.script_entry_points and
ctx.dynamic_load_patterns; update the DeadCodeDetector::new call in
detect_smells to pass ctx.script_entry_points and ctx.dynamic_load_patterns
(and/or slices/references as required by the constructor signature) instead of
HashSet::new and Vec::new so the detector receives the package.json-driven entry
points and dynamic load patterns (refer to detect_smells, DeadCodeDetector::new,
ctx.script_entry_points, ctx.dynamic_load_patterns).
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
crates/archlint/tests/dead_code_test_only_test.rscrates/archlint/tests/dead_symbols_barrel_test.rs
🧰 Additional context used
🧬 Code graph analysis (2)
crates/archlint/tests/dead_symbols_barrel_test.rs (1)
crates/archlint/tests/common/mod.rs (1)
analyze_fixture_with_config(23-104)
crates/archlint/tests/dead_code_test_only_test.rs (2)
crates/archlint/tests/common/mod.rs (1)
analyze_fixture_with_config(23-104)crates/archlint/src/detectors/hygiene/dead_code.rs (1)
new(229-292)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (6)
crates/archlint/tests/dead_symbols_barrel_test.rs (3)
8-32: LGTM — config helper is clean and well-structured.The
create_dead_symbols_confighelper correctly constructs theserde_yaml::Mappingand wires it intoRuleFullConfig. Good use of..Default::default()for the remainingConfigfields.
34-42: LGTM —is_dead_symbolhelper is concise.
44-76: LGTM — barrel re-export test cases cover the key scenarios well.Good inclusion of filtered smell names in the assertion message on line 58 for easier debugging.
crates/archlint/tests/dead_code_test_only_test.rs (3)
47-63: LGTM — clean assertion helpers.The
ends_withapproach is appropriate for matching fixture file paths by name.
65-112: LGTM — good coverage of bothcount_test_importsstates.The disabled test validates all four expected outcomes (flagged test-only util, non-flagged production util, entry point, and test file), while the enabled test confirms the key behavioral difference.
114-140: LGTM — correctly validates the default-off semantic.Inline config construction (without setting
count_test_imports) is the right approach to confirm the default behavior.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
coderabbitai
bot
added
the
documentation
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
crates/archlint/src/main.rs (1)
90-126: 🧹 Nitpick | 🔵 TrivialPre-existing: quiet/verbose priority is inconsistent between the two log-level functions.
In
set_initial_log_level(lines 55–59),verbose > 0is checked beforeis_quiet(), so verbose wins. Here inset_final_log_level(lines 92–97), the guardargs.is_quiet()is matched first, so quiet wins. Sinceset_final_log_levelruns last (whenRUST_LOGis unset), quiet effectively takes precedence in the common case — but the inverted order in the other function could surprise during debugging ifRUST_LOGis set.Not a regression from this PR, but worth aligning the precedence order in a follow-up to avoid confusion.
crates/archlint/src/detectors/hygiene/dead_symbols.rs (3)
690-713: 🛠️ Refactor suggestion | 🟠 MajorMisleading parameter name:
entry_pointshere actually receivesroot_entry_points.In
check_file_exportsandcheck_export_usage, the parameter namedentry_pointsactually receives theroot_entry_pointssubset fromcheck_dead_exports(line 682). This shadow naming is confusing and error-prone — a future reader/editor could easily assume it's the full entry-point set. Rename the parameter toroot_entry_pointsto match what's actually passed.♻️ Proposed fix
fn check_file_exports( file_path: &Path, symbols: &FileSymbols, file_symbols: &HashMap<PathBuf, FileSymbols>, - entry_points: &HashSet<PathBuf>, + root_entry_points: &HashSet<PathBuf>, symbol_usages: &HashMap<(PathBuf, String), HashSet<PathBuf>>, all_project_usages: &HashSet<String>, ) -> Vec<ArchSmell> { symbols .exports .iter() .filter(|export| !export.is_reexport && export.name != "default" && export.name != "*") .filter_map(|export| { Self::check_export_usage( file_path, export, file_symbols, - entry_points, + root_entry_points, symbol_usages, all_project_usages, ) }) .collect() } fn check_export_usage( file_path: &Path, export: &crate::parser::ExportedSymbol, file_symbols: &HashMap<PathBuf, FileSymbols>, - entry_points: &HashSet<PathBuf>, + root_entry_points: &HashSet<PathBuf>, symbol_usages: &HashMap<(PathBuf, String), HashSet<PathBuf>>, all_project_usages: &HashSet<String>, ) -> Option<ArchSmell> { let is_consumed = Self::is_symbol_consumed( symbol_usages, file_symbols, - entry_points, + root_entry_points, file_path, export.name.as_str(), );
382-407:⚠️ Potential issue | 🟠 MajorLocal symbols inside test files will be falsely flagged as dead when
count_test_importsisfalse.
check_dead_local_symbolsiterates allfile_symbols(including test files) and checks each local definition againstall_project_usages. Butcollect_all_usages(line 237) excludes test files from contributing to that usage set. As a result, a helper function defined and used entirely within a test file will appear unused and be reported as dead code.Either skip test-excluded files in
check_dead_local_symbols, or include test-file-internal usages for their own symbols.🐛 Proposed fix — skip excluded files in local-symbol check
This requires passing the
test_excludedset tocheck_dead_local_symbols. Alternatively, filter at the call site:all_smells.extend(Self::check_dead_local_symbols( file_symbols, &all_project_usages, + ).into_iter().filter(|smell| { + smell.files.first().map_or(true, |p| !test_excluded.contains(p)) + })); - ));A cleaner approach would be to pass
&test_excludedintocheck_dead_local_symbolsand skip those files during iteration.
135-141:⚠️ Potential issue | 🟠 MajorSame false-positive risk applies to
check_dead_methodsfor test files.
check_dead_methods(line 135) also iterates allfile_symbolsincluding test files, butsymbol_usagesexcludes test-file imports. Methods defined in test-file classes that are only called within those test files would be incorrectly flagged. Apply the same exclusion treatment here.
🤖 Fix all issues with AI agents
In `@crates/archlint/src/detectors/hygiene/dead_symbols.rs`:
- Around line 58-71: The is_root_entry_point function currently walks upward
from entry_point calling Path::exists() to locate package.json, causing many
synchronous filesystem stats; modify the detection flow to avoid runtime fs
calls by having the initial file scan collect package.json-containing
directories (e.g., into a HashSet<PathBuf>) and change is_root_entry_point to
accept that cache (or a reference to it) instead of probing the FS; update
callers of is_root_entry_point to pass the precomputed set and use
set.contains(&parent_dir) / ancestor checks to determine root membership without
calling exists().
- Around line 102-109: The comment in the test dead_symbols_barrel_test.rs
incorrectly states the default for count_test_imports and there is no explicit
test for the default behavior; update the misleading comment in
test_barrel_with_test_imports_counted to reflect that unwrap_or(false) makes the
default false, and add a new test (analogous to
test_dead_code_default_flags_test_only_usage) that leaves count_test_imports
unset and asserts the expected behavior when count_test_imports is false (use
the same setup as test_barrel_with_test_imports_counted but omit setting the
option), referencing the rule resolution via ctx.resolve_rule("dead_symbols",
None), the count_test_imports option, and the use of
Self::find_test_files/file_symbols to validate results.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
crates/archlint/src/api/options.rscrates/archlint/src/args.rscrates/archlint/src/cli.rscrates/archlint/src/detectors/hygiene/dead_symbols.rscrates/archlint/src/main.rs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-09T12:52:01.859Z
Learnt from: fixcik
Repo: archlinter/archlint PR: 17
File: crates/archlint/src/framework/detector.rs:76-91
Timestamp: 2026-01-09T12:52:01.859Z
Learning: In crates/archlint/src/framework/detector.rs, the matches_rules function uses OR semantics when both any_of and all_of fields are populated in MatchRules. This is intentional for flexibility in preset detection rules, not a bug.
Applied to files:
crates/archlint/src/detectors/hygiene/dead_symbols.rs
🧬 Code graph analysis (1)
crates/archlint/src/detectors/hygiene/dead_symbols.rs (2)
crates/archlint/src/engine/detector_runner.rs (1)
new(24-26)crates/archlint/src/parser/types.rs (3)
new(126-137)new(184-206)new(229-238)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: SonarQube
- GitHub Check: Lint
- GitHub Check: Cursor Bugbot
🔇 Additional comments (11)
crates/archlint/src/api/options.rs (1)
69-69: LGTM!The literal
0correctly maps to the newu8verbosity level, consistent with theScanArgs.verbosetype change.crates/archlint/src/args.rs (1)
58-60: LGTM!Using
ArgAction::Countwithu8is the idiomatic clap pattern for tiered verbosity. The doc comment clearly documents the semantics.crates/archlint/src/cli.rs (2)
52-54: LGTM!Consistent with the
ScanArgstype change.ArgAction::Countcorrectly paired withu8.
202-204: LGTM!
DiffArgs.verbosemirrors the same count-based pattern — consistent across all CLI entry points.crates/archlint/src/main.rs (3)
44-50: LGTM!Clean mapping function. The wildcard arm for
>= 2correctly collapses all higher counts toTrace, which is the maximum useful granularity.
52-88: LGTM!All command branches consistently use
verbose > 0as the gate and delegate toverbose_to_levelfor level resolution.
142-142: LGTM!Trace formatting now uses a labeled
"trace:"prefix with dim styling, consistent with the other log-level prefixes.crates/archlint/src/detectors/hygiene/dead_symbols.rs (4)
4-4: Good addition of tiered logging withdebug!andtrace!.The separation of summary-level
debug!logs from per-symboltrace!logs aligns well with the-v/-vvverbosity support described in the PR.
266-323: Consumption tracing with cycle protection looks correct.The
visitedset properly prevents infinite recursion on circular re-export chains, and the two-phase check (direct named import → wildcard re-export tracing) is well-structured.
831-1310: Existing tests retained and unchanged — no issues.
371-376: The namespace member access check at line 373 is correct. The parser properly populateslocal_usageswith bare member names (e.g.,"foo"fromNS.foo()), and the dead symbols detector correctly verifies consumption by checking if these names appear inlocal_usages. Verified through parser implementation and integration tests.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| let rule = ctx.resolve_rule("dead_symbols", None); | ||
| let count_test_imports: bool = rule.get_option("count_test_imports").unwrap_or(false); | ||
|
|
||
| let test_excluded: HashSet<PathBuf> = if count_test_imports { | ||
| HashSet::new() | ||
| } else { | ||
| Self::find_test_files(file_symbols, &ctx.project_path) | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the backward-compat test has been updated to match default=false
rg -n "test_barrel_default_counts_test_imports" --type=rust -A 30Repository: archlinter/archlint
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Find all test files related to dead_symbols
fd "dead_symbols" --type f | grep -E "(test|spec)" | head -20
# Also search for count_test_imports in test context
echo "---"
rg "count_test_imports" --type=rust -B 2 -A 2Repository: archlinter/archlint
Length of output: 7059
🏁 Script executed:
#!/bin/bash
# Get all test functions in dead_symbols_barrel_test.rs
rg "^fn test_" crates/archlint/tests/dead_symbols_barrel_test.rs -A 15 | head -100Repository: archlinter/archlint
Length of output: 2072
🏁 Script executed:
#!/bin/bash
# Check the structure of dead_code_test_only_test.rs default test for comparison
rg "test_dead_code_default_flags_test_only_usage" --type=rust -A 20Repository: archlinter/archlint
Length of output: 1857
Fix misleading test comment and add explicit default behavior test.
The test test_barrel_with_test_imports_counted in crates/archlint/tests/dead_symbols_barrel_test.rs contains a misleading comment stating "With count_test_imports=true (default)" when the actual default is false (per the code: unwrap_or(false)). Additionally, unlike dead_code_test_only_test.rs which includes an explicit test_dead_code_default_flags_test_only_usage() test for default behavior, dead_symbols_barrel_test.rs lacks a corresponding test that validates behavior when count_test_imports is not set. Add an explicit test for the default case and correct the comment to avoid confusion.
🤖 Prompt for AI Agents
In `@crates/archlint/src/detectors/hygiene/dead_symbols.rs` around lines 102 -
109, The comment in the test dead_symbols_barrel_test.rs incorrectly states the
default for count_test_imports and there is no explicit test for the default
behavior; update the misleading comment in test_barrel_with_test_imports_counted
to reflect that unwrap_or(false) makes the default false, and add a new test
(analogous to test_dead_code_default_flags_test_only_usage) that leaves
count_test_imports unset and asserts the expected behavior when
count_test_imports is false (use the same setup as
test_barrel_with_test_imports_counted but omit setting the option), referencing
the rule resolution via ctx.resolve_rule("dead_symbols", None), the
count_test_imports option, and the use of Self::find_test_files/file_symbols to
validate results.
fixcik
changed the title
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 (2)
crates/archlint/src/detectors/hygiene/dead_code.rs (1)
232-267: 🧹 Nitpick | 🔵 TrivialEntry-point patterns mix single-star and double-star globs — intentional but worth documenting.
TEST_FILE_PATTERNSuses**/*.test.ts(recursive) while entry patterns here use*.test.ts(filename-only). This works becauseis_entry_point(line 556-569) matches againstfile_namefor*-prefixed patterns, whereas exclude patterns match against the full relative path. A brief comment noting the intentional glob-style difference would help maintainers.crates/archlint/src/detectors/hygiene/dead_symbols.rs (1)
655-676: 🛠️ Refactor suggestion | 🟠 MajorParameter
entry_pointsincheck_file_exportsandcheck_export_usageactually receivesroot_entry_points— rename for clarity.In
check_dead_exports(line 664),entry_points(all) is used for the entry-point filter, butroot_entry_pointsis passed tocheck_file_exportsas theentry_pointsparameter. This naming mismatch makes it easy to confuse which set is being used where.♻️ Rename parameters to reflect what they actually receive
fn check_file_exports( file_path: &Path, symbols: &FileSymbols, file_symbols: &HashMap<PathBuf, FileSymbols>, - entry_points: &HashSet<PathBuf>, + root_entry_points: &HashSet<PathBuf>, symbol_usages: &HashMap<(PathBuf, String), HashSet<PathBuf>>, all_project_usages: &HashSet<String>, ) -> Vec<ArchSmell> {fn check_export_usage( file_path: &Path, export: &crate::parser::ExportedSymbol, file_symbols: &HashMap<PathBuf, FileSymbols>, - entry_points: &HashSet<PathBuf>, + root_entry_points: &HashSet<PathBuf>, symbol_usages: &HashMap<(PathBuf, String), HashSet<PathBuf>>, all_project_usages: &HashSet<String>, ) -> Option<ArchSmell> {Also applies to: 678-701
🤖 Fix all issues with AI agents
In `@crates/archlint/src/detectors/hygiene/dead_code.rs`:
- Around line 60-90: The combined_exclude vector may receive duplicate patterns
when count_test_imports is false because ctx.config.ignore may already equal
TEST_FILE_PATTERNS and you then extend combined_exclude with TEST_FILE_PATTERNS
again; update the logic around combined_exclude (used with self.exclude,
rule.exclude, ctx.config.ignore and TEST_FILE_PATTERNS) to deduplicate entries
before final use—e.g., collect entries into a HashSet<String> (or dedupe after
extension) so combined_exclude contains unique globs while preserving the
current branching behavior for count_test_imports and the explicit addition of
TEST_FILE_PATTERNS when ignore could be empty.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
crates/archlint/src/config/types.rscrates/archlint/src/detectors/hygiene/dead_code.rscrates/archlint/src/detectors/hygiene/dead_symbols.rscrates/archlint/tests/dead_code_test_only_test.rscrates/archlint/tests/dead_symbols_barrel_test.rsresources/archlint.schema.json
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: fixcik
Repo: archlinter/archlint PR: 36
File: crates/archlint/src/detectors/hygiene/dead_code.rs:69-74
Timestamp: 2026-02-13T17:29:28.000Z
Learning: In the archlint project, the `count_test_imports` option defaults to `false` for both `dead_code` and `dead_symbols` detectors. This is an intentional breaking change to enforce stricter hygiene: production code referenced only by tests is flagged as dead by default, encouraging teams to keep production code actively used in production contexts.
Learnt from: fixcik
Repo: archlinter/archlint PR: 36
File: crates/archlint/src/detectors/hygiene/dead_symbols.rs:29-47
Timestamp: 2026-02-13T17:29:31.835Z
Learning: In `crates/archlint/src/detectors/hygiene/dead_symbols.rs`, the `count_test_imports` option intentionally defaults to `false` as a breaking change by design. When `false`, imports from test files are not counted as usage, and code referenced only by tests is flagged as dead. Setting it to `true` restores the prior behavior where test imports count as usage.
📚 Learning: 2026-02-13T17:29:28.000Z
Learnt from: fixcik
Repo: archlinter/archlint PR: 36
File: crates/archlint/src/detectors/hygiene/dead_code.rs:69-74
Timestamp: 2026-02-13T17:29:28.000Z
Learning: In the archlint project, the `count_test_imports` option defaults to `false` for both `dead_code` and `dead_symbols` detectors. This is an intentional breaking change to enforce stricter hygiene: production code referenced only by tests is flagged as dead by default, encouraging teams to keep production code actively used in production contexts.
Applied to files:
resources/archlint.schema.jsoncrates/archlint/tests/dead_code_test_only_test.rscrates/archlint/src/detectors/hygiene/dead_code.rscrates/archlint/tests/dead_symbols_barrel_test.rscrates/archlint/src/config/types.rscrates/archlint/src/detectors/hygiene/dead_symbols.rs
📚 Learning: 2026-02-13T17:29:31.835Z
Learnt from: fixcik
Repo: archlinter/archlint PR: 36
File: crates/archlint/src/detectors/hygiene/dead_symbols.rs:29-47
Timestamp: 2026-02-13T17:29:31.835Z
Learning: In `crates/archlint/src/detectors/hygiene/dead_symbols.rs`, the `count_test_imports` option intentionally defaults to `false` as a breaking change by design. When `false`, imports from test files are not counted as usage, and code referenced only by tests is flagged as dead. Setting it to `true` restores the prior behavior where test imports count as usage.
Applied to files:
crates/archlint/tests/dead_code_test_only_test.rscrates/archlint/src/detectors/hygiene/dead_code.rscrates/archlint/tests/dead_symbols_barrel_test.rscrates/archlint/src/config/types.rscrates/archlint/src/detectors/hygiene/dead_symbols.rs
📚 Learning: 2026-01-09T12:52:01.859Z
Learnt from: fixcik
Repo: archlinter/archlint PR: 17
File: crates/archlint/src/framework/detector.rs:76-91
Timestamp: 2026-01-09T12:52:01.859Z
Learning: In crates/archlint/src/framework/detector.rs, the matches_rules function uses OR semantics when both any_of and all_of fields are populated in MatchRules. This is intentional for flexibility in preset detection rules, not a bug.
Applied to files:
crates/archlint/src/detectors/hygiene/dead_code.rscrates/archlint/src/detectors/hygiene/dead_symbols.rs
🧬 Code graph analysis (1)
crates/archlint/src/detectors/hygiene/dead_code.rs (1)
crates/archlint/src/detectors/hygiene/dead_symbols.rs (2)
rule(432-432)rule(441-441)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Cursor Bugbot
- GitHub Check: SonarQube
- GitHub Check: Lint
🔇 Additional comments (9)
crates/archlint/src/config/types.rs (2)
10-11: Good fix:skip_serializing_ifremoved fromignorefield.This correctly resolves the prior issue where
ignore: []couldn't be persisted — an explicit empty list will now be serialized and preserved on round-trip.
441-465: Clean centralization of test file patterns.
TEST_FILE_PATTERNSas apub constis a good single source of truth, anddefault_ignore_patterns()correctly derives from it. This eliminates the prior drift betweentypes.rs(11 patterns) and the detector-local constants (17 patterns).crates/archlint/src/detectors/hygiene/dead_code.rs (1)
1-1: Good: importing centralizedTEST_FILE_PATTERNSinstead of maintaining a local copy.resources/archlint.schema.json (1)
8-26: Schema default aligns withTEST_FILE_PATTERNS— LGTM.All 17 patterns match the centralized constant in
types.rs.crates/archlint/tests/dead_code_test_only_test.rs (2)
1-47: Well-structured test helpers addressing previous review feedback.Both
ignore: Vec::new()with the explanatory comment and forwarding ofctx.script_entry_points/ctx.dynamic_load_patternsto the detector are good improvements.
67-142: Good coverage of all threecount_test_importsscenarios.The tests correctly validate: disabled (flags test-only code), enabled (keeps test-only code), and default behavior (flags test-only code, matching
unwrap_or(false)).crates/archlint/src/detectors/hygiene/dead_symbols.rs (2)
248-362: Barrel re-export tracing with cycle protection looks correct.The
visitedset inis_symbol_consumed_recursiveprevents infinite loops in circular re-export chains. The three-way check (direct named import → re-export through root entry point → recursive barrel tracing → namespace local usage) covers the key consumption scenarios well.
83-90:count_test_importsdefaultfalseis intentional — confirmed by learnings.This is the expected breaking change for stricter hygiene. Based on learnings, the
count_test_importsoption defaults tofalsefor bothdead_codeanddead_symbolsdetectors as an intentional breaking change.crates/archlint/tests/dead_symbols_barrel_test.rs (1)
44-92: Good barrel tracing test coverage for explicitcount_test_importsvalues.The three tests cover the key scenarios: dead when not consumed (false), alive when consumed via barrel (false), and alive when test imports count (true). Previous review concerns about the misleading comment have been addressed.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| all_smells.extend(Self::check_dead_exports( | ||
| file_symbols, | ||
| entry_points, | ||
| &root_entry_points, |
There was a problem hiding this comment.
Test file exports falsely flagged as dead symbols
Medium Severity
When count_test_imports=false, check_dead_local_symbols correctly receives test_excluded and skips test files, but check_dead_exports and check_dead_methods do not. Since symbol_usages and all_project_usages already exclude test file imports, any exports defined in test files (e.g., test helpers) will appear to have zero consumers and be falsely reported as dead symbols. The get_rule_for_file post-filter only checks rule.exclude, not config.ignore, so these false positives are not caught downstream either.
Additional Locations (2)
Distinguish root vs sub-package entry points so monorepo scans correctly detect dead symbols while standalone package scans treat public API as alive. Split debug logs into debug (summaries) and trace (per-symbol) levels, and support -v (debug) / -vv (trace) via verbose count flag.
… once Eliminates filesystem I/O and redundant glob compilation: - Cache package_json_dirs in AnalysisContext (eliminates ~30-100 stat syscalls from is_root_entry_point) - Use LazyLock for COMPILED_TEST_PATTERNS (compile 17 glob patterns once instead of per-call) - Also fix clippy::missing_const_for_fn warning in verbose_to_level
fixcik
force-pushed
the
feat/dead-code-count-test-imports
branch
from
d090425 to
f9ff4e7
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@crates/archlint/src/detectors/hygiene/dead_symbols.rs`:
- Around line 14-19: COMPILED_TEST_PATTERNS currently uses std::sync::LazyLock
which requires Rust 1.80 and breaks our MSRV; replace the LazyLock
initialization with once_cell::sync::Lazy to preserve behavior on Rust 1.75: add
once_cell as a dependency if missing, change the static declaration from
LazyLock to once_cell::sync::Lazy, and keep the closure that maps
TEST_FILE_PATTERNS -> glob::Pattern the same so COMPILED_TEST_PATTERNS continues
to be lazily initialized.
- Around line 259-275: Rename the misleading parameter named entry_points to
root_entry_points across the consumption tracing chain: update the function
signatures and all internal calls for is_symbol_consumed,
is_symbol_consumed_recursive, and check_wildcard_importers (and any callers that
pass the filtered package.json set) so the parameter name reflects that this set
is the filtered "root" entry points; ensure you rename the parameter in each
function signature and update every invocation site and recursive call to use
root_entry_points (preserving types and behavior) so compilation succeeds and
semantics remain unchanged.
In `@crates/archlint/tests/dead_symbols_barrel_test.rs`:
- Around line 8-32: Add the same explanatory inline comment next to the ignore:
Vec::new() in create_dead_symbols_config to state that it intentionally
overrides default_ignore_patterns() so test files remain in the dependency graph
and the detector's TEST_FILE_PATTERNS will handle exclusion (match the comment
used in dead_code_test_only_test.rs around lines 31-32); update the comment in
dead_symbols_barrel_test.rs adjacent to ignore: Vec::new() for consistency.
- Around line 44-92: Add a fourth test (e.g., fn test_barrel_default_behavior())
that mirrors the existing barrel tests but builds the configuration without
explicitly setting count_test_imports (exercise the unwrap_or(false) default),
using create_dead_symbols_config / or construct the config to omit the
count_test_imports flag, call analyze_fixture_with_config("dead_symbols_barrel",
config), run
archlint::detectors::dead_symbols::DeadSymbolsDetector.detect(&ctx), and assert
that is_dead_symbol(&smells, "testHelper") is true (same expectation as
count_test_imports=false); model the test structure on
test_dead_code_default_flags_test_only_usage to ensure the default-path is
covered.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (16)
crates/archlint/src/api/analyzer.rscrates/archlint/src/api/options.rscrates/archlint/src/args.rscrates/archlint/src/cli.rscrates/archlint/src/config/types.rscrates/archlint/src/detectors/hygiene/dead_code.rscrates/archlint/src/detectors/hygiene/dead_symbols.rscrates/archlint/src/engine/context.rscrates/archlint/src/engine/runner.rscrates/archlint/src/incremental/state.rscrates/archlint/src/main.rscrates/archlint/src/package_json.rscrates/archlint/tests/common/mod.rscrates/archlint/tests/dead_code_test_only_test.rscrates/archlint/tests/dead_symbols_barrel_test.rsresources/archlint.schema.json
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: fixcik
Repo: archlinter/archlint PR: 36
File: crates/archlint/src/detectors/hygiene/dead_code.rs:69-74
Timestamp: 2026-02-13T17:29:28.000Z
Learning: In the archlint project, the `count_test_imports` option defaults to `false` for both `dead_code` and `dead_symbols` detectors. This is an intentional breaking change to enforce stricter hygiene: production code referenced only by tests is flagged as dead by default, encouraging teams to keep production code actively used in production contexts.
Learnt from: fixcik
Repo: archlinter/archlint PR: 36
File: crates/archlint/src/detectors/hygiene/dead_symbols.rs:29-47
Timestamp: 2026-02-13T17:29:31.835Z
Learning: In `crates/archlint/src/detectors/hygiene/dead_symbols.rs`, the `count_test_imports` option intentionally defaults to `false` as a breaking change by design. When `false`, imports from test files are not counted as usage, and code referenced only by tests is flagged as dead. Setting it to `true` restores the prior behavior where test imports count as usage.
📚 Learning: 2026-02-13T17:29:31.835Z
Learnt from: fixcik
Repo: archlinter/archlint PR: 36
File: crates/archlint/src/detectors/hygiene/dead_symbols.rs:29-47
Timestamp: 2026-02-13T17:29:31.835Z
Learning: In `crates/archlint/src/detectors/hygiene/dead_symbols.rs`, the `count_test_imports` option intentionally defaults to `false` as a breaking change by design. When `false`, imports from test files are not counted as usage, and code referenced only by tests is flagged as dead. Setting it to `true` restores the prior behavior where test imports count as usage.
Applied to files:
crates/archlint/src/api/options.rscrates/archlint/tests/dead_code_test_only_test.rscrates/archlint/src/args.rscrates/archlint/src/cli.rscrates/archlint/src/detectors/hygiene/dead_code.rscrates/archlint/src/detectors/hygiene/dead_symbols.rscrates/archlint/tests/dead_symbols_barrel_test.rscrates/archlint/src/config/types.rs
📚 Learning: 2026-02-13T17:29:28.000Z
Learnt from: fixcik
Repo: archlinter/archlint PR: 36
File: crates/archlint/src/detectors/hygiene/dead_code.rs:69-74
Timestamp: 2026-02-13T17:29:28.000Z
Learning: In the archlint project, the `count_test_imports` option defaults to `false` for both `dead_code` and `dead_symbols` detectors. This is an intentional breaking change to enforce stricter hygiene: production code referenced only by tests is flagged as dead by default, encouraging teams to keep production code actively used in production contexts.
Applied to files:
crates/archlint/src/api/options.rscrates/archlint/tests/dead_code_test_only_test.rsresources/archlint.schema.jsoncrates/archlint/src/cli.rscrates/archlint/src/detectors/hygiene/dead_code.rscrates/archlint/src/detectors/hygiene/dead_symbols.rscrates/archlint/tests/dead_symbols_barrel_test.rscrates/archlint/src/config/types.rs
📚 Learning: 2026-01-09T12:52:01.859Z
Learnt from: fixcik
Repo: archlinter/archlint PR: 17
File: crates/archlint/src/framework/detector.rs:76-91
Timestamp: 2026-01-09T12:52:01.859Z
Learning: In crates/archlint/src/framework/detector.rs, the matches_rules function uses OR semantics when both any_of and all_of fields are populated in MatchRules. This is intentional for flexibility in preset detection rules, not a bug.
Applied to files:
crates/archlint/src/detectors/hygiene/dead_code.rscrates/archlint/src/detectors/hygiene/dead_symbols.rs
🧬 Code graph analysis (6)
crates/archlint/src/engine/context.rs (1)
crates/archlint/src/incremental/state.rs (1)
new(46-67)
crates/archlint/tests/dead_code_test_only_test.rs (2)
crates/archlint/tests/common/mod.rs (1)
analyze_fixture_with_config(23-107)crates/archlint/src/detectors/hygiene/dead_code.rs (1)
new(225-288)
crates/archlint/src/main.rs (1)
crates/archlint/src/no_cli_mocks.rs (2)
style(80-82)dim(68-70)
crates/archlint/tests/dead_symbols_barrel_test.rs (1)
crates/archlint/tests/common/mod.rs (1)
analyze_fixture_with_config(23-107)
crates/archlint/src/package_json.rs (1)
crates/archlint/src/incremental/state.rs (1)
new(46-67)
crates/archlint/tests/common/mod.rs (1)
crates/archlint/src/incremental/state.rs (1)
new(46-67)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Cursor Bugbot
- GitHub Check: SonarQube
- GitHub Check: Lint
🔇 Additional comments (24)
resources/archlint.schema.json (1)
8-26: Default ignore patterns in schema align withTEST_FILE_PATTERNS.The 17 patterns here match the centralized
TEST_FILE_PATTERNSconstant inconfig/types.rs. This keeps the schema documentation consistent with runtime behavior.crates/archlint/src/api/options.rs (1)
69-69: LGTM — consistent with theverbose: u8change inScanArgs.crates/archlint/src/args.rs (1)
58-60: LGTM — standard clapArgAction::Countpattern for tiered verbosity.crates/archlint/src/package_json.rs (1)
15-15: LGTM — clean propagation ofpackage.jsondirectory discovery.The
if let Some(dir)guard onpath.parent()is appropriately defensive.Also applies to: 24-24, 40-42
crates/archlint/src/engine/context.rs (1)
45-46: LGTM —package_json_dirsproperly added toAnalysisContextand its test default.Also applies to: 114-114
crates/archlint/src/engine/runner.rs (1)
122-122: LGTM —package_json_dirscorrectly propagated fromPackageConfigintoAnalysisContext.crates/archlint/src/incremental/state.rs (1)
38-38: LGTM —package_json_dirsconsistently added to struct, constructor, andclear().Also applies to: 64-64, 83-83
crates/archlint/src/config/types.rs (2)
10-11: Confirmskip_serializing_ifwas removed fromignorefield — past review issue addressed.The previous review flagged that
skip_serializing_if = "Vec::is_empty"combined withdefault = "default_ignore_patterns"made it impossible for users to opt out by settingignore: []. The current code shows only#[serde(default = "default_ignore_patterns")], which preserves an explicit empty list correctly.
441-465: TEST_FILE_PATTERNS centralization is correctly implemented.The 17-pattern constant in
config/types.rsis now the single source of truth. Bothdead_code.rsanddead_symbols.rsimport it (lines 1), eliminating duplicates and addressing the prior mismatch betweendefault_ignore_patterns(11 patterns) and detector constants (17 patterns).crates/archlint/src/detectors/hygiene/dead_code.rs (2)
1-1: LGTM — centralized import ofTEST_FILE_PATTERNS.Clean extraction from
config::typesinstead of inlining the patterns here.
60-90:count_test_importsbranching logic is sound.The two branches correctly handle both modes: filtering out test patterns from
config.ignorewhen counting test imports, and explicitly including them when not. The defaultfalseis intentional per prior discussion. Based on learnings, this intentional breaking change enforces stricter hygiene where production code referenced only by tests is flagged as dead.crates/archlint/src/cli.rs (2)
52-54: LGTM — standard clapCountpattern for tiered verbosity.
ArgAction::Countwithu8is the idiomatic clap approach for-v/-vvflags.
202-204: Consistent withCli.verbosechange.crates/archlint/tests/common/mod.rs (1)
86-90: LGTM — correct propagation ofpackage_json_dirsthrough test setup.The new field is properly extracted from
PackageConfigand wired intoAnalysisContext, consistent with the production code path inanalyzer.rsandrunner.rs.Also applies to: 105-105
crates/archlint/tests/dead_code_test_only_test.rs (2)
38-47: LGTM — test helper correctly propagates entry points and dynamic load patterns.Previous review concerns about discarding
ctx.script_entry_pointsandctx.dynamic_load_patternshave been addressed.
67-142: Comprehensive test coverage for all threecount_test_importsscenarios.The tests validate explicit
false, explicittrue, and default (unset →false) behaviors, with good assertions on both flagged and unflagged files. The default-behavior test at line 116 properly exercises theunwrap_or(false)fallback path.crates/archlint/src/api/analyzer.rs (1)
86-86: LGTM — consistent propagation ofpackage_json_dirsthrough scan and analysis context.Follows the same pattern as
entry_pointsanddynamic_load_patterns.Also applies to: 159-159
crates/archlint/src/main.rs (2)
44-50: Cleanconst fnfor verbose level mapping.The three-tier mapping (0 → Info, 1 → Debug, 2+ → Trace) is clear and the
const fnensures zero runtime overhead.
52-126: Consistent application ofverbose_to_levelacross all command paths.Both
set_initial_log_levelandset_final_log_levelsymmetrically useverbose > 0checks andverbose_to_level()for Scan, Watch, Diff, and default paths.crates/archlint/src/detectors/hygiene/dead_symbols.rs (5)
44-64: Root entry point logic is correct and avoids filesystem I/O.The previous review noted a concern about synchronous
exists()calls. This implementation uses the precomputedpackage_json_dirsset instead, which is the right approach. The fallback on line 63 (assume root when nopackage.jsonis found) is a safe default.
314-369: Barrel tracing logic with cycle detection is well-implemented.The
check_wildcard_importersmethod correctly:
- Distinguishes true re-exports (
export * from) from namespace imports (import * as X)- Treats root entry-point barrels as public API (alive)
- Recursively traces through barrel pass-throughs with the
visitedset preventing cycles- Checks
local_usagesfor namespace importsOne edge case worth noting: if a symbol is re-exported through a chain of non-root barrels and none of those barrels is ultimately imported by a consumer, the symbol will correctly be flagged as dead since all branches of the recursion will exhaust without finding a consumer.
90-97:count_test_importsdefault is intentionallyfalse.Per prior discussion, this is a deliberate breaking change. Based on learnings, the
count_test_importsoption intentionally defaults tofalseas a breaking change by design to enforce stricter hygiene.
662-683: Correct separation ofentry_pointsvsroot_entry_pointsat the call site.Line 671 uses the full
entry_pointsto skip dead-export analysis on all entry-point files, while line 677 passesroot_entry_pointsfor consumption tracing. This ensures sub-package entry points aren't themselves flagged, but their barrel re-exports don't automatically make symbols "alive" for the root project.
371-402: Skipping test-excluded files in local symbol analysis prevents false positives.When
count_test_imports=false, test files are excluded from usage counting. It's correct to also skip test files' own local definitions from dead symbol analysis — otherwise, internal test helpers would be spuriously flagged as dead.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| static COMPILED_TEST_PATTERNS: LazyLock<Vec<glob::Pattern>> = LazyLock::new(|| { | ||
| TEST_FILE_PATTERNS | ||
| .iter() | ||
| .filter_map(|p| glob::Pattern::new(p).ok()) | ||
| .collect() | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd crates/archlint && cat Cargo.toml | grep -A 5 -B 5 "edition\|rust-version"Repository: archlinter/archlint
Length of output: 277
🏁 Script executed:
cd crates/archlint && sed -n '1,30p' src/detectors/hygiene/dead_symbols.rsRepository: archlinter/archlint
Length of output: 1023
🏁 Script executed:
cd crates/archlint && sed -n '14,19p' src/detectors/hygiene/dead_symbols.rsRepository: archlinter/archlint
Length of output: 257
🏁 Script executed:
cat Cargo.toml | grep -A 3 "edition\|rust-version"Repository: archlinter/archlint
Length of output: 242
🏁 Script executed:
# Check for any MSRV or rust-version in workspace or root
rg "rust-version|MSRV" . --max-depth 2 2>/dev/null | head -20Repository: archlinter/archlint
Length of output: 97
LazyLock requires Rust 1.80+, but the project's MSRV is 1.75—code will not compile on supported versions.
The review approves the LazyLock usage, but LazyLock stabilized in Rust 1.80, while the workspace specifies rust-version = "1.75". This violates the project's minimum supported Rust version. Replace LazyLock with once_cell::sync::Lazy (or add a dependency on once_cell for older Rust versions) to maintain MSRV compatibility.
🤖 Prompt for AI Agents
In `@crates/archlint/src/detectors/hygiene/dead_symbols.rs` around lines 14 - 19,
COMPILED_TEST_PATTERNS currently uses std::sync::LazyLock which requires Rust
1.80 and breaks our MSRV; replace the LazyLock initialization with
once_cell::sync::Lazy to preserve behavior on Rust 1.75: add once_cell as a
dependency if missing, change the static declaration from LazyLock to
once_cell::sync::Lazy, and keep the closure that maps TEST_FILE_PATTERNS ->
glob::Pattern the same so COMPILED_TEST_PATTERNS continues to be lazily
initialized.
| fn is_symbol_consumed( | ||
| symbol_usages: &HashMap<(PathBuf, String), HashSet<PathBuf>>, | ||
| file_symbols: &HashMap<PathBuf, FileSymbols>, | ||
| entry_points: &HashSet<PathBuf>, | ||
| file_path: &Path, | ||
| symbol_name: &str, | ||
| ) -> bool { | ||
| let mut visited = HashSet::new(); | ||
| Self::is_symbol_consumed_recursive( | ||
| symbol_usages, | ||
| file_symbols, | ||
| entry_points, | ||
| file_path, | ||
| symbol_name, | ||
| &mut visited, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Rename entry_points parameter to root_entry_points in the consumption tracing chain.
is_symbol_consumed, is_symbol_consumed_recursive, and check_wildcard_importers all receive root_entry_points (filtered to the scanned project's own package.json) but name the parameter entry_points. This is misleading since the codebase has both concepts (e.g., check_dead_exports on line 663 accepts full entry_points alongside root_entry_points). Renaming clarifies the semantics without changing behavior.
♻️ Proposed rename across the consumption tracing chain
fn is_symbol_consumed(
symbol_usages: &HashMap<(PathBuf, String), HashSet<PathBuf>>,
file_symbols: &HashMap<PathBuf, FileSymbols>,
- entry_points: &HashSet<PathBuf>,
+ root_entry_points: &HashSet<PathBuf>,
file_path: &Path,
symbol_name: &str,
) -> bool {
let mut visited = HashSet::new();
Self::is_symbol_consumed_recursive(
symbol_usages,
file_symbols,
- entry_points,
+ root_entry_points,
file_path,
symbol_name,
&mut visited,
)
}
fn is_symbol_consumed_recursive(
symbol_usages: &HashMap<(PathBuf, String), HashSet<PathBuf>>,
file_symbols: &HashMap<PathBuf, FileSymbols>,
- entry_points: &HashSet<PathBuf>,
+ root_entry_points: &HashSet<PathBuf>,
file_path: &Path,
symbol_name: &str,
visited: &mut HashSet<PathBuf>,Apply the same rename in check_wildcard_importers.
🤖 Prompt for AI Agents
In `@crates/archlint/src/detectors/hygiene/dead_symbols.rs` around lines 259 -
275, Rename the misleading parameter named entry_points to root_entry_points
across the consumption tracing chain: update the function signatures and all
internal calls for is_symbol_consumed, is_symbol_consumed_recursive, and
check_wildcard_importers (and any callers that pass the filtered package.json
set) so the parameter name reflects that this set is the filtered "root" entry
points; ensure you rename the parameter in each function signature and update
every invocation site and recursive call to use root_entry_points (preserving
types and behavior) so compilation succeeds and semantics remain unchanged.
| fn create_dead_symbols_config(count_test_imports: bool) -> Config { | ||
| let mut options = serde_yaml::Mapping::new(); | ||
| options.insert( | ||
| serde_yaml::Value::String("count_test_imports".to_string()), | ||
| serde_yaml::Value::Bool(count_test_imports), | ||
| ); | ||
|
|
||
| let mut rules = HashMap::new(); | ||
| rules.insert( | ||
| "dead_symbols".to_string(), | ||
| RuleConfig::Full(RuleFullConfig { | ||
| enabled: Some(true), | ||
| exclude: Vec::new(), | ||
| severity: None, | ||
| options: serde_yaml::Value::Mapping(options), | ||
| }), | ||
| ); | ||
|
|
||
| Config { | ||
| rules, | ||
| entry_points: vec!["main.ts".to_string()], | ||
| ignore: Vec::new(), | ||
| ..Default::default() | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Well-structured test helper and config setup.
ignore: Vec::new() correctly overrides default_ignore_patterns() to ensure test files are present in the dependency graph while letting the detector's own TEST_FILE_PATTERNS handle exclusion logic. Consider adding the same inline comment you added in dead_code_test_only_test.rs (lines 31-32) for consistency.
🤖 Prompt for AI Agents
In `@crates/archlint/tests/dead_symbols_barrel_test.rs` around lines 8 - 32, Add
the same explanatory inline comment next to the ignore: Vec::new() in
create_dead_symbols_config to state that it intentionally overrides
default_ignore_patterns() so test files remain in the dependency graph and the
detector's TEST_FILE_PATTERNS will handle exclusion (match the comment used in
dead_code_test_only_test.rs around lines 31-32); update the comment in
dead_symbols_barrel_test.rs adjacent to ignore: Vec::new() for consistency.
| #[test] | ||
| fn test_barrel_reexport_without_consumer_is_dead() { | ||
| // testHelper is re-exported through index.ts barrel, but no production | ||
| // code imports it from the barrel. Only the test file imports it directly. | ||
| // With count_test_imports=false, testHelper should be dead. | ||
| let config = create_dead_symbols_config(false); | ||
| let ctx = analyze_fixture_with_config("dead_symbols_barrel", config); | ||
|
|
||
| let detector = archlint::detectors::dead_symbols::DeadSymbolsDetector; | ||
| let smells = detector.detect(&ctx); | ||
|
|
||
| assert!( | ||
| is_dead_symbol(&smells, "testHelper"), | ||
| "testHelper should be flagged: re-exported through barrel but never consumed. Got smells: {:?}", | ||
| smells.iter().filter_map(|s| if let archlint::detectors::SmellType::DeadSymbol { name, .. } = &s.smell_type { Some(name.as_str()) } else { None }).collect::<Vec<_>>() | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_barrel_reexport_with_consumer_is_alive() { | ||
| // prodHelper is re-exported through index.ts barrel AND imported | ||
| // by main.ts from the barrel. It should NOT be dead. | ||
| let config = create_dead_symbols_config(false); | ||
| let ctx = analyze_fixture_with_config("dead_symbols_barrel", config); | ||
|
|
||
| let detector = archlint::detectors::dead_symbols::DeadSymbolsDetector; | ||
| let smells = detector.detect(&ctx); | ||
|
|
||
| assert!( | ||
| !is_dead_symbol(&smells, "prodHelper"), | ||
| "prodHelper should NOT be flagged: consumed through barrel by main.ts" | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_barrel_with_test_imports_counted() { | ||
| // With count_test_imports=true, testHelper should NOT be dead | ||
| // because the test file's import counts as usage. | ||
| let config = create_dead_symbols_config(true); | ||
| let ctx = analyze_fixture_with_config("dead_symbols_barrel", config); | ||
|
|
||
| let detector = archlint::detectors::dead_symbols::DeadSymbolsDetector; | ||
| let smells = detector.detect(&ctx); | ||
|
|
||
| assert!( | ||
| !is_dead_symbol(&smells, "testHelper"), | ||
| "testHelper should NOT be flagged when count_test_imports=true" | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Good coverage for barrel re-export tracing with both count_test_imports modes.
The three tests validate the key scenarios: dead symbol via barrel, alive symbol via barrel consumer, and test-import counting. Consider adding a fourth test for the default behavior (no explicit count_test_imports option), analogous to test_dead_code_default_flags_test_only_usage in dead_code_test_only_test.rs, to ensure the unwrap_or(false) default is exercised end-to-end.
🤖 Prompt for AI Agents
In `@crates/archlint/tests/dead_symbols_barrel_test.rs` around lines 44 - 92, Add
a fourth test (e.g., fn test_barrel_default_behavior()) that mirrors the
existing barrel tests but builds the configuration without explicitly setting
count_test_imports (exercise the unwrap_or(false) default), using
create_dead_symbols_config / or construct the config to omit the
count_test_imports flag, call analyze_fixture_with_config("dead_symbols_barrel",
config), run
archlint::detectors::dead_symbols::DeadSymbolsDetector.detect(&ctx), and assert
that is_dead_symbol(&smells, "testHelper") is true (same expectation as
count_test_imports=false); model the test structure on
test_dead_code_default_flags_test_only_usage to ensure the default-path is
covered.
| all_smells.extend(Self::check_dead_exports( | ||
| file_symbols, | ||
| entry_points, | ||
| &root_entry_points, |
There was a problem hiding this comment.
Test files not skipped in exports/methods checks
Medium Severity
When count_test_imports=false, test_excluded is passed to check_dead_local_symbols to skip test files, but check_dead_exports and check_dead_methods never receive test_excluded and don't skip test files. Since symbol_usages and all_project_usages already have test-file imports stripped out, exports and methods defined in test files appear to have no consumers, producing false positive "dead symbol" reports. The post-filter in detect() uses rule.exclude (not config.ignore), so default test patterns from config.ignore don't catch these either.
Additional Locations (1)
…atibility LazyLock requires Rust 1.80+, but project MSRV is 1.75. Replace with once_cell::sync::Lazy to maintain compatibility with supported Rust versions. Fixes: thread 8ccf0a (CodeRabbit critical issue) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…ad_methods When count_test_imports=false, test_excluded was only passed to check_dead_local_symbols. Exports and methods defined in test files appeared to have no consumers (since symbol_usages excludes test imports), producing false positive "dead symbol" reports. Now both check_dead_exports and check_dead_methods receive test_excluded and skip test files when count_test_imports=false. Fixes: threads 8446a8, cbc104 (Cursor bot medium severity issues) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…nsumption tracing In the consumption tracing chain (is_symbol_consumed, is_symbol_consumed_recursive, check_wildcard_importers, check_file_exports, check_export_usage), the parameter was named entry_points but actually received root_entry_points (filtered to the scanned project's own package.json). This was misleading since the codebase has both concepts - check_dead_exports accepts full entry_points AND root_entry_points separately. Renaming clarifies the semantics without changing behavior. Addresses: thread a7df09 (CodeRabbit refactor suggestion) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…tracing - Add inline comment explaining ignore: Vec::new() override (consistency with dead_code_test_only_test.rs) - Add test_barrel_default_flags_test_only_usage to verify unwrap_or(false) default is exercised end-to-end Addresses: threads 9aca53, 98f46d (CodeRabbit trivial nitpicks) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| // Actual namespace import (import * as X): check if symbol name is used | ||
| if syms.local_usages.contains(symbol_name) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Barrel re-export else if skips namespace import usage check
Low Severity
When an importer file does both export * from './source' (barrel re-export) and import * as X from './source' (namespace import), the is_reexport check on line 334 returns true via .any() finding the re-export import entry. Because of the else if on line 364, the local_usages check for the namespace import is skipped entirely. This means the file's own consumption of the symbol through the namespace import goes undetected, and the symbol may be falsely flagged as dead if no downstream consumer exists after barrel tracing. The is_used_by_name check in check_export_usage partially mitigates this in most practical cases.
| "testHelper should be flagged: re-exported through barrel but never consumed. Got smells: {:?}", | ||
| smells.iter().filter_map(|s| if let archlint::detectors::SmellType::DeadSymbol { name, .. } = &s.smell_type { Some(name.as_str()) } else { None }).collect::<Vec<_>>() | ||
| ); | ||
| } |
There was a problem hiding this comment.
Integration tests don't exercise barrel tracing code paths
Medium Severity
The analyze_fixture_with_config helper inserts parsed symbols with unresolved relative import sources (e.g. ./prod_util) while file_symbols keys are absolute paths. Unlike production's EngineBuilder::resolve_symbols, the test helper never resolves imports. This causes build_symbol_imports_map keys to mismatch is_symbol_consumed_recursive lookups, so the barrel tracing logic is never actually exercised. All four barrel tests pass only because the is_used_by_name fallback check catches symbols via local_usages, giving false confidence that barrel tracing works.
Additional Locations (1)
1 participant
Summary
count_test_importsoption fordead_codeanddead_symbolsdetectors — code used only in tests is flagged as dead by default (false); set totrueto restore old behavior.test.ts,.spec.ts,__tests__/, etc.)dead_symbols:export * fromis no longer treated as actual usage — traces through barrel chains to verify the symbol reaches a real consumerpackage.jsonwhitelist symbols as public API; sub-package entry points in monorepos are traced normally-vfor debug (summaries),-vvfor trace (per-symbol tracing)Config.ignoreserde default: test file patterns are now preserved when loading from YAML config filesConfiguration
Test plan
count_test_imports=falseflags test-only codecount_test_imports=truekeeps test-only codeexport *are not falsely marked alive-vshows debug logs,-vvshows trace logs, no flags = clean outputCloses #35
Closes #37
Note
Medium Risk
Changes core detector logic for dead code/symbol detection (including monorepo entry-point handling), which can shift results and affect CI gating; new tests and explicit option defaults reduce risk but regressions in edge cases are still possible.
Overview
Improves
dead_code/dead_symbolsaccuracy by adding acount_test_importsoption (default false) so code referenced only from test files can be flagged as dead, while allowing opt-in to count test imports as usage.dead_symbolsnow distinguishes consumption from barrel re-exports (export * from) by tracing through barrel chains (cycle-safe) and only treating re-exported symbols as public API when they flow through root entry points (derived from discoveredpackage.jsondirectories for monorepos). This also introduces tiered CLI verbosity (-vdebug,-vvtrace) and centralizes default test ignore patterns in config/schema, with new fixtures/tests covering the new behaviors.Written by Cursor Bugbot for commit 13cdad2. This will update automatically on new commits. Configure here.