Improve error message in invalid template slot

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

@@ -1139,10 +1139,11 @@ fn interpret_primary(
                 }
             }
         }
-        cst::Primary::Slot(node) => match node.ok_or_missing()? {
-            cst::Slot::Principal => Ok(Either::Right(Expr::slot(ast::SlotId::principal()))),
-            cst::Slot::Resource => Ok(Either::Right(Expr::slot(ast::SlotId::resource()))),
-        },
+        cst::Primary::Slot(node) => Ok(Either::Right(Expr::slot(
+            node.ok_or_missing()?
+                .try_into()
+                .map_err(|e| node.to_ast_err(e))?,
+        ))),
         cst::Primary::Expr(e) => Ok(Either::Right(e.try_into()?)),
         cst::Primary::EList(nodes) => nodes
             .into_iter()

cedar-policy-core/src/parser.rs

@@ -774,8 +774,10 @@ mod parse_tests {
                 resource == ?blah
             };
             "#;
-        // TODO(#451): improve these errors
-        let error = ExpectedErrorMessage::error("invalid token");
+        let error = ExpectedErrorMessage::error_and_help(
+            "`?blah` is not a valid template slot",
+            "a template slot may only be `?principal` or `?resource`",
+        );
         assert_matches!(parse_policy(None, src), Err(e) => {
             expect_some_error_matches(src, &e, &error);
         });
@@ -868,8 +870,10 @@ mod parse_tests {
                 resource == ?blah
             };
             "#;
-        // TODO(#451): improve these errors
-        let error = ExpectedErrorMessage::error("invalid token");
+        let error = ExpectedErrorMessage::error_and_help(
+            "`?blah` is not a valid template slot",
+            "a template slot may only be `?principal` or `?resource`",
+        );
         assert_matches!(parse_policy(None, src), Err(e) => {
             expect_some_error_matches(src, &e, &error);
         });

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

@@ -369,6 +369,8 @@ pub enum Slot {
     Principal,
     /// Slot for Resource Constraints
     Resource,
+    /// Slot other than one of the valid slots
+    Other(SmolStr),
 }
 
 impl Slot {

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

@@ -1043,7 +1043,7 @@ enum OneOrMultipleRefs {
 
 impl RefKind for OneOrMultipleRefs {
     fn err_str() -> &'static str {
-        "an entity uid, set of entity uids, or template slot"
+        "an entity uid or set of entity uids"
     }
 
     fn create_slot(errs: &mut ParseErrors, span: miette::SourceSpan) -> Option<Self> {
@@ -1782,9 +1782,15 @@ impl ASTNode<Option<cst::Primary>> {
         let prim = maybe_prim?;
         match prim {
             cst::Primary::Slot(s) => {
+                // Call `create_slot` first so that we fail immediately if the
+                // `RefKind` does not permit slots, and only then complain if
+                // it's the wrong slot. This avoids getting an error
+                // `found ?action instead of ?action` when `action` doesn't
+                // support slots.
+                let slot_ref = T::create_slot(errs, self.loc)?;
                 let slot = s.as_inner()?;
                 if slot.matches(var) {
-                    T::create_slot(errs, self.loc)
+                    Some(slot_ref)
                 } else {
                     errs.push(self.to_ast_err(ToASTErrorKind::wrong_node(
                         T::err_str(),
@@ -1903,17 +1909,29 @@ impl ASTNode<Option<cst::Primary>> {
 }
 
 impl ASTNode<Option<cst::Slot>> {
-    fn into_expr(self, _errs: &mut ParseErrors) -> Option<ast::Expr> {
-        let (s, src) = self.into_inner();
-        s.map(|s| ast::ExprBuilder::new().with_source_span(src).slot(s.into()))
+    fn into_expr(&self, errs: &mut ParseErrors) -> Option<ast::Expr> {
+        match self.as_inner()?.try_into() {
+            Ok(slot_id) => Some(
+                ast::ExprBuilder::new()
+                    .with_source_span(self.loc)
+                    .slot(slot_id),
+            ),
+            Err(e) => {
+                errs.push(self.to_ast_err(e));
+                None
+            }
+        }
     }
 }
 
-impl From<cst::Slot> for ast::SlotId {
-    fn from(slot: cst::Slot) -> ast::SlotId {
+impl TryFrom<&cst::Slot> for ast::SlotId {
+    type Error = ToASTErrorKind;
+
+    fn try_from(slot: &cst::Slot) -> Result<Self, Self::Error> {
         match slot {
-            cst::Slot::Principal => ast::SlotId::principal(),
-            cst::Slot::Resource => ast::SlotId::resource(),
+            cst::Slot::Principal => Ok(ast::SlotId::principal()),
+            cst::Slot::Resource => Ok(ast::SlotId::resource()),
+            cst::Slot::Other(slot) => Err(ToASTErrorKind::InvalidSlot(slot.clone())),
         }
     }
 }
@@ -4189,4 +4207,39 @@ mod tests {
             });
         }
     }
+
+    #[test]
+    fn invalid_slot() {
+        let invalid_policies = [
+            (
+                r#"permit(principal, action, resource) when { principal == ?foo};"#,
+                ExpectedErrorMessage::error_and_help(
+                    "`?foo` is not a valid template slot",
+                    "a template slot may only be `?principal` or `?resource`",
+                )
+            ),
+            (
+                r#"permit(principal == ?foo, action, resource);"#,
+                ExpectedErrorMessage::error("expected an entity uid or matching template slot, found ?foo instead of ?principal"),
+            ),
+            (
+                r#"permit(principal, action == ?action, resource);"#,
+                ExpectedErrorMessage::error("expected single entity uid or set of entity uids, got: template slot"),
+            ),
+            (
+                r#"permit(principal, action in [?bar], resource);"#,
+                ExpectedErrorMessage::error("expected single entity uid, got: template slot"),
+            ),
+            (
+                r#"permit(principal, action, resource in ?baz);"#,
+                ExpectedErrorMessage::error("expected an entity uid or matching template slot, found ?baz instead of ?resource"),
+            ),
+        ];
+
+        for (p_src, expected) in invalid_policies {
+            assert_matches!(parse_policy_template(None, p_src), Err(e) => {
+                expect_err(p_src, &e, &expected);
+            });
+        }
+    }
 }

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

@@ -383,6 +383,10 @@ pub enum ToASTErrorKind {
     #[error(transparent)]
     #[diagnostic(transparent)]
     InvalidIs(#[from] InvalidIsError),
+    /// Returned when a policy contains a template slot other than `?principal` or `?resource`
+    #[error("`{0}` is not a valid template slot")]
+    #[diagnostic(help("a template slot may only be `?principal` or `?resource`"))]
+    InvalidSlot(SmolStr),
 }
 
 impl ToASTErrorKind {
@@ -568,6 +572,7 @@ lazy_static! {
         ("CONTEXT", "`context`"),
         ("PRINCIPAL_SLOT", "`?principal`"),
         ("RESOURCE_SLOT", "`?resource`"),
+        ("OTHER_SLOT", "template slot"),
         ("IDENTIFIER", "identifier"),
         ("NUMBER", "number"),
         ("STRINGLIT", "string literal"),

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

@@ -442,10 +442,11 @@ impl fmt::Display for Str {
 impl std::fmt::Display for Slot {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         let src = match self {
-            Slot::Principal => "principal",
-            Slot::Resource => "resource",
+            Slot::Principal => "?principal",
+            Slot::Resource => "?resource",
+            Slot::Other(slot) => slot.as_ref(),
         };
-        write!(f, "?{src}")
+        write!(f, "{src}")
     }
 }
 

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

@@ -44,6 +44,7 @@ match {
     // Valid slots, hardcoded for now, may be generalized later
     "?principal" => PRINCIPAL_SLOT,
     "?resource" => RESOURCE_SLOT,
+    r"\?[_a-zA-Z][_a-zA-Z0-9]*" => OTHER_SLOT,
 
     // data input
     r"[_a-zA-Z][_a-zA-Z0-9]*" => IDENTIFIER,
@@ -345,6 +346,8 @@ Slot: Node<Option<cst::Slot>> = {
         => Node::new(Some(cst::Slot::Principal), l, r),
     <l:@L> RESOURCE_SLOT <r:@R>
         => Node::new(Some(cst::Slot::Resource), l, r),
+    <l:@L> <s: OTHER_SLOT> <r:@R>
+        => Node::new(Some(cst::Slot::Other(s.into())), l, r),
 }
 
 // LITERAL   := BOOL | INT | STR

cedar-policy/CHANGELOG.md

@@ -35,6 +35,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Changed
 
+- Improve parser error message when a policy includes an invalid template slot.
+  The error now identifies that the policy used an invalid slot and suggests using
+  one of the valid slots.
 - Improve parser error messages to more reliably notice that a function or
   method does exists when it is called with an incorrect number of arguments or
   using the wrong call style.