Handle non-standard p5/py5 exceptions

cypress/e2e/spec-wc-skulpt.cy.js

@@ -60,6 +60,29 @@ describe("Running the code with skulpt", () => {
     getP5Canvas().should("exist");
   });
 
+  it("shows an error and stops the sketch when a p5 preload raises", () => {
+    // preload/setup run before any canvas exists, so VisualOutputPane can't
+    // stop the sketch. The shim must stop it, otherwise p5 treats the callback
+    // as successful and carries on into setup/draw - here draw() would print,
+    // which is asserted never to happen.
+    runCode(
+      "from p5 import *\n\ndef preload():\n\traise ValueError('boom in preload')\ndef setup():\n\tsize(400, 400)\ndef draw():\n\tprint('draw ran')\nrun(frame_rate=2)",
+    );
+
+    getErrorMessage().should("contain.text", "ValueError: boom in preload");
+    getSkulptRunner().should("not.contain", "draw ran");
+  });
+
+  it("shows an error message when a p5 sketch raises in draw", () => {
+    // draw() runs every frame via requestAnimationFrame; the shim routes its
+    // errors to the editor rather than window.onerror.
+    runCode(
+      "from p5 import *\n\ndef setup():\n\tsize(400, 400)\ndef draw():\n\traise ValueError('boom in draw')\nrun(frame_rate=2)",
+    );
+
+    getErrorMessage().should("contain.text", "ValueError: boom in draw");
+  });
+
   it("runs a simple py5 program", () => {
     runCode(
       "import py5\ndef setup():\n\tpy5.size(400, 400)\ndef draw():\n\tpy5.background(255)\npy5.run_sketch()",

cypress/helpers/editor.js

@@ -93,7 +93,11 @@ const dragHandle = (getHandle, { deltaX = 0, deltaY = 0 }) => {
     const clientX = left + width / 2;
     const clientY = top + height / 2;
 
-    cy.wrap($handle).trigger("mousedown", { button: 0, clientX, clientY });
+    // Re-query the handle instead of reusing $handle. If the app re-renders
+    // while trigger() is waiting for the element to become actionable, a
+    // wrapped element is detached for good and trigger() burns its full
+    // timeout; a query chain retries against the current element.
+    getHandle().trigger("mousedown", { button: 0, clientX, clientY });
 
     getHandle()
       .parent("div")
@@ -125,6 +129,14 @@ export const openFilePanel = () =>
 
 export const loadPythonStarterProject = () => {
   cy.findByText("blank-python-starter").click();
+
+  // web-component.html fetches the project JSON, then throws away the current
+  // <editor-wc> and prepends a brand new one. The component being replaced
+  // already has a visible Run button, so waiting on that alone can resolve
+  // against the doomed component and leave the test holding elements that
+  // detach part-way through. Only the replacement publishes an identifier.
+  cy.get("#project-identifier").should("have.text", "blank-python-starter");
+
   getEditorShadow().findByRole("button", { name: /run/i }).should("be.visible");
 };
 

public/shims/processing/p5/p5-shim.js

@@ -1510,47 +1510,76 @@ const $builtinmodule = function (name) {
         Sk.builtin.str("py5_imported"),
       ].includes(mod.__name__);
 
+      // p5 runs preload/setup/draw asynchronously, so an uncaught exception
+      // there escapes to window.onerror. Catching it isn't enough on its own:
+      // p5 would treat the callback as successful and carry on into the draw
+      // loop, and VisualOutputPane only stops the sketch once a canvas exists -
+      // so a failure before then would keep looping. Surface the error and stop
+      // the loop here, and skip the remaining callbacks.
+      let sketchStopped = false;
+      const stopOnError = (e) => {
+        sketchStopped = true;
+        mod.pInst?.noLoop();
+        Sk.uncaughtException(e);
+      };
+
       sketch.preload = function () {
         if (Sk.globals["preload"] && !isPy5Version) {
-          Sk.misceval.callsimArray(Sk.globals["preload"]);
+          try {
+            Sk.misceval.callsimArray(Sk.globals["preload"]);
+          } catch (e) {
+            stopOnError(e);
+          }
         }
       };
 
       sketch.setup = function () {
-        if (Sk.globals["settings"] && isPy5Version) {
-          Sk.misceval.callsimArray(Sk.globals["settings"]);
+        if (sketchStopped) {
+          return;
         }
-        if (Sk.globals["setup"]) {
-          Sk.misceval.callsimArray(Sk.globals["setup"]);
-
-          for (const cb of Object.keys(callBacks)) {
-            if (Sk.globals[cb]) {
-              sketch[callBacks[cb]] = new Function(
-                "try {Sk.misceval.callsimArray(Sk.globals['" +
-                  cb +
-                  "']);} catch(e) {Sk.uncaughtException(e);}",
-              );
+        try {
+          if (Sk.globals["settings"] && isPy5Version) {
+            Sk.misceval.callsimArray(Sk.globals["settings"]);
+          }
+          if (Sk.globals["setup"]) {
+            Sk.misceval.callsimArray(Sk.globals["setup"]);
+
+            for (const cb of Object.keys(callBacks)) {
+              if (Sk.globals[cb]) {
+                sketch[callBacks[cb]] = new Function(
+                  "try {Sk.misceval.callsimArray(Sk.globals['" +
+                    cb +
+                    "']);} catch(e) {Sk.uncaughtException(e);}",
+                );
+              }
             }
           }
+        } catch (e) {
+          stopOnError(e);
         }
       };
 
       mod.pInst.frameRate(frame_rate.v);
 
       sketch.draw = function () {
-        mod.pInst.scale(scaleFactor, scaleFactor);
-        if (mod.__name__ === Sk.builtin.str("py5")) {
-          mod.frame_count = new Sk.builtin.int_(sketch.frameCount);
-        } else {
-          Sk.builtins.frame_count = new Sk.builtin.int_(sketch.frameCount);
+        if (sketchStopped) {
+          return;
         }
+        // Wrap the whole body (not just the user draw() call) - draw runs every
+        // frame and any exception would otherwise escape to window.onerror.
+        try {
+          mod.pInst.scale(scaleFactor, scaleFactor);
+          if (mod.__name__ === Sk.builtin.str("py5")) {
+            mod.frame_count = new Sk.builtin.int_(sketch.frameCount);
+          } else {
+            Sk.builtins.frame_count = new Sk.builtin.int_(sketch.frameCount);
+          }
 
-        if (Sk.globals["draw"]) {
-          try {
+          if (Sk.globals["draw"]) {
             Sk.misceval.callsimArray(Sk.globals["draw"]);
-          } catch (e) {
-            Sk.uncaughtException(e);
           }
+        } catch (e) {
+          stopOnError(e);
         }
       };
 

public/shims/processing/py5/py5-shim.js

@@ -1473,33 +1473,55 @@ const $builtinmodule = function (name) {
 
       mod.pInst = sketch;
 
+      // p5 runs settings/setup/draw asynchronously, so an uncaught exception
+      // there escapes to window.onerror. Catching it isn't enough on its own:
+      // p5 would treat the callback as successful and carry on into the draw
+      // loop, and VisualOutputPane only stops the sketch once a canvas exists -
+      // so a failure before then would keep looping. Surface the error and stop
+      // the loop here, and skip the remaining callbacks.
+      let sketchStopped = false;
+      const stopOnError = (e) => {
+        sketchStopped = true;
+        mod.pInst?.noLoop();
+        Sk.uncaughtException(e);
+      };
+
       sketch.setup = function () {
-        if (Sk.globals["settings"]) {
-          Sk.misceval.callsimArray(Sk.globals["settings"]);
-        }
-        if (Sk.globals["setup"]) {
-          Sk.misceval.callsimArray(Sk.globals["setup"]);
-
-          for (const cb of Object.keys(callBacks)) {
-            if (Sk.globals[cb]) {
-              sketch[callBacks[cb]] = new Function(
-                "try {Sk.misceval.callsimArray(Sk.globals['" +
-                  cb +
-                  "']);} catch(e) {Sk.uncaughtException(e);}",
-              );
+        try {
+          if (Sk.globals["settings"]) {
+            Sk.misceval.callsimArray(Sk.globals["settings"]);
+          }
+          if (Sk.globals["setup"]) {
+            Sk.misceval.callsimArray(Sk.globals["setup"]);
+
+            for (const cb of Object.keys(callBacks)) {
+              if (Sk.globals[cb]) {
+                sketch[callBacks[cb]] = new Function(
+                  "try {Sk.misceval.callsimArray(Sk.globals['" +
+                    cb +
+                    "']);} catch(e) {Sk.uncaughtException(e);}",
+                );
+              }
             }
           }
+        } catch (e) {
+          stopOnError(e);
         }
       };
 
       sketch.draw = function () {
-        mod.frame_count = new Sk.builtin.int_(sketch.frameCount);
-        if (Sk.globals["draw"]) {
-          try {
+        if (sketchStopped) {
+          return;
+        }
+        // Wrap the whole body (not just the user draw() call) - draw runs every
+        // frame and any exception would otherwise escape to window.onerror.
+        try {
+          mod.frame_count = new Sk.builtin.int_(sketch.frameCount);
+          if (Sk.globals["draw"]) {
             Sk.misceval.callsimArray(Sk.globals["draw"]);
-          } catch (e) {
-            Sk.uncaughtException(e);
           }
+        } catch (e) {
+          stopOnError(e);
         }
       };
 

src/components/Editor/Runners/PythonRunner/SkulptRunner/SkulptRunner.jsx

@@ -412,27 +412,33 @@ const SkulptRunner = ({
         message: errorMessage,
       };
     } else {
-      const errorDescription = (err.tp$str && err.tp$str().v)
+      // err is normally a Skulpt exception (tp$str/traceback), but native
+      // code called from Python (e.g. the p5/py5 shims) can throw plain JS
+      // errors instead, which don't have that shape - fall back gracefully
+      // rather than letting this handler itself throw.
+      const rawDescription =
+        typeof err.tp$str === "function" ? err.tp$str().v : undefined;
+      const errorDescription = (rawDescription || err.message || "")
         .replace(/\[(.*?)\]/, "")
         .replace(/\.$/, "");
-      const errorType = err.tp$name || err.constructor.name;
-      let lineNumber = err.traceback[0].lineno;
-      let fileName = err.traceback[0].filename;
+      const errorType = err.tp$name || err.constructor?.name;
+      let lineNumber = err.traceback?.[0]?.lineno;
+      let fileName = err.traceback?.[0]?.filename;
 
       // If this is an error in the sense_hat.py shim, use the next entry in
       // the traceback as this will be the line in the shim which we don't want
       // to show to users, so that the error message will instead point to the
       // line in the user's code which caused the error.
       if (
-        err.traceback.length > 1 &&
+        err.traceback?.length > 1 &&
         fileName === "./sense_hat.py" &&
         ["RuntimeError", "ValueError"].includes(errorType)
       ) {
         lineNumber = err.traceback[1].lineno;
         fileName = err.traceback[1].filename;
       }
 
-      fileName = fileName.replace(/^\.\//, "");
+      fileName = fileName ? fileName.replace(/^\.\//, "") : fileName;
 
       if (errorType === "ImportError" && window.crossOriginIsolated) {
         const articleLink = `https://help.editor.raspberrypi.org/hc/en-us/articles/30841379339924-What-Python-libraries-are-available-in-the-Code-Editor`;
@@ -447,7 +453,9 @@ const SkulptRunner = ({
 
       const { createError } = ApiCallHandler({ reactAppApiEndpoint });
 
-      errorMessage = `${errorType}: ${errorDescription} on line ${lineNumber} of ${fileName}${
+      const location =
+        lineNumber && fileName ? ` on line ${lineNumber} of ${fileName}` : "";
+      errorMessage = `${errorType}: ${errorDescription}${location}${
         explanation ? `. ${explanation}` : ""
       }`;
       createError(projectIdentifier, userId, {

src/components/Editor/Runners/PythonRunner/SkulptRunner/SkulptRunner.test.jsx

@@ -396,6 +396,77 @@ describe("When an error originates in the sense_hat shim", () => {
   });
 });
 
+describe("When native code called from Python throws a plain JS error (e.g. the p5/py5 shims), rather than a Skulpt exception", () => {
+  let store;
+  let asyncToPromiseSpy;
+
+  beforeEach(() => {
+    asyncToPromiseSpy = jest
+      .spyOn(Sk.misceval, "asyncToPromise")
+      .mockReturnValue(
+        Promise.reject(
+          new TypeError("Cannot read properties of undefined (reading 'foo')"),
+        ),
+      );
+
+    const middlewares = [];
+    const mockStore = configureMockStore(middlewares);
+    const initialState = {
+      editor: {
+        project: {
+          components: [
+            {
+              name: "main",
+              extension: "py",
+              content: "import py5",
+            },
+          ],
+          image_list: [],
+        },
+        codeRunTriggered: true,
+      },
+      auth: {
+        user,
+      },
+    };
+    store = mockStore(initialState);
+    render(
+      <Provider store={store}>
+        <SkulptRunner active={true} />
+      </Provider>,
+    );
+  });
+
+  afterEach(() => {
+    asyncToPromiseSpy.mockRestore();
+  });
+
+  test("falls back to the native error message instead of throwing", () => {
+    expect(store.getActions()).toEqual(
+      expect.arrayContaining([
+        setError(
+          "TypeError: Cannot read properties of undefined (reading 'foo')",
+        ),
+      ]),
+    );
+  });
+
+  test("sets errorDetails using the native error type and message", () => {
+    expect(store.getActions()).toEqual(
+      expect.arrayContaining([
+        setErrorDetails({
+          type: "TypeError",
+          line: undefined,
+          file: undefined,
+          description: "Cannot read properties of undefined (reading 'foo')",
+          message:
+            "TypeError: Cannot read properties of undefined (reading 'foo')",
+        }),
+      ]),
+    );
+  });
+});
+
 describe("When an error has occurred", () => {
   let mockStore;
   let store;