diff --git a/src/friendly_errors/sketch_verifier.js b/src/friendly_errors/sketch_verifier.js index 45a40608cd..6188c0ae92 100644 --- a/src/friendly_errors/sketch_verifier.js +++ b/src/friendly_errors/sketch_verifier.js @@ -2,6 +2,8 @@ import { parse } from 'acorn'; import { simple as walk } from 'acorn-walk'; import * as constants from '../core/constants'; import { FES } from './fes'; +import { strandsBuiltinFunctions as builtInGLSLFunctions } from '../strands/strands_builtins'; +import { strandsAddedP5Globals } from '../strands/strands_api'; // List of functions to ignore as they either are meant to be re-defined or // generate false positive outputs. @@ -78,6 +80,51 @@ export const verifierUtils = { // `lineOffset` here to correct them. const lineOffset = -1; + function isStrandsBuilderCall(node) { + if (node.type !== 'CallExpression' || !node.arguments?.length) return false; + + const callee = node.callee; + // buildFilterShader(fn), ... + if (callee.type === 'Identifier' && /^build\w*Shader$/.test(callee.name)) { + return true; + } + + // baseFilterShader().modify(fn), ... + if ( + callee.type === 'MemberExpression' && + callee.property?.type === 'Identifier' && + callee.property.name === 'modify' && + callee.object?.type === 'CallExpression' && + callee.object.callee?.type === 'Identifier' && + /^base\w*Shader$/.test(callee.object.callee.name) + ) { + return true; + } + return false; + } + + function recordCallbackBody(arg, strandsFunctionNames, strandsBodyRanges) { + if (!arg) return; + + // named: buildFilterShader(displaceColorsCallback) + if (arg.type === 'Identifier') { + strandsFunctionNames.add(arg.name); + return; + } + + // inline: buildFilterShader(() => { … }) / function () { … } + if ( + arg.type === 'FunctionExpression' || + arg.type === 'ArrowFunctionExpression' + ) { + if (arg.body?.type === 'BlockStatement') { + strandsBodyRanges.push([arg.body.start, arg.body.end]); + } else if (arg.start != null) { + strandsBodyRanges.push([arg.start, arg.end]); + } + } + } + try { const ast = parse(code, { ecmaVersion: 'latest', @@ -85,6 +132,53 @@ export const verifierUtils = { locations: true // This helps us get the line number. }); + const strandsFunctionNames = new Set(); + const strandsBodyRanges = []; + + walk(ast, { + CallExpression(node) { + if (!isStrandsBuilderCall(node)) return; + recordCallbackBody( + node.arguments[0], + strandsFunctionNames, + strandsBodyRanges + ); + } + }); + + // resolve named callbacks to body ranges + walk(ast, { + FunctionDeclaration(node) { + if (node.id && strandsFunctionNames.has(node.id.name) && node.body) { + strandsBodyRanges.push([node.body.start, node.body.end]); + } + }, + VariableDeclarator(node) { + if ( + node.id?.type === 'Identifier' && + strandsFunctionNames.has(node.id.name) && + node.init && + (node.init.type === 'FunctionExpression' || + node.init.type === 'ArrowFunctionExpression') + ) { + const body = node.init.body; + if (body?.type === 'BlockStatement') { + strandsBodyRanges.push([body.start, body.end]); + } else { + strandsBodyRanges.push([node.init.start, node.init.end]); + } + } + } + }); + + function isInsideStrands(node) { + if (node.start == null) return false; + for (const [s, e] of strandsBodyRanges) { + if (node.start >= s && node.start < e) return true; + } + return false; + } + walk(ast, { VariableDeclarator(node) { if (node.id.type === 'Identifier') { @@ -97,7 +191,8 @@ export const verifierUtils = { : 'variables'; userDefinitions[category].push({ name: node.id.name, - line: node.loc.start.line + lineOffset + line: node.loc.start.line + lineOffset, + insideStrands: isInsideStrands(node) }); } }, @@ -105,7 +200,8 @@ export const verifierUtils = { if (node.id && node.id.type === 'Identifier') { userDefinitions.functions.push({ name: node.id.name, - line: node.loc.start.line + lineOffset + line: node.loc.start.line + lineOffset, + insideStrands: isInsideStrands(node) }); } }, @@ -115,7 +211,8 @@ export const verifierUtils = { if (node.id && node.id.type === 'Identifier') { userDefinitions.variables.push({ name: node.id.name, - line: node.loc.start.line + lineOffset + line: node.loc.start.line + lineOffset, + insideStrands: isInsideStrands(node) }); } } @@ -182,8 +279,13 @@ export const verifierUtils = { ) ); - for (let { name, line } of allDefinitions) { - if (!ignoreFunction.includes(name) && globalFunctions.has(name)) { + for (let { name, line, insideStrands } of allDefinitions) { + if ( + !ignoreFunction.includes(name) && + !insideStrands && + globalFunctions.has(name) && + !strandsAddedP5Globals.has(name) + ) { const message = generateFriendlyError( FES.log`function`, name, @@ -194,6 +296,19 @@ export const verifierUtils = { } } + // strands/GLSL check + for (const { name, line, insideStrands } of allDefinitions) { + if (!insideStrands) continue; + if (Object.hasOwn(builtInGLSLFunctions, name)) { + const message = generateFriendlyError( + 'function', + name, + line + 1 + ); + FES.log`${message}`(); + return true; + } + } return false; }, diff --git a/src/strands/strands_api.js b/src/strands/strands_api.js index b873ae916a..79e9935432 100644 --- a/src/strands/strands_api.js +++ b/src/strands/strands_api.js @@ -27,6 +27,9 @@ import { STRANDS_INTERNAL_NAME_PREFIX } from './strands_names'; +// Names that strands adds to p5.prototype but were not original p5 globals. +export const strandsAddedP5Globals = new Set(); + const BUILTIN_GLOBAL_SPECS = { width: { typeInfo: DataType.float1, get: p => p.width }, height: { typeInfo: DataType.float1, get: p => p.height }, @@ -472,6 +475,9 @@ export function initGlobalStrandsAPI(p5, fn, strandsContext) { for (const [functionName, overrides] of Object.entries( strandsBuiltinFunctions )) { + if (!Object.hasOwn(fn, functionName)) { + strandsAddedP5Globals.add(functionName); + } const isp5Function = overrides[0].isp5Function; if (isp5Function) { const originalFn = fn[functionName]; diff --git a/test/unit/core/sketch_overrides.js b/test/unit/core/sketch_overrides.js index 410b5da7c8..f80075ea9b 100644 --- a/test/unit/core/sketch_overrides.js +++ b/test/unit/core/sketch_overrides.js @@ -75,41 +75,50 @@ suite('Sketch Verifier', function () { functions: [ { line: 5, - name: 'foo' + name: 'foo', + insideStrands: false, }, { line: 6, - name: 'bar' + name: 'bar', + insideStrands: false, }, { line: 7, - name: 'baz' + name: 'baz', + insideStrands: false, } ], variables: [ { line: 1, - name: 'x' + name: 'x', + insideStrands: false, }, { line: 2, - name: 'y' + name: 'y', + insideStrands: false, }, { line: 3, - name: 'z' + name: 'z', + insideStrands: false, }, { line: 4, - name: 'v1' + name: 'v1', + insideStrands: false, }, { line: 4, - name: 'v2' + name: 'v2', + insideStrands: false, }, { line: 4, - name: 'v3' + name: 'v3', + insideStrands: false, } ] }; @@ -143,19 +152,23 @@ suite('Sketch Verifier', function () { variables: [ { line: 2, - name: 'x' + name: 'x', + insideStrands: false, }, { line: 6, - name: 'y' + name: 'y', + insideStrands: false, }, { line: 11, - name: 'z' + name: 'z', + insideStrands: false, }, { line: 13, - name: 'i' + name: 'i', + insideStrands: false, } ] }; @@ -176,6 +189,80 @@ suite('Sketch Verifier', function () { expect(result).toEqual({ variables: [], functions: [] }); consoleSpy.mockRestore(); }); + + suite('strands region detection', function () { + test('does not mark GLSL names outside strands hooks', function () { + const code = ` + function setup() { + const length = 0; + createCanvas(100, 100, WEBGL); + } + `; + const result = verifierUtils.extractUserDefinedVariablesAndFuncs(code); + const lengthDef = result.variables.find(d => d.name === 'length'); + expect(lengthDef).toBeDefined(); + expect(lengthDef.insideStrands).toBe(false); + }); + + test('marks names in a named build*Shader callback', function () { + const code = ` + function setup() { + buildFilterShader(hook); + } + function hook() { + filterColor.begin(); + const length = 0; + filterColor.end(); + } + `; + const result = verifierUtils.extractUserDefinedVariablesAndFuncs(code); + const lengthDef = result.variables.find(d => d.name === 'length'); + expect(lengthDef).toBeDefined(); + expect(lengthDef.insideStrands).toBe(true); + }); + + test('handles inline buildFilterShader arrow callbacks', function () { + const code = ` + function setup() { + buildFilterShader(() => { + filterColor.begin(); + const length = 0; + filterColor.end(); + }); + } + `; + const result = verifierUtils.extractUserDefinedVariablesAndFuncs(code); + const lengthDef = result.variables.find(d => d.name === 'length'); + expect(lengthDef).toBeDefined(); + expect(lengthDef.insideStrands).toBe(true); + }); + + test('handles baseFilterShader().modify callbacks', function () { + const code = ` + function setup() { + baseFilterShader().modify(() => { + filterColor.begin(); + const length = 0; + filterColor.end(); + }); + } + `; + const result = verifierUtils.extractUserDefinedVariablesAndFuncs(code); + const lengthDef = result.variables.find(d => d.name === 'length'); + expect(lengthDef).toBeDefined(); + expect(lengthDef.insideStrands).toBe(true); + }); + + test('does not crash or flag unrelated object modify calls', function () { + const code = ` + const obj = { modify: fn => fn() }; + obj.modify(() => { const length = 0; }); + `; + const result = verifierUtils.extractUserDefinedVariablesAndFuncs(code); + const lengthDef = result.variables.find(d => d.name === 'length'); + expect(lengthDef?.insideStrands).toBe(false); + }); + }); }); suite('checkForConstsAndFuncs()', function () { @@ -186,6 +273,7 @@ suite('Sketch Verifier', function () { setup() {} draw() {} rect() {} + max() {} } beforeEach(function () { @@ -264,5 +352,60 @@ suite('Sketch Verifier', function () { expect(result).toBe(false); }); + + test('warns on strands builtin only when insideStrands is true', function () { + const outside = { + variables: [{ name: 'length', line: 0, insideStrands: false }], + functions: [] + }; + // If length is only special-cased via the strands path (not a p5 global), + // outside should not warn for that reason: + const outsideResult = verifierUtils.checkForConstsAndFuncs(outside, MockP5); + // may still be false unless length conflicts with something else + + const inside = { + variables: [{ name: 'length', line: 1, insideStrands: true }], + functions: [] + }; + const insideResult = verifierUtils.checkForConstsAndFuncs(inside, MockP5); + // true only if length is in builtInGLSLFunctions + if (insideResult) { + expect(consoleSpy).toHaveBeenCalledWith( + expect.stringContaining('length') + ); + } + }); + + test('warns on overlapping builtins in both scopes', function () { + const outside = { + variables: [{ name: 'max', line: 0, insideStrands: false }], + functions: [] + }; + const outsideResult = verifierUtils.checkForConstsAndFuncs( + outside, + MockP5 + ); + expect(outsideResult).toBe(true); + expect(consoleSpy).toHaveBeenCalledWith( + expect.stringContaining( + 'function "max" on line 1 is being redeclared and conflicts with a p5.js function' + ) + ); + + consoleSpy.mockClear(); + + const inside = { + variables: [{ name: 'max', line: 1, insideStrands: true }], + functions: [] + }; + const insideResult = verifierUtils.checkForConstsAndFuncs( + inside, + MockP5 + ); + expect(insideResult).toBe(true); + expect(consoleSpy).toHaveBeenCalledWith( + expect.stringContaining('max') + ); + }); }); });