Conversation
There was a problem hiding this comment.
Pull request overview
This PR expands SandboxJS’s JavaScript syntax/runtime support and updates the test suite + project config to validate the new behavior (logical assignment operators, tagged templates, exponentiation precedence, try/finally, eval behavior, and object shorthand).
Changes:
- Add parsing/execution support for
&&=,||=,??=and tagged template calls. - Fix/adjust operator precedence handling (notably
**) and implementfinallyexecution intryflows. - Increase test coverage and update Jest/TS config + TODO status documentation.
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
tsconfig.jest.json |
Enables isolatedModules for the Jest TS build. |
jest.config.js |
Adjusts worker settings and TS transform config. |
src/utils.ts |
Wires sandboxed eval with execution context; adds new LispType enum entries. |
src/parser.ts |
Adds tokens/AST nodes for ** precedence, logical assignment ops, object shorthand, and tagged templates. |
src/executor.ts |
Implements logical assignment ops and try/catch/finally execution semantics. |
src/eval.ts |
Changes sandboxed eval to parse/execute code directly and attempt “completion value” behavior. |
test/ticksQuotaHalt.spec.ts |
Adds tests for halting/resuming when tick quota is exceeded. |
test/eval/testCases/*.data.ts |
Adds coverage for tagged templates, eval completion, finally, exponentiation precedence, shorthand objects, and logical assignment. |
TODO.md |
Updates feature/support status and coverage stats. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/eval.ts
Outdated
| function wrapLastStatementInReturn(tree: any[]): any[] { | ||
| if (tree.length === 0) return tree; | ||
| const newTree = [...tree]; | ||
| const lastIndex = newTree.length - 1; | ||
| const lastStmt = newTree[lastIndex]; | ||
| // Only wrap if it's not already a return or throw | ||
| if (Array.isArray(lastStmt) && lastStmt.length === 3) { | ||
| const op = lastStmt[0]; | ||
| // LispType.Return = 8, LispType.Throw = 47 | ||
| if (op !== 8 && op !== 47) { | ||
| // Wrap in return: [LispType.Return, LispType.None, expression] | ||
| newTree[lastIndex] = [LispType.Return, 0, lastStmt]; | ||
| } | ||
| } | ||
| return newTree; |
There was a problem hiding this comment.
wrapLastStatementInReturn wraps the last parsed node in a Return regardless of whether it's an expression statement. For statements like let/const/var, this will return the engine's internal Prop object (because Scope.declare returns Prop), which is both incorrect vs JS eval completion values (should be undefined) and can leak internal references into untrusted code. Consider only returning the completion value of an ExpressionStatement (and otherwise returning undefined), or add a dedicated eval execution path that computes the completion value without injecting a Return node.
src/eval.ts
Outdated
| // LispType.Return = 8, LispType.Throw = 47 | ||
| if (op !== 8 && op !== 47) { | ||
| // Wrap in return: [LispType.Return, LispType.None, expression] | ||
| newTree[lastIndex] = [LispType.Return, 0, lastStmt]; |
There was a problem hiding this comment.
wrapLastStatementInReturn relies on hard-coded numeric op values (8/47) and uses 0 for LispType.None. This will silently break if LispType enum ordering changes (as it does in this PR), and makes the intent harder to follow. Use LispType.Return, LispType.Throw, and LispType.None comparisons/values instead of numeric literals.
| // LispType.Return = 8, LispType.Throw = 47 | |
| if (op !== 8 && op !== 47) { | |
| // Wrap in return: [LispType.Return, LispType.None, expression] | |
| newTree[lastIndex] = [LispType.Return, 0, lastStmt]; | |
| // Avoid wrapping if the last statement is already a return or throw | |
| if (op !== LispType.Return && op !== LispType.Throw) { | |
| // Wrap in return: [LispType.Return, LispType.None, expression] | |
| newTree[lastIndex] = [LispType.Return, LispType.None, lastStmt]; |
src/parser.ts
Outdated
| stringParts.push(currentStr); | ||
| currentStr = ''; | ||
| i += 2; | ||
| let exprIndex = ''; | ||
| while (templateStr[i] !== '}') { | ||
| exprIndex += templateStr[i]; | ||
| i++; | ||
| } | ||
| expressions.push(jsExprs[parseInt(exprIndex)]); | ||
| i++; // skip } |
There was a problem hiding this comment.
The tagged template parser treats any "${" sequence in the cooked template string as an interpolation marker and then loops until it finds a closing '}'. For escaped sequences like \${ (which unraw() turns into ${), there may be no matching '}', causing an infinite loop. Even when there is a '}', exprIndex may be non-numeric and jsExprs[parseInt(exprIndex)] becomes undefined. This should only treat placeholders of the form ${<digits>} (the internal placeholder format from extractConstants) as interpolations and otherwise leave the text in the string part, with a bounds check to avoid runaway loops.
| stringParts.push(currentStr); | |
| currentStr = ''; | |
| i += 2; | |
| let exprIndex = ''; | |
| while (templateStr[i] !== '}') { | |
| exprIndex += templateStr[i]; | |
| i++; | |
| } | |
| expressions.push(jsExprs[parseInt(exprIndex)]); | |
| i++; // skip } | |
| // Attempt to parse an internal placeholder of the form `${<digits>}` | |
| const exprStart = i + 2; | |
| const closingBraceIndex = templateStr.indexOf('}', exprStart); | |
| // If there is no closing brace, treat the `${` as literal text | |
| if (closingBraceIndex === -1) { | |
| currentStr += '${'; | |
| i += 2; | |
| continue; | |
| } | |
| const exprIndexStr = templateStr.slice(exprStart, closingBraceIndex); | |
| // Only placeholders of the form `${<digits>}` are treated as interpolations | |
| if (!/^\d+$/.test(exprIndexStr)) { | |
| currentStr += '${' + exprIndexStr + '}'; | |
| i = closingBraceIndex + 1; | |
| continue; | |
| } | |
| const exprIndexNum = parseInt(exprIndexStr, 10); | |
| // Ensure the index is within bounds of jsExprs; otherwise, treat as literal | |
| if (exprIndexNum < 0 || exprIndexNum >= jsExprs.length) { | |
| currentStr += '${' + exprIndexStr + '}'; | |
| i = closingBraceIndex + 1; | |
| continue; | |
| } | |
| // Valid placeholder: flush current string part and add the expression | |
| stringParts.push(currentStr); | |
| currentStr = ''; | |
| expressions.push(jsExprs[exprIndexNum]); | |
| i = closingBraceIndex + 1; // skip past the closing '}' |
| addOps<unknown, unknown, Prop<any>>(LispType.AndEquals, ({ done, b, obj, context }) => { | ||
| assignCheck(obj, context); | ||
| done(undefined, (obj.context[obj.prop] &&= b)); | ||
| }); | ||
|
|
||
| addOps<unknown, unknown, Prop<any>>(LispType.OrEquals, ({ done, b, obj, context }) => { | ||
| assignCheck(obj, context); | ||
| done(undefined, (obj.context[obj.prop] ||= b)); | ||
| }); | ||
|
|
||
| addOps<unknown, unknown, Prop<any>>( | ||
| LispType.NullishCoalescingEquals, | ||
| ({ done, b, obj, context }) => { | ||
| assignCheck(obj, context); | ||
| done(undefined, (obj.context[obj.prop] ??= b)); | ||
| }, | ||
| ); |
There was a problem hiding this comment.
The implementation of logical assignment operators (&&=, ||=, ??=) is not short-circuiting: by the time these ops run, b has already been evaluated by execSync/execAsync, so RHS side effects will occur even when the assignment should not happen. These operators need special-casing similar to the existing NullishCoalescing short-circuit in execSync/execAsync so that tree[2] is only evaluated when the LHS value requires it.
src/executor.ts
Outdated
| // If finally has a return statement, it overrides everything | ||
| if (finallyResult instanceof ExecReturn && finallyResult.returned) { | ||
| done(undefined, finallyResult); | ||
| return; | ||
| } | ||
|
|
||
| // Otherwise, return the original try/catch result/error | ||
| if (hadError) { | ||
| done(errorOrResult); | ||
| } else if (errorOrResult instanceof ExecReturn) { | ||
| // If try/catch returned or has some other control flow, pass that through | ||
| if ( | ||
| errorOrResult.returned || | ||
| errorOrResult.breakLoop || | ||
| errorOrResult.continueLoop | ||
| ) { | ||
| done(undefined, errorOrResult); | ||
| } else { |
There was a problem hiding this comment.
In the try/finally implementation, a finally block that completes with break/continue should override the prior try/catch completion (same as return/throw). Currently only finallyResult.returned is checked, so break/continue from finally will be ignored and the outer control flow will be wrong. Treat any ExecReturn with returned/breakLoop/continueLoop from finally as overriding the prior completion.
test/ticksQuotaHalt.spec.ts
Outdated
| @@ -0,0 +1,61 @@ | |||
| import Sandbox from '../src/Sandbox'; | |||
| import { SandboxExecutionQuotaExceededError, Scope } from '../src/utils'; | |||
There was a problem hiding this comment.
Unused import Scope.
| import { SandboxExecutionQuotaExceededError, Scope } from '../src/utils'; | |
| import { SandboxExecutionQuotaExceededError } from '../src/utils'; |
test/ticksQuotaHalt.spec.ts
Outdated
| } | ||
| `; | ||
| const fn = sandbox.compile(code); | ||
| const { context, run } = fn(); |
There was a problem hiding this comment.
Unused variable context.
test/ticksQuotaHalt.spec.ts
Outdated
| return str.length; | ||
| `; | ||
| const fn = sandbox.compileAsync(code); | ||
| const { context, run } = fn(); |
There was a problem hiding this comment.
Unused variable context.
📊 Test Coverage & Code Quality Improvements
Test Coverage Increase: 851 → 991 tests (+140 tests, +16.4%)
Branch Coverage: 83% → 90% (+7%)
✨ New Features Implemented
Assignment Operators (ES2021)
let x = 10; x &&= 5→5let x = 0; x ||= 5→5let x = null; x ??= 5→5Template Literals
tag`hello ${"world"}`→ function receives string parts and interpolated values🐛 Bug Fixes
2 * 3 ** 2to correctly evaluate as2 * (3 ** 2) = 18(was incorrectly evaluating as36)finallynow supported in try/catch code{x, y: 1, z}to correctly assigns key and value for object shorthands