From 8ee49f32cc1091defac5b86fe96a5327c3cfd215 Mon Sep 17 00:00:00 2001 From: Maksym Mishchenko Date: Wed, 23 Sep 2026 12:34:14 +0200 Subject: [PATCH 1/2] fix: keep prior rule locations in concise conflict diagnostics Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- CHANGELOG.md | 4 + bindings/csharp/Regorus.Tests/RegorusTests.cs | 24 +++++ bindings/ffi/src/engine.rs | 40 +++++++- src/interpreter.rs | 6 +- src/tests/interpreter/mod.rs | 52 ++++++++++ .../cases/rule/multiple_outputs.yaml | 8 +- tests/rvm/compiler.rs | 96 +++++++++++++++++++ 7 files changed, 222 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 488645efb..39078a58a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- Rule-conflict diagnostics now retain the current source envelope and identify the previous rule by file, line, and column without nesting a second diagnostic. RVM conflict result semantics are unchanged. + ## [0.12.0](https://github.com/microsoft/regorus/compare/regorus-v0.11.0...regorus-v0.12.0) - 2026-09-01 ### Added diff --git a/bindings/csharp/Regorus.Tests/RegorusTests.cs b/bindings/csharp/Regorus.Tests/RegorusTests.cs index c4aa7ca4f..6fabdd4fd 100644 --- a/bindings/csharp/Regorus.Tests/RegorusTests.cs +++ b/bindings/csharp/Regorus.Tests/RegorusTests.cs @@ -27,6 +27,30 @@ public void Basic_evaluation_succeeds() Assert.AreEqual("\"Hello\"", result); } + [TestMethod] + public void Rule_conflict_preserves_error_status_and_reports_previous_location() + { + using var engine = new Engine(); + engine.AddPolicy( + @"C:\policy files\first.rego", + "package test\np := 1\n"); + engine.AddPolicy( + @"C:\policy files\second.rego", + "package test\np := 2\n"); + + var ex = Assert.ThrowsException( + () => engine.EvalRule("data.test.p")); + + StringAssert.Contains( + ex.Message, + @"rule conflicts with rule at C:\policy files\first.rego:2:1"); + StringAssert.Contains(ex.Message, @"C:\policy files\second.rego:2:1"); + StringAssert.Contains(ex.Message, "p := 2"); + Assert.IsFalse(ex.Message.Contains("p := 1", StringComparison.Ordinal)); + Assert.IsFalse(ex.Message.Contains("defined here", StringComparison.Ordinal)); + Assert.IsFalse(ex.Message.Contains('"')); + } + [TestMethod] public void Evaluation_using_file_policies_succeeds() { diff --git a/bindings/ffi/src/engine.rs b/bindings/ffi/src/engine.rs index 52016cde5..7bdd143de 100644 --- a/bindings/ffi/src/engine.rs +++ b/bindings/ffi/src/engine.rs @@ -81,8 +81,8 @@ mod tests { #[cfg(all(test, feature = "std"))] mod panic_tests { use super::{ - regorus_engine_drop, regorus_engine_eval_query, regorus_engine_get_policies, - regorus_engine_new, + regorus_engine_add_policy, regorus_engine_drop, regorus_engine_eval_query, + regorus_engine_eval_rule, regorus_engine_get_policies, regorus_engine_new, }; use crate::common::{regorus_result_drop, RegorusStatus}; use crate::panic_guard::{is_poisoned, reset_poison}; @@ -90,6 +90,42 @@ mod panic_tests { use regorus::Value; use std::ffi::{CStr, CString}; + #[test] + fn rule_conflict_preserves_error_status_and_message() { + let _poison_test_lock = crate::panic_guard::lock_poison_test_state(); + crate::panic_guard::reset_poison(); + + let engine = regorus_engine_new(); + assert!(!engine.is_null()); + + let first_path = CString::new(r"C:\policy files\first.rego").unwrap(); + let first_policy = CString::new("package test\np := 1\n").unwrap(); + let second_path = CString::new(r"C:\policy files\second.rego").unwrap(); + let second_policy = CString::new("package test\np := 2\n").unwrap(); + let query = CString::new("data.test.p").unwrap(); + + let first_result = + regorus_engine_add_policy(engine, first_path.as_ptr(), first_policy.as_ptr()); + assert!(matches!(first_result.status, RegorusStatus::Ok)); + regorus_result_drop(first_result); + + let second_result = + regorus_engine_add_policy(engine, second_path.as_ptr(), second_policy.as_ptr()); + assert!(matches!(second_result.status, RegorusStatus::Ok)); + regorus_result_drop(second_result); + + let result = regorus_engine_eval_rule(engine, query.as_ptr()); + assert!(matches!(result.status, RegorusStatus::Error)); + assert!(!result.error_message.is_null()); + unsafe { + let message = CStr::from_ptr(result.error_message).to_str().unwrap(); + assert!(message.contains(r"rule conflicts with rule at C:\policy files\first.rego:2:1")); + assert!(!message.contains("defined here")); + regorus_result_drop(result); + } + regorus_engine_drop(engine); + } + #[test] fn catches_extension_panics_and_marks_poison() { let _poison_test_lock = crate::panic_guard::lock_poison_test_state(); diff --git a/src/interpreter.rs b/src/interpreter.rs index a4b8d565c..e91615fba 100644 --- a/src/interpreter.rs +++ b/src/interpreter.rs @@ -3888,8 +3888,10 @@ impl Interpreter { if let Some((_, r)) = conflict { bail!(refr.span().error(&format!( - "rule conflicts with the following rule:\n{}", - r.span().message("", "defined here") + "rule conflicts with rule at {}:{}:{}", + r.span().source.file(), + r.span().line, + r.span().col ))); } self.rule_values diff --git a/src/tests/interpreter/mod.rs b/src/tests/interpreter/mod.rs index 0498962f0..f6768ad51 100644 --- a/src/tests/interpreter/mod.rs +++ b/src/tests/interpreter/mod.rs @@ -30,6 +30,58 @@ use timer_test_support::{ apply_engine_timer, configure_time_source, reset_time_source, GlobalTimerGuard, }; +#[test] +fn rule_conflict_reports_previous_rule_location_without_nested_diagnostic() { + let mut engine = Engine::new(); + engine + .add_policy( + r"C:\policy files\first.rego".to_string(), + "package test\np := 1\n".to_string(), + ) + .unwrap(); + engine + .add_policy( + r"C:\policy files\second.rego".to_string(), + "package test\np := 2\n".to_string(), + ) + .unwrap(); + + let error = engine.eval_rule("data.test.p".to_string()).unwrap_err(); + let message = error.to_string(); + assert!(message.contains("--> C:\\policy files\\second.rego:2:1")); + assert!(message.contains("| p := 2")); + assert!(message.contains("error: rule conflicts with rule at C:\\policy files\\first.rego:2:1")); + assert_eq!(message.matches("\n--> ").count(), 1); + assert_eq!(message.matches("| ^").count(), 1); + assert_eq!(message.matches("error: ").count(), 1); + assert!(!message.contains("defined here")); + assert!(!message.contains("\n--> C:\\policy files\\first.rego:2:1")); + assert!(!message.contains("p := 1")); + assert!(!message.contains('"')); +} + +#[test] +fn rule_conflict_reports_current_location_for_same_file_rules() { + let mut engine = Engine::new(); + engine + .add_policy( + r"C:\policy files\same.rego".to_string(), + "package test\np := 1\np := 2\n".to_string(), + ) + .unwrap(); + + let error = engine.eval_rule("data.test.p".to_string()).unwrap_err(); + let message = error.to_string(); + assert!(message.contains("--> C:\\policy files\\same.rego:3:1")); + assert!(message.contains("| p := 2")); + assert!(message.contains("error: rule conflicts with rule at C:\\policy files\\same.rego:2:1")); + assert_eq!(message.matches("\n--> ").count(), 1); + assert_eq!(message.matches("| ^").count(), 1); + assert_eq!(message.matches("error: ").count(), 1); + assert!(!message.contains("p := 1")); + assert!(!message.contains('"')); +} + mod timer_test_support { use super::{ExecutionTimerTestConfig, TimeSourceTestConfig}; #[cfg(any(test, not(feature = "std")))] diff --git a/tests/interpreter/cases/rule/multiple_outputs.yaml b/tests/interpreter/cases/rule/multiple_outputs.yaml index 25b08cfbc..9c84e18c8 100644 --- a/tests/interpreter/cases/rule/multiple_outputs.yaml +++ b/tests/interpreter/cases/rule/multiple_outputs.yaml @@ -109,7 +109,7 @@ cases: p["a"] := 2 query: data.test.p - error: "rule conflicts with the following rule" + error: "rule conflicts with rule at" - note: partial_object_same_key_object_values_conflict_no_deep_merge data: {} @@ -122,7 +122,7 @@ cases: p["a"] := {"y": 2} query: data.test.p - error: "rule conflicts with the following rule" + error: "rule conflicts with rule at" # ---------------------------------------------------------------------------- # Ref-head rules: combine across disjoint sub-paths. @@ -154,7 +154,7 @@ cases: p.q.r := 2 query: data.test.p - error: "rule conflicts with the following rule" + error: "rule conflicts with rule at" - note: refhead_same_node_object_values_conflict_no_deep_merge data: {} @@ -167,7 +167,7 @@ cases: p.q := {"s": 2} query: data.test.p - error: "rule conflicts with the following rule" + error: "rule conflicts with rule at" # ---------------------------------------------------------------------------- # Dynamic partial objects (keys computed at eval time). diff --git a/tests/rvm/compiler.rs b/tests/rvm/compiler.rs index 47bc39831..f0b8012f3 100644 --- a/tests/rvm/compiler.rs +++ b/tests/rvm/compiler.rs @@ -4,6 +4,7 @@ use regorus::languages::rego::compiler::Compiler; use regorus::rvm::instructions::GuardMode; +use regorus::rvm::vm::{RegoVM, VmError}; use regorus::rvm::Instruction; use regorus::{Engine, Rc, Value}; use std::collections::BTreeSet; @@ -50,6 +51,101 @@ fn assert_literal_exists(program: ®orus::rvm::program::Program, expected: &Va ); } +#[test] +fn rule_data_conflict_preserves_error_identity_and_pc() { + let program = compile_rule( + r#" + package test + p := 1 + "#, + ); + let mut vm = RegoVM::new(); + vm.load_program(program); + + let error = vm + .set_data(Value::from_json_str(r#"{"test":{"p":2}}"#).unwrap()) + .unwrap_err(); + assert!(matches!( + error, + VmError::RuleDataConflict { ref message, pc: 0 } + if message.contains("rule defines path 'test.p'") + )); +} + +fn run_rvm_policy(module: &str, input: &str) -> anyhow::Result { + let program = compile_rule(module); + let mut vm = RegoVM::new(); + vm.load_program(program); + vm.set_input(Value::from_json_str(input)?); + Ok(vm.execute_entry_point_by_name("data.test.p")?) +} + +fn run_interpreter_policy(module: &str, input: &str) -> anyhow::Result { + let mut engine = Engine::new(); + engine.add_policy("test.rego".to_string(), module.to_string())?; + engine.set_input(Value::from_json_str(input)?); + engine.eval_rule("data.test.p".to_string()) +} + +#[test] +fn complete_rule_conflict_is_undefined_in_rvm_and_interpreter() { + let module = r#" + package test + p := input.left if input.enabled + p := input.right if input.enabled + "#; + + let conflict_input = r#"{"enabled":true,"left":1,"right":2}"#; + assert_eq!( + run_rvm_policy(module, conflict_input).unwrap(), + Value::Undefined + ); + let interpreter_error = run_interpreter_policy(module, conflict_input) + .unwrap_err() + .to_string(); + assert!( + interpreter_error.contains("rule conflicts"), + "unexpected interpreter error: {interpreter_error}" + ); + + let equal_input = r#"{"enabled":true,"left":1,"right":1}"#; + assert_eq!(run_rvm_policy(module, equal_input).unwrap(), Value::from(1)); + assert_eq!( + run_interpreter_policy(module, equal_input).unwrap(), + Value::from(1) + ); +} + +#[test] +fn function_rule_conflict_is_undefined_in_rvm_and_interpreter() { + let module = r#" + package test + f(x) := input.left if input.enabled + f(x) := input.right if input.enabled + p := f(1) + "#; + + let conflict_input = r#"{"enabled":true,"left":1,"right":2}"#; + assert_eq!( + run_rvm_policy(module, conflict_input).unwrap(), + Value::Undefined + ); + let interpreter_error = run_interpreter_policy(module, conflict_input) + .unwrap_err() + .to_string(); + assert!( + interpreter_error.contains("functions must not produce multiple outputs"), + "unexpected interpreter error: {interpreter_error}" + ); + + let equal_input = r#"{"enabled":true,"left":1,"right":1}"#; + assert_eq!(run_rvm_policy(module, equal_input).unwrap(), Value::from(1)); + assert_eq!( + run_interpreter_policy(module, equal_input).unwrap(), + Value::from(1) + ); +} + #[test] fn constant_array_is_hoisted() { let program = compile_rule( From 47f45bf5cf909d67339da24de3d579d5247ff771 Mon Sep 17 00:00:00 2001 From: Maksym Mishchenko Date: Wed, 23 Sep 2026 17:08:55 +0200 Subject: [PATCH 2/2] fix: escape line breaks in prior rule locations Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- CHANGELOG.md | 2 +- src/interpreter.rs | 6 +++- src/tests/interpreter/mod.rs | 68 ++++++++++++++++++++++++++++++++++++ 3 files changed, 74 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 39078a58a..01c038598 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed -- Rule-conflict diagnostics now retain the current source envelope and identify the previous rule by file, line, and column without nesting a second diagnostic. RVM conflict result semantics are unchanged. +- Rule-conflict diagnostics now retain the current source envelope and identify the previous rule by file, line, and column without nesting a second diagnostic; literal CR/LF characters in the previous file label are escaped. RVM conflict result semantics are unchanged. ## [0.12.0](https://github.com/microsoft/regorus/compare/regorus-v0.11.0...regorus-v0.12.0) - 2026-09-01 diff --git a/src/interpreter.rs b/src/interpreter.rs index e91615fba..0ac20eb75 100644 --- a/src/interpreter.rs +++ b/src/interpreter.rs @@ -3889,7 +3889,11 @@ impl Interpreter { if let Some((_, r)) = conflict { bail!(refr.span().error(&format!( "rule conflicts with rule at {}:{}:{}", - r.span().source.file(), + r.span() + .source + .file() + .replace('\n', "\\n") + .replace('\r', "\\r"), r.span().line, r.span().col ))); diff --git a/src/tests/interpreter/mod.rs b/src/tests/interpreter/mod.rs index f6768ad51..4bdaa7017 100644 --- a/src/tests/interpreter/mod.rs +++ b/src/tests/interpreter/mod.rs @@ -82,6 +82,74 @@ fn rule_conflict_reports_current_location_for_same_file_rules() { assert!(!message.contains('"')); } +#[test] +fn rule_conflict_reports_escaped_newline_in_previous_rule_file_label() { + let mut engine = Engine::new(); + engine + .add_policy( + "C:\\policy files\\first\nsplit.rego".to_string(), + "package test\np := 1\n".to_string(), + ) + .unwrap(); + engine + .add_policy( + r"C:\policy files\second.rego".to_string(), + "package test\np := 2\n".to_string(), + ) + .unwrap(); + + let error = engine.eval_rule("data.test.p".to_string()).unwrap_err(); + let message = error.to_string(); + let conflict_line = message + .lines() + .find(|line| line.starts_with("error: rule conflicts")) + .unwrap(); + assert_eq!( + conflict_line, + "error: rule conflicts with rule at C:\\policy files\\first\\nsplit.rego:2:1" + ); + assert!(message.contains("--> C:\\policy files\\second.rego:2:1")); + assert!(message.contains("| p := 2")); + assert!(!conflict_line.contains('\r')); + assert!(!conflict_line.contains('\n')); + assert!(!message.contains("defined here")); + assert!(!message.contains('"')); +} + +#[test] +fn rule_conflict_reports_escaped_carriage_return_in_previous_rule_file_label() { + let mut engine = Engine::new(); + engine + .add_policy( + "C:\\policy files\\first\rsplit.rego".to_string(), + "package test\np := 1\n".to_string(), + ) + .unwrap(); + engine + .add_policy( + r"C:\policy files\second.rego".to_string(), + "package test\np := 2\n".to_string(), + ) + .unwrap(); + + let error = engine.eval_rule("data.test.p".to_string()).unwrap_err(); + let message = error.to_string(); + let conflict_line = message + .lines() + .find(|line| line.starts_with("error: rule conflicts")) + .unwrap(); + assert_eq!( + conflict_line, + "error: rule conflicts with rule at C:\\policy files\\first\\rsplit.rego:2:1" + ); + assert!(message.contains("--> C:\\policy files\\second.rego:2:1")); + assert!(message.contains("| p := 2")); + assert!(!conflict_line.contains('\r')); + assert!(!conflict_line.contains('\n')); + assert!(!message.contains("defined here")); + assert!(!message.contains('"')); +} + mod timer_test_support { use super::{ExecutionTimerTestConfig, TimeSourceTestConfig}; #[cfg(any(test, not(feature = "std")))]