Merge pull request #9235 from bhabishnu/fix/strands-hook-lifecycle-errors

fix: add friendly errors for hook lifecycle misuse

Author: davepagurek

src/strands/p5.strands.js

@@ -6,6 +6,7 @@
 
 import { transpileStrandsToJS } from './strands_transpiler';
 import { BlockType } from './ir_types';
+import * as FES from './strands_FES';
 
 import { createDirectedAcyclicGraph } from './ir_dag';
 import {
@@ -304,6 +305,12 @@ function strands(p5, fn) {
           strandsCallback();
         }
         if (options.hook) strandsContext.renderer._pInst[options.hook].end();
+        if (strandsContext.activeHook) {
+          FES.userError(
+            'scope error',
+            `It looks like ${strandsContext.activeHook._publicName}.begin() was called without a matching ${strandsContext.activeHook._publicName}.end().`
+          );
+        }
         popBlock(strandsContext.cfg);
 
         // 3. Generate shader code hooks object from the IR

src/strands/strands_api.js

@@ -1440,9 +1440,14 @@ export function createShaderHooksFunctions(strandsContext, fn, shader) {
       finishHook();
     };
 
-    // In the flat strands API, this is how result-returning hooks
-    // are used
+    // In the flat strands API, result-returning hooks use set().
     hook.set = function (result) {
+      if (!hook._active) {
+        FES.userError(
+          'scope error',
+          `It looks like you're trying to call ${hook._publicName}.set() outside of its begin()/end() block.`
+        );
+      }
       hook._result = result;
     };
     hook._active = false;
@@ -1530,6 +1535,23 @@ export function createShaderHooksFunctions(strandsContext, fn, shader) {
       strandsContext.activeHook = undefined;
 
       const expectedReturnType = hookType.returnType;
+      const expectsReturnValue =
+        isStructType(expectedReturnType) ||
+        (expectedReturnType.dataType &&
+          expectedReturnType.typeName?.trim() !== 'void');
+
+      if (
+        expectsReturnValue &&
+        userReturned === undefined &&
+        hook.earlyReturns.length === 0
+      ) {
+        FES.userError(
+          'scope error',
+          `${hook._publicName} requires a value. Make sure to call ` +
+            `${hook._publicName}.set(value) before ${hook._publicName}.end().`
+        );
+      }
+
       let rootNodeID = null;
       const handleRetVal = retNode => {
         if (isStructType(expectedReturnType)) {
@@ -1674,6 +1696,9 @@ export function createShaderHooksFunctions(strandsContext, fn, shader) {
     for (const name of aliases) {
       augmentFnTemporary(fn, strandsContext, name, hook);
     }
+    // Name used in error messages: an explicit alias like filterColor if
+    // there is one, otherwise the unprefixed alias like finalColor
+    hook._publicName = aliases[1] ?? hookType.name;
     hook.earlyReturns = [];
   }
 }

test/unit/webgl/p5.Shader.js

@@ -4233,6 +4233,116 @@ suite('p5.Shader', function () {
       });
     });
 
+    test('set() outside a hook block reports a scope error', () => {
+      myp5.createCanvas(50, 50, myp5.WEBGL);
+
+      try {
+        myp5.baseFilterShader().modify(
+          () => {
+            myp5.filterColor.begin();
+            myp5.filterColor.set([1, 0, 0, 1]);
+            myp5.filterColor.end();
+            myp5.filterColor.set([0, 1, 0, 1]);
+          },
+          { myp5 }
+        );
+      } catch {
+        /* expected */
+      }
+
+      const scopeCall = mockUserError.mock.calls.find(
+        call => call[0] === 'scope error' && call[1].includes('set()')
+      );
+      assert.isDefined(scopeCall, 'scope error should have been called');
+      assert.include(scopeCall[1], 'filterColor.set()');
+      assert.notInclude(scopeCall[1], 'getColor');
+    });
+
+    test('ending a value-returning hook without a value reports a scope error', () => {
+      myp5.createCanvas(50, 50, myp5.WEBGL);
+
+      try {
+        myp5.baseFilterShader().modify(
+          () => {
+            myp5.filterColor.begin();
+            myp5.filterColor.end();
+          },
+          { myp5 }
+        );
+      } catch {
+        /* expected */
+      }
+
+      const scopeCall = mockUserError.mock.calls.find(
+        call =>
+          call[0] === 'scope error' &&
+          call[1].includes('requires a value')
+      );
+      assert.isDefined(scopeCall, 'scope error should have been called');
+      assert.include(scopeCall[1], 'filterColor requires a value');
+      assert.include(
+        scopeCall[1],
+        'filterColor.set(value) before filterColor.end()'
+      );
+      assert.notInclude(scopeCall[1], 'getColor');
+    });
+
+    test('value-required error uses the hook\'s own public name', () => {
+      myp5.createCanvas(50, 50, myp5.WEBGL);
+
+      try {
+        myp5.baseMaterialShader().modify(
+          () => {
+            myp5.finalColor.begin();
+            myp5.finalColor.end();
+          },
+          { myp5 }
+        );
+      } catch {
+        /* expected */
+      }
+
+      const scopeCall = mockUserError.mock.calls.find(
+        call =>
+          call[0] === 'scope error' &&
+          call[1].includes('requires a value')
+      );
+      assert.isDefined(scopeCall, 'scope error should have been called');
+      assert.include(scopeCall[1], 'finalColor requires a value');
+      assert.include(
+        scopeCall[1],
+        'finalColor.set(value) before finalColor.end()'
+      );
+      assert.notInclude(scopeCall[1], 'getFinalColor');
+      assert.notInclude(scopeCall[1], 'filterColor');
+    });
+
+    test('missing end() reports a scope error', () => {
+      myp5.createCanvas(50, 50, myp5.WEBGL);
+
+      try {
+        myp5.baseFilterShader().modify(
+          () => {
+            myp5.filterColor.begin();
+            myp5.filterColor.set([1, 0, 0, 1]);
+          },
+          { myp5 }
+        );
+      } catch {
+        /* expected */
+      }
+      const scopeCall = mockUserError.mock.calls.find(
+        call =>
+          call[0] === 'scope error' &&
+          call[1].includes('without a matching')
+      );
+      assert.isDefined(scopeCall, 'scope error should have been called');
+      assert.include(
+        scopeCall[1],
+        'filterColor.begin() was called without a matching filterColor.end()'
+      );
+    });
+
     test('scope error uses unprefixed hook name', () => {
       myp5.createCanvas(50, 50, myp5.WEBGL);