fix(interpreter): support walk with assignment binding ([k, v] := walk(obj)) - #81
Open
anakrish wants to merge 3 commits into
Open
fix(interpreter): support walk with assignment binding ([k, v] := walk(obj))#81anakrish wants to merge 3 commits into
anakrish wants to merge 3 commits into
Conversation
microsoft#794) Bumps the per-dependency group in /bindings/ruby with 1 update: [rb_sys](https://github.com/oxidize-rb/rb-sys). Updates `rb_sys` from 0.9.128 to 0.9.130 - [Release notes](https://github.com/oxidize-rb/rb-sys/releases) - [Commits](oxidize-rb/rb-sys@v0.9.128...v0.9.130) --- updated-dependencies: - dependency-name: rb_sys dependency-version: 0.9.130 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: per-dependency ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
The interpreter's WalkBindingPlan only handled the function-call output parameter pattern (walk(obj, [k, v])). When walk appeared in an assignment expression ([k, v] := walk(obj)), the loop hoister tagged the loop's value field with the walk call expression, but the interpreter could not find the binding plan because it only looked in the call's last parameter. hoist.rs: after analyzing the RHS of an AssignExpr, if the RHS pushed exactly one new loop whose type is Walk, update that loop's value field to point at the full assignment expression so the binding plan can be found by eidx. interpreter.rs: introduce a WalkBindingPlan enum with Destructuring (original walk(obj,[k,v]) form) and Assignment ([k,v] := walk(obj) form) variants. get_walk_binding_plan detects AssignExpr as the loop value and retrieves the BindingPlan::Assignment for it; the walk iteration dispatch then calls either execute_destructuring_plan or execute_assignment_plan accordingly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Fixes interpreter handling of walk used in assignment binding form ([path, val] := walk(obj)), aligning interpreter binding dispatch with loop hoisting behavior.
Changes:
- Add
WalkBindingPlanto distinguish destructuring vs assignment binding forwalkloops. - Update interpreter to detect
AssignExpr-backed walk loops and execute the correct binding plan. - Adjust loop hoister to point hoisted walk loop
valueat the full assignment expression for expression-index-based binding lookup.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/interpreter/cases/binding/walk_assignment.yaml | Adds regression tests covering walk in := assignment binding form. |
| src/interpreter.rs | Introduces WalkBindingPlan and updates binding-plan lookup + walk iteration dispatch. |
| src/compiler/hoist.rs | Ensures hoisted walk loops in assignment context reference the AssignExpr for binding-plan lookup. |
| bindings/ruby/Gemfile.lock | Bumps rb_sys dependency version. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+868
to
+870
| ) && loops.len() == loop_count.saturating_add(1) | ||
| { | ||
| if let Some(loop_info) = loops.last_mut() { |
| # Copyright (c) Microsoft Corporation. | ||
| # Licensed under the MIT License. | ||
|
|
||
| # Tests for walk with the := assignment form (k, v := walk(obj)). |
Comment on lines
+855
to
+873
| let loop_count = loops.len(); | ||
| self.analyze_expr(module_idx, rhs, context, loops)?; | ||
| if matches!( | ||
| rhs.as_ref(), | ||
| E::Call { fcn, params, .. } | ||
| if params.len() == 1 | ||
| && matches!( | ||
| fcn.as_ref(), | ||
| E::Var { | ||
| value: Value::String(name), | ||
| .. | ||
| } if name.as_ref() == "walk" | ||
| ) | ||
| ) && loops.len() == loop_count.saturating_add(1) | ||
| { | ||
| if let Some(loop_info) = loops.last_mut() { | ||
| loop_info.value = expr.clone(); | ||
| } | ||
| } |
Replace the fragile loops.len() == loop_count + 1 check with a search in loops[loop_count..] for the first loop with loop_type == LoopType::Walk. The old check assumed exactly one new loop was added during RHS analysis. When walk's argument itself contains unbound-indexed sub-expressions (e.g. walk(objs[_])), the hoister adds an IndexIteration loop for the index before the Walk loop, making loops.len() == loop_count + 2 and causing the Walk loop assignment to be silently skipped. Also: - Fix test comment syntax: (k, v := walk(obj)) -> ([k, v] := walk(obj)) - Add regression YAML test: walk-assignment-with-indexed-arg, which exercises the case where the argument to walk introduces an extra hoisted loop. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the interpreter to handle
walkwhen used in the assignment form[path, val] := walk(obj), which previously only worked in the function-call output parameter formwalk(obj, [path, val]).Problem
The loop hoister in
hoist.rscorrectly detectedwalkcalls in assignment context and pushed a Walk loop. However,get_walk_binding_planin the interpreter looked for the binding plan only at the walk call's last parameter position, so it never found anAssignmentPlanfor the assignment form. The walk loop then dispatched using the destructuring path, which caused incorrect or missing bindings.Changes
src/interpreter.rs: Introduce aWalkBindingPlanenum (Destructuring/Assignment). Updateget_walk_binding_planto detect when the loop value is anAssignExprand retrieve the assignment binding plan. Update the walk iteration dispatch to call the appropriate execution path.src/compiler/hoist.rs: After analyzing anAssignExprwhose RHS is awalk(…)call, update the newly-pushed Walk loop'svaluefield to point at the full assignment expression so the interpreter can find it by expression index.Tests
New YAML test file:
tests/interpreter/cases/binding/walk_assignment.yaml[_, v] := walk(obj)[path, _] := walk(obj)Part of OPA v1.2.0 → v1.19.1 upgrade.
Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com