Repository: cedar-policy/cedar
Pull request #1422 (2025-01-13 16:21:03)

partial fix for 1421
## Description of changes

Resolves some of the issues identified in #1421.  Specifically,
- Eliminates duplication between `typecheck_by_request_env()` and `multi_typecheck_by_request_env()` by refactoring the former to call the latter
- Refactors `multi_typecheck_by_request_env()` to call the existing utility `apply_typecheck_fn_by_request_env()`; refactors the latter to handle multiple policies efficiently
- Renames `multi_typecheck_by_request_env()` to `typecheck_multi_by_request_env()` because I find it clearer (all typechecking functions now begin `typecheck_*()`)

## Issue #, if available

#1421

## Checklist for requesting a review

The change in this PR is (choose one, and delete the other options):

- [x] A change "invisible" to users (e.g., documentation, changes to "internal" crates like `cedar-policy-core`, `cedar-validator`, etc.)

I confirm that this PR (choose one, and delete the other options):

- [x] Does not update the CHANGELOG because my change does not significantly impact released code.

I confirm that [`cedar-spec`](https://github.com/cedar-policy/cedar-spec) (choose one, and delete the other options):

- [x] Requires updates, and I have made / will make these updates myself. (Please include in your description a timeline or link to the relevant PR in `cedar-spec`, and how you have tested that your updates are correct.)

I confirm that [`docs.cedarpolicy.com`](https://docs.cedarpolicy.com/) (choose one, and delete the other options):

- [x] Does not require updates because my change does not impact the Cedar language specification.



Patch:
diff --git a/cedar-policy-validator/src/typecheck.rs b/cedar-policy-validator/src/typecheck.rs
index 8f00f5a2..49e781ae 100644
--- a/cedar-policy-validator/src/typecheck.rs
+++ b/cedar-policy-validator/src/typecheck.rs
@@ -24,7 +24,11 @@ mod typecheck_answer;
 use itertools::Itertools;
 pub(crate) use typecheck_answer::TypecheckAnswer;
 
-use std::{borrow::Cow, collections::HashSet, iter::zip};
+use std::{
+    borrow::Cow,
+    collections::{HashMap, HashSet},
+    iter::zip,
+};
 
 use crate::{
     extension_schema::ExtensionFunctionType,
@@ -46,6 +50,7 @@ use cedar_policy_core::{
     },
     expr_builder::ExprBuilder as _,
     extensions::Extensions,
+    parser::Loc,
 };
 
 #[cfg(not(target_arch = "wasm32"))]
@@ -143,10 +148,27 @@ impl<'a> Typechecker<'a> {
         &'b self,
         t: &'b Template,
     ) -> Vec<(RequestEnv<'b>, PolicyCheck)> {
-        self.apply_typecheck_fn_by_request_env(t, |request, expr| {
+        let map = self.typecheck_multi_by_request_env([t]);
+        map.into_values()
+            .filter(|(v, _)| !v.is_empty())
+            .next()
+            .unwrap_or_default() // if all the entries have empty vecs, return an empty vec
+            .0
+    }
+
+    /// Same as `typecheck_by_request_env()`, but typechecks multiple policies
+    /// at once and returns all the results indexed by policy ID, more
+    /// efficiently than calling `typecheck_by_request_env()` multiple times.
+    ///
+    /// The `Loc` of each policy is also returned, for error reporting purposes.
+    pub fn typecheck_multi_by_request_env<'b>(
+        &'b self,
+        ts: impl IntoIterator<Item = &'b Template>,
+    ) -> HashMap<PolicyID, (Vec<(RequestEnv<'b>, PolicyCheck)>, Option<Loc>)> {
+        self.apply_typecheck_fn_by_request_env(ts, |request, expr| {
             let mut type_errors = Vec::new();
             let empty_prior_capability = CapabilitySet::new();
-            let ty = self.expect_type(
+            let ans = self.expect_type(
                 request,
                 &empty_prior_capability,
                 expr,
@@ -155,8 +177,8 @@ impl<'a> Typechecker<'a> {
                 |_| None,
             );
 
-            let is_false = ty.contains_type(&Type::singleton_boolean(false));
-            match (is_false, ty.typechecked(), ty.into_typed_expr()) {
+            let is_false = ans.contains_type(&Type::singleton_boolean(false));
+            match (is_false, ans.typechecked(), ans.into_typed_expr()) {
                 (false, true, None) => PolicyCheck::Fail(type_errors),
                 (false, true, Some(e)) => PolicyCheck::Success(e),
                 (false, false, _) => PolicyCheck::Fail(type_errors),
@@ -168,75 +190,49 @@ impl<'a> Typechecker<'a> {
         })
     }
 
-    /// Utility abstracting the common logic for strict and regular typechecking
-    /// by request environment.
+    /// Apply `typecheck_fn` to each policy in every schema-defined request
+    /// environment, and collect all the results.
+    ///
+    /// Results are returned indexed by the policy ID of the policy they belong to.
+    /// Results for a single policy are returned in no particular order.
+    /// The `Loc` of each policy is also returned, for error reporting purposes.
     fn apply_typecheck_fn_by_request_env<'b, F, C>(
         &'b self,
-        t: &'b Template,
+        ts: impl IntoIterator<Item = &'b Template>,
         typecheck_fn: F,
-    ) -> Vec<(RequestEnv<'b>, C)>
+    ) -> HashMap<PolicyID, (Vec<(RequestEnv<'b>, C)>, Option<Loc>)>
     where
         F: Fn(&RequestEnv<'b>, &Expr) -> C,
     {
-        let mut result_checks = Vec::new();
+        let mut ret = HashMap::new();
 
-        // Validate each (principal, resource) pair with the substituted policy
-        // for the corresponding action. Implemented as for loop to make it
-        // explicit that `expect_type` will be called for every element of
-        // request_env without short circuiting.
-        let policy_condition = &t.condition();
-        for unlinked_e in self.unlinked_request_envs() {
-            for linked_e in self.link_request_env(&unlinked_e, t) {
-                let check = typecheck_fn(&linked_e, policy_condition);
-                result_checks.push((linked_e, check))
-            }
-        }
-        result_checks
-    }
+        // compute `.condition()` just once for each policy, and cache it here
+        let ts: Vec<(&'b Template, Expr)> = ts.into_iter().map(|t| (t, t.condition())).collect();
 
-    /// Additional entry point for typechecking requests. This method takes a slice
-    /// over policies and typechecks each under every schema-defined request environment.
-    ///
-    /// The result contains these environments in no particular order, but each list of
-    /// policy checks will always match the original order.
-    pub fn multi_typecheck_by_request_env(
-        &self,
-        policy_templates: &[&Template],
-    ) -> Vec<(RequestEnv<'_>, Vec<PolicyCheck>)> {
-        let mut env_checks = Vec::new();
-        for request in self.unlinked_request_envs() {
-            let mut policy_checks = Vec::new();
-            for t in policy_templates.iter() {
-                let condition_expr = t.condition();
-                for linked_env in self.link_request_env(&request, t) {
-                    let mut type_errors = Vec::new();
-                    let empty_prior_capability = CapabilitySet::new();
-                    let ty = self.expect_type(
-                        &linked_env,
-                        &empty_prior_capability,
-                        &condition_expr,
-                        Type::primitive_boolean(),
-                        &mut type_errors,
-                        |_| None,
-                    );
+        // initialize the entry for each `PolicyID` by inserting the appropriate loc
+        for (t, _) in &ts {
+            ret.insert(t.id().clone(), (Vec::new(), t.loc().cloned()));
+        }
 
-                    let is_false = ty.contains_type(&Type::singleton_boolean(false));
-                    match (is_false, ty.typechecked(), ty.into_typed_expr()) {
-                        (false, true, None) => policy_checks.push(PolicyCheck::Fail(type_errors)),
-                        (false, true, Some(e)) => policy_checks.push(PolicyCheck::Success(e)),
-                        (false, false, _) => policy_checks.push(PolicyCheck::Fail(type_errors)),
-                        (true, _, Some(e)) => {
-                            policy_checks.push(PolicyCheck::Irrelevant(type_errors, e))
-                        }
-                        // PANIC SAFETY: `is_false` implies `e` has a type implies `Some(e)`.
-                        #[allow(clippy::unreachable)]
-                        (true, _, None) => unreachable!(),
-                    }
-                }
+        // Validate each (principal, resource) pair with the substituted policy
+        // for the corresponding action.
+        //
+        // this ordering of loop nesting is chosen in order to call
+        // `unlinked_request_envs()` just once for all policies
+        for unlinked_e in self.unlinked_request_envs() {
+            for (t, cond) in &ts {
+                // PANIC SAFETY: already inserted this key above
+                #[allow(clippy::expect_used)]
+                ret.get_mut(t.id())
+                    .expect("already inserted this key above")
+                    .0
+                    .extend(self.link_request_env(&unlinked_e, t).map(|linked_e| {
+                        let check = typecheck_fn(&linked_e, cond);
+                        (linked_e, check)
+                    }));
             }
-            env_checks.push((request, policy_checks));
         }
-        env_checks
+        ret
     }
 
     fn unlinked_request_envs(&self) -> impl Iterator<Item = RequestEnv<'_>> + '_ {
diff --git a/cedar-policy/src/api.rs b/cedar-policy/src/api.rs
index dc10a721..c68664cf 100644
--- a/cedar-policy/src/api.rs
+++ b/cedar-policy/src/api.rs
@@ -71,7 +71,7 @@ pub(crate) mod version {
         static ref SDK_VERSION: Version = env!("CARGO_PKG_VERSION").parse().unwrap();
         // Cedar language version
         // The patch version field may be unnecessary
-        static ref LANG_VERSION: Version = Version::new(4, 0, 0);
+        static ref LANG_VERSION: Version = Version::new(4, 2, 0);
     }
     /// Get the Cedar SDK Semantic Versioning version
     #[allow(clippy::module_name_repetitions)]


Test patch:
diff --git a/cedar-policy/src/tests.rs b/cedar-policy/src/tests.rs
index 3e2e1afb..6a9f6a76 100644
--- a/cedar-policy/src/tests.rs
+++ b/cedar-policy/src/tests.rs
@@ -6292,7 +6292,7 @@ mod version_tests {
 
     #[test]
     fn test_lang_version() {
-        assert_eq!(get_lang_version().to_string(), "4.0.0");
+        assert_eq!(get_lang_version().to_string(), "4.2.0");
     }
 }