Conversation
⏺ All CI checks pass clean: - Code formatting ✓ - Clippy lints ✓ (all function_casts_as_integer fixed) - 376 tests passed, 0 failed - Integration tests ✓ - Seq lint ✓ (117 files) The function_casts_as_integer lint was indeed new in a recent Rust stable release — it required adding an intermediate as *const () cast before casting function items to integer types. Fixed across 7 files in seq-runtime: scheduler.rs, signal.rs, terminal.rs, cond.rs, list_ops.rs, quotations.rs, and weave.rs.
Code Review: PR #352 - Stash Stacks (Aux Stack Operations)OverviewThis PR implements auxiliary stack operations ( ✅ Strengths1. Comprehensive Implementation
2. Type Safety
3. Code Generation
4. Testing
5. Documentation
🔍 Code Quality Observations1. Potential Optimization Opportunity 2. Resource Linting - Conservative Join 3. Edge Case: Nested Control Flow 4. Error Messages 🐛 Potential Issues1. CRITICAL - Aux Stack Scope in Codegen Branches Recommendation: Add assertions or comments documenting the assumption that branches are always emitted in a controlled manner with proper state restoration. 2. Quotation Aux Stack Isolation
3. Integer Underflow Risk 🔒 Security Considerations1. Stack Bounds Recommendation: Consider adding debug assertions in unsafe builds or documenting that the typechecker provides the safety guarantee. 2. Memory Safety
⚡ Performance Considerations1. Alloca Cost Assessment: Acceptable tradeoff - allocas are cheap, and the simplicity is worth it. 2. Spill on Every Aux Operation Assessment: Correct choice - safety over premature optimization. Future optimization could be selective spilling if profiling shows this as a bottleneck. 📋 Test Coverage Gaps
📝 Minor Code Style Issues1. Consistent Formatting 2. Issue References 3. Naming
🎯 RecommendationsHigh Priority:
Medium Priority: Low Priority: ✅ Final AssessmentThis is a high-quality PR that implements a non-trivial feature correctly and comprehensively. The implementation demonstrates:
The potential issues raised are mostly defensive suggestions rather than actual bugs. The critical item about quotation aux stack isolation should be verified, but the typechecker isolation likely provides the necessary guarantee. Recommendation: Approve with minor suggestions. The CI passes confirm the implementation works correctly. Address the quotation codegen question and consider adding the suggested tests in a follow-up if desired. Great work! 🎉 |
…ngs led to: Bug fixed — aux in quotations: - Quotations are compiled as separate LLVM functions, so they don't have access to the enclosing word's aux alloca slots. The typechecker was allowing >aux/aux> inside quotations, which would have generated invalid LLVM IR. - Fix: Reject aux ops in quotations at the typechecker level with a clear error message directing users to extract the body into a named word. Added in_quotation_scope flag. - Defense in depth: codegen_quotation() now saves/restores current_aux_slots and current_aux_sp so even if the typechecker check were bypassed, codegen wouldn't reference stale alloca names. Debug assertion added: - aux> codegen now has debug_assert!(self.current_aux_sp > 0) to catch underflow if the typechecker ever has a bug. New tests (3): - test_aux_in_match_balanced — aux used symmetrically across match arms ✓ - test_aux_in_match_unbalanced_error — different aux states in match arms caught ✓ - test_aux_in_quotation_rejected — clear error for aux inside quotations ✓
PR Review: Stash Stacks (Aux Stack Implementation)SummaryThis PR implements auxiliary stack operations ( ✅ StrengthsExcellent Type Safety
Well-Designed Architecture
Comprehensive Testing
Code Quality
|
🔍 Observations & Minor Concerns1. Clippy Lint Fixes (function_casts_as_integer)The PR description mentions fixing the Verification: Runtime files (scheduler.rs:1130-1133, signal.rs:1147, etc.) correctly use 2. Resource Lint Conservative JoinIn resource_lint.rs:325-330, the aux stack join operation takes the longer aux stack to avoid false negatives. This seems overly conservative - if two branches have different aux stack depths at a join point, shouldnt that be an error? The typechecker already enforces balanced aux stacks across branches, so in practice they should always be the same length at join points. Impact: Low - this is defensive programming that wont cause bugs, just might miss some edge case lints 3. Debug Assertion in Codegendispatch.rs:109-112 has a Mitigation: The typechecker is thoroughly tested, so this is unlikely to occur in practice. 4. Test Coverage GapThe integration tests (test-aux.seq) dont include test cases for the error conditions (unbalanced aux stack, empty stack, quotations, mismatched branches). These ARE tested in unit tests (typechecker.rs), so coverage exists, just not at the integration level. 5. DocumentationThe builtin docs (builtins.rs:41-47) are concise and clear. Consider whether users might benefit from additional documentation about performance characteristics, when to use aux vs. alternatives, and the word-local scope limitation. Impact: Low - the current docs are adequate for basic usage |
🔒 Security ConsiderationsNo security concerns identified:
⚡ Performance ConsiderationsPositive:
Neutral:
🧪 Test Coverage AssessmentExcellent overall coverage:
Could add (nice-to-have):
|
📋 RecommendationsRequired: NoneAll critical concerns are already addressed. The implementation is solid. Optional Improvements:
✨ Overall AssessmentStrong approval with minor suggestions This is high-quality work that demonstrates:
The aux stack feature is well-designed, properly scoped (word-local), and integrates cleanly with existing systems (typechecker, codegen, resource lint). The "defense in depth" approach (typechecker + codegen assertions + saved/restored state) shows good engineering judgment. Recommendation: Approve and merge after considering the optional improvements above. Great work on this feature! The test coverage and error handling are particularly impressive. 🎉 |
1 participant
⏺ All CI checks pass clean:
#350
The function_casts_as_integer lint was indeed new in a recent Rust stable release — it required adding an intermediate as *const () cast before casting
function items to integer types. Fixed across 7 files in seq-runtime: scheduler.rs, signal.rs, terminal.rs, cond.rs, list_ops.rs, quotations.rs, and
weave.rs.