Version 1.4.1 adds reserved function “size”

[da091005]

Topic

Between version 1.4.0 and 1.4.1 a reserved function “size” was added. I feel like this variable name is used way too commonly in sketches to have it as a reserved function.

[limzykenneth]

As far as I can see there is no size function exposed or defined by p5.js in the global scope and trying to assign a value to size in 1.4.1 does not result in a warning. Can you provide more info about what you have observed?

[da091005]

This might be web-editor specific then. But when I change the source from 1.4.1 to 1.4.0 the error goes away.

[limzykenneth]

Can you share a link to the sketch you are using to test this? Thanks.

[da091005]

Hello, yes it’s an incomplete sketch, but it has a class that uses “size” as one of the attributes. But, even when I change those variable names inside the class, there is still a variable “size” in the main sketch and I still get the warning. I even went to some of my old sketches and updated the source from 1.4.0. to 1.4.1 and I get the same warning on those (because I frequently use “size” as a variable name.

https://editor.p5js.org/UnOriginalPun/sketches/xYNvt_mYr

[limzykenneth]

@almchung Can you perhaps have a look at this as I think it may be related to some FES bug?

[davepagurek]

It looks like this line in the FES source is getting its list of reserved function names from the names of all the methods on all p5 classes: https://github.com/processing/p5.js/blob/194c0dc472e6eeea1f3460d850b5680772eb7123/src/core/friendly_errors/sketch_reader.js#L305

I wonder if it's marking size as reserved because it exists on p5.TypedDict.prototype? https://github.com/processing/p5.js/blob/374acfb44588bfd565c54d61264df197d798d121/src/data/p5.TypedDict.js#L118

[mishaushev]

so what is the solution to reserved function "size"?

[davepagurek]

For now, you can ignore it, it's FES misreporting it as a reserved word when it isn't really.

If someone wants to make a PR to fix this, I believe they would need to update this line to only include some p5 constructors instead of all of them: https://github.com/processing/p5.js/blob/194c0dc472e6eeea1f3460d850b5680772eb7123/src/core/friendly_errors/sketch_reader.js#L305

I think maybe just p5, p5.Renderer, p5.RendererGL, and p5.Renderer2D? Am I missing anything else that contributes global methods?

[mishaushev]

For now, you can ignore it, it's FES misreporting it as a reserved word when it isn't really.

If someone wants to make a PR to fix this, I believe they would need to update this line to only include some p5 constructors instead of all of them:

https://github.com/processing/p5.js/blob/194c0dc472e6eeea1f3460d850b5680772eb7123/src/core/friendly_errors/sketch_reader.js#L305

I think maybe just p5, p5.Renderer, p5.RendererGL, and p5.Renderer2D? Am I missing anything else that contributes global methods?

But what to do if the code doesn't run cause of the issue? How can I ignore it?

[davepagurek]

Does it go away if you use p5.disableFriendlyErrors = true? https://p5js.org/reference/#/p5/disableFriendlyErrors

[limzykenneth]

@mishaushev FES warnings does not cause the sketch to halt, it merely prints the message to console. If your code is halting there probably is another problem causing an actual error.

[Qianqianye]

I think maybe just p5, p5.Renderer, p5.RendererGL, and p5.Renderer2D? Am I missing anything else that contributes global methods?

These seem right to me. Looping in @limzykenneth @almchung to double check. Thanks!

[limzykenneth]

p5.TypedDict has a member method of size as well so may want to check that. Some manual test after a fix would help as there may be some that we missed.

[Ucodia]

I would love to help fixing this one. I had a look at the existing code and understand why the current code is inaccurate in determining reserved function names. size is definitely not the only word that should not be in here, add and invert3x3 are some other example, see the full list here: current-reserved-functions.txt. As a result some words are currently missing, such as resizeCanvas.

I tried another method such as the following

  const reservedFunctions = Object.entries(p5.instance.constructor.prototype)
    .filter(([k, v]) => typeof v === "function" && !k.includes("_"))
    .map(([k, v]) => k);

This gives us something very close to what we find in the reference documentation here: https://p5js.org/reference/

So before I get into implementation, I would like to understand, what functions do we want to warn about overriding exactly? I would imagine it should be any function which added by p5 on the global context this, am I correct?

[davepagurek]

Thanks for your interest @Ucodia! I think the issue is that on this line https://github.com/processing/p5.js/blob/194c0dc472e6eeea1f3460d850b5680772eb7123/src/core/friendly_errors/sketch_reader.js#L305 we are adding all methods on all p5 classes as reserved functions. However, not all p5 classes have their methods added to the global scope: I believe only p5, p5.Renderer, p5.RendererGL, and p5.Renderer2D do. So I think by using those classes instead of the full list of all p5 classes (p5Constructors), we can fix this issue.

[Ucodia]

Got it, thanks I'll experiment with the suggestion and report the list of words that comes out of it.

[aditya-shrivastavv]

https://github.com/processing/p5.js/blob/194c0dc472e6eeea1f3460d850b5680772eb7123/src/core/friendly_errors/sketch_reader.js#L305

we are adding all methods on all p5 classes as reserved functions. However, not all p5 classes have their methods added to the global scope: I believe only p5, p5.Renderer, p5.RendererGL, and p5.Renderer2D do. So I think by using those classes instead of the full list of all p5 classes (p5Constructors), we can fix this issue.

@davepagurek I think I can do it. I just want to ask that if we only want to include 4 classes ( namely p5, p5.Renderer, p5.RendererGL, and p5.Renderer2D ) can't we do it by being explicit about it and add them directly on basis of their name, instead of adding all of them and filtering them later ? Like below -

see here for more details https://github.com/processing/p5.js/blob/d49417ce6f02f4cefce2361699d08f40b7d72561/src/core/friendly_errors/sketch_reader.js#L321

[davepagurek]

@aditya-shrivastavv Agreed, it's not necessary to literally use filter(...) on the array, but rather just to only iterate over those four classes' methods.

[aditya-shrivastavv]

Please assign this issue to me, I am working on it.

[almchung]

Hi everyone. I revisited this issue while reviewing a PR and wanted to outline reserved function Friendly Errors issues and propose another approach to resolving this problem.

To provide some context, there are currently 326 reserved functions that should NOT be overridden by a user. (v.1.6.0)

1. Current implementation:
   1. The current function in sketch_reader.js checks 239 functions. As noted in this issue, we get 172 false positives (ex: size, name, value) while catching 67 true positives. But it misses 259 cases (false negatives, ex: shininess, round, red).
   2. There is another error handling code catching most reserved function cases (and generating separate messages) without the false positives, but only 10 false negatives: main.js L691-L695. False negatives include print and loadX (loadBytes, loadFont, loadImage, loadJSON, ...) functions.
2. The suggestions in this thread (fixing sketch_reader.js-functionArray) will resolve the OP's concern regarding size, as well as will reduce false positives (63). On the other hand, this approach will yield more false negatives (273).
   1. In addition, I think this approach of dynamically loading/retrieving objects and functions each time may not be efficient (computation-wise) and error-prone (due to limitations that we cannot accurately obtain a complete list of reserved function names).
3. I would like us to consider a slightly different approach where we obtain a complete list of functions at build time (statically), which has the additional benefit of supporting future decoupling of FES. More specifically, we can obtain a complete list of reserved functions during the build step of browserify where we finalize the code and write to a file.
   1. For instance, we are already replacing a placeholder string (for version) here at build time. We can do something similar by declaring a placeholder array in constants.js to be replaced at build time.
   2. Right before writing to the p5.js file, we can use regex to extract all global function names (e.g. _main.default.prototype.<name> = function ...) and replace the placeholder string with these names.
   3. In sketch_reader.js, instead of retrieving a dynamic list of functions, we can use the fixed list of function names from the previous step (as it is declared in constants.js).
   4. The caveat is this method will depend on browserify, so the code may need to be updated if we decide to move away from browserify.

I feel that if we can manage to make this approach (in 3) work, then we may not have to worry about false positives/negatives, so I want to know what others think. I am hopeful that this approach would work, but I admit I may be missing something.

[davepagurek]

Thanks @almchung! I think that approach sounds good and would avoid some of the current issues we have, so if @aditya-shrivastavv feels like taking it on, I'd support that, but otherwise I think it's fine to accept incremental progress on the current system in the mean time.

I think the approach is sound as long as we aren't dynamically assigning values to p5.prototype(which it seems like we only do for constants) so I think it should be ok!

On the other hand, this approach will yield more false negatives (273).

Are those false negatives caught by main.js L691-L695? If so, I think it's OK to proceed with the change. Otherwise, agreed, we probably want to try to catch these in some other way.

[davepagurek]

One other concern would be a dependency on having a full build. We don't support anything else currently, but there are some open issues discussing paths to being able to only include used files when importing p5 (https://github.com/processing/p5.js/issues/5740), so we'd probably want to think about how this would affect a future where that's possible. It might mean that generating the list of reserved strings is a separate build command that builds a full p5 release, then extracts function names via a regex, then writes the result out to a file, which is then read as a regular array of strings during regular builds.

[almchung]

@davepagurek: Sounds great! I also agree we should work on this issue incrementally.

Are those false negatives caught by main.js L691-L695? If so, I think it's OK to proceed with the change. Otherwise, agreed, we probably want to try to catch these in some other way.

Yes, main.js L691-L695 will catch most cases, just excluding ~10 false negatives listed in 1-ii.

[davepagurek]

@almchung Thanks to #6055 I think this particular issue can be closed, but I don't want your suggestion for an update to how FES to be lost. Would you be interested in opening a new issue about the refactor you have in mind?

[bojidar-bg]

Um... I'm not sure this issue was quite fully fixed. Trying with a minimal reproduction example that uses local variables instead of global variables, I can observe a similar spurious error on p5 versions >1.4.1, including p5 1.10.0. (Let me know if I should open a new issue for that)

function setup() {
  let size = 1
}

[davepagurek]

In 2.0 we're completely replacing the implementation of this with something that actually parses the AST of your code, which should help avoid the majority of these false positives. hang tight!