Keep p5.strands transpiler output valid for comma-joined expressions

src/strands/strands_transpiler.js

@@ -442,6 +442,29 @@ function addCopyingAndReturn(functionBody, varsToReturn, sourcePrefix = null) {
   });
 }
 
+/*
+ * Rewrites `node` in place into the call expression
+ * `object.methodName(...args)`.
+ *
+ * The rewritten node stays an *expression*, so it is still valid wherever the
+ * original one was: inside a comma (sequence) expression, a ternary, or a call
+ * argument. Turning it into an ExpressionStatement instead would make
+ * escodegen emit a `;` in those positions, producing invalid JavaScript.
+ */
+function replaceWithMethodCall(node, object, methodName, args) {
+  delete node.operator;
+  delete node.left;
+  delete node.right;
+  node.type = 'CallExpression';
+  node.callee = {
+    type: 'MemberExpression',
+    computed: false,
+    object,
+    property: { type: 'Identifier', name: methodName }
+  };
+  node.arguments = args;
+}
+
 const ASTCallbacks = {
   UnaryExpression(node, state, ancestors) {
     if (
@@ -704,22 +727,12 @@ const ASTCallbacks = {
     }
     // Handle direct varying variable assignment: myVarying = value
     if (state.varyings[node.left.name]) {
-      node.type = 'ExpressionStatement';
-      node.expression = {
-        type: 'CallExpression',
-        callee: {
-          type: 'MemberExpression',
-          object: {
-            type: 'Identifier',
-            name: node.left.name
-          },
-          property: {
-            type: 'Identifier',
-            name: 'bridge'
-          }
-        },
-        arguments: [node.right]
-      };
+      replaceWithMethodCall(
+        node,
+        { type: 'Identifier', name: node.left.name },
+        'bridge',
+        [node.right]
+      );
     }
     // Handle swizzle assignment to varying variable: myVarying.xyz = value
     // Note: node.left.object might be worldPos.getValue() due to prior Identifier transformation
@@ -729,22 +742,9 @@ const ASTCallbacks = {
         const value = node.right;
         const callee = source.object;
         const member = source.property;
-        node.right = undefined;
-        node.left = undefined;
-        node.operator = undefined;
-        node.callee = {
-          type: 'MemberExpression',
-          object: callee,
-          property: {
-            type: 'Identifier',
-            name: 'set'
-          }
-        };
-        node.arguments = [member, value];
-        node.type = 'CallExpression';
+        replaceWithMethodCall(node, callee, 'set', [member, value]);
         return;
       }
-
       let varyingName = null;
 
       // Check if it's a direct identifier: myVarying.xyz
@@ -767,28 +767,12 @@ const ASTCallbacks = {
 
       if (varyingName) {
         const swizzlePattern = node.left.property.name;
-        node.type = 'ExpressionStatement';
-        node.expression = {
-          type: 'CallExpression',
-          callee: {
-            type: 'MemberExpression',
-            object: {
-              type: 'Identifier',
-              name: varyingName
-            },
-            property: {
-              type: 'Identifier',
-              name: 'bridgeSwizzle'
-            }
-          },
-          arguments: [
-            {
-              type: 'Literal',
-              value: swizzlePattern
-            },
-            node.right
-          ]
-        };
+        replaceWithMethodCall(
+          node,
+          { type: 'Identifier', name: varyingName },
+          'bridgeSwizzle',
+          [{ type: 'Literal', value: swizzlePattern }, node.right]
+        );
       }
     }
   },
@@ -1607,7 +1591,28 @@ function functionHasSetInControlFlow(functionNode) {
 
   return hasSetInControlFlow;
 }
-
+/*
+ * Does this statement contain a `<hook>.<methodName>()` call?
+ *
+ * Also looks inside comma (sequence) expressions, so that minified-style code
+ * such as `hook.begin(), doSomething();` is still recognised.
+ */
+function statementCallsHookMethod(stmt, methodName, exprString) {
+  if (stmt.type !== 'ExpressionStatement') {
+    return false;
+  }
+  const expressions =
+    stmt.expression?.type === 'SequenceExpression'
+      ? stmt.expression.expressions
+      : [stmt.expression];
+  return expressions.some(
+    expr =>
+      expr?.type === 'CallExpression' &&
+      expr.callee?.type === 'MemberExpression' &&
+      expr.callee?.property?.name === methodName &&
+      escodegen.generate(expr.callee.object) === exprString
+  );
+}
 // Transform a function to use __setValue pattern instead of .set() calls in branches/loops
 function transformFunctionSetCalls(functionNode) {
   if (!functionNode.body || functionNode.body.type !== 'BlockStatement') {
@@ -1662,26 +1667,22 @@ function transformFunctionSetCalls(functionNode) {
 
     let beginCallIndex = -1;
     for (let i = 0; i < functionNode.body.body.length; i++) {
-      const stmt = functionNode.body.body[i];
       if (
-        stmt.type === 'ExpressionStatement' &&
-        stmt.expression?.type === 'CallExpression' &&
-        stmt.expression?.callee?.type === 'MemberExpression' &&
-        stmt.expression?.callee?.property?.name === 'begin'
+        statementCallsHookMethod(functionNode.body.body[i], 'begin', exprString)
       ) {
-        const beginExprString = escodegen.generate(
-          stmt.expression.callee.object
-        );
-        if (beginExprString === exprString) {
-          beginCallIndex = i;
-          break;
-        }
+        beginCallIndex = i;
+        break;
       }
     }
 
-    // Insert intermediate variable after .begin() if found, otherwise at the start
+    // Declare the intermediate variable *before* the statement containing
+    // .begin(), not after it. Minified code can put .begin() and a .set() in
+    // the same comma expression (`hook.begin(), hook.set(x)`). Every .set() on
+    // this hook is rewritten below to assign to this variable, so declaring it
+    // after that statement would use it before its `let` runs. (This pass only
+    // runs for functions with a .set() inside if/for; see #8576.)
     if (beginCallIndex !== -1) {
-      functionNode.body.body.splice(beginCallIndex + 1, 0, intermediateVarDecl);
+      functionNode.body.body.splice(beginCallIndex, 0, intermediateVarDecl);
     } else {
       functionNode.body.body.unshift(intermediateVarDecl);
     }
@@ -1697,25 +1698,16 @@ function transformFunctionSetCalls(functionNode) {
         ) {
           const currentExprString = escodegen.generate(node.callee.object);
           if (currentExprString === exprString && node.arguments.length > 0) {
-            // Find the parent statement
-            let parentStmt = null;
-            for (let i = ancestors.length - 1; i >= 0; i--) {
-              if (ancestors[i].type === 'ExpressionStatement') {
-                parentStmt = ancestors[i];
-                break;
-              }
-            }
-
-            if (parentStmt) {
-              // Replace the .set() call with an assignment
-              parentStmt.type = 'ExpressionStatement';
-              parentStmt.expression = {
-                type: 'AssignmentExpression',
-                operator: '=',
-                left: { type: 'Identifier', name: intermediateVarName },
-                right: node.arguments[0]
-              };
-            }
+            // Replace the .set() call itself with an assignment, in place.
+            // Replacing the enclosing statement instead would discard any
+            // sibling expressions when the call is part of a comma expression.
+            const value = node.arguments[0];
+            delete node.callee;
+            delete node.arguments;
+            node.type = 'AssignmentExpression';
+            node.operator = '=';
+            node.left = { type: 'Identifier', name: intermediateVarName };
+            node.right = value;
           }
         }
       }
@@ -1741,18 +1733,11 @@ function transformFunctionSetCalls(functionNode) {
     // Find the .end() call for this hook
     let endCallIndex = -1;
     for (let i = 0; i < functionNode.body.body.length; i++) {
-      const stmt = functionNode.body.body[i];
       if (
-        stmt.type === 'ExpressionStatement' &&
-        stmt.expression?.type === 'CallExpression' &&
-        stmt.expression?.callee?.type === 'MemberExpression' &&
-        stmt.expression?.callee?.property?.name === 'end'
+        statementCallsHookMethod(functionNode.body.body[i], 'end', exprString)
       ) {
-        const endExprString = escodegen.generate(stmt.expression.callee.object);
-        if (endExprString === exprString) {
-          endCallIndex = i;
-          break;
-        }
+        endCallIndex = i;
+        break;
       }
     }
 

test/unit/webgl/p5.Shader.js

@@ -3377,6 +3377,134 @@ suite('p5.Shader', function () {
       assert.approximately(pixelColor[2], 0, 5);
     });
 
+    suite('comma operator (#9178)', () => {
+      test('handles comma-joined hook calls with a shared variable', () => {
+        myp5.createCanvas(50, 50, myp5.WEBGL);
+        myp5.pixelDensity(1);
+
+        const testShader = myp5.baseMaterialShader().modify(
+          () => {
+            let processedNormal = myp5.sharedVec3();
+            myp5.objectInputs.begin();
+            myp5.objectInputs.position += [0, 0, 0];
+            myp5.objectInputs.end();
+
+            // Comma-joined, as a JavaScript minifier would emit it
+            myp5.pixelInputs.begin(),
+              (processedNormal = myp5.normalize(myp5.pixelInputs.normal)),
+              myp5.pixelInputs.end();
+
+            myp5.finalColor.begin();
+            myp5.finalColor.set([myp5.abs(processedNormal), 1]);
+            myp5.finalColor.end();
+          },
+          { myp5 }
+        );
+
+        myp5.background(255, 0, 0);
+        myp5.noStroke();
+        myp5.shader(testShader);
+        myp5.plane(myp5.width, myp5.height);
+
+        const centerColor = myp5.get(25, 25);
+        assert.approximately(centerColor[0], 0, 5);
+        assert.approximately(centerColor[1], 0, 5);
+        assert.approximately(centerColor[2], 255, 5);
+      });
+
+      test('handles a comma-joined swizzle assignment', () => {
+        myp5.createCanvas(5, 5, myp5.WEBGL);
+
+        expect(() => {
+          myp5.baseMaterialShader().modify(
+            () => {
+              // The shared variable is consumed by the p5.strands transpiler, not by JS.
+              /* oxlint-disable-next-line no-unused-vars */
+              let processedNormal = myp5.sharedVec3();
+              myp5.pixelInputs.begin(),
+                (processedNormal.xy = myp5.pixelInputs.normal.xy),
+                myp5.pixelInputs.end();
+            },
+            { myp5 }
+          );
+        }).not.toThrow();
+      });
+
+      test('keeps sibling expressions around a .set() call in control flow', () => {
+        myp5.createCanvas(50, 50, myp5.WEBGL);
+
+        const testShader = myp5.baseFilterShader().modify(
+          () => {
+            myp5.filterColor.begin();
+            let value = 1;
+            let c = [0, 1, 0, 1];
+            if (value > 0.5) {
+              (c = [1, 0, 0, 1]), myp5.filterColor.set(c);
+            }
+            myp5.filterColor.end();
+          },
+          { myp5 }
+        );
+
+        myp5.background(255, 255, 255);
+        myp5.filter(testShader);
+
+        // Red only if the `c = [1, 0, 0, 1]` sibling survived transpilation
+        const pixelColor = myp5.get(25, 25);
+        assert.approximately(pixelColor[0], 255, 5);
+        assert.approximately(pixelColor[1], 0, 5);
+        assert.approximately(pixelColor[2], 0, 5);
+      });
+
+      test('finds .begin() and .end() inside comma expressions', () => {
+        myp5.createCanvas(50, 50, myp5.WEBGL);
+
+        const testShader = myp5.baseFilterShader().modify(
+          () => {
+            let step = 0;
+            myp5.filterColor.begin(), (step = 1);
+            if (step > 0.5) {
+              myp5.filterColor.set([1, 0, 0, 1]);
+            }
+            myp5.filterColor.end(), (step = 2);
+          },
+          { myp5 }
+        );
+
+        myp5.background(255, 255, 255);
+        myp5.filter(testShader);
+
+        const pixelColor = myp5.get(25, 25);
+        assert.approximately(pixelColor[0], 255, 5);
+        assert.approximately(pixelColor[1], 0, 5);
+        assert.approximately(pixelColor[2], 0, 5);
+      });
+
+      test('handles .set() right after .begin() in the same comma expression', () => {
+        myp5.createCanvas(50, 50, myp5.WEBGL);
+
+        const testShader = myp5.baseFilterShader().modify(
+          () => {
+            myp5.filterColor.begin(), myp5.filterColor.set([0, 1, 0, 1]);
+            let value = 1;
+            if (value > 0.5) {
+              myp5.filterColor.set([1, 0, 0, 1]);
+            }
+            myp5.filterColor.end();
+          },
+          { myp5 }
+        );
+
+        myp5.background(255, 255, 255);
+        myp5.filter(testShader);
+
+        const pixelColor = myp5.get(25, 25);
+        assert.approximately(pixelColor[0], 255, 5);
+        assert.approximately(pixelColor[1], 0, 5);
+        assert.approximately(pixelColor[2], 0, 5);
+      });
+    });
+
     test('handle .set() in for loop with flat API', () => {
       myp5.createCanvas(50, 50, myp5.WEBGL);