Feature/detect outside variables strands #8208
Conversation
- Create detectOutsideVariableReferences() function in strands_transpiler.js - Uses Acorn AST parsing to collect declared variables from strand code - Analyzes uniform initializer functions to find variable references - Checks if referenced variables are declared in strand context - Provides helpful error messages to users - Integrates detection before transpilation in p5.strands.js - Add unit tests for the detection logic Addresses issue processing#8172 - Help users debug when accessing outside variables in uniform initializers within p5.strands
…stors parameter - Check for >= 2 arguments instead of > 0 for uniform functions - Properly pass ancestors parameter to walk function - Add comments clarifying uniform function signature
|
Hey @davepagurek! I've re-implemented the outside variable detection following your feedback:
|
| // Simulate code that references mouseX (not declared in strand context) | ||
| const code = ` | ||
| const myUniform = uniform('color', () => { | ||
| return mouseX; // mouseX is not declared |
There was a problem hiding this comment.
I think you might be checking for a different scenario than we need. In the callback of a uniform creation function, that's the right spot to have p5 globals. So this is ok:
baseMaterialShader.modify(() => {
const myUniform = uniformFloat(() => mouseX)
getWorldPosition((inputs) => {
inputs.position.x += myUniform
return inputs
})
})...but this should show a warning:
baseMaterialShader.modify(() => {
getWorldPosition((inputs) => {
inputs.position.x += mouseX
return inputs
})
})Additionally, just uniform() on its own isn't a p5 function, it will always have a type after it, like uniformFloat or uniformVec2.
src/strands/strands_transpiler.js
Outdated
|
|
||
| // Close block scope when exiting | ||
| if (node.type === 'BlockStatement' && scopeChain.length > 1) { | ||
| scopeChain.pop(); |
There was a problem hiding this comment.
Right now it looks like scopeChain doesn't get used really -- it gets added to and then popped off the array before anything reads from it. Maybe consider doing the check all in one step, so that when you encounter a variable identifier, you can check to see if it should be visible in the current scope?
- Don't check uniform callbacks (they should allow outer scope) - Check the main strand code for undeclared variables - Use simpler two-pass approach: collect declarations, then check references - Update tests to reflect correct behavior - Handle function parameters and property access properly Addresses Dave's comments about checking the wrong scenario
|
@davepagurek Fixed! Now checking strand code (not uniform callbacks) and using the simpler two-pass approach. Tests updated. Ready for another look when you have time. |
|
|
||
| test('should not error when variable is declared', function() { | ||
| const code = ` | ||
| baseMaterialShader.modify(() => { |
There was a problem hiding this comment.
I think this is testing something different from what the function would actually receive. This looks like code that would be found in a p5 sketch, but just the contents of the modify callback is sent into the your function normally.
I think a good next step would be to try to do a full integration test by running yarn dev:global and editing the sketch in preview/global. If that works, you can also show the maintainers by building the library with npm run build, and then uploading lib/p5.js to a p5 web editor sketch so that others can run the same test. I suspect doing that will reveal some of the problems with the implementation more quickly than in review (but feel free to reach out if you run into issues while doing so!)
src/strands/strands_transpiler.js
Outdated
| if (isProperty) return; | ||
|
|
||
| // Skip if it's a function parameter | ||
| const isParam = ancestors.some(anc => |
There was a problem hiding this comment.
What is this case aiming to catch? inputs in e.g. getWorldInputs((inputs) => { ... })?
| if (inUniformCallback) return; // Allow outer scope access in uniform callbacks | ||
|
|
||
| // Check if this variable is declared anywhere in the strand | ||
| if (!declaredVars.has(varName)) { |
There was a problem hiding this comment.
I think what you had before with pushing/popping scopes was not a bad approach -- currently, it looks like this will pass, when it should not:
baseMaterialShader().modify(() => {
getWorldInputs(() => {
let i = 1
inputs.position.x += i // OK, i is in scope
return inputs
})
getFinalColor((color) => {
color.r += i // Not OK, i is not in scope
return color
})
})- Trace through ancestor chain to check if variable is in scope - Only look for variable declarations in ancestor nodes - Handles nested scopes properly (e.g. i declared in callback A, used in callback B) - Simplify function parameter detection
|
@davepagurek Thanks for catching those issues! Updated the tests to pass just the function body, and the scope checking should now properly exclude sibling scopes using the ancestor chain. |
- Fixed function parameter detection by comparing parameter names instead of AST nodes - Added comprehensive built-in function list to ignore list - Simplified scope detection logic using two-pass AST traversal - All unit tests now passing - Error detection working correctly in integration tests
2 participants
Resolves #8172
Changes
This PR adds detection for outside variable references in
p5.strandsuniform initializers, providing helpful error messages to users when they reference variables that aren't declared in the strand context.What was implemented:
detectOutsideVariableReferences()function insrc/strands/strands_transpiler.jsthat:uniformFloat('color', () => ...))src/strands/p5.strands.jstest/unit/strands/strands_transpiler.jswith test cases for:mouseX,windowWidth)Approach:
Following @davepagurek's feedback, this implementation:
This is more robust than checking for hardcoded variable names, as it analyzes the actual code structure and works with any user-defined variables.
Screenshots
N/A (infrastructure change - no visual impact)
PR Checklist