navicore/patch-seq

Fork 0

⏺ All 867 tests pass (294 in compiler, up from 293 with the new regre… #339

Merged

navicore merged 2 commits from i-338 into main

2026-01-26 21:48:41 +00:00

navicore commented

2026-01-26 21:34:54 +00:00

(Migrated from github.com)

…ssion test).
https://github.com/navicore/patch-seq/issues/338

Summary: Bug #338 Fix

Root Cause: In codegen_word_call (statements.rs), when a user-defined word with a specialized version (like edit-done returning i64) was called in tail
position within an if-then branch:

will_emit_tail_call returned true (expecting musttail + ret to be emitted)
But try_specialized_dispatch was checked first and took the specialized path
The specialized path only emits call i64 @func() without a terminator
codegen_branch saw emitted_tail_call = true and skipped emitting br label %merge
Result: LLVM basic block without a terminator → invalid IR

The Fix: Added a check in codegen_word_call (lines 22-35 in statements.rs) to determine if we would be in a tail call position for a user-defined word
before trying specialized dispatch. If so, skip specialized dispatch and use the normal tail call path which properly emits musttail + ret.

// Check if this would be a tail call position for a user-defined word.
// If so, skip specialized dispatch - we need the tail call path to emit
// musttail + ret for proper TCO. (Issue #338)
let is_seq_word = !BUILTIN_SYMBOLS.contains_key(name) ...;
let would_tail_call = position == TailPosition::Tail && is_seq_word && ...;

if !would_tail_call {
if let Some(result) = self.try_specialized_dispatch(...) { ... }
}

Files Changed:

crates/compiler/src/codegen/statements.rs - Added tail call check before specialized dispatch
crates/compiler/src/codegen/mod.rs - Added regression test

…ssion test). https://github.com/navicore/patch-seq/issues/338 Summary: Bug #338 Fix Root Cause: In codegen_word_call (statements.rs), when a user-defined word with a specialized version (like edit-done returning i64) was called in tail position within an if-then branch: 1. will_emit_tail_call returned true (expecting musttail + ret to be emitted) 2. But try_specialized_dispatch was checked first and took the specialized path 3. The specialized path only emits call i64 @func() without a terminator 4. codegen_branch saw emitted_tail_call = true and skipped emitting br label %merge 5. Result: LLVM basic block without a terminator → invalid IR The Fix: Added a check in codegen_word_call (lines 22-35 in statements.rs) to determine if we would be in a tail call position for a user-defined word before trying specialized dispatch. If so, skip specialized dispatch and use the normal tail call path which properly emits musttail + ret. // Check if this would be a tail call position for a user-defined word. // If so, skip specialized dispatch - we need the tail call path to emit // musttail + ret for proper TCO. (Issue #338) let is_seq_word = !BUILTIN_SYMBOLS.contains_key(name) ...; let would_tail_call = position == TailPosition::Tail && is_seq_word && ...; if !would_tail_call { if let Some(result) = self.try_specialized_dispatch(...) { ... } } Files Changed: - crates/compiler/src/codegen/statements.rs - Added tail call check before specialized dispatch - crates/compiler/src/codegen/mod.rs - Added regression test

claude[bot] commented

2026-01-26 21:36:40 +00:00

(Migrated from github.com)

Code Review for PR #339

Summary

This PR fixes issue #338 where invalid LLVM IR was generated when a specialized function was called in tail position within an if-then branch. The fix correctly prioritizes tail call optimization over specialized dispatch.

✅ Code Quality & Best Practices

Strong Points:

Root cause analysis is excellent: The PR description clearly explains the problem (specialized dispatch was checked before determining tail call position, leaving basic blocks without terminators)
Minimal, surgical fix: The change is focused and doesn't refactor unnecessarily - exactly what's needed
Good naming: would_tail_call clearly expresses intent and mirrors the existing can_tail_call pattern used later in the function
Consistent with codebase patterns: The conditions in would_tail_call mirror those in can_tail_call (lines 81-85), maintaining consistency

Code Structure:

The fix is in the right place - codegen_word_call in statements.rs:28-47
Logic flow is clear: inline ops → tail call check → specialized dispatch → regular dispatch
Follows the project's style guidelines from CONTRIBUTING.md (focused PR, one fix)

✅ Bug Fix Validation

The Fix is Correct:
The logic correctly identifies when specialized dispatch should be skipped:

let would_tail_call = position == TailPosition::Tail
    && !self.inside_closure
    && !self.inside_main
    && !self.inside_quotation
    && is_seq_word;

This matches the exact conditions needed for tail calls (statements.rs:81-85), ensuring that when codegen_branch expects a terminator (musttail + ret), it will actually be emitted.

Why This Works:

will_emit_tail_call (words.rs:431-468) returns true for user-defined words in tail position
codegen_branch (words.rs:389) sees this and expects the branch to emit its own terminator
Previously, try_specialized_dispatch would intercept and emit only call i64 @func() without a terminator
Now, specialized dispatch is skipped in tail position, forcing the normal path which emits musttail call + ret

✅ Test Coverage

Excellent Regression Test:
The test at mod.rs:1158-1269 is comprehensive:

✅ Reproduces the exact bug scenario (specialized call in if-then branch)
✅ Tests the fix doesn't panic with "basic block lacks terminator"
✅ Verifies specialized version is still generated (@seq_get_value_i64)
✅ Confirms tail call path is used (musttail call tailcc ptr @seq_get_value)
✅ Clear documentation explaining the bug and fix

Test Quality:

Well-structured with clear comments
Tests both that the bug is fixed AND that specialization still works (just not in tail position)
The assertion on line 1266 is the key validation: ensuring musttail is used instead of specialized dispatch

✅ Performance Considerations

No Performance Regression:

Specialized dispatch is still used in non-tail positions (the common case)
Tail call optimization is preserved (actually fixed - it was broken before)
The additional conditions (would_tail_call check) are cheap boolean checks
No new allocations or complex operations introduced

Trade-off is Correct:

Skipping specialized dispatch in tail position is the right trade-off
TCO (tail call optimization) prevents stack overflow in recursive code
Slightly less efficient call in tail position, but correctness > micro-optimization

✅ Security Considerations

No Security Issues:

Fix is in the code generator, not runtime
Doesn't introduce new memory access patterns
Doesn't change FFI boundaries or external interfaces
The fix makes the code MORE correct (generates valid IR)

🔍 Minor Observations

1. Code Duplication (Not Blocking):
The conditions for would_tail_call (lines 36-40) and can_tail_call (lines 81-85) are identical. Consider extracting to a helper method to reduce duplication and ensure they stay in sync:

fn can_emit_tail_call_for_seq_word(&self, position: TailPosition) -> bool {
    position == TailPosition::Tail
        && !self.inside_closure
        && !self.inside_main
        && !self.inside_quotation
}

Then use:

let would_tail_call = self.can_emit_tail_call_for_seq_word(position) && is_seq_word;

However, this is a minor style point - the current duplication is clear and the conditions are simple enough.

2. Edge Case Coverage:
The test covers the if-then scenario. Consider if similar issues could occur with:

Match expressions with tail calls in arms (though match uses the same codegen_branch path, so likely covered)
Nested if statements with specialized calls

The existing test suite (867 tests passing) likely covers these, but worth noting.

✅ Final Assessment

Verdict: LGTM ✅

This is a high-quality bug fix:

✅ Solves the reported issue correctly
✅ Minimal, focused changes (20 additions, 3 deletions in source)
✅ Comprehensive regression test (113 additions)
✅ No performance or security concerns
✅ Follows project conventions
✅ All 867 tests pass (including 294 compiler tests)

Recommendation: Approve and merge. The minor duplication point is not worth blocking for - it can be addressed in a future refactoring if desired.

Great work on the thorough root cause analysis and surgical fix! The detailed commit message and test documentation will help future maintainers understand this subtle interaction between specialized dispatch and tail call optimization.

## Code Review for PR #339 ### Summary This PR fixes issue #338 where invalid LLVM IR was generated when a specialized function was called in tail position within an if-then branch. The fix correctly prioritizes tail call optimization over specialized dispatch. --- ### ✅ Code Quality & Best Practices **Strong Points:** - **Root cause analysis is excellent**: The PR description clearly explains the problem (specialized dispatch was checked before determining tail call position, leaving basic blocks without terminators) - **Minimal, surgical fix**: The change is focused and doesn't refactor unnecessarily - exactly what's needed - **Good naming**: `would_tail_call` clearly expresses intent and mirrors the existing `can_tail_call` pattern used later in the function - **Consistent with codebase patterns**: The conditions in `would_tail_call` mirror those in `can_tail_call` (lines 81-85), maintaining consistency **Code Structure:** - The fix is in the right place - `codegen_word_call` in statements.rs:28-47 - Logic flow is clear: inline ops → tail call check → specialized dispatch → regular dispatch - Follows the project's style guidelines from CONTRIBUTING.md (focused PR, one fix) --- ### ✅ Bug Fix Validation **The Fix is Correct:** The logic correctly identifies when specialized dispatch should be skipped: ```rust let would_tail_call = position == TailPosition::Tail && !self.inside_closure && !self.inside_main && !self.inside_quotation && is_seq_word; ``` This matches the exact conditions needed for tail calls (statements.rs:81-85), ensuring that when `codegen_branch` expects a terminator (musttail + ret), it will actually be emitted. **Why This Works:** 1. `will_emit_tail_call` (words.rs:431-468) returns `true` for user-defined words in tail position 2. `codegen_branch` (words.rs:389) sees this and expects the branch to emit its own terminator 3. Previously, `try_specialized_dispatch` would intercept and emit only `call i64 @func()` without a terminator 4. Now, specialized dispatch is skipped in tail position, forcing the normal path which emits `musttail call + ret` --- ### ✅ Test Coverage **Excellent Regression Test:** The test at mod.rs:1158-1269 is comprehensive: - ✅ Reproduces the exact bug scenario (specialized call in if-then branch) - ✅ Tests the fix doesn't panic with "basic block lacks terminator" - ✅ Verifies specialized version is still generated (`@seq_get_value_i64`) - ✅ Confirms tail call path is used (`musttail call tailcc ptr @seq_get_value`) - ✅ Clear documentation explaining the bug and fix **Test Quality:** - Well-structured with clear comments - Tests both that the bug is fixed AND that specialization still works (just not in tail position) - The assertion on line 1266 is the key validation: ensuring musttail is used instead of specialized dispatch --- ### ✅ Performance Considerations **No Performance Regression:** - Specialized dispatch is still used in non-tail positions (the common case) - Tail call optimization is preserved (actually fixed - it was broken before) - The additional conditions (`would_tail_call` check) are cheap boolean checks - No new allocations or complex operations introduced **Trade-off is Correct:** - Skipping specialized dispatch in tail position is the right trade-off - TCO (tail call optimization) prevents stack overflow in recursive code - Slightly less efficient call in tail position, but correctness > micro-optimization --- ### ✅ Security Considerations **No Security Issues:** - Fix is in the code generator, not runtime - Doesn't introduce new memory access patterns - Doesn't change FFI boundaries or external interfaces - The fix makes the code MORE correct (generates valid IR) --- ### 🔍 Minor Observations **1. Code Duplication (Not Blocking):** The conditions for `would_tail_call` (lines 36-40) and `can_tail_call` (lines 81-85) are identical. Consider extracting to a helper method to reduce duplication and ensure they stay in sync: ```rust fn can_emit_tail_call_for_seq_word(&self, position: TailPosition) -> bool { position == TailPosition::Tail && !self.inside_closure && !self.inside_main && !self.inside_quotation } ``` Then use: ```rust let would_tail_call = self.can_emit_tail_call_for_seq_word(position) && is_seq_word; ``` However, this is a minor style point - the current duplication is clear and the conditions are simple enough. **2. Edge Case Coverage:** The test covers the if-then scenario. Consider if similar issues could occur with: - Match expressions with tail calls in arms (though match uses the same `codegen_branch` path, so likely covered) - Nested if statements with specialized calls The existing test suite (867 tests passing) likely covers these, but worth noting. --- ### ✅ Final Assessment **Verdict: LGTM** ✅ This is a high-quality bug fix: - ✅ Solves the reported issue correctly - ✅ Minimal, focused changes (20 additions, 3 deletions in source) - ✅ Comprehensive regression test (113 additions) - ✅ No performance or security concerns - ✅ Follows project conventions - ✅ All 867 tests pass (including 294 compiler tests) **Recommendation:** Approve and merge. The minor duplication point is not worth blocking for - it can be addressed in a future refactoring if desired. --- Great work on the thorough root cause analysis and surgical fix! The detailed commit message and test documentation will help future maintainers understand this subtle interaction between specialized dispatch and tail call optimization.

claude[bot] commented

2026-01-26 21:43:27 +00:00

(Migrated from github.com)

Code Review for PR #339: Fix for Issue #338

Summary

This PR fixes a critical compiler bug where specialized function calls in tail position within if-then branches resulted in invalid LLVM IR due to missing terminator instructions. The fix is well-targeted and includes a comprehensive regression test.

✅ Strengths

Root Cause Analysis: The PR description provides excellent documentation of the problem:
- Clear explanation of the interaction between will_emit_tail_call and try_specialized_dispatch
- Detailed sequence showing how the bug manifests
- Well-documented fix rationale
Minimal, Surgical Fix: The change in statements.rs:28-44 is precisely scoped:
- Only affects the specific edge case (tail position + user-defined word)
- Preserves specialized dispatch for non-tail positions
- Doesn't impact performance for the common case
Comprehensive Test Coverage: The test in mod.rs:1157-1267 is exemplary:
- Reproduces the exact conditions from issue #338
- Tests the specific pattern: specialized word (get-value) in tail position in an if-then branch
- Verifies both that IR is valid AND that the correct code path (musttail) is taken
- Clear comments explaining what's being tested and why
Code Quality:
- Comments explain the "why" not just the "what" (lines 28-32 in statements.rs)
- Variable names are descriptive (would_tail_call, is_seq_word)
- Logic follows the same pattern as the existing tail call detection (lines 79-83)

🔍 Observations & Questions

Logic Duplication: The would_tail_call check (lines 36-40) duplicates the logic from can_tail_call (lines 79-83). This is:
- Correct for the fix (must check before specialized dispatch)
- Potential maintenance risk if tail call conditions change in the future
Suggestion: Consider extracting to a helper method like fn can_emit_tail_call(&self, is_seq_word: bool) -> bool to keep the logic DRY. However, this is a minor refactoring opportunity, not a blocker.
Test Assertion Precision: The test verifies the presence of musttail call tailcc ptr @seq_get_value (line 1265), which is excellent. However:
- Question: Should the test also assert the absence of a call to the specialized version (@seq_get_value_i64) in the then-branch to more explicitly verify the fix?
- Current test checks specialized version exists (line 1259), but not where it's used
Edge Case Coverage: The fix handles user-defined words in tail position. Are there similar issues with:
- Specialized dispatch in match expressions?
- Other control flow constructs?
Note: The current code already calls spill_virtual_stack before control flow (control_flow.rs:22, 150), so this may already be handled. Just worth confirming.

🔒 Security & Performance

No security concerns: Pure compiler logic, no external input handling
No performance regression: Specialized dispatch still works for non-tail positions, which is the vast majority of cases
Correctness improvement: Fixes generation of invalid IR, preventing potential undefined behavior

📋 Minor Nits

Line 33: The check !self.external_builtins.contains_key(name) appears in both is_seq_word definitions (line 33-35 and 66-74). Consider extracting this to a helper method for clarity.
Test naming: test_issue_338_specialized_call_in_if_branch_has_terminator is descriptive but long. The name accurately describes the test, though.

✅ Verdict

LGTM with minor suggestions for future refactoring.

This is a high-quality fix:

Correctly addresses the root cause
Minimal code changes
Excellent test coverage
Well-documented

The duplicate logic for tail call detection is a minor code smell but doesn't affect correctness. The fix appropriately prioritizes correctness (musttail + ret) over optimization (specialized dispatch) in the specific case where they conflict.

Recommended: Approve and merge. Consider the refactoring suggestions in a follow-up PR if desired.

📝 Test Results Referenced

From PR description: "All 867 tests pass (294 in compiler, up from 293 with the new regression test)"

Excellent - the new test increases coverage without breaking existing functionality.

## Code Review for PR #339: Fix for Issue #338 ### Summary This PR fixes a critical compiler bug where specialized function calls in tail position within if-then branches resulted in invalid LLVM IR due to missing terminator instructions. The fix is well-targeted and includes a comprehensive regression test. --- ### ✅ Strengths 1. **Root Cause Analysis**: The PR description provides excellent documentation of the problem: - Clear explanation of the interaction between `will_emit_tail_call` and `try_specialized_dispatch` - Detailed sequence showing how the bug manifests - Well-documented fix rationale 2. **Minimal, Surgical Fix**: The change in `statements.rs:28-44` is precisely scoped: - Only affects the specific edge case (tail position + user-defined word) - Preserves specialized dispatch for non-tail positions - Doesn't impact performance for the common case 3. **Comprehensive Test Coverage**: The test in `mod.rs:1157-1267` is exemplary: - Reproduces the exact conditions from issue #338 - Tests the specific pattern: specialized word (`get-value`) in tail position in an if-then branch - Verifies both that IR is valid AND that the correct code path (musttail) is taken - Clear comments explaining what's being tested and why 4. **Code Quality**: - Comments explain the "why" not just the "what" (lines 28-32 in statements.rs) - Variable names are descriptive (`would_tail_call`, `is_seq_word`) - Logic follows the same pattern as the existing tail call detection (lines 79-83) --- ### 🔍 Observations & Questions 1. **Logic Duplication**: The `would_tail_call` check (lines 36-40) duplicates the logic from `can_tail_call` (lines 79-83). This is: - **Correct** for the fix (must check before specialized dispatch) - **Potential maintenance risk** if tail call conditions change in the future **Suggestion**: Consider extracting to a helper method like `fn can_emit_tail_call(&self, is_seq_word: bool) -> bool` to keep the logic DRY. However, this is a minor refactoring opportunity, not a blocker. 2. **Test Assertion Precision**: The test verifies the presence of `musttail call tailcc ptr @seq_get_value` (line 1265), which is excellent. However: - **Question**: Should the test also assert the *absence* of a call to the specialized version (`@seq_get_value_i64`) in the then-branch to more explicitly verify the fix? - Current test checks specialized version exists (line 1259), but not where it's used 3. **Edge Case Coverage**: The fix handles user-defined words in tail position. Are there similar issues with: - Specialized dispatch in match expressions? - Other control flow constructs? **Note**: The current code already calls `spill_virtual_stack` before control flow (control_flow.rs:22, 150), so this may already be handled. Just worth confirming. --- ### 🔒 Security & Performance - **No security concerns**: Pure compiler logic, no external input handling - **No performance regression**: Specialized dispatch still works for non-tail positions, which is the vast majority of cases - **Correctness improvement**: Fixes generation of invalid IR, preventing potential undefined behavior --- ### 📋 Minor Nits 1. **Line 33**: The check `!self.external_builtins.contains_key(name)` appears in both `is_seq_word` definitions (line 33-35 and 66-74). Consider extracting this to a helper method for clarity. 2. **Test naming**: `test_issue_338_specialized_call_in_if_branch_has_terminator` is descriptive but long. The name accurately describes the test, though. --- ### ✅ Verdict **LGTM with minor suggestions for future refactoring.** This is a high-quality fix: - Correctly addresses the root cause - Minimal code changes - Excellent test coverage - Well-documented The duplicate logic for tail call detection is a minor code smell but doesn't affect correctness. The fix appropriately prioritizes correctness (musttail + ret) over optimization (specialized dispatch) in the specific case where they conflict. **Recommended**: Approve and merge. Consider the refactoring suggestions in a follow-up PR if desired. --- ### 📝 Test Results Referenced From PR description: "All 867 tests pass (294 in compiler, up from 293 with the new regression test)" Excellent - the new test increases coverage without breaking existing functionality.