fix(evaluator): Evaluate same-sheet uncached formula refs (GH-208)

[arcaputo3]

Summary

• Mirror the SheetRef recursive eval pattern in the TExpr.Ref case of Evaluator.scala so that same-sheet references to CellValue.Formula(str, None) (uncached) cells are recursively parsed and evaluated instead of failing with TypeMismatch("Numeric", Formula(...))
• Fixes multi-hop formula chains like A1=100, B1=A1*2, C1=B1+50 where =C1 previously errored
• Add 5 tests covering: 2-hop chain, 3-hop chain, uncached formula in arithmetic, cross-sheet→same-sheet chain, and SUM over uncached formulas

Test plan

• All 174 evaluator tests pass (./mill xl-evaluator.test)
• Formatting passes (./mill __.checkFormat)
• CI passes

Closes #208

🤖 Generated with Claude Code

[claude]

Code Review: PR #214 - Fix Same-Sheet Uncached Formula Evaluation

Summary

This PR successfully mirrors the cross-sheet uncached formula evaluation pattern (from TExpr.SheetRef) to same-sheet references (TExpr.Ref), fixing issue #208 where multi-hop formula chains like A1=100, B1=A1*2, C1=B1+50 would fail with TypeMismatch errors.

✅ Strengths

1. Code Quality & Consistency

• ✅ Perfect pattern mirroring: The implementation in TExpr.Ref (lines 303-312) exactly mirrors the TExpr.SheetRef pattern (lines 260-272), ensuring consistency
• ✅ Type safety preserved: Uses the same decode(resultCell) approach to maintain type-safe codec handling
• ✅ Proper error propagation: Correctly maps codec errors to EvalError.CodecFailed

2. Depth Tracking & Cycle Protection

• ✅ Recursion protection: Correctly passes currentDepth to evalCrossSheetFormula, leveraging existing MaxCrossSheetRecursionDepth protection (line 165)
• ✅ Works with existing cycle detection: The change integrates seamlessly with the existing circular reference detection from fix(evaluator): Cross-sheet reference to formula cell returns 0 instead of evaluating #161

3. Test Coverage (5 new tests)

• ✅ 2-hop chain (lines 1570-1583): Basic multi-hop scenario
• ✅ 3-hop chain (lines 1585-1599): Validates deeper recursion
• ✅ Arithmetic integration (lines 1601-1613): Tests formula refs in expressions
• ✅ Cross-sheet → same-sheet (lines 1615-1632): Tests hybrid scenarios
• ✅ Range operations (lines 1634-1647): Tests SUM over uncached formulas

All test cases follow the repository's pure functional style and use proper assertions.

🔍 Observations & Suggestions

1. Circular Reference Test Coverage
While the code inherits cycle protection from evalCrossSheetFormula, there's no explicit test for same-sheet circular references (e.g., A1=B1, B1=A1). Consider adding:

test("GH-208: detect same-sheet circular uncached reference") {
  val sheet = sheetWith(
    "Main",
    ref"A1" -> CellValue.Formula("B1", None),
    ref"B1" -> CellValue.Formula("A1", None)
  )
  val wb = workbookWith(sheet)
  val result = sheet.evaluateFormula("=A1", workbook = Some(wb))
  assert(result.isLeft, "Expected cycle detection error")
}

2. Performance Consideration (Minor)
The recursive evaluation happens for every uncached same-sheet formula reference during evaluation. For deeply nested chains or ranges with many uncached formulas, this could lead to redundant parsing/evaluation. This is likely acceptable given:

• The existing design already does this for cross-sheet refs
• Most real-world Excel files have cached values
• The depth limit prevents excessive recursion

Future optimization could consider memoization at the evaluation context level.

3. Documentation
The comment on line 302 is clear, but consider adding a reference to the mirrored pattern:

// GH-208: Handle same-sheet formula cells without cached values by recursively evaluating
// (mirrors TExpr.SheetRef pattern from GH-161, lines 260-272)

🎯 Verdict

LGTM with minor suggestions. This is a solid fix that:

• ✅ Correctly solves the reported issue
• ✅ Maintains code consistency and purity
• ✅ Follows repository conventions
• ✅ Includes comprehensive test coverage
• ✅ Preserves existing safety guarantees (cycle protection)

The circular reference test suggestion is a nice-to-have for completeness, but not blocking since the protection mechanism is already proven for cross-sheet scenarios.

Minor: Test Count Discrepancy

The PR description mentions "174 evaluator tests" but the repository STATUS.md shows different counts. This is likely just outdated documentation and doesn't affect code quality.

---

Recommendation: Approve and merge after considering the circular reference test addition. Excellent work mirroring the established pattern! 🚀