⏺ All tests pass. The new deep-nesting lint is now active.

[navicore]

Summary:

• Added MAX_NESTING_DEPTH = 4 constant
• Added structural analysis that counts if/else nesting depth in the AST
• Triggers a hint when nesting reaches 4+ levels
• Found 4 issues in the lisp examples, including the 7-level monster in eval-builtin-with-env

The lint suggests using cond or extracting to helper words. That 7-level function is a perfect candidate for cond:

Current (7 levels of if/else):

dup "+" string.equal? if drop eval-add-with-env else
dup "-" string.equal? if drop eval-sub-with-env else ...7 levels deep... then then

With cond (flat):

[ dup "+" string.equal? ] [ drop eval-add-with-env ]
[ dup "-" string.equal? ] [ drop eval-sub-with-env ]
[ dup "*" string.equal? ] [ drop eval-mul-with-env ]
...
[ true ] [ drop drop slist ]
7 cond

[claude]

Code Review - PR #283: Deep Nesting Lint

Great work adding structural analysis to the linter! This is a valuable addition that will help improve code quality. Here's my detailed feedback:

---

✅ Strengths

1. Well-designed architecture: The recursive AST traversal properly handles all statement types (If, Quotation, Match)
2. Clear intent: The code is well-commented and the purpose is immediately clear
3. Good default threshold: MAX_NESTING_DEPTH = 4 is a reasonable starting point
4. Proper severity: Using Severity::Hint is appropriate for a stylistic concern
5. Accurate detection: The algorithm correctly identifies the problematic 7-level nesting in eval-builtin-with-env

---

🔍 Code Quality Issues

1. Inconsistent nesting calculation for quotations (lint.rs:298-303)

The current implementation treats quotations as "starting fresh" with a nesting count of 0. However, this may not align with user expectations in all cases.

Current behavior:

Statement::Quotation { body, .. } => {
    // Quotations start fresh nesting count (they're separate code blocks)
    body.iter()
        .map(|s| Self::if_nesting_depth(s, 0))
        .max()
        .unwrap_or(0)
}

Question to consider: Should a quotation containing deeply nested ifs be flagged even if it's inside another if? The current implementation would reset the count, potentially missing complex nested logic. Consider whether this is the desired behavior or if quotations should inherit some context.

2. Missing test coverage ⚠️

No unit tests were added for the new functionality. This is critical for:

• Verifying the nesting depth calculation is correct
• Ensuring edge cases are handled (empty statements, quotations, match arms)
• Preventing regressions

Recommended tests:

#[test]
fn test_deep_nesting_detected() {
    // Test that 4+ levels triggers the lint
}

#[test]
fn test_shallow_nesting_ok() {
    // Test that 3 levels doesn't trigger
}

#[test]
fn test_quotation_nesting_behavior() {
    // Test how quotations affect nesting count
}

#[test]
fn test_match_arms_dont_count_as_nesting() {
    // Verify match arms work as expected
}

3. Potential off-by-one in edge cases

Lines 283-286 and 289-294: The use of .unwrap_or(new_depth) in both branches means that an empty branch still counts as adding one level of nesting. Is this intentional?

Example:

: test
  condition if
    # Empty then branch - does this count as depth 1 or 0?
  then
;

Current code would count this as depth 1 due to unwrap_or(new_depth). Consider if empty branches should be treated differently.

---

🐛 Potential Bugs

4. Diagnostic location information is incomplete (lint.rs:240-256)

The deep-nesting diagnostic uses start_index: 0, end_index: 0 and fallback_line, which provides poor user experience compared to pattern-based lints.

Issues:

• No precise location of the deeply nested code
• No column information
• Users can't easily navigate to the problematic section

Suggestion: Consider traversing the AST to find the specific If statement that exceeds the threshold and use its span information.

---

🎯 Best Practices

5. Magic number in comparison (lint.rs:239)

if max_depth >= MAX_NESTING_DEPTH {

Consider making the comparison operator part of the constant's documentation, or using a named function like is_deeply_nested(max_depth) for better clarity.

6. Error message could be more actionable

Current message:

"deeply nested if/else ({} levels) - consider using `cond` or extracting to helper words"

Enhancement: Add a link to documentation or examples showing how to refactor using cond. This would be especially helpful for new users.

---

⚡ Performance Considerations

No significant performance issues. The algorithm is O(n) where n is the number of statements, which is acceptable. The recursive approach is clean and matches the existing codebase style.

---

🔒 Security Concerns

None identified. This is a static analysis feature with no security implications.

---

📝 Documentation

The constant MAX_NESTING_DEPTH has good inline documentation. Consider adding:

1. Module-level documentation explaining the new structural lint
2. Examples in the lint engine's doc comments
3. User-facing documentation about how to interpret and fix deep-nesting warnings

---

✅ Recommendations

Must have before merge:

1. Add unit tests for the new nesting depth functions
2. Verify and document the behavior for quotations (reset to 0 vs. inherited depth)

Should have:
3. Improve diagnostic location information to point to the specific problematic statement
4. Consider extracting the nesting depth logic to a separate, testable module

Nice to have:
5. Add integration test with a real .seq file showing the lint in action
6. Enhance error message with documentation links
7. Make the threshold configurable via lint config

---

📊 Summary

Category	Rating	Notes
Code Quality	⭐⭐⭐⭐☆	Clean implementation, needs tests
Correctness	⭐⭐⭐⭐☆	Logic appears sound, edge cases need verification
Performance	⭐⭐⭐⭐⭐	Efficient O(n) traversal
Security	⭐⭐⭐⭐⭐	No concerns
Test Coverage	⭐⭐☆☆☆	Missing tests for new functionality
Documentation	⭐⭐⭐☆☆	Inline comments good, needs broader docs

Overall: Approve with minor changes recommended

The core implementation is solid and addresses a real code quality issue. The main gap is test coverage. Once tests are added and the quotation behavior is clarified, this will be ready to merge.

Great job identifying the 7-level nesting monster in eval-builtin-with-env - that's exactly the kind of issue this lint should catch! 🎯

[claude]

PR Review: Deep Nesting Lint

Summary

This PR adds a structural lint that detects deeply nested if/else chains (≥4 levels) and suggests using cond or helper words. The implementation is clean and well-integrated into the existing lint infrastructure.

Code Quality: ✅ Strong

Strengths:

• Clean, readable implementation following existing patterns
• Good separation of concerns with max_if_nesting_depth and if_nesting_depth methods
• Comprehensive doc comments explaining the logic
• Consistent with the codebase's Rust style

Minor observations:

• The recursive implementation is elegant and handles all statement types correctly
• Good use of iterator methods (map, max, flat_map)

Architecture: ✅ Well-designed

The lint integrates seamlessly into the existing architecture:

• Follows the structural analysis pattern (non-pattern-based lint)
• Placed appropriately in lint_word alongside pattern matching
• Uses the existing LintDiagnostic structure

Design decisions:

• Quotations reset nesting depth (lines 306-311): Correct choice since quotations are separate code blocks
• Match arms don't count as if nesting (lines 313-320): Reasonable, as match is a different control flow construct
• Both then/else branches checked with max() (line 304): Correctly identifies the deepest path

Correctness: ✅ Appears sound

The nesting calculation logic is correct:

1. Each if increments depth by 1
2. Recursively checks both then/else branches
3. Returns the maximum depth found
4. Properly handles None for missing else branches

Edge cases handled:

• Empty statement lists → unwrap_or(0) or unwrap_or(new_depth)
• Nested structures within quotations
• Match expressions with multiple arms

Performance: ✅ Good

• Single-pass traversal of the AST
• No unnecessary allocations
• Depth calculation happens once per word during linting
• Negligible overhead since it's part of the existing lint pass

Potential Issues: ⚠️ Minor concerns

1. Threshold choice (MAX_NESTING_DEPTH = 4)

The constant is well-documented, but the choice of 4 seems somewhat arbitrary. Consider:

• Lines 27-29 explain the rationale
• The PR description shows a 7-level example that definitely needs refactoring
• 4 is reasonable but could benefit from empirical validation

Suggestion: Document why 4 was chosen (e.g., "based on analysis of examples/" or "follows industry standards like Checkstyle's default")

2. Missing test coverage

The existing test suite (lines 573-852) doesn't include tests for the new deep-nesting lint. This is the most significant gap.

Recommendation: Add tests for:

• Test with exactly MAX_NESTING_DEPTH - should NOT trigger
• Test with MAX_NESTING_DEPTH + 1 - SHOULD trigger
• Test with quotations that reset depth
• Test with match expressions

3. Diagnostic location accuracy

Lines 249-256 use fallback_line and set start_index: 0, end_index: 0. This means:

• The diagnostic points to the word definition, not the specific nested if
• Less precise than pattern-based lints that use actual spans

Consider: Could track the span of the outermost if statement for more precise reporting. Not critical but would improve UX.

4. Recursion through lint_nested

Lines 259-260 call lint_nested, which will traverse the same statements again. This means:

• Nested if/else blocks are checked multiple times for patterns
• However, max_if_nesting_depth is only called once per word
• Not a bug, but worth noting for future optimization

Security: ✅ No concerns

• No unsafe code
• No user input parsing in this PR
• No security-sensitive operations

Style & Conventions: ✅ Consistent

Following the LINT_DESIGN.md philosophy:

• Phase 1 focuses on syntactic patterns ✓
• Diagnostic format matches existing lints ✓
• Severity appropriately set to Hint ✓

Suggestions for Improvement

Priority: High

1. Add unit tests for the nesting detection logic
   • Test various nesting levels
   • Test edge cases (empty branches, quotations, match)

Priority: Medium

2. Document the threshold choice in the constant comment
3. Consider tracking source location of the deepest if for better diagnostics

Priority: Low

4. Configurable threshold: Future enhancement to allow users to set MAX_NESTING_DEPTH via lint config

Overall Assessment: ✅ Approve

This is a solid PR that adds valuable functionality:

• Clean implementation
• Solves a real problem (7-level nesting in eval.seq)
• Low risk changes
• Good foundation for structural lints

Primary requirement before merge: Add test coverage for the new functionality.

---

Recommendation: Approve with request for tests. The code is production-ready, just needs test coverage to maintain quality standards.