Improve error message and add help text for LUB errors (#809)

Signed-off-by: John Kastner <[email protected]>

Author: john-h-kastner-aws

cedar-policy-core/src/parser.rs

@@ -24,6 +24,7 @@ mod cst_to_ast;
 pub mod err;
 /// implementations for formatting, like `Display`
 mod fmt;
+pub use fmt::join_with_conjunction;
 /// Source location struct
 mod loc;
 pub use loc::Loc;

cedar-policy-core/src/parser/fmt.rs

@@ -453,7 +453,7 @@ impl std::fmt::Display for Slot {
 
 /// Format an iterator as a natural-language string, separating items with
 /// commas and a conjunction (e.g., "and", "or") between the last two items.
-pub(crate) fn join_with_conjunction<T, W: Write>(
+pub fn join_with_conjunction<T, W: Write>(
     f: &mut W,
     conjunction: &str,
     items: impl IntoIterator<Item = T>,

cedar-policy-validator/src/type_error.rs

@@ -19,7 +19,7 @@
 use std::{collections::BTreeSet, fmt::Display};
 
 use cedar_policy_core::ast::{CallStyle, EntityUID, Expr, ExprKind, Name, Var};
-use cedar_policy_core::parser::Loc;
+use cedar_policy_core::parser::{join_with_conjunction, Loc};
 
 use crate::types::{EntityLUB, EntityRecordKind, RequestEnv};
 
@@ -127,12 +127,19 @@ impl TypeError {
 
     /// Construct a type error for when a least upper bound cannot be found for
     /// a collection of types.
-    pub(crate) fn incompatible_types(on_expr: Expr, types: impl IntoIterator<Item = Type>) -> Self {
+    pub(crate) fn incompatible_types(
+        on_expr: Expr,
+        types: impl IntoIterator<Item = Type>,
+        hint: LubHelp,
+        context: LubContext,
+    ) -> Self {
         Self {
             on_expr: Some(on_expr),
             source_loc: None,
             kind: TypeErrorKind::IncompatibleTypes(IncompatibleTypes {
                 types: types.into_iter().collect::<BTreeSet<_>>(),
+                hint,
+                context,
             }),
         }
     }
@@ -241,7 +248,8 @@ pub enum TypeErrorKind {
     #[diagnostic(transparent)]
     UnexpectedType(UnexpectedType),
     /// The typechecker could not compute a least upper bound for `types`.
-    #[error("unable to find upper bound for types: [{}]", .0.types.iter().join(","))]
+    #[error(transparent)]
+    #[diagnostic(transparent)]
     IncompatibleTypes(IncompatibleTypes),
     /// The typechecker detected an access to a record or entity attribute
     /// that it could not statically guarantee would be present.
@@ -331,9 +339,48 @@ pub(crate) enum UnexpectedTypeHelp {
 }
 
 /// Structure containing details about an incompatible type error.
-#[derive(Debug, Clone, Hash, Eq, PartialEq)]
+#[derive(Diagnostic, Error, Debug, Clone, Hash, Eq, PartialEq)]
+#[diagnostic(help("{context} must have compatible types. {hint}"))]
 pub struct IncompatibleTypes {
     pub(crate) types: BTreeSet<Type>,
+    pub(crate) hint: LubHelp,
+    pub(crate) context: LubContext,
+}
+
+impl Display for IncompatibleTypes {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        write!(f, "the types ")?;
+        join_with_conjunction(f, "and", self.types.iter(), |f, t| write!(f, "{t}"))?;
+        write!(f, " are not compatible")
+    }
+}
+
+#[derive(Error, Debug, Clone, Hash, Eq, PartialEq)]
+pub(crate) enum LubHelp {
+    #[error("Corresponding attributes of compatible record types must have the same optionality, either both being required or both being optional")]
+    AttributeQualifier,
+    #[error("Compatible record types must have exactly the same attributes")]
+    RecordWidth,
+    #[error("Different entity types are never compatible even when their attributes would be compatible")]
+    EntityType,
+    #[error("Entity and record types are never compatible even when their attributes would be compatible")]
+    EntityRecord,
+    #[error("Types must be exactly equal to be compatible")]
+    None,
+}
+
+#[derive(Error, Debug, Clone, Hash, Eq, PartialEq)]
+pub(crate) enum LubContext {
+    #[error("elements of a set")]
+    Set,
+    #[error("both branches of a conditional")]
+    Conditional,
+    #[error("both operands to a `==` expression")]
+    Equality,
+    #[error("elements of the first operand and the second operand to a `contains` expression")]
+    Contains,
+    #[error("elements of both set operands to a `containsAll` or `containsAny` expression")]
+    ContainsAnyAll,
 }
 
 /// Structure containing details about a missing attribute error.

cedar-policy-validator/src/typecheck.rs

@@ -43,7 +43,8 @@ use crate::{
     types::{
         AttributeType, Effect, EffectSet, EntityRecordKind, OpenTag, Primitive, RequestEnv, Type,
     },
-    AttributeAccess, UnexpectedTypeHelp, ValidationMode, ValidationWarning, ValidationWarningKind,
+    AttributeAccess, LubContext, UnexpectedTypeHelp, ValidationMode, ValidationWarning,
+    ValidationWarningKind,
 };
 
 use super::type_error::TypeError;
@@ -740,6 +741,7 @@ impl<'a> Typechecker<'a> {
                                     e,
                                     vec![typ_then.data().clone(), typ_else.data().clone()],
                                     type_errors,
+                                    LubContext::Conditional,
                                 );
                                 let has_lub = lub_ty.is_some();
                                 let annot_expr = ExprBuilder::with_data(lub_ty)
@@ -1271,6 +1273,7 @@ impl<'a> Typechecker<'a> {
                         e,
                         elem_expr_types.iter().map(|ety| ety.data().clone()),
                         type_errors,
+                        LubContext::Set,
                     );
                     match elem_lub {
                         _ if self.mode.is_strict() && exprs.is_empty() => {
@@ -1388,6 +1391,7 @@ impl<'a> Typechecker<'a> {
                                 lhs_ty.data(),
                                 rhs_ty.data(),
                                 type_errors,
+                                LubContext::Equality,
                             )
                         } else {
                             TypecheckAnswer::success(
@@ -1517,6 +1521,7 @@ impl<'a> Typechecker<'a> {
                                     },
                                     expr_ty_arg2.data(),
                                     type_errors,
+                                    LubContext::Contains,
                                 )
                             } else {
                                 TypecheckAnswer::success(
@@ -1573,6 +1578,7 @@ impl<'a> Typechecker<'a> {
                                 expr_ty_arg1.data(),
                                 expr_ty_arg2.data(),
                                 type_errors,
+                                LubContext::ContainsAnyAll,
                             )
                         } else {
                             TypecheckAnswer::success(
@@ -1594,6 +1600,7 @@ impl<'a> Typechecker<'a> {
         lhs_ty: &Option<Type>,
         rhs_ty: &Option<Type>,
         type_errors: &mut Vec<TypeError>,
+        context: LubContext,
     ) -> TypecheckAnswer<'b> {
         match annotated_expr.data() {
             Some(Type::False) => {
@@ -1603,20 +1610,25 @@ impl<'a> Typechecker<'a> {
                 TypecheckAnswer::success(ExprBuilder::with_data(Some(Type::True)).val(true))
             }
             _ => match (lhs_ty, rhs_ty) {
-                (Some(lhs_ty), Some(rhs_ty))
-                    if Type::least_upper_bound(self.schema, lhs_ty, rhs_ty, self.mode)
-                        .is_none() =>
-                {
-                    type_errors.push(TypeError::incompatible_types(
-                        unannotated_expr.clone(),
-                        [lhs_ty.clone(), rhs_ty.clone()],
-                    ));
-                    TypecheckAnswer::fail(annotated_expr)
+                (Some(lhs_ty), Some(rhs_ty)) => {
+                    if let Err(lub_hint) =
+                        Type::least_upper_bound(self.schema, lhs_ty, rhs_ty, self.mode)
+                    {
+                        type_errors.push(TypeError::incompatible_types(
+                            unannotated_expr.clone(),
+                            [lhs_ty.clone(), rhs_ty.clone()],
+                            lub_hint,
+                            context,
+                        ));
+                        TypecheckAnswer::fail(annotated_expr)
+                    } else {
+                        // We had `Some` type for lhs and rhs and these types
+                        // were compatible.
+                        TypecheckAnswer::success(annotated_expr)
+                    }
                 }
-                // Either we had `Some` type for lhs and rhs and these types
-                // were compatible, or we failed to a compute a type for either
-                // lhs or rhs, meaning we already failed typechecking for that
-                // expression.
+                // We failed to compute a type for either lhs or rhs, meaning
+                // we already failed typechecking for that expression.
                 _ => TypecheckAnswer::success(annotated_expr),
             },
         }
@@ -2383,6 +2395,7 @@ impl<'a> Typechecker<'a> {
         expr: &Expr,
         answers: impl IntoIterator<Item = Option<Type>>,
         type_errors: &mut Vec<TypeError>,
+        context: LubContext,
     ) -> Option<Type> {
         answers
             .into_iter()
@@ -2393,17 +2406,22 @@ impl<'a> Typechecker<'a> {
             .and_then(|typechecked_types| {
                 let lub =
                     Type::reduce_to_least_upper_bound(self.schema, &typechecked_types, self.mode);
-                if lub.is_none() {
-                    // A type error is generated if we could not find a least
-                    // upper bound for the types. The computed least upper bound
-                    // will be None, so this function will correctly report this
-                    // as a failure.
-                    type_errors.push(TypeError::incompatible_types(
-                        expr.clone(),
-                        typechecked_types,
-                    ));
+                match lub {
+                    Err(lub_hint) => {
+                        // A type error is generated if we could not find a least
+                        // upper bound for the types. The computed least upper bound
+                        // will be None, so this function will correctly report this
+                        // as a failure.
+                        type_errors.push(TypeError::incompatible_types(
+                            expr.clone(),
+                            typechecked_types,
+                            lub_hint,
+                            context,
+                        ));
+                        None
+                    }
+                    Ok(lub) => Some(lub),
                 }
-                lub
             })
     }
 

cedar-policy-validator/src/typecheck/test_expr.rs

@@ -27,7 +27,7 @@ use smol_str::SmolStr;
 
 use crate::{
     type_error::TypeError, types::Type, AttributeAccess, AttributesOrContext, EntityType,
-    NamespaceDefinition, SchemaFragment, UnexpectedTypeHelp, ValidationMode,
+    LubContext, LubHelp, NamespaceDefinition, SchemaFragment, UnexpectedTypeHelp, ValidationMode,
 };
 
 use super::test_utils::{
@@ -144,6 +144,8 @@ fn heterogeneous_set() {
         vec![TypeError::incompatible_types(
             set,
             vec![Type::singleton_boolean(true), Type::primitive_long()],
+            LubHelp::None,
+            LubContext::Set,
         )],
     );
 }
@@ -671,6 +673,8 @@ fn record_get_attr_lub_typecheck_fails() {
                 )]),
                 Type::primitive_long(),
             ],
+            LubHelp::None,
+            LubContext::Conditional,
         )],
     );
 }
@@ -1025,6 +1029,8 @@ fn if_no_lub_error() {
         vec![TypeError::incompatible_types(
             if_expr,
             vec![Type::primitive_long(), Type::primitive_string()],
+            LubHelp::None,
+            LubContext::Conditional,
         )],
     );
 }
@@ -1038,6 +1044,8 @@ fn if_typecheck_fails() {
             TypeError::incompatible_types(
                 if_expr,
                 vec![Type::primitive_long(), Type::primitive_string()],
+                LubHelp::None,
+                LubContext::Conditional,
             ),
             TypeError::expected_type(
                 Expr::val("fail"),

cedar-policy-validator/src/typecheck/test_partial.rs

@@ -653,6 +653,7 @@ mod fail_partial_schema {
     use std::str::FromStr;
 
     use super::*;
+    use crate::{LubContext, LubHelp};
 
     #[test]
     fn error_on_declared_attr() {
@@ -694,6 +695,8 @@ mod fail_partial_schema {
             vec![TypeError::incompatible_types(
                 Expr::from_str("if resource.foo then principal.age else (if resource.bar then principal.name else principal.unknown)").unwrap(),
                 vec![Type::primitive_long(), Type::primitive_string()],
+                LubHelp::None,
+                LubContext::Conditional,
             )],
         );
     }

cedar-policy-validator/src/typecheck/test_policy.rs

@@ -38,7 +38,8 @@ use crate::{
     type_error::TypeError,
     typecheck::{test_utils::static_to_template, PolicyCheck},
     types::{EntityLUB, Type},
-    AttributeAccess, NamespaceDefinition, ValidationMode, ValidationWarningKind,
+    AttributeAccess, LubContext, LubHelp, NamespaceDefinition, ValidationMode,
+    ValidationWarningKind,
 };
 
 fn simple_schema_file() -> NamespaceDefinition {
@@ -716,7 +717,9 @@ fn entity_record_lub_is_none() {
                 [
                     Type::closed_record_with_required_attributes([("name".into(), Type::primitive_string())]),
                     Type::named_entity_reference_from_str("User"),
-                ]
+                ],
+                LubHelp::EntityRecord,
+                LubContext::Conditional,
             )
         ],
     );
@@ -937,6 +940,8 @@ fn record_entity_lub_non_term() {
                 )]),
                 Type::named_entity_reference_from_str("U"),
             ],
+            LubHelp::EntityRecord,
+            LubContext::Conditional,
         )],
     );
 }