Fix partial object iteration dropping bindings and guard unsupported RVM patterns#718
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a correctness bug where partial object rules with dynamic keys would stop after the first matching binding, causing missing keys in the produced object. The fix aligns interpreter constantness detection and RVM rule typing so both execution paths emit all bindings for dynamic-key partial objects.
Changes:
- Interpreter: tighten “constant rule” detection so early-return is only enabled when the object key expression is a simple literal.
- RVM compiler: classify bracket-head comprehension rules (
p[k] if ...) asPartialObjectinstead ofPartialSet. - Add regression YAML cases covering dynamic-key partial objects, explicit-value partial objects, multi-body iteration, array iteration, and
containspartial sets (plus interpreter-only v1 coverage for constant-key and multilevel shapes).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/rvm/rego/cases/partial_object_rules.yaml | Adds RVM+interpreter regression cases for partial object iteration (and contains partial set) behavior. |
| tests/interpreter/cases/rule/partial_object_v1.yaml | Adds interpreter-focused Rego v1 cases for constant-key and multilevel partial object shapes. |
| src/languages/rego/compiler/rules.rs | Adjusts rule-type classification so p[k] if { ... } compiles as a partial object. |
| src/interpreter.rs | Prevents incorrect early-return for dynamic-key partial object rules by tightening constness detection. |
| BAZ: 3 | ||
| FOO: 1 | ||
|
|
||
| - note: partial_object_constant_key_still_works |
RVM: Add compiler errors for unsupported partial object patterns (addresses #719)The interpreter fix in this PR is correct and approved. However, the RVM classification change ( What to doAdd compiler error guards so unsupported patterns fall back to the interpreter. The RVM compiler already does this for other unsupported features (e.g., Step 1: Add error variants to
|
|
@copilot Take a look at #718 (comment) |
Addressed in f2ff982. I added explicit RVM compiler errors for constant-key and nested-bracket partial object heads, validated them in the rule compilation path, and updated the RVM regressions to expect those compiler errors while allowing interpreter success.
Done in f2ff982. |
Review Findings — Round 2The interpreter fix is correct and approved. The RVM compiler error guards need adjustments: Finding 1 (High): Validation guard is too narrow —
|
|
@copilot Please address the findings in the previous comment. |
Addressed in bf43dd4. The partial-object guard now applies to all
Done in bf43dd4. |
| cases: | ||
| - note: constant_key_partial_object_v1 | ||
| data: {} | ||
| input: | ||
| enabled: true | ||
| modules: | ||
| - | | ||
| package test | ||
| import rego.v1 | ||
|
|
||
| p["fixed"] if { | ||
| input.enabled | ||
| } |
Review Findings — Round 3The core fix looks solid and all tests pass. Four remaining edge cases to consider: Finding 1 (High): Hidden dynamic bracket in prefix chain
if matches!(prefix.as_ref(), Expr::RefBrack { .. }) {
return Err(CompilerError::PartialObjectNestedKeyUnsupported.at(refr.span()));
}A rule like Fix: Walk the full prefix chain to reject any dynamic fn has_dynamic_bracket(expr: &ExprRef) -> bool {
match expr.as_ref() {
Expr::RefBrack { refr, index, .. } => {
!matches!(index.as_ref(),
Expr::String { .. } | Expr::RawString { .. } | Expr::Number { .. }
| Expr::Bool { .. } | Expr::Null { .. }
) || has_dynamic_bracket(refr)
}
Expr::RefDot { refr, .. } => has_dynamic_bracket(refr),
_ => false,
}
}Then replace the simple Note: this pattern is exotic and currently errors (safe fallback to interpreter), not a silent miscompile. Finding 2 (Medium): Static string-bracket prefix over-rejected
Fix: The Finding 3 (Low): Missing interpreter-only tests for
|
|
@copilot Please address the findings in the previous comment. |
Addressed in 43698c1. The prefix check now walks the full reference chain so hidden dynamic brackets like
Done in 43698c1. |
|
@copilot Please address the findings in the previous comment. |
Addressed in 4f6651a. The undefined-key/value and conflicting-duplicate-key cases now use
Done in 4f6651a. |
|
@copilot Please take a look at #718 (comment) |
Took another look — this was already addressed in 4f6651a, and there aren’t any additional changes needed beyond that update. |
Missing: Regression tests for the exact issue #712 reproducerThe PR fixes the bug but doesn't have a test for the exact scenario reported in the issue. Please add these to lock down v0/v1 semantics for the original reproducer: # --- Issue #712 exact reproducer: violations[k] in v0 vs v1 ---
# v0: violations[k] { ... } is a partial SET (no "if" keyword)
- note: issue_712_reproducer_v0_partial_set
data: {}
input:
servers:
FOO: 1
BAR: 2
BAZ: 3
modules:
- |
package test
violations[k] {
some k, _ in input.servers
}
query: data.test.violations
want_result:
set!: ["BAR", "BAZ", "FOO"]
# v1: violations[k] if { ... } is a partial OBJECT (key -> true)
# This is the exact bug from issue #712 — was producing only one entry
- note: issue_712_reproducer_v1_partial_object
data: {}
input:
servers:
FOO: 1
BAR: 2
BAZ: 3
modules:
- |
package test
violations[k] if {
some k, _ in input.servers
}
query: data.test.violations
want_result:
BAR: true
BAZ: true
FOO: true
# v1: violations contains k if { ... } is a partial SET
- note: issue_712_reproducer_v1_contains_partial_set
data: {}
input:
servers:
FOO: 1
BAR: 2
BAZ: 3
modules:
- |
package test
violations contains k if {
some k, _ in input.servers
}
query: data.test.violations
want_result:
set!: ["BAR", "BAZ", "FOO"]These three tests together lock down the v0/v1 semantic distinction that caused the original issue. Add them to both |
Added the issue #712 reproducer coverage in a639b9d. The RVM suite now includes the v1 partial-object and
Done in a639b9d. |
| let Expr::RefBrack { | ||
| refr: prefix, | ||
| index, | ||
| .. | ||
| } = refr.as_ref() | ||
| else { | ||
| return Ok(()); | ||
| }; | ||
|
|
||
| if Self::has_unsupported_bracket_prefix(prefix) { | ||
| return Err(CompilerError::PartialObjectNestedKeyUnsupported.at(refr.span())); | ||
| } | ||
|
|
||
| if Self::is_simple_literal(index) { | ||
| return Err(CompilerError::PartialObjectConstantKeyUnsupported.at(index.span())); | ||
| } | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
…ification
Partial object rules with dynamic keys (e.g. `violations[k] if { ... }`)
only produced a single entry instead of collecting all bindings. Two
independent bugs caused this:
1. Interpreter: the early-return optimization in eval_output_expr_in_loop
checked whether the rule_ref was constant but never verified whether
the key expression was also constant. A variable key like `k` was
treated as constant output, causing the loop to exit after the first
iteration. Fixed by gating early-return on key_expr constness.
2. RVM: compute_rule_type incorrectly classified `p[k] if { ... }` as
PartialSet instead of PartialObject. OPA v1 semantics define this
form as a partial object (key -> true). Fixed the classification and
added compiler error guards for patterns the RVM codegen cannot yet
handle (constant keys, nested bracket keys), ensuring graceful
fallback to the interpreter.
The OPA test harness now skips RVM validation per-case when partial
object compiler errors are raised, rather than blanket-skipping entire
folders. This preserves RVM coverage for unrelated tests in the same
folders.
Closes #712
anakrish
force-pushed
the
copilot/fix-partial-set-rule-iteration
branch
from
f81b8f6 to
74445fd
Compare
Partial object rules with dynamic keys (
violations[k] if { ... }) only produced a single entry instead of collecting all bindings. Two independent bugs:Interpreter: the early-return optimization in
eval_output_expr_in_loopnever checked whether the key expression was constant. A variable key likekwas treated as constant output, causing the loop to exit after the first match.RVM:
compute_rule_typemisclassifiedp[k] if { ... }asPartialSetinstead ofPartialObject. Added compiler error guards for patterns the codegen cannot yet handle (constant keys, nested brackets), so they fall back to the interpreter cleanly.Example
With
{"items": {"FOO": 1, "BAR": 2, "BAZ": 3}}, now correctly evaluates to{"BAR": true, "BAZ": true, "FOO": true}instead of just{"FOO": true}.Tests
containsset semanticsCloses #712