FES throws an error when non globally exposed function's name used in other context

[aditya-shrivastavv]

Resolves #5753

Changes:

Previously we were adding all the methods of all p5 classes as reserved functions in the functionArray, but all p5 classes do not have their functions added to the global scope.
So, as suggested by @davepagurek that only the methods/functions inside p5, p5.Renderer, p5.RendererGL, and p5.Renderer2D are available in the global scope, so only those should be added to functionArray so that FES do not throw an error when a function's name (which is not exposed or defined by p5.js in the global scope) is used as a variable or any other way.

PR Checklist

• npm run lint passes
• Inline documentation is included / updated
• Unit tests are included / updated

[aditya-shrivastavv]

1. more classes can be added as per need.
2. I think the name of identifier globalFunctions is not accurate, should it be changed ?

[davepagurek]

right, maybe something like classesWithGlobalFunctions would be more accurate?

[almchung]

Hi @aditya-shrivastavv, thank you for submitting this PR.
I shared my thoughts in the discussion thread (see #5753) as I feel that this approach may not completely resolve the underlying issue (of false positives and false negatives). If you would like to take a stab at a different approach, please feel free to do so — I can answer any questions you may have.
Also, regardless of the solution being used, please add unit tests to ensure that the OP’s issue would be resolved after the change and also to increase test coverage. For instance, you can mimic this specific unit test so that the OP’s case of declaring size variable before setup() would NOT throw an error after the changes.

[davepagurek]

Thanks for making this change @aditya-shrivastavv! Agreed with @almchung, let's copy that unit test they linked and pick a few example functions to check for (maybe a fill-related one, a typography-related one, a shape-related one?)

[aditya-shrivastavv]

Thanks, @davepagurek and @almchung for your feedback, I have read #5753 and understood what problems my approach can cause in different situations.
But, as suggested by @davepagurek in this comment, looking at our future goals, we might have to ignore some of the caveats. Since it's a matter of discussion whether we want to extract function names in the build process via regex or do it in the functions array way, what should be my next steps in this PR?
anyway I sincerely appreciate the input of @almchung on this problem

[almchung]

@aditya-shrivastavv: it would be great to have the unit tests related to this issue, as @davepagurek also mentioned.

[aditya-shrivastavv]

I think there is a similar test already written for this, which detects the reassignment of the p5.js functions outside setup.
here
is that what we want? should I write a new test or should I modify the existing one?

[almchung]

The previous tests didn't catch the false positive cases discussed in #5753 so we need new tests for those.

[davepagurek]

Thanks for adding some tests! I think my only remaining comments are about naming things in the code, after that I think this is good to go!

[davepagurek]

We should probably name this test something like "does not detect ..." to make it clear that we don't want it catching this false positive.

[aditya-shrivastavv]

done !
Is there anything else I can do or I did wrong ?

[davepagurek]

Looks great, thanks @aditya-shrivastavv!

[aditya-shrivastavv]

hey, @davepagurek I think we should rename the identifier globalFunctions to something better to make it much more clear for future.

[davepagurek]

@aditya-shrivastavv if you want to make another PR renaming the variable that sounds good to me!