Conversation
- Added a `scope` attribute to the `ASTNode` class to track the lexical scope at the node's location, which is populated by `ScopeBuilder`. - Imported necessary scope classes in the `__init__.py` file for better organization and accessibility.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…Builder - Replace ScopeBuilder visitor pattern with build_scope() methods on each AST node class for more explicit, per-node scope propagation - Include all statement types (not just ModuleInstantiation) when flattening in builder.py so declarations can be hoisted and scopes attached everywhere - Remove ScopeBuilder from public API exports Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR introduces lexical scope analysis to the OpenSCAD AST by adding a Scope model and propagating per-node scope information through new build_scope() methods, alongside extensive documentation and tests.
Changes:
- Added
Scope+build_scopes()and attached ascopeattribute to AST nodes for lexical name resolution. - Reworked AST node scope propagation (hoisting and per-node
build_scope()traversal) and adjusted AST builder output for module bodies. - Added comprehensive scoping docs and a large new scope-focused test suite; added release publishing workflow and updated packaging/lock metadata.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/openscad_parser/ast/scope.py |
Adds Scope and build_scopes() with top-level hoisting. |
src/openscad_parser/ast/nodes.py |
Introduces ASTNode.scope and implements build_scope() across many node types, including hoisting helpers. |
src/openscad_parser/ast/builder.py |
Changes module body flattening to preserve all statement nodes (not only module instantiations). |
src/openscad_parser/ast/__init__.py |
Exposes Scope and build_scopes in the public AST API. |
tests/test_scope.py |
Adds a large suite validating scope creation, hoisting, and lookup behavior. |
tests/test_ast_generation.py |
Adds AST-generation tests for unary NOT operators and helper parsing routine. |
tests/test_expressions.py |
Minor whitespace cleanup. |
docs/SCOPING_RULES.md |
Adds detailed scoping rules reference to guide/validate implementation. |
.github/workflows/publish.yml |
Adds PyPI publish workflow on GitHub Releases. |
pyproject.toml |
Bumps project version to 2.1.0. |
uv.lock |
Updates lockfile for Python >=3.11, dependency/extras changes, and local editable package entry. |
.claude/skills/review-code/SKILL.md |
Adds internal guidance document for code review. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| position: "Position" | ||
| scope: "Scope | None" = field(default=None, kw_only=True) |
There was a problem hiding this comment.
scope is now a dataclass field on ASTNode, but serialization._serialize_node() serializes all dataclass fields (except position). After build_scopes() runs, node.scope becomes a Scope instance, which _serialize_value() cannot handle, so ast_to_dict/json/yaml will raise TypeError. Either exclude scope from serialization (e.g., skip this field in serialization.py), or make scope non-serializable (e.g., not a dataclass field / mark it and have serialization honor the marker). Also consider repr=False, compare=False to avoid scope affecting debug output and equality.
There was a problem hiding this comment.
scope should be excluded from serialization.
| def build_scope(self, parent_scope: "Scope") -> None: | ||
| self.scope = parent_scope | ||
| self.name.build_scope(parent_scope) | ||
| self.expr.build_scope(parent_scope) |
There was a problem hiding this comment.
Assignment.build_scope() currently visits the RHS expression using parent_scope. Because assignments are hoisted into the scope before visiting nodes (via build_scopes() / _collect_hoisted_declarations()), this makes the RHS able to resolve the variable being defined (e.g., x = x + 1;) to the current Assignment, which contradicts the scoping rules documented in docs/SCOPING_RULES.md (RHS should not see the variable being defined). Consider evaluating self.expr in a scope that excludes the current assignment’s binding (falling back to any outer binding), while still supporting the documented exception for function-literal recursion if needed.
There was a problem hiding this comment.
The SCOPING_RULES.md file is correct in this matter. The RHS should only see variables declared BEFORE this assignment statement. However, function literals declared inside the RHS of this assignment should, for their bodies, see all variables declared in the parent scope.
|
@copilot open a new pull request to apply changes based on the comments in this thread |
…on exclusion, Assignment RHS pre-assignment scope Co-authored-by: revarbat <350001+revarbat@users.noreply.github.com>
…tion literals are closures, lazy-evaluated) Co-authored-by: revarbat <350001+revarbat@users.noreply.github.com>
Fix None filtering, scope serialization, and Assignment RHS scoping
Summary
Scopeclass andbuild_scopes()function for lexical scope analysis of OpenSCAD ASTbuild_scope()methods to all AST node classes for explicit, per-node scope propagation (replacesScopeBuildervisitor)scopeattribute toASTNodebase class, populated after callingbuild_scopes()docs/SCOPING_RULES.md) and test suite (tests/test_scope.py, 700+ lines)Test plan
tests/test_scope.pycovers variable resolution, hoisting, closures, and nested scopestests/test_ast_generation.pycovers AST builder changes🤖 Generated with Claude Code