io.write

[navicore]

#161

• 225 compiler tests - includes new test_codegen_io_write
• 290 runtime tests - includes new test_write
• 126 integration tests - includes new test-io-write-basic, test-io-write-empty, test-io-write-with-newline

Now the reviewer has comprehensive test coverage:

Test Type	File	Tests
Unit (runtime)	io.rs	test_write
Codegen	codegen.rs	test_codegen_io_write
Integration	test-io-write.seq	3 tests

[claude]

Pull Request Review: io.write primitive

Summary

This PR adds an io.write primitive that outputs strings without a trailing newline, complementing the existing io.write-line. The implementation is clean, well-tested, and directly addresses the LSP protocol compliance needs outlined in issue #161.

Code Quality: ✅ Excellent

The implementation follows the established patterns in the codebase perfectly:

• Consistent with existing code: patch_seq_write (io.rs:94-116) mirrors patch_seq_write_line exactly, just without the newline write
• Proper documentation: Clear doc comments explaining the use case, stack effect, safety requirements, and concurrency behavior
• Correct placement: Added in logical order before io.write-line in both builtins.rs:274 and codegen.rs:622

Test Coverage: ✅ Comprehensive

Excellent multi-layer testing strategy:

• ✅ Runtime test (io.rs:423-429): Unit test for the core function
• ✅ Codegen test (codegen.rs:5114-5140): Verifies LLVM IR generation
• ✅ Integration tests (test-io-write.seq): 3 tests covering basic, empty string, and embedded newline cases

This matches the testing pattern for other I/O primitives. The integration tests appropriately acknowledge the limitation of not being able to assert stdout content.

Potential Issues: ⚠️ Minor

1. Error Handling (Low Priority)

The libc::write call (io.rs:105-109) doesn't check the return value for errors or partial writes. However, this is consistent with patch_seq_write_line (io.rs:66-71), which has the same behavior.

Recommendation: This is acceptable for now since it matches the existing error handling philosophy. Consider tracking error handling improvements for all I/O operations in a future issue.

2. Missing Comment Consistency

In patch_seq_write_line, there's a detailed comment explaining why libc::write is used instead of Rust's std::io::stdout() (io.rs:60-62):

// Write directly to fd 1 using libc to avoid Rust's std::io::stdout() RefCell.
// Rust's standard I/O uses RefCell which panics on concurrent access from
// multiple coroutines on the same thread.

The new patch_seq_write function lacks this explanatory comment. While the doc comment mentions concurrency, it doesn't explain the technical rationale for using libc::write.

Suggestion: Add the same inline comment at io.rs:104 for consistency and future maintainability.

Performance: ✅ Good

• Uses the same efficient libc::write approach as write_line
• Proper mutex usage to prevent contention without blocking OS threads
• No unnecessary allocations or copies

Security: ✅ No Concerns

• Follows the same safety contract as other I/O primitives
• Relies on compiler-generated code for type safety
• String handling is safe (UTF-8 validated by Rust's String type)

Best Practices: ✅ Excellent

• Follows DRY principle by reusing existing patterns
• Clear naming convention (io.write vs io.write-line)
• Proper export in the module's public API (io.rs:404)
• Consistent with the codebase's unsafe code conventions

Minor Observations

1. Ordering: The new io.write is placed before io.write-line in the builtin signatures (builtins.rs:274-275). This seems intentional and reasonable (base case before variant).

2. Integration test comments: The tests acknowledge they can't assert stdout content (test-io-write.seq:3). This is pragmatic, though a future enhancement could capture and verify output.

Verdict: ✅ APPROVED

This is a high-quality PR that:

• Solves a real problem (LSP protocol compliance)
• Follows existing patterns meticulously
• Has comprehensive test coverage
• Makes minimal, focused changes

The only suggestion is adding the inline comment about libc::write rationale for consistency, but this is not blocking.

Files changed: 4 files, +103 lines
Risk level: Low - Well-isolated change to add new functionality

Great work! 🎉