mirror of
https://github.com/microsoft/regorus.git
synced 2026-08-05 02:16:11 +00:00
fix(rvm): assert every-quantifier results so failing cases don't pass (#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.
This commit is contained in:
committed by
GitHub
parent
9a486c79bf
commit
6ef5e74eb2
@@ -13,7 +13,7 @@ use crate::ast::{self, ExprRef, LiteralStmt, Query};
|
||||
use crate::compiler::destructuring_planner::plans::BindingPlan;
|
||||
use crate::compiler::hoist::{HoistedLoop, LoopType};
|
||||
use crate::lexer::Span;
|
||||
use crate::rvm::instructions::{LoopMode, LoopStartParams};
|
||||
use crate::rvm::instructions::{GuardMode, LoopMode, LoopStartParams};
|
||||
use crate::rvm::Instruction;
|
||||
use crate::Value;
|
||||
use alloc::format;
|
||||
@@ -197,6 +197,19 @@ impl<'a> Compiler<'a> {
|
||||
*end = loop_end;
|
||||
}
|
||||
|
||||
// The loop writes its overall pass/fail into `result_reg`
|
||||
// (`success_count == total_iterations` for `Every`). The enclosing query
|
||||
// must fail (evaluate to undefined) when the quantifier does not hold, so
|
||||
// guard on `result_reg` here. Without this the `every` result is computed
|
||||
// but discarded, leaving the surrounding rule to always succeed.
|
||||
self.emit_instruction(
|
||||
Instruction::Guard {
|
||||
register: result_reg,
|
||||
mode: GuardMode::Condition,
|
||||
},
|
||||
span,
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
@@ -312,6 +325,25 @@ impl<'a> Compiler<'a> {
|
||||
*end = loop_end;
|
||||
}
|
||||
|
||||
// A hoisted index-iteration loop inside an `every` body acts as a
|
||||
// condition on the current iteration: if the indexed reference matches
|
||||
// nothing the iteration must fail. The `every` body emits no context
|
||||
// yield, so the loop result register is otherwise discarded (same
|
||||
// situation as `some ... in`). Guard on it so a non-matching indexed
|
||||
// reference fails the enclosing `every` iteration.
|
||||
if matches!(
|
||||
self.context_stack.last().map(|c| &c.context_type),
|
||||
Some(ContextType::Every)
|
||||
) {
|
||||
self.emit_instruction(
|
||||
Instruction::Guard {
|
||||
register: result_reg,
|
||||
mode: GuardMode::Condition,
|
||||
},
|
||||
collection.span(),
|
||||
);
|
||||
}
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
|
||||
@@ -70,12 +70,31 @@ impl<'a> Compiler<'a> {
|
||||
..
|
||||
} = &stmt.literal
|
||||
{
|
||||
self.compile_some_in_loop_with_remaining_statements(
|
||||
let some_result_reg = self.compile_some_in_loop_with_remaining_statements(
|
||||
key,
|
||||
value,
|
||||
collection,
|
||||
&stmts[idx..],
|
||||
)?;
|
||||
// Inside an `every` body a `some ... in` acts as a condition
|
||||
// on the current iteration: if it matches nothing the
|
||||
// iteration must fail. Unlike a top-level rule body (where
|
||||
// per-iteration context yields produce the results), the
|
||||
// `every` body has no yield, so the loop result register is
|
||||
// otherwise discarded. Guard on it so a `some` that matches
|
||||
// nothing fails the enclosing `every` iteration.
|
||||
if matches!(
|
||||
self.context_stack.last().map(|c| &c.context_type),
|
||||
Some(ContextType::Every)
|
||||
) {
|
||||
self.emit_instruction(
|
||||
Instruction::Guard {
|
||||
register: some_result_reg,
|
||||
mode: GuardMode::Condition,
|
||||
},
|
||||
&stmt.span,
|
||||
);
|
||||
}
|
||||
return Ok(());
|
||||
}
|
||||
}
|
||||
|
||||
@@ -495,8 +495,15 @@ impl RegoVM {
|
||||
// over a virtual null element.
|
||||
Ok(Some(IterationState::Single { consumed: false }))
|
||||
} else {
|
||||
// Standard Rego or count/forEach: non-collection → immediate result.
|
||||
let result = non_collection_result(mode);
|
||||
// Standard Rego: iterating a non-collection scalar (number,
|
||||
// string, bool, null, undefined) yields no iterations. For
|
||||
// `every` this makes the quantifier undefined (it fails) — it
|
||||
// is NOT vacuously true, which only applies to a genuinely
|
||||
// empty collection. `any`/`forEach` remain false.
|
||||
let result = match *mode {
|
||||
LoopMode::Every => Value::Undefined,
|
||||
LoopMode::Any | LoopMode::ForEach => Value::Bool(false),
|
||||
};
|
||||
self.set_register(params.result_reg, result)?;
|
||||
self.pc = usize::from(params.loop_end).saturating_sub(1);
|
||||
Ok(None)
|
||||
|
||||
17
tests/opa.rs
17
tests/opa.rs
@@ -26,7 +26,6 @@ const OPA_TODO_FOLDERS: &[&str] = &[
|
||||
"baseandvirtualdocs",
|
||||
"dataderef",
|
||||
"defaultkeyword",
|
||||
"every",
|
||||
"fix1863",
|
||||
"functions",
|
||||
"partialdocconstants",
|
||||
@@ -102,6 +101,20 @@ fn log_rvm_skip(case_note: &str, folder_name: Option<&str>) {
|
||||
}
|
||||
}
|
||||
|
||||
/// Allows temporarily enabling RVM verification for folders otherwise listed in
|
||||
/// `OPA_TODO_FOLDERS`, without editing the source. Set `OPA_UNSKIP_FOLDERS` to a
|
||||
/// comma-separated list of folder names (e.g. `every,functions`) or `all`.
|
||||
/// Intended for auditing latent RVM bugs in currently-skipped constructs.
|
||||
fn folder_rvm_unskipped(folder: &str) -> bool {
|
||||
match std::env::var("OPA_UNSKIP_FOLDERS") {
|
||||
Ok(list) => {
|
||||
let list = list.trim();
|
||||
list.eq_ignore_ascii_case("all") || list.split(',').any(|f| f.trim() == folder)
|
||||
}
|
||||
Err(_) => false,
|
||||
}
|
||||
}
|
||||
|
||||
fn setup_engine_for_case(case: &TestCase, is_rego_v0_test: bool) -> Result<EngineSetup> {
|
||||
let mut engine = Engine::new();
|
||||
|
||||
@@ -388,7 +401,7 @@ fn run_opa_tests(opa_tests_dir: String, folders: &[String]) -> Result<()> {
|
||||
let folder_name = folder_name_from_path(path_dir);
|
||||
let skip_rvm_for_folder = folder_name
|
||||
.as_deref()
|
||||
.map(|folder| OPA_TODO_FOLDERS.contains(&folder))
|
||||
.map(|folder| OPA_TODO_FOLDERS.contains(&folder) && !folder_rvm_unskipped(folder))
|
||||
.unwrap_or(false);
|
||||
|
||||
if path.is_dir() {
|
||||
|
||||
@@ -44,6 +44,295 @@ cases:
|
||||
query: data.test.main
|
||||
want_result: true
|
||||
|
||||
- note: every_body_fails_for_one_element
|
||||
data: {}
|
||||
modules:
|
||||
- |
|
||||
package test
|
||||
main := result if {
|
||||
every x in [1, 2, 3] {
|
||||
x > 1
|
||||
}
|
||||
result := true
|
||||
}
|
||||
query: data.test.main
|
||||
want_result: "#undefined"
|
||||
|
||||
- note: every_body_fails_for_all_elements
|
||||
data: {}
|
||||
modules:
|
||||
- |
|
||||
package test
|
||||
main := result if {
|
||||
every x in [1, 2, 3] {
|
||||
x > 100
|
||||
}
|
||||
result := true
|
||||
}
|
||||
query: data.test.main
|
||||
want_result: "#undefined"
|
||||
|
||||
- note: every_used_directly_as_condition_false
|
||||
data: {}
|
||||
modules:
|
||||
- |
|
||||
package test
|
||||
allowed if {
|
||||
every x in [1, 2, 3] {
|
||||
x > 1
|
||||
}
|
||||
}
|
||||
main := allowed
|
||||
query: data.test.main
|
||||
want_result: "#undefined"
|
||||
|
||||
- note: every_used_directly_as_condition_true
|
||||
data: {}
|
||||
modules:
|
||||
- |
|
||||
package test
|
||||
allowed if {
|
||||
every x in [1, 2, 3] {
|
||||
x > 0
|
||||
}
|
||||
}
|
||||
main := allowed
|
||||
query: data.test.main
|
||||
want_result: true
|
||||
|
||||
- note: every_over_non_iterable_number_is_undefined
|
||||
data: {}
|
||||
modules:
|
||||
- |
|
||||
package test
|
||||
allowed if {
|
||||
every x in 42 {
|
||||
x > 1
|
||||
}
|
||||
}
|
||||
main := allowed
|
||||
query: data.test.main
|
||||
want_result: "#undefined"
|
||||
|
||||
- note: every_over_non_iterable_string_is_undefined
|
||||
data: {}
|
||||
modules:
|
||||
- |
|
||||
package test
|
||||
allowed if {
|
||||
every x in "hello" {
|
||||
x == x
|
||||
}
|
||||
}
|
||||
main := allowed
|
||||
query: data.test.main
|
||||
want_result: "#undefined"
|
||||
|
||||
- note: every_over_empty_array_is_vacuously_true
|
||||
data: {}
|
||||
modules:
|
||||
- |
|
||||
package test
|
||||
allowed if {
|
||||
every x in [] {
|
||||
x > 1
|
||||
}
|
||||
}
|
||||
main := allowed
|
||||
query: data.test.main
|
||||
want_result: true
|
||||
|
||||
- note: every_with_inner_some_matching_nothing_fails
|
||||
data: {}
|
||||
modules:
|
||||
- |
|
||||
package test
|
||||
allowed if {
|
||||
every c in [1, 2] {
|
||||
some x in []
|
||||
c == x
|
||||
}
|
||||
}
|
||||
main := allowed
|
||||
query: data.test.main
|
||||
want_result: "#undefined"
|
||||
|
||||
- note: every_with_inner_some_matching_succeeds
|
||||
data: {}
|
||||
modules:
|
||||
- |
|
||||
package test
|
||||
allowed if {
|
||||
every c in [1, 2] {
|
||||
some x in [1, 2, 3]
|
||||
c == x
|
||||
}
|
||||
}
|
||||
main := allowed
|
||||
query: data.test.main
|
||||
want_result: true
|
||||
|
||||
- note: every_with_hoisted_index_matching_nothing_fails
|
||||
data: {}
|
||||
modules:
|
||||
- |
|
||||
package test
|
||||
allowed if {
|
||||
every c in [1] {
|
||||
some i
|
||||
[2, 3][i] == c
|
||||
}
|
||||
}
|
||||
main := allowed
|
||||
query: data.test.main
|
||||
want_result: "#undefined"
|
||||
|
||||
- note: every_with_hoisted_index_matching_succeeds
|
||||
data: {}
|
||||
modules:
|
||||
- |
|
||||
package test
|
||||
allowed if {
|
||||
every c in [2, 3] {
|
||||
some i
|
||||
[1, 2, 3][i] == c
|
||||
}
|
||||
}
|
||||
main := allowed
|
||||
query: data.test.main
|
||||
want_result: true
|
||||
|
||||
- note: every_keyval_over_object_succeeds
|
||||
data: {}
|
||||
modules:
|
||||
- |
|
||||
package test
|
||||
allowed if {
|
||||
every k, v in {"a": 1, "b": 2} {
|
||||
v > 0
|
||||
}
|
||||
}
|
||||
main := allowed
|
||||
query: data.test.main
|
||||
want_result: true
|
||||
|
||||
- note: every_keyval_over_object_fails
|
||||
data: {}
|
||||
modules:
|
||||
- |
|
||||
package test
|
||||
allowed if {
|
||||
every k, v in {"a": 1, "b": 2} {
|
||||
v > 1
|
||||
}
|
||||
}
|
||||
main := allowed
|
||||
query: data.test.main
|
||||
want_result: "#undefined"
|
||||
|
||||
- note: every_over_set_domain_succeeds
|
||||
data: {}
|
||||
modules:
|
||||
- |
|
||||
package test
|
||||
allowed if {
|
||||
every x in {1, 2, 3} {
|
||||
x > 0
|
||||
}
|
||||
}
|
||||
main := allowed
|
||||
query: data.test.main
|
||||
want_result: true
|
||||
|
||||
- note: every_over_empty_object_is_vacuously_true
|
||||
data: {}
|
||||
modules:
|
||||
- |
|
||||
package test
|
||||
allowed if {
|
||||
every k, v in {} {
|
||||
v > 1
|
||||
}
|
||||
}
|
||||
main := allowed
|
||||
query: data.test.main
|
||||
want_result: true
|
||||
|
||||
- note: every_over_empty_set_is_vacuously_true
|
||||
data: {}
|
||||
modules:
|
||||
- |
|
||||
package test
|
||||
allowed if {
|
||||
every x in set() {
|
||||
x > 1
|
||||
}
|
||||
}
|
||||
main := allowed
|
||||
query: data.test.main
|
||||
want_result: true
|
||||
|
||||
- note: every_over_undefined_domain_is_undefined
|
||||
data: {}
|
||||
modules:
|
||||
- |
|
||||
package test
|
||||
allowed if {
|
||||
every x in input.missing {
|
||||
x > 0
|
||||
}
|
||||
}
|
||||
main := allowed
|
||||
query: data.test.main
|
||||
want_result: "#undefined"
|
||||
|
||||
- note: every_nested_inside_every_succeeds
|
||||
data: {}
|
||||
modules:
|
||||
- |
|
||||
package test
|
||||
allowed if {
|
||||
every row in [[1, 2], [3, 4]] {
|
||||
every c in row {
|
||||
c > 0
|
||||
}
|
||||
}
|
||||
}
|
||||
main := allowed
|
||||
query: data.test.main
|
||||
want_result: true
|
||||
|
||||
- note: every_nested_inside_every_fails
|
||||
data: {}
|
||||
modules:
|
||||
- |
|
||||
package test
|
||||
allowed if {
|
||||
every row in [[1, 2], [3, 0]] {
|
||||
every c in row {
|
||||
c > 0
|
||||
}
|
||||
}
|
||||
}
|
||||
main := allowed
|
||||
query: data.test.main
|
||||
want_result: "#undefined"
|
||||
|
||||
- note: every_with_outer_binding
|
||||
data: {}
|
||||
modules:
|
||||
- |
|
||||
package test
|
||||
allowed if {
|
||||
threshold := 5
|
||||
every x in [6, 7, 8] {
|
||||
x > threshold
|
||||
}
|
||||
}
|
||||
main := allowed
|
||||
query: data.test.main
|
||||
want_result: true
|
||||
|
||||
- note: simple_loop_test
|
||||
data: {}
|
||||
modules:
|
||||
|
||||
Reference in New Issue
Block a user