Skip to content

Fix/fes warnings glsl keywords 9038 - #9122

Open
skyyash wants to merge 7 commits into
processing:mainfrom
skyyash:fix/fes-warnings-glsl-keywords-9038
Open

skyyash wants to merge 7 commits into
processing:mainfrom
skyyash:fix/fes-warnings-glsl-keywords-9038

Conversation

@skyyash

@skyyash skyyash commented Sep 1, 2026 •

Copy link
Copy Markdown
Contributor

Resolves #9038

Changes:

  1. walk through ast and check for isStrandsBuildersCall.
  2. add callback body ranges to strandsBodyRanges using recordCallbackBody.
  3. find if node isInsideStrands.
  4. include insideStrands in variables and functions of userDefinitions.
  5. warn when builtInGLSLFunctions keywords used insideStrands.
  6. added tests to test/unit/core/sketch_overrides.js.

PR Checklist

@p5-bot

p5-bot Bot commented Sep 2, 2026 •

Copy link
Copy Markdown

Continuous Release

CDN link

Published Packages

Commit hash: b5ca533

Previous deployments

0de494e


This is an automated message.

Comment thread src/friendly_errors/sketch_verifier.js Outdated
if (
callee.type === 'MemberExpression' &&
callee.property?.type === 'Identifier' &&
callee.property.name === 'modify'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think we should add a condition to this checking that the .modify() is called on a base*Shader call? Just thinking that modify on its own may be a bit too far-reaching

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense! done

@davepagurek davepagurek left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work on this! Just one minor case I left a comment for, after that itll be good to go!

Comment thread src/friendly_errors/sketch_verifier.js Outdated

for (let { name, line } of allDefinitions) {
if (!ignoreFunction.includes(name) && globalFunctions.has(name)) {
if (!ignoreFunction.includes(name) && globalFunctions.has(name) && !Object.hasOwn(builtInGLSLFunctions, name)) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the !Object.hasOwn(builtInGLSLFunctions, name) guard here causes a regression. Names like max, floor, sin, abs, sqrt are both p5 globals and GLSL builtins, so with this guard a top-level function max() {} outside of any strands callback no longer triggers the existing p5 conflict warning (it did on main), and the new strands loop below skips it too since insideStrands is false.

So, we can remove this !Object.hasOwn(builtInGLSLFunctions, name), wdyt?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm. removing it will make warning appear for common functions but will cause warnings for strands identifers like length used outside strands block... lemme think

@skyyash skyyash Sep 28, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@perminder-17 we can create a new variable to store strandsAddedP5Globals, names strands adds to p5.prototype but were not original p5 globals. then skip only those in loop 1, so outside length stays silent while outside max still warns. loop 2 keeps the full builtin list, so inside max and length both warn. what do you think?

earlier i thought of using this pattern (const isp5Function = overrides[0].isp5Function;) used in src/strands/strands_api.js but then i realized that flag does not mean the same thing. It means p5 has something similar, not the same name on (eg, for distance its true but p5 has dist not distance)

@skyyash
skyyash requested a review from perminder-17 October 3, 2026 05:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[p5.js 2.0+ Bug Report]: Should not emit FES warnings on GLSL keywords outside of p5.strands

3 participants