Conversation
Introduces the fundamental capabilities for reading and writing raw bytes, a prerequisite for handling binary file uploads and downloads for projects like WFLHub. This change adds new syntax for opening files in binary mode, reading entire files or a specific number of bytes, writing binary data, and retrieving file sizes. A new `Binary` value type is integrated throughout the compiler pipeline, including the parser, type checker, and interpreter. - Adds `open ... for reading/writing binary` syntax - Implements `read binary from`, `read N bytes from`, and `file size of` expressions - Adds `write binary into ` statement - Includes integration tests for binary data round-tripping Additionally, adds the Product Requirements Document (PRD) outlining this and other planned language enhancements.
|
Claude finished @logbie's task —— View job Code Review: Binary File I/O ImplementationTodo List
Executive SummaryThis PR successfully implements binary file I/O capabilities for WFL, adding fundamental support needed for projects like WFLHub. The implementation is well-architected, follows WFL conventions, and integrates cleanly with the existing compiler pipeline. Detailed Review✅ Code Quality & ArchitectureStrengths:
Good patterns observed:
|
📝 WalkthroughWalkthroughAdds first-class binary I/O: new AST and parser forms (WriteBinaryStatement, ReadBinaryContent/ReadBinaryN, FileSizeOf), Binary type/value, lexer keywords, interpreter IoClient binary methods and runtime paths, typechecking and transpiler support, tests, and a migration from Rc→Arc for string literals. Changes
Sequence Diagram(s)sequenceDiagram
participant Script as WFL Script
participant Parser as Parser
participant Interpreter as Interpreter
participant IoClient as IoClient
participant FS as File System
rect rgba(100,150,200,0.5)
Note over Script,FS: Binary Write Flow
Script->>Parser: "write binary CONTENT to TARGET"
Parser-->>Interpreter: WriteBinaryStatement
Interpreter->>Interpreter: eval CONTENT
Interpreter->>Interpreter: eval TARGET -> handle_id
Interpreter->>IoClient: write_binary(handle_id, bytes)
IoClient->>FS: write bytes (sync/flush)
FS-->>IoClient: OK
IoClient-->>Interpreter: Result
Interpreter-->>Script: completion / error
end
rect rgba(150,100,200,0.5)
Note over Script,FS: Binary Read & Size Flow
Script->>Parser: "read binary from HANDLE" / "read N bytes from HANDLE" / "file size of HANDLE"
Parser-->>Interpreter: ReadBinaryContent/ReadBinaryN/FileSizeOf
Interpreter->>Interpreter: eval HANDLE -> handle_id
Interpreter->>IoClient: read_binary / read_binary_n / file_size(handle_id)
IoClient->>FS: read/stat
FS-->>IoClient: bytes / size
IoClient-->>Interpreter: Result
Interpreter-->>Script: Value::Binary / Number / error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Pull request overview
This PR adds binary file I/O capabilities to WFL, enabling reading and writing of raw binary data without UTF-8 encoding. This is a prerequisite for handling binary file uploads and downloads in projects like WFLHub. Additionally, a comprehensive Product Requirements Document (PRD) is included that outlines this and 9 other planned language enhancements.
Changes:
- Introduces
open file at ... for reading/writing binarysyntax for binary file operations - Implements
read binary from,read N bytes from, andfile size ofexpressions - Adds
write binary ... into ...statement for writing binary data - Integrates new
Binaryvalue type throughout the compiler pipeline (parser, lexer, typechecker, interpreter, transpiler) - Includes comprehensive integration tests for binary I/O round-tripping
- Adds WFLHub Language Gaps PRD documenting 10 planned language features
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| wflhub_language_gaps_prd.md | New PRD document outlining 10 language features needed for WFLHub, with binary I/O as the first P0 blocker feature |
| tests/binary_io_test.rs | Integration tests validating binary read/write operations, roundtrip accuracy, partial reads, and backward compatibility with text I/O |
| src/parser/ast.rs | Adds WriteBinaryStatement, ReadBinaryContent, ReadBinaryN, FileSizeOf expression variants, Binary type, and ReadBinary/WriteBinary file open modes |
| src/lexer/token.rs | Adds KeywordBinary and KeywordBytes tokens for new syntax |
| src/parser/stmt/io.rs | Extends file open parsing to detect binary keyword and adds parsing for write binary statements |
| src/parser/expr/primary.rs | Adds parsing for read binary from, read N bytes from, and file size of expressions |
| src/interpreter/value.rs | Adds Binary(Vec<u8>) value variant with Display, Debug, and equality implementations |
| src/interpreter/mod.rs | Implements binary I/O operations: read_binary(), read_binary_n(), write_binary(), file_size() in IoClient |
| src/typechecker/mod.rs | Adds type checking for binary operations and Binary type support |
| src/transpiler/javascript.rs | Adds JavaScript transpilation support for binary file operations |
| src/fixer/mod.rs | Adds Binary type formatting support |
| src/analyzer/mod.rs | Adds analysis support for binary I/O statements and expressions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| write binary contents into output_handle | ||
|
|
||
| // Get file size in bytes | ||
| store size as size of archive_handle |
There was a problem hiding this comment.
The syntax size of archive_handle is inconsistent with the pattern shown on line 50 which uses file size of archive_handle. The expression should be file size of archive_handle to match the implemented syntax shown elsewhere in the PR (e.g., src/parser/expr/primary.rs line 408).
| store size as size of archive_handle | |
| store size as file size of archive_handle |
| if let Some(next_token) = self.cursor.peek() { | ||
| // "read content from <handle>" | ||
| if next_token.token == Token::KeywordContent { |
There was a problem hiding this comment.
The nested if structure here creates unnecessary indentation. The original pattern used if let Some(next_token) = self.cursor.peek() && next_token.token == Token::KeywordContent which was more concise. Consider using the combined condition pattern for all branches or restructuring as an early return pattern to reduce nesting.
| if matches!( | ||
| next_token.token, | ||
| Token::IntLiteral(_) | Token::Identifier(_) | ||
| ) { | ||
| // Speculatively parse count expression, then check for "bytes" | ||
| let saved_pos = self.cursor.checkpoint(); |
There was a problem hiding this comment.
The speculative parsing logic with checkpoint/rewind is complex and would benefit from a more detailed comment explaining why both IntLiteral and Identifier need to be checked, and what the fallback behavior is when the pattern doesn't match (to distinguish from a variable named "read").
| let mut path = PathBuf::from(env!("CARGO_MANIFEST_DIR")); | ||
| path.push("target"); | ||
| path.push("release"); | ||
| if cfg!(windows) { | ||
| path.push("wfl.exe"); | ||
| } else { | ||
| path.push("wfl"); | ||
| } | ||
| path |
There was a problem hiding this comment.
This function always assumes the release binary exists at target/release/wfl, but during testing, the binary is typically built in debug mode at target/debug/wfl. This will cause test failures in standard development workflows where cargo test is run without a prior release build. Consider checking for the debug binary first, or using the CARGO_BIN_EXE_wfl environment variable which cargo sets during test runs.
| let mut path = PathBuf::from(env!("CARGO_MANIFEST_DIR")); | |
| path.push("target"); | |
| path.push("release"); | |
| if cfg!(windows) { | |
| path.push("wfl.exe"); | |
| } else { | |
| path.push("wfl"); | |
| } | |
| path | |
| // Prefer the path that Cargo sets for the binary during tests. | |
| if let Ok(bin_path) = std::env::var("CARGO_BIN_EXE_wfl") { | |
| return PathBuf::from(bin_path); | |
| } | |
| let mut base = PathBuf::from(env!("CARGO_MANIFEST_DIR")); | |
| base.push("target"); | |
| // First try the debug build, which is what `cargo test` normally builds. | |
| let mut debug_path = base.clone(); | |
| debug_path.push("debug"); | |
| if cfg!(windows) { | |
| debug_path.push("wfl.exe"); | |
| } else { | |
| debug_path.push("wfl"); | |
| } | |
| if debug_path.exists() { | |
| return debug_path; | |
| } | |
| // Fall back to the release build to preserve existing behavior. | |
| let mut release_path = base; | |
| release_path.push("release"); | |
| if cfg!(windows) { | |
| release_path.push("wfl.exe"); | |
| } else { | |
| release_path.push("wfl"); | |
| } | |
| release_path |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7eb6d9e50a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| Expression::ReadBinaryContent { file_handle, .. } => { | ||
| let fh = self.transpile_expression(file_handle)?; | ||
| Ok(format!("WFL.file.readBinary({}.path)", fh)) |
There was a problem hiding this comment.
The transpiler now emits WFL.file.readBinary(...), WFL.file.readBinaryN(...), and WFL.file.writeBinary(...), but src/transpiler/runtime.rs only defines read, write, append, etc. under WFL.file and has no binary variants, so any transpiled program that uses the new binary syntax will throw a runtime TypeError when those methods are called.
Useful? React with 👍 / 👎.
| if !file_handles.contains_key(handle_id) { | ||
| return Err(format!("Invalid file handle: {handle_id}")); | ||
| } |
There was a problem hiding this comment.
write binary ... into <target> accepts text targets in parsing/type-checking, but this implementation rejects any target that is not an already-open handle in file_handles; passing a path string (for example "out.bin") fails with Invalid file handle instead of writing the file, unlike the existing text write path fallback behavior.
Useful? React with 👍 / 👎.
| let mut bytes = Vec::with_capacity(items.len()); | ||
| for item in items.iter() { | ||
| match item { | ||
| Value::Number(n) => bytes.push(*n as u8), |
There was a problem hiding this comment.
Converting list elements with *n as u8 silently truncates/clamps non-integer or out-of-range numbers (for example 1.9 or 300.0), which can corrupt binary output without surfacing an error; binary write should validate that each number is an integer in [0,255] and fail otherwise.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@src/interpreter/mod.rs`:
- Around line 742-755: The file_size implementation only looks up the id in
file_handles and errors on miss; change IoClient::file_size so when
file_handles.get(handle_id) is None it treats handle_id as a raw filesystem
path: attempt tokio::fs::metadata(handle_id).await and return meta.len() on
success or an Err with the metadata error on failure; keep the existing behavior
when the id is found (use the stored path), and reuse symbols file_size,
file_handles, and tokio::fs::metadata to locate where to add the fallback.
- Around line 6802-6838: The ReadBinaryN branch currently casts Value::Number to
usize with `as usize` which can silently mis-handle negatives, NaN, fractions or
overflow; update the match for `count_value` inside the Expression::ReadBinaryN
arm to validate the number is finite, non-negative, an integer (fract() == 0.0),
and ≤ usize::MAX (use usize::MAX as f64) before converting, and on any failure
return a RuntimeError with a clear message; keep the error construction pattern
used elsewhere (RuntimeError::new(..., *line, *column)) and then call
`self.io_client.read_binary_n(&handle_str, n_usize).await` with the validated
`n_usize`.
- Around line 2953-3008: In the Statement::WriteBinaryStatement arm (inside
evaluate_expression results handling) validate each Value::Number in the
Value::List branch before converting to u8: ensure the number is finite, has no
fractional component (is an integer), and is within 0.0..=255.0; if any item
fails, return a RuntimeError (use the existing error creation pattern with line
and column). This replaces the current blind cast (*n as u8) with an explicit
check (n.is_finite(), n.fract() == 0.0, 0.0 <= *n && *n <= 255.0) and only then
push the value as u8 when calling io_client.write_binary; keep error messages
consistent with other RuntimeError uses and reference
content_value.type_name()/item for context.
In `@src/transpiler/javascript.rs`:
- Around line 601-612: The generated call in Statement::WriteBinaryStatement
currently emits WFL.file.writeBinary(target, content) which passes a handle
object instead of its path; update the transpilation in transpile_expression
usage within the WriteBinaryStatement branch so it emits
WFL.file.writeBinary(<target>.path, <content>) when the target expression is a
handle (i.e., follow the same handle -> .path emission used by other binary ops
and by open-created handles), ensuring the target_expr is transformed to its
.path form before formatting the WFL.file.writeBinary(...) call.
In `@src/typechecker/mod.rs`:
- Around line 1079-1114: The WriteBinaryStatement type check in
Statement::WriteBinaryStatement is too strict: infer_expression_type(content)
currently rejects list literals that infer as List(Any) or List(Unknown),
causing valid byte lists like [1,2,3] to be rejected; update the content-type
condition in that match arm to accept Type::List(_) where the inner type is
Number OR Any OR Unknown (i.e., treat Type::List(Box::new(Type::Number)),
Type::List(Box::new(Type::Any)), and Type::List(Box::new(Type::Unknown)) as
valid), while keeping the existing allowances for Type::Binary, Type::Unknown,
Type::Any, and Type::Error; adjust the conditional in the branch that calls
self.type_error to include these additional List variants so literal lists are
accepted as binary content.
In `@wflhub_language_gaps_prd.md`:
- Around line 736-760: The fenced code block containing the dependency graph
that starts with "Binary I/O (`#1`) ──────────────────────► Multipart Form Parsing
(`#5`)" is missing a language identifier; add a language tag (e.g., text) to the
opening fence so it becomes "```text" and keep the closing fence as "```" to
satisfy Markdownlint MD040 and ensure the block is lint-clean.
- Around line 831-841: The fenced code block showing the sample .wflcfg project
configuration in wflhub_language_gaps_prd.md is missing a language tag (MD040);
update the opening fence to include a language identifier (e.g., change "```" to
"```text") for the block containing the database_url/jwt_secret/smtp_host lines
so the markdown linter passes.
Introduces a 50MB limit for both reading and writing binary data to prevent potential memory exhaustion from very large operations. Adds input validation to catch common errors, including checks for: - Negative byte counts when reading. - Values outside the valid 0-255 range when writing a list of numbers as bytes. Improves runtime error messages for type mismatches to be more user-friendly by showing the value's type name instead of its debug representation. Also adds documentation for binary-related syntax in the AST.
Adds stricter validation for byte values and counts in binary operations, ensuring they are finite, non-negative integers within the expected range. Allows `GET_FILE_SIZE` to accept file paths as a fallback in addition to file handles, increasing its versatility. Relaxes type checking for `writeBinary` to accept lists of type `Any` or `Unknown`. Fixes JavaScript transpilation for `writeBinary` to correctly pass the file path to the runtime function.
Adds `text` language specifiers to code blocks to ensure they are rendered correctly as plain text.
|
Claude finished @logbie's task —— View job Code Review: Binary File I/O ImplementationTodo List
Executive SummaryThis PR successfully implements binary file I/O capabilities for WFL, adding fundamental support needed for projects like WFLHub. The implementation is well-architected, follows WFL conventions, and integrates cleanly with the existing compiler pipeline. Detailed Review✅ Code Quality & ArchitectureStrengths:
Good patterns observed:
|
|
Claude finished @logbie's task —— View job Code Review: Binary File I/O ImplementationTodo List
Executive SummaryThis PR successfully implements binary file I/O capabilities for WFL, adding fundamental support needed for projects like WFLHub. The implementation is well-architected, follows WFL conventions, and integrates cleanly with the existing compiler pipeline. Detailed Review✅ Code Quality & ArchitectureStrengths:
Good patterns observed:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| async fn read_binary(&self, handle_id: &str) -> Result<Vec<u8>, String> { | ||
| let mut file_handles = self.file_handles.lock().await; | ||
|
|
||
| if !file_handles.contains_key(handle_id) { | ||
| return Err(format!("Invalid file handle: {handle_id}")); | ||
| } | ||
|
|
||
| let mut file_clone = match file_handles.get_mut(handle_id).unwrap().1.try_clone().await { | ||
| Ok(clone) => clone, | ||
| Err(e) => return Err(format!("Failed to clone file handle: {e}")), | ||
| }; | ||
|
|
||
| drop(file_handles); | ||
|
|
||
| // Seek to start before reading all | ||
| match AsyncSeekExt::seek(&mut file_clone, std::io::SeekFrom::Start(0)).await { | ||
| Ok(_) => {} | ||
| Err(e) => return Err(format!("Failed to seek in file: {e}")), | ||
| } |
There was a problem hiding this comment.
try_clone() typically duplicates the same underlying OS file description, which commonly shares a single cursor/offset across clones. Seeking this cloned handle back to 0 is therefore likely to reset the file position for other uses of the same handle (and can also race with other reads/writes), producing surprising behavior. A safer approach is to reopen the file by stored path for “read all” operations (independent cursor), or keep a single handle and perform seek/read under the same lock (accepting that the cursor moves deterministically).
| self.type_error( | ||
| "Expected Binary or List of Number for write binary content".to_string(), | ||
| Some(Type::Binary), | ||
| Some(content_type), | ||
| *_line, | ||
| *_column, | ||
| ); | ||
| } | ||
| let target_type = self.infer_expression_type(target); | ||
| if target_type != Type::Custom("File".to_string()) | ||
| && target_type != Type::Text | ||
| && target_type != Type::Unknown | ||
| && target_type != Type::Error | ||
| { | ||
| self.type_error( | ||
| "Expected a file handle or string".to_string(), | ||
| Some(Type::Custom("File".to_string())), | ||
| Some(target_type), | ||
| *_line, | ||
| *_column, | ||
| ); | ||
| } |
There was a problem hiding this comment.
The expected type passed into type_error(...) doesn’t match what the checker actually accepts: (1) the message allows Binary or List of Number, but expected is only Binary; (2) the message allows “file handle or string”, but expected is only File. This can produce misleading diagnostics (and potentially misleading fixer suggestions). Consider passing None (so the message is authoritative) or extending the type error representation to express multiple acceptable types consistently.
| // 50MB limit to prevent memory exhaustion | ||
| const MAX_BINARY_WRITE: usize = 50 * 1024 * 1024; | ||
| let bytes = match &content_value { | ||
| Value::Binary(b) => b.clone(), |
There was a problem hiding this comment.
For Value::Binary, this clones the entire buffer before writing. Since the write path already accepts a &[u8], you can avoid the cloning by passing the existing Vec<u8> slice directly (restructuring slightly to satisfy borrowing across await if needed).
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/interpreter/mod.rs`:
- Around line 666-691: read_binary currently reads the entire file without
enforcing the 50MB limit; after you obtain the cloned file handle (in
read_binary, after the successful try_clone of
file_handles.get_mut(...).unwrap().1) call the async metadata method on the
cloned file to get its size (e.g., metadata().await?.len()), compare against a
50 * 1024 * 1024 byte cap and return an Err if it exceeds the cap, and only then
proceed to seek and call AsyncReadExt::read_to_end; ensure the error message
clearly states the file is too large and uses the same Result<String> error
pattern as the existing failure branches.
- Around line 717-740: In write_binary, add a guard that rejects payloads larger
than 50 MB by checking data.len() (e.g. if data.len() > 50 * 1024 * 1024) and
returning an Err with a clear message before attempting to clone or write to the
handle; place this check after validating the handle_id with file_handles and
before calling file_handles.get_mut(...).try_clone().await so you avoid
unnecessary clones/writes; keep the rest of the flow (AsyncWriteExt::write_all,
flush, and calling Self::sync_file_with_windows_handling) unchanged.
In `@wflhub_language_gaps_prd.md`:
- Around line 59-77: The example uses the wrong file-size syntax; change the
statement that currently reads the size from using "size of archive_handle" to
the implemented syntax "file size of archive_handle" (i.e., update the line
referencing archive_handle to use the "file size of <handle>" form used in tests
like binary_io_test.rs), then validate all WFL examples in this document with
the MCP tooling before finalizing.
🧹 Nitpick comments (2)
wflhub_language_gaps_prd.md (1)
874-881: Prioritize resolving open questions before Phase 1 implementation.Several open questions could impact Phase 1 implementation, particularly:
Question 4 (Binary value interop): This should be resolved before implementing Binary I/O (
#1), as it's a P0 blocker. The decision affects how Binary values integrate with existing language operations and could require additional AST/typechecker work.Question 3 (Session storage backend): While sessions are Phase 2, deciding between memory-only vs database-backed affects the implementation approach for feature
#6.Consider creating a separate decision document or ADR (Architecture Decision Record) to resolve these questions with concrete examples and tradeoffs before starting implementation.
src/typechecker/mod.rs (1)
1079-1116: Consider usingare_types_compatiblefor the content-type check.The current approach enumerates specific
Listinner types (Number,Any,Unknown) with direct equality. AListwith another compatible inner type (e.g.,List(Error)) would be rejected. Usingare_types_compatiblewould be more robust and consistent with how other parts of the type checker handle subtype relationships. That said, this matches the pattern used elsewhere in the file (e.g.,WriteFileStatement), so it's acceptable for now.
| async fn read_binary(&self, handle_id: &str) -> Result<Vec<u8>, String> { | ||
| let mut file_handles = self.file_handles.lock().await; | ||
|
|
||
| if !file_handles.contains_key(handle_id) { | ||
| return Err(format!("Invalid file handle: {handle_id}")); | ||
| } | ||
|
|
||
| let mut file_clone = match file_handles.get_mut(handle_id).unwrap().1.try_clone().await { | ||
| Ok(clone) => clone, | ||
| Err(e) => return Err(format!("Failed to clone file handle: {e}")), | ||
| }; | ||
|
|
||
| drop(file_handles); | ||
|
|
||
| // Seek to start before reading all | ||
| match AsyncSeekExt::seek(&mut file_clone, std::io::SeekFrom::Start(0)).await { | ||
| Ok(_) => {} | ||
| Err(e) => return Err(format!("Failed to seek in file: {e}")), | ||
| } | ||
|
|
||
| let mut contents = Vec::new(); | ||
| match AsyncReadExt::read_to_end(&mut file_clone, &mut contents).await { | ||
| Ok(_) => Ok(contents), | ||
| Err(e) => Err(format!("Failed to read binary file: {e}")), | ||
| } | ||
| } |
There was a problem hiding this comment.
Enforce the 50 MB cap before reading an entire binary file.
read_binary reads the whole file without a size guard, which can bypass the 50 MB limit and exhaust memory. Add a metadata size check before read_to_end.
🔧 Suggested fix
async fn read_binary(&self, handle_id: &str) -> Result<Vec<u8>, String> {
let mut file_handles = self.file_handles.lock().await;
if !file_handles.contains_key(handle_id) {
return Err(format!("Invalid file handle: {handle_id}"));
}
let mut file_clone = match file_handles.get_mut(handle_id).unwrap().1.try_clone().await {
Ok(clone) => clone,
Err(e) => return Err(format!("Failed to clone file handle: {e}")),
};
drop(file_handles);
+ // 50MB limit to prevent memory exhaustion
+ const MAX_BINARY_READ: u64 = 50 * 1024 * 1024;
+ let meta = file_clone
+ .metadata()
+ .await
+ .map_err(|e| format!("Failed to get file size: {e}"))?;
+ if meta.len() > MAX_BINARY_READ {
+ return Err(format!(
+ "Binary file size {} exceeds maximum allowed ({})",
+ meta.len(),
+ MAX_BINARY_READ
+ ));
+ }
+
// Seek to start before reading all
match AsyncSeekExt::seek(&mut file_clone, std::io::SeekFrom::Start(0)).await {
Ok(_) => {}
Err(e) => return Err(format!("Failed to seek in file: {e}")),
}🤖 Prompt for AI Agents
In `@src/interpreter/mod.rs` around lines 666 - 691, read_binary currently reads
the entire file without enforcing the 50MB limit; after you obtain the cloned
file handle (in read_binary, after the successful try_clone of
file_handles.get_mut(...).unwrap().1) call the async metadata method on the
cloned file to get its size (e.g., metadata().await?.len()), compare against a
50 * 1024 * 1024 byte cap and return an Err if it exceeds the cap, and only then
proceed to seek and call AsyncReadExt::read_to_end; ensure the error message
clearly states the file is too large and uses the same Result<String> error
pattern as the existing failure branches.
| async fn write_binary(&self, handle_id: &str, data: &[u8]) -> Result<(), String> { | ||
| let mut file_handles = self.file_handles.lock().await; | ||
|
|
||
| if !file_handles.contains_key(handle_id) { | ||
| return Err(format!("Invalid file handle: {handle_id}")); | ||
| } | ||
|
|
||
| let mut file_clone = match file_handles.get_mut(handle_id).unwrap().1.try_clone().await { | ||
| Ok(clone) => clone, | ||
| Err(e) => return Err(format!("Failed to clone file handle: {e}")), | ||
| }; | ||
|
|
||
| drop(file_handles); | ||
|
|
||
| match AsyncWriteExt::write_all(&mut file_clone, data).await { | ||
| Ok(_) => match file_clone.flush().await { | ||
| Ok(_) => { | ||
| Self::sync_file_with_windows_handling(&mut file_clone, "write_binary").await | ||
| } | ||
| Err(e) => Err(format!("Failed to flush binary file: {e}")), | ||
| }, | ||
| Err(e) => Err(format!("Failed to write binary data: {e}")), | ||
| } | ||
| } |
There was a problem hiding this comment.
Add a max-size guard in write_binary for Binary payloads.
Caller-side checks only cover list conversions; direct Binary writes can bypass the 50 MB limit. Enforce the cap here for consistency.
🔧 Suggested fix
async fn write_binary(&self, handle_id: &str, data: &[u8]) -> Result<(), String> {
let mut file_handles = self.file_handles.lock().await;
if !file_handles.contains_key(handle_id) {
return Err(format!("Invalid file handle: {handle_id}"));
}
let mut file_clone = match file_handles.get_mut(handle_id).unwrap().1.try_clone().await {
Ok(clone) => clone,
Err(e) => return Err(format!("Failed to clone file handle: {e}")),
};
drop(file_handles);
+ // 50MB limit to prevent memory exhaustion
+ const MAX_BINARY_WRITE: usize = 50 * 1024 * 1024;
+ if data.len() > MAX_BINARY_WRITE {
+ return Err(format!(
+ "Binary payload size {} exceeds maximum allowed ({})",
+ data.len(),
+ MAX_BINARY_WRITE
+ ));
+ }
+
match AsyncWriteExt::write_all(&mut file_clone, data).await {
Ok(_) => match file_clone.flush().await {
Ok(_) => {
Self::sync_file_with_windows_handling(&mut file_clone, "write_binary").await
}🤖 Prompt for AI Agents
In `@src/interpreter/mod.rs` around lines 717 - 740, In write_binary, add a guard
that rejects payloads larger than 50 MB by checking data.len() (e.g. if
data.len() > 50 * 1024 * 1024) and returning an Err with a clear message before
attempting to clone or write to the handle; place this check after validating
the handle_id with file_handles and before calling
file_handles.get_mut(...).try_clone().await so you avoid unnecessary
clones/writes; keep the rest of the flow (AsyncWriteExt::write_all, flush, and
calling Self::sync_file_with_windows_handling) unchanged.
| ```wfl | ||
| // Open for binary read | ||
| open file at "package.wflpkg" for reading binary as archive_handle | ||
|
|
||
| // Open for binary write | ||
| open file at "output.wflpkg" for writing binary as output_handle | ||
|
|
||
| // Read raw bytes (returns a binary buffer value) | ||
| store contents as read binary from archive_handle | ||
|
|
||
| // Read specific byte count | ||
| store chunk as read 4096 bytes from archive_handle | ||
|
|
||
| // Write raw bytes | ||
| write binary contents into output_handle | ||
|
|
||
| // Get file size in bytes | ||
| store size as size of archive_handle | ||
| ``` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the actual file location
fd -type f -name "*language_gaps*" -o -name "*wflhub*"Repository: WebFirstLanguage/wfl
Length of output: 234
🏁 Script executed:
#!/bin/bash
# Search for file size related operations in the codebase
ast-grep --pattern 'FileSizeOf'Repository: WebFirstLanguage/wfl
Length of output: 102
🏁 Script executed:
#!/bin/bash
# Search for file size syntax in parser and implementation files
rg -n -i "file.*size|size.*of" --type=rust -A 3 -B 3 | head -100Repository: WebFirstLanguage/wfl
Length of output: 5750
🏁 Script executed:
#!/bin/bash
# Search for binary file operations in the codebase
rg -n "binary.*read|binary.*write" --type=rust -A 2 | head -80Repository: WebFirstLanguage/wfl
Length of output: 1257
🏁 Script executed:
#!/bin/bash
# Find the exact location and path of the file
find . -type f -name "*language_gaps*" -o -name "*wflhub_language*" 2>/dev/nullRepository: WebFirstLanguage/wfl
Length of output: 93
🏁 Script executed:
#!/bin/bash
# Check if file exists in Docs directory
find ./Docs -type f -name "*language*" -o -name "*wflhub*" 2>/dev/null | head -20Repository: WebFirstLanguage/wfl
Length of output: 160
🏁 Script executed:
#!/bin/bash
# Look for the PR or document in question
git ls-files | grep -i "language_gaps\|wflhub_language" | head -20Repository: WebFirstLanguage/wfl
Length of output: 91
Fix file size syntax in code example to match implementation.
The code example at lines 59-77 uses size of archive_handle for the file size operation, but the actual implemented syntax is file size of <handle>. This is visible in the test files (e.g., tests/binary_io_test.rs):
store sz as file size of rh
Update line 76 from store size as size of archive_handle to store size as file size of archive_handle.
Additionally, per the learnings on documentation code examples, validate all WFL code examples in this PRD using MCP tools before finalizing, especially if this content will be incorporated into the Docs/ directory.
🤖 Prompt for AI Agents
In `@wflhub_language_gaps_prd.md` around lines 59 - 77, The example uses the wrong
file-size syntax; change the statement that currently reads the size from using
"size of archive_handle" to the implemented syntax "file size of archive_handle"
(i.e., update the line referencing archive_handle to use the "file size of
<handle>" form used in tests like binary_io_test.rs), then validate all WFL
examples in this document with the MCP tooling before finalizing.
Switches the representation of string literals in the Abstract Syntax Tree (AST) from `Rc` to the atomically reference-counted `Arc`. This change makes the AST data structures thread-safe, laying the groundwork for future concurrent processing, such as parallel analysis or execution. The parser, interpreter, and all related tests are updated to accommodate this change.
|
Claude finished @logbie's task —— View job Code Review: Binary File I/O Implementation
|
Adds dedicated steps to build and run tests for the language server package in both the main CI and nightly workflows. This ensures the LSP is continuously validated, helping to catch regressions and compilation issues early.
|
Claude finished @logbie's task —— View job Code Review: Binary File I/O ImplementationTodo List
Executive SummaryThis PR successfully implements binary file I/O capabilities for WFL, adding fundamental support needed for projects like WFLHub. The implementation is well-architected, follows WFL conventions, and integrates cleanly with the existing compiler pipeline. Detailed Review✅ Code Quality & ArchitectureStrengths:
Good patterns observed:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| async fn read_binary(&self, handle_id: &str) -> Result<Vec<u8>, String> { | ||
| let mut file_handles = self.file_handles.lock().await; | ||
|
|
||
| if !file_handles.contains_key(handle_id) { | ||
| return Err(format!("Invalid file handle: {handle_id}")); | ||
| } | ||
|
|
||
| let mut file_clone = match file_handles.get_mut(handle_id).unwrap().1.try_clone().await { | ||
| Ok(clone) => clone, | ||
| Err(e) => return Err(format!("Failed to clone file handle: {e}")), | ||
| }; | ||
|
|
||
| drop(file_handles); | ||
|
|
||
| // Seek to start before reading all | ||
| match AsyncSeekExt::seek(&mut file_clone, std::io::SeekFrom::Start(0)).await { | ||
| Ok(_) => {} | ||
| Err(e) => return Err(format!("Failed to seek in file: {e}")), | ||
| } | ||
|
|
||
| let mut contents = Vec::new(); | ||
| match AsyncReadExt::read_to_end(&mut file_clone, &mut contents).await { | ||
| Ok(_) => Ok(contents), | ||
| Err(e) => Err(format!("Failed to read binary file: {e}")), | ||
| } | ||
| } |
There was a problem hiding this comment.
try_clone() on a file handle typically shares the underlying file offset with the original handle (e.g., via dup semantics), and the explicit seek(..Start(0)) will therefore mutate the position for all clones/operations on that handle. This can break sequential reads (read N bytes then read binary) and can cause races if multiple tasks read/write the same handle concurrently. A more robust approach is to store each file handle behind a per-handle async lock (e.g., Arc<tokio::sync::Mutex<File>>) so you don’t need try_clone(), and avoid forcibly seeking to 0 unless the language semantics explicitly require “read entire file from start regardless of cursor”.
| FileOpenMode::ReadBinary => "rb", | ||
| FileOpenMode::WriteBinary => "wb", |
There was a problem hiding this comment.
Node.js fs open flags don’t use a 'b' suffix (I/O is byte-oriented by default), so "rb" / "wb" are not valid standard flags and may break transpiled programs depending on how WFL.file.open is implemented. Consider mapping ReadBinary/WriteBinary to "r"/"w" and letting the runtime distinguish text vs binary at the read/write API level (or use a runtime-specific flag string that you know WFL.file supports).
| FileOpenMode::ReadBinary => "rb", | |
| FileOpenMode::WriteBinary => "wb", | |
| FileOpenMode::ReadBinary => "r", | |
| FileOpenMode::WriteBinary => "w", |
| Statement::WriteBinaryStatement { | ||
| content, target, .. | ||
| } => { | ||
| let content_expr = self.transpile_expression(content)?; | ||
| let target_expr = self.transpile_expression(target)?; | ||
| Ok(format!( | ||
| "{}WFL.file.writeBinary({}.path, {});\n", | ||
| self.indent(), | ||
| target_expr, | ||
| content_expr | ||
| )) | ||
| } |
There was a problem hiding this comment.
This transpilation assumes target_expr is an object with a .path field. However, the typechecker branch for WriteBinaryStatement explicitly allows target to be Text (string), which would produce invalid JS like "out.bin".path. Either (1) tighten the typechecker to require a file-handle type for write binary in JS-transpiled mode, or (2) generate code that resolves a path at runtime (e.g., typeof x === 'string' ? x : x.path) and use that consistently for all file APIs.
| if matches!( | ||
| next_token.token, | ||
| Token::IntLiteral(_) | Token::Identifier(_) | ||
| ) { | ||
| // Speculatively parse count expression, then check for "bytes" | ||
| let saved_pos = self.cursor.checkpoint(); | ||
| if let Ok(count_expr) = self.parse_primary_expression() | ||
| && let Some(bytes_tok) = self.cursor.peek() | ||
| && bytes_tok.token == Token::KeywordBytes | ||
| { | ||
| self.bump_sync(); // Consume "bytes" | ||
| self.expect_token( | ||
| Token::KeywordFrom, | ||
| "Expected 'from' after 'read N bytes'", | ||
| )?; | ||
| let file_handle = self.parse_primary_expression()?; | ||
| return Ok(Expression::ReadBinaryN { | ||
| file_handle: Box::new(file_handle), | ||
| count: Box::new(count_expr), | ||
| line: token_line, | ||
| column: token_column, | ||
| }); | ||
| } | ||
| // Not "read N bytes from" pattern, restore position | ||
| self.cursor.rewind(saved_pos); | ||
| } |
There was a problem hiding this comment.
read <N> bytes from ... only accepts N as a primary expression starting with an int literal or identifier, which prevents common patterns like read (n + 1) bytes from fh or read length_of_chunk bytes from fh when those are non-primary expressions. Since you’re already doing speculative parsing with a checkpoint, consider parsing count with parse_expression() (and only committing if it’s followed by bytes from) so the feature matches the “count can be an expression” expectation implied by the AST (count: Box<Expression>).
| if matches!( | |
| next_token.token, | |
| Token::IntLiteral(_) | Token::Identifier(_) | |
| ) { | |
| // Speculatively parse count expression, then check for "bytes" | |
| let saved_pos = self.cursor.checkpoint(); | |
| if let Ok(count_expr) = self.parse_primary_expression() | |
| && let Some(bytes_tok) = self.cursor.peek() | |
| && bytes_tok.token == Token::KeywordBytes | |
| { | |
| self.bump_sync(); // Consume "bytes" | |
| self.expect_token( | |
| Token::KeywordFrom, | |
| "Expected 'from' after 'read N bytes'", | |
| )?; | |
| let file_handle = self.parse_primary_expression()?; | |
| return Ok(Expression::ReadBinaryN { | |
| file_handle: Box::new(file_handle), | |
| count: Box::new(count_expr), | |
| line: token_line, | |
| column: token_column, | |
| }); | |
| } | |
| // Not "read N bytes from" pattern, restore position | |
| self.cursor.rewind(saved_pos); | |
| } | |
| // Speculatively parse count expression as a full expression, then check for "bytes" | |
| let saved_pos = self.cursor.checkpoint(); | |
| if let Ok(count_expr) = self.parse_expression() | |
| && let Some(bytes_tok) = self.cursor.peek() | |
| && bytes_tok.token == Token::KeywordBytes | |
| { | |
| self.bump_sync(); // Consume "bytes" | |
| self.expect_token( | |
| Token::KeywordFrom, | |
| "Expected 'from' after 'read N bytes'", | |
| )?; | |
| let file_handle = self.parse_primary_expression()?; | |
| return Ok(Expression::ReadBinaryN { | |
| file_handle: Box::new(file_handle), | |
| count: Box::new(count_expr), | |
| line: token_line, | |
| column: token_column, | |
| }); | |
| } | |
| // Not "read N bytes from" pattern, restore position | |
| self.cursor.rewind(saved_pos); |
| ) -> Result<Option<Value>, RuntimeError> { | ||
| match literal { | ||
| Literal::String(s) => Ok(Some(Value::Text(s.clone()))), | ||
| Literal::String(s) => Ok(Some(Value::Text(Rc::from(&**s)))), |
There was a problem hiding this comment.
After switching Literal::String to Arc<str>, the interpreter converts it to a fresh Rc<str> each time the literal is evaluated. This adds repeated allocations/atomic refcount churn for hot paths (especially in expression evaluation). Consider aligning the string backing types end-to-end (e.g., make Value::Text use Arc<str> too, or keep literals as Rc<str> in interpreter-only builds) so literal evaluation is a cheap clone without re-allocation.
| } | ||
| Expression::Literal(literal, _line, _column) => match literal { | ||
| Literal::String(s) => Ok(Value::Text(s.clone())), | ||
| Literal::String(s) => Ok(Value::Text(Rc::from(&**s))), |
There was a problem hiding this comment.
After switching Literal::String to Arc<str>, the interpreter converts it to a fresh Rc<str> each time the literal is evaluated. This adds repeated allocations/atomic refcount churn for hot paths (especially in expression evaluation). Consider aligning the string backing types end-to-end (e.g., make Value::Text use Arc<str> too, or keep literals as Rc<str> in interpreter-only builds) so literal evaluation is a cheap clone without re-allocation.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/analyzer/static_analyzer.rs (2)
638-748:⚠️ Potential issue | 🟠 MajorMissing handling for new binary
Expressionvariants inmark_used_in_expression.
ReadBinaryContent { file_handle, .. },ReadBinaryN { file_handle, count, .. }, andFileSizeOf { file_handle, .. }all fall through to the_ => {}wildcard. Variables used as file handles (or byte counts) in these expressions won't be marked as used, causing false positive "unused variable" warnings.Proposed fix — add before the wildcard arm
Expression::AwaitExpression { expression, .. } => { self.mark_used_in_expression(expression, usages); } + Expression::ReadBinaryContent { file_handle, .. } + | Expression::FileSizeOf { file_handle, .. } => { + self.mark_used_in_expression(file_handle, usages); + } + Expression::ReadBinaryN { file_handle, count, .. } => { + self.mark_used_in_expression(file_handle, usages); + self.mark_used_in_expression(count, usages); + } _ => {}
566-629:⚠️ Potential issue | 🟠 MajorAdd missing
WriteBinaryStatementhandler inmark_used_variables.The
WriteBinaryStatement { content, target, .. }variant is not matched and falls through to_ => {}, causing variables referenced incontentandtargetexpressions to be incorrectly reported as unused.Proposed fix
Statement::CloseFileStatement { file, .. } => { self.mark_used_in_expression(file, usages); } + Statement::WriteBinaryStatement { content, target, .. } => { + self.mark_used_in_expression(content, usages); + self.mark_used_in_expression(target, usages); + } Statement::WaitForStatement { inner, .. } => {
🧹 Nitpick comments (3)
.github/workflows/nightly.yml (1)
239-241: LSP tests are redundantly re-run in this step.The preceding
Run testsstep (line 237) executescargo test --release --locked --target …without specifying a package, which by default runs tests for all workspace members—includingwfl-lsp. The new step (lines 239-241) re-executes those same tests with--verbose, adding unnecessary CI time.Consider either:
- Adding
--verboseto the existing test step and removing this one, or- Adding
--exclude wfl-lspto the general step if you want separate, verbose LSP output.src/parser/stmt/io.rs (1)
29-211: Consider extracting line/column fromExpressionvia a helper method.Every arm in this match does the exact same thing: extract
line/columnfrom the expression and wrap it inDisplayStatement. This currently requires updating ~40 arms whenever a newExpressionvariant is added. A helper likeexpr.position() -> (i32, i32)(or a trait method) would collapse this entire match into a 3-line body.This is a pre-existing pattern, so no urgency — just flagging it since this PR made it even longer.
src/analyzer/static_analyzer.rs (1)
766-886:WriteBinaryStatementfalls through to_ => 0in CFG line/column extraction.The new
WriteBinaryStatementvariant isn't listed in the exhaustive match arms that extractline/columnfor CFG nodes (repeated ~8 times inbuild_cfg). It silently falls to_ => 0, so any unreachable-code diagnostic on a binary write statement would report position(0, 0).This is a pre-existing structural issue (the match is duplicated many times), but the new variant exacerbates it. Consider adding
Statement::WriteBinaryStatement { line, .. } => *lineto each block — or, better, extracting aStatement::position(&self) -> (usize, usize)helper to eliminate all these duplicated matches.
Replaces `Rc` with `Arc` for the `Value::Text` variant to ensure thread safety for string data. This fundamental change prepares the interpreter for future concurrency and multi-threading capabilities, such as handling web server requests in parallel. Using `Arc` allows text values to be safely shared across multiple threads. This also slightly improves performance when handling string literals by avoiding an unnecessary allocation and copy.
|
Claude finished @logbie's task —— View job Code Review: Binary File I/O ImplementationTodo List
Executive SummaryThis PR successfully implements binary file I/O capabilities for WFL, adding fundamental support needed for projects like WFLHub. The implementation is well-architected, follows WFL conventions, and integrates cleanly with the existing compiler pipeline. Detailed Review✅ Code Quality & ArchitectureStrengths:
Good patterns observed:
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/stdlib/helpers.rs (1)
199-229:⚠️ Potential issue | 🟡 MinorStale doc comments still reference
Rc<str>.The function signature was updated to return
Arc<str>, but the doc comment (lines 199–226) still mentionsRc<str>in multiple places: the summary line, the Returns section, and the example (which showsRc::from(...)usage). These should be updated to reflect theArcmigration.📝 Proposed doc fix
-/// Extracts a text value from a WFL Value, returning it as a reference-counted string. +/// Extracts a text value from a WFL Value, returning it as a thread-safe reference-counted string. /// -/// Returns an `Rc<str>` to enable efficient memory sharing without copying the string -/// data. This is the standard way to extract text values in the WFL runtime. +/// Returns an `Arc<str>` to enable efficient, thread-safe memory sharing without copying the +/// string data. This is the standard way to extract text values in the WFL runtime. /// /// # Arguments /// /// * `value` - The WFL Value to extract from /// /// # Returns /// -/// Returns an `Rc<str>` clone (incrementing the reference count) if the value is a Text variant. +/// Returns an `Arc<str>` clone (incrementing the reference count) if the value is a Text variant. /// The underlying string data is not copied, only the reference count is incremented. ... /// ```ignore /// pub fn native_uppercase(args: Vec<Value>) -> Result<Value, RuntimeError> { /// check_arg_count("uppercase", &args, 1)?; /// let text = expect_text(&args[0])?; -/// Ok(Value::Text(Rc::from(text.to_uppercase()))) +/// Ok(Value::Text(Arc::from(text.to_uppercase()))) /// } /// ```
🧹 Nitpick comments (3)
src/stdlib/pattern.rs (1)
292-293: Nit: redundant rebinding ofchar_to_byte.The immutable binding on line 292 is immediately re-bound as mutable on line 293. Simplify by declaring it
mutupfront.✏️ Suggested simplification
- let char_to_byte: Vec<usize> = text.char_indices().map(|(byte_idx, _)| byte_idx).collect(); - let mut char_to_byte = char_to_byte; + let mut char_to_byte: Vec<usize> = text.char_indices().map(|(byte_idx, _)| byte_idx).collect();src/interpreter/value.rs (2)
377-397:PartialEqforBinaryis correct but note the fast-path duplication.
Binarycomparison appears in both the fast-path (line 390) andeq_with_visited(line 484). This is harmless since Binary can't contain cycles, but it means theeq_with_visitedarm is unreachable for Binary—the fast path always returns first. Not a problem, just worth knowing if you ever refactor the equality logic.
8-9:Valueremains!Send + !Sync—thread-safety goal only partially achieved with Arc migration.The commit migrates
TexttoArc<str>to "make text values thread-safe and prepare the interpreter for concurrency," butValuestill wrapsRc<RefCell<…>>forList,Object,Function,Future,ContainerInstance, and other variants (lines 16–18, 20–24, 30–34). This meansValuecannot be sent across thread boundaries.To fully enable thread-safe concurrency, migrate remaining
Rc<RefCell<…>>types toArc<Mutex<…>>orArc<RwLock<…>>where needed. This is a natural next step once the interpreter scales to true multi-threaded execution contexts.
Introduces the fundamental capabilities for reading and writing raw bytes, a prerequisite for handling binary file uploads and downloads for projects like WFLHub.
This change adds new syntax for opening files in binary mode, reading entire files or a specific number of bytes, writing binary data, and retrieving file sizes. A new
Binaryvalue type is integrated throughout the compiler pipeline, including the parser, type checker, and interpreter.open ... for reading/writing binarysyntaxread binary from,read N bytes from, andfile size ofexpressionswrite binary intostatementAdditionally, adds the Product Requirements Document (PRD) outlining this and other planned language enhancements.
Summary by CodeRabbit
New Features
Language / Syntax
Tests
Documentation
CI