Trace option 2b

[navicore]

No description provided.

[claude]

Pull Request Review: Trace option 2b

Summary

This PR implements Phase 2b of resource leak detection, adding cross-word analysis and proper branch joining for resource tracking. This is a significant improvement over Phase 2a, which only performed single-word analysis and used conservative (then-branch-only) state merging.

Code Quality: ✅ Excellent

Strengths

1. Well-structured architecture: The two-pass approach is elegant:

   • Pass 1: Collect resource return information for each word
   • Pass 2: Analyze with cross-word context

2. Comprehensive documentation: The module-level comments clearly explain the approach, architecture, and known limitations.

3. Proper separation of concerns:

   • ProgramResourceAnalyzer for cross-word analysis
   • ResourceAnalyzer maintained for backward compatibility/single-word analysis
   • Clear distinction between simulation (pass 1) and analysis (pass 2)

4. Strong test coverage: 15 unit tests covering various scenarios including:

   • Basic leak detection
   • Branch consistency checking
   • Stack operation tracking
   • Resource transfer via strand.spawn
   • Complex multi-operation scenarios

Potential Issues Found

🟡 Medium: Code Duplication Between Analyzers

Location: resource_lint.rs:400-850 and resource_lint.rs:1000-1400

The simulate_word_call and analyze_word_call_with_context methods have significant code duplication (~95% identical). Similarly for statement and statements methods. This creates maintenance burden - any bug fix or new stack operation must be updated in multiple places.

Recommendation: Consider refactoring to share the core logic, perhaps with a trait or a shared internal method that takes a "emit diagnostics" flag.

🟡 Medium: Potential False Negatives with dup/over on Resources

Location: resource_lint.rs:693-722, resource_lint.rs:1093-1120

When resources are duplicated via dup or over, both copies share the same id. The current implementation tracks consumption by ID, which means:

• If one copy is properly consumed (e.g., chan.close), the other copy won't warn when dropped
• This could miss legitimate double-free scenarios or use-after-free patterns

Example:

: potential-issue ( -- )
  chan.make dup    # Two references, same ID
  chan.close       # Consumes one copy by ID
  drop            # Drops second copy - no warning because ID marked consumed
;

Recommendation: Consider tracking resource instances separately from IDs, or document this limitation explicitly in the "Known Limitations" section.

🟢 Low: join Method Could Create Unbounded Stack Growth

Location: resource_lint.rs:220-288

The join method appends resources from the other branch that aren't in self:

for (id, resource) in other_resources {
    if !self_resource_ids.contains(&id) && !definitely_consumed.iter().any(|r| r.id == id) {
        joined_stack.push(StackValue::Resource(resource));
    }
}

In pathological cases with many branches having different stack shapes, this could cause the abstract stack to grow. However, this is unlikely in practice since Seq requires balanced stack effects in branches.

Recommendation: Add a comment explaining the assumption about balanced stack effects, or add a defensive check/warning if stacks differ significantly in depth.

🟢 Low: Missing Stack Effect Information for Some Operations

Location: resource_lint.rs:795-799

"chan.send" | "chan.receive" => {
    state.pop();
    state.pop();
    state.push_unknown();
    state.push_unknown();
}

The comment shows these operations don't accurately model their full stack effects. This is acknowledged in limitations but could lead to analysis imprecision.

Recommendation: Document the actual stack effects for these operations in comments for future improvement.

Performance Considerations: ✅ Good

• Two-pass approach is O(n*m) where n = number of words, m = average word body size
• HashMap lookups for word info are O(1) amortized
• Cloning states for branch analysis could be expensive for deep nesting, but reasonable for typical code
• No obvious performance concerns for typical codebases

Suggestion: Consider adding a benchmark for the resource analyzer on medium/large codebases to track performance regression.

Security Concerns: ✅ None Identified

This is a static analysis tool with no security implications. The analyzer itself:

• Doesn't execute user code
• Doesn't modify files
• Only produces diagnostic messages
• Has no external I/O or network access

Best Practices

✅ Followed Well

• Comprehensive error messages with line numbers and actionable suggestions
• Proper use of Rust idioms (let-chains, pattern matching)
• Conservative analysis (false negatives over false positives)
• Good test coverage with descriptive test names

🔄 Could Improve

• Public API surface: Both ResourceAnalyzer and ProgramResourceAnalyzer are exported. Consider deprecating ResourceAnalyzer or making it private if cross-word analysis should always be used.
• Diagnostic categorization: All diagnostics use Severity::Warning. Consider using different severities or adding confidence levels.

Additional Observations

Benchmark File Changes

The benchmarks/LATEST_RUN.txt update appears to be an automated timestamp update. This is fine but consider:

• Whether benchmark files should be committed (could use .gitignore and CI artifacts instead)
• Adding benchmark results to PR descriptions for performance-sensitive changes

Integration Changes Look Good

The changes to lib.rs and main.rs are minimal and correct:

• Proper export of new ProgramResourceAnalyzer
• Correct usage in lint_file replacing word-by-word analysis with program-level analysis
• Good migration from Phase 2a to Phase 2b

Recommendations Summary

High Priority

1. ✅ Merge as-is - the code is production-ready despite the issues noted

Medium Priority

2. 🔧 Address code duplication between simulate_* and analyze_* methods (future refactor)
3. 📝 Document the shared-ID behavior for duplicated resources in "Known Limitations"

Low Priority

4. 📚 Add benchmark for resource analyzer performance
5. 🧹 Consider API cleanup (single public analyzer)
6. 💬 Add stack effect comments for incomplete operations

Verdict: ✅ APPROVE

This is excellent work that significantly improves the resource leak detection capabilities. The implementation is sound, well-tested, and properly documented. The issues identified are minor and don't block merging - they can be addressed in follow-up PRs if desired.

The cross-word analysis and proper branch joining are important improvements that will catch more real bugs while maintaining low false-positive rates.

---

Review performed by Claude Code on crates/compiler/src/resource_lint.rs (+966 lines)