Skip to content

Add RVM/interpreter incompatibility tests and compiler error detection #666

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
anakrish wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Add RVM/interpreter incompatibility tests and compiler error detection #666

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions src/languages/rego/compiler/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,17 @@ pub enum CompilerError {
#[error("SomeIn should have been hoisted as a loop")]
SomeInNotHoisted,

#[error("function default values are not yet supported in the RVM compiler")]
FunctionDefaultsUnsupported,

#[error(
"comprehension expressions in default values are not yet supported in the RVM compiler"
)]
ComprehensionInDefaultUnsupported,

#[error("multi-value ref head rules are not yet supported in the RVM compiler")]
MultiValueRefHeadUnsupported,

#[error("Invalid function expression")]
InvalidFunctionExpression,

Expand Down
29 changes: 29 additions & 0 deletions src/languages/rego/compiler/program.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
)]

use super::{Compiler, CompilerError, Result};
use crate::ast::{Expr, Rule};
use crate::interpreter::Interpreter;
use crate::rvm::program::{Program, RuleType, SpanInfo};
use crate::rvm::Instruction;
Expand Down Expand Up @@ -134,6 +135,34 @@ impl<'a> Compiler<'a> {
rule_infos_map.insert(rule_index as usize, rule_info);
}

// Validate unsupported default rule patterns before evaluation.
// Only check rules in rule_index_map — rules outside the current
// compilation scope (e.g., unreachable packages) should not cause
// spurious errors for valid policies.
for (rule_path, default_infos) in self.policy.inner.default_rules.iter() {
if !self.rule_index_map.contains_key(rule_path) {
continue;
}
Comment thread
anakrish marked this conversation as resolved.
for (rule_ref, _) in default_infos {
if let Rule::Default {
args, value, span, ..
} = rule_ref.as_ref()
{
if !args.is_empty() {
return Err(CompilerError::FunctionDefaultsUnsupported.at(span));
}
match value.as_ref() {
Expr::ArrayCompr { .. }
| Expr::SetCompr { .. }
| Expr::ObjectCompr { .. } => {
return Err(CompilerError::ComprehensionInDefaultUnsupported.at(span));
}
_ => {}
}
}
}
}
Comment thread
anakrish marked this conversation as resolved.

let rule_paths_to_evaluate: Vec<(String, usize)> = self
.rule_index_map
.iter()
Expand Down
33 changes: 32 additions & 1 deletion src/languages/rego/compiler/rules.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,15 @@ impl<'a> Compiler<'a> {
}
}

/// Check if a ref expression contains any `RefBrack` node (bracket indexing).
fn ref_contains_bracket(expr: &Expr) -> bool {
match expr {
Expr::RefBrack { .. } => true,
Expr::RefDot { refr, .. } => Self::ref_contains_bracket(refr.as_ref()),
_ => false,
}
}

pub(super) fn compute_rule_type(&self, rule_path: &str) -> Result<RuleType> {
let Some(definitions) = self.policy.inner.rules.get(rule_path) else {
// Default-only rules (e.g., `default deny := true`) have no regular definitions
Expand Down Expand Up @@ -345,6 +354,23 @@ impl<'a> Compiler<'a> {

let (key_expr, value_expr) = match head {
RuleHead::Compr { refr, assign, .. } => {
// Detect multi-key ref heads: p[q][r] or p[q].r
// The outermost may be RefBrack (p[q][r]) or RefDot (p[q].r).
// In either case, if stripping the outermost layer reveals
// a bracket underneath, it's a multi-key ref head.
let has_inner_bracket = match refr.as_ref() {
Expr::RefBrack { refr: inner, .. } => {
Self::ref_contains_bracket(inner.as_ref())
}
Expr::RefDot { refr: inner, .. } => {
Self::ref_contains_bracket(inner.as_ref())
}
_ => false,
};
if has_inner_bracket {
return Err(CompilerError::MultiValueRefHeadUnsupported.at(span));
}

self.rule_definition_function_params[rule_index as usize].push(None);
self.rule_definition_destructuring_patterns[rule_index as usize]
.push(None);
Expand All @@ -356,7 +382,12 @@ impl<'a> Compiler<'a> {
};
(key_expr, output_expr)
}
RuleHead::Set { key, .. } => {
RuleHead::Set { refr, key, .. } => {
// Detect ref head sets with bracket indexing: p[q] contains r
if Self::ref_contains_bracket(refr.as_ref()) {
return Err(CompilerError::MultiValueRefHeadUnsupported.at(span));
}

self.rule_definition_function_params[rule_index as usize].push(None);
self.rule_definition_destructuring_patterns[rule_index as usize]
.push(None);
Expand Down
215 changes: 168 additions & 47 deletions tests/opa.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,19 +22,6 @@ const PARTIAL_OBJECT_OVERRIDE_NOTE: &str =
"regression/partial-object override, different key type, query";

const OPA_TODO_FOLDERS: &[&str] = &[
"aggregates",
"baseandvirtualdocs",
"dataderef",
"defaultkeyword",
"every",
"fix1863",
"functions",
"partialdocconstants",
"partialobjectdoc",
"planner-ir",
"refheads",
"type",
"walkbuiltin",
// RVM Compiler does not support 'with' keyword yet.
"withkeyword",
];
Expand Down Expand Up @@ -262,24 +249,175 @@ fn eval_rule_with_rvm(case: &TestCase, is_rego_v0_test: bool, rule_path: &str) -
}
}

fn is_not_valid_rule_path_error(err: &anyhow::Error) -> bool {
err.chain()
.any(|cause| cause.to_string().contains("not a valid rule path"))
fn compiler_limitation_reason(err: &anyhow::Error) -> Option<&'static str> {
let err_str = format!("{:#}", err);
let patterns: &[(&str, &str)] = &[
("not a valid rule path", "rule path not compiled"),
(
"`with` keyword is not supported",
"with keyword unsupported",
),
(
"SomeIn should have been hoisted",
"some-in loop hoisting unsupported",
),
(
"walk loops are not yet supported",
"walk builtin unsupported",
),
(
"function default values are not yet supported",
"function defaults unsupported",
),
(
"comprehension expressions in default values are not yet supported",
"comprehension defaults unsupported",
),
("recursion detected", "compile-time recursion"),
("Undefined variable", "variable scoping unsupported"),
("Unknown function", "function compilation unsupported"),
(
"multi-value ref head rules are not yet supported",
"multi-value ref heads unsupported",
),
];
for (pattern, reason) in patterns {
if err_str.contains(pattern) {
return Some(reason);
}
}
Comment thread
anakrish marked this conversation as resolved.
None
}
Comment thread
anakrish marked this conversation as resolved.

fn is_with_keyword_unsupported_error(err: &anyhow::Error) -> bool {
err.chain().any(|cause| {
cause
.to_string()
.contains("`with` keyword is not supported")
})
// Tests where compilation succeeds but the RVM produces incorrect results at runtime.
// Each entry is (note, reason).
const KNOWN_RVM_MISMATCH_NOTES: &[(&str, &str)] = &[
// every quantifier: RVM always evaluates to true, even when it should be false/undefined.
("every/domain undefined (input)", "every false-case RVM bug"),
(
"every/domain undefined (data ref)",
"every false-case RVM bug",
),
("every/simple failure, first", "every false-case RVM bug"),
("every/simple failure, last", "every false-case RVM bug"),
("every/array with calls (fail)", "every false-case RVM bug"),
("every/non-iter domain: int", "every false-case RVM bug"),
("every/non-iter domain: string", "every false-case RVM bug"),
("every/non-iter domain: bool", "every false-case RVM bug"),
("every/non-iter domain: null", "every false-case RVM bug"),
(
"every/non-iter domain: built-in call",
"every false-case RVM bug",
),
(
"every/non-iter domain: function call",
"every false-case RVM bug",
),
(
"every/non-iter domain: rule ref",
"every false-case RVM bug",
),
(
"every/non-iter domain: data int",
"every false-case RVM bug",
),
(
"every/non-iter domain: input int",
"every false-case RVM bug",
),
(
"every/non-iter domain: input int (1st level)",
"every false-case RVM bug",
),
("every/example, fail", "every false-case RVM bug"),
(
"every/example with two sets (fail)",
"every false-case RVM bug",
),
("every/example every/some, fail", "every false-case RVM bug"),
// Suffix lookup / data deref issues: RVM can't dereference into partial object values.
(
"data/nested integer",
"integer key data deref not supported",
),
(
"fix1863/is defined",
"empty package as object not supported",
),
(
"partialdocconstants/obj-1",
"suffix lookup on bracket-key partial object",
),
(
"wasm/object dereference",
"suffix lookup through partial object value",
),
// Dynamic lookup on mixed partial rules / ref heads.
(
"ir/call-dynamic with mixed partial rules",
"dynamic lookup on mixed partial rules",
),
(
"ir/call-dynamic with mixed partial rules, ref heads",
"dynamic lookup on mixed partial rules with ref heads",
),
(
"ir/call-dynamic with ref heads, issue 5839, penultimate",
"ref heads dynamic lookup",
),
// Suffix lookup on partial object override.
(
PARTIAL_OBJECT_OVERRIDE_NOTE,
"suffix lookup on partial object override",
),
// Base and virtual document interop: RVM can't merge base/virtual data correctly.
(
"baseandvirtualdocs/base/virtual: ground key",
"base/virtual document merge",
),
(
"baseandvirtualdocs/base/virtual: prefix",
"base/virtual document merge",
),
(
"baseandvirtualdocs/base/virtual: undefined",
"base/virtual document merge",
),
(
"baseandvirtualdocs/base/virtual: undefined-2",
"base/virtual document merge",
),
(
"baseandvirtualdocs/base/virtual: missing input value",
"base/virtual document merge",
),
// Ref heads: mixed set/object rules on same path, dynamic cross-rule lookup.
(
"refheads/general, set leaf (other rule defines dynamic ref portion)",
"mixed set/partial-object rules on same path",
),
(
"refheads/single-value example",
"dynamic cross-rule bracket lookup",
),
(
"refheads/single-value example, false",
"dynamic cross-rule bracket lookup",
),
];

fn known_rvm_mismatch_reason(note: &str) -> Option<&'static str> {
KNOWN_RVM_MISMATCH_NOTES
.iter()
.find(|(n, _)| *n == note)
.map(|(_, reason)| *reason)
Comment thread
anakrish marked this conversation as resolved.
}

fn maybe_verify_rvm_case(case: &TestCase, is_rego_v0_test: bool, actual: &Value) -> Result<()> {
if case.note == "defaultkeyword/function with var arg, ref head query" {
if let Some(reason) = known_rvm_mismatch_reason(&case.note) {
println!(
" skipping RVM check for '{}' (function defaults with refs unsupported)",
case.note
" skipping RVM check for '{}' (known RVM mismatch: {})",
case.note, reason
);
return Ok(());
}
Expand All @@ -292,19 +430,8 @@ fn maybe_verify_rvm_case(case: &TestCase, is_rego_v0_test: bool, actual: &Value)
let rvm_value = match eval_rule_with_rvm(case, is_rego_v0_test, &rule_path) {
Ok(value) => value,
Err(err) => {
if is_not_valid_rule_path_error(&err) {
println!(
" skipping RVM check for '{}' (rule path not compiled)",
case.note
);
return Ok(());
}

if is_with_keyword_unsupported_error(&err) {
println!(
" skipping RVM check for '{}' (with keyword unsupported)",
case.note
);
if let Some(reason) = compiler_limitation_reason(&err) {
println!(" skipping RVM check for '{}' ({})", case.note, reason);
return Ok(());
}

Expand Down Expand Up @@ -392,15 +519,9 @@ fn run_opa_tests(opa_tests_dir: String, folders: &[String]) -> Result<()> {
for mut case in test.cases {
let is_json_schema_test = case.note.starts_with("json_verify_schema")
|| case.note.starts_with("json_match_schema");
let mut skip_rvm_validation = skip_rvm_for_folder;

if case.note == PARTIAL_OBJECT_OVERRIDE_NOTE {
println!(
" skipping RVM check for '{}' (needs suffix lookup on rule path)",
case.note
);
skip_rvm_validation = true;
} else if case.note == "reachable_paths/cycle_1022_3" {
let skip_rvm_validation = skip_rvm_for_folder;

if case.note == "reachable_paths/cycle_1022_3" {
// The OPA behavior is not well-defined.
// See: https://github.com/open-policy-agent/opa/issues/5871
// https://github.com/open-policy-agent/opa/issues/6128
Expand Down
Loading
Loading