diff --git a/changelog/unreleased/1559.md b/changelog/unreleased/1559.md new file mode 100644 index 0000000000..7a749e49ff --- /dev/null +++ b/changelog/unreleased/1559.md @@ -0,0 +1,4 @@ +- `effects/node/virtual`: `readFile`/`readBytesOp` narrow via + `assert(Array.isArray(file), …)` instead of an unreachable defensive + `return`; `rmOp` drops its unreachable "is a directory" guard entirely + [#1559](https://github.com/functionalscript/functionalscript/pull/1559) diff --git a/fjs/effects/node/virtual/module.f.mjs b/fjs/effects/node/virtual/module.f.mjs index eb574d6d32..8df056b4a6 100644 --- a/fjs/effects/node/virtual/module.f.mjs +++ b/fjs/effects/node/virtual/module.f.mjs @@ -10,7 +10,7 @@ * @import { Dir, State, _Entity } from './types.ts' */ -import { todo } from '../../../asserts/module.f.mjs' +import { assert, todo } from '../../../asserts/module.f.mjs' import { isProperPrefix, join, parse } from '../../../path/module.f.mjs' import { utf8ToString } from '../../../text/module.f.mjs' import { empty, length, maxLengthBytes, msb, vec } from '../../../types/bit_vec/module.f.mjs' @@ -92,8 +92,11 @@ const readFile = readOperation((dir, path) => { const file = dir[path[0]] if (typeof file === 'function') { throw new Error(`'${path[0]}' is a JsModule; readFile not supported`) } if (file === undefined) { return enoent } - if (!Array.isArray(file)) { return error(`'${path[0]}' is not a file`) } - const chunks = /** @type {readonly Vec[]} */ (file) + // `operation`'s wrapper descends into every plain-object (`Dir`) entry + // before this op ever runs, and the `JsModule` case already threw above, + // so `file` here is always a `Vec[]` — never a bare `Dir`. + assert(Array.isArray(file), `'${path[0]}' is not a file`) + const chunks = file const capBits = maxLengthBytes * 8n let result = empty for (const chunk of chunks) { @@ -167,7 +170,11 @@ const rmOp = (dir, path) => { const [name] = path const entry = dir[name] if (entry === undefined) { return [dir, error('no such file')] } - if (!Array.isArray(entry) && typeof entry === 'object') { return [dir, error('is a directory')] } + // No "is a directory" guard here: `operation`'s wrapper descends into + // every plain-object (`Dir`) entry before this op ever runs, so `entry` + // is always a `Vec[]` or a `JsModule` — never a bare `Dir` — and rm can + // always proceed. (`rm` on a genuinely non-empty directory instead hits + // `path.length !== 1` above, once the wrapper has descended into it.) const { [name]: _, ...rest } = dir return [rest, okVoid] } @@ -195,7 +202,13 @@ const extractEntity = (dir, path) => { /** @type {(dir: Dir, path: readonly string[], entity: _Entity) => readonly [Dir, IoResult]} */ const insertEntityAt = (dir, path, entity) => { - if (path.length === 0) { return [dir, error('cannot insert at root')] } + // `insertEntityAt`'s only external caller, `rename`, always rejects an + // empty `dst` earlier — `isProperPrefix([], srcParsed)` is true whenever + // `srcParsed` is non-empty, so renaming onto root is already caught as + // "onto an ancestor" before this function runs. The recursive self-calls + // below never pass an empty path either (they only recurse when + // `path.length > 1`, with a non-empty remainder). + assert(path.length > 0, 'cannot insert at root') if (path.length === 1) { const [name] = path const existing = dir[name] @@ -250,13 +263,16 @@ const readBytesOp = (path, offset, size) => readOperation((dir, p) => { const file = dir[p[0]] if (typeof file === 'function') { throw new Error(`'${p[0]}' is a JsModule; readBytes not supported`) } if (file === undefined) { return enoent } - if (!Array.isArray(file)) { return error(`'${p[0]}' is not a file`) } + // `operation`'s wrapper descends into every plain-object (`Dir`) entry + // before this op ever runs, and the `JsModule` case already threw above, + // so `file` here is always a `Vec[]` — never a bare `Dir`. + assert(Array.isArray(file), `'${p[0]}' is not a file`) if (!Number.isInteger(offset)) { return error(`Offset ${offset} is not an integer`) } if (!Number.isInteger(size)) { return error(`Chunk size ${size} is not an integer`) } if (offset < 0) { return error(`Offset ${offset} is negative`) } if (size < 0) { return error(`Chunk size ${size} is negative`) } if (BigInt(size) > maxLengthBytes) { return error(`Chunk size ${size} exceeds maximum allowed size of ${maxLengthBytes} bytes`) } - const chunks = /** @type {readonly Vec[]} */ (file) + const chunks = file let toSkip = BigInt(offset) * 8n let toRead = BigInt(size) * 8n let result = empty diff --git a/fjs/effects/node/virtual/proof.f.mjs b/fjs/effects/node/virtual/proof.f.mjs index 8e5901d1be..3c261e733d 100644 --- a/fjs/effects/node/virtual/proof.f.mjs +++ b/fjs/effects/node/virtual/proof.f.mjs @@ -3,8 +3,8 @@ */ import { assert, assertEq } from '../../../asserts/module.f.mjs' -import { access, awaitIfPromise, fetch, rm, writeFile, readFile, readdir, import_, rename, readBytes, writeBytes, stat } from '../module.f.mjs' -import { maxLengthBytes, vec, vec8 } from '../../../types/bit_vec/module.f.mjs' +import { access, awaitIfPromise, fetch, rm, writeFile, readFile, readdir, import_, rename, readBytes, writeBytes, stat, createExclusive } from '../module.f.mjs' +import { empty, length, maxLengthBytes, vec, vec8 } from '../../../types/bit_vec/module.f.mjs' import { emptyState, virtual } from './module.f.mjs' export const proof = { @@ -19,7 +19,10 @@ export const proof = { const [, result] = virtual(emptyState)(rm('notexist.txt')) assert(result[0] === 'error') }, - isDirectory: () => { + onDirectory: () => { + // `operation`'s wrapper descends into 'mydir' (a plain object), + // so rmOp itself runs with an empty remaining path and rejects + // via its `path.length !== 1` guard, not a directory-specific one. /** @type {Dir} */ const inner = {} /** @type {Dir} */ @@ -110,6 +113,132 @@ export const proof = { const root = { 'a.f.ts': /** @type {JsModule} */ (() => ({})) } virtual({ ...emptyState, root })(readFile('a.f.ts')) }, + readBytesOnJsModule: () => { + // readBytes on a JsModule path covers typeof file === 'function' branch + /** @type {Dir} */ + const root = { 'a.f.ts': /** @type {JsModule} */ (() => ({})) } + virtual({ ...emptyState, root })(readBytes('a.f.ts', 0, 1)) + }, + }, + readFileSkipsEmptyChunk: () => { + // A file stored with a zero-length chunk ahead of real data: readFile's + // loop must skip it (`chunkLen === 0n`) rather than concatenating it. + /** @type {Dir} */ + const root = { 'f': [empty, vec8(0x42n)] } + const [, result] = virtual({ ...emptyState, root })(readFile('f')) + assert(result[0] === 'ok', result) + assertEq(length(result[1]), 8n) + }, + readdirSkipsUndefinedEntry: () => { + // `Dir`'s index signature is optional (`{[name]?: _Entity}`), so an + // entry can legitimately be present with value `undefined` (e.g. after + // a rename leaves a stale key in some future refactor). readdir's loop + // must skip such entries rather than reporting them. + /** @type {Dir} */ + const root = { 'd': { 'a': undefined, 'b': [vec8(0x42n)] } } + const [, result] = virtual({ ...emptyState, root })(readdir('d', {})) + assert(result[0] === 'ok', result) + assertEq(result[1].length, 1) + }, + renameEmptySrc: () => { + // rename('', dst): src parses to the root path itself. + const [, result] = virtual(emptyState)(rename('', 'dst')) + assert(result[0] === 'error') + assertEq(result[1], 'cannot extract root') + }, + renameSrcThroughFile: () => { + // rename('a/b', dst) where 'a' is a file, not a directory: the + // intermediate segment can't be descended into. + /** @type {Dir} */ + const root = { 'a': [vec8(0x42n)] } + const [, result] = virtual({ ...emptyState, root })(rename('a/b', 'dst')) + assert(result[0] === 'error') + }, + renameSrcThreeLevelsMissing: () => { + // rename('a/b/c', dst) where 'a/b' exists but 'c' doesn't: the error + // from the deepest extractEntity call propagates through two levels + // of recursion. + /** @type {Dir} */ + const root = { 'a': { 'b': {} } } + const [, result] = virtual({ ...emptyState, root })(rename('a/b/c', 'dst')) + assert(result[0] === 'error') + }, + renameDstMissingIntermediate: () => { + // rename(src, 'missingdir/x'): the destination's parent doesn't exist. + /** @type {Dir} */ + const root = { 'src': [vec8(0x42n)] } + const [, result] = virtual({ ...emptyState, root })(rename('src', 'missingdir/x')) + assert(result[0] === 'error') + }, + renameDstThroughFile: () => { + // rename(src, 'blocker/x') where 'blocker' is a file, not a directory. + /** @type {Dir} */ + const root = { 'src': [vec8(0x42n)], 'blocker': [vec8(0x1n)] } + const [, result] = virtual({ ...emptyState, root })(rename('src', 'blocker/x')) + assert(result[0] === 'error') + assertEq(result[1], 'not a directory') + }, + renameDstNestedError: () => { + // rename(src, 'a/b/c') where 'a/b' is a file: insertEntityAt's error + // one level down propagates through the outer recursive call. + /** @type {Dir} */ + const root = { 'src': [vec8(0x1n)], 'a': { 'b': [vec8(0x2n)] } } + const [, result] = virtual({ ...emptyState, root })(rename('src', 'a/b/c')) + assert(result[0] === 'error') + assertEq(result[1], 'not a directory') + }, + createExclusiveNestedMissing: () => { + // createExclusive('a/b') where 'a' doesn't exist: the operation + // wrapper falls through with the full remaining path. Start from a + // non-empty root and check it survives untouched, so a mutant that + // returns the right error tag alongside a wiped dir would be caught. + /** @type {Dir} */ + const root = { keep: [vec8(0x1n)] } + const [state, result] = virtual({ ...emptyState, root })(createExclusive('a/b')) + assert(result[0] === 'error') + assertEq(Object.keys(state.root).length, 1) + }, + writeBytesNestedMissing: () => { + // writeBytes('a/b', ...) where 'a' doesn't exist. Non-empty root, as above. + /** @type {Dir} */ + const root = { keep: [vec8(0x1n)] } + const [state, result] = virtual({ ...emptyState, root })(writeBytes('a/b', 0, vec8(0x1n))) + assert(result[0] === 'error') + assertEq(Object.keys(state.root).length, 1) + }, + writeBytesMissingFile: () => { + // writeBytes on a path that doesn't exist at all: writeBytes never + // creates. Non-empty root, as above. + /** @type {Dir} */ + const root = { keep: [vec8(0x1n)] } + const [state, result] = virtual({ ...emptyState, root })(writeBytes('missing', 0, vec8(0x1n))) + assert(result[0] === 'error') + assertEq(Object.keys(state.root).length, 1) + }, + writeBytesOnJsModule: () => { + // writeBytes on a JsModule entry covers the `!Array.isArray(file)` + // branch (unlike readFile/readBytes, writeBytes has no separate throw + // for JsModule, so this is the reachable way to hit "not a file"). + /** @type {Dir} */ + const root = { 'a.f.ts': /** @type {JsModule} */ (() => ({})) } + const [, result] = virtual({ ...emptyState, root })(writeBytes('a.f.ts', 0, vec8(0x1n))) + assert(result[0] === 'error') + }, + writeBytesNegativeOffset: () => { + /** @type {Dir} */ + const root = { 'file': [vec8(0x1n)] } + const [, result] = virtual({ ...emptyState, root })(writeBytes('file', -1, vec8(0x2n))) + assert(result[0] === 'error') + assertEq(result[1], 'Offset -1 is invalid') + }, + statNestedMissing: () => { + // stat('a/b') where 'a' doesn't exist. + const [, result] = virtual(emptyState)(stat('a/b')) + assert(result[0] === 'error') + }, + statMissingFile: () => { + const [, result] = virtual(emptyState)(stat('missing')) + assert(result[0] === 'error') }, renameSamePath: () => { // rename('a', 'a') should succeed as a no-op, not reject