✨ Add read_write mode for bidirectional file I/O #41
Conversation
Implements ioModeReadWrite constant to support bidirectional file access. This non-standard ISO Prolog extension allows files to be opened for both reading and writing, enabling VFS device use cases.
WalkthroughThis PR adds bidirectional read-write mode support to enable simultaneous reading and writing operations on the same stream. Changes include a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #41 +/- ##
==========================================
+ Coverage 98.27% 98.30% +0.03%
==========================================
Files 24 24
Lines 7055 7063 +8
==========================================
+ Hits 6933 6943 +10
+ Misses 90 88 -2
Partials 32 32
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
engine/stream.go (1)
472-479: Consider a more efficient mapping approach.The array literal with keyed elements creates a sparse array (potentially 1000+ elements) with most slots being zero values. While this works, it's memory-inefficient. Consider refactoring to use a switch statement or map for better performance and clarity:
🔎 Proposed refactor using switch statement
func (m ioMode) Term() Term { - return [...]Term{ - ioModeRead: atomRead, - ioModeWrite: atomWrite, - ioModeAppend: atomAppend, - ioModeReadWrite: atomReadWrite, - }[m] + switch m { + case ioModeRead: + return atomRead + case ioModeWrite: + return atomWrite + case ioModeAppend: + return atomAppend + case ioModeReadWrite: + return atomReadWrite + default: + return nil + } }Note: This is an existing pattern in the codebase, so addressing it is optional for this PR.
engine/builtin_test.go (1)
3691-3785: Read-writeOpen/4tests are solid; consider using stream APIs instead of rawsourceThe new
read_writesubtests (can read/write, properties, SetInput/SetOutput) give good end‑to‑end coverage forOpen/4withatomReadWriteand ensure the resulting stream:
- Can actually read and write.
- Exposes both
inputandoutputproperties.- Is accepted by both
SetInputandSetOutput.One minor consistency nit: in the “can read and write” subtest you call
s.initRead()but then read directly vias.source.(io.Reader).Read(b)instead of going throughs.buforReadRunelike other tests. Using theStreamread helpers (initRead+io.ReadAll(s.buf)orReadRune) would keep the test aligned with the abstraction and catch regressions inStream’s own logic, not just the underlyingio.Reader.This is non‑blocking, but worth considering to make the test more robust.
Example change for the read path (illustrative)
- assert.NoError(t, s.initRead()) - b := make([]byte, 7) - n, err := s.source.(io.Reader).Read(b) - assert.NoError(t, err) - assert.Equal(t, 7, n) - assert.Equal(t, "initial", string(b)) + assert.NoError(t, s.initRead()) + b, err := io.ReadAll(s.buf) + assert.NoError(t, err) + assert.True(t, strings.HasPrefix(string(b), "initial"))engine/stream_test.go (1)
729-808: Dedicated TestStream_ReadWrite nicely validates bidirectional behavior
TestStream_ReadWritegives a focused check that a read‑writeStreambacked by a single*os.File:
- Can initialize buffered reading (
initRead).- Exposes a valid text writer via
textWriter().- Successfully performs both
ReadRuneandWriteRune.- Advertises both
inputandoutputinproperties().This is a good complement to the builtin‑level tests and ensures the core
Streamabstraction behaves as expected in read_write mode. If you ever want to tighten it further, you could optionally assert ons.positionand/or the final file contents, but it’s already effective as is.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.mdengine/atom.goengine/builtin.goengine/builtin_test.goengine/stream.goengine/stream_test.go
🧰 Additional context used
🧬 Code graph analysis (2)
engine/stream_test.go (2)
engine/stream.go (1)
Stream(34-50)engine/atom.go (1)
Atom(202-202)
engine/stream.go (1)
engine/term.go (1)
Term(12-15)
🔇 Additional comments (18)
engine/atom.go (1)
152-152: LGTM!The new
atomReadWriteatom constant is correctly defined and follows the established pattern for well-known atoms.README.md (1)
45-45: LGTM!The documentation clearly describes the new
read_writemode feature and its intended use case for half-duplex transactional devices.engine/stream.go (5)
271-271: LGTM!The
Flush()method correctly includesioModeReadWritein the permission check, allowing bidirectional streams to be flushed.
307-307: LGTM!The
initRead()andreset()functions correctly includeioModeReadWritein their permission checks, enabling read operations on bidirectional streams.Also applies to: 328-328
382-383: LGTM!The
properties()method correctly reports bothinputandoutputproperties for read-write streams, accurately reflecting their bidirectional capabilities.
408-408: LGTM!The
textWriter()andbinaryWriter()methods correctly includeioModeReadWritein their permission checks, enabling write operations on bidirectional streams.Also applies to: 420-420
468-469: LGTM!The
ioModeReadWriteconstant is correctly defined usingos.O_RDWR, which is the standard flag for bidirectional file access.engine/builtin.go (4)
1196-1196: LGTM!The
SetInput()function correctly accepts streams inioModeReadWrite, allowing bidirectional streams to be set as the current input stream.
1211-1211: LGTM!The
SetOutput()function correctly accepts streams inioModeReadWrite, allowing bidirectional streams to be set as the current output stream.
1257-1260: LGTM!The
Open()function correctly mapsatomReadWritetoioModeReadWrite, enabling Prolog code to open files with theread_writemode.
1279-1282: Code implementation is correct. Tests inTestStream_ReadWriteand theread_writetest inbuiltin_test.goalready verify that position tracking and buffering work correctly when alternating between read and write operations on the same file. Setting bothsourceandsinkto the same file handle and callinginitRead()is the correct approach for bidirectional I/O initialization.engine/builtin_test.go (3)
3364-3389: Extend SetInput coverage to read_write streamsAllowing
SetInputto accept areadWritestream (and verifyingvm.inputis updated) correctly exercises the newioModeReadWriteas an input-capable mode. The setup usingos.Stdin/os.Stdoutis fine in tests and matches existing patterns.
3402-3427: Extend SetOutput coverage to read_write streamsSimilarly, adding a
readWritefixture and positive case inTestSetOutputvalidates thatioModeReadWriteis treated as output-capable, while existing permission-error cases remain intact.
6501-6571: Good additional edge coverage for SetStreamPositionThe new tests around
SetStreamPosition/3:
- Exercise non‑integer
positionvalues (atom, float, compound) and asserttypeError(integer, _).- Verify that a direct
*Streamwithreposition: false(not just an alias‑resolved stream) correctly raisespermission_error(reposition, stream, ...).These cover important edge cases for the read_write / repositionable stream story and look correct as written.
engine/stream_test.go (4)
278-357: Treating read_write as a valid input mode for binary readsAdding the
"read-write binary"case toTestStream_ReadByteconfirms thatReadByteacceptsioModeReadWritein combination withstreamTypeBinaryand still updatesposition/endOfStreamas expected. This aligns with the intended half‑duplex semantics.
359-477: Treating read_write as a valid input mode for text readsThe
"read-write text"case inTestStream_ReadRunesimilarly ensures thatReadRuneworks whenmode == ioModeReadWriteandstreamType == streamTypeText, keeping the existing EOF and error behaviors unchanged for other modes and types.
563-611: WriteByte supports read_write; mock expectations updated appropriatelyChanging the mock to
.Twice()and adding the"read-write"test case verifies thatWriteByte:
- Accepts
ioModeReadWritewith a binary stream.- Still rejects pure input mode and wrong stream type as before.
The position bookkeeping (
pos: 1) is checked for both successful writes, which is useful.
613-665: WriteRune supports read_write; consistent with text semanticsThe analogous changes in
TestStream_WriteRune(twice‑called mock and"read-write"case) confirm thatWriteRuneoperates correctly inioModeReadWritefor text streams while preserving existing errors for input mode and binary streams.
This PR introduces an additional
read_writemode to enable half-duplex transactional devices in the host's virtual file system (VFS), particularly for the axoned chain. This allows Prolog programs to use files as device interfaces supporting both command input and response output, enabling exchange and interaction with the host system.Note: This is a delimited deviation from the ISO Prolog standard, maintaining coherence with existing I/O modes while extending functionality for VFS device interactions.