fix(interpreter,rvm): evaluate key expression in set membership (k, v in set) - #79
Open
anakrish wants to merge 3 commits into
Open
fix(interpreter,rvm): evaluate key expression in set membership (k, v in set)#79anakrish 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>
… in set) Before this fix, 'k, v in set' (Membership with a non-None key) always returned false for sets regardless of whether the key and value were members. Interpreter: when a key is provided for a Set, evaluate both the key and value expressions and check that key == value AND value ∈ set, matching OPA semantics where a set element acts as its own key. RVM: thread the key expression through compile_membership; when present, emit Index(collection, key) → Eq(indexed_value, value) instead of the simpler Contains instruction, keeping interpreter and RVM in agreement. 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 set membership semantics when using an explicit key (k, v in set) so that both interpreter and RVM honor the key expression instead of ignoring/short-circuiting.
Changes:
- Interpreter: evaluates the key expression for
Value::Setmembership and enforceskey == valueplus membership. - RVM compiler: threads the optional
keythrough membership compilation and emitsIndex+Eqwhen a key is provided. - Adds interpreter and RVM regression test cases for set membership with explicit key.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/rvm/rego/cases/set_membership_key.yaml | Adds RVM regression tests for k, v in set behavior. |
| tests/interpreter/cases/in/set_membership_key.yaml | Adds interpreter regression tests for set membership with explicit key, plus unchanged array/object sanity checks. |
| src/languages/rego/compiler/expressions/operations.rs | Updates membership compilation to accept an optional key and emit different instruction sequences accordingly. |
| src/languages/rego/compiler/expressions.rs | Wires Expr::Membership.key through to compile_membership. |
| src/interpreter.rs | Fixes interpreter set-membership evaluation to account for the key expression. |
| 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
+2320
to
+2321
| let key = self.eval_expr(key)?; | ||
| key == value && set.contains(&value) |
Comment on lines
+7
to
+15
| cases: | ||
| - note: set-key-value-true | ||
| data: {} | ||
| modules: | ||
| - | | ||
| package test | ||
| import future.keywords.in | ||
| s := {1, 2, 3} | ||
| x if { 1, 1 in s } |
Comment on lines
+7
to
+15
| cases: | ||
| - note: set-key-value-true | ||
| data: {} | ||
| modules: | ||
| - | | ||
| package test | ||
| import future.keywords.in | ||
| s := {1, 2, 3} | ||
| x if { 1, 1 in s } |
… tests When evaluating k, v in set with both sides bound, the check is now: - k == v and v in set (both bound, normal case) - when key is Undefined: bind key to value if value is in set (unification) - when value is Undefined: bind value to key if key is in set (unification) This enables k to be bound at eval time when it is declared as a local variable (via 'some k') and the interpreter reaches the membership expression before k has a value. Full scheduler-level support for treating k, v in set as a generator when k is a comprehension output variable is out of scope for this change; such patterns continue to require the 'some k, v in set' (SomeIn) form. Interpreter tests added: - set-key-binds-via-some-in: iterate with some k, v in set; v == 20 - set-key-value-bound-both-equal-true: k := 20; k, 20 in s - set-key-value-bound-both-not-equal-false: k := 10; k, 20 in s - set-key-value-bound-not-member-false: k := 99; k, 99 in s RVM tests added: - same as interpreter tests (using count-based assertion for the not-member case) 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 evaluation of the key expression in set membership checks (
k, v in set).Problem
In OPA,
k, v in setchecks thatvis a member ofsetAND that the keykequalsv(since sets have no separate keys). The previous interpreter code short-circuited tofalsewhenever a key was present for a Set value, without evaluating the key expression. The RVM compiler didn't thread the key parameter at all, so it also silently ignored it.Changes
Interpreter (
src/interpreter.rs): In theValue::Setarm ofeval_membership, when a key is present, bind the key expression tovalue(since set keys equal their values) and check set membership.RVM compiler (
src/languages/rego/compiler/expressions.rs+operations.rs): Thread the optionalkeyparameter throughcompile_membership. When key isSome, emitIndex+Eqinstructions instead of justContains.Tests
tests/interpreter/cases/in/set_membership_key.yaml— interpreter-side teststests/rvm/rego/cases/set_membership_key.yaml— RVM-side testsBoth interpreter and RVM produce identical results, preserving dual-execution-path parity.
Part of OPA v1.2.0 → v1.19.1 upgrade.
Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com