[2.0]: Properties of objects aren't being checked correctly by FES

[davepagurek]

Most appropriate sub-area of p5.js?

• Accessibility
• Color
• Core/Environment/Rendering
• Data
• DOM
• Events
• Image
• IO
• Math
• Typography
• Utilities
• WebGL
• Build process
• Unit testing
• Internationalization
• Friendly errors
• Other (specify if possible)

p5.js version

2.0.0

Web browser and version

Firefox

Operating system

MacOS

Steps to reproduce this

Steps:

If you run npm run docs and then check docs/parameterData.json, if you search for textToModel, it currently has these parameters:

"textToModel": {
      "overloads": [
        [
          "String",
          "Number",
          "Number",
          "Number",
          "Number",
          "Object?",
          "Number?",
          "Number?"
        ]
      ]
    }

This is because we're documenting the properties of the object like this:

* @param {String} str The text string to convert into a 3D model.
* @param {Number} x The x-coordinate for the starting position of the text.
* @param {Number} y The y-coordinate for the starting position of the text.
* @param {Number} width Maximum width of the text block (wraps text if exceeded).
* @param {Number} height Maximum height of the text block.
* @param {Object} [options] Configuration options for the 3D text:
* @param {Number} [options.extrude=0] The depth to extrude the text. A value of 0 produces
* flat text; higher values create thicker, 3D models.
* @param {Number} [options.sampleFactor=1] A factor controlling the level of detail for the text contours.
*  Higher values result in smoother curves.

It looks like we're mistakenly parsing options.extrude and options.sampleFactor as separate parameters, and not properties of options.

This likely needs a change in utils/convert.js.

[IIITM-Jay]

Hi @davepagurek ,

I looked at the code to find out how the parsing logic is being written and I found that the function getParams(entry) is indeed not filtering out nested parameters like options.extrude, which should be part of options, not separate entries.

The issue lies in the line here:

return (entry.tags || []).filter(t => t.title === 'param').map(node => {
    const param = entry.params.find(param => param.name === node.name);
    ...
  });

A very heuristic approach would be like,

.filter(t => t.title === 'param' && !t.name.includes('.'))

Of course, it will work, but then it is not based on structure, it will just be based on assumptions of how naming is being done. What I meant is, it will become a bold statement as - "If the parameter name contains a (.) (like options.extrude), we assume it’s nested, so skip it."

Instead, we can use, a set to build all the top-level parameter names defined in entry.params. This will allow fast lookups when checking if a @param tag name is a top-level parameter in constant-time O(1) using .has(...) check.

const signatureParamNames = new Set((entry.params || []).map(param => param.name));

and then

.filter(t => t.title === 'param' && signatureParamNames.has(t.name))
 .map(node => {
      const param = (entry.params || []).find(param => param.name === node.name);

I have also observed two other notable things in the existing function:

1. The entry.params is checked without fallback:

const param = entry.params.find(param => param.name === node.name);

We should safely access entry.params in case it's missing (i.e., undefined). If entry.params doesn’t exist (e.g., incomplete doc output), using entry.params.find(...) without fallback would throw a runtime error. Fallback to [] ensures find(...) doesn’t break the loop.

2. The if - else block as below could be optimized using nullish fallback pattern (||) with optional chaining (?.) as they doesn't have that much differences and are similar too.

if (param) {
      return {
        ...node,
        description: param.description
      };
    } else {
      return {
        ...node,
        description: {
          type: 'html',
          value: node.description
        }
      };
    }

optimized one:

return {
        ...node,
        description: param?.description || {
          type: 'html',
          value: node.description
        }
      };

@ksen0 and @limzykenneth , would also love to hear your thoughts and feedback here.
Thanks!

[IIITM-Jay]

I have checked it too with the changes as discussed above, it is coming as expected:

 "textToModel": {
      "overloads": [
        [
          "String",
          "Number",
          "Number",
          "Number",
          "Number",
          "Object?"
        ]
      ]
    }

and yet another example with the below params as one of the overloads of createModel:

/**
   * @method createModel
   * @param  {String} modelString
   * @param  {String} [fileType]
   * @param  {Object} [options]
   * @param  {function(p5.Geometry)} [options.successCallback]
   * @param  {function(Event)} [options.failureCallback]
   * @param  {boolean} [options.normalize]
   * @param  {boolean} [options.flipU]
   * @param  {boolean} [options.flipV]
   * @return {p5.Geometry} the <a href="#/p5.Geometry">p5.Geometry</a> object
   */

it is coming as:

[
          "String",
          "String?",
          "Object?"
]

[IIITM-Jay]

Hi @davepagurek , @ksen0 , @limzykenneth ,
if there is a consensus on this and you folks feel that this is the correct way to approach the issue, I would be more than happy to proceed ahead raising a PR. Will revise the PR too as per the feedback and changes as required.
Thanks!

[davepagurek]

Hi @IIITM-Jay! I think there are two things to address here. The first thing is to not treat nested properties like separate positional parameters, but the second is the ability to check those types at runtime. Because we need that second case, we will need to do more than filter out the nested parameter types -- I think we'll have to pass that data into FES still. So the idea would be to turn the textToModel overloads from this:

 "textToModel": {
      "overloads": [
        [
          "String",
          "Number",
          "Number",
          "Number",
          "Number",
          "Object?"
        ]
      ]
    }

...to something like this (as an example syntax):

 "textToModel": {
      "overloads": [
        [
          "String",
          "Number",
          "Number",
          "Number",
          "Number",
          "{ extrude: Number?, sampleFactor: Number?  }?"
        ]
      ]
    }

...and then parse that out and check it in FES here:

p5.js/src/core/friendly_errors/param_validator.js

Line 143 in a46490d

fn.generateZodSchemasForFunc = function (func) {

(Another possible approach to avoid stringifying and then re-parsing could be to change the parameterData.json format to include more structure, similar to what we see in the data.json output of convert.mjs.)

To do so, I think you'll have to identify which properties are nested properties. We don't need to worry too much about efficiency since this is only run once at compile time (end users of p5.js don't incur that cost, just us when building the library) so we can favour simplicity for now.

There are two ways, it seems, for us to get nested parameters:

1. As you mentioned, properties with a . in the name. Checking for the existince of a . in the string is probably sufficient for now.
2. Having a property type that comes from a @typedef. I'm not sure we have any of these yet, so we can maybe ignore them for now?

[IIITM-Jay]

Hi @davepagurek! Thanks for the insights on this. I am really enjoying this as it looks like I again got something to dive deeper and playaround with the approaches for a suitable one.

... so we can favour simplicity for now.

As you mentioned, properties with a . in the name. Checking for the existince of a . in the string is probably sufficient for now.

I agree that sometimes simplicity is favoured over the optimization for easier maintenance and enhanced readability. So, I guess yes the very first approach that came to my mind as I mentioned earlier and you also recognized will suffice for filter out the nested properties.

Will work around and come back with the syntax changes as well checking it with FES.

Having a property type that comes from a @typedef. I'm not sure we have any of these yet, so we can maybe ignore them for now?

Yeah! I have checked so far, @typedef is not present yet for now.

[davepagurek]

Thanks for checking for @typedefs!

As far as PRs go, feel free to work on one if you're interested in taking this on! If tackling the problem in two stages helps, your initial idea of just removing the object properties from the array in parameterData.json could be a great first step as it prevents FES from mistakenly checking other positional properties. Changing the Object? to something with more information about its properties could be done as an additional PR if you'd like. Or feel free to tackle both issues at once if you feel inspired 🙂

[IIITM-Jay]

...to something like this (as an example syntax):

I am thinking to have a syntax like:

"textToModel": {
  "overloads": [
    [
      "String",
      "Number",
      "Number",
      "Number",
      "Number",
      {
        "type": "Object",
        "optional": true,
        "properties": {
          "extrude": "Number?",
          "sampleFactor": "Number?"
        }
      }
    ]
  ]
}

I have given an additional key as optional with boolean values to indicate that this parameter would be optional.
@davepagurek would this be ok for syntax

Or feel free to tackle both issues at once if you feel inspired

I am thinking to, let's see

[davepagurek]

@IIITM-Jay so if an array element is just a string, e.g. "Something", it's equivalent to { type: "Something" }? I think that's a good compromise to save space!

Maybe we allow a trailing ? on the string, so "Something?" is equivalent to { type: "Something", optional: true }, since it's fairly simple to check if typeName.endsWith('?'). But maybe we draw the line there to avoid the more complicated structured parsing. So:

• arrays (currently Something[]) have to be { type: "Array", element: "Something" }
• tuples (currently [TypeA, TypeB, TypeC] becomes something like { type: 'Tuple', elements: ['TypeA', 'TypeB', 'TypeC'] }
• unions (currently "TypeA|TypeB|TypeC") becomes { type: 'Union', elements: ['TypeA', 'TypeB', 'TypeC'] }

The main reason for this is that combinations of those things start making us think harder about the order we parse things in, e.g. A|B[] currently should be interpreted as A | (B[]) and not (A|B)[] and we end up having to be careful with the order we check for parts of the string to ensure this. So switching those out to be done via JSON instead of the string could save us a lot of complex code in FES.

[IIITM-Jay]

Hi @davepagurek, hmm... got some interesting things here to flex my brains.

e.g. A|B[] currently should be interpreted as A | (B[]) and not (A|B)[]

In my opinion, this isn't inherently tricky—it's more about the effort required to implement proper parsing. The ambiguity doesn't actually exist if we handle operator precedence correctly and explicitly.
With that said, I believe this can be handled quite easily by JavaScript but cleanly—especially with TypeScript, which adds helpful type-safety and guardrails. Speaking as a TypeScript developer myself, I've found it makes this kind of parsing logic much easier to implement and maintain. But ofcourse JavaScript could also work

My concerns arise when I see examples like A[]?. What it could mean:

• Optional array of A: { type: "Array", element: "A", optional: true } ✅
• Array of optional A: { type: "Array", element: { type: "A", optional: true } } ❌ (not actually represented by this string)

Why in my sense it creates ambiguous to users/ readers?
Does ? apply to the array or to its element? In this form, it's unclear.
And yes, here only structured JSON could help use here an not the basic string formats.

Yet another example:
A|B|C[]
What it could mean:

• A | B | (C[]) ✅, cuurently as what you explained

• Or maybe (A | B | C)[] ❌ (invalid interpretation unless explicitly stated), but my question why not this?. so in this case we have to explicitly check for the occurrence of parentheses (). If not present go with first interpretation, otherwise based on mentioned parentheses ()

Take a look again:
A|B|C? means

• Should be: A | B | (C?)
• But might be parsed as: A | B | C, with ? ignored, if ? is not correctly bound.

These are some really ambiguous examples that came to my mind first. @davepagurek aren't you find this interesting to dive into, but I am really to be honest. Tagging folks to get more thoughts.

@ksen0 , @limzykenneth - would love to hear your thoughts as well.

[IIITM-Jay]

Hi @davepagurek the last example of above is literally critical...

In my view,
A|B|C? should mean A? | B? | C?
I don't think this should mean A | B | (C?)

as I check for this,

"p5": {
    "describe": {
      "overloads": [
        [
          "String",
          "FALLBACK|LABEL?"
        ]
      ]
    },

as compared to the earlier example of the above comment A|B|C[], the same logic couldn't be applied here. Have a look. So these needs explicit and proper handling. Soon will be ready to raise a PR handling these and then revise the same as appropriately required.

CC: @ksen0 , @limzykenneth

[IIITM-Jay]

and for the above, I have safely generated as

 "p5": {
    "describe": {
      "overloads": [
        [
          {
            "type": "String",
            "optional": false
          },
          {
            "type": "union",
            "elements": [
              {
                "type": "FALLBACK",
                "optional": true
              },
              {
                "type": "LABEL",
                "optional": true
              }
            ],
            "optional": true
          }
        ]
      ]
    },

What do you say, @davepagurek , @ksen0 , @limzykenneth ?

[davepagurek]

I think overall my preference is that any time we have the potential for ambiguity, we fall back to structured JSON to avoid the ambiguity, to avoid having extra complexity in FES. So if there's a situation where we'd have both optional + some other construct like arrays or unions, fall back to structured JSON.

This way the FES codebase would be easier for contributors to understand, and contributors don't have to think about recursive parsing at all.

The parsing problem is possibly avoidable, because we're creating the problem ourselves: in convert.mjs, we already have this structured for us in nested objects, and what we're currently doing is re-serializing this as a single string, and then at runtime in FES, parsing this back out of the string back into nested objects again. This adds a step where things can get out of sync, and parsers, while definitely doable, add a lot of code complexity that might be better to avoid.

The one major benefit of the serialize-to-string-and-parse approach is that we can pack the data into a smaller package, but we can maybe get that out of JSON too by picking small property names?

[IIITM-Jay]

The main reason for this is that combinations of those things start making us think harder about the order we parse things in, e.g. A|B[] currently should be interpreted as A | (B[]) and not (A|B)[] and we end up having to be careful with the order we check for parts of the string to ensure this. So switching those out to be done via JSON instead of the string could save us a lot of complex code in FES.

for your concerns above, for me I am able to generate for the below

 "red": {
      "overloads": [
        [
          "p5.Color|Number[]|String"
        ]
      ]
    },

as

"red": {
      "overloads": [
        [
          {
            "type": "union",
            "elements": [
              {
                "type": "p5.Color",
                "optional": true
              },
              {
                "type": "Array",
                "element": "Number",
                "optional": true
              },
              {
                "type": "String",
                "optional": true
              }
            ],
            "optional": false
          }
        ]
      ]
    },

This is what you expected @davepagurek ?

[davepagurek]

That looks good to me @IIITM-Jay! Also if we want to make the JSON small to minimize file size, we can maybe remove optional: false (basically it'll be undefined instead of false, which is mostly equivalent in an if statement) and in some of our other minified json, we also have replaced true with 1 (which is not exactly the same, but also is mostly equivalent in an if statement)

[IIITM-Jay]

That looks good to me @IIITM-Jay! Also if we want to make the JSON small to minimize file size, we can maybe remove optional: false

@davepagurek Hi Dave, that's exactly what I have planned earlier keeping in mind about the size of the JSON that it will grow as a result of redundant keys, but then you made a point on my earlier PR when I kept optional key for only optional field :

it might make sense to apply the same format to all parameters. Currently, it looks like there are two different ways a parameter might be optional, either ending with a ? as in String? or via the optional: true parameter for an object. How do you feel about a format like this?

[
  { "type": "String", "optional": false },
  { "type": "String", "optional": false },
  {
    "type": "Object",
    "optional": true,
    "properties": {
      "successCallback":  { "type": "Function", "optional": true, parameters: [{ type: "p5.Geometry" }] },
      "failureCallback": { "type": "Function", "optional": true, parameters: [{ type: "Event" }] },
      "normalize": { "type": "boolean", "optional": true },
      "flipU": { "type": "boolean", "optional": true },
      "flipV": { "type": "boolean", "optional": true }
    }
  }
]

I thought to keep my point there at PR for minimization of JSON by removing redundant optional field here and keep only at required places, but then I thought as you suggested for consistent format. Anyways, will work too on optimization of JSON.
Thanks again for reviewing this.

[davepagurek]

Ah sorry if my point was a bit unclear, I was mostly referring to specifying optionality in JSON (via { type: "Something", optional: true }) or in a string ("Something?"), which relies on us being able to parse it in two different ways. If we use just JSON, and we check if something is optional like if (node.optional), then { type: "Something" } and { type: "Something", optional: false } follow the same control flow without us explicitly handling two formats in our code, so we don't incur extra technical cost to handle those cases.

[IIITM-Jay]

Hi @davepagurek , I have raised a PR #7843 as per our discussions as above, Would love to hear your feedback and thoughts on the same. Will incorporate appropriate changes and revise the PR as required.
Thanks!

[IIITM-Jay]

Hi @davepagurek , if by any chance, you got some time to look at the PR, may I request to provide suggestions and feedback on the same.
Thank you so much!

[davepagurek]

Thanks @IIITM-Jay! Sorry for the delay, I've been busy helping with Processing interviews this week. I'll try to get you a review tonight or tomorrow 🙂