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.
| 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.
|
@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