fix loop protection breaking shader strings and p5.strands functions by Nixxx19 · Pull Request #4083 · processing/p5.js-web-editor

Nixxx19 · 2026-04-15T11:25:52Z

issue:

fixes #4080

demo:

changes:

replaced regex-based loop protection with acorn ast-based approach in a new jspreprocess.js module
strings are never visited by the ast parser, so glsl for loops inside template literals and quoted strings are no longer modified
added two-pass detection for p5.strands shader functions: first pass collects function names passed to build*shader() and base*shader().modify(), second pass skips loop protection inside those functions
removed loop-protect and decomment dependencies from embedframe.jsx
added 13 unit tests covering all cases

i have verified that this pull request:

has no linting errors (npm run lint)
has no test errors (npm run test)
has no typecheck errors (npm run typecheck)
is from a uniquely-named feature branch and is up to date with the develop branch.
is descriptively named and links to an issue number, i.e. fixes #4080
meets the standards outlined in the accessibility guidelines

davepagurek

Great work on this! Consider this review nonblocking, just sharing some thoughts for you and @raclim to consider.

davepagurek · 2026-04-15T12:05:10Z

+    callee.property.name === 'modify' &&
+    callee.object.type === 'CallExpression' &&
+    callee.object.callee.type === 'Identifier' &&
+    /^base\w*Shader$/.test(callee.object.callee.name);


Might be worth noting that this might miss some things, e.g. if a plugin author exposes a shader of their own that has its own name or is just a variable and not a function. If we just check that it's a MemberExpression calling a property called modify that would capture all that, but may have some false positives if users have a method called modify in non-p5.strands code. I think either could be a reasonable compromise, @raclim let me know if you have any thoughts on which side to err on!

good point, thank you! updated it to match any .modify() call instead of just base*Shader().modify(), which should cover plugin-defined shaders too. there could be false positives if someone has an unrelated .modify() call, but that feels like the safer tradeoff. happy to hear your thoughts on this.

I think @davepagurek's suggestion sounds good to me! I think we can keep the check broad for now, and refine it later down the line if needed.

thank you both for the input! will keep it broad for now and revisit if false positives come up.

davepagurek · 2026-04-15T12:07:04Z

+  const loops = [];
+
+  function visitNode(node, ancestors) {
+    const isInsideShader = ancestors.some((ancestor, idx) => {


nice, looks good!

davepagurek · 2026-04-15T12:10:10Z

+  try {
+    ast = acorn.parse(jsText, {
+      ecmaVersion: 'latest',
+      sourceType: 'script',


Maybe worth considering that users could use <script type="module"> scripts, e.g. https://editor.p5js.org/davepagurek/sketches/-foIv7eQa -- worth testing to see if parsing fails in those if you use sourceType: 'script' and you have an import statement, or if we'd need to pass in the source type based on how the script tag imports it.

thank you for catching this! added a fallback that tries sourceType: 'script' first, and if that throws (e.g. for a module script with import statements), it retries with sourceType: 'module'. added a test for this case too.

davepagurek · 2026-04-15T12:15:52Z

+  const insertions = [];
+
+  loops.forEach((loop) => {
+    const { line } = loop.loc.start;


Possibly worth considering whether it's preferable to still do line-based insertions like this, which may have issues with multiple loops on the same line like what's implied by the comment in the current implementation:

p5.js-web-editor/client/modules/Preview/EmbedFrame.jsx

Lines 66 to 69 in 37bc396

// Detect and fix multiple consecutive loops on the same line (e.g. "for(){}for(){}")

// which can bypass loop protection. Add semicolons between them so each loop

// is properly wrapped by loopProtect. See #3891.

// Match: for/while/do-while loops followed immediately by another loop

...or if instead it would be better to make changes at the AST level using Acorn by modifying the AST nodes, and then use a tool like escodegen to re-emit the full source code from that. (This is what we do in p5.strands after transpiling shader functions: https://github.com/processing/p5.js/blob/9498619e7b31f1b3a4e0fda5fcdc19d38e7eff20/src/strands/strands_transpiler.js#L1772) The downside of this approach is that escodegen doesn't preserve everything about your source code like whitespace and semicolon usage, so it might look a little different if you put a debugger statement and look at the code actually being run by the browser. Also it would mean adding another library, not sure how much that adds to the bundle size. But it would mean more targeted insertions and less likelihood for it happening incorrectly.

thank you for the suggestion! switched to the escodegen approach. we now modify the ast nodes directly and regenerate the source using escodegen.generate(). added escodegen as a dependency. the tradeoff on whitespace/formatting is real but feels worth it for the correctness guarantees. please let me know if the bundle size is a concern and we can fall back to character-offset string insertions instead, which also handles multiple loops on the same line correctly since we apply them back-to-front.

davepagurek · 2026-04-15T17:20:51Z

+  });
+
+  walk.simple(ast, {
+    BlockStatement(node) {


Do you think we can simplify this step and modify the nodes in place in the earlier pass where we collect the loops to modify? Then we wouldn't need to find them again with another pass here? Or correct me if there's something that that would miss!

i chose the two-pass approach initially to keep the collection and injection steps separate, but you are right that since we already have the ancestors available in the walk, we can do it all in one pass. simplified it to a single walk.ancestor call that both checks for shader functions and injects the protection in place. updated in the latest commit. thank you for the suggestion!

raclim · 2026-04-15T17:48:03Z

+  walk.ancestor(ast, {
+    ForStatement: visitNode,
+    WhileStatement: visitNode,
+    DoWhileStatement: visitNode


Is there anything else(i.e for...of loops) we might want to cover here? Also okay with leaving it as is!

for...of and for...in loops are parsed as separate ast node types (ForOfStatement and ForInStatement), so they are not currently covered. happy to add support for both if you think that would be a good addition!

Nixxx19 added 2 commits April 15, 2026 16:55

fix loop protection breaking shader strings and p5.strands functions

6273dca

Merge branch 'develop' into nityam/fix-loop-protect-shader-strings-4080

78eee42

Nixxx19 mentioned this pull request Apr 15, 2026

Loop protection breaks shaders #4080

Open

davepagurek reviewed Apr 15, 2026

View reviewed changes

Nixxx19 added 2 commits April 15, 2026 22:25

address review feedback on loop protection

cc3ef43

Merge branch 'develop' into nityam/fix-loop-protect-shader-strings-4080

a0b8d98

davepagurek reviewed Apr 15, 2026

View reviewed changes

raclim reviewed Apr 15, 2026

View reviewed changes

simplify loop protection to single ast pass

a1872ad

	// Detect and fix multiple consecutive loops on the same line (e.g. "for(){}for(){}")
	// which can bypass loop protection. Add semicolons between them so each loop
	// is properly wrapped by loopProtect. See #3891.
	// Match: for/while/do-while loops followed immediately by another loop

Uh oh!

Conversation

Nixxx19 commented Apr 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

issue:

demo:

changes:

i have verified that this pull request:

Uh oh!

davepagurek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nixxx19 Apr 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nixxx19 Apr 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nixxx19 Apr 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nixxx19 Apr 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nixxx19 Apr 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Nixxx19 commented Apr 15, 2026 •

edited

Loading

Nixxx19 Apr 15, 2026 •

edited

Loading

Nixxx19 Apr 15, 2026 •

edited

Loading

Nixxx19 Apr 15, 2026 •

edited

Loading

Nixxx19 Apr 15, 2026 •

edited

Loading

Nixxx19 Apr 15, 2026 •

edited

Loading