Improve error on incorrect function/method calls (#482)

john-h-kastner-aws · web-flow · commit 481643297034 · 2023-12-08T15:49:57.000-05:00
diff --git a/cedar-policy-core/src/est/expr.rs b/cedar-policy-core/src/est/expr.rs
@@ -1266,19 +1266,25 @@ impl TryFrom<&ASTNode<Option<cst::Member>>> for Expr {
     }
 }
 
-fn extract_single_argument(
-    es: impl ExactSizeIterator<Item = Expr>,
+/// Return the single argument in `args` iterator, or return a wrong arity error
+/// if the iterator has 0 elements or more than 1 element.
+pub fn extract_single_argument<T>(
+    args: impl ExactSizeIterator<Item = T>,
     fn_name: &'static str,
     span: miette::SourceSpan,
-) -> Result<Expr, ParseErrors> {
-    let mut iter = es.fuse().peekable();
+) -> Result<T, ToASTError> {
+    let mut iter = args.fuse().peekable();
     let first = iter.next();
     let second = iter.peek();
     match (first, second) {
-        (None, _) => Err(ToASTError::new(ToASTErrorKind::wrong_arity(fn_name, 1, 0), span).into()),
-        (Some(_), Some(_)) => {
-            Err(ToASTError::new(ToASTErrorKind::wrong_arity(fn_name, 1, iter.len()), span).into())
-        }
+        (None, _) => Err(ToASTError::new(
+            ToASTErrorKind::wrong_arity(fn_name, 1, 0),
+            span,
+        )),
+        (Some(_), Some(_)) => Err(ToASTError::new(
+            ToASTErrorKind::wrong_arity(fn_name, 1, iter.len() + 1),
+            span,
+        )),
         (Some(first), None) => Ok(first),
     }
 }
diff --git a/cedar-policy-core/src/parser/cst_to_ast.rs b/cedar-policy-core/src/parser/cst_to_ast.rs
@@ -44,6 +44,7 @@ use crate::ast::{
     ExprConstructionError, PatternElem, PolicySetError, PrincipalConstraint,
     PrincipalOrResourceConstraint, ResourceConstraint,
 };
+use crate::est::extract_single_argument;
 use itertools::Either;
 use smol_str::SmolStr;
 use std::cmp::Ordering;
@@ -451,32 +452,37 @@ impl ast::Id {
         errs: &mut ParseErrors,
         span: miette::SourceSpan,
     ) -> Option<ast::Expr> {
-        let mut adj_args = args.iter_mut().peekable();
-        match (self.as_ref(), adj_args.next(), adj_args.peek()) {
-            ("contains", Some(a), None) => {
-                // move the value out of the argument, replacing it with a dummy,
-                // after this we can no longer use the original args
-                let arg = mem::replace(a, ast::Expr::val(false));
-                Some(construct_method_contains(e, arg, span))
-            }
-            ("containsAll", Some(a), None) => {
-                let arg = mem::replace(a, ast::Expr::val(false));
-                Some(construct_method_contains_all(e, arg, span))
-            }
-            ("containsAny", Some(a), None) => {
-                let arg = mem::replace(a, ast::Expr::val(false));
-                Some(construct_method_contains_any(e, arg, span))
-            }
-            (name, _, _) => {
-                if EXTENSION_STYLES.methods.contains(&name) {
+        match self.as_ref() {
+            "contains" => extract_single_argument(args.into_iter(), "contains", span)
+                .map(|arg| construct_method_contains(e, arg, span))
+                .map_err(|err| errs.push(err))
+                .ok(),
+            "containsAll" => extract_single_argument(args.into_iter(), "containsAll", span)
+                .map(|arg| construct_method_contains_all(e, arg, span))
+                .map_err(|err| errs.push(err))
+                .ok(),
+            "containsAny" => extract_single_argument(args.into_iter(), "containsAny", span)
+                .map(|arg| construct_method_contains_any(e, arg, span))
+                .map_err(|err| errs.push(err))
+                .ok(),
+            id => {
+                if EXTENSION_STYLES.methods.contains(&id) {
                     args.insert(0, e);
                     // INVARIANT (MethodStyleArgs), we call insert above, so args is non-empty
-                    Some(construct_ext_meth(name.to_string(), args, span))
+                    Some(construct_ext_meth(id.to_string(), args, span))
                 } else {
-                    errs.push(ToASTError::new(
-                        ToASTErrorKind::InvalidMethodName(name.to_string()),
-                        span,
-                    ));
+                    let unqual_name = ast::Name::unqualified_name(self.clone());
+                    if EXTENSION_STYLES.functions.contains(&unqual_name) {
+                        errs.push(ToASTError::new(
+                            ToASTErrorKind::MethodCallOnFunction(unqual_name.id),
+                            span,
+                        ));
+                    } else {
+                        errs.push(ToASTError::new(
+                            ToASTErrorKind::InvalidMethodName(id.to_string()),
+                            span,
+                        ));
+                    }
                     None
                 }
             }
@@ -2008,15 +2014,14 @@ impl ast::Name {
         // error on standard methods
         if self.path.is_empty() {
             let id = self.id.as_ref();
-            match id {
-                "contains" | "containsAll" | "containsAny" => {
-                    errs.push(ToASTError::new(
-                        ToASTErrorKind::FunctionCallOnMethod(self.id),
-                        span,
-                    ));
-                    return None;
-                }
-                _ => {}
+            if EXTENSION_STYLES.methods.contains(id)
+                || matches!(id, "contains" | "containsAll" | "containsAny")
+            {
+                errs.push(ToASTError::new(
+                    ToASTErrorKind::FunctionCallOnMethod(self.id),
+                    span,
+                ));
+                return None;
             }
         }
         if EXTENSION_STYLES.functions.contains(&self) {
@@ -4116,4 +4121,72 @@ mod tests {
             }
         );
     }
+
+    #[test]
+    fn invalid_methods_function_calls() {
+        let invalid_exprs = [
+            (
+                r#"contains([], 1)"#,
+                ExpectedErrorMessage::error_and_help(
+                    "`contains` is a method, not a function",
+                    "use a method-style call: `e.contains(..)`",
+                ),
+            ),
+            (
+                r#"[].contains()"#,
+                ExpectedErrorMessage::error(
+                    "call to `contains` requires exactly 1 argument, but got 0 arguments",
+                ),
+            ),
+            (
+                r#"[].contains(1, 2)"#,
+                ExpectedErrorMessage::error(
+                    "call to `contains` requires exactly 1 argument, but got 2 arguments",
+                ),
+            ),
+            (
+                r#"[].containsAll()"#,
+                ExpectedErrorMessage::error(
+                    "call to `containsAll` requires exactly 1 argument, but got 0 arguments",
+                ),
+            ),
+            (
+                r#"[].containsAll(1, 2)"#,
+                ExpectedErrorMessage::error(
+                    "call to `containsAll` requires exactly 1 argument, but got 2 arguments",
+                ),
+            ),
+            (
+                r#"[].containsAny()"#,
+                ExpectedErrorMessage::error(
+                    "call to `containsAny` requires exactly 1 argument, but got 0 arguments",
+                ),
+            ),
+            (
+                r#"[].containsAny(1, 2)"#,
+                ExpectedErrorMessage::error(
+                    "call to `containsAny` requires exactly 1 argument, but got 2 arguments",
+                ),
+            ),
+            (
+                r#""1.1.1.1".ip()"#,
+                ExpectedErrorMessage::error_and_help(
+                    "`ip` is a function, not a method",
+                    "use a function-style call: `ip(..)`",
+                ),
+            ),
+            (
+                r#"greaterThan(1, 2)"#,
+                ExpectedErrorMessage::error_and_help(
+                    "`greaterThan` is a method, not a function",
+                    "use a method-style call: `e.greaterThan(..)`",
+                ),
+            ),
+        ];
+        for (src, expected) in invalid_exprs {
+            assert_matches!(parse_expr(src), Err(e) => {
+                expect_err(src, &e, &expected);
+            });
+        }
+    }
 }
diff --git a/cedar-policy-core/src/parser/err.rs b/cedar-policy-core/src/parser/err.rs
@@ -260,6 +260,10 @@ pub enum ToASTErrorKind {
     #[error("`{0}` is a method, not a function")]
     #[diagnostic(help("use a method-style call: `e.{0}(..)`"))]
     FunctionCallOnMethod(crate::ast::Id),
+    /// Returned when a policy attempts to call a function in the method style
+    #[error("`{0}` is a function, not a method")]
+    #[diagnostic(help("use a function-style call: `{0}(..)`"))]
+    MethodCallOnFunction(crate::ast::Id),
     /// Returned when the right hand side of a `like` expression is not a constant pattern literal
     #[error("right hand side of a `like` expression must be a pattern literal, but got `{0}`")]
     InvalidPattern(String),
@@ -357,10 +361,10 @@ pub enum ToASTErrorKind {
     /// Returned when a CST expression is invalid
     #[error("`{0}` is not a valid expression")]
     InvalidExpression(cst::Name),
-    /// Returned when a function has wrong arity
+    /// Returned when a function or method is called with the wrong arity
     #[error("call to `{name}` requires exactly {expected} argument{}, but got {got} argument{}", if .expected == &1 { "" } else { "s" }, if .got == &1 { "" } else { "s" })]
     WrongArity {
-        /// Name of the function being called
+        /// Name of the function or method being called
         name: &'static str,
         /// The expected number of arguments
         expected: usize,
diff --git a/cedar-policy/CHANGELOG.md b/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 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.
 - Include source spans on more parser error messages.
 - Better integration with `miette` for various error types. If you have
   previously been just using the `Display` trait to get the error message from a
