Minor edits to ParseError (#908)

Author: khieta

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

@@ -71,7 +71,7 @@ impl std::fmt::Display for Literal {
 }
 
 impl std::str::FromStr for Literal {
-    type Err = parser::err::ParseErrors;
+    type Err = parser::err::LiteralParseError;
 
     fn from_str(s: &str) -> Result<Literal, Self::Err> {
         parser::parse_literal(s)

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

@@ -297,7 +297,7 @@ pub enum PartialValueToRestrictedExprError {
 }
 
 impl std::str::FromStr for RestrictedExpr {
-    type Err = RestrictedExprParseError;
+    type Err = RestrictedExpressionParseError;
 
     fn from_str(s: &str) -> Result<RestrictedExpr, Self::Err> {
         parser::parse_restrictedexpr(s)
@@ -596,7 +596,7 @@ impl<'a> Hash for RestrictedExprShapeOnly<'a> {
 }
 
 /// Error when constructing a restricted expression from unrestricted
-
+/// expression
 #[derive(Debug, Clone, PartialEq, Eq, Error)]
 pub enum RestrictedExprError {
     /// An expression was expected to be a "restricted" expression, but contained
@@ -654,17 +654,21 @@ impl RestrictedExprError {
 }
 
 /// Errors possible from `RestrictedExpr::from_str()`
+//
+// CAUTION: this type is publicly exported in `cedar-policy`.
+// Don't make fields `pub`, don't make breaking changes, and use caution when
+// adding public methods.
 #[derive(Debug, Clone, PartialEq, Eq, Diagnostic, Error)]
-pub enum RestrictedExprParseError {
-    /// Failed to parse the expression entirely
-    #[error("failed to parse restricted expression: {0}")]
+pub enum RestrictedExpressionParseError {
+    /// Failed to parse the expression
+    #[error(transparent)]
     #[diagnostic(transparent)]
     Parse(#[from] ParseErrors),
     /// Parsed successfully as an expression, but failed to construct a
     /// restricted expression, for the reason indicated in the underlying error
     #[error(transparent)]
     #[diagnostic(transparent)]
-    RestrictedExpr(#[from] RestrictedExprError),
+    InvalidRestrictedExpression(#[from] RestrictedExprError),
 }
 
 #[cfg(test)]
@@ -721,7 +725,7 @@ mod test {
         let str = r#"{ foo: 37, bar: "hi", foo: 101 }"#;
         assert_eq!(
             RestrictedExpr::from_str(str),
-            Err(RestrictedExprParseError::Parse(ParseErrors(vec![
+            Err(RestrictedExpressionParseError::Parse(ParseErrors(vec![
                 ParseError::ToAST(ToASTError::new(
                     ToASTErrorKind::ExprConstructionError(
                         ExprConstructionError::DuplicateKeyInRecordLiteral { key: "foo".into() }

cedar-policy-core/src/parser.rs

@@ -40,7 +40,7 @@ use smol_str::SmolStr;
 use std::collections::HashMap;
 
 use crate::ast;
-use crate::ast::RestrictedExprParseError;
+use crate::ast::RestrictedExpressionParseError;
 use crate::est;
 
 /// simple main function for parsing policies
@@ -243,7 +243,7 @@ pub(crate) fn parse_expr(ptext: &str) -> Result<ast::Expr, err::ParseErrors> {
 /// `FromStr` impl or its constructors
 pub(crate) fn parse_restrictedexpr(
     ptext: &str,
-) -> Result<ast::RestrictedExpr, RestrictedExprParseError> {
+) -> Result<ast::RestrictedExpr, RestrictedExpressionParseError> {
     let expr = parse_expr(ptext)?;
     Ok(ast::RestrictedExpr::new(expr)?)
 }
@@ -286,24 +286,19 @@ pub(crate) fn parse_name(name: &str) -> Result<ast::Name, err::ParseErrors> {
 ///
 /// Private to this crate. Users outside Core should use `Literal`'s `FromStr` impl
 /// or its constructors
-pub(crate) fn parse_literal(val: &str) -> Result<ast::Literal, err::ParseErrors> {
+pub(crate) fn parse_literal(val: &str) -> Result<ast::Literal, err::LiteralParseError> {
     let mut errs = err::ParseErrors::new();
     let cst = text_to_cst::parse_primary(val)?;
     let Some(ast) = cst.to_expr(&mut errs) else {
-        return Err(errs);
+        return Err(err::LiteralParseError::Parse(errs));
     };
     if errs.is_empty() {
-        match ast.into_expr_kind() {
-            ast::ExprKind::Lit(v) => Ok(v),
-            _ => Err(
-                err::ParseError::ParseLiteral(err::ParseLiteralError::ParseLiteral(
-                    val.to_string(),
-                ))
-                .into(),
-            ),
+        match ast.expr_kind() {
+            ast::ExprKind::Lit(v) => Ok(v.clone()),
+            _ => Err(err::LiteralParseError::InvalidLiteral(ast)),
         }
     } else {
-        Err(errs)
+        Err(err::LiteralParseError::Parse(errs))
     }
 }
 
@@ -1146,4 +1141,40 @@ mod parse_tests {
             "expected `!=`, `&&`, `(`, `*`, `+`, `-`, `.`, `::`, `<`, `<=`, `==`, `>`, `>=`, `[`, `||`, `has`, `in`, `is`, `like`, or `then`",
         )
     }
+
+    #[test]
+    fn string_escapes() {
+        // test strings with valid escapes
+        // convert a string `s` to `<double-quote> <escaped-form-of-s> <double-quote>`
+        // and test if the resulting string literal AST contains exactly `s`
+        // for instance, "\u{1F408}"" is converted into r#""\u{1F408}""#,
+        // the latter should be parsed into `Literal(String("🐈"))` and
+        // `🐈` is represented by '\u{1F408}'
+        let test_valid = |s: &str| {
+            let r = parse_literal(&format!("\"{}\"", s.escape_default()));
+            assert_eq!(r, Ok(ast::Literal::String(s.into())));
+        };
+        test_valid("\t");
+        test_valid("\0");
+        test_valid("👍");
+        test_valid("🐈");
+        test_valid("\u{1F408}");
+        test_valid("abc\tde\\fg");
+        test_valid("aaa\u{1F408}bcd👍👍👍");
+        // test string with invalid escapes
+        let test_invalid = |s: &str, en: usize| {
+            let r = parse_literal(&format!("\"{}\"", s));
+            assert_matches!(r, Err(err::LiteralParseError::Parse(errs)) => {
+                assert_eq!(errs.len(), en);
+            });
+        };
+        // invalid escape `\a`
+        test_invalid("\\a", 1);
+        // invalid escape `\b`
+        test_invalid("\\b", 1);
+        // invalid escape `\p`
+        test_invalid("\\\\aa\\p", 1);
+        // invalid escape `\a` and empty unicode escape
+        test_invalid(r"\aaa\u{}", 2);
+    }
 }

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

@@ -3459,11 +3459,9 @@ mod tests {
         expect_some_error_matches(
             src,
             &errs,
-            &ExpectedErrorMessageBuilder::error(
-                "the input `\\*` is not a valid escape: InvalidEscape",
-            )
-            .exactly_one_underline(r#""string\*with\*escaped\*stars""#)
-            .build(),
+            &ExpectedErrorMessageBuilder::error("the input `\\*` is not a valid escape")
+                .exactly_one_underline(r#""string\*with\*escaped\*stars""#)
+                .build(),
         );
     }
 
@@ -3723,47 +3721,11 @@ mod tests {
         );
     }
 
-    #[test]
-    fn string_escapes() {
-        // test strings with valid escapes
-        // convert a string `s` to `<double-quote> <escaped-form-of-s> <double-quote>`
-        // and test if the resulting string literal AST contains exactly `s`
-        // for instance, "\u{1F408}"" is converted into r#""\u{1F408}""#,
-        // the latter should be parsed into `Literal(String("🐈"))` and
-        // `🐈` is represented by '\u{1F408}'
-        let test_valid = |s: &str| {
-            let r = parse_literal(&format!("\"{}\"", s.escape_default()));
-            assert!(r.is_ok());
-            assert_eq!(r.unwrap(), ast::Literal::String(s.into()));
-        };
-        test_valid("\t");
-        test_valid("\0");
-        test_valid("👍");
-        test_valid("🐈");
-        test_valid("\u{1F408}");
-        test_valid("abc\tde\\fg");
-        test_valid("aaa\u{1F408}bcd👍👍👍");
-        // test string with invalid escapes
-        let test_invalid = |s: &str, en: usize| {
-            let r = parse_literal(&format!("\"{}\"", s));
-            assert!(r.is_err());
-            assert_eq!(r.unwrap_err().len(), en);
-        };
-        // invalid escape `\a`
-        test_invalid("\\a", 1);
-        // invalid escape `\b`
-        test_invalid("\\b", 1);
-        // invalid escape `\p`
-        test_invalid("\\\\aa\\p", 1);
-        // invalid escape `\a` and empty unicode escape
-        test_invalid(r"\aaa\u{}", 2);
-    }
-
     #[test]
     fn unescape_err_positions() {
         let assert_invalid_escape = |p_src, underline| {
             assert_matches!(parse_policy_template(None, p_src), Err(e) => {
-                expect_err(p_src, &miette::Report::new(e), &ExpectedErrorMessageBuilder::error("the input `\\q` is not a valid escape: InvalidEscape").exactly_one_underline(underline).build());
+                expect_err(p_src, &miette::Report::new(e), &ExpectedErrorMessageBuilder::error("the input `\\q` is not a valid escape").exactly_one_underline(underline).build());
             });
         };
         assert_invalid_escape(

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

@@ -26,7 +26,7 @@ use miette::{Diagnostic, LabeledSpan, SourceSpan};
 use smol_str::SmolStr;
 use thiserror::Error;
 
-use crate::ast::{self, ExprConstructionError, InputInteger, PolicyID, RestrictedExprError, Var};
+use crate::ast::{self, Expr, ExprConstructionError, InputInteger, PolicyID, Var};
 use crate::parser::fmt::join_with_conjunction;
 use crate::parser::loc::Loc;
 use crate::parser::node::Node;
@@ -43,55 +43,36 @@ pub(crate) type RawErrorRecovery<'a> = lalr::ErrorRecovery<RawLocation, RawToken
 
 type OwnedRawParseError = lalr::ParseError<RawLocation, String, RawUserError>;
 
-/// For errors during parsing
+/// Errors that can occur when parsing Cedar policies or expressions.
+//
+// CAUTION: this type is publicly exported in `cedar-policy`.
+// Don't make fields `pub`, don't make breaking changes, and use caution when
+// adding public methods.
 #[derive(Clone, Debug, Diagnostic, Error, PartialEq, Eq)]
 pub enum ParseError {
-    /// Error from the CST parser.
+    /// Error from the text -> CST parser
     #[error(transparent)]
     #[diagnostic(transparent)]
     ToCST(#[from] ToCSTError),
-    /// Error in the CST -> AST transform, mostly well-formedness issues.
+    /// Error from the CST -> AST transform
     #[error(transparent)]
     #[diagnostic(transparent)]
     ToAST(#[from] ToASTError),
-    /// Error concerning restricted expressions.
-    #[error(transparent)]
-    #[diagnostic(transparent)]
-    RestrictedExpr(#[from] RestrictedExprError),
-    /// Errors concerning parsing literals on their own
-    #[error(transparent)]
-    #[diagnostic(transparent)]
-    ParseLiteral(#[from] ParseLiteralError),
 }
 
-impl ParseError {
-    /// Extract a primary source span locating the error, if one is available.
-    pub fn primary_source_span(&self) -> Option<SourceSpan> {
-        match self {
-            ParseError::ToCST(to_cst_err) => Some(to_cst_err.primary_source_span()),
-            ParseError::ToAST(to_ast_err) => Some(to_ast_err.source_loc().span),
-            ParseError::RestrictedExpr(restricted_expr_err) => match restricted_expr_err {
-                RestrictedExprError::InvalidRestrictedExpression { expr, .. } => {
-                    expr.source_loc().map(|loc| loc.span)
-                }
-            },
-            ParseError::ParseLiteral(parse_lit_err) => parse_lit_err
-                .labels()
-                .and_then(|mut it| it.next().map(|lspan| *lspan.inner())),
-        }
-    }
-}
-
-/// Errors in the top-level parse literal entrypoint
+/// Errors possible from `Literal::from_str()`
 #[derive(Debug, Clone, PartialEq, Diagnostic, Error, Eq)]
-pub enum ParseLiteralError {
-    /// The top-level parser endpoint for parsing a literal encountered a non-literal.
-    /// Since this can be any possible other expression, we just return it as a string.
-    #[error("`{0}` is not a literal")]
-    ParseLiteral(String),
+pub enum LiteralParseError {
+    /// Failed to parse the input
+    #[error(transparent)]
+    #[diagnostic(transparent)]
+    Parse(#[from] ParseErrors),
+    /// Parsed successfully as an expression, but failed to construct a literal
+    #[error("invalid literal: `{0}`")]
+    InvalidLiteral(Expr),
 }
 
-/// Errors in the CST -> AST transform, mostly well-formedness issues.
+/// Error from the CST -> AST transform
 #[derive(Debug, Error, Clone, PartialEq, Eq)]
 #[error("{kind}")]
 pub struct ToASTError {
@@ -487,7 +468,7 @@ pub enum InvalidIsError {
     WrongOp(cst::RelOp),
 }
 
-/// Error from the CST parser.
+/// Error from the text -> CST parser
 #[derive(Clone, Debug, Error, PartialEq, Eq)]
 pub struct ToCSTError {
     err: OwnedRawParseError,
@@ -685,6 +666,10 @@ pub fn expected_to_string(expected: &[String], config: &ExpectedTokenConfig) ->
 }
 
 /// Multiple parse errors.
+//
+// CAUTION: this type is publicly exported in `cedar-policy`.
+// Don't make fields `pub`, don't make breaking changes, and use caution when
+// adding public methods.
 #[derive(Clone, Debug, Default, PartialEq, Eq)]
 pub struct ParseErrors(pub Vec<ParseError>);
 
@@ -696,13 +681,8 @@ impl ParseErrors {
         ParseErrors(Vec::new())
     }
 
-    /// Constructs a new, empty `ParseErrors` with the specified capacity.
-    pub fn with_capacity(capacity: usize) -> Self {
-        ParseErrors(Vec::with_capacity(capacity))
-    }
-
     /// Add an error to the `ParseErrors`
-    pub(super) fn push(&mut self, err: impl Into<ParseError>) {
+    pub(crate) fn push(&mut self, err: impl Into<ParseError>) {
         self.0.push(err.into());
     }
 }

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

@@ -132,9 +132,8 @@ impl std::fmt::Display for UnescapeError {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         write!(
             f,
-            "the input `{}` is not a valid escape: {:?}",
+            "the input `{}` is not a valid escape",
             &self.input[self.range.clone()],
-            &self.err
         )
     }
 }

cedar-policy/src/api.rs

@@ -32,9 +32,7 @@ pub use authorizer::Decision;
 use cedar_policy_core::ast;
 #[cfg(feature = "partial-eval")]
 use cedar_policy_core::ast::BorrowedRestrictedExpr;
-use cedar_policy_core::ast::{
-    ContextCreationError, ExprConstructionError, Integer, RestrictedExprParseError,
-}; // `ContextCreationError` is unsuitable for `pub use` because it contains internal types like `RestrictedExpr`
+use cedar_policy_core::ast::{ContextCreationError, ExprConstructionError, Integer}; // `ContextCreationError` is unsuitable for `pub use` because it contains internal types like `RestrictedExpr`
 use cedar_policy_core::authorizer;
 use cedar_policy_core::entities::{ContextSchema, Dereference};
 use cedar_policy_core::est;
@@ -2927,7 +2925,7 @@ fn ip_extension_name() -> ast::Name {
 }
 
 impl FromStr for RestrictedExpression {
-    type Err = RestrictedExprParseError;
+    type Err = RestrictedExpressionParseError;
 
     /// create a `RestrictedExpression` using Cedar syntax
     fn from_str(expression: &str) -> Result<Self, Self::Err> {

cedar-policy/src/api/err.rs

@@ -21,14 +21,15 @@ use crate::EntityUid;
 use crate::PolicyId;
 use cedar_policy_core::ast;
 use cedar_policy_core::ast::Name;
+pub use cedar_policy_core::ast::RestrictedExpressionParseError;
 use cedar_policy_core::authorizer;
 use cedar_policy_core::est;
 pub use cedar_policy_core::evaluator::{evaluation_errors, EvaluationError};
 pub use cedar_policy_core::extensions::{
     extension_function_lookup_errors, ExtensionFunctionLookupError,
 };
 use cedar_policy_core::parser;
-pub use cedar_policy_core::parser::err::ParseErrors;
+pub use cedar_policy_core::parser::err::{ParseError, ParseErrors};
 pub use cedar_policy_validator::human_schema::SchemaWarning;
 pub use cedar_policy_validator::{
     TypeErrorKind, UnsupportedFeature, ValidationErrorKind, ValidationWarningKind,