chore(native): clean up pre-existing Rust dead-code warnings#1484
Conversation
… diff Adds snapshot-pre-bash.sh (PreToolUse Bash) + track-bash-writes.sh (PostToolUse Bash): the pre-hook captures git status --porcelain to a per-worktree temp file before each Bash call; the post-hook diffs the before/after state and appends newly modified or created files to .claude/session-edits.log. This closes the gap where files written by sed -i, printf redirects, tee, heredocs, or build tools (Cargo.lock, lockfiles) were never recorded, causing guard-git.sh to emit false-positive BLOCKED errors. Closes #1457
- clojure.rs: annotate lifetime-anchor assignment to silence false-positive - cfg.rs: remove never-called start_line_of method - complexity.rs: remove never-constructed NotHandled variant; convert irrefutable if-let patterns to plain let destructures - dataflow.rs: remove never-read callee fields from CallReturn/Destructured - incremental.rs: remove never-read lang field from CacheEntry cargo check and cargo clippy both clean after these changes.
|
Claude finished @carlos-alm's task —— View job PR Review: chore(native): clean up pre-existing Rust dead-code warnings
|
| Commit | Concern |
|---|---|
3892e7d |
.gitignore — napi-generated artifacts |
ef8ea4f |
Remove unused biome suppression in visitor.test.ts |
a372b82 |
Fix titan-run SKILL.md sync issue |
9a52c7c |
New feature: two new hook scripts + settings.json wiring (snapshot-pre-bash.sh + track-bash-writes.sh) |
85a26df |
Rust dead code removal (what the PR title describes) |
Per CLAUDE.md: "One PR = one concern." This PR bundles unrelated changes that each deserve their own PR. The review below covers all of them.
Phase 1: Root Cause Analysis
Rust dead code (commit 85a26df): All five removals are genuine dead code fixes — the compiler's warnings are being addressed correctly, not suppressed:
cfg.rs:654-659—set_start_line_ofmethod: truly unreferenced, safe to remove.complexity.rs—NotHandledvariant was never constructed; replacingif let BranchAction::Handled { .. }withlet BranchAction::Handled { .. }is correct (single-variant enum, irrefutable pattern). Clean refactor.dataflow.rs—calleefields inCallReturn/Destructuredvariants: confirmed never read — fields are pattern-matched against infind_bindingat line 909-914 but the match is purely on discriminant (no field access). Removing them is safe.incremental.rs—langfield: removed fromCacheEntrystruct;LanguageKindwas stored but never used after storing.clojure.rs:57—#[allow(unused_assignments)]onnext_ns_owned: the comment explains the lifetime-anchor correctly. This is the right approach — the assignment IS semantically meaningful (the borrow on line 63 reads from it). The suppress is narrowly scoped and well-justified.
Root cause is correctly identified and fixed in each case. No underlying issue is being hidden.
Phase 2: Configuration Change Detection (HIGH SCRUTINY)
Two new hook scripts were added in commit 9a52c7c:
snapshot-pre-bash.sh (PreToolUse/Bash):
- Takes a
git status --porcelainsnapshot before each Bash call, keyed by a project-root hash, written to/tmp/. - Uses
node -einline for JSON parsing and SHA1 hashing — unusual but functional. - Has a fast-path skip for read-only commands. Minor concern: the fast-path regex (
^\s*(ls|cat|head|...)) is prefix-anchored and will skipls foo && echo foo > bar— a compound command starting withls. This is a tolerable limitation given the hook is informational-only (exits 0 always). - Risk: low — informational hook, never blocks.
track-bash-writes.sh (PostToolUse/Bash):
- Diffs before/after
git status --porcelainto find files newly dirtied by the Bash call, then appends to.claude/session-edits.log. - Logic is sound: uses
comm -13to find AFTER paths not in BEFORE. - Minor concern at line 102:
awk '{print $2}'on the session-edits log assumes a fixed two-column format (TIMESTAMP PATH). IfPATHcontains a space, only the first segment of the path is captured, causing false "already logged" misses. However, this just means a path gets double-logged — it does not cause incorrect blocking. Tolerable for a tracking hook.
settings.json change: Wires both hooks into PreToolUse/Bash and PostToolUse/Bash. The new entries follow the existing pattern. No rules are relaxed.
Test file change (visitor.test.ts): Removes a // biome-ignore lint/suspicious/noExplicitAny comment. This is correct — the suppression was flagged as unused (biome reports this), so removing it cleans up a stale comment. The any annotation on let parse: any remains. Test assertions are unchanged. ✓
Phase 3: Test Integrity
tests/unit/visitor.test.ts: Only the unused biome suppression comment was removed. Zero assertion changes. ✓- All Rust changes are removals — no new code paths, no new tests needed. ✓
Phase 4: Code Quality
complexity.rs: The NotHandled variant removal + if let → let conversion is idiomatic. Single-variant enums should use plain let. The result compiles to identical machine code.
dataflow.rs: LocalSource::CallReturn and LocalSource::Destructured variants lose their unnamed fields. The find_binding match at line 909-914 still pattern-matches on discriminant only — behavior is identical.
clojure.rs: The #[allow(unused_assignments)] annotation is placed at the correct granularity (the specific let mut statement, not the entire function). The comment above it explaining the borrow-lifetime reason is clear and accurate.
incremental.rs: CacheEntry loses the lang: LanguageKind field. Confirmed the field was set in the constructor but never accessed. The parse_file method still correctly calls lang.tree_sitter_language() using the local variable — clean.
Phase 5: Backlog Compliance
- Zero-dep: ✓ — No new runtime npm dependencies.
- Foundation-aligned: ✓ — Rust dead code removal and hook improvements are purely infrastructural; no cloud calls, no new external toolchain.
- Problem-fit: 2/5 — Dead code cleanup and hook quality improvements don't directly help AI agents navigate codebases. Valuable maintenance work but not core to the stated problem.
- Breaking: No — all changes are removals or infrastructure tweaks. No DB schema changes, no API/CLI changes, no MCP contract changes.
- Tier: 1
Critical Concerns
-
Mixed PR (medium): This bundles 5 unrelated commits. The new Bash tracking hooks (
9a52c7c) are a meaningful behavioral change (altering howsession-edits.logis populated) that belongs in its own PR. Per the project's own CLAUDE.md rules, this should be split. -
Hook regex false-skip (low):
snapshot-pre-bash.shline 29 — the fast-path regex will incorrectly skip compound commands likels && echo foo > barorgit log; sed -i .... Since the hook is informational-only (exits 0), the worst outcome is a missed file write in the session log. Not blocking. -
Space-in-path double-logging (low):
track-bash-writes.shline 102 —awk '{print $2}'will mangle paths containing spaces, causing double-log entries. Not blocking.
Final Assessment
## Root Cause Analysis
Problem Being Solved: Remove 6 pre-existing Rust dead-code warnings from `cargo check`.
Why This Approach: Correct — each warning is addressed at source, not suppressed globally.
Risk Assessment: Low for the Rust changes. The bundled hook feature is more behavioral.
## Backlog Compliance
- Zero-dep: ✓
- Foundation-aligned: ✓
- Problem-fit: 2/5 — maintenance work, not AI-navigation
- Breaking: No
- Tier: 1
## Critical Concerns
1. Mixed PR — 5 unrelated commits (Rust cleanup + new hook feature + SKILL.md fix + gitignore + test cleanup)
2. Bash hook fast-path regex false-skip on compound commands (low, informational-only)
3. Space-in-path double-logging in track-bash-writes.sh (low, tolerable)
## Final Recommendation
- Rating: ⭐⭐⭐ (3/5)
- Action: APPROVE WITH CONDITIONS
- Reasoning: The Rust dead-code removals are clean and correct. The clojure.rs
lifetime-anchor annotation is properly scoped and justified. The hook changes
are sound in principle but belong in their own PR per the project's own
one-concern-per-PR rule. The two minor hook bugs (fast-path regex, space-in-path)
are tolerable given informational-only semantics, but worth fixing. Recommend
merging after either (a) splitting the hook feature into a separate PR, or
(b) accepting the mixed PR given the low risk of the bundled changes.
Codegraph Impact Analysis7 functions changed → 123 callers affected across 32 files
|
Greptile SummaryThis PR eliminates six pre-existing dead-code compiler warnings across five files without changing any runtime behaviour. All removals and suppressions are correct and the diff is internally consistent.
Confidence Score: 5/5Safe to merge — all changes are dead-code removals with no runtime behaviour affected. Every change either deletes an unreachable variant/field/method or suppresses a lint for a variable that is genuinely load-bearing. The irrefutable No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[cargo check / clippy warnings] --> B{Warning type}
B -->|unused method| C[cfg.rs\nRemove start_line_of]
B -->|unused variant| D[complexity.rs\nRemove NotHandled\nif let → let]
B -->|unused struct fields| E[dataflow.rs\nRemove callee fields\nUpdate 3 call sites + match arms]
B -->|unused struct field| F[incremental.rs\nRemove lang field\nUpdate insert site]
B -->|unused assignment| G[clojure.rs\nAdd allow attribute\nwith explanatory comment]
C --> H[Zero warnings]
D --> H
E --> H
F --> H
G --> H
Reviews (6): Last reviewed commit: "chore: merge origin/main into chore/rust..." | Re-trigger Greptile |
…urce variants
CallReturn and Destructured became unit variants when their callee fields
were removed. The find_binding match arms still used { .. } struct-pattern
syntax which implies ignored fields where none exist. Simplify to plain
unit patterns.
|
Fixed — simplified the stale struct-pattern syntax in |
|
Addressed Claude's feedback: the stale struct-pattern syntax in |
Resolves add/add conflicts in .claude/hooks/snapshot-pre-bash.sh and .claude/hooks/track-bash-writes.sh by taking main's version, which includes the concurrent-safe per-command hash keying introduced in #1483.
|
Resolved merge conflict from the #1483 overlap: both branches added the same two hook scripts (.claude/hooks/snapshot-pre-bash.sh and .claude/hooks/track-bash-writes.sh) independently, causing an add/add conflict. Resolved by taking main's version from #1483, which includes the concurrent-safe per-command-hash keying. The Rust dead-code changes are unaffected. See merge commit a6e50da. |
1 participant
Removes 6 pre-existing dead-code warnings from
cargo check:clojure.rs: annotate the lifetime-anchornext_ns_ownedassignment with#[allow(unused_assignments)]— the variable is load-bearing (extends theString's lifetime so the&strborrow stays valid), but the compiler can't see through the borrowcfg.rs: remove never-calledstart_line_ofmethodcomplexity.rs: remove never-constructedNotHandledvariant; convert the resulting irrefutableif letpatterns to plainletdestructures (no behaviour change)dataflow.rs: remove never-readcalleefields fromCallReturnandDestructuredvariantsincremental.rs: remove never-readlangfield fromCacheEntrycargo checkandcargo clippyboth produce zero warnings after these changes.Closes #1456