Skip to content

Commit 6ef5e74

Browse files
authored
fix(rvm): assert every-quantifier results so failing cases don't pass (microsoft#765)
The RVM was silently succeeding on `every` quantifiers (and loops nested inside an `every` body) that should have failed. In each case the loop computed a pass/fail into a register that the surrounding query then ignored, so the RVM disagreed with the interpreter. Four related fixes: - compile_every_quantifier: guard the loop result so a failing `every` body makes the rule undefined instead of always succeeding. - resolve_iteration_state: `every` over a non-iterable scalar (number, string, bool, null, undefined) is now undefined, not vacuously true. Only genuinely empty collections stay true; any/forEach are untouched. - a `some ... in` inside an `every` body now guards its loop result, so a `some` that matches nothing fails the current iteration. Top-level rule bodies still rely on context yields and are unaffected. - a hoisted index iteration (`some i` / `arr[i]`) inside an `every` body gets the same guard. Also drop `every` from OPA_TODO_FOLDERS so the interpreter-vs-RVM differential suite covers it, add an OPA_UNSKIP_FOLDERS env override for auditing other still-skipped folders, and add regression cases for every variant above.
1 parent 9a486c7 commit 6ef5e74

5 files changed

Lines changed: 366 additions & 6 deletions

File tree

src/languages/rego/compiler/loops.rs

Lines changed: 33 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ use crate::ast::{self, ExprRef, LiteralStmt, Query};
1313
use crate::compiler::destructuring_planner::plans::BindingPlan;
1414
use crate::compiler::hoist::{HoistedLoop, LoopType};
1515
use crate::lexer::Span;
16-
use crate::rvm::instructions::{LoopMode, LoopStartParams};
16+
use crate::rvm::instructions::{GuardMode, LoopMode, LoopStartParams};
1717
use crate::rvm::Instruction;
1818
use crate::Value;
1919
use alloc::format;
@@ -197,6 +197,19 @@ impl<'a> Compiler<'a> {
197197
*end = loop_end;
198198
}
199199

200+
// The loop writes its overall pass/fail into `result_reg`
201+
// (`success_count == total_iterations` for `Every`). The enclosing query
202+
// must fail (evaluate to undefined) when the quantifier does not hold, so
203+
// guard on `result_reg` here. Without this the `every` result is computed
204+
// but discarded, leaving the surrounding rule to always succeed.
205+
self.emit_instruction(
206+
Instruction::Guard {
207+
register: result_reg,
208+
mode: GuardMode::Condition,
209+
},
210+
span,
211+
);
212+
200213
Ok(())
201214
}
202215

@@ -312,6 +325,25 @@ impl<'a> Compiler<'a> {
312325
*end = loop_end;
313326
}
314327

328+
// A hoisted index-iteration loop inside an `every` body acts as a
329+
// condition on the current iteration: if the indexed reference matches
330+
// nothing the iteration must fail. The `every` body emits no context
331+
// yield, so the loop result register is otherwise discarded (same
332+
// situation as `some ... in`). Guard on it so a non-matching indexed
333+
// reference fails the enclosing `every` iteration.
334+
if matches!(
335+
self.context_stack.last().map(|c| &c.context_type),
336+
Some(ContextType::Every)
337+
) {
338+
self.emit_instruction(
339+
Instruction::Guard {
340+
register: result_reg,
341+
mode: GuardMode::Condition,
342+
},
343+
collection.span(),
344+
);
345+
}
346+
315347
Ok(())
316348
}
317349

src/languages/rego/compiler/queries.rs

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -70,12 +70,31 @@ impl<'a> Compiler<'a> {
7070
..
7171
} = &stmt.literal
7272
{
73-
self.compile_some_in_loop_with_remaining_statements(
73+
let some_result_reg = self.compile_some_in_loop_with_remaining_statements(
7474
key,
7575
value,
7676
collection,
7777
&stmts[idx..],
7878
)?;
79+
// Inside an `every` body a `some ... in` acts as a condition
80+
// on the current iteration: if it matches nothing the
81+
// iteration must fail. Unlike a top-level rule body (where
82+
// per-iteration context yields produce the results), the
83+
// `every` body has no yield, so the loop result register is
84+
// otherwise discarded. Guard on it so a `some` that matches
85+
// nothing fails the enclosing `every` iteration.
86+
if matches!(
87+
self.context_stack.last().map(|c| &c.context_type),
88+
Some(ContextType::Every)
89+
) {
90+
self.emit_instruction(
91+
Instruction::Guard {
92+
register: some_result_reg,
93+
mode: GuardMode::Condition,
94+
},
95+
&stmt.span,
96+
);
97+
}
7998
return Ok(());
8099
}
81100
}

src/rvm/vm/loops.rs

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -495,8 +495,15 @@ impl RegoVM {
495495
// over a virtual null element.
496496
Ok(Some(IterationState::Single { consumed: false }))
497497
} else {
498-
// Standard Rego or count/forEach: non-collection → immediate result.
499-
let result = non_collection_result(mode);
498+
// Standard Rego: iterating a non-collection scalar (number,
499+
// string, bool, null, undefined) yields no iterations. For
500+
// `every` this makes the quantifier undefined (it fails) — it
501+
// is NOT vacuously true, which only applies to a genuinely
502+
// empty collection. `any`/`forEach` remain false.
503+
let result = match *mode {
504+
LoopMode::Every => Value::Undefined,
505+
LoopMode::Any | LoopMode::ForEach => Value::Bool(false),
506+
};
500507
self.set_register(params.result_reg, result)?;
501508
self.pc = usize::from(params.loop_end).saturating_sub(1);
502509
Ok(None)

tests/opa.rs

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@ const OPA_TODO_FOLDERS: &[&str] = &[
2626
"baseandvirtualdocs",
2727
"dataderef",
2828
"defaultkeyword",
29-
"every",
3029
"fix1863",
3130
"functions",
3231
"partialdocconstants",
@@ -102,6 +101,20 @@ fn log_rvm_skip(case_note: &str, folder_name: Option<&str>) {
102101
}
103102
}
104103

104+
/// Allows temporarily enabling RVM verification for folders otherwise listed in
105+
/// `OPA_TODO_FOLDERS`, without editing the source. Set `OPA_UNSKIP_FOLDERS` to a
106+
/// comma-separated list of folder names (e.g. `every,functions`) or `all`.
107+
/// Intended for auditing latent RVM bugs in currently-skipped constructs.
108+
fn folder_rvm_unskipped(folder: &str) -> bool {
109+
match std::env::var("OPA_UNSKIP_FOLDERS") {
110+
Ok(list) => {
111+
let list = list.trim();
112+
list.eq_ignore_ascii_case("all") || list.split(',').any(|f| f.trim() == folder)
113+
}
114+
Err(_) => false,
115+
}
116+
}
117+
105118
fn setup_engine_for_case(case: &TestCase, is_rego_v0_test: bool) -> Result<EngineSetup> {
106119
let mut engine = Engine::new();
107120

@@ -388,7 +401,7 @@ fn run_opa_tests(opa_tests_dir: String, folders: &[String]) -> Result<()> {
388401
let folder_name = folder_name_from_path(path_dir);
389402
let skip_rvm_for_folder = folder_name
390403
.as_deref()
391-
.map(|folder| OPA_TODO_FOLDERS.contains(&folder))
404+
.map(|folder| OPA_TODO_FOLDERS.contains(&folder) && !folder_rvm_unskipped(folder))
392405
.unwrap_or(false);
393406

394407
if path.is_dir() {

0 commit comments

Comments
 (0)