Update sketch verifier to check for redefinitions and print friendly messages

src/core/friendly_errors/friendly_errors_utils.js

@@ -0,0 +1,31 @@
+/**
+ * Loads and returns a mapping of p5 constructor names to their corresponding
+ * functions. This function iterates through all properties of the p5 object
+ * identifying constructor functions (those starting with a capital letter) and
+ * storing them in an object.
+ * @returns {Object} An object mapping p5 constructor names to their functions.
+ */
+function loadP5Constructors(p5) {
+  // Mapping names of p5 types to their constructor functions.
+  // p5Constructors:
+  //   - Color: f()
+  //   - Graphics: f()
+  //   - Vector: f()
+  // and so on.
+  const p5Constructors = {};
+
+  // Make a list of all p5 classes to be used for argument validation
+  // This must be done only when everything has loaded otherwise we get
+  // an empty array
+  for (let key of Object.keys(p5)) {
+    // Get a list of all constructors in p5. They are functions whose names
+    // start with a capital letter
+    if (typeof p5[key] === 'function' && key[0] !== key[0].toLowerCase()) {
+      p5Constructors[key] = p5[key];
+    }
+  }
+
+  return p5Constructors;
+}
+
+export { loadP5Constructors };

src/core/friendly_errors/sketch_verifier.js

@@ -1,11 +1,44 @@
-import * as acorn from 'acorn';
-import * as walk from 'acorn-walk';
+import { parse } from 'acorn';
+import { simple as walk } from 'acorn-walk';
+import * as constants from '../constants';
+import { loadP5Constructors } from './friendly_errors_utils';
 
 /**
  * @for p5
  * @requires core
  */
 function sketchVerifier(p5, fn) {
+  // List of functions to ignore as they either are meant to be re-defined or
+  // generate false positive outputs.
+  const ignoreFunction = [
+    'setup',
+    'draw',
+    'preload',
+    'deviceMoved',
+    'deviceTurned',
+    'deviceShaken',
+    'doubleClicked',
+    'mousePressed',
+    'mouseReleased',
+    'mouseMoved',
+    'mouseDragged',
+    'mouseClicked',
+    'mouseWheel',
+    'touchStarted',
+    'touchMoved',
+    'touchEnded',
+    'keyPressed',
+    'keyReleased',
+    'keyTyped',
+    'windowResized',
+    'name',
+    'parent',
+    'toString',
+    'print',
+    'stop',
+    'onended'
+  ];
+
   /**
    * Fetches the contents of a script element in the user's sketch.
    * 
@@ -66,13 +99,13 @@ function sketchVerifier(p5, fn) {
     const lineOffset = -1;
 
     try {
-      const ast = acorn.parse(code, {
+      const ast = parse(code, {
         ecmaVersion: 2021,
         sourceType: 'module',
         locations: true  // This helps us get the line number.
       });
 
-      walk.simple(ast, {
+      walk(ast, {
         VariableDeclarator(node) {
           if (node.id.type === 'Identifier') {
             const category = node.init && ['ArrowFunctionExpression', 'FunctionExpression'].includes(node.init.type)
@@ -111,11 +144,101 @@ function sketchVerifier(p5, fn) {
     return userDefinitions;
   }
 
+  /**
+   * Checks user-defined variables and functions for conflicts with p5.js
+   * constants and global functions.
+   * 
+   * This function performs two main checks:
+   * 1. Verifies if any user definition conflicts with p5.js constants.
+   * 2. Checks if any user definition conflicts with global functions from
+   * p5.js renderer classes.
+   * 
+   * If a conflict is found, it reports a friendly error message and halts
+   * further checking.
+   * 
+   * @param {Object} userDefinitions - An object containing user-defined variables and functions.
+   * @param {Array<{name: string, line: number}>} userDefinitions.variables - Array of user-defined variable names and their line numbers.
+   * @param {Array<{name: string, line: number}>} userDefinitions.functions - Array of user-defined function names and their line numbers.
+   * @returns {boolean} - Returns true if a conflict is found, false otherwise.
+   */
+  fn.checkForConstsAndFuncs = function (userDefinitions, p5Constructors) {
+    const allDefinitions = [
+      ...userDefinitions.variables,
+      ...userDefinitions.functions
+    ];
+
+    // Helper function that generates a friendly error message that contains
+    // the type of redefinition (constant or function), the name of the
+    // redefinition, the line number in user's code, and a link to its
+    // reference on the p5.js website.
+    function generateFriendlyError(errorType, name, line) {
+      const url = `https://p5js.org/reference/#/p5/${name}`;
+      const message = `${errorType} "${name}" on line ${line} is being redeclared and conflicts with a p5.js ${errorType.toLowerCase()}. JavaScript does not support declaring a ${errorType.toLowerCase()} more than once. p5.js reference: ${url}.`;
+      return message;
+    }
+
+    // Helper function that checks if a user definition has already been defined
+    // in the p5.js library, either as a constant or as a function.
+    function checkForRedefinition(name, libValue, line, type) {
+      try {
+        const userValue = eval("name");
+        if (libValue !== userValue) {
+          let message = generateFriendlyError(type, name, line);
+          console.log(message);
+          return true;
+        }
+      } catch (e) {
+        // If eval fails, the function hasn't been redefined
+        return false;
+      }
+      return false;
+    }
+
+    // Checks for constant redefinitions.
+    for (let { name, line } of allDefinitions) {
+      const libDefinition = constants[name];
+      if (libDefinition !== undefined) {
+        if (checkForRedefinition(name, libDefinition, line, "Constant")) {
+          return true;
+        }
+      }
+    }
+
+    // The new rules for attaching anything to global are (if true for both of
+    // the following):
+    //    - It is a member of p5.prototype
+    //   - Its name does not start with `_`
+    const globalFunctions = new Set(
+      Object.keys(p5.prototype).filter(key => !key.startsWith('_'))
+    );
+
+    for (let { name, line } of allDefinitions) {
+      if (!ignoreFunction.includes(name) && globalFunctions.has(name)) {
+        const prototypeFunc = p5.prototype[name];
+        if (prototypeFunc && checkForRedefinition(name, prototypeFunc, line, "Function")) {
+          return true;
+        }
+      }
+    }
+
+    return false;
+  }
+
   fn.run = async function () {
+    const p5Constructors = await new Promise(resolve => {
+      if (document.readyState === 'complete') {
+        resolve(loadP5Constructors(p5));
+      } else {
+        window.addEventListener('load', () => resolve(loadP5Constructors(p5)));
+      }
+    });
+
     const userCode = await fn.getUserCode();
     const userDefinedVariablesAndFuncs = fn.extractUserDefinedVariablesAndFuncs(userCode);
 
-    return userDefinedVariablesAndFuncs;
+    if (fn.checkForConstsAndFuncs(userDefinedVariablesAndFuncs, p5Constructors)) {
+      return;
+    }
   }
 }
 

test/unit/core/sketch_overrides.js

@@ -1,12 +1,21 @@
 import sketchVerifier from '../../../src/core/friendly_errors/sketch_verifier.js';
+import { loadP5Constructors } from '../../../src/core/friendly_errors/friendly_errors_utils.js';
 
-suite('Validate Params', function () {
+suite('Sketch Verifier', function () {
   const mockP5 = {
-    _validateParameters: vi.fn()
+    _validateParameters: vi.fn(),
+    Color: function () { },
+    Vector: function () { },
+    prototype: {
+      rect: function () { },
+      ellipse: function () { },
+    }
   };
   const mockP5Prototype = {};
+  let p5Constructors;
 
   beforeAll(function () {
+    p5Constructors = loadP5Constructors(mockP5);
     sketchVerifier(mockP5, mockP5Prototype);
   });
 
@@ -179,42 +188,149 @@ suite('Validate Params', function () {
     });
   });
 
+  suite('checkForConstsAndFuncs()', function () {
+    // Set up for this suite of tests
+    let consoleSpy;
+
+    beforeEach(function () {
+      consoleSpy = vi.spyOn(console, 'log').mockImplementation(() => { });
+    });
+
+    afterEach(function () {
+      consoleSpy.mockRestore();
+    });
+
+    test('Detects conflict with p5.js constant', function () {
+      const userDefinitions = {
+        variables: [{ name: 'PI', line: 1 }],
+        functions: []
+      };
+      const result = mockP5Prototype.checkForConstsAndFuncs(userDefinitions, p5Constructors);
+
+      expect(result).toBe(true);
+      expect(consoleSpy).toHaveBeenCalledWith(expect.stringContaining('Constant "PI" on line 1 is being redeclared and conflicts with a p5.js constant'));
+    });
+
+    test('Detects conflict with p5.js global function', function () {
+      const userDefinitions = {
+        variables: [],
+        functions: [{ name: 'rect', line: 2 }]
+      };
+      const result = mockP5Prototype.checkForConstsAndFuncs(userDefinitions, p5Constructors);
+
+      expect(result).toBe(true);
+      expect(consoleSpy).toHaveBeenCalledWith(expect.stringContaining('Function "rect" on line 2 is being redeclared and conflicts with a p5.js function'));
+    });
+
+    test('Allows redefinition of whitelisted functions', function () {
+      const userDefinitions = {
+        variables: [],
+        functions: [
+          { name: 'setup', line: 1 },
+          { name: 'draw', line: 2 },
+          { name: 'preload', line: 3 }
+        ]
+      };
+
+      const result = mockP5Prototype.checkForConstsAndFuncs(userDefinitions, p5Constructors);
+
+      expect(result).toBe(false);
+      expect(consoleSpy).not.toHaveBeenCalled();
+    });
+
+    test('Returns false when no conflicts are found', function () {
+      const userDefinitions = {
+        variables: [{ name: 'img', line: 1 }],
+        functions: [{ name: 'cut', line: 2 }]
+      };
+
+      const result = mockP5Prototype.checkForConstsAndFuncs(userDefinitions, p5Constructors);
+
+      expect(result).toBe(false);
+    });
+  });
+
   suite('run()', function () {
-    test('Returns extracted variables and functions', async function () {
+    let originalReadyState;
+
+    beforeEach(function () {
+      originalReadyState = document.readyState;
+      vi.spyOn(window, 'addEventListener');
+    });
+
+    afterEach(function () {
+      Object.defineProperty(document, 'readyState', { value: originalReadyState });
+      vi.restoreAllMocks();
+    });
+
+    test('Extracts user-defined variables and functions and checks for conflicts', async function () {
+      Object.defineProperty(document, 'readyState', { value: 'complete' });
       const mockScript = `
         let x = 5;
         const y = 10;
         function foo() {}
         const bar = () => {};
+        class MyClass {}
       `;
       mockP5Prototype.getUserCode = vi.fn(() => Promise.resolve(mockScript));
-
-      const result = await mockP5Prototype.run();
-      const expectedResult = {
-        "functions": [
-          {
-            "line": 3,
-            "name": "foo",
-          },
-          {
-            "line": 4,
-            "name": "bar",
-          },
+      mockP5Prototype.extractUserDefinedVariablesAndFuncs = vi.fn(() => ({
+        variables: [
+          { name: 'x', line: 1 },
+          { name: 'y', line: 2 },
+          { name: 'MyClass', line: 5 }
         ],
-        "variables": [
-          {
-            "line": 1,
-            "name": "x",
-          },
-          {
-            "line": 2,
-            "name": "y",
-          },
+        functions: [
+          { name: 'foo', line: 3 },
+          { name: 'bar', line: 4 }
+        ]
+      }));
+      mockP5Prototype.checkForConstsAndFuncs = vi.fn(() => false);
+
+      await mockP5Prototype.run();
+
+      expect(mockP5Prototype.getUserCode).toHaveBeenCalledTimes(1);
+      expect(mockP5Prototype.extractUserDefinedVariablesAndFuncs).toHaveBeenCalledWith(mockScript);
+      expect(mockP5Prototype.checkForConstsAndFuncs).toHaveBeenCalledWith({
+        variables: [
+          { name: 'x', line: 1 },
+          { name: 'y', line: 2 },
+          { name: 'MyClass', line: 5 }
         ],
-      };
+        functions: [
+          { name: 'foo', line: 3 },
+          { name: 'bar', line: 4 }
+        ]
+      },
+        expect.any(Object) // This is the p5Constructors object
+      );
+      expect(window.addEventListener).not.toHaveBeenCalled();
+    });
+
+    test('Stops execution when a conflict is found', async function () {
+      Object.defineProperty(document, 'readyState', { value: 'complete' });
+
+      const mockScript = `
+        let PI = 3.14;
+        function setup() {}
+      `;
+      mockP5Prototype.getUserCode = vi.fn(() => Promise.resolve(mockScript));
+      mockP5Prototype.extractUserDefinedVariablesAndFuncs = vi.fn(() => ({
+        variables: [{ name: 'PI', line: 1 }],
+        functions: [{ name: 'setup', line: 2 }]
+      }));
+      mockP5Prototype.checkForConstsAndFuncs = vi.fn(() => true);
+
+      const result = await mockP5Prototype.run();
 
       expect(mockP5Prototype.getUserCode).toHaveBeenCalledTimes(1);
-      expect(result).toEqual(expectedResult);
+      expect(mockP5Prototype.extractUserDefinedVariablesAndFuncs).toHaveBeenCalledWith(mockScript);
+      expect(mockP5Prototype.checkForConstsAndFuncs).toHaveBeenCalledWith(
+        {
+          variables: [{ name: 'PI', line: 1 }],
+          functions: [{ name: 'setup', line: 2 }]
+        },
+        expect.any(Object));  // This is the p5Constructors object
+      expect(result).toBeUndefined();
     });
   });
 });