Merge pull request #7291 from sproutleaf/dev-2.0

Final changes for parameter validation

Author: limzykenneth

package.json

@@ -27,8 +27,7 @@
     "gifenc": "^1.0.3",
     "libtess": "^1.2.2",
     "omggif": "^1.0.10",
-    "opentype.js": "^1.3.1",
-    "zod-validation-error": "^3.3.1"
+    "opentype.js": "^1.3.1"
   },
   "devDependencies": {
     "@rollup/plugin-commonjs": "^25.0.7",
@@ -83,4 +82,4 @@
       "pre-commit": "lint-staged"
     }
   }
-}
+}

src/core/friendly_errors/param_validator.js

@@ -5,7 +5,6 @@
 import p5 from '../main.js';
 import * as constants from '../constants.js';
 import { z } from 'zod';
-import { fromError } from 'zod-validation-error';
 import dataDoc from '../../../docs/parameterData.json';
 
 function validateParams(p5, fn) {
@@ -20,7 +19,7 @@ function validateParams(p5, fn) {
   // and so on.
   const p5Constructors = {};
 
-  fn._loadP5Constructors = function () {
+  fn.loadP5Constructors = function () {
     // 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
@@ -84,6 +83,17 @@ function validateParams(p5, fn) {
   // Add web API schemas to the schema map.
   Object.assign(schemaMap, webAPISchemas);
 
+  // For mapping 0-indexed parameters to their ordinal representation, e.g.
+  // "first" for 0, "second" for 1, "third" for 2, etc.
+  const ordinals = ["first", "second", "third", "fourth", "fifth", "sixth", "seventh", "eighth", "ninth", "tenth"];
+
+  function extractFuncNameAndClass(func) {
+    const ichDot = func.lastIndexOf('.');
+    const funcName = func.slice(ichDot + 1);
+    const funcClass = func.slice(0, ichDot !== -1 ? ichDot : 0) || 'p5';
+    return { funcName, funcClass };
+  }
+
   /**
    * This is a helper function that generates Zod schemas for a function based on
    * the parameter data from `docs/parameterData.json`.
@@ -102,21 +112,24 @@ function validateParams(p5, fn) {
    * Where each array in `overloads` represents a set of valid overloaded
    * parameters, and `?` is a shorthand for `Optional`.
    *
-   * TODO:
-   * - [ ] Support for p5 constructors
-   * - [ ] Support for more obscure types, such as `lerpPalette` and optional
-   * objects in `p5.Geometry.computeNormals()`
-   * (see https://github.com/processing/p5.js/pull/7186#discussion_r1724983249)
-   *
-   * @param {String} func - Name of the function.
+   * @method generateZodSchemasForFunc
+   * @param {String} func - Name of the function. Expect global functions like `sin` and class methods like `p5.Vector.add`
    * @returns {z.ZodSchema} Zod schema
    */
-  function generateZodSchemasForFunc(func) {
-    // Expect global functions like `sin` and class methods like `p5.Vector.add`
-    const ichDot = func.lastIndexOf('.');
-    const funcName = func.slice(ichDot + 1);
-    const funcClass = func.slice(0, ichDot !== -1 ? ichDot : 0) || 'p5';
+  fn.generateZodSchemasForFunc = function (func) {
+    // A special case for `p5.Color.paletteLerp`, which has an unusual and
+    // complicated function signature not shared by any other function in p5.
+    if (func === 'p5.Color.paletteLerp') {
+      return z.tuple([
+        z.array(z.tuple([
+          z.instanceof(p5.Color),
+          z.number()
+        ])),
+        z.number()
+      ]);
+    }
 
+    const { funcName, funcClass } = extractFuncNameAndClass(func);
     let funcInfo = dataDoc[funcClass][funcName];
 
     let overloads = [];
@@ -140,9 +153,7 @@ function validateParams(p5, fn) {
       }
       // All p5 objects start with `p5` in the documentation, i.e. `p5.Camera`.
       else if (baseType.startsWith('p5')) {
-        console.log('type', baseType);
         const className = baseType.substring(baseType.indexOf('.') + 1);
-        console.log('className', p5Constructors[className]);
         typeSchema = z.instanceof(p5Constructors[className]);
       }
       // For primitive types and web API objects.
@@ -230,34 +241,6 @@ function validateParams(p5, fn) {
       : z.union(overloadSchemas);
   }
 
-  /**
-   * This is a helper function to print out the Zod schema in a readable format.
-   * This is for debugging purposes only and will be removed in the future.
-   *
-   * @param {z.ZodSchema} schema - Zod schema.
-   * @param {number} indent - Indentation level.
-   */
-  function printZodSchema(schema, indent = 0) {
-    const i = ' '.repeat(indent);
-    const log = msg => console.log(`${i}${msg}`);
-
-    if (schema instanceof z.ZodUnion || schema instanceof z.ZodTuple) {
-      const type = schema instanceof z.ZodUnion ? 'Union' : 'Tuple';
-      log(`${type}: [`);
-
-      const items = schema instanceof z.ZodUnion
-        ? schema._def.options
-        : schema.items;
-      items.forEach((item, index) => {
-        log(`  ${type === 'Union' ? 'Option' : 'Item'} ${index + 1}:`);
-        printZodSchema(item, indent + 4);
-      });
-      log(']');
-    } else {
-      log(schema.constructor.name);
-    }
-  }
-
   /**
    * Finds the closest schema to the input arguments.
    *
@@ -269,22 +252,45 @@ function validateParams(p5, fn) {
    * @param {Array} args - User input arguments.
    * @returns {z.ZodSchema} Closest schema matching the input arguments.
    */
-  function findClosestSchema(schema, args) {
+  fn.findClosestSchema = function (schema, args) {
     if (!(schema instanceof z.ZodUnion)) {
       return schema;
     }
 
     // Helper function that scores how close the input arguments are to a schema.
     // Lower score means closer match.
     const scoreSchema = schema => {
+      let score = Infinity;
       if (!(schema instanceof z.ZodTuple)) {
         console.warn('Schema below is not a tuple: ');
         printZodSchema(schema);
-        return Infinity;
+        return score;
       }
 
+      const numArgs = args.length;
       const schemaItems = schema.items;
-      let score = Math.abs(schemaItems.length - args.length) * 2;
+      const numSchemaItems = schemaItems.length;
+      const numRequiredSchemaItems = schemaItems.filter(item => !item.isOptional()).length;
+
+      if (numArgs >= numRequiredSchemaItems && numArgs <= numSchemaItems) {
+        score = 0;
+      }
+      // Here, give more weight to mismatch in number of arguments.
+      //
+      // For example, color() can either take [Number, Number?] or
+      // [Number, Number, Number, Number?] as list of parameters.
+      // If the user passed in 3 arguments, [10, undefined, undefined], it's
+      // more than likely that they intended to pass in 3 arguments, but the
+      // last two arguments are invalid.
+      //
+      // If there's no bias towards matching the number of arguments, the error
+      // message will show that we're expecting at most 2 arguments, but more
+      // are received.
+      else {
+        score = Math.abs(
+          numArgs < numRequiredSchemaItems ? numRequiredSchemaItems - numArgs : numArgs - numSchemaItems
+        ) * 4;
+      }
 
       for (let i = 0; i < Math.min(schemaItems.length, args.length); i++) {
         const paramSchema = schemaItems[i];
@@ -313,6 +319,118 @@ function validateParams(p5, fn) {
     return closestSchema;
   }
 
+  /**
+   * Prints a friendly error message after parameter validation, if validation
+   * has failed.
+   * 
+   * @method _friendlyParamError
+   * @private
+   * @param {z.ZodError} zodErrorObj - The Zod error object containing validation errors.
+   * @param {String} func - Name of the function. Expect global functions like `sin` and class methods like `p5.Vector.add`
+   * @returns {String} The friendly error message.
+   */
+  fn.friendlyParamError = function (zodErrorObj, func) {
+    let message;
+    // The `zodErrorObj` might contain multiple errors of equal importance
+    // (after scoring the schema closeness in `findClosestSchema`). Here, we
+    // always print the first error so that user can work through the errors
+    // one by one.
+    let currentError = zodErrorObj.errors[0];
+
+    // Helper function to build a type mismatch message.
+    const buildTypeMismatchMessage = (actualType, expectedTypeStr, position) => {
+      const positionStr = position ? `at the ${ordinals[position]} parameter` : '';
+      const actualTypeStr = actualType ? `, but received ${actualType}` : '';
+      return `Expected ${expectedTypeStr} ${positionStr}${actualTypeStr}`;
+    }
+
+    // Union errors occur when a parameter can be of multiple types but is not
+    // of any of them. In this case, aggregate all possible types and print
+    // a friendly error message that indicates what the expected types are at
+    // which position (position is not 0-indexed, for accessibility reasons).
+    const processUnionError = (error) => {
+      const expectedTypes = new Set();
+      let actualType;
+
+      error.unionErrors.forEach(err => {
+        const issue = err.issues[0];
+        if (issue) {
+          if (!actualType) {
+            actualType = issue.received;
+          }
+
+          if (issue.code === 'invalid_type') {
+            expectedTypes.add(issue.expected);
+          }
+          // The case for constants. Since we don't want to print out the actual
+          // constant values in the error message, the error message will
+          // direct users to the documentation.
+          else if (issue.code === 'invalid_literal') {
+            expectedTypes.add("constant (please refer to documentation for allowed values)");
+          } else if (issue.code === 'custom') {
+            const match = issue.message.match(/Input not instance of (\w+)/);
+            if (match) expectedTypes.add(match[1]);
+          }
+        }
+      });
+
+      if (expectedTypes.size > 0) {
+        const expectedTypesStr = Array.from(expectedTypes).join(' or ');
+        const position = error.path.join('.');
+
+        message = buildTypeMismatchMessage(actualType, expectedTypesStr, position);
+      }
+
+      return message;
+    }
+
+    switch (currentError.code) {
+      case 'invalid_union': {
+        processUnionError(currentError);
+        break;
+      }
+      case 'too_small': {
+        const minArgs = currentError.minimum;
+        message = `Expected at least ${minArgs} argument${minArgs > 1 ? 's' : ''}, but received fewer`;
+        break;
+      }
+      case 'invalid_type': {
+        message = buildTypeMismatchMessage(currentError.received, currentError.expected, currentError.path.join('.'));
+        break;
+      }
+      case 'too_big': {
+        const maxArgs = currentError.maximum;
+        message = `Expected at most ${maxArgs} argument${maxArgs > 1 ? 's' : ''}, but received more`;
+        break;
+      }
+      default: {
+        console.log('Zod error object', currentError);
+      }
+    }
+
+    // Let the user know which function is generating the error.
+    message += ` in ${func}().`;
+
+    // Generates a link to the documentation based on the given function name.
+    // TODO: Check if the link is reachable before appending it to the error
+    // message. 
+    const generateDocumentationLink = (func) => {
+      const { funcName, funcClass } = extractFuncNameAndClass(func);
+      const p5BaseUrl = 'https://p5js.org/reference';
+      const url = `${p5BaseUrl}/${funcClass}/${funcName}`;
+
+      return url;
+    }
+
+    if (currentError.code === 'too_big' || currentError.code === 'too_small') {
+      const documentationLink = generateDocumentationLink(func);
+      message += ` For more information, see ${documentationLink}.`;
+    }
+
+    console.log(message);
+    return message;
+  }
+
   /**
    * Runs parameter validation by matching the input parameters to Zod schemas
    * generated from the parameter data from `docs/parameterData.json`.
@@ -322,9 +440,9 @@ function validateParams(p5, fn) {
    * @returns {Object} The validation result.
    * @returns {Boolean} result.success - Whether the validation was successful.
    * @returns {any} [result.data] - The parsed data if validation was successful.
-   * @returns {import('zod-validation-error').ZodValidationError} [result.error] - The validation error if validation failed.
+   * @returns {String} [result.error] - The validation error message if validation has failed.
    */
-  fn._validateParams = function (func, args) {
+  fn.validate = function (func, args) {
     if (p5.disableFriendlyErrors) {
       return; // skip FES
     }
@@ -334,18 +452,17 @@ function validateParams(p5, fn) {
     // user intended to call the function with non-undefined arguments. Skip
     // regular workflow and return a friendly error message right away.
     if (Array.isArray(args) && args.every(arg => arg === undefined)) {
-      const undefinedError = new Error(`All arguments for function ${func} are undefined. There is likely an error in the code.`);
-      const zodUndefinedError = fromError(undefinedError);
+      const undefinedErrorMessage = `All arguments for ${func}() are undefined. There is likely an error in the code.`;
 
       return {
         success: false,
-        error: zodUndefinedError
+        error: undefinedErrorMessage
       };
     }
 
     let funcSchemas = schemaRegistry.get(func);
     if (!funcSchemas) {
-      funcSchemas = generateZodSchemasForFunc(func);
+      funcSchemas = fn.generateZodSchemasForFunc(func);
       schemaRegistry.set(func, funcSchemas);
     }
 
@@ -355,12 +472,13 @@ function validateParams(p5, fn) {
         data: funcSchemas.parse(args)
       };
     } catch (error) {
-      const closestSchema = findClosestSchema(funcSchemas, args);
-      const validationError = fromError(closestSchema.safeParse(args).error);
+      const closestSchema = fn.findClosestSchema(funcSchemas, args);
+      const zodError = closestSchema.safeParse(args).error;
+      const errorMessage = fn.friendlyParamError(zodError, func);
 
       return {
         success: false,
-        error: validationError
+        error: errorMessage
       };
     }
   };
@@ -370,5 +488,5 @@ export default validateParams;
 
 if (typeof p5 !== 'undefined') {
   validateParams(p5, p5.prototype);
-  p5.prototype._loadP5Constructors();
+  p5.prototype.loadP5Constructors();
 }

test/js/chai_helpers.js

@@ -1,5 +1,4 @@
 import p5 from '../../src/app.js';
-import { ValidationError } from 'zod-validation-error';
 
 // Setup chai
 var expect = chai.expect;
@@ -12,7 +11,7 @@ assert.arrayApproximately = function (arr1, arr2, delta, desc) {
   }
 }
 
-assert.deepCloseTo = function(actual, expected, digits = 4) {
+assert.deepCloseTo = function (actual, expected, digits = 4) {
   expect(actual.length).toBe(expected.length);
   for (let i = 0; i < actual.length; i++) {
     expect(actual[i]).withContext(`[${i}]`).toBeCloseTo(expected[i], digits);
@@ -27,14 +26,4 @@ assert.validationError = function (fn) {
   } else {
     assert.doesNotThrow(fn, Error, 'got unwanted exception');
   }
-};
-
-// A custom assertion for validation results for the new parameter validation
-// system.
-assert.validationResult = function (result, expectSuccess) {
-  if (expectSuccess) {
-    assert.isTrue(result.success);
-  } else {
-    assert.instanceOf(result.error, ValidationError);
-  }
-};
+};