evalCode(type=module) throws instead of returning rejected promise (bellard#161)

Author: justjake

c/interface.c

@@ -736,9 +736,12 @@ MaybeAsync(JSValue *) QTS_Eval(JSContext *ctx, BorrowedHeapChar *js_code, size_t
     JS_FreeValue(ctx, eval_result);
     return jsvalue_to_heap(JS_GetModuleNamespace(ctx, module));
   } else if (state == JS_PROMISE_REJECTED) {
-    QTS_DEBUG("QTS_Eval: result: JS_PROMISE_REJECTED")
-    // Module failed, just return the rejected result.
-    return jsvalue_to_heap(eval_result);
+    // Throw the rejection response, to match evaluation of non-module code,
+    // or of code with a syntax error.
+    QTS_DEBUG("QTS_Eval: result: JS_PROMISE_REJECTED - throw rejection reason")
+    JS_Throw(ctx, JS_PromiseResult(ctx, eval_result));
+    JS_FreeValue(ctx, eval_result);
+    return jsvalue_to_heap(JS_EXCEPTION);
   } else if (state == JS_PROMISE_PENDING) {
     QTS_DEBUG("QTS_Eval: result: JS_PROMISE_PENDING")
     // return moduleDone.then(() => module_namespace)
@@ -783,7 +786,6 @@ JSValue *QTS_GetModuleNamespace(JSContext *ctx, JSValueConst *module_func_obj) {
 
 OwnedHeapChar *QTS_Typeof(JSContext *ctx, JSValueConst *value) {
   const char *result = "unknown";
-  uint32_t tag = JS_VALUE_GET_TAG(*value);
 
   if (JS_IsNumber(*value)) {
     result = "number";

packages/quickjs-emscripten-core/src/context.ts

@@ -1,15 +1,15 @@
-import {
-  type EitherModule,
-  type EvalDetectModule,
-  type EvalFlags,
-  type JSBorrowedCharPointer,
-  type JSContextPointer,
-  type JSRuntimePointer,
-  type JSValueConstPointer,
-  type JSValuePointer,
-  type JSValuePointerPointer,
-  type EitherFFI,
-  JSPromiseStateEnum,
+import { JSPromiseStateEnum } from "@jitl/quickjs-ffi-types"
+import type {
+  EvalFlags,
+  EitherModule,
+  EvalDetectModule,
+  JSBorrowedCharPointer,
+  JSContextPointer,
+  JSRuntimePointer,
+  JSValueConstPointer,
+  JSValuePointer,
+  JSValuePointerPointer,
+  EitherFFI,
 } from "@jitl/quickjs-ffi-types"
 import { debugLog } from "./debug"
 import type { JSPromiseState } from "./deferred-promise"
@@ -966,9 +966,10 @@ export class QuickJSContext
 
   /**
    * Dump a JSValue to Javascript in a best-effort fashion.
+   * If the value is a promise, dumps the promise's state.
    * Returns `handle.toString()` if it cannot be serialized to JSON.
    */
-  dump(handle: QuickJSHandle) {
+  dump(handle: QuickJSHandle): any {
     this.runtime.assertOwned(handle)
     const type = this.typeof(handle)
     if (type === "string") {
@@ -983,6 +984,20 @@ export class QuickJSContext
       return this.getSymbol(handle)
     }
 
+    // It's confusing if we dump(promise) and just get back {} because promise
+    // has no properties, so dump promise state.
+    const asPromiseState = this.getPromiseState(handle)
+    if (asPromiseState.type === "fulfilled" && !asPromiseState.notAPromise) {
+      handle.dispose()
+      return { type: asPromiseState.type, value: asPromiseState.value.consume(this.dump) }
+    } else if (asPromiseState.type === "pending") {
+      handle.dispose()
+      return { type: asPromiseState.type }
+    } else if (asPromiseState.type === "rejected") {
+      handle.dispose()
+      return { type: asPromiseState.type, error: asPromiseState.error.consume(this.dump) }
+    }
+
     const str = this.memory.consumeJSCharPointer(this.ffi.QTS_Dump(this.ctx.value, handle.value))
     try {
       return JSON.parse(str)

packages/quickjs-emscripten/src/quickjs.test.ts

@@ -4,6 +4,7 @@
 
 import assert from "assert"
 import fs from "fs"
+import { inspect as baseInspect } from "node:util"
 import { describe, beforeEach, afterEach, afterAll as after, it, expect } from "vitest"
 import type {
   QuickJSHandle,
@@ -401,7 +402,7 @@ function contextTests(getContext: GetTestContext, isDebug = false) {
       value.dispose()
     })
 
-    it("on failure: returns { error: exception }", () => {
+    it("on syntax error: returns { error: exception }", () => {
       const result = vm.evalCode(`["this", "should", "fail].join(' ')`)
       if (!result.error) {
         assert.fail("result should be an error")
@@ -413,6 +414,35 @@ function contextTests(getContext: GetTestContext, isDebug = false) {
       result.error.dispose()
     })
 
+    it("module: on syntax error: returns { error: exception }", () => {
+      const result = vm.evalCode(`["this", "should", "fail].join(' ')`, "mod.js", {
+        type: "module",
+      })
+      if (!result.error) {
+        assert.fail("result should be an error")
+      }
+      assertError(vm.dump(result.error), {
+        name: "SyntaxError",
+        message: "unexpected end of string",
+      })
+      result.error.dispose()
+    })
+
+    it("module: on TypeError: returns { error: exception }", () => {
+      const result = vm.evalCode(`null.prop`, "mod.js", { type: "module" })
+      if (!result.error) {
+        result.value.consume((val) =>
+          assert.fail(`result should be an error, instead is: ${inspect(vm.dump(val))}`),
+        )
+        return
+      }
+      const error = result.error.consume(vm.dump)
+      assertError(error, {
+        name: "TypeError",
+        message: "cannot read property 'prop' of null",
+      })
+    })
+
     it("runs in the global context", () => {
       vm.unwrapResult(vm.evalCode(`var declaredWithEval = 'Nice!'`)).dispose()
       const declaredWithEval = vm.getProp(vm.global, "declaredWithEval")
@@ -1377,3 +1407,10 @@ applyVitestHack(QuickJSWASMModule, () => ({
   type: "QuickJSWASMModule",
   __vitest__: true,
 }))
+
+const inspect = (val: unknown) => {
+  return baseInspect(val, {
+    depth: 30,
+    colors: true,
+  })
+}