remove HierarchyNotRespected validation error

cedar-policy-validator/src/diagnostics.rs

@@ -156,12 +156,6 @@ pub enum ValidationError {
     #[diagnostic(transparent)]
     #[error(transparent)]
     NonLitExtConstructor(#[from] validation_errors::NonLitExtConstructor),
-    /// To pass strict validation a policy cannot contain an `in` expression
-    /// where the entity type on the left might not be able to be a member of
-    /// the entity type on the right.
-    #[error(transparent)]
-    #[diagnostic(transparent)]
-    HierarchyNotRespected(#[from] validation_errors::HierarchyNotRespected),
     /// Returned when an internal invariant is violated (should not happen; if
     /// this is ever returned, please file an issue)
     #[error(transparent)]
@@ -379,21 +373,6 @@ impl ValidationError {
         .into()
     }
 
-    pub(crate) fn hierarchy_not_respected(
-        source_loc: Option<Loc>,
-        policy_id: PolicyID,
-        in_lhs: Option<EntityType>,
-        in_rhs: Option<EntityType>,
-    ) -> Self {
-        validation_errors::HierarchyNotRespected {
-            source_loc,
-            policy_id,
-            in_lhs,
-            in_rhs,
-        }
-        .into()
-    }
-
     pub(crate) fn internal_invariant_violation(
         source_loc: Option<Loc>,
         policy_id: PolicyID,

cedar-policy-validator/src/diagnostics/validation_errors.rs

@@ -464,28 +464,19 @@ impl Diagnostic for FunctionArgumentValidation {
 
 /// Structure containing details about a hierarchy not respected error
 #[derive(Debug, Clone, Hash, Eq, PartialEq, Error)]
-#[error("for policy `{policy_id}`, operands to `in` do not respect the entity hierarchy")]
+#[error("Internal invariant violated: `HierarchyNotRespected` error should never occur. Please file an issue")]
 pub struct HierarchyNotRespected {
     /// Source location
     pub source_loc: Option<Loc>,
     /// Policy ID where the error occurred
     pub policy_id: PolicyID,
-    /// LHS (descendant) of the hierarchy relationship
-    pub in_lhs: Option<EntityType>,
-    /// RHS (ancestor) of the hierarchy relationship
-    pub in_rhs: Option<EntityType>,
 }
 
 impl Diagnostic for HierarchyNotRespected {
     impl_diagnostic_from_source_loc_opt_field!(source_loc);
 
     fn help<'a>(&'a self) -> Option<Box<dyn Display + 'a>> {
-        match (&self.in_lhs, &self.in_rhs) {
-            (Some(in_lhs), Some(in_rhs)) => Some(Box::new(format!(
-                "`{in_lhs}` cannot be a descendant of `{in_rhs}`"
-            ))),
-            _ => None,
-        }
+        Some(Box::new("please file an issue at <https://github.com/cedar-policy/cedar/issues> including the schema and policy that caused this error"))
     }
 }
 

cedar-policy-validator/src/typecheck.rs

@@ -1896,8 +1896,6 @@ impl<'a> SingleEnvTypechecker<'a> {
                             .is_in(lhs_expr, rhs_expr),
                     );
                 }
-                let lhs_ty = lhs_expr.data().clone();
-                let rhs_ty = rhs_expr.data().clone();
                 let lhs_as_euid_lit = self.replace_action_var_with_euid(lhs);
                 let rhs_as_euid_lit = self.replace_action_var_with_euid(rhs);
                 match (lhs_as_euid_lit.expr_kind(), rhs_as_euid_lit.expr_kind()) {
@@ -1969,82 +1967,11 @@ impl<'a> SingleEnvTypechecker<'a> {
                             .is_in(lhs_expr, rhs_expr),
                     ),
                 }
-                .then_typecheck(|type_of_in, _| {
-                    if !self.mode.is_strict() {
-                        TypecheckAnswer::success(type_of_in)
-                    } else if matches!(type_of_in.data(), Some(Type::True | Type::False)) {
-                        TypecheckAnswer::success(type_of_in)
-                    } else {
-                        match (lhs_ty, rhs_ty) {
-                            (Some(lhs_ty), Some(rhs_ty)) => {
-                                match (
-                                    Self::get_as_single_entity_type(lhs_ty),
-                                    Self::get_as_single_entity_type(rhs_ty),
-                                ) {
-                                    (Some(lhs_name), Some(rhs_name)) => {
-                                        let lhs_ty_in_rhs_ty = self
-                                            .schema
-                                            .get_entity_type(&rhs_name)
-                                            .map(|ety| ety.descendants.contains(&lhs_name))
-                                            .unwrap_or(false);
-                                        // A schema may always declare that an action entity is a member of another action entity,
-                                        // regardless of their exact types (i.e., their namespaces), so we shouldn't treat it as an error.
-                                        let action_in_action =
-                                            lhs_name.is_action() && rhs_name.is_action();
-                                        if lhs_name == rhs_name
-                                            || action_in_action
-                                            || lhs_ty_in_rhs_ty
-                                        {
-                                            TypecheckAnswer::success(type_of_in)
-                                        } else {
-                                            // We could actually just return `Type::False`, but this is incurs a larger Dafny proof update.
-                                            type_errors.push(
-                                                ValidationError::hierarchy_not_respected(
-                                                    in_expr.source_loc().cloned(),
-                                                    self.policy_id.clone(),
-                                                    Some(lhs_name),
-                                                    Some(rhs_name),
-                                                ),
-                                            );
-                                            TypecheckAnswer::fail(type_of_in)
-                                        }
-                                    }
-                                    _ => {
-                                        type_errors.push(ValidationError::hierarchy_not_respected(
-                                            in_expr.source_loc().cloned(),
-                                            self.policy_id.clone(),
-                                            None,
-                                            None,
-                                        ));
-                                        TypecheckAnswer::fail(type_of_in)
-                                    }
-                                }
-                            }
-                            // An argument type is `None`, so one the arguments must have failed to typecheck already.
-                            // There's no other interesting error to report in this case.
-                            _ => TypecheckAnswer::fail(type_of_in),
-                        }
-                    }
-                })
+                .then_typecheck(|type_of_in, _| TypecheckAnswer::success(type_of_in))
             })
         })
     }
 
-    fn get_as_single_entity_type(ty: Type) -> Option<EntityType> {
-        match ty {
-            Type::EntityOrRecord(EntityRecordKind::Entity(lub)) => lub.into_single_entity(),
-            Type::EntityOrRecord(EntityRecordKind::ActionEntity { name, .. }) => Some(name),
-            Type::Set {
-                element_type: Some(element_type),
-            } => match *element_type {
-                Type::EntityOrRecord(EntityRecordKind::Entity(lub)) => lub.into_single_entity(),
-                Type::EntityOrRecord(EntityRecordKind::ActionEntity { name, .. }) => Some(name),
-                _ => None,
-            },
-            _ => None,
-        }
-    }
-
     // Given an expression, if that expression is a literal or the `action`
     // variable, return it as an EntityUID. Return `None` otherwise.
     fn euid_from_euid_literal_or_action(&self, e: &Expr) -> Option<EntityUID> {

cedar-policy/CHANGELOG.md

@@ -60,6 +60,11 @@ Cedar Language Version: 4.2
 
 - Stopped emitting warnings for identifiers containing certain printable ASCII
   characters (e.g., `/` and `:`) (#1336, resolving #621)
+- `HierarchyNotRespected` validation error is no longer returned (although the
+  error variant remains, to avoid a breaking change). This means that in some
+  edge cases, policies that previously failed to validate under strict validation
+  will now pass validation, probably with an `ImpossiblePolicy` warning. (#1355,
+  resolving #638)
 
 ### Fixed
 

cedar-policy/src/api/err.rs

@@ -421,9 +421,9 @@ pub enum ValidationError {
     #[diagnostic(transparent)]
     #[error(transparent)]
     NonLitExtConstructor(#[from] validation_errors::NonLitExtConstructor),
-    /// To pass strict validation a policy cannot contain an `in` expression
-    /// where the entity type on the left might not be able to be a member of
-    /// the entity type on the right.
+    /// This error type is no longer ever returned, but remains here for
+    /// backwards-compatibility (removing the variant entirely would be a
+    /// breaking change).
     #[error(transparent)]
     #[diagnostic(transparent)]
     HierarchyNotRespected(#[from] validation_errors::HierarchyNotRespected),
@@ -514,9 +514,6 @@ impl From<cedar_policy_validator::ValidationError> for ValidationError {
             cedar_policy_validator::ValidationError::NonLitExtConstructor(e) => {
                 Self::NonLitExtConstructor(e.into())
             }
-            cedar_policy_validator::ValidationError::HierarchyNotRespected(e) => {
-                Self::HierarchyNotRespected(e.into())
-            }
             cedar_policy_validator::ValidationError::InternalInvariantViolation(e) => {
                 Self::InternalInvariantViolation(e.into())
             }