reserved function "width" error message shows for valid code

[KevinWorkman]

I'm using the latest version of the p5.js editor (p5.js version 1.4.1), and I have code like this:

function setup() {
  createCanvas(400, 400);
}

function draw() {
  background(220);
  
  // This line causes an error: "p5.js says: you have used a p5.js reserved function "width" 
  // make sure you change the function name to something else."
  let x = constrain(200, 0, width);
  
  circle(x, 200, 100);
}

When I run the sketch, the code works fine and I see a circle, but I also see this error message in the console:

p5.js says: you have used a p5.js reserved function "width" make sure you change the function name to something else.

+ More info: https://p5js.org/reference/#/p5/width 

Details about the bug:

• p5.js version: 1.4.1
• Web browser and version: Chrome 103.0.5060.114
• Operating System: Windows 10
• Steps to reproduce this: Run this sketch: https://editor.p5js.org/KevinWorkman/sketches/1hXBA9MFY

[3ru]

This problem seems to have occurred since ver 1.4.1. (It was not reproduced in 1.4.0)
Hi @Qianqianye, This is not an editor issue and this should be moved to the p5.js repo.

[limzykenneth]

It is a bit hard for me to debug this as I tracked it down to some weird bug in FES. Basically _validateParameters with constrain will somehow produce this error when passed width as an argument value, although not with other functions such as lerp or dist that I've tested.

Someone more familiar with FES might want to take a look? @almchung @outofambit

[almchung]

Hi everyone,

I think FES's current regex method for checking reserved function names needs to be improved. This particular error message (fes.sketchReaderErrors.reservedFunc) is generated by friendly_errors/sketch_reader.js/fesCodeReader() at the load event. The current algorithm looks for parts matching the pattern, let/const [something] (using letConstName), then if [something] part contains any of our reserved function names, it will throw the error message. This is not desired since it's not exclusive to the case we want to detect, thus generating false positives as we see in this case.

It would be great if anyone familiar with Javascript regular expressions could take a look at this problem.

[davepagurek]

I took a crack at another regex, where capture groups 1 and 2 are the variable names, and the content to the right of the = does not end up in a capture group:

/^(?:let|const|var)\s+(?:(\w+)(?:\s*=[^\n,]+)?,\s+)?(\w+)(?:\s*=\s*[^\n]+)?$/gm

Here are some test cases. Excuse the phone screenshot, I'm killing time in an airport waiting for a flight :)

Breaking down the regex a bit more, this detects an optional bunch of declarations followed by a comma and then a declaration followed by a newline. Inside each comma separated declaration, it knows everything from the = to the comma doesn't need to get captured. The final declaration in a list can skip anything from the = to the end of the line. I think this needs to be modified a little more to handle semicolons, maybe just by using \n|;?

That said, this still breaks if the right hand side of the assignment has a comma in it (e.g. if you assign something to a function with a parameter list.) In general, capturing all cases is beyond what regexes are capable of. A less brittle but heavier solution would be to include something like the babel/parser library, which would let us accurately figure out variable declarations. Does p5 have a policy about including other libraries and when that's warranted?

[limzykenneth]

I agree that using regex for this case is likely reaching the limit of what we can do with it and could also have potential performance issues and edge cases.

There isn't a comprehensive policy about including other libraries per se, just some considerations that I can think of. We do package some libraries already, most notably opentype.js so it's not impossible. However the largest concerns I have for bundling a parser is maintainability and final package size.

As long as the parser included is relatively stable, maintainability might not be that big of an issue. Package size however I think could be a problem, although this only concerns unminified build as we can exclude the parser from minified build that doesn't have FES. 1.4.2 is already at about 4.3MB uncompressed. It would be great if we could cherry pick parser feature to get what we need in terms of recognizing certain syntaxes but I'm not sure if that's even a realistic expectation.

[almchung]

I agree with both of you. Instead of adding more complexity (or trying to cover all edge cases), it would make sense to prioritize removing any false positives to minimize confusion.

[limzykenneth]

Let's go with the regex fix for now then. Anyone want to take this up and file a PR? Comment here and I'll assign the issue to you.

[davepagurek]

I can finish up the regex above in a PR + add some more tests!

[davepagurek]

Btw @KevinWorkman it seems like this is a combination of problems: first, not detecting that width is in the right hand side of an assignment, but second, that it's only detecting width because p5.Pulse from the p5.sound library has a width function. If I remove the p5.sound script tag from the sketch you linked, then the error goes away, so that can be a temporary workaround if the logs are annoying you.

I think that second issue, that p5.Pulse.prototype.width is getting detected from just width, is the same issue described here: #5753