better error messages

Signed-off-by: Shaobo He <[email protected]>

cedar-policy-core/src/ast/expr.rs

@@ -1858,6 +1858,14 @@ impl<T> Expr<T> {
             }
         }
     }
+
+    pub(crate) fn is_true(&self) -> bool {
+        matches!(self.expr_kind(), ExprKind::Lit(Literal::Bool(true)))
+    }
+
+    pub(crate) fn is_false(&self) -> bool {
+        matches!(self.expr_kind(), ExprKind::Lit(Literal::Bool(false)))
+    }
 }
 
 /// AST variables

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

@@ -1006,36 +1006,36 @@ impl Node<Option<cst::Add>> {
     // Peel the grammar onion until we see valid RHS
     pub(crate) fn to_has_rhs(&self) -> Result<Either<SmolStr, NonEmpty<UnreservedId>>> {
         let inner @ cst::Add { initial, extended } = self.try_as_inner()?;
-        let err = || {
-            ToASTError::new(
-                ToASTErrorKind::InvalidAttribute(inner.to_string().into()),
-                self.loc.clone(),
-            )
-            .into()
+        let err = |loc| {
+            ToASTError::new(ToASTErrorKind::InvalidHasRHS(inner.to_string().into()), loc).into()
         };
         let construct_attrs = |first: UnreservedId, rest: &[Node<Option<cst::MemAccess>>]| {
             let mut acc = nonempty![first];
-            rest.iter().try_for_each(|ma| {
-                let ma = ma.try_as_inner()?;
+            rest.iter().try_for_each(|ma_node| {
+                let ma = ma_node.try_as_inner()?;
                 match ma {
                     cst::MemAccess::Field(id) => {
                         acc.push(id.to_unreserved_ident()?);
                         Ok(())
                     }
-                    _ => Err(err()),
+                    _ => Err(err(ma_node.loc.clone())),
                 }
             })?;
             Ok::<nonempty::NonEmpty<UnreservedId>, ParseErrors>(acc)
         };
         if !extended.is_empty() {
-            return Err(err());
+            return Err(err(self.loc.clone()));
         }
         let cst::Mult { initial, extended } = initial.try_as_inner()?;
         if !extended.is_empty() {
-            return Err(err());
+            return Err(err(self.loc.clone()));
         }
-        if let cst::Unary { op: None, item } = initial.try_as_inner()? {
-            let cst::Member { item, access } = item.try_as_inner()?;
+        if let cst::Unary {
+            op: None,
+            item: item_node,
+        } = initial.try_as_inner()?
+        {
+            let cst::Member { item, access } = item_node.try_as_inner()?;
             let item = item.to_expr_or_special()?;
             match (item, access.as_slice()) {
                 (ExprOrSpecial::StrLit { lit, loc }, []) => Ok(Either::Left(
@@ -1064,12 +1064,26 @@ impl Node<Option<cst::Add>> {
                         .into())
                     }
                 }
-                _ => Err(err()),
+                // Attempt to return a precise error message for RHS like `true.<...>`
+                (ExprOrSpecial::Expr { loc, expr }, _) if expr.is_true() => Err(ToASTError::new(
+                    ToASTErrorKind::ReservedIdentifier(cst::Ident::True),
+                    loc,
+                )
+                .into()),
+                // Attempt to return a precise error message for RHS like `false.<...>`
+                (ExprOrSpecial::Expr { loc, expr }, _) if expr.is_false() => Err(ToASTError::new(
+                    ToASTErrorKind::ReservedIdentifier(cst::Ident::False),
+                    loc,
+                )
+                .into()),
+                (ExprOrSpecial::Expr { loc, .. }, _) => Err(err(loc)),
+                _ => Err(err(self.loc.clone())),
             }
         } else {
-            Err(err())
+            Err(err(self.loc.clone()))
         }
     }
+
     pub(crate) fn to_expr_or_special(&self) -> Result<ExprOrSpecial<'_>> {
         let add = self.try_as_inner()?;
 
@@ -2787,8 +2801,8 @@ mod tests {
         expect_some_error_matches(
             src,
             &errs,
-            &ExpectedErrorMessageBuilder::error("invalid attribute name: 1")
-                .help("attribute names can either be identifiers or string literals")
+            &ExpectedErrorMessageBuilder::error("invalid RHS of a `has` operation: 1")
+                .help("valid RHS of a `has` operation is either a sequence of identifiers separated by `.` or a string literal")
                 .exactly_one_underline("1")
                 .build(),
         );
@@ -3002,8 +3016,8 @@ mod tests {
         expect_some_error_matches(
             src,
             &errs,
-            &ExpectedErrorMessageBuilder::error("invalid attribute name: 1")
-                .help("attribute names can either be identifiers or string literals")
+            &ExpectedErrorMessageBuilder::error("invalid RHS of a `has` operation: 1")
+                .help("valid RHS of a `has` operation is either a sequence of identifiers separated by `.` or a string literal")
                 .exactly_one_underline("1")
                 .build(),
         );
@@ -4828,5 +4842,42 @@ mod tests {
         "#), Ok(p) => {
             println!("{p}");
         });
+
+        let policy = r#"permit(principal, action, resource) when {
+            principal has a.if
+          };"#;
+        assert_matches!(
+            parse_policy(None, policy),
+            Err(e) => {
+                expect_n_errors(policy, &e, 1);
+                expect_some_error_matches(policy, &e, &ExpectedErrorMessageBuilder::error(
+                    "this identifier is reserved and cannot be used: if",
+                ).exactly_one_underline(r#"if"#).build());
+            }
+        );
+        let policy = r#"permit(principal, action, resource) when {
+            principal has if.a
+          };"#;
+        assert_matches!(
+            parse_policy(None, policy),
+            Err(e) => {
+                expect_n_errors(policy, &e, 1);
+                expect_some_error_matches(policy, &e, &ExpectedErrorMessageBuilder::error(
+                    "this identifier is reserved and cannot be used: if",
+                ).exactly_one_underline(r#"if"#).build());
+            }
+        );
+        let policy = r#"permit(principal, action, resource) when {
+            principal has true.if
+          };"#;
+        assert_matches!(
+            parse_policy(None, policy),
+            Err(e) => {
+                expect_n_errors(policy, &e, 1);
+                expect_some_error_matches(policy, &e, &ExpectedErrorMessageBuilder::error(
+                    "this identifier is reserved and cannot be used: true",
+                ).exactly_one_underline(r#"true"#).build());
+            }
+        );
     }
 }

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

@@ -237,6 +237,10 @@ pub enum ToASTErrorKind {
     #[error("invalid attribute name: {0}")]
     #[diagnostic(help("attribute names can either be identifiers or string literals"))]
     InvalidAttribute(SmolStr),
+    /// Returned when the RHS of a `has` operation is invalid
+    #[error("invalid RHS of a `has` operation: {0}")]
+    #[diagnostic(help("valid RHS of a `has` operation is either a sequence of identifiers separated by `.` or a string literal"))]
+    InvalidHasRHS(SmolStr),
     /// Returned when attempting to use an attribute with a namespace
     #[error("`{0}` cannot be used as an attribute as it contains a namespace")]
     PathAsAttribute(String),

cedar-policy-core/src/parser/grammar.lalrpop

@@ -172,6 +172,12 @@ AnyIdent: Node<Option<cst::Ident>> = {
 }
 pub Ident: Node<Option<cst::Ident>> = AnyIdent;
 
+#[inline]
+IfIdent: Node<Option<cst::Ident>> = {
+    <l:@L> IF <r:@R>
+        => Node::with_source_loc(Some(cst::Ident::If), Loc::new(l..r, Arc::clone(src))),
+}
+
 // Cond := ('when' | 'unless') '{' Expr '}'
 Cond: Node<Option<cst::Cond>> = {
     <l:@L> <i:AnyIdent> "{" <e:Expr> "}" <r:@R>
@@ -211,10 +217,9 @@ Relation: Node<Option<cst::Relation>> = {
     // reporting. RFC 62 (extended has operator) allows a sequence of
     // identifiers separated by . as RHS. Hence, we need to extend this rule to
     // `HAS IF { MemAccess }`, as opposed to the original `HAS IF`.
-    <l:@L> <t:Add> HAS IF <a:MemAccess*> <r:@R> => {
+    <l:@L> <t:Add> HAS <ii:IfIdent> <a:MemAccess*> <r:@R> => {
         // Create an add expression from this identifier
-        let id0 = Node::with_source_loc(Some(cst::Ident::If), Loc::new(l..r, Arc::clone(src)));
-        let id1 = Node::with_source_loc(Some(cst::Name{path: vec![], name: id0}), Loc::new(l..r, Arc::clone(src)));
+        let id1 = Node::with_source_loc(Some(cst::Name{path: vec![], name: ii}), Loc::new(l..r, Arc::clone(src)));
         let id2 = Node::with_source_loc(Some(cst::Primary::Name(id1)), Loc::new(l..r, Arc::clone(src)));
         let id3 = Node::with_source_loc(Some(cst::Member{ item: id2, access: a }), Loc::new(l..r, Arc::clone(src)));
         let id4 = Node::with_source_loc(Some(cst::Unary{op: None, item:id3}), Loc::new(l..r, Arc::clone(src)));

cedar-policy/src/tests.rs

@@ -6314,11 +6314,10 @@ mod reserved_keywords_in_policies {
                         id.into(),
                         "attribute names can either be identifiers or string literals".into(),
                     );
-                    assert_invalid_expression_with_help(
+                    assert_invalid_expression(
                         format!("principal has {id}"),
-                        format!("invalid attribute name: {id}"),
-                        id.into(),
-                        "attribute names can either be identifiers or string literals".into(),
+                        RESERVED_IDENT_MSG(id),
+                        format!("{id}"),
                     );
                 }
                 "if" => {
@@ -6330,7 +6329,7 @@ mod reserved_keywords_in_policies {
                     assert_invalid_expression(
                         format!("principal has {id}"),
                         RESERVED_IDENT_MSG(id),
-                        format!("principal has {id}"),
+                        format!("{id}"),
                     );
                 }
                 _ => {