Repository navigation
fix(fs): wire fs.appendFileSync end-to-end (closes #226) - #231
Merged
Merged
Conversation
Closes PerryTS#226. Pre-fix every `fs.appendFileSync(path, content)` wrote 0 bytes — the file wasn't even created on a fresh path. The runtime function (`crates/perry-runtime/src/fs.rs::js_fs_append_file_sync`) was already correct (`OpenOptions::new().create(true).append(true).write_all(...)`), the gap was in two earlier passes. The HIR carries a dedicated `Expr::FsAppendFileSync(path, content)` variant for this. The named-import path of the SWC→HIR lowerer already produced it (`import { appendFileSync } from "fs"`), and the JS / WASM backends both had codegen arms for it. Two things on the LLVM track were missing: 1. `crates/perry-hir/src/lower/expr_call.rs` — the namespace-import path (`import * as fs from "fs"; fs.appendFileSync(...)`) had arms for `writeFileSync` / `existsSync` / `mkdirSync` / `unlinkSync` etc. but not `appendFileSync`. Without this, the call slipped past as a generic `Call { callee: PropertyGet { object: NativeModuleRef("fs"), property: "appendFileSync" }, args: [...] }` and `Expr::FsAppendFileSync` never got emitted for the most common usage shape. Added the arm next to `writeFileSync`. 2. `crates/perry-codegen/src/expr.rs` — even when the HIR variant was emitted (named-import path), the LLVM backend had no arm next to `Expr::FsWriteFileSync`. The call fell through and was silently dropped. Added the arm; same shape as `FsWriteFileSync`, calls `js_fs_append_file_sync`. Plus the runtime declaration in `crates/perry-codegen/src/runtime_decls.rs` so the LLVM module declares `js_fs_append_file_sync` for the linker. # Verified `cargo build --release -p perry` clean, `cargo test -p perry-hir -p perry-codegen --lib` green (20 tests, no regressions). End-to-end against `node --experimental-strip-types` on the new `test-files/test_fs_appendfile.ts`: perry: node: line one line one <blank> line one line one line two line two <blank> line one line one line two line two line three line three Identical output. # Issue diagnosis correction The issue body diagnosed only the codegen-arm-faltante (path 2 above). While iterating on this fix I hit the surprising case where adding just that arm changed nothing for `import * as fs from "fs"; fs.append(...)`. Tracing the HIR confirmed the namespace-import path needed extending too. Both arms in this commit; either alone is insufficient for the common usage pattern.
Contributor
|
Doc tests fail (expected, okay), once mac and linux greenlight will merge! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #226.
What was broken
Pre-fix every
fs.appendFileSync(path, content)wrote 0 bytes — the file wasn't even created on a fresh path. Confirmed againstnode --experimental-strip-types(Node correctly accumulates content, Perry produced an empty file).Why the issue's diagnosis was incomplete
The issue body pointed at
crates/perry-codegen/src/expr.rsnext toFsWriteFileSync— that arm was indeed missing. But adding it alone changes nothing for the common usage shape, because the HIRExpr::FsAppendFileSyncvariant is never emitted for namespace imports.The SWC→HIR lowerer in
crates/perry-hir/src/lower/expr_call.rshas two paths forfs.X(...)calls:import { appendFileSync } from "fs"— already had theappendFileSyncarm.import * as fs from "fs"; fs.appendFileSync(...)— had arms forwriteFileSync/existsSync/mkdirSync/unlinkSync/rmRecursive/readFileBuffer, but notappendFileSync. Without this,fs.appendFileSync(path, content)slipped past as a genericCall { callee: PropertyGet { object: NativeModuleRef("fs"), property: "appendFileSync" }, args: [...] }and the typed variant never got emitted.That namespace shape is what most user code actually writes (
import * as fs from "fs"), so the bug surfaced as "every call writes 0 bytes" in practice.writeFileSyncworks because it's in both paths;appendFileSyncwas in only one.The runtime function (
crates/perry-runtime/src/fs.rs::js_fs_append_file_sync) was already correct —OpenOptions::new().create(true).append(true).write_all(content).What changed
appendFileSyncarm to the namespace-import path next towriteFileSync.Expr::FsAppendFileSyncarm next toFsWriteFileSync. Same shape: lower path + content, calljs_fs_append_file_sync, discard the i32 status.js_fs_append_file_syncso the LLVM module can resolve the symbol at link.test-files/test_fs_appendfile.tsexercising the commonimport * as fs from "fs"shape with three sequential appends.JS and WASM backends already had arms for the HIR variant since it was originally added, so they're unaffected.
Verified
cargo build --release -p perryclean;cargo test -p perry-hir -p perry-codegen --libgreen (20 tests, no regressions).Byte-for-byte match against
node --experimental-strip-typeson the regression test:Test plan
cargo test --workspace --exclude perry-ui-ios --exclude perry-ui-tvos --exclude perry-ui-watchos --exclude perry-ui-gtk4 --exclude perry-ui-android --exclude perry-ui-windowstest-files/test_fs_appendfile.tswith the maintainer's localperryand confirm output matches Node