Polish keyword-ref test expectations and validate coverage

src/interpreter.rs

@@ -586,7 +586,7 @@ impl Interpreter {
                                 } else {
                                     format!("{ref_path}.{index}.{}", path.join("."))
                                 };
-                                self.ensure_matching_rules_for_dynamic_data_index(&ref_path)?;
+                                self.ensure_rule_evaluated(ref_path)?;
                             }
                         }
 
@@ -2960,136 +2960,6 @@ impl Interpreter {
         Ok(())
     }
 
-    /// Ensures all rule/default-rule paths matching a dynamic data lookup are evaluated.
-    /// Matches both exact path (`data.a.b`) and descendants with the `data.a.b.` prefix.
-    fn ensure_matching_rules_for_dynamic_data_index(&mut self, path: &str) -> Result<()> {
-        self.check_execution_time()?;
-        let path_prefix = format!("{path}.");
-        let mut matching_paths: Vec<String> = self
-            .compiled_policy
-            .default_rules
-            .keys()
-            .chain(self.compiled_policy.rules.keys())
-            .filter(|rule_path| *rule_path == path || rule_path.starts_with(&path_prefix))
-            .cloned()
-            .collect();
-        matching_paths.sort();
-        matching_paths.dedup();
-
-        for rule_path in matching_paths {
-            self.ensure_rule_evaluated(rule_path)?;
-        }
-
-        Ok(())
-    }
-
-    /// Builds a canonical `data` path from field components.
-    /// For an empty field list, returns `"data"`.
-    fn build_data_path(fields: &[&str]) -> String {
-        if fields.is_empty() {
-            "data".to_string()
-        } else {
-            format!("data.{}", fields.join("."))
-        }
-    }
-
-    /// Returns `true` when `prefix` matches `path` on segment boundaries.
-    ///
-    /// Examples:
-    /// - `path_is_prefix("data.auth", "data.auth") == true`
-    /// - `path_is_prefix("data.auth", "data.auth.allow") == true`
-    /// - `path_is_prefix("data.auth", "data.authorization") == false`
-    fn path_is_prefix(prefix: &str, path: &str) -> bool {
-        if path == prefix {
-            return true;
-        }
-        path.get(prefix.len()..)
-            .is_some_and(|suffix| suffix.starts_with('.'))
-    }
-
-    /// Checks whether a `requested_path` can contain values produced by an active rule.
-    ///
-    /// The active rule path is logically `module_path.rule_path`, but this check avoids
-    /// allocating that joined string in tight evaluation loops.
-    ///
-    /// Examples:
-    /// - request `data` matches module `data.authz` (module expansion needed)
-    /// - request `data.authz` matches rule `allow`
-    /// - request `data.authz.allow` matches rule `allow`
-    /// - request `data.auth` does not match module `data.authz`
-    fn request_matches_active_rule_path(
-        requested_path: &str,
-        module_path: &str,
-        rule_path: &str,
-    ) -> bool {
-        if Self::path_is_prefix(requested_path, module_path) {
-            return true;
-        }
-
-        requested_path
-            .strip_prefix(module_path)
-            .and_then(|suffix| suffix.strip_prefix('.'))
-            .is_some_and(|requested_rule_prefix| {
-                Self::path_is_prefix(requested_rule_prefix, rule_path)
-            })
-    }
-
-    fn should_defer_module_eval_for_path(&self, requested_path: &str) -> Result<bool> {
-        for active_rule in &self.active_rules {
-            let module = self.get_rule_module(active_rule)?;
-            let module_path = get_path_string(&module.package.refr, Some("data"))?;
-            let rule_path = get_path_string(Self::get_rule_refr(active_rule), None)?;
-            if Self::request_matches_active_rule_path(requested_path, &module_path, &rule_path) {
-                return Ok(true);
-            }
-        }
-        Ok(false)
-    }
-
-    /// Resolves `data.<fields...>` while preserving correct rule semantics.
-    /// When a rule is already active, this avoids eager module-wide evaluation so legitimate
-    /// cross-package references are not misidentified as cyclic recursion.
-    fn lookup_data_path(&mut self, fields: &[&str]) -> Result<Value> {
-        if self.is_processed(fields)? {
-            return Ok(Self::get_value_chained(self.data.clone(), fields));
-        }
-
-        // If "data" is used in a query without any fields, then evaluate all modules.
-        if fields.is_empty() && self.active_rules.is_empty() {
-            for module in self.compiled_policy.modules.clone().iter() {
-                for rule in &module.policy {
-                    self.eval_rule(module, rule)?;
-                }
-            }
-        }
-
-        // While a rule is active, avoid eagerly evaluating all matching modules.
-        // This prevents re-entry through sibling rules or other modules from being
-        // misclassified as cyclic recursion.
-        let requested_path = Self::build_data_path(fields);
-        if self.active_rules.is_empty()
-            || !self.should_defer_module_eval_for_path(&requested_path)?
-        {
-            self.ensure_module_evaluated(requested_path.clone())?;
-        }
-
-        for i in (1..=fields.len()).rev() {
-            let prefix = fields.iter().take(i).copied().collect::<Vec<_>>();
-            let prefix_path = Self::build_data_path(&prefix);
-            if self.compiled_policy.rules.contains_key(&prefix_path)
-                || self
-                    .compiled_policy
-                    .default_rules
-                    .contains_key(&prefix_path)
-            {
-                self.ensure_rule_evaluated(prefix_path)?;
-                break;
-            }
-        }
-
-        Ok(Self::get_value_chained(self.data.clone(), fields))
-    }
-
     fn is_processed(&self, path: &[&str]) -> Result<bool> {
         let mut obj = &self.processed_paths;
         for p in path {
@@ -3140,7 +3010,39 @@ impl Interpreter {
 
         // Ensure that rules are evaluated
         if name.text() == "data" {
-            self.lookup_data_path(fields)
+            if self.is_processed(fields)? {
+                return Ok(Self::get_value_chained(self.data.clone(), fields));
+            }
+
+            // If "data" is used in a query, without any fields, then evaluate all the modules.
+            if fields.is_empty() && self.active_rules.is_empty() {
+                for module in self.compiled_policy.modules.clone().iter() {
+                    for rule in &module.policy {
+                        self.eval_rule(module, rule)?;
+                    }
+                }
+            }
+
+            // With modifiers may be used to specify part of a module that that not yet been
+            // evaluated. Therefore ensure that module is evaluated first.
+            let requested_path = format!("data.{}", fields.join("."));
+            self.ensure_module_evaluated(requested_path.clone())?;
+
+            for i in (1..=fields.len()).rev() {
+                let prefix = fields.iter().take(i).copied().collect::<Vec<_>>();
+                let prefix_path = format!("data.{}", prefix.join("."));
+                if self.compiled_policy.rules.contains_key(&prefix_path)
+                    || self
+                        .compiled_policy
+                        .default_rules
+                        .contains_key(&prefix_path)
+                {
+                    self.ensure_rule_evaluated(prefix_path)?;
+                    break;
+                }
+            }
+
+            Ok(Self::get_value_chained(self.data.clone(), fields))
         } else if !self.compiled_policy.modules.is_empty() {
             let module = self.current_module()?;
             let parsed_path = Parser::get_path_ref_components(&module.package.refr)?;
@@ -3190,17 +3092,6 @@ impl Interpreter {
 
             if !found {
                 if let Some(imported_var) = self.compiled_policy.imports.get(&rule_path).cloned() {
-                    if let Ok(import_path) = get_path_string(&imported_var, None) {
-                        if import_path == "data" || import_path.starts_with("data.") {
-                            let combined_path = if fields.is_empty() {
-                                import_path
-                            } else {
-                                format!("{}.{}", import_path, fields.join("."))
-                            };
-                            let data_fields: Vec<&str> = combined_path.split('.').skip(1).collect();
-                            return self.lookup_data_path(&data_fields);
-                        }
-                    }
                     return Ok(Self::get_value_chained(
                         self.eval_expr(&imported_var)?,
                         fields,

src/languages/rego/compiler/references.rs

@@ -338,15 +338,6 @@ impl<'a> Compiler<'a> {
         // No rule found; fall back to module-level imports.
         let import_key = format!("{}.{}", &self.current_package, root);
         if let Some(import_expr) = self.policy.inner.imports.get(&import_key) {
-            if let Ok(mut import_chain) = parse_reference_chain(import_expr) {
-                if let ReferenceRoot::Variable(import_root) = &import_chain.root {
-                    if import_root == "data" {
-                        import_chain.components.extend(chain.components.clone());
-                        return self.compile_data_chain(&import_chain, span);
-                    }
-                }
-            }
-
             let import_reg =
                 self.compile_rego_expr_with_span(import_expr, import_expr.span(), false)?;
             if chain.components.is_empty() {

src/parser.rs

@@ -354,6 +354,33 @@ impl<'source> Parser<'source> {
         }
     }
 
+    /// Parse a field name after `.` in a ref expression.
+    ///
+    /// Unlike [`Self::parse_var`] and [`Self::parse_ident`], this method accepts **any**
+    /// `TokenKind::Ident` token, including reserved keywords (e.g. `as`, `default`, `else`,
+    /// `false`, `if`, `import`, `in`, `not`, `null`, `package`, `some`, `true`, `with`).
+    ///
+    /// The position immediately after `.` is unambiguously a field name, so there is no
+    /// syntactic ambiguity with statement-level keywords. This matches OPA's
+    /// `keywords_in_refs` capability, which is enabled by default in standard OPA builds.
+    ///
+    /// # Example
+    /// ```rego
+    /// allow if { input.v0.package.format == "npm" }  # `package` is a keyword but valid here
+    /// ```
+    fn parse_ref_field(&mut self) -> Result<Span> {
+        let span = self.tok.1.clone();
+        match self.tok.0 {
+            TokenKind::Ident => {
+                self.next_token()?;
+                Ok(span)
+            }
+            _ => Err(self
+                .source
+                .error(self.tok.1.line, self.tok.1.col, "expecting identifier")),
+        }
+    }
+
     fn read_number(&mut self, span: Span) -> Result<Expr> {
         match Number::from_str(span.text()) {
             Ok(v) => Ok(Expr::Number {
@@ -743,9 +770,10 @@ impl<'source> Parser<'source> {
                     );
                 }
                 "." => {
-                    // Read identifier.
+                    // Read identifier. Keywords are allowed as field names in
+                    // dot-notation refs (e.g. `input.package.name`).
                     self.next_token()?;
-                    let field = self.parse_var()?;
+                    let field = self.parse_ref_field()?;
                     span.end = self.end;
 
                     // Disallow any whitespace between . and identifier.
@@ -1418,9 +1446,10 @@ impl<'source> Parser<'source> {
                     );
                 }
                 "." => {
-                    // Read identifier.
+                    // Read identifier. Keywords are allowed as field names in
+                    // dot-notation refs (e.g. `import data.my.package`).
                     self.next_token()?;
-                    let field = self.parse_ident()?;
+                    let field = self.parse_ref_field()?;
                     span.end = self.end;
 
                     // Disallow any whitespace between . and identifier.
@@ -1523,7 +1552,8 @@ impl<'source> Parser<'source> {
                 "." => {
                     let sep_pos = self.tok.1.start;
                     self.next_token()?;
-                    let field = self.parse_var()?;
+                    // Keywords are allowed as field names in dot-notation refs.
+                    let field = self.parse_ref_field()?;
                     span.end = self.end;
 
                     // Disallow any whitespace between . and identifier.

tests/engine/mod.rs

@@ -197,34 +197,3 @@ fn get_policy_parameters() -> Result<()> {
 
     Ok(())
 }
-
-#[test]
-fn cross_package_import_lookup_does_not_false_cycle() -> Result<()> {
-    let mut engine = Engine::new();
-    engine.add_policy(
-        "registry.rego".to_string(),
-        r#"package registry
-           import data.registry.packages.package_a
-           import rego.v1
-
-           allow_stage1 if package_a.allow_stage1
-         "#
-        .to_string(),
-    )?;
-    engine.add_policy(
-        "package_a.rego".to_string(),
-        r#"package registry.packages.package_a
-           import data.registry
-           import rego.v1
-
-           allow_stage2 if registry.allow_stage1
-           allow_stage1 := true
-         "#
-        .to_string(),
-    )?;
-
-    let result = engine.eval_rule("data.registry.packages.package_a.allow_stage2".to_string())?;
-    assert_eq!(result, Value::Bool(true));
-
-    Ok(())
-}

tests/interpreter/cases/refr/keywords_in_refs.yaml

@@ -0,0 +1,268 @@
+# Copyright (c) Microsoft Corporation.
+# Licensed under the MIT License.
+#
+# Tests for keywords-as-field-names in dot-notation refs.
+# Matches OPA's `keywords_in_refs` behavior, enabled by default.
+cases:
+  - note: keywords_in_refs/package field
+    modules:
+      - |
+        package test
+        allow if {
+          input.v0.package.format == "npm"
+        }
+    input:
+      v0:
+        package:
+          format: npm
+    query: data.test.allow
+    want_result: true
+
+  - note: keywords_in_refs/as field
+    modules:
+      - |
+        package test
+        x = input.as.type
+    input:
+      as:
+        type: string
+    query: data.test.x
+    want_result: string
+
+  - note: keywords_in_refs/default field
+    modules:
+      - |
+        package test
+        x = input.default.value
+    input:
+      default:
+        value: 42
+    query: data.test.x
+    want_result: 42
+
+  - note: keywords_in_refs/else field
+    modules:
+      - |
+        package test
+        x = input.else.value
+    input:
+      else:
+        value: hello
+    query: data.test.x
+    want_result: hello
+
+  - note: keywords_in_refs/import field
+    modules:
+      - |
+        package test
+        x = input.import.name
+    input:
+      import:
+        name: foo
+    query: data.test.x
+    want_result: foo
+
+  - note: keywords_in_refs/not field
+    modules:
+      - |
+        package test
+        x = input.not.allowed
+    input:
+      not:
+        allowed: false
+    query: data.test.x
+    want_result: false
+
+  - note: keywords_in_refs/null field
+    modules:
+      - |
+        package test
+        x = input.null.value
+    input:
+      "null":
+        value: 1
+    query: data.test.x
+    want_result: 1
+
+  - note: keywords_in_refs/some field
+    modules:
+      - |
+        package test
+        x = input.some.field
+    input:
+      some:
+        field: bar
+    query: data.test.x
+    want_result: bar
+
+  - note: keywords_in_refs/true field
+    modules:
+      - |
+        package test
+        x = input.true.x
+    input:
+      "true":
+        x: 2
+    query: data.test.x
+    want_result: 2
+
+  - note: keywords_in_refs/false field
+    modules:
+      - |
+        package test
+        x = input.false.x
+    input:
+      "false":
+        x: 3
+    query: data.test.x
+    want_result: 3
+
+  - note: keywords_in_refs/with field
+    modules:
+      - |
+        package test
+        x = input.with.config
+    input:
+      with:
+        config: test
+    query: data.test.x
+    want_result: test
+
+  - note: keywords_in_refs/future keywords (if, in, every, contains)
+    modules:
+      - |
+        package test
+        import future.keywords
+        x if {
+          input.if.condition == true
+          input.in.set == "member"
+          input.every.item == "x"
+          input.contains.key == "val"
+        }
+    input:
+      if:
+        condition: true
+      in:
+        set: member
+      every:
+        item: x
+      contains:
+        key: val
+    query: data.test.x
+    want_result: true
+
+  - note: keywords_in_refs/chained keywords
+    modules:
+      - |
+        package test
+        x = input.package.import.default
+    input:
+      package:
+        import:
+          default: chained
+    query: data.test.x
+    want_result: chained
+
+  - note: keywords_in_refs/data path with keyword
+    modules:
+      - |
+        package test
+        x = data.mydata.package.name
+    data:
+      mydata:
+        package:
+          name: mypackage
+    query: data.test.x
+    want_result: mypackage
+
+  - note: keywords_in_refs/rego v1 all keywords
+    modules:
+      - |
+        package test
+        import rego.v1
+        allow if {
+          input.package.format == "npm"
+          input.default.value == 1
+          input.if.enabled == true
+          input.in.set == "member"
+          input.not.flag == false
+          input.with.config == "ok"
+        }
+    input:
+      package:
+        format: npm
+      default:
+        value: 1
+      if:
+        enabled: true
+      in:
+        set: member
+      not:
+        flag: false
+      with:
+        config: ok
+    query: data.test.allow
+    want_result: true
+
+  - note: keywords_in_refs/future keywords without import
+    modules:
+      - |
+        package test
+        x = [input.if.flag, input.in.value, input.every.item, input.contains.key]
+    input:
+      if:
+        flag: true
+      in:
+        value: member
+      every:
+        item: each
+      contains:
+        key: present
+    query: data.test.x
+    want_result: [true, "member", "each", "present"]
+
+  - note: keywords_in_refs/package path keywords
+    modules:
+      - |
+        package words.if.default
+        value = 7
+      - |
+        package test
+        x = data.words.if.default.value
+    query: data.test.x
+    want_result: 7
+
+  - note: keywords_in_refs/import path keywords
+    modules:
+      - |
+        package test
+        import data.catalog.if.default as kw
+        x = kw.value
+    data:
+      catalog:
+        if:
+          default:
+            value: 99
+    query: data.test.x
+    want_result: 99
+
+  - note: keywords_in_refs/rule head keyword path
+    modules:
+      - |
+        package test
+        policy.default.level := 3
+    query: data.test.policy.default.level
+    want_result: 3
+
+  - note: keywords_in_refs/mixed dot keyword and dynamic bracket
+    modules:
+      - |
+        package test
+        x = input.package[segment].value
+        segment = "import"
+    input:
+      package:
+        import:
+          value: from_dynamic
+    query: data.test.x
+    want_result: from_dynamic

tests/rvm/rego/cases/chained_access.yaml

@@ -315,3 +315,109 @@ cases:
         }
     query: data.test.main
     want_result: "/api/v1/users"
+
+  - note: keywords_in_refs/package_field
+    data: {}
+    input:
+      v0:
+        package:
+          format: npm
+    modules:
+      - |
+        package test
+        allow := true if {
+          input.v0.package.format == "npm"
+        }
+    query: data.test.allow
+    want_result: true
+
+  - note: keywords_in_refs/multiple_keywords
+    data: {}
+    input:
+      default:
+        value: 42
+      import:
+        name: foo
+      not:
+        allowed: false
+      with:
+        config: ok
+    modules:
+      - |
+        package test
+        import rego.v1
+        result if {
+          input.default.value == 42
+          input.import.name == "foo"
+          input.not.allowed == false
+          input.with.config == "ok"
+        }
+    query: data.test.result
+    want_result: true
+
+  - note: keywords_in_refs/future_keywords_without_import
+    data: {}
+    input:
+      if:
+        flag: true
+      in:
+        value: member
+      every:
+        item: each
+      contains:
+        key: present
+    modules:
+      - |
+        package test
+        result := [input.if.flag, input.in.value, input.every.item, input.contains.key]
+    query: data.test.result
+    want_result: [true, "member", "each", "present"]
+
+  - note: keywords_in_refs/package_path_keywords
+    data: {}
+    modules:
+      - |
+        package words.if.default
+        value := 7
+      - |
+        package test
+        result := data.words.if.default.value
+    query: data.test.result
+    want_result: 7
+
+  - note: keywords_in_refs/import_path_keywords
+    data:
+      catalog:
+        if:
+          default:
+            value: 99
+    modules:
+      - |
+        package test
+        import data.catalog.if.default as kw
+        result := kw.value
+    query: data.test.result
+    want_result: 99
+
+  - note: keywords_in_refs/rule_head_keyword_path
+    data: {}
+    modules:
+      - |
+        package test
+        policy.default.level := 3
+    query: data.test.policy.default.level
+    want_result: 3
+
+  - note: keywords_in_refs/mixed_dot_keyword_and_dynamic_bracket
+    data: {}
+    input:
+      package:
+        import:
+          value: from_dynamic
+    modules:
+      - |
+        package test
+        segment := "import"
+        result := input.package[segment].value
+    query: data.test.result
+    want_result: "from_dynamic"

tests/rvm/rego/cases/imports.yaml

@@ -95,37 +95,3 @@ cases:
         }
     query: data.rules.present
     want_result: true
-  - note: import_cross_package_no_false_cycle
-    modules:
-      - |
-        package registry
-        import data.registry.packages.package_a
-        import rego.v1
-
-        allow_stage1 if package_a.allow_stage1
-      - |
-        package registry.packages.package_a
-        import data.registry
-        import rego.v1
-
-        allow_stage2 if registry.allow_stage1
-        allow_stage1 := true
-    query: data.registry.packages.package_a.allow_stage2
-    want_result: true
-  - note: dynamic_data_index_evaluates_matching_rules
-    modules:
-      - |
-        package registry
-        import rego.v1
-
-        allow_stage1 if {
-          name := "package_a"
-          data.registry.packages[name].allow_stage2
-        }
-      - |
-        package registry.packages.package_a
-        import rego.v1
-
-        allow_stage2 := true
-    query: data.registry.allow_stage1
-    want_result: true