From 3b0f5930bcd1d2e1a8708c952f0c31c8c8da30e7 Mon Sep 17 00:00:00 2001 From: Craig Disselkoen Date: Mon, 18 Dec 2023 14:08:54 -0800 Subject: [PATCH] carry source code in `Loc` struct (#516) --- cedar-policy-cli/src/lib.rs | 8 +- cedar-policy-core/src/ast/expr.rs | 65 +-- cedar-policy-core/src/ast/policy.rs | 6 +- cedar-policy-core/src/ast/restricted_expr.rs | 7 +- cedar-policy-core/src/est.rs | 4 +- cedar-policy-core/src/est/expr.rs | 56 +- cedar-policy-core/src/from_normalized_str.rs | 3 +- cedar-policy-core/src/parser.rs | 10 +- cedar-policy-core/src/parser/cst_to_ast.rs | 497 +++++++++--------- cedar-policy-core/src/parser/err.rs | 36 +- cedar-policy-core/src/parser/grammar.lalrpop | 192 +++---- cedar-policy-core/src/parser/loc.rs | 41 ++ cedar-policy-core/src/parser/node.rs | 32 +- cedar-policy-core/src/parser/text_to_cst.rs | 39 +- cedar-policy-formatter/src/pprint/doc.rs | 112 ++-- cedar-policy-validator/src/expr_iterator.rs | 64 ++- cedar-policy-validator/src/schema.rs | 12 +- cedar-policy-validator/src/str_checks.rs | 14 +- cedar-policy-validator/src/type_error.rs | 86 +-- cedar-policy-validator/src/typecheck.rs | 142 +++-- .../src/validation_result.rs | 25 +- cedar-policy/src/api.rs | 41 +- cedar-policy/src/tests.rs | 82 +++ 23 files changed, 840 insertions(+), 734 deletions(-) create mode 100644 cedar-policy-core/src/parser/loc.rs diff --git a/cedar-policy-cli/src/lib.rs b/cedar-policy-cli/src/lib.rs index af7e51397..808ffeb9b 100644 --- a/cedar-policy-cli/src/lib.rs +++ b/cedar-policy-cli/src/lib.rs @@ -956,9 +956,7 @@ fn read_from_file(filename: impl AsRef, context: &str) -> Result { /// Read a policy set, in Cedar human syntax, from the file given in `filename`, /// or from stdin if `filename` is `None`. -fn read_policy_set( - filename: Option + std::marker::Copy>, -) -> miette::Result { +fn read_policy_set(filename: Option + std::marker::Copy>) -> Result { let context = "policy set"; let ps_str = read_from_file_or_stdin(filename, context)?; let ps = PolicySet::from_str(&ps_str) @@ -975,9 +973,7 @@ fn read_policy_set( /// Read a policy, in Cedar JSON (EST) syntax, from the file given in `filename`, /// or from stdin if `filename` is `None`. -fn read_json_policy( - filename: Option + std::marker::Copy>, -) -> miette::Result { +fn read_json_policy(filename: Option + std::marker::Copy>) -> Result { let context = "JSON policy"; let json = match filename.as_ref() { Some(path) => { diff --git a/cedar-policy-core/src/ast/expr.rs b/cedar-policy-core/src/ast/expr.rs index 7db2ec25a..6c0988297 100644 --- a/cedar-policy-core/src/ast/expr.rs +++ b/cedar-policy-core/src/ast/expr.rs @@ -14,7 +14,7 @@ * limitations under the License. */ -use crate::{ast::*, parser::err::ParseErrors}; +use crate::{ast::*, parser::err::ParseErrors, parser::Loc}; use miette::Diagnostic; use serde::{Deserialize, Serialize}; use smol_str::SmolStr; @@ -35,7 +35,7 @@ use thiserror::Error; #[derive(Serialize, Deserialize, Hash, Debug, Clone, PartialEq, Eq)] pub struct Expr { expr_kind: ExprKind, - source_span: Option, + source_loc: Option, data: T, } @@ -183,10 +183,10 @@ impl From for Expr { } impl Expr { - fn new(expr_kind: ExprKind, source_span: Option, data: T) -> Self { + fn new(expr_kind: ExprKind, source_loc: Option, data: T) -> Self { Self { expr_kind, - source_span, + source_loc, data, } } @@ -198,7 +198,7 @@ impl Expr { &self.expr_kind } - /// Access the inner `ExprKind`, taking ownership. + /// Access the inner `ExprKind`, taking ownership and consuming the `Expr`. pub fn into_expr_kind(self) -> ExprKind { self.expr_kind } @@ -208,14 +208,15 @@ impl Expr { &self.data } - /// Access the data stored on the `Expr`, taking ownership. + /// Access the data stored on the `Expr`, taking ownership and consuming the + /// `Expr`. pub fn into_data(self) -> T { self.data } - /// Access the `SourceSpan` stored on the `Expr`. - pub fn source_span(&self) -> Option { - self.source_span + /// Access the `Loc` stored on the `Expr`. + pub fn source_loc(&self) -> Option<&Loc> { + self.source_loc.as_ref() } /// Update the data for this `Expr`. A convenient function used by the @@ -654,10 +655,10 @@ impl std::fmt::Display for Unknown { } /// Builder for constructing `Expr` objects annotated with some `data` -/// (possibly taking default value) and optionally a `source_span`. +/// (possibly taking default value) and optionally a `source_loc`. #[derive(Debug)] pub struct ExprBuilder { - source_span: Option, + source_loc: Option, data: T, } @@ -669,7 +670,7 @@ where /// takes a default value. pub fn new() -> Self { Self { - source_span: None, + source_loc: None, data: T::default(), } } @@ -678,8 +679,8 @@ where /// Defined only for `T: Default` because the caller would otherwise need to /// provide a `data` for the intermediate `not` Expr node. pub fn noteq(self, e1: Expr, e2: Expr) -> Expr { - match self.source_span { - Some(source_span) => ExprBuilder::new().with_source_span(source_span), + match &self.source_loc { + Some(source_loc) => ExprBuilder::new().with_source_loc(source_loc.clone()), None => ExprBuilder::new(), } .not(self.with_expr_kind(ExprKind::BinaryApp { @@ -698,40 +699,40 @@ impl Default for ExprBuilder { impl ExprBuilder { /// Construct a new `ExprBuild` where the specified data will be stored on - /// the `Expr`. This constructor does not populate the `source_span` field, - /// so `with_source_span` should be called if constructing an `Expr` where + /// the `Expr`. This constructor does not populate the `source_loc` field, + /// so `with_source_loc` should be called if constructing an `Expr` where /// the source location is known. pub fn with_data(data: T) -> Self { Self { - source_span: None, + source_loc: None, data, } } /// Update the `ExprBuilder` to build an expression with some known location /// in policy source code. - pub fn with_source_span(self, source_span: miette::SourceSpan) -> Self { - self.with_maybe_source_span(Some(source_span)) + pub fn with_source_loc(self, source_loc: Loc) -> Self { + self.with_maybe_source_loc(Some(source_loc)) } /// Utility used the validator to get an expression with the same source /// location as an existing expression. This is done when reconstructing the /// `Expr` with type information. - pub fn with_same_source_span(self, expr: &Expr) -> Self { - self.with_maybe_source_span(expr.source_span) + pub fn with_same_source_loc(self, expr: &Expr) -> Self { + self.with_maybe_source_loc(expr.source_loc.clone()) } - /// internally used to update SourceSpan to the given `Some` or `None` - fn with_maybe_source_span(mut self, maybe_source_span: Option) -> Self { - self.source_span = maybe_source_span; + /// internally used to update `.source_loc` to the given `Some` or `None` + fn with_maybe_source_loc(mut self, maybe_source_loc: Option) -> Self { + self.source_loc = maybe_source_loc; self } /// Internally used by the following methods to construct an `Expr` - /// containing the `data` and `source_span` in this `ExprBuilder` with some + /// containing the `data` and `source_loc` in this `ExprBuilder` with some /// inner `ExprKind`. fn with_expr_kind(self, expr_kind: ExprKind) -> Expr { - Expr::new(expr_kind, self.source_span, self.data) + Expr::new(expr_kind, self.source_loc, self.data) } /// Create an `Expr` that's just a single `Literal`. @@ -1018,11 +1019,11 @@ impl ExprBuilder { /// /// This may create multiple AST `&&` nodes. If it does, all the nodes will have the same /// source location and the same `T` data (taken from this builder) unless overridden, e.g., - /// with another call to `with_source_span()`. + /// with another call to `with_source_loc()`. pub fn and_nary(self, first: Expr, others: impl IntoIterator>) -> Expr { others.into_iter().fold(first, |acc, next| { Self::with_data(self.data.clone()) - .with_maybe_source_span(self.source_span) + .with_maybe_source_loc(self.source_loc.clone()) .and(acc, next) }) } @@ -1033,11 +1034,11 @@ impl ExprBuilder { /// /// This may create multiple AST `||` nodes. If it does, all the nodes will have the same /// source location and the same `T` data (taken from this builder) unless overridden, e.g., - /// with another call to `with_source_span()`. + /// with another call to `with_source_loc()`. pub fn or_nary(self, first: Expr, others: impl IntoIterator>) -> Expr { others.into_iter().fold(first, |acc, next| { Self::with_data(self.data.clone()) - .with_maybe_source_span(self.source_span) + .with_maybe_source_loc(self.source_loc.clone()) .or(acc, next) }) } @@ -1046,7 +1047,7 @@ impl ExprBuilder { pub fn greater(self, e1: Expr, e2: Expr) -> Expr { // e1 > e2 is defined as !(e1 <= e2) let leq = Self::with_data(self.data.clone()) - .with_maybe_source_span(self.source_span) + .with_maybe_source_loc(self.source_loc.clone()) .lesseq(e1, e2); self.not(leq) } @@ -1055,7 +1056,7 @@ impl ExprBuilder { pub fn greatereq(self, e1: Expr, e2: Expr) -> Expr { // e1 >= e2 is defined as !(e1 < e2) let leq = Self::with_data(self.data.clone()) - .with_maybe_source_span(self.source_span) + .with_maybe_source_loc(self.source_loc.clone()) .less(e1, e2); self.not(leq) } diff --git a/cedar-policy-core/src/ast/policy.rs b/cedar-policy-core/src/ast/policy.rs index 44f55f599..53a686299 100644 --- a/cedar-policy-core/src/ast/policy.rs +++ b/cedar-policy-core/src/ast/policy.rs @@ -1660,7 +1660,7 @@ mod test { ast::{entity, name, EntityUID}, parser::{ err::{ParseError, ParseErrors, ToASTError, ToASTErrorKind}, - parse_policy, + parse_policy, Loc, }, }; @@ -2008,7 +2008,7 @@ mod test { ToASTErrorKind::UnexpectedTemplate { slot: crate::parser::cst::Slot::Principal }, - miette::SourceSpan::from(0..50) + Loc::new(0..50, Arc::from(policy_str)), )) ); assert_eq!(errs.len(), 1); @@ -2019,7 +2019,7 @@ mod test { ToASTErrorKind::UnexpectedTemplate { slot: crate::parser::cst::Slot::Principal }, - miette::SourceSpan::from(50..74) + Loc::new(50..74, Arc::from(policy_str)), )))); assert_eq!(errs.len(), 2); } diff --git a/cedar-policy-core/src/ast/restricted_expr.rs b/cedar-policy-core/src/ast/restricted_expr.rs index 2950bf42f..fdb6c950e 100644 --- a/cedar-policy-core/src/ast/restricted_expr.rs +++ b/cedar-policy-core/src/ast/restricted_expr.rs @@ -623,7 +623,9 @@ pub enum RestrictedExprParseError { mod test { use super::*; use crate::parser::err::{ParseError, ToASTError, ToASTErrorKind}; + use crate::parser::Loc; use std::str::FromStr; + use std::sync::Arc; #[test] fn duplicate_key() { @@ -667,12 +669,13 @@ mod test { ); // duplicate key is also an error when parsing from string + let str = r#"{ foo: 37, bar: "hi", foo: 101 }"#; assert_eq!( - RestrictedExpr::from_str(r#"{ foo: 37, bar: "hi", foo: 101 }"#), + RestrictedExpr::from_str(str), Err(RestrictedExprParseError::Parse(ParseErrors(vec![ ParseError::ToAST(ToASTError::new( ToASTErrorKind::DuplicateKeyInRecordLiteral { key: "foo".into() }, - miette::SourceSpan::from(0..32) + Loc::new(0..32, Arc::from(str)) )) ]))), ) diff --git a/cedar-policy-core/src/est.rs b/cedar-policy-core/src/est.rs index 13c8271a6..4b10fe568 100644 --- a/cedar-policy-core/src/est.rs +++ b/cedar-policy-core/src/est.rs @@ -118,11 +118,11 @@ impl TryFrom for Policy { .conds .into_iter() .map(|node| { - let (cond, span) = node.into_inner(); + let (cond, loc) = node.into_inner(); let cond = cond.ok_or_else(|| { ParseErrors(vec![ToASTError::new( ToASTErrorKind::EmptyClause(None), - span, + loc, ) .into()]) })?; diff --git a/cedar-policy-core/src/est/expr.rs b/cedar-policy-core/src/est/expr.rs index 84277c005..cef51141b 100644 --- a/cedar-policy-core/src/est/expr.rs +++ b/cedar-policy-core/src/est/expr.rs @@ -24,7 +24,7 @@ use crate::entities::{ use crate::extensions::Extensions; use crate::parser::cst::{self, Ident}; use crate::parser::err::{ParseErrors, ToASTError, ToASTErrorKind}; -use crate::parser::{unescape, Node}; +use crate::parser::{unescape, Loc, Node}; use crate::{ast, FromNormalizedStr}; use either::Either; use itertools::Itertools; @@ -1122,19 +1122,20 @@ fn interpret_primary( .collect::, ParseErrors>>()?, ))), (path, id) => { - let (l, r) = match (path.first(), path.last()) { + let (l, r, src) = match (path.first(), path.last()) { (Some(l), Some(r)) => ( - l.loc.offset(), - r.loc.offset() + r.loc.len() + ident_to_str_len(id), + l.loc.start(), + r.loc.end() + ident_to_str_len(id), + Arc::clone(&l.loc.src), ), - (_, _) => (0, 0), + (_, _) => (0, 0, Arc::from("")), }; Err(ToASTError::new( ToASTErrorKind::InvalidExpression(cst::Name { path: path.to_vec(), - name: Node::with_source_loc(Some(id.clone()), l..r), + name: Node::with_source_loc(Some(id.clone()), node.loc.span(l..r)), }), - miette::SourceSpan::from(l..r), + Loc::new(l..r, src), ) .into()) } @@ -1225,15 +1226,15 @@ impl TryFrom<&Node>> for Expr { match attr.as_str() { "contains" => Either::Right(Expr::contains( left, - extract_single_argument(args, "contains()", access.loc)?, + extract_single_argument(args, "contains()", &access.loc)?, )), "containsAll" => Either::Right(Expr::contains_all( left, - extract_single_argument(args, "containsAll()", access.loc)?, + extract_single_argument(args, "containsAll()", &access.loc)?, )), "containsAny" => Either::Right(Expr::contains_any( left, - extract_single_argument(args, "containsAny()", access.loc)?, + extract_single_argument(args, "containsAny()", &access.loc)?, )), _ => { // have to add the "receiver" argument as @@ -1274,7 +1275,7 @@ impl TryFrom<&Node>> for Expr { pub fn extract_single_argument( args: impl ExactSizeIterator, fn_name: &'static str, - span: miette::SourceSpan, + loc: &Loc, ) -> Result { let mut iter = args.fuse().peekable(); let first = iter.next(); @@ -1282,11 +1283,11 @@ pub fn extract_single_argument( match (first, second) { (None, _) => Err(ToASTError::new( ToASTErrorKind::wrong_arity(fn_name, 1, 0), - span, + loc.clone(), )), (Some(_), Some(_)) => Err(ToASTError::new( ToASTErrorKind::wrong_arity(fn_name, 1, iter.len() + 1), - span, + loc.clone(), )), (Some(first), None) => Ok(first), } @@ -1323,17 +1324,18 @@ impl TryFrom<&Node>> for Expr { (&[], cst::Ident::Resource) => Ok(Expr::var(ast::Var::Resource)), (&[], cst::Ident::Context) => Ok(Expr::var(ast::Var::Context)), (path, id) => { - let (l, r) = match (path.first(), path.last()) { - (Some(l), Some(r)) => ( - l.loc.offset(), - r.loc.offset() + r.loc.len() + ident_to_str_len(id), - ), - (_, _) => (0, 0), + let loc = match (path.first(), path.last()) { + (Some(lnode), Some(rnode)) => { + let l = lnode.loc.start(); + let r = rnode.loc.end() + ident_to_str_len(id); + Loc::new(l..r, Arc::clone(&lnode.loc.src)) + } + (_, _) => Loc::new(0, Arc::from("")), }; Err(name .to_ast_err(ToASTErrorKind::InvalidExpression(cst::Name { path: path.to_vec(), - name: Node::with_source_loc(Some(id.clone()), l..r), + name: Node::with_source_loc(Some(id.clone()), loc), })) .into()) } @@ -1379,12 +1381,16 @@ mod test { #[test] fn test_invalid_expr_from_cst_name() { + let src = "some_long_str"; let path = vec![Node::with_source_loc( - Some(cst::Ident::Ident("some_long_str".into())), - 0..12, + Some(cst::Ident::Ident(src.into())), + Loc::new(0..12, Arc::from(src)), )]; - let name = Node::with_source_loc(Some(cst::Ident::Else), 13..16); - let cst_name = Node::with_source_loc(Some(cst::Name { path, name }), 0..16); + let name = Node::with_source_loc(Some(cst::Ident::Else), Loc::new(13..16, Arc::from(src))); + let cst_name = Node::with_source_loc( + Some(cst::Name { path, name }), + Loc::new(0..16, Arc::from(src)), + ); assert_matches!(Expr::try_from(&cst_name), Err(e) => { assert!(e.len() == 1); @@ -1392,7 +1398,7 @@ mod test { ParseError::ToAST(to_ast_error) => { assert_matches!(to_ast_error.kind(), ToASTErrorKind::InvalidExpression(e) => { println!("{e:?}"); - assert_eq!(e.name.loc.offset() + e.name.loc.len(), 16); + assert_eq!(e.name.loc.end(), 16); }); } ); diff --git a/cedar-policy-core/src/from_normalized_str.rs b/cedar-policy-core/src/from_normalized_str.rs index 87ddd40e3..6c23a7951 100644 --- a/cedar-policy-core/src/from_normalized_str.rs +++ b/cedar-policy-core/src/from_normalized_str.rs @@ -1,4 +1,5 @@ use crate::parser::err::{ParseError, ParseErrors, ToASTError, ToASTErrorKind}; +use crate::parser::Loc; use std::fmt::Display; use std::str::FromStr; @@ -39,7 +40,7 @@ pub trait FromNormalizedStr: FromStr + Display { src: s.to_string(), normalized_src, }, - miette::SourceSpan::from(diff_byte), + Loc::new(diff_byte, s.into()), )) .into()) } diff --git a/cedar-policy-core/src/parser.rs b/cedar-policy-core/src/parser.rs index 27bf5c57e..17b1c5dd3 100644 --- a/cedar-policy-core/src/parser.rs +++ b/cedar-policy-core/src/parser.rs @@ -24,6 +24,9 @@ mod cst_to_ast; pub mod err; /// implementations for formatting, like `Display` mod fmt; +/// Source location struct +mod loc; +pub use loc::Loc; /// Metadata wrapper for CST Nodes mod node; pub use node::Node; @@ -80,12 +83,7 @@ pub fn parse_policyset_and_also_return_policy_text( let texts = cst .with_generated_policyids() .expect("shouldn't be None since parse_policies() and to_policyset() didn't return Err") - .map(|(id, policy)| { - ( - id, - &text[policy.loc.offset()..(policy.loc.offset() + policy.loc.len())], - ) - }) + .map(|(id, policy)| (id, &text[policy.loc.start()..policy.loc.end()])) .collect::>(); Ok((texts, pset)) } else { diff --git a/cedar-policy-core/src/parser/cst_to_ast.rs b/cedar-policy-core/src/parser/cst_to_ast.rs index 012e21b23..638642472 100644 --- a/cedar-policy-core/src/parser/cst_to_ast.rs +++ b/cedar-policy-core/src/parser/cst_to_ast.rs @@ -35,10 +35,13 @@ // cases where there is a secondary conversion. This prevents any further // cloning. -use super::err::{ParseError, ParseErrors, Ref, RefCreationError, ToASTError, ToASTErrorKind}; +use super::cst; +use super::err::{ + self, ParseError, ParseErrors, Ref, RefCreationError, ToASTError, ToASTErrorKind, +}; +use super::loc::Loc; use super::node::Node; use super::unescape::{to_pattern, to_unescaped_string}; -use super::{cst, err}; use crate::ast::{ self, ActionConstraint, CallStyle, EntityReference, EntityType, EntityUID, ExprConstructionError, Integer, PatternElem, PolicySetError, PrincipalConstraint, @@ -186,7 +189,7 @@ impl Node> { ParseError::ToAST(err) => match err.kind() { ToASTErrorKind::SlotsInConditionClause { slot, .. } => Some(ToASTError::new( ToASTErrorKind::UnexpectedTemplate { slot: slot.clone() }, - err.source_span(), + err.source_loc().clone(), )), _ => None, }, @@ -278,7 +281,7 @@ impl Node> { action, resource, conds, - self.loc, + &self.loc, )) } } @@ -295,26 +298,26 @@ impl cst::Policy { ) { // Tracks where the last variable in the scope ended. We'll point to // this position to indicate where to fill in vars if we're missing one. - let mut end_of_last_var = self.effect.loc.offset() + self.effect.loc.len(); + let mut end_of_last_var = self.effect.loc.end(); let mut vars = self.variables.iter().peekable(); let principal = if let Some(head1) = vars.next() { - end_of_last_var = head1.loc.offset() + head1.loc.len(); + end_of_last_var = head1.loc.end(); head1.to_principal_constraint(errs) } else { errs.push(ToASTError::new( ToASTErrorKind::MissingScopeConstraint(ast::Var::Principal), - miette::SourceSpan::from(end_of_last_var), + self.effect.loc.span(end_of_last_var), )); None }; let action = if let Some(head2) = vars.next() { - end_of_last_var = head2.loc.offset() + head2.loc.len(); + end_of_last_var = head2.loc.end(); head2.to_action_constraint(errs) } else { errs.push(ToASTError::new( ToASTErrorKind::MissingScopeConstraint(ast::Var::Action), - miette::SourceSpan::from(end_of_last_var), + self.effect.loc.span(end_of_last_var), )); None }; @@ -323,7 +326,7 @@ impl cst::Policy { } else { errs.push(ToASTError::new( ToASTErrorKind::MissingScopeConstraint(ast::Var::Resource), - miette::SourceSpan::from(end_of_last_var), + self.effect.loc.span(end_of_last_var), )); None }; @@ -457,37 +460,37 @@ impl ast::Id { e: ast::Expr, mut args: Vec, errs: &mut ParseErrors, - span: miette::SourceSpan, + loc: &Loc, ) -> Option { match self.as_ref() { - "contains" => extract_single_argument(args.into_iter(), "contains", span) - .map(|arg| construct_method_contains(e, arg, span)) + "contains" => extract_single_argument(args.into_iter(), "contains", loc) + .map(|arg| construct_method_contains(e, arg, loc.clone())) .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)) + "containsAll" => extract_single_argument(args.into_iter(), "containsAll", loc) + .map(|arg| construct_method_contains_all(e, arg, loc.clone())) .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)) + "containsAny" => extract_single_argument(args.into_iter(), "containsAny", loc) + .map(|arg| construct_method_contains_any(e, arg, loc.clone())) .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(id.to_string(), args, span)) + Some(construct_ext_meth(id.to_string(), args, loc.clone())) } else { 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, + loc.clone(), )); } else { errs.push(ToASTError::new( ToASTErrorKind::InvalidMethodName(id.to_string()), - span, + loc.clone(), )); } None @@ -648,7 +651,7 @@ impl Node> { Some(ActionConstraint::Any) }?; - match action_constraint_contains_only_action_types(action_constraint, self.loc) { + match action_constraint_contains_only_action_types(action_constraint, &self.loc) { Ok(a) => Some(a), Err(mut id_errs) => { errs.append(&mut id_errs); @@ -661,7 +664,7 @@ impl Node> { /// Check that all of the EUIDs in an action constraint have the type `Action`, under an arbitrary namespace fn action_constraint_contains_only_action_types( a: ActionConstraint, - span: miette::SourceSpan, + loc: &Loc, ) -> Result { match a { ActionConstraint::Any => Ok(a), @@ -678,7 +681,7 @@ fn action_constraint_contains_only_action_types( .map(|euid| { ToASTError::new( ToASTErrorKind::InvalidActionType(euid.as_ref().clone()), - span, + loc.clone(), ) }) .collect()) @@ -690,7 +693,7 @@ fn action_constraint_contains_only_action_types( } else { Err(ParseErrors(vec![ToASTError::new( ToASTErrorKind::InvalidActionType(euid.as_ref().clone()), - span, + loc.clone(), ) .into()])) } @@ -742,7 +745,7 @@ impl Node> { if maybe_is_when { (e, true) } else { - (construct_expr_not(e, self.loc), false) + (construct_expr_not(e, self.loc.clone()), false) } }) } @@ -773,37 +776,25 @@ impl Node> { /// terms to a general Expr expression and then immediately unwrapping them. pub(crate) enum ExprOrSpecial<'a> { /// Any expression except a variable, name, or string literal - Expr { - expr: ast::Expr, - loc: miette::SourceSpan, - }, + Expr { expr: ast::Expr, loc: Loc }, /// Variables, which act as expressions or names - Var { - var: ast::Var, - loc: miette::SourceSpan, - }, + Var { var: ast::Var, loc: Loc }, /// Name that isn't an expr and couldn't be converted to var - Name { - name: ast::Name, - loc: miette::SourceSpan, - }, + Name { name: ast::Name, loc: Loc }, /// String literal, not yet unescaped /// Must be processed with to_unescaped_string or to_pattern before inclusion in the AST - StrLit { - lit: &'a SmolStr, - loc: miette::SourceSpan, - }, + StrLit { lit: &'a SmolStr, loc: Loc }, } impl ExprOrSpecial<'_> { fn to_ast_err(&self, kind: impl Into) -> ToASTError { ToASTError::new( kind.into(), - *match self { - ExprOrSpecial::Expr { loc, .. } => loc, - ExprOrSpecial::Var { loc, .. } => loc, - ExprOrSpecial::Name { loc, .. } => loc, - ExprOrSpecial::StrLit { loc, .. } => loc, + match self { + ExprOrSpecial::Expr { loc, .. } => loc.clone(), + ExprOrSpecial::Var { loc, .. } => loc.clone(), + ExprOrSpecial::Name { loc, .. } => loc.clone(), + ExprOrSpecial::StrLit { loc, .. } => loc.clone(), }, ) } @@ -825,7 +816,7 @@ impl ExprOrSpecial<'_> { errs.extend( escape_errs .into_iter() - .map(|e| ToASTError::new(ToASTErrorKind::Unescape(e), loc)), + .map(|e| ToASTError::new(ToASTErrorKind::Unescape(e), loc.clone())), ); None } @@ -844,7 +835,7 @@ impl ExprOrSpecial<'_> { errs.extend( escape_errs .into_iter() - .map(|e| ToASTError::new(ToASTErrorKind::Unescape(e), loc)), + .map(|e| ToASTError::new(ToASTErrorKind::Unescape(e), loc.clone())), ); None } @@ -986,8 +977,8 @@ impl Node> { match (maybe_guard, maybe_then, maybe_else) { (Some(i), Some(t), Some(e)) => Some(ExprOrSpecial::Expr { - expr: construct_expr_if(i, t, e, self.loc), - loc: self.loc, + expr: construct_expr_if(i, t, e, self.loc.clone()), + loc: self.loc.clone(), }), _ => None, } @@ -1001,17 +992,9 @@ impl Node> { /// or runtime data. trait RefKind: Sized { fn err_str() -> &'static str; - fn create_single_ref( - e: EntityUID, - errs: &mut ParseErrors, - span: miette::SourceSpan, - ) -> Option; - fn create_multiple_refs( - es: Vec, - errs: &mut ParseErrors, - span: miette::SourceSpan, - ) -> Option; - fn create_slot(errs: &mut ParseErrors, span: miette::SourceSpan) -> Option; + fn create_single_ref(e: EntityUID, errs: &mut ParseErrors, loc: &Loc) -> Option; + fn create_multiple_refs(es: Vec, errs: &mut ParseErrors, loc: &Loc) -> Option; + fn create_slot(errs: &mut ParseErrors, loc: &Loc) -> Option; } struct SingleEntity(pub EntityUID); @@ -1021,30 +1004,26 @@ impl RefKind for SingleEntity { "an entity uid" } - fn create_single_ref( - e: EntityUID, - _errs: &mut ParseErrors, - _span: miette::SourceSpan, - ) -> Option { + fn create_single_ref(e: EntityUID, _errs: &mut ParseErrors, _loc: &Loc) -> Option { Some(SingleEntity(e)) } fn create_multiple_refs( _es: Vec, errs: &mut ParseErrors, - span: miette::SourceSpan, + loc: &Loc, ) -> Option { errs.push(ToASTError::new( RefCreationError::one_expected(Ref::Single, Ref::Set).into(), - span, + loc.clone(), )); None } - fn create_slot(errs: &mut ParseErrors, span: miette::SourceSpan) -> Option { + fn create_slot(errs: &mut ParseErrors, loc: &Loc) -> Option { errs.push(ToASTError::new( RefCreationError::one_expected(Ref::Single, Ref::Template).into(), - span, + loc.clone(), )); None } @@ -1055,26 +1034,22 @@ impl RefKind for EntityReference { "an entity uid or matching template slot" } - fn create_slot(_: &mut ParseErrors, _span: miette::SourceSpan) -> Option { + fn create_slot(_: &mut ParseErrors, _loc: &Loc) -> Option { Some(EntityReference::Slot) } - fn create_single_ref( - e: EntityUID, - _errs: &mut ParseErrors, - _span: miette::SourceSpan, - ) -> Option { + fn create_single_ref(e: EntityUID, _errs: &mut ParseErrors, _loc: &Loc) -> Option { Some(EntityReference::euid(e)) } fn create_multiple_refs( _es: Vec, errs: &mut ParseErrors, - span: miette::SourceSpan, + loc: &Loc, ) -> Option { errs.push(ToASTError::new( RefCreationError::two_expected(Ref::Single, Ref::Template, Ref::Set).into(), - span, + loc.clone(), )); None } @@ -1092,26 +1067,22 @@ impl RefKind for OneOrMultipleRefs { "an entity uid or set of entity uids" } - fn create_slot(errs: &mut ParseErrors, span: miette::SourceSpan) -> Option { + fn create_slot(errs: &mut ParseErrors, loc: &Loc) -> Option { errs.push(ToASTError::new( RefCreationError::two_expected(Ref::Single, Ref::Set, Ref::Template).into(), - span, + loc.clone(), )); None } - fn create_single_ref( - e: EntityUID, - _errs: &mut ParseErrors, - _span: miette::SourceSpan, - ) -> Option { + fn create_single_ref(e: EntityUID, _errs: &mut ParseErrors, _loc: &Loc) -> Option { Some(OneOrMultipleRefs::Single(e)) } fn create_multiple_refs( es: Vec, _errs: &mut ParseErrors, - _span: miette::SourceSpan, + _loc: &Loc, ) -> Option { Some(OneOrMultipleRefs::Multiple(es)) } @@ -1135,8 +1106,8 @@ impl Node> { (f, None, _, 0) => f, (Some(f), Some(s), r, e) if 1 + r == e => { f.into_expr(errs).map(|e| ExprOrSpecial::Expr { - expr: construct_expr_or(e, s, rest, self.loc), - loc: self.loc, + expr: construct_expr_or(e, s, rest, &self.loc), + loc: self.loc.clone(), }) } _ => None, @@ -1203,8 +1174,8 @@ impl Node> { (f, None, _, 0) => f, (Some(f), Some(s), r, e) if 1 + r == e => { f.into_expr(errs).map(|e| ExprOrSpecial::Expr { - expr: construct_expr_and(e, s, rest, self.loc), - loc: self.loc, + expr: construct_expr_and(e, s, rest, &self.loc), + loc: self.loc.clone(), }) } _ => None, @@ -1287,8 +1258,8 @@ impl Node> { (_, None, 1) => None, (f, None, 0) => f, (Some(f), Some((op, s)), _) => f.into_expr(errs).map(|e| ExprOrSpecial::Expr { - expr: construct_expr_rel(e, *op, s, self.loc), - loc: self.loc, + expr: construct_expr_rel(e, *op, s, self.loc.clone()), + loc: self.loc.clone(), }), _ => None, } @@ -1299,8 +1270,8 @@ impl Node> { field.to_expr_or_special(errs)?.into_valid_attr(errs), ) { (Some(t), Some(s)) => Some(ExprOrSpecial::Expr { - expr: construct_expr_has(t, s, self.loc), - loc: self.loc, + expr: construct_expr_has(t, s, self.loc.clone()), + loc: self.loc.clone(), }), _ => None, } @@ -1311,8 +1282,8 @@ impl Node> { pattern.to_expr_or_special(errs)?.into_pattern(errs), ) { (Some(t), Some(s)) => Some(ExprOrSpecial::Expr { - expr: construct_expr_like(t, s, self.loc), - loc: self.loc, + expr: construct_expr_like(t, s, self.loc.clone()), + loc: self.loc.clone(), }), _ => None, } @@ -1331,17 +1302,22 @@ impl Node> { .to_expr(errs) .map(|in_entity| ExprOrSpecial::Expr { expr: construct_expr_and( - construct_expr_is(t.clone(), n, self.loc), - construct_expr_rel(t, cst::RelOp::In, in_entity, self.loc), + construct_expr_is(t.clone(), n, self.loc.clone()), + construct_expr_rel( + t, + cst::RelOp::In, + in_entity, + self.loc.clone(), + ), std::iter::empty(), - self.loc, + &self.loc, ), - loc: self.loc, + loc: self.loc.clone(), }) } None => Some(ExprOrSpecial::Expr { - expr: construct_expr_is(t, n, self.loc), - loc: self.loc, + expr: construct_expr_is(t, n, self.loc.clone()), + loc: self.loc.clone(), }), }, _ => None, @@ -1384,8 +1360,8 @@ impl Node> { .collect(); if !more.is_empty() { Some(ExprOrSpecial::Expr { - expr: construct_expr_add(maybe_first?.into_expr(errs)?, more, self.loc), - loc: self.loc, + expr: construct_expr_add(maybe_first?.into_expr(errs)?, more, &self.loc), + loc: self.loc.clone(), }) } else { maybe_first @@ -1475,11 +1451,11 @@ impl Node> { #[allow(clippy::indexing_slicing)] Some(ExprOrSpecial::Expr { expr: construct_expr_mul( - construct_expr_num(constantints[0], self.loc), + construct_expr_num(constantints[0], self.loc.clone()), constantints[1..].iter().copied(), - self.loc, + &self.loc, ), - loc: self.loc, + loc: self.loc.clone(), }) } else { // PANIC SAFETY Checked above that `nonconstantints` has at least one element @@ -1489,8 +1465,8 @@ impl Node> { .next() .expect("already checked that it's not empty"); Some(ExprOrSpecial::Expr { - expr: construct_expr_mul(nonconstantint, constantints, self.loc), - loc: self.loc, + expr: construct_expr_mul(nonconstantint, constantints, &self.loc), + loc: self.loc.clone(), }) } } else { @@ -1539,14 +1515,17 @@ impl Node> { let item = maybe_item().and_then(|i| i.into_expr(errs)); if n % 2 == 0 { item.map(|i| ExprOrSpecial::Expr { - expr: construct_expr_not(construct_expr_not(i, self.loc), self.loc), - loc: self.loc, + expr: construct_expr_not( + construct_expr_not(i, self.loc.clone()), + self.loc.clone(), + ), + loc: self.loc.clone(), }) } else { // safe to collapse to ! item.map(|i| ExprOrSpecial::Expr { - expr: construct_expr_not(i, self.loc), - loc: self.loc, + expr: construct_expr_not(i, self.loc.clone()), + loc: self.loc.clone(), }) } } @@ -1558,11 +1537,12 @@ impl Node> { // decreases by one. let (last, rc) = if let Some(cst::Literal::Num(n)) = unary.item.to_lit() { match n.cmp(&(i64::MAX as u64 + 1)) { - Ordering::Equal => { - (Some(construct_expr_num(i64::MIN, unary.item.loc)), c - 1) - } + Ordering::Equal => ( + Some(construct_expr_num(i64::MIN, unary.item.loc.clone())), + c - 1, + ), Ordering::Less => ( - Some(construct_expr_num(-(*n as i64), unary.item.loc)), + Some(construct_expr_num(-(*n as i64), unary.item.loc.clone())), c - 1, ), Ordering::Greater => { @@ -1577,10 +1557,12 @@ impl Node> { }; // Fold the expression into a series of negation operations. (0..rc) - .fold(last, |r, _| r.map(|e| (construct_expr_neg(e, self.loc)))) + .fold(last, |r, _| { + r.map(|e| (construct_expr_neg(e, self.loc.clone()))) + }) .map(|expr| ExprOrSpecial::Expr { expr, - loc: self.loc, + loc: self.loc.clone(), }) } Some(cst::NegOp::OverBang) => { @@ -1685,9 +1667,9 @@ impl Node> { name, ast::Name::unqualified_name(ast::Id::new_unchecked("")), ); - head = nn.into_func(args, errs, self.loc).map(|expr| Expr { + head = nn.into_func(args, errs, self.loc.clone()).map(|expr| Expr { expr, - loc: self.loc, + loc: self.loc.clone(), }); tail = rest; } @@ -1721,10 +1703,15 @@ impl Node> { // move the id out of the slice as well, to avoid cloning the internal string let id = mem::replace(i, ast::Id::new_unchecked("")); head = id - .to_meth(construct_expr_var(var, *var_loc), args, errs, self.loc) + .to_meth( + construct_expr_var(var, var_loc.clone()), + args, + errs, + &self.loc, + ) .map(|expr| Expr { expr, - loc: self.loc, + loc: self.loc.clone(), }); tail = rest; } @@ -1735,9 +1722,9 @@ impl Node> { let expr = mem::replace(expr, ast::Expr::val(false)); // move the id out of the slice as well, to avoid cloning the internal string let id = mem::replace(i, ast::Id::new_unchecked("")); - head = id.to_meth(expr, args, errs, self.loc).map(|expr| Expr { + head = id.to_meth(expr, args, errs, &self.loc).map(|expr| Expr { expr, - loc: self.loc, + loc: self.loc.clone(), }); tail = rest; } @@ -1749,7 +1736,7 @@ impl Node> { let args = std::mem::take(a); let id = mem::replace(i, ast::Id::new_unchecked("")); let maybe_expr = match to_unescaped_string(lit) { - Ok(s) => Some(construct_expr_string(s, *lit_loc)), + Ok(s) => Some(construct_expr_string(s, lit_loc.clone())), Err(escape_errs) => { errs.extend( escape_errs @@ -1760,9 +1747,9 @@ impl Node> { } }; head = maybe_expr.and_then(|e| { - id.to_meth(e, args, errs, self.loc).map(|expr| Expr { + id.to_meth(e, args, errs, &self.loc).map(|expr| Expr { expr, - loc: self.loc, + loc: self.loc.clone(), }) }); tail = rest; @@ -1793,11 +1780,11 @@ impl Node> { let id = mem::replace(i, ast::Id::new_unchecked("")); head = Some(Expr { expr: construct_expr_attr( - construct_expr_var(var, *var_loc), + construct_expr_var(var, var_loc.clone()), id.to_smolstr(), - self.loc, + self.loc.clone(), ), - loc: self.loc, + loc: self.loc.clone(), }); tail = rest; } @@ -1806,8 +1793,8 @@ impl Node> { let expr = mem::replace(expr, ast::Expr::val(false)); let id = mem::replace(i, ast::Id::new_unchecked("")); head = Some(Expr { - expr: construct_expr_attr(expr, id.to_smolstr(), self.loc), - loc: self.loc, + expr: construct_expr_attr(expr, id.to_smolstr(), self.loc.clone()), + loc: self.loc.clone(), }); tail = rest; } @@ -1815,7 +1802,7 @@ impl Node> { (Some(StrLit { lit, loc: lit_loc }), [Some(Field(i)), rest @ ..]) => { let id = mem::replace(i, ast::Id::new_unchecked("")); let maybe_expr = match to_unescaped_string(lit) { - Ok(s) => Some(construct_expr_string(s, *lit_loc)), + Ok(s) => Some(construct_expr_string(s, lit_loc.clone())), Err(escape_errs) => { errs.extend( escape_errs @@ -1826,8 +1813,8 @@ impl Node> { } }; head = maybe_expr.map(|e| Expr { - expr: construct_expr_attr(e, id.to_smolstr(), self.loc), - loc: self.loc, + expr: construct_expr_attr(e, id.to_smolstr(), self.loc.clone()), + loc: self.loc.clone(), }); tail = rest; } @@ -1836,8 +1823,12 @@ impl Node> { let var = mem::replace(var, ast::Var::Principal); let s = mem::take(i); head = Some(Expr { - expr: construct_expr_attr(construct_expr_var(var, *var_loc), s, self.loc), - loc: self.loc, + expr: construct_expr_attr( + construct_expr_var(var, var_loc.clone()), + s, + self.loc.clone(), + ), + loc: self.loc.clone(), }); tail = rest; } @@ -1846,8 +1837,8 @@ impl Node> { let expr = mem::replace(expr, ast::Expr::val(false)); let s = mem::take(i); head = Some(Expr { - expr: construct_expr_attr(expr, s, self.loc), - loc: self.loc, + expr: construct_expr_attr(expr, s, self.loc.clone()), + loc: self.loc.clone(), }); tail = rest; } @@ -1855,7 +1846,7 @@ impl Node> { (Some(StrLit { lit, loc: lit_loc }), [Some(Index(i)), rest @ ..]) => { let id = mem::take(i); let maybe_expr = match to_unescaped_string(lit) { - Ok(s) => Some(construct_expr_string(s, *lit_loc)), + Ok(s) => Some(construct_expr_string(s, lit_loc.clone())), Err(escape_errs) => { errs.extend( escape_errs @@ -1866,8 +1857,8 @@ impl Node> { } }; head = maybe_expr.map(|e| Expr { - expr: construct_expr_attr(e, id, self.loc), - loc: self.loc, + expr: construct_expr_attr(e, id, self.loc.clone()), + loc: self.loc.clone(), }); tail = rest; } @@ -1918,7 +1909,7 @@ impl Node> { // 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_ref = T::create_slot(errs, &self.loc)?; let slot = s.as_inner()?; if slot.matches(var) { Some(slot_ref) @@ -1943,7 +1934,7 @@ impl Node> { ))); None } - cst::Primary::Ref(x) => T::create_single_ref(x.to_ref(errs)?, errs, self.loc), + cst::Primary::Ref(x) => T::create_single_ref(x.to_ref(errs)?, errs, &self.loc), cst::Primary::Name(name) => { let found = match name.as_inner() { Some(name) => format!("name `{name}`"), @@ -1960,7 +1951,7 @@ impl Node> { cst::Primary::EList(lst) => { let v: Option> = lst.iter().map(|expr| expr.to_ref(var, errs)).collect(); - T::create_multiple_refs(v?, errs, self.loc) + T::create_multiple_refs(v?, errs, &self.loc) } cst::Primary::RInits(_) => { errs.push(self.to_ast_err(ToASTErrorKind::wrong_node( @@ -1984,36 +1975,41 @@ impl Node> { match prim { cst::Primary::Literal(lit) => lit.to_expr_or_special(errs), - cst::Primary::Ref(r) => r - .to_expr(errs) - .map(|expr| ExprOrSpecial::Expr { expr, loc: r.loc }), - cst::Primary::Slot(s) => s - .clone() - .into_expr(errs) - .map(|expr| ExprOrSpecial::Expr { expr, loc: s.loc }), + cst::Primary::Ref(r) => r.to_expr(errs).map(|expr| ExprOrSpecial::Expr { + expr, + loc: r.loc.clone(), + }), + cst::Primary::Slot(s) => s.clone().into_expr(errs).map(|expr| ExprOrSpecial::Expr { + expr, + loc: s.loc.clone(), + }), #[allow(clippy::manual_map)] cst::Primary::Name(n) => { // if `n` isn't a var we don't want errors, we'll get them later if let Some(var) = n.to_var(&mut ParseErrors::new()) { - Some(ExprOrSpecial::Var { var, loc: self.loc }) + Some(ExprOrSpecial::Var { + var, + loc: self.loc.clone(), + }) } else if let Some(name) = n.to_name(errs) { Some(ExprOrSpecial::Name { name, - loc: self.loc, + loc: self.loc.clone(), }) } else { None } } - cst::Primary::Expr(e) => e - .to_expr(errs) - .map(|expr| ExprOrSpecial::Expr { expr, loc: e.loc }), + cst::Primary::Expr(e) => e.to_expr(errs).map(|expr| ExprOrSpecial::Expr { + expr, + loc: e.loc.clone(), + }), cst::Primary::EList(es) => { let list: Vec<_> = es.iter().filter_map(|e| e.to_expr(errs)).collect(); if list.len() == es.len() { Some(ExprOrSpecial::Expr { - expr: construct_expr_set(list, self.loc), - loc: self.loc, + expr: construct_expr_set(list, self.loc.clone()), + loc: self.loc.clone(), }) } else { None @@ -2022,10 +2018,10 @@ impl Node> { cst::Primary::RInits(is) => { let rec: Vec<_> = is.iter().filter_map(|i| i.to_init(errs)).collect(); if rec.len() == is.len() { - match construct_expr_record(rec, self.loc) { + match construct_expr_record(rec, self.loc.clone()) { Ok(expr) => Some(ExprOrSpecial::Expr { expr, - loc: self.loc, + loc: self.loc.clone(), }), Err(e) => { errs.push(e); @@ -2059,7 +2055,7 @@ impl Node> { match self.as_inner()?.try_into() { Ok(slot_id) => Some( ast::ExprBuilder::new() - .with_source_span(self.loc) + .with_source_loc(self.loc) .slot(slot_id), ), Err(e) => { @@ -2099,7 +2095,7 @@ impl Node> { errs.push(self.to_ast_err(ToASTErrorKind::TypeConstraints)); None } - None => Some(construct_expr_bool(true, self.loc)), + None => Some(construct_expr_bool(true, self.loc.clone())), } } @@ -2158,11 +2154,11 @@ impl Node> { impl ast::Name { /// Convert the `Name` into a `String` attribute, which fails if it had any namespaces - fn into_valid_attr(self, errs: &mut ParseErrors, span: miette::SourceSpan) -> Option { + fn into_valid_attr(self, errs: &mut ParseErrors, loc: Loc) -> Option { if !self.path.is_empty() { errs.push(ToASTError::new( ToASTErrorKind::PathAsAttribute(self.to_string()), - span, + loc, )); None } else { @@ -2174,7 +2170,7 @@ impl ast::Name { self, args: Vec, errs: &mut ParseErrors, - span: miette::SourceSpan, + loc: Loc, ) -> Option { // error on standard methods if self.path.is_empty() { @@ -2184,15 +2180,15 @@ impl ast::Name { { errs.push(ToASTError::new( ToASTErrorKind::FunctionCallOnMethod(self.id), - span, + loc, )); return None; } } if EXTENSION_STYLES.functions.contains(&self) { - Some(construct_ext_func(self, args, span)) + Some(construct_ext_func(self, args, loc)) } else { - errs.push(ToASTError::new(ToASTErrorKind::NotAFunction(self), span)); + errs.push(ToASTError::new(ToASTErrorKind::NotAFunction(self), loc)); None } } @@ -2238,7 +2234,7 @@ impl Node> { } fn to_expr(&self, errs: &mut ParseErrors) -> Option { self.to_ref(errs) - .map(|euid| construct_expr_ref(euid, self.loc)) + .map(|euid| construct_expr_ref(euid, self.loc.clone())) } } @@ -2251,17 +2247,17 @@ impl Node> { match lit { cst::Literal::True => Some(ExprOrSpecial::Expr { - expr: construct_expr_bool(true, self.loc), - loc: self.loc, + expr: construct_expr_bool(true, self.loc.clone()), + loc: self.loc.clone(), }), cst::Literal::False => Some(ExprOrSpecial::Expr { - expr: construct_expr_bool(false, self.loc), - loc: self.loc, + expr: construct_expr_bool(false, self.loc.clone()), + loc: self.loc.clone(), }), cst::Literal::Num(n) => match Integer::try_from(*n) { Ok(i) => Some(ExprOrSpecial::Expr { - expr: construct_expr_num(i, self.loc), - loc: self.loc, + expr: construct_expr_num(i, self.loc.clone()), + loc: self.loc.clone(), }), Err(_) => { errs.push(self.to_ast_err(ToASTErrorKind::IntegerLiteralTooLarge(*n))); @@ -2270,7 +2266,10 @@ impl Node> { }, cst::Literal::Str(s) => { let maybe_str = s.as_valid_string(errs); - maybe_str.map(|lit| ExprOrSpecial::StrLit { lit, loc: self.loc }) + maybe_str.map(|lit| ExprOrSpecial::StrLit { + lit, + loc: self.loc.clone(), + }) } } } @@ -2304,7 +2303,7 @@ fn construct_template_policy( action: ast::ActionConstraint, resource: ast::ResourceConstraint, conds: Vec, - span: miette::SourceSpan, + loc: &Loc, ) -> ast::Template { let construct_template = |non_head_constraint| { ast::Template::new( @@ -2322,12 +2321,12 @@ fn construct_template_policy( // a left fold of conditions // e.g., [c1, c2, c3,] --> ((c1 && c2) && c3) construct_template(match conds_iter.next() { - Some(e) => construct_expr_and(first_expr, e, conds_iter, span), + Some(e) => construct_expr_and(first_expr, e, conds_iter, loc), None => first_expr, }) } else { // use `true` to mark the absence of non-head constraints - construct_template(construct_expr_bool(true, span)) + construct_template(construct_expr_bool(true, loc.clone())) } } fn construct_id(s: String) -> ast::Id { @@ -2351,64 +2350,62 @@ fn construct_refr(p: ast::Name, n: SmolStr) -> ast::EntityUID { let eid = ast::Eid::new(n); ast::EntityUID::from_components(p, eid) } -fn construct_expr_ref(r: ast::EntityUID, span: miette::SourceSpan) -> ast::Expr { - ast::ExprBuilder::new().with_source_span(span).val(r) +fn construct_expr_ref(r: ast::EntityUID, loc: Loc) -> ast::Expr { + ast::ExprBuilder::new().with_source_loc(loc).val(r) } -fn construct_expr_num(n: Integer, span: miette::SourceSpan) -> ast::Expr { - ast::ExprBuilder::new().with_source_span(span).val(n) +fn construct_expr_num(n: Integer, loc: Loc) -> ast::Expr { + ast::ExprBuilder::new().with_source_loc(loc).val(n) } -fn construct_expr_string(s: SmolStr, span: miette::SourceSpan) -> ast::Expr { - ast::ExprBuilder::new().with_source_span(span).val(s) +fn construct_expr_string(s: SmolStr, loc: Loc) -> ast::Expr { + ast::ExprBuilder::new().with_source_loc(loc).val(s) } -fn construct_expr_bool(b: bool, span: miette::SourceSpan) -> ast::Expr { - ast::ExprBuilder::new().with_source_span(span).val(b) +fn construct_expr_bool(b: bool, loc: Loc) -> ast::Expr { + ast::ExprBuilder::new().with_source_loc(loc).val(b) } -fn construct_expr_neg(e: ast::Expr, span: miette::SourceSpan) -> ast::Expr { - ast::ExprBuilder::new().with_source_span(span).neg(e) +fn construct_expr_neg(e: ast::Expr, loc: Loc) -> ast::Expr { + ast::ExprBuilder::new().with_source_loc(loc).neg(e) } -fn construct_expr_not(e: ast::Expr, span: miette::SourceSpan) -> ast::Expr { - ast::ExprBuilder::new().with_source_span(span).not(e) +fn construct_expr_not(e: ast::Expr, loc: Loc) -> ast::Expr { + ast::ExprBuilder::new().with_source_loc(loc).not(e) } -fn construct_expr_var(v: ast::Var, span: miette::SourceSpan) -> ast::Expr { - ast::ExprBuilder::new().with_source_span(span).var(v) +fn construct_expr_var(v: ast::Var, loc: Loc) -> ast::Expr { + ast::ExprBuilder::new().with_source_loc(loc).var(v) } -fn construct_expr_if( - i: ast::Expr, - t: ast::Expr, - e: ast::Expr, - span: miette::SourceSpan, -) -> ast::Expr { - ast::ExprBuilder::new().with_source_span(span).ite(i, t, e) +fn construct_expr_if(i: ast::Expr, t: ast::Expr, e: ast::Expr, loc: Loc) -> ast::Expr { + ast::ExprBuilder::new().with_source_loc(loc).ite(i, t, e) } fn construct_expr_or( f: ast::Expr, s: ast::Expr, chained: impl IntoIterator, - span: miette::SourceSpan, + loc: &Loc, ) -> ast::Expr { - let first = ast::ExprBuilder::new().with_source_span(span).or(f, s); + let first = ast::ExprBuilder::new() + .with_source_loc(loc.clone()) + .or(f, s); chained.into_iter().fold(first, |a, n| { - ast::ExprBuilder::new().with_source_span(span).or(a, n) + ast::ExprBuilder::new() + .with_source_loc(loc.clone()) + .or(a, n) }) } fn construct_expr_and( f: ast::Expr, s: ast::Expr, chained: impl IntoIterator, - span: miette::SourceSpan, + loc: &Loc, ) -> ast::Expr { - let first = ast::ExprBuilder::new().with_source_span(span).and(f, s); + let first = ast::ExprBuilder::new() + .with_source_loc(loc.clone()) + .and(f, s); chained.into_iter().fold(first, |a, n| { - ast::ExprBuilder::new().with_source_span(span).and(a, n) + ast::ExprBuilder::new() + .with_source_loc(loc.clone()) + .and(a, n) }) } -fn construct_expr_rel( - f: ast::Expr, - rel: cst::RelOp, - s: ast::Expr, - span: miette::SourceSpan, -) -> ast::Expr { - let builder = ast::ExprBuilder::new().with_source_span(span); +fn construct_expr_rel(f: ast::Expr, rel: cst::RelOp, s: ast::Expr, loc: Loc) -> ast::Expr { + let builder = ast::ExprBuilder::new().with_source_loc(loc); match rel { cst::RelOp::Less => builder.less(f, s), cst::RelOp::LessEq => builder.lesseq(f, s), @@ -2423,11 +2420,11 @@ fn construct_expr_rel( fn construct_expr_add( f: ast::Expr, chained: impl IntoIterator, - span: miette::SourceSpan, + loc: &Loc, ) -> ast::Expr { let mut expr = f; for (op, next_expr) in chained { - let builder = ast::ExprBuilder::new().with_source_span(span); + let builder = ast::ExprBuilder::new().with_source_loc(loc.clone()); expr = match op { cst::AddOp::Plus => builder.add(expr, next_expr), cst::AddOp::Minus => builder.sub(expr, next_expr), @@ -2439,91 +2436,75 @@ fn construct_expr_add( fn construct_expr_mul( f: ast::Expr, chained: impl IntoIterator, - span: miette::SourceSpan, + loc: &Loc, ) -> ast::Expr { let mut expr = f; for next_expr in chained { expr = ast::ExprBuilder::new() - .with_source_span(span) + .with_source_loc(loc.clone()) .mul(expr, next_expr as Integer) } expr } -fn construct_expr_has(t: ast::Expr, s: SmolStr, span: miette::SourceSpan) -> ast::Expr { - ast::ExprBuilder::new() - .with_source_span(span) - .has_attr(t, s) +fn construct_expr_has(t: ast::Expr, s: SmolStr, loc: Loc) -> ast::Expr { + ast::ExprBuilder::new().with_source_loc(loc).has_attr(t, s) } -fn construct_expr_attr(e: ast::Expr, s: SmolStr, span: miette::SourceSpan) -> ast::Expr { - ast::ExprBuilder::new() - .with_source_span(span) - .get_attr(e, s) +fn construct_expr_attr(e: ast::Expr, s: SmolStr, loc: Loc) -> ast::Expr { + ast::ExprBuilder::new().with_source_loc(loc).get_attr(e, s) } -fn construct_expr_like(e: ast::Expr, s: Vec, span: miette::SourceSpan) -> ast::Expr { - ast::ExprBuilder::new().with_source_span(span).like(e, s) +fn construct_expr_like(e: ast::Expr, s: Vec, loc: Loc) -> ast::Expr { + ast::ExprBuilder::new().with_source_loc(loc).like(e, s) } -fn construct_expr_is(e: ast::Expr, n: ast::Name, span: miette::SourceSpan) -> ast::Expr { +fn construct_expr_is(e: ast::Expr, n: ast::Name, loc: Loc) -> ast::Expr { ast::ExprBuilder::new() - .with_source_span(span) + .with_source_loc(loc) .is_entity_type(e, n) } -fn construct_ext_func( - name: ast::Name, - args: Vec, - span: miette::SourceSpan, -) -> ast::Expr { +fn construct_ext_func(name: ast::Name, args: Vec, loc: Loc) -> ast::Expr { // INVARIANT (MethodStyleArgs): CallStyle is not MethodStyle, so any args vector is fine ast::ExprBuilder::new() - .with_source_span(span) + .with_source_loc(loc) .call_extension_fn(name, args) } -fn construct_method_contains(e0: ast::Expr, e1: ast::Expr, span: miette::SourceSpan) -> ast::Expr { +fn construct_method_contains(e0: ast::Expr, e1: ast::Expr, loc: Loc) -> ast::Expr { ast::ExprBuilder::new() - .with_source_span(span) + .with_source_loc(loc) .contains(e0, e1) } -fn construct_method_contains_all( - e0: ast::Expr, - e1: ast::Expr, - span: miette::SourceSpan, -) -> ast::Expr { +fn construct_method_contains_all(e0: ast::Expr, e1: ast::Expr, loc: Loc) -> ast::Expr { ast::ExprBuilder::new() - .with_source_span(span) + .with_source_loc(loc) .contains_all(e0, e1) } -fn construct_method_contains_any( - e0: ast::Expr, - e1: ast::Expr, - span: miette::SourceSpan, -) -> ast::Expr { +fn construct_method_contains_any(e0: ast::Expr, e1: ast::Expr, loc: Loc) -> ast::Expr { ast::ExprBuilder::new() - .with_source_span(span) + .with_source_loc(loc) .contains_any(e0, e1) } // INVARIANT (MethodStyleArgs), args must be non-empty -fn construct_ext_meth(n: String, args: Vec, span: miette::SourceSpan) -> ast::Expr { +fn construct_ext_meth(n: String, args: Vec, loc: Loc) -> ast::Expr { let id = ast::Id::new_unchecked(n); let name = ast::Name::unqualified_name(id); // INVARIANT (MethodStyleArgs), args must be non-empty ast::ExprBuilder::new() - .with_source_span(span) + .with_source_loc(loc) .call_extension_fn(name, args) } -fn construct_expr_set(s: Vec, span: miette::SourceSpan) -> ast::Expr { - ast::ExprBuilder::new().with_source_span(span).set(s) +fn construct_expr_set(s: Vec, loc: Loc) -> ast::Expr { + ast::ExprBuilder::new().with_source_loc(loc).set(s) } fn construct_expr_record( kvs: Vec<(SmolStr, ast::Expr)>, - span: miette::SourceSpan, + loc: Loc, ) -> Result { ast::ExprBuilder::new() - .with_source_span(span) + .with_source_loc(loc.clone()) .record(kvs) .map_err(|e| match e { ExprConstructionError::DuplicateKeyInRecordLiteral { key } => { - ToASTError::new(ToASTErrorKind::DuplicateKeyInRecordLiteral { key }, span) + ToASTError::new(ToASTErrorKind::DuplicateKeyInRecordLiteral { key }, loc) } }) } diff --git a/cedar-policy-core/src/parser/err.rs b/cedar-policy-core/src/parser/err.rs index 7dfb05238..8a860c158 100644 --- a/cedar-policy-core/src/parser/err.rs +++ b/cedar-policy-core/src/parser/err.rs @@ -27,10 +27,10 @@ use smol_str::SmolStr; use thiserror::Error; use crate::ast::{self, InputInteger, PolicyID, RestrictedExprError, Var}; -use crate::parser::unescape::UnescapeError; - use crate::parser::fmt::join_with_conjunction; +use crate::parser::loc::Loc; use crate::parser::node::Node; +use crate::parser::unescape::UnescapeError; use super::cst; @@ -69,11 +69,15 @@ impl ParseError { pub fn primary_source_span(&self) -> Option { match self { ParseError::ToCST(to_cst_err) => Some(to_cst_err.primary_source_span()), - ParseError::ToAST(to_ast_err) => Some(to_ast_err.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 { .. } => None, + RestrictedExprError::InvalidRestrictedExpression { expr, .. } => { + expr.source_loc().map(|loc| loc.span) + } }, - ParseError::ParseLiteral(_) => None, + ParseError::ParseLiteral(parse_lit_err) => parse_lit_err + .labels() + .and_then(|mut it| it.next().map(|lspan| lspan.inner().clone())), } } } @@ -92,16 +96,14 @@ pub enum ParseLiteralError { #[error("{kind}")] pub struct ToASTError { kind: ToASTErrorKind, - source_span: miette::SourceSpan, + loc: Loc, } -// Aside from `labels` which is constructed based on the `source_span` in this -// struct, everything is forwarded directly from `kind`. +// Construct `labels` and `source_code` based on the `loc` in this struct; +// and everything else forwarded directly to `kind`. impl Diagnostic for ToASTError { fn labels(&self) -> Option + '_>> { - Some(Box::new(iter::once(LabeledSpan::underline( - self.source_span, - )))) + Some(Box::new(iter::once(LabeledSpan::underline(self.loc.span)))) } fn code<'a>(&'a self) -> Option> { @@ -121,7 +123,7 @@ impl Diagnostic for ToASTError { } fn source_code(&self) -> Option<&dyn miette::SourceCode> { - self.kind.source_code() + Some(&self.loc.src as &dyn miette::SourceCode) } fn diagnostic_source(&self) -> Option<&dyn Diagnostic> { @@ -131,8 +133,8 @@ impl Diagnostic for ToASTError { impl ToASTError { /// Construct a new `ToASTError`. - pub fn new(kind: ToASTErrorKind, source_span: miette::SourceSpan) -> Self { - Self { kind, source_span } + pub fn new(kind: ToASTErrorKind, loc: Loc) -> Self { + Self { kind, loc } } /// Get the error kind. @@ -140,8 +142,8 @@ impl ToASTError { &self.kind } - pub(crate) fn source_span(&self) -> miette::SourceSpan { - self.source_span + pub(crate) fn source_loc(&self) -> &Loc { + &self.loc } } @@ -502,7 +504,7 @@ impl ToCSTError { OwnedRawParseError::ExtraToken { token: (token_start, _, token_end), } => SourceSpan::from(*token_start..*token_end), - OwnedRawParseError::User { error } => error.loc, + OwnedRawParseError::User { error } => error.loc.span, } } diff --git a/cedar-policy-core/src/parser/grammar.lalrpop b/cedar-policy-core/src/parser/grammar.lalrpop index 58962d353..5f5b7c2f5 100644 --- a/cedar-policy-core/src/parser/grammar.lalrpop +++ b/cedar-policy-core/src/parser/grammar.lalrpop @@ -1,4 +1,5 @@ use std::str::FromStr; +use std::sync::Arc; use lalrpop_util::{ParseError, ErrorRecovery}; @@ -6,7 +7,10 @@ use crate::parser::*; use crate::parser::err::{RawErrorRecovery, RawUserError}; use crate::parser::node::Node; -grammar<'err>(errors: &'err mut Vec>); +/// `errors` collects generated errors. +/// +/// `src` is the (full) original source being parsed, which the source locations l,r index into. +grammar<'err, 's>(errors: &'err mut Vec>, src: &'s Arc); extern { type Error = RawUserError; @@ -73,12 +77,12 @@ Comma: Vec = { // Policies := {Policy} pub Policies: Node> = { - => Node::with_source_loc(Some(cst::Policies(ps)),l..r), + => Node::with_source_loc(Some(cst::Policies(ps)), Loc::new(l..r, Arc::clone(src))), } // Annotations := {'@' Ident '(' String ')'} Annotation: Node> = { - "@" "(" ")" => Node::with_source_loc(Some(cst::Annotation{key,value}),l..r) + "@" "(" ")" => Node::with_source_loc(Some(cst::Annotation{key,value}), Loc::new(l..r, Arc::clone(src))) } // Policy := "label" ('permit' | 'forbid') '(' {VariableDef} ')' {Cond} ; @@ -90,8 +94,8 @@ pub Policy: Node> = { ";" - => Node::with_source_loc(Some(cst::Policy{ annotations,effect,variables,conds }),l..r), - => { errors.push(err); Node::with_source_loc(None,l..r) }, + => Node::with_source_loc(Some(cst::Policy{ annotations,effect,variables,conds }), Loc::new(l..r, Arc::clone(src))), + => { errors.push(err); Node::with_source_loc(None, Loc::new(l..r, Arc::clone(src))) }, } // VariableDef := Variable [':' Name] ['is' Add] [('in' | '==') Expr] @@ -101,50 +105,50 @@ pub Policy: Node> = { VariableDef: Node> = { )?> )?> - => Node::with_source_loc(Some(cst::VariableDef{ variable,unused_type_name,entity_type,ineq, }),l..r), + => Node::with_source_loc(Some(cst::VariableDef{ variable,unused_type_name,entity_type,ineq, }), Loc::new(l..r, Arc::clone(src))), } // Identifier, but not the special ones CommonIdent: Node> = { PRINCIPAL - => Node::with_source_loc(Some(cst::Ident::Principal),l..r), + => Node::with_source_loc(Some(cst::Ident::Principal), Loc::new(l..r, Arc::clone(src))), ACTION - => Node::with_source_loc(Some(cst::Ident::Action),l..r), + => Node::with_source_loc(Some(cst::Ident::Action), Loc::new(l..r, Arc::clone(src))), RESOURCE - => Node::with_source_loc(Some(cst::Ident::Resource),l..r), + => Node::with_source_loc(Some(cst::Ident::Resource), Loc::new(l..r, Arc::clone(src))), CONTEXT - => Node::with_source_loc(Some(cst::Ident::Context),l..r), + => Node::with_source_loc(Some(cst::Ident::Context), Loc::new(l..r, Arc::clone(src))), PERMIT - => Node::with_source_loc(Some(cst::Ident::Permit),l..r), + => Node::with_source_loc(Some(cst::Ident::Permit), Loc::new(l..r, Arc::clone(src))), FORBID - => Node::with_source_loc(Some(cst::Ident::Forbid),l..r), + => Node::with_source_loc(Some(cst::Ident::Forbid), Loc::new(l..r, Arc::clone(src))), WHEN - => Node::with_source_loc(Some(cst::Ident::When),l..r), + => Node::with_source_loc(Some(cst::Ident::When), Loc::new(l..r, Arc::clone(src))), UNLESS - => Node::with_source_loc(Some(cst::Ident::Unless),l..r), + => Node::with_source_loc(Some(cst::Ident::Unless), Loc::new(l..r, Arc::clone(src))), IN - => Node::with_source_loc(Some(cst::Ident::In),l..r), + => Node::with_source_loc(Some(cst::Ident::In), Loc::new(l..r, Arc::clone(src))), HAS - => Node::with_source_loc(Some(cst::Ident::Has),l..r), + => Node::with_source_loc(Some(cst::Ident::Has), Loc::new(l..r, Arc::clone(src))), LIKE - => Node::with_source_loc(Some(cst::Ident::Like),l..r), + => Node::with_source_loc(Some(cst::Ident::Like), Loc::new(l..r, Arc::clone(src))), IS - => Node::with_source_loc(Some(cst::Ident::Is),l..r), + => Node::with_source_loc(Some(cst::Ident::Is), Loc::new(l..r, Arc::clone(src))), THEN - => Node::with_source_loc(Some(cst::Ident::Then),l..r), + => Node::with_source_loc(Some(cst::Ident::Then), Loc::new(l..r, Arc::clone(src))), ELSE - => Node::with_source_loc(Some(cst::Ident::Else),l..r), + => Node::with_source_loc(Some(cst::Ident::Else), Loc::new(l..r, Arc::clone(src))), - => Node::with_source_loc(Some(cst::Ident::Ident( i.into() )),l..r), + => Node::with_source_loc(Some(cst::Ident::Ident( i.into() )), Loc::new(l..r, Arc::clone(src))), } // The special ones, play multiple roles SpecialIdent: Node> = { IF - => Node::with_source_loc(Some(cst::Ident::If),l..r), + => Node::with_source_loc(Some(cst::Ident::If), Loc::new(l..r, Arc::clone(src))), TRUE - => Node::with_source_loc(Some(cst::Ident::True),l..r), + => Node::with_source_loc(Some(cst::Ident::True), Loc::new(l..r, Arc::clone(src))), FALSE - => Node::with_source_loc(Some(cst::Ident::False),l..r), + => Node::with_source_loc(Some(cst::Ident::False), Loc::new(l..r, Arc::clone(src))), } #[inline] AnyIdent: Node> = { @@ -155,54 +159,54 @@ pub Ident: Node> = AnyIdent; // Cond := ('when' | 'unless') '{' Expr '}' Cond: Node> = { "{" "}" - => Node::with_source_loc(Some(cst::Cond{cond: i, expr: Some(e)}),l..r), + => Node::with_source_loc(Some(cst::Cond{cond: i, expr: Some(e)}), Loc::new(l..r, Arc::clone(src))), // specifically catch the error case for empty-body, so we can report a good // error message "{" "}" - => Node::with_source_loc(Some(cst::Cond{cond: i, expr: None}),l..r), + => Node::with_source_loc(Some(cst::Cond{cond: i, expr: None}), Loc::new(l..r, Arc::clone(src))), } // Expr := Or | 'if' Expr 'then' Expr 'else' Expr pub Expr: Node> = { - => Node::with_source_loc(Some(cst::Expr{ expr: Box::new(cst::ExprData::Or(o)) }),l..r), + => Node::with_source_loc(Some(cst::Expr{ expr: Box::new(cst::ExprData::Or(o)) }), Loc::new(l..r, Arc::clone(src))), IF THEN ELSE - => Node::with_source_loc(Some(cst::Expr{ expr: Box::new(cst::ExprData::If(i,t,e)) }),l..r), - => { errors.push(err); Node::with_source_loc(None,l..r) }, + => Node::with_source_loc(Some(cst::Expr{ expr: Box::new(cst::ExprData::If(i,t,e)) }), Loc::new(l..r, Arc::clone(src))), + => { errors.push(err); Node::with_source_loc(None, Loc::new(l..r, Arc::clone(src))) }, } // Or := And {'||' And} Or: Node> = { )*> - => Node::with_source_loc(Some(cst::Or{initial: i, extended: e}),l..r), + => Node::with_source_loc(Some(cst::Or{initial: i, extended: e}), Loc::new(l..r, Arc::clone(src))), } // And := Relation {'&&' Relation} And: Node> = { )*> - => Node::with_source_loc(Some(cst::And{initial: i, extended: e}),l..r), + => Node::with_source_loc(Some(cst::And{initial: i, extended: e}), Loc::new(l..r, Arc::clone(src))), } // Relation := Add {RelOp Add} | Add HAS Add | Add LIKE Add | Add IS Add (IN Add)? Relation: Node> = { - => Node::with_source_loc(Some(cst::Relation::Common{initial: i, extended: e}),l..r), + => Node::with_source_loc(Some(cst::Relation::Common{initial: i, extended: e}), Loc::new(l..r, Arc::clone(src))), HAS - => Node::with_source_loc(Some(cst::Relation::Has{target: t, field: f}),l..r), + => Node::with_source_loc(Some(cst::Relation::Has{target: t, field: f}), Loc::new(l..r, Arc::clone(src))), HAS IF => { // Create an add expression from this identifier - let id0 = Node::with_source_loc(Some(cst::Ident::If),l..r); - let id1 = Node::with_source_loc(Some(cst::Name{path: vec![], name: id0}),l..r); - let id2 = Node::with_source_loc(Some(cst::Primary::Name(id1)),l..r); - let id3 = Node::with_source_loc(Some(cst::Member{ item: id2, access: vec![] }),l..r); - let id4 = Node::with_source_loc(Some(cst::Unary{op: None, item:id3}),l..r); - let id5 = Node::with_source_loc(Some(cst::Mult{initial: id4, extended: vec![]}),l..r); - let id6 = Node::with_source_loc(Some(cst::Add{initial:id5, extended: vec![]}),l..r); + 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 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: vec![] }), 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))); + let id5 = Node::with_source_loc(Some(cst::Mult{initial: id4, extended: vec![]}), Loc::new(l..r, Arc::clone(src))); + let id6 = Node::with_source_loc(Some(cst::Add{initial:id5, extended: vec![]}), Loc::new(l..r, Arc::clone(src))); - Node::with_source_loc(Some(cst::Relation::Has{target: t, field: id6}),l..r) + Node::with_source_loc(Some(cst::Relation::Has{target: t, field: id6}), Loc::new(l..r, Arc::clone(src))) }, LIKE - => Node::with_source_loc(Some(cst::Relation::Like{target: t, pattern: p}),l..r), + => Node::with_source_loc(Some(cst::Relation::Like{target: t, pattern: p}), Loc::new(l..r, Arc::clone(src))), IS )?> - => Node::with_source_loc(Some(cst::Relation::IsIn{target: t, entity_type: n, in_entity: e}),l..r), + => Node::with_source_loc(Some(cst::Relation::IsIn{target: t, entity_type: n, in_entity: e}), Loc::new(l..r, Arc::clone(src))), } // RelOp := '<' | '<=' | '>=' | '>' | '!=' | '==' | 'in' RelOp: cst::RelOp = { @@ -227,51 +231,51 @@ MultOp: cst::MultOp = { // Add := Mult {('+' | '-') Mult} Add: Node> = { - => Node::with_source_loc(Some(cst::Add{initial:i, extended: e}),l..r), + => Node::with_source_loc(Some(cst::Add{initial:i, extended: e}), Loc::new(l..r, Arc::clone(src))), } // Mult := Unary {('*' | '/' | '%') Unary} Mult: Node> = { - => Node::with_source_loc(Some(cst::Mult{initial: i, extended: e}),l..r), + => Node::with_source_loc(Some(cst::Mult{initial: i, extended: e}), Loc::new(l..r, Arc::clone(src))), } // Unary := ['!' {'!'} | '-' {'-'}] Member Unary: Node> = { - => Node::with_source_loc(Some(cst::Unary{op: None, item:m}),l..r), + => Node::with_source_loc(Some(cst::Unary{op: None, item:m}), Loc::new(l..r, Arc::clone(src))), "!" - => Node::with_source_loc(Some(cst::Unary{op: Some(cst::NegOp::Bang(1)), item:m}),l..r), + => Node::with_source_loc(Some(cst::Unary{op: Some(cst::NegOp::Bang(1)), item:m}), Loc::new(l..r, Arc::clone(src))), "!" "!" - => Node::with_source_loc(Some(cst::Unary{op: Some(cst::NegOp::Bang(2)), item:m}),l..r), + => Node::with_source_loc(Some(cst::Unary{op: Some(cst::NegOp::Bang(2)), item:m}), Loc::new(l..r, Arc::clone(src))), "!" "!" "!" - => Node::with_source_loc(Some(cst::Unary{op: Some(cst::NegOp::Bang(3)), item:m}),l..r), + => Node::with_source_loc(Some(cst::Unary{op: Some(cst::NegOp::Bang(3)), item:m}), Loc::new(l..r, Arc::clone(src))), "!" "!" "!" "!" - => Node::with_source_loc(Some(cst::Unary{op: Some(cst::NegOp::Bang(4)), item:m}),l..r), + => Node::with_source_loc(Some(cst::Unary{op: Some(cst::NegOp::Bang(4)), item:m}), Loc::new(l..r, Arc::clone(src))), "!" "!" "!" "!" "!"+ - => Node::with_source_loc(Some(cst::Unary{op: Some(cst::NegOp::OverBang), item:m}),l..r), + => Node::with_source_loc(Some(cst::Unary{op: Some(cst::NegOp::OverBang), item:m}), Loc::new(l..r, Arc::clone(src))), "-" - => Node::with_source_loc(Some(cst::Unary{op: Some(cst::NegOp::Dash(1)), item:m}),l..r), + => Node::with_source_loc(Some(cst::Unary{op: Some(cst::NegOp::Dash(1)), item:m}), Loc::new(l..r, Arc::clone(src))), "-" "-" - => Node::with_source_loc(Some(cst::Unary{op: Some(cst::NegOp::Dash(2)), item:m}),l..r), + => Node::with_source_loc(Some(cst::Unary{op: Some(cst::NegOp::Dash(2)), item:m}), Loc::new(l..r, Arc::clone(src))), "-" "-" "-" - => Node::with_source_loc(Some(cst::Unary{op: Some(cst::NegOp::Dash(3)), item:m}),l..r), + => Node::with_source_loc(Some(cst::Unary{op: Some(cst::NegOp::Dash(3)), item:m}), Loc::new(l..r, Arc::clone(src))), "-" "-" "-" "-" - => Node::with_source_loc(Some(cst::Unary{op: Some(cst::NegOp::Dash(4)), item:m}),l..r), + => Node::with_source_loc(Some(cst::Unary{op: Some(cst::NegOp::Dash(4)), item:m}), Loc::new(l..r, Arc::clone(src))), "-" "-" "-" "-" "-"+ - => Node::with_source_loc(Some(cst::Unary{op: Some(cst::NegOp::OverDash), item:m}),l..r), + => Node::with_source_loc(Some(cst::Unary{op: Some(cst::NegOp::OverDash), item:m}), Loc::new(l..r, Arc::clone(src))), } // Member := Primary { MemAccess } Member: Node> = { - => Node::with_source_loc(Some(cst::Member{ item: p, access: a }),l..r), + => Node::with_source_loc(Some(cst::Member{ item: p, access: a }), Loc::new(l..r, Arc::clone(src))), } // MemAccess := '.' IDENT | '(' [ExprList] ')' | '[' Expr ']' MemAccess: Node> = { "." - => Node::with_source_loc(Some(cst::MemAccess::Field(i)),l..r), + => Node::with_source_loc(Some(cst::MemAccess::Field(i)), Loc::new(l..r, Arc::clone(src))), "(" > ")" - => Node::with_source_loc(Some(cst::MemAccess::Call(es)),l..r), + => Node::with_source_loc(Some(cst::MemAccess::Call(es)), Loc::new(l..r, Arc::clone(src))), "[" "]" - => Node::with_source_loc(Some(cst::MemAccess::Index(e)),l..r), + => Node::with_source_loc(Some(cst::MemAccess::Index(e)), Loc::new(l..r, Arc::clone(src))), } // Primary := LITERAL | // Ref | @@ -282,19 +286,19 @@ MemAccess: Node> = { // '{' [MapOrFieldInits] '}' pub Primary: Node> = { - => Node::with_source_loc(Some(cst::Primary::Literal(lit)),l..r), + => Node::with_source_loc(Some(cst::Primary::Literal(lit)), Loc::new(l..r, Arc::clone(src))), - => Node::with_source_loc(Some(cst::Primary::Ref(refr)),l..r), + => Node::with_source_loc(Some(cst::Primary::Ref(refr)), Loc::new(l..r, Arc::clone(src))), - => Node::with_source_loc(Some(cst::Primary::Name(n)),l..r), + => Node::with_source_loc(Some(cst::Primary::Name(n)), Loc::new(l..r, Arc::clone(src))), - => Node::with_source_loc(Some(cst::Primary::Slot(s)),l..r), + => Node::with_source_loc(Some(cst::Primary::Slot(s)), Loc::new(l..r, Arc::clone(src))), "(" ")" - => Node::with_source_loc(Some(cst::Primary::Expr(e)),l..r), + => Node::with_source_loc(Some(cst::Primary::Expr(e)), Loc::new(l..r, Arc::clone(src))), "[" > "]" - => Node::with_source_loc(Some(cst::Primary::EList(es)),l..r), + => Node::with_source_loc(Some(cst::Primary::EList(es)), Loc::new(l..r, Arc::clone(src))), "{" > "}" - => Node::with_source_loc(Some(cst::Primary::RInits(is)),l..r), + => Node::with_source_loc(Some(cst::Primary::RInits(is)), Loc::new(l..r, Arc::clone(src))), } // Name := IDENT {'::' IDENT} @@ -304,71 +308,71 @@ pub Name: Node> = NameInline; #[inline] NameInline: Node> = { - => Node::with_source_loc(Some(cst::Name{path: vec![], name: n}),l..r), + => Node::with_source_loc(Some(cst::Name{path: vec![], name: n}), Loc::new(l..r, Arc::clone(src))), "::")+> - => Node::with_source_loc(Some(cst::Name{path: p, name: n}),l..r) + => Node::with_source_loc(Some(cst::Name{path: p, name: n}), Loc::new(l..r, Arc::clone(src))) } // Ref := Name '::' (STR | '{' [RefInits] '}') pub Ref: Node> = { "::" - => Node::with_source_loc(Some(cst::Ref::Uid{path:n,eid:s}),l..r), + => Node::with_source_loc(Some(cst::Ref::Uid{path:n,eid:s}), Loc::new(l..r, Arc::clone(src))), "::" "{" > "}" - => Node::with_source_loc(Some(cst::Ref::Ref{path:n,rinits:is}),l..r), + => Node::with_source_loc(Some(cst::Ref::Ref{path:n,rinits:is}), Loc::new(l..r, Arc::clone(src))), } // RefInit := IDENT ':' LITERAL RefInit: Node> = { ":" - => Node::with_source_loc(Some(cst::RefInit(i,lit)),l..r), + => Node::with_source_loc(Some(cst::RefInit(i,lit)), Loc::new(l..r, Arc::clone(src))), } // RecInit := Expr ':' Expr -or- IDENT : Expr RecInit: Node> = { IF ":" => { // Create an expression from this identifier - let id0 = Node::with_source_loc(Some(cst::Ident::If),l..r); - let id1 = Node::with_source_loc(Some(cst::Name{path: vec![], name: id0}),l..r); - let id2 = Node::with_source_loc(Some(cst::Primary::Name(id1)),l..r); - let id3 = Node::with_source_loc(Some(cst::Member{ item: id2, access: vec![] }),l..r); - let id4 = Node::with_source_loc(Some(cst::Unary{op: None, item:id3}),l..r); - let id5 = Node::with_source_loc(Some(cst::Mult{initial: id4, extended: vec![]}),l..r); - let id6 = Node::with_source_loc(Some(cst::Add{initial:id5, extended: vec![]}),l..r); - let id7 = Node::with_source_loc(Some(cst::Relation::Common{initial: id6, extended: vec![]}),l..r); - let id8 = Node::with_source_loc(Some(cst::And{initial: id7, extended: vec![]}),l..r); - let id9 = Node::with_source_loc(Some(cst::Or{initial: id8, extended: vec![]}),l..r); - let e1 = Node::with_source_loc(Some(cst::Expr{ expr: Box::new(cst::ExprData::Or(id9)) }),l..r); + 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 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: vec![] }), 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))); + let id5 = Node::with_source_loc(Some(cst::Mult{initial: id4, extended: vec![]}), Loc::new(l..r, Arc::clone(src))); + let id6 = Node::with_source_loc(Some(cst::Add{initial:id5, extended: vec![]}), Loc::new(l..r, Arc::clone(src))); + let id7 = Node::with_source_loc(Some(cst::Relation::Common{initial: id6, extended: vec![]}), Loc::new(l..r, Arc::clone(src))); + let id8 = Node::with_source_loc(Some(cst::And{initial: id7, extended: vec![]}), Loc::new(l..r, Arc::clone(src))); + let id9 = Node::with_source_loc(Some(cst::Or{initial: id8, extended: vec![]}), Loc::new(l..r, Arc::clone(src))); + let e1 = Node::with_source_loc(Some(cst::Expr{ expr: Box::new(cst::ExprData::Or(id9)) }), Loc::new(l..r, Arc::clone(src))); - Node::with_source_loc(Some(cst::RecInit(e1,e2)),l..r) + Node::with_source_loc(Some(cst::RecInit(e1,e2)), Loc::new(l..r, Arc::clone(src))) }, ":" - => Node::with_source_loc(Some(cst::RecInit(e1,e2)),l..r), + => Node::with_source_loc(Some(cst::RecInit(e1,e2)), Loc::new(l..r, Arc::clone(src))), } Slot: Node> = { PRINCIPAL_SLOT - => Node::with_source_loc(Some(cst::Slot::Principal), l..r), + => Node::with_source_loc(Some(cst::Slot::Principal), Loc::new(l..r, Arc::clone(src))), RESOURCE_SLOT - => Node::with_source_loc(Some(cst::Slot::Resource), l..r), + => Node::with_source_loc(Some(cst::Slot::Resource), Loc::new(l..r, Arc::clone(src))), - => Node::with_source_loc(Some(cst::Slot::Other(s.into())), l..r), + => Node::with_source_loc(Some(cst::Slot::Other(s.into())), Loc::new(l..r, Arc::clone(src))), } // LITERAL := BOOL | INT | STR Literal: Node> = { TRUE - => Node::with_source_loc(Some(cst::Literal::True),l..r), + => Node::with_source_loc(Some(cst::Literal::True), Loc::new(l..r, Arc::clone(src))), FALSE - => Node::with_source_loc(Some(cst::Literal::False),l..r), + => Node::with_source_loc(Some(cst::Literal::False), Loc::new(l..r, Arc::clone(src))), =>? match u64::from_str(n) { - Ok(n) => Ok(Node::with_source_loc(Some(cst::Literal::Num(n)),l..r)), + Ok(n) => Ok(Node::with_source_loc(Some(cst::Literal::Num(n)), Loc::new(l..r, Arc::clone(src)))), Err(e) => Err(ParseError::User { - error: Node::with_source_loc(format!("integer parse error: {e}"),l..r), + error: Node::with_source_loc(format!("integer parse error: {e}"), Loc::new(l..r, Arc::clone(src))), }), }, - => Node::with_source_loc(Some(cst::Literal::Str(s)),l..r), + => Node::with_source_loc(Some(cst::Literal::Str(s)), Loc::new(l..r, Arc::clone(src))), } Str: Node> = { - => Node::with_source_loc(Some(cst::Str::String(s[1..(s.len() - 1)].into())),l..r), + => Node::with_source_loc(Some(cst::Str::String(s[1..(s.len() - 1)].into())), Loc::new(l..r, Arc::clone(src))), } diff --git a/cedar-policy-core/src/parser/loc.rs b/cedar-policy-core/src/parser/loc.rs new file mode 100644 index 000000000..7bb2bf153 --- /dev/null +++ b/cedar-policy-core/src/parser/loc.rs @@ -0,0 +1,41 @@ +use serde::{Deserialize, Serialize}; +use std::sync::Arc; + +/// Represents a source location: index/range, and a reference to the source +/// code which that index/range indexes into +#[derive(Debug, Clone, PartialEq, Eq, Hash, Deserialize, Serialize)] +pub struct Loc { + /// `SourceSpan` indicating a specific source code location or range + pub span: miette::SourceSpan, + + /// Original source code (which the above source span indexes into) + pub src: Arc, +} + +impl Loc { + /// Create a new `Loc` + pub fn new(span: impl Into, src: Arc) -> Self { + Self { + span: span.into(), + src, + } + } + + /// Create a new `Loc` with the same source code but a different span + pub fn span(&self, span: impl Into) -> Self { + Self { + span: span.into(), + src: Arc::clone(&self.src), + } + } + + /// Get the index representing the start of the source span + pub fn start(&self) -> usize { + self.span.offset() + } + + /// Get the index representing the end of the source span + pub fn end(&self) -> usize { + self.span.offset() + self.span.len() + } +} diff --git a/cedar-policy-core/src/parser/node.rs b/cedar-policy-core/src/parser/node.rs index 0c78584eb..1f4718133 100644 --- a/cedar-policy-core/src/parser/node.rs +++ b/cedar-policy-core/src/parser/node.rs @@ -21,6 +21,7 @@ use miette::Diagnostic; use serde::{Deserialize, Serialize}; use super::err::{ToASTError, ToASTErrorKind}; +use super::loc::Loc; /// Metadata for our syntax trees #[derive(Debug, Clone, Deserialize, Serialize)] @@ -29,23 +30,20 @@ pub struct Node { pub node: T, /// Source location - pub loc: miette::SourceSpan, + pub loc: Loc, } impl Node { /// Create a new Node with the given source location - pub fn with_source_loc(node: T, loc: impl Into) -> Self { - Node { - node, - loc: loc.into(), - } + pub fn with_source_loc(node: T, loc: Loc) -> Self { + Node { node, loc } } /// Transform the inner value while retaining the attached source info. pub fn map(self, f: impl FnOnce(T) -> R) -> Node { Node { node: f(self.node), - loc: self.loc, + loc: self.loc.clone(), } } @@ -53,7 +51,7 @@ impl Node { pub fn as_ref(&self) -> Node<&T> { Node { node: &self.node, - loc: self.loc, + loc: self.loc.clone(), } } @@ -61,18 +59,18 @@ impl Node { pub fn as_mut(&mut self) -> Node<&mut T> { Node { node: &mut self.node, - loc: self.loc, + loc: self.loc.clone(), } } /// Consume the `Node`, yielding the node and attached source info. - pub fn into_inner(self) -> (T, miette::SourceSpan) { + pub fn into_inner(self) -> (T, Loc) { (self.node, self.loc) } /// Utility to construct a `ToAstError` with the source location taken from this node. pub fn to_ast_err(&self, error_kind: impl Into) -> ToASTError { - ToASTError::new(error_kind.into(), self.loc) + ToASTError::new(error_kind.into(), self.loc.clone()) } } @@ -136,7 +134,7 @@ impl Diagnostic for Node { fn labels(&self) -> Option + '_>> { Some(Box::new(std::iter::once(miette::LabeledSpan::underline( - self.loc, + self.loc.span, )))) } @@ -175,7 +173,7 @@ impl Node> { pub fn collapse(&self) -> Option> { self.node.as_ref().map(|node| Node { node, - loc: self.loc, + loc: self.loc.clone(), }) } @@ -183,16 +181,16 @@ impl Node> { /// if no main data or if `f` returns `None`. pub fn apply(&self, f: F) -> Option where - F: FnOnce(&T, miette::SourceSpan) -> Option, + F: FnOnce(&T, &Loc) -> Option, { - f(self.node.as_ref()?, self.loc) + f(self.node.as_ref()?, &self.loc) } - /// Apply the function `f` to the main data and source info, consuming them. + /// Apply the function `f` to the main data and `Loc`, consuming them. /// Returns `None` if no main data or if `f` returns `None`. pub fn into_apply(self, f: F) -> Option where - F: FnOnce(T, miette::SourceSpan) -> Option, + F: FnOnce(T, Loc) -> Option, { f(self.node?, self.loc) } diff --git a/cedar-policy-core/src/parser/text_to_cst.rs b/cedar-policy-core/src/parser/text_to_cst.rs index 41962b2ee..6b48605ba 100644 --- a/cedar-policy-core/src/parser/text_to_cst.rs +++ b/cedar-policy-core/src/parser/text_to_cst.rs @@ -31,9 +31,9 @@ lalrpop_mod!( "/src/parser/grammar.rs" ); -use lazy_static::lazy_static; - use super::*; +use node::Node; +use std::sync::Arc; /// This helper function calls a generated parser, collects errors that could be /// generated multiple ways, and returns a single Result where the error type is @@ -43,12 +43,13 @@ fn parse_collect_errors<'a, P, T>( parse: impl FnOnce( &P, &mut Vec>, + &Arc, &'a str, ) -> Result>, text: &'a str, ) -> Result { let mut errs = Vec::new(); - let result = parse(parser, &mut errs, text); + let result = parse(parser, &mut errs, &Arc::from(text), text); let mut errors: err::ParseErrors = errs .into_iter() @@ -69,7 +70,7 @@ fn parse_collect_errors<'a, P, T>( } // Thread-safe "global" parsers, initialized at first use -lazy_static! { +lazy_static::lazy_static! { static ref POLICIES_PARSER: grammar::PoliciesParser = grammar::PoliciesParser::new(); static ref POLICY_PARSER: grammar::PolicyParser = grammar::PolicyParser::new(); static ref EXPR_PARSER: grammar::ExprParser = grammar::ExprParser::new(); @@ -80,37 +81,37 @@ lazy_static! { } /// Create CST for multiple policies from text -pub fn parse_policies(text: &str) -> Result>, err::ParseErrors> { +pub fn parse_policies(text: &str) -> Result>, err::ParseErrors> { parse_collect_errors(&*POLICIES_PARSER, grammar::PoliciesParser::parse, text) } /// Create CST for one policy statement from text -pub fn parse_policy(text: &str) -> Result>, err::ParseErrors> { +pub fn parse_policy(text: &str) -> Result>, err::ParseErrors> { parse_collect_errors(&*POLICY_PARSER, grammar::PolicyParser::parse, text) } /// Create CST for one Expression from text -pub fn parse_expr(text: &str) -> Result>, err::ParseErrors> { +pub fn parse_expr(text: &str) -> Result>, err::ParseErrors> { parse_collect_errors(&*EXPR_PARSER, grammar::ExprParser::parse, text) } /// Create CST for one Entity Ref (i.e., UID) from text -pub fn parse_ref(text: &str) -> Result>, err::ParseErrors> { +pub fn parse_ref(text: &str) -> Result>, err::ParseErrors> { parse_collect_errors(&*REF_PARSER, grammar::RefParser::parse, text) } /// Create CST for one Primary value from text -pub fn parse_primary(text: &str) -> Result>, err::ParseErrors> { +pub fn parse_primary(text: &str) -> Result>, err::ParseErrors> { parse_collect_errors(&*PRIMARY_PARSER, grammar::PrimaryParser::parse, text) } /// Parse text as a Name, or fail if it does not parse as a Name -pub fn parse_name(text: &str) -> Result>, err::ParseErrors> { +pub fn parse_name(text: &str) -> Result>, err::ParseErrors> { parse_collect_errors(&*NAME_PARSER, grammar::NameParser::parse, text) } /// Parse text as an identifier, or fail if it does not parse as an identifier -pub fn parse_ident(text: &str) -> Result>, err::ParseErrors> { +pub fn parse_ident(text: &str) -> Result>, err::ParseErrors> { parse_collect_errors(&*IDENT_PARSER, grammar::IdentParser::parse, text) } @@ -892,16 +893,14 @@ mod tests { #[test] fn policies6() { // test that an error doesn't stop the parser + let src = r#" + // use a number to error + 3(principal:p,action:a,resource:r)when{w}unless{u}advice{"doit"}; + permit(principal:p,action:a,resource:r)when{w}unless{u}advice{"doit"}; + permit(principal:p,action:a,resource:r)when{w}unless{u}advice{"doit"}; + "#; let policies = POLICIES_PARSER - .parse( - &mut Vec::new(), - r#" - // use a number to error - 3(principal:p,action:a,resource:r)when{w}unless{u}advice{"doit"}; - permit(principal:p,action:a,resource:r)when{w}unless{u}advice{"doit"}; - permit(principal:p,action:a,resource:r)when{w}unless{u}advice{"doit"}; - "#, - ) + .parse(&mut Vec::new(), &Arc::from(src), src) .expect("parser error") .node .expect("no data"); diff --git a/cedar-policy-formatter/src/pprint/doc.rs b/cedar-policy-formatter/src/pprint/doc.rs index 93c784cb8..8ef555137 100644 --- a/cedar-policy-formatter/src/pprint/doc.rs +++ b/cedar-policy-formatter/src/pprint/doc.rs @@ -37,7 +37,7 @@ impl Doc for Ident { impl Doc for Node> { fn to_doc(&self, context: &mut Context<'_>) -> Option> { let vd = self.as_inner()?; - let start_comment = get_comment_at_start(self.loc, &mut context.tokens)?; + let start_comment = get_comment_at_start(self.loc.span, &mut context.tokens)?; let var_doc = vd.variable.as_inner()?.to_doc(context)?; let is_doc = match &vd.entity_type { @@ -45,13 +45,13 @@ impl Doc for Node> { RcDoc::line() .append(add_comment( RcDoc::text("is"), - get_comment_after_end(vd.variable.loc, &mut context.tokens)?, + get_comment_after_end(vd.variable.loc.span, &mut context.tokens)?, RcDoc::nil(), )) .group() .append(RcDoc::line().append(add_comment( entity_type.to_doc(context)?, - get_comment_at_start(entity_type.loc, &mut context.tokens)?, + get_comment_at_start(entity_type.loc.span, &mut context.tokens)?, RcDoc::nil(), ))) .nest(context.config.indent_width) @@ -60,15 +60,15 @@ impl Doc for Node> { None => Some(RcDoc::nil()), }?; - let end_comment = get_comment_at_end(self.loc, &mut context.tokens)?; + let end_comment = get_comment_at_end(self.loc.span, &mut context.tokens)?; Some(match &vd.ineq { Some((op, rhs)) => { let op_comment = match &vd.entity_type { Some(entity_type) => { - get_comment_after_end(entity_type.loc, &mut context.tokens)? + get_comment_after_end(entity_type.loc.span, &mut context.tokens)? } - None => get_comment_after_end(vd.variable.loc, &mut context.tokens)?, + None => get_comment_after_end(vd.variable.loc.span, &mut context.tokens)?, }; get_leading_comment_doc_from_str(&start_comment.leading_comment).append( var_doc @@ -105,16 +105,16 @@ impl Doc for Node> { impl Doc for Node> { fn to_doc(&self, context: &mut Context<'_>) -> Option> { let cond = self.as_inner()?; - let lb_comment = get_comment_after_end(cond.cond.loc, &mut context.tokens)?; - let rb_comment = get_comment_at_end(self.loc, &mut context.tokens)?; - let cond_comment = get_comment_at_start(cond.cond.loc, &mut context.tokens)?; + let lb_comment = get_comment_after_end(cond.cond.loc.span, &mut context.tokens)?; + let rb_comment = get_comment_at_end(self.loc.span, &mut context.tokens)?; + let cond_comment = get_comment_at_start(cond.cond.loc.span, &mut context.tokens)?; let rb_doc = add_comment(RcDoc::text("}"), rb_comment, RcDoc::nil()); let cond_doc = cond.cond.to_doc(context)?; Some(match cond.expr.as_ref() { Some(expr) => { let expr_leading_comment = - get_leading_comment_at_start(expr.loc, &mut context.tokens)?; + get_leading_comment_at_start(expr.loc.span, &mut context.tokens)?; let expr_doc = expr.to_doc(context)?; get_leading_comment_doc_from_str(&cond_comment.leading_comment).append( cond_doc @@ -180,9 +180,9 @@ impl Doc for Node> { .nest(context.config.indent_width), ) } - let if_comment = get_comment_at_start(self.loc, &mut context.tokens)?; - let else_comment = get_comment_after_end(c.loc, &mut context.tokens)?; - let then_comment = get_comment_after_end(t.loc, &mut context.tokens)?; + let if_comment = get_comment_at_start(self.loc.span, &mut context.tokens)?; + let else_comment = get_comment_after_end(c.loc.span, &mut context.tokens)?; + let then_comment = get_comment_after_end(t.loc.span, &mut context.tokens)?; Some( pp_group("if", if_comment, c, context) .append(RcDoc::line()) @@ -207,7 +207,7 @@ impl Doc for Node> { let es: Vec<_> = std::iter::once(initial).chain(extended.iter()).collect(); let mut d: RcDoc<'_> = RcDoc::nil(); for e in es.iter().take(es.len() - 1) { - let op_comment = get_comment_after_end(e.loc, &mut context.tokens)?; + let op_comment = get_comment_after_end(e.loc.span, &mut context.tokens)?; d = d .append(e.to_doc(context)) .append(RcDoc::space()) @@ -227,7 +227,7 @@ impl Doc for Node> { let es: Vec<_> = std::iter::once(initial).chain(extended.iter()).collect(); let mut d: RcDoc<'_> = RcDoc::nil(); for e in es.iter().take(es.len() - 1) { - let op_comment = get_comment_after_end(e.loc, &mut context.tokens)?; + let op_comment = get_comment_after_end(e.loc.span, &mut context.tokens)?; d = d .append(e.to_doc(context)) .append(RcDoc::space()) @@ -253,7 +253,7 @@ impl Doc for Node> { .append(RcDoc::space()) .append(add_comment( RcDoc::as_string(op), - get_comment_after_end(initial.loc, &mut context.tokens)?, + get_comment_after_end(initial.loc.span, &mut context.tokens)?, RcDoc::nil(), )) .append(RcDoc::space()) @@ -269,7 +269,7 @@ impl Doc for Node> { .append(RcDoc::line()) .append(add_comment( RcDoc::text("has"), - get_comment_after_end(target.loc, &mut context.tokens)?, + get_comment_after_end(target.loc.span, &mut context.tokens)?, RcDoc::nil(), )) .append(RcDoc::line()) @@ -282,7 +282,7 @@ impl Doc for Node> { .append(RcDoc::line()) .append(add_comment( RcDoc::text("like"), - get_comment_after_end(target.loc, &mut context.tokens)?, + get_comment_after_end(target.loc.span, &mut context.tokens)?, RcDoc::nil(), )) .append(RcDoc::line()) @@ -299,7 +299,7 @@ impl Doc for Node> { .append(RcDoc::space()) .append(add_comment( RcDoc::text("is"), - get_comment_after_end(target.loc, &mut context.tokens)?, + get_comment_after_end(target.loc.span, &mut context.tokens)?, RcDoc::nil(), )) .append(RcDoc::space()) @@ -314,7 +314,7 @@ impl Doc for Node> { .append(RcDoc::line()) .append(add_comment( RcDoc::text("in"), - get_comment_after_end(entity_type.loc, &mut context.tokens)?, + get_comment_after_end(entity_type.loc.span, &mut context.tokens)?, RcDoc::nil(), )) .append(RcDoc::space()) @@ -350,7 +350,7 @@ impl Doc for Node> { .append(RcDoc::space()) .append(add_comment( op.to_doc(context)?, - get_comment_after_end(pair.1.loc, &mut context.tokens)?, + get_comment_after_end(pair.1.loc.span, &mut context.tokens)?, RcDoc::nil(), )) .append(RcDoc::line()) @@ -386,7 +386,7 @@ impl Doc for Node> { .append(RcDoc::space()) .append(add_comment( op.to_doc(context)?, - get_comment_after_end(pair.1.loc, &mut context.tokens)?, + get_comment_after_end(pair.1.loc.span, &mut context.tokens)?, RcDoc::nil(), )) .append(RcDoc::line()) @@ -408,7 +408,7 @@ impl Doc for Node> { NegOp::OverBang | NegOp::OverDash => None, NegOp::Bang(n) | NegOp::Dash(n) => { let comment = get_comment_in_range( - (self.loc.offset()..e.item.loc.offset()).into(), + (self.loc.start()..e.item.loc.start()).into(), &mut context.tokens, ); if comment.len() != n as usize { @@ -468,7 +468,7 @@ impl Doc for Node> { .append(RcDoc::line_()) .append(add_comment( RcDoc::as_string(":"), - get_comment_after_end(e.0.loc, &mut context.tokens)?, + get_comment_after_end(e.0.loc.span, &mut context.tokens)?, RcDoc::nil(), )) .append(value_doc), @@ -492,7 +492,7 @@ impl Doc for Node> { Some(( d.append(add_comment( RcDoc::as_string("::"), - get_comment_after_end(e.loc, &mut context.tokens)?, + get_comment_after_end(e.loc.span, &mut context.tokens)?, RcDoc::nil(), )) .append(p.to_doc(context)?), @@ -502,7 +502,7 @@ impl Doc for Node> { .0 .append(add_comment( RcDoc::as_string("::"), - get_comment_after_end(path.last()?.loc, &mut context.tokens)?, + get_comment_after_end(path.last()?.loc.span, &mut context.tokens)?, RcDoc::nil(), )) .append(n.to_doc(context)), @@ -516,7 +516,7 @@ impl Doc for Node> { let e = self.as_inner()?; Some(add_comment( RcDoc::as_string(e), - get_comment_at_start(self.loc, &mut context.tokens)?, + get_comment_at_start(self.loc.span, &mut context.tokens)?, RcDoc::nil(), )) } @@ -529,7 +529,7 @@ impl Doc for Node> { path.to_doc(context)? .append(add_comment( RcDoc::text("::"), - get_comment_after_end(path.loc, &mut context.tokens)?, + get_comment_after_end(path.loc.span, &mut context.tokens)?, RcDoc::nil(), )) .append(eid.to_doc(context)?), @@ -543,7 +543,7 @@ impl Doc for Node> { fn to_doc(&self, context: &mut Context<'_>) -> Option> { Some(add_comment( RcDoc::as_string(self.as_inner()?), - get_comment_at_start(self.loc, &mut context.tokens)?, + get_comment_at_start(self.loc.span, &mut context.tokens)?, RcDoc::nil(), )) } @@ -553,7 +553,7 @@ impl Doc for Node> { fn to_doc(&self, context: &mut Context<'_>) -> Option> { Some(add_comment( RcDoc::as_string(self.as_inner()?), - get_comment_at_start(self.loc, &mut context.tokens)?, + get_comment_at_start(self.loc.span, &mut context.tokens)?, RcDoc::nil(), )) } @@ -569,7 +569,7 @@ impl Doc for Node> { Primary::Expr(e) => Some( add_comment( RcDoc::text("("), - get_comment_at_start(self.loc, &mut context.tokens)?, + get_comment_at_start(self.loc.span, &mut context.tokens)?, RcDoc::nil(), ) .append(RcDoc::nil()) @@ -577,7 +577,7 @@ impl Doc for Node> { .append(RcDoc::nil()) .append(add_comment( RcDoc::text(")"), - get_comment_at_end(self.loc, &mut context.tokens)?, + get_comment_at_end(self.loc.span, &mut context.tokens)?, RcDoc::nil(), )) .group(), @@ -593,7 +593,7 @@ impl Doc for Node> { Some(( d.append(add_comment( RcDoc::as_string(","), - get_comment_after_end(e.loc, &mut context.tokens)?, + get_comment_after_end(e.loc.span, &mut context.tokens)?, RcDoc::nil(), )) .append(RcDoc::line()) @@ -605,12 +605,12 @@ impl Doc for Node> { }, add_comment( RcDoc::text("["), - get_comment_at_start(self.loc, &mut context.tokens)?, + get_comment_at_start(self.loc.span, &mut context.tokens)?, RcDoc::nil(), ), add_comment( RcDoc::text("]"), - get_comment_at_end(self.loc, &mut context.tokens)?, + get_comment_at_end(self.loc.span, &mut context.tokens)?, RcDoc::nil(), ), )), @@ -625,7 +625,7 @@ impl Doc for Node> { Some(( d.append(add_comment( RcDoc::as_string(","), - get_comment_after_end(e.loc, &mut context.tokens)?, + get_comment_after_end(e.loc.span, &mut context.tokens)?, RcDoc::nil(), )) .append(RcDoc::line()) @@ -637,12 +637,12 @@ impl Doc for Node> { }, add_comment( RcDoc::text("{"), - get_comment_at_start(self.loc, &mut context.tokens)?, + get_comment_at_start(self.loc.span, &mut context.tokens)?, RcDoc::nil(), ), add_comment( RcDoc::text("}"), - get_comment_at_end(self.loc, &mut context.tokens)?, + get_comment_at_end(self.loc.span, &mut context.tokens)?, RcDoc::nil(), ), )), @@ -658,7 +658,7 @@ impl Doc for Node> { MemAccess::Field(f) => Some( add_comment( RcDoc::text("."), - get_comment_at_start(self.loc, &mut context.tokens)?, + get_comment_at_start(self.loc.span, &mut context.tokens)?, RcDoc::nil(), ) .append(f.to_doc(context)), @@ -666,7 +666,7 @@ impl Doc for Node> { MemAccess::Call(args) => Some( add_comment( RcDoc::text("("), - get_comment_at_start(self.loc, &mut context.tokens)?, + get_comment_at_start(self.loc.span, &mut context.tokens)?, RcDoc::nil(), ) .append(RcDoc::line_()) @@ -682,7 +682,7 @@ impl Doc for Node> { Some(( d.append(add_comment( RcDoc::as_string(","), - get_comment_after_end(e.loc, &mut context.tokens)?, + get_comment_after_end(e.loc.span, &mut context.tokens)?, RcDoc::nil(), )) .append(RcDoc::line()) @@ -697,14 +697,14 @@ impl Doc for Node> { .append(RcDoc::line_()) .append(add_comment( RcDoc::text(")"), - get_comment_at_end(self.loc, &mut context.tokens)?, + get_comment_at_end(self.loc.span, &mut context.tokens)?, RcDoc::nil(), )), ), MemAccess::Index(idx) => Some( add_comment( RcDoc::text("["), - get_comment_at_start(self.loc, &mut context.tokens)?, + get_comment_at_start(self.loc.span, &mut context.tokens)?, RcDoc::nil(), ) .append(RcDoc::line_()) @@ -712,7 +712,7 @@ impl Doc for Node> { .append(RcDoc::line_()) .append(add_comment( RcDoc::text("]"), - get_comment_at_end(self.loc, &mut context.tokens)?, + get_comment_at_end(self.loc.span, &mut context.tokens)?, RcDoc::nil(), )), ), @@ -727,17 +727,17 @@ impl Doc for Node> { let val_doc = annotation.value.to_doc(context); let at_doc = add_comment( RcDoc::text("@"), - get_comment_at_start(self.loc, &mut context.tokens)?, + get_comment_at_start(self.loc.span, &mut context.tokens)?, RcDoc::nil(), ); let lp_doc = add_comment( RcDoc::text("("), - get_comment_after_end(annotation.key.loc, &mut context.tokens)?, + get_comment_after_end(annotation.key.loc.span, &mut context.tokens)?, RcDoc::nil(), ); let rp_doc = add_comment( RcDoc::text(")"), - get_comment_at_end(self.loc, &mut context.tokens)?, + get_comment_at_end(self.loc.span, &mut context.tokens)?, RcDoc::hardline(), ); Some( @@ -754,7 +754,7 @@ impl Doc for Node> { fn to_doc(&self, context: &mut Context<'_>) -> Option> { Some(add_comment( self.as_inner()?.to_doc(context)?, - get_comment_at_start(self.loc, &mut context.tokens)?, + get_comment_at_start(self.loc.span, &mut context.tokens)?, RcDoc::nil(), )) } @@ -769,7 +769,7 @@ impl Doc for Node> { RcDoc::nil(), ); let eff_leading_comment = - get_leading_comment_at_start(policy.effect.loc, &mut context.tokens)?; + get_leading_comment_at_start(policy.effect.loc.span, &mut context.tokens)?; let eff_doc = policy.effect.to_doc(context)?; let vars = &policy.variables; let principal_doc = vars.get(0)?.to_doc(context)?; @@ -785,13 +785,13 @@ impl Doc for Node> { principal_doc .append(add_comment( RcDoc::text(","), - get_comment_after_end(vars.get(0)?.loc, &mut context.tokens)?, + get_comment_after_end(vars.get(0)?.loc.span, &mut context.tokens)?, RcDoc::space(), )) .append(action_doc) .append(add_comment( RcDoc::text(","), - get_comment_after_end(vars.get(1)?.loc, &mut context.tokens)?, + get_comment_after_end(vars.get(1)?.loc.span, &mut context.tokens)?, RcDoc::space(), )) .append(resource_doc) @@ -803,13 +803,13 @@ impl Doc for Node> { principal_doc .append(add_comment( RcDoc::text(","), - get_comment_after_end(vars.get(0)?.loc, &mut context.tokens)?, + get_comment_after_end(vars.get(0)?.loc.span, &mut context.tokens)?, RcDoc::hardline(), )) .append(action_doc) .append(add_comment( RcDoc::text(","), - get_comment_after_end(vars.get(1)?.loc, &mut context.tokens)?, + get_comment_after_end(vars.get(1)?.loc.span, &mut context.tokens)?, RcDoc::hardline(), )) .append(resource_doc), @@ -828,7 +828,7 @@ impl Doc for Node> { .append(RcDoc::line()) .append(add_comment( RcDoc::text("("), - get_comment_after_end(policy.effect.loc, &mut context.tokens)?, + get_comment_after_end(policy.effect.loc.span, &mut context.tokens)?, RcDoc::nil(), )) .group(), @@ -837,7 +837,7 @@ impl Doc for Node> { .append(vars_doc) .append(add_comment( RcDoc::text(")"), - get_comment_after_end(vars.get(2)?.loc, &mut context.tokens)?, + get_comment_after_end(vars.get(2)?.loc.span, &mut context.tokens)?, if conds.is_empty() { RcDoc::nil() } else { @@ -847,7 +847,7 @@ impl Doc for Node> { .append(cond_doc) .append(add_comment( RcDoc::text(";"), - get_comment_at_end(self.loc, &mut context.tokens)?, + get_comment_at_end(self.loc.span, &mut context.tokens)?, RcDoc::nil(), )), ) diff --git a/cedar-policy-validator/src/expr_iterator.rs b/cedar-policy-validator/src/expr_iterator.rs index 800935732..f07fc61a5 100644 --- a/cedar-policy-validator/src/expr_iterator.rs +++ b/cedar-policy-validator/src/expr_iterator.rs @@ -17,6 +17,7 @@ use cedar_policy_core::ast::{ EntityType, EntityUID, Expr, ExprKind, Literal, Name, PatternElem, Template, }; +use cedar_policy_core::parser::Loc; /// Returns an iterator over all literal entity uids in the expression. pub(super) fn expr_entity_uids(expr: &Expr) -> impl Iterator { @@ -76,11 +77,11 @@ pub(super) fn policy_entity_type_names(template: &Template) -> impl Iterator { /// String Literals - String(Option, &'a str), + String(Option<&'a Loc>, &'a str), /// Identifiers - Identifier(Option, &'a str), + Identifier(Option<&'a Loc>, &'a str), /// Pattern Strings - Pattern(Option, &'a [PatternElem]), + Pattern(Option<&'a Loc>, &'a [PatternElem]), } /// Returns an iterator over all text (strings and identifiers) in the expression. @@ -89,67 +90,64 @@ pub(super) fn expr_text(e: &'_ Expr) -> impl Iterator> { } // Returns a vector containing the text in the top level expression -fn text_in_expr(e: &'_ Expr) -> impl IntoIterator> { +fn text_in_expr<'a>(e: &'a Expr) -> impl IntoIterator> { match e.expr_kind() { - ExprKind::Lit(lit) => text_in_lit(e.source_span(), lit).into_iter().collect(), + ExprKind::Lit(lit) => text_in_lit(e.source_loc(), lit).into_iter().collect(), ExprKind::ExtensionFunctionApp { fn_name, .. } => { - text_in_name(e.source_span(), fn_name).collect() + text_in_name(e.source_loc(), fn_name).collect() } - ExprKind::GetAttr { attr, .. } => vec![TextKind::Identifier(e.source_span(), attr)], - ExprKind::HasAttr { attr, .. } => vec![TextKind::Identifier(e.source_span(), attr)], + ExprKind::GetAttr { attr, .. } => vec![TextKind::Identifier(e.source_loc(), attr)], + ExprKind::HasAttr { attr, .. } => vec![TextKind::Identifier(e.source_loc(), attr)], ExprKind::Like { pattern, .. } => { - vec![TextKind::Pattern(e.source_span(), pattern.get_elems())] + vec![TextKind::Pattern(e.source_loc(), pattern.get_elems())] } ExprKind::Record(map) => map .keys() - .map(|attr| TextKind::Identifier(e.source_span(), attr)) + .map(|attr| TextKind::Identifier(e.source_loc(), attr)) .collect(), _ => vec![], } } -fn text_in_lit( - span: Option, - lit: &Literal, -) -> impl IntoIterator> { +fn text_in_lit<'a>( + loc: Option<&'a Loc>, + lit: &'a Literal, +) -> impl IntoIterator> { match lit { Literal::Bool(_) => vec![], Literal::Long(_) => vec![], - Literal::String(s) => vec![TextKind::String(span, s)], - Literal::EntityUID(euid) => text_in_euid(span, euid).collect(), + Literal::String(s) => vec![TextKind::String(loc, s)], + Literal::EntityUID(euid) => text_in_euid(loc, euid).collect(), } } -fn text_in_euid( - span: Option, - euid: &EntityUID, -) -> impl Iterator> { - text_in_entity_type(span, euid.entity_type()) +fn text_in_euid<'a>( + loc: Option<&'a Loc>, + euid: &'a EntityUID, +) -> impl Iterator> { + text_in_entity_type(loc, euid.entity_type()) .into_iter() .chain(std::iter::once(TextKind::Identifier( - span, + loc, euid.eid().as_ref(), ))) } -fn text_in_entity_type( - span: Option, - ty: &EntityType, -) -> impl IntoIterator> { +fn text_in_entity_type<'a>( + loc: Option<&'a Loc>, + ty: &'a EntityType, +) -> impl IntoIterator> { match ty { - EntityType::Specified(ty) => text_in_name(span, ty).collect::>(), + EntityType::Specified(ty) => text_in_name(loc, ty).collect::>(), EntityType::Unspecified => vec![], } } -fn text_in_name( - span: Option, - name: &Name, -) -> impl Iterator> { +fn text_in_name<'a>(loc: Option<&'a Loc>, name: &'a Name) -> impl Iterator> { name.namespace_components() - .map(move |id| TextKind::Identifier(span, id.as_ref())) + .map(move |id| TextKind::Identifier(loc, id.as_ref())) .chain(std::iter::once(TextKind::Identifier( - span, + loc, name.basename().as_ref(), ))) } diff --git a/cedar-policy-validator/src/schema.rs b/cedar-policy-validator/src/schema.rs index 0c96e0a78..618bc708d 100644 --- a/cedar-policy-validator/src/schema.rs +++ b/cedar-policy-validator/src/schema.rs @@ -629,6 +629,7 @@ impl TryInto for NamespaceDefinitionWithActionAttributes { #[allow(clippy::indexing_slicing)] #[cfg(test)] mod test { + use std::sync::Arc; use std::{collections::BTreeMap, str::FromStr}; use crate::types::Type; @@ -636,6 +637,7 @@ mod test { use cedar_policy_core::ast::RestrictedExpr; use cedar_policy_core::parser::err::{ParseError, ToASTError, ToASTErrorKind}; + use cedar_policy_core::parser::Loc; use cool_asserts::assert_matches; use serde_json::json; @@ -1698,13 +1700,14 @@ mod test { .expect("constructing the fragment itself should succeed"); // should this fail in the future? let err = ValidatorSchema::try_from(fragment) .expect_err("should error due to invalid entity type name"); + let problem_field = "User // comment"; let expected_err = ParseError::ToAST(ToASTError::new( ToASTErrorKind::NonNormalizedString { kind: "Id", - src: "User // comment".to_string(), + src: problem_field.to_string(), normalized_src: "User".to_string(), }, - miette::SourceSpan::from(4..4), + Loc::new(4, Arc::from(problem_field)), )) .into(); @@ -1728,13 +1731,14 @@ mod test { .expect("constructing the fragment itself should succeed"); // should this fail in the future? let err = ValidatorSchema::try_from(fragment) .expect_err("should error due to invalid schema namespace"); + let problem_field = "ABC :: //comment \n XYZ "; let expected_err = ParseError::ToAST(ToASTError::new( ToASTErrorKind::NonNormalizedString { kind: "Name", - src: "ABC :: //comment \n XYZ ".to_string(), + src: problem_field.to_string(), normalized_src: "ABC::XYZ".to_string(), }, - miette::SourceSpan::from(3..3), + Loc::new(3, Arc::from(problem_field)), )) .into(); match err { diff --git a/cedar-policy-validator/src/str_checks.rs b/cedar-policy-validator/src/str_checks.rs index 4d1de8cad..3e1b87f3d 100644 --- a/cedar-policy-validator/src/str_checks.rs +++ b/cedar-policy-validator/src/str_checks.rs @@ -96,10 +96,10 @@ pub fn confusable_string_checks<'a>( } }; - if let Some(w) = warning { + if let Some(kind) = warning { warnings.push(ValidationWarning { - location: SourceLocation::new(policy.id(), loc), - kind: w, + location: SourceLocation::new(policy.id(), loc.cloned()), + kind, }) } } @@ -149,12 +149,12 @@ const BIDI_CHARS: [char; 9] = [ #[allow(clippy::indexing_slicing)] #[cfg(test)] mod test { - use super::*; use cedar_policy_core::{ ast::{PolicyID, PolicySet}, - parser::parse_policy, + parser::{parse_policy, Loc}, }; + use std::sync::Arc; #[test] fn strs() { @@ -252,7 +252,7 @@ mod test { location, &SourceLocation::new( &PolicyID::from_string("test"), - Some(miette::SourceSpan::from(64..94)) + Some(Loc::new(64..94, Arc::from(src))) ), ); } @@ -283,7 +283,7 @@ mod test { location, &SourceLocation::new( &PolicyID::from_string("test"), - Some(miette::SourceSpan::from(90..131)) + Some(Loc::new(90..131, Arc::from(src))) ) ); } diff --git a/cedar-policy-validator/src/type_error.rs b/cedar-policy-validator/src/type_error.rs index c97531bf7..f2818e57f 100644 --- a/cedar-policy-validator/src/type_error.rs +++ b/cedar-policy-validator/src/type_error.rs @@ -19,6 +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 crate::types::{EntityLUB, EntityRecordKind, RequestEnv}; @@ -35,31 +36,30 @@ use thiserror::Error; #[derive(Debug, Hash, PartialEq, Eq, Error)] #[error("{kind}")] pub struct TypeError { - // This struct has both `on_expr` and `source_location` because many tests + // This struct has both `on_expr` and `source_loc` because many tests // were written to check that an error was raised on a particular expression // rather than at a source location. This is redundant (particularly since // an `Expr` already has a source location embedded in it). // For greater efficiency, we could remove `on_expr` and rewrite the affected - // tests to only check for the correct `source_location`. + // tests to only check for the correct `source_loc`. pub(crate) on_expr: Option, - pub(crate) source_location: Option, + pub(crate) source_loc: Option, pub(crate) kind: TypeErrorKind, } -// custom impl of `Diagnostic`: source location is from .source_span(), +// custom impl of `Diagnostic`: source location and source code are from .source_loc(), // everything else forwarded to .kind impl Diagnostic for TypeError { fn labels(&self) -> Option + '_>> { - self.source_span().as_ref().map(|info| { - let label = miette::LabeledSpan::underline(*info); - let ret: Box> = - Box::new(std::iter::once(label)); - ret + self.source_loc().map(|loc| { + let label = miette::LabeledSpan::underline(loc.span); + Box::new(std::iter::once(label)) as Box> }) } fn source_code(&self) -> Option<&dyn miette::SourceCode> { - self.kind.source_code() + self.source_loc() + .map(|loc| &loc.src as &dyn miette::SourceCode) } fn code<'a>(&'a self) -> Option> { @@ -93,18 +93,18 @@ impl TypeError { self.kind } - /// Extract the source location (span) of this type error. - pub fn source_span(&self) -> Option { - match self.source_location { - Some(_) => self.source_location, - None => self.on_expr.as_ref().and_then(|e| e.source_span()), + /// Extract the source location of this type error. + pub fn source_loc(&self) -> Option<&Loc> { + match &self.source_loc { + Some(loc) => Some(loc), + None => self.on_expr.as_ref().and_then(|e| e.source_loc()), } } /// Deconstruct the type error into its kind and location. - pub fn kind_and_location(self) -> (TypeErrorKind, Option) { - let span = self.source_span(); - (self.kind, span) + pub fn kind_and_location(self) -> (TypeErrorKind, Option) { + let loc = self.source_loc().cloned(); + (self.kind, loc) } /// Construct a type error for when an unexpected type occurs in an expression. @@ -116,7 +116,7 @@ impl TypeError { ) -> Self { Self { on_expr: Some(on_expr), - source_location: None, + source_loc: None, kind: TypeErrorKind::UnexpectedType(UnexpectedType { expected: expected.into_iter().collect::>(), actual, @@ -130,7 +130,7 @@ impl TypeError { pub(crate) fn incompatible_types(on_expr: Expr, types: impl IntoIterator) -> Self { Self { on_expr: Some(on_expr), - source_location: None, + source_loc: None, kind: TypeErrorKind::IncompatibleTypes(IncompatibleTypes { types: types.into_iter().collect::>(), }), @@ -145,7 +145,7 @@ impl TypeError { ) -> Self { Self { on_expr: Some(on_expr), - source_location: None, + source_loc: None, kind: TypeErrorKind::UnsafeAttributeAccess(UnsafeAttributeAccess { attribute_access, suggestion, @@ -160,7 +160,7 @@ impl TypeError { ) -> Self { Self { on_expr: Some(on_expr), - source_location: None, + source_loc: None, kind: TypeErrorKind::UnsafeOptionalAttributeAccess(UnsafeOptionalAttributeAccess { attribute_access, }), @@ -170,7 +170,7 @@ impl TypeError { pub(crate) fn impossible_policy(on_expr: Expr) -> Self { Self { on_expr: Some(on_expr), - source_location: None, + source_loc: None, kind: TypeErrorKind::ImpossiblePolicy, } } @@ -178,7 +178,7 @@ impl TypeError { pub(crate) fn undefined_extension(on_expr: Expr, name: String) -> Self { Self { on_expr: Some(on_expr), - source_location: None, + source_loc: None, kind: TypeErrorKind::UndefinedFunction(UndefinedFunction { name }), } } @@ -186,7 +186,7 @@ impl TypeError { pub(crate) fn multiply_defined_extension(on_expr: Expr, name: String) -> Self { Self { on_expr: Some(on_expr), - source_location: None, + source_loc: None, kind: TypeErrorKind::MultiplyDefinedFunction(MultiplyDefinedFunction { name }), } } @@ -194,7 +194,7 @@ impl TypeError { pub(crate) fn wrong_number_args(on_expr: Expr, expected: usize, actual: usize) -> Self { Self { on_expr: Some(on_expr), - source_location: None, + source_loc: None, kind: TypeErrorKind::WrongNumberArguments(WrongNumberArguments { expected, actual }), } } @@ -202,7 +202,7 @@ impl TypeError { pub(crate) fn arg_validation_error(on_expr: Expr, msg: String) -> Self { Self { on_expr: Some(on_expr), - source_location: None, + source_loc: None, kind: TypeErrorKind::FunctionArgumentValidationError(FunctionArgumentValidationError { msg, }), @@ -212,7 +212,7 @@ impl TypeError { pub(crate) fn empty_set_forbidden(on_expr: Expr) -> Self { Self { on_expr: None, - source_location: on_expr.source_span(), + source_loc: on_expr.source_loc().cloned(), kind: TypeErrorKind::EmptySetForbidden, } } @@ -220,7 +220,7 @@ impl TypeError { pub(crate) fn non_lit_ext_constructor(on_expr: Expr) -> Self { Self { on_expr: None, - source_location: on_expr.source_span(), + source_loc: on_expr.source_loc().cloned(), kind: TypeErrorKind::NonLitExtConstructor, } } @@ -232,7 +232,7 @@ impl TypeError { ) -> Self { Self { on_expr: None, - source_location: on_expr.source_span(), + source_loc: on_expr.source_loc().cloned(), kind: TypeErrorKind::HierarchyNotRespected(HierarchyNotRespected { in_lhs, in_rhs }), } } @@ -240,7 +240,7 @@ impl TypeError { /// Represents the different kinds of type errors and contains information /// specific to that type error kind. -#[derive(Debug, Diagnostic, Error, Hash, Eq, PartialEq)] +#[derive(Debug, Clone, Diagnostic, Error, Hash, Eq, PartialEq)] #[non_exhaustive] pub enum TypeErrorKind { /// The typechecker expected to see a subtype of one of the types in @@ -295,7 +295,7 @@ pub enum TypeErrorKind { } /// Structure containing details about an unexpected type error. -#[derive(Diagnostic, Error, Debug, Hash, Eq, PartialEq)] +#[derive(Diagnostic, Error, Debug, Clone, Hash, Eq, PartialEq)] #[error("unexpected type: expected {} but saw {}", match .expected.iter().next() { Some(single) if .expected.len() == 1 => format!("{}", single), @@ -310,7 +310,7 @@ pub struct UnexpectedType { help: Option, } -#[derive(Error, Debug, Hash, Eq, PartialEq)] +#[derive(Error, Debug, Clone, Hash, Eq, PartialEq)] pub(crate) enum UnexpectedTypeHelp { #[error("try using `like` to examine the contents of a string")] TryUsingLike, @@ -335,13 +335,13 @@ pub(crate) enum UnexpectedTypeHelp { } /// Structure containing details about an incompatible type error. -#[derive(Debug, Hash, Eq, PartialEq)] +#[derive(Debug, Clone, Hash, Eq, PartialEq)] pub struct IncompatibleTypes { pub(crate) types: BTreeSet, } /// Structure containing details about a missing attribute error. -#[derive(Debug, Hash, Eq, PartialEq, Error)] +#[derive(Debug, Clone, Hash, Eq, PartialEq, Error)] #[error("attribute {attribute_access} not found")] pub struct UnsafeAttributeAccess { attribute_access: AttributeAccess, @@ -363,7 +363,7 @@ impl Diagnostic for UnsafeAttributeAccess { } /// Structure containing details about an unsafe optional attribute error. -#[derive(Error, Diagnostic, Debug, Hash, Eq, PartialEq)] +#[derive(Error, Diagnostic, Debug, Clone, Hash, Eq, PartialEq)] #[error("unable to guarantee safety of access to optional attribute {attribute_access}")] #[diagnostic(help("try testing for the attribute with `{} && ..`", attribute_access.suggested_has_guard()))] pub struct UnsafeOptionalAttributeAccess { @@ -371,40 +371,40 @@ pub struct UnsafeOptionalAttributeAccess { } /// Structure containing details about an undefined function error. -#[derive(Debug, Hash, Eq, PartialEq)] +#[derive(Debug, Clone, Hash, Eq, PartialEq)] pub struct UndefinedFunction { name: String, } /// Structure containing details about a multiply defined function error. -#[derive(Debug, Hash, Eq, PartialEq)] +#[derive(Debug, Clone, Hash, Eq, PartialEq)] pub struct MultiplyDefinedFunction { name: String, } /// Structure containing details about a wrong number of arguments error. -#[derive(Debug, Hash, Eq, PartialEq)] +#[derive(Debug, Clone, Hash, Eq, PartialEq)] pub struct WrongNumberArguments { expected: usize, actual: usize, } /// Structure containing details about a wrong call style error. -#[derive(Debug, Hash, Eq, PartialEq)] +#[derive(Debug, Clone, Hash, Eq, PartialEq)] pub struct WrongCallStyle { expected: CallStyle, actual: CallStyle, } /// Structure containing details about a function argument validation error. -#[derive(Debug, Hash, Eq, PartialEq, Diagnostic, Error)] +#[derive(Debug, Clone, Hash, Eq, PartialEq, Diagnostic, Error)] #[error("{msg}")] pub struct FunctionArgumentValidationError { msg: String, } /// Structure containing details about a hierarchy not respected error -#[derive(Debug, Hash, Eq, PartialEq, Error)] +#[derive(Debug, Clone, Hash, Eq, PartialEq, Error)] #[error("operands to `in` do not respect the entity hierarchy")] pub struct HierarchyNotRespected { in_lhs: Option, @@ -428,7 +428,7 @@ impl Diagnostic for HierarchyNotRespected { /// report that the record attribute `foo` of an entity type (e.g., `User`) /// needs attributes `bar` instead of giving up when the immediate target of the /// attribute access is not a entity. -#[derive(Debug, Hash, Eq, PartialEq)] +#[derive(Debug, Clone, Hash, Eq, PartialEq)] pub(crate) enum AttributeAccess { /// The attribute access is some sequence of attributes accesses eventually /// targeting an EntityLUB. diff --git a/cedar-policy-validator/src/typecheck.rs b/cedar-policy-validator/src/typecheck.rs index 39c707726..526ee365e 100644 --- a/cedar-policy-validator/src/typecheck.rs +++ b/cedar-policy-validator/src/typecheck.rs @@ -547,7 +547,7 @@ impl<'a> Typechecker<'a> { // the request type. ExprKind::Var(Var::Principal) => TypecheckAnswer::success( ExprBuilder::with_data(Some(request_env.principal_type())) - .with_same_source_span(e) + .with_same_source_loc(e) .var(Var::Principal), ), // While the EntityUID for Action is held in the request context, @@ -558,7 +558,7 @@ impl<'a> Typechecker<'a> { match request_env.action_type(self.schema) { Some(ty) => TypecheckAnswer::success( ExprBuilder::with_data(Some(ty)) - .with_same_source_span(e) + .with_same_source_loc(e) .var(Var::Action), ), // `None` if the action entity is not defined in the schema. @@ -568,18 +568,18 @@ impl<'a> Typechecker<'a> { // public entry points, but it can occur if calling // `typecheck` directly which happens in our tests. None => TypecheckAnswer::fail( - ExprBuilder::new().with_same_source_span(e).var(Var::Action), + ExprBuilder::new().with_same_source_loc(e).var(Var::Action), ), } } ExprKind::Var(Var::Resource) => TypecheckAnswer::success( ExprBuilder::with_data(Some(request_env.resource_type())) - .with_same_source_span(e) + .with_same_source_loc(e) .var(Var::Resource), ), ExprKind::Var(Var::Context) => TypecheckAnswer::success( ExprBuilder::with_data(Some(request_env.context_type())) - .with_same_source_span(e) + .with_same_source_loc(e) .var(Var::Context), ), ExprKind::Unknown(u) => { @@ -602,25 +602,25 @@ impl<'a> Typechecker<'a> { } else { Type::any_entity_reference() })) - .with_same_source_span(e) + .with_same_source_loc(e) .slot(*slotid), ), // Literal booleans get singleton type according to their value. ExprKind::Lit(Literal::Bool(val)) => TypecheckAnswer::success( ExprBuilder::with_data(Some(Type::singleton_boolean(*val))) - .with_same_source_span(e) + .with_same_source_loc(e) .val(*val), ), // Other literal primitive values have the type of that primitive value. ExprKind::Lit(Literal::Long(val)) => TypecheckAnswer::success( ExprBuilder::with_data(Some(Type::primitive_long())) - .with_same_source_span(e) + .with_same_source_loc(e) .val(*val), ), ExprKind::Lit(Literal::String(val)) => TypecheckAnswer::success( ExprBuilder::with_data(Some(Type::primitive_string())) - .with_same_source_span(e) + .with_same_source_loc(e) .val(val.clone()), ), @@ -640,18 +640,16 @@ impl<'a> Typechecker<'a> { ExprBuilder::with_data(Some(Type::possibly_unspecified_entity_reference( euid.entity_type().clone(), ))) - .with_same_source_span(e) + .with_same_source_loc(e) .val(euid.clone()), ), Some(ty) => TypecheckAnswer::success( ExprBuilder::with_data(Some(ty)) - .with_same_source_span(e) + .with_same_source_loc(e) .val(euid.clone()), ), None => TypecheckAnswer::fail( - ExprBuilder::new() - .with_same_source_span(e) - .val(euid.clone()), + ExprBuilder::new().with_same_source_loc(e).val(euid.clone()), ), } } @@ -734,7 +732,7 @@ impl<'a> Typechecker<'a> { ); let has_lub = lub_ty.is_some(); let annot_expr = ExprBuilder::with_data(lub_ty) - .with_same_source_span(e) + .with_same_source_loc(e) .ite(typ_test, typ_then, typ_else); if has_lub { // Effect is not handled in the LUB computation, @@ -803,7 +801,7 @@ impl<'a> Typechecker<'a> { // was false. (Some(_), Some(Type::False)) => TypecheckAnswer::success( ExprBuilder::with_data(Some(Type::False)) - .with_same_source_span(e) + .with_same_source_loc(e) .and(typ_left, typ_right), ), @@ -816,7 +814,7 @@ impl<'a> Typechecker<'a> { (Some(_), Some(Type::True)) => { TypecheckAnswer::success_with_effect( ExprBuilder::with_data(typ_left.data().clone()) - .with_same_source_span(e) + .with_same_source_loc(e) .and(typ_left, typ_right), eff_left.union(&eff_right), ) @@ -824,7 +822,7 @@ impl<'a> Typechecker<'a> { (Some(Type::True), Some(_)) => { TypecheckAnswer::success_with_effect( ExprBuilder::with_data(typ_right.data().clone()) - .with_same_source_span(e) + .with_same_source_loc(e) .and(typ_left, typ_right), eff_right.union(&eff_right), ) @@ -834,7 +832,7 @@ impl<'a> Typechecker<'a> { // know the result type is boolean. (Some(_), Some(_)) => TypecheckAnswer::success_with_effect( ExprBuilder::with_data(Some(Type::primitive_boolean())) - .with_same_source_span(e) + .with_same_source_loc(e) .and(typ_left, typ_right), eff_left.union(&eff_right), ), @@ -843,7 +841,7 @@ impl<'a> Typechecker<'a> { // typecheck, so the `&&` expression also fails. _ => TypecheckAnswer::fail( ExprBuilder::with_data(Some(Type::primitive_boolean())) - .with_same_source_span(e) + .with_same_source_loc(e) .and(typ_left, typ_right), ), } @@ -896,7 +894,7 @@ impl<'a> Typechecker<'a> { (Some(_), Some(Type::True)) => { TypecheckAnswer::success_with_effect( ExprBuilder::with_data(Some(Type::True)) - .with_same_source_span(e) + .with_same_source_loc(e) .or(ty_expr_left, ty_expr_right), eff_right, ) @@ -909,7 +907,7 @@ impl<'a> Typechecker<'a> { (Some(typ_left), Some(Type::False)) => { TypecheckAnswer::success_with_effect( ExprBuilder::with_data(Some(typ_left.clone())) - .with_same_source_span(e) + .with_same_source_loc(e) .or(ty_expr_left, ty_expr_right), eff_left, ) @@ -917,7 +915,7 @@ impl<'a> Typechecker<'a> { (Some(Type::False), Some(typ_right)) => { TypecheckAnswer::success_with_effect( ExprBuilder::with_data(Some(typ_right.clone())) - .with_same_source_span(e) + .with_same_source_loc(e) .or(ty_expr_left, ty_expr_right), eff_right, ) @@ -928,13 +926,13 @@ impl<'a> Typechecker<'a> { // intersection of their effect sets. (Some(_), Some(_)) => TypecheckAnswer::success_with_effect( ExprBuilder::with_data(Some(Type::primitive_boolean())) - .with_same_source_span(e) + .with_same_source_loc(e) .or(ty_expr_left, ty_expr_right), eff_right.intersect(&eff_left), ), _ => TypecheckAnswer::fail( ExprBuilder::with_data(Some(Type::primitive_boolean())) - .with_same_source_span(e) + .with_same_source_loc(e) .or(ty_expr_left, ty_expr_right), ), } @@ -979,7 +977,7 @@ impl<'a> Typechecker<'a> { let annot_expr = ExprBuilder::with_data( attr_ty.clone().map(|attr_ty| attr_ty.attr_type), ) - .with_same_source_span(e) + .with_same_source_loc(e) .get_attr(typ_expr_actual.clone(), attr.clone()); match attr_ty { Some(ty) => { @@ -1009,7 +1007,7 @@ impl<'a> Typechecker<'a> { { TypecheckAnswer::success( ExprBuilder::with_data(Some(Type::Never)) - .with_same_source_span(e) + .with_same_source_loc(e) .get_attr(typ_expr_actual, attr.clone()), ) } @@ -1029,7 +1027,7 @@ impl<'a> Typechecker<'a> { } None => TypecheckAnswer::fail( ExprBuilder::new() - .with_same_source_span(e) + .with_same_source_loc(e) .get_attr(typ_expr_actual, attr.clone()), ), }) @@ -1078,7 +1076,7 @@ impl<'a> Typechecker<'a> { }; TypecheckAnswer::success_with_effect( ExprBuilder::with_data(Some(type_of_has)) - .with_same_source_span(e) + .with_same_source_loc(e) .has_attr(typ_expr_actual, attr.clone()), EffectSet::singleton(Effect::new(expr, attr)), ) @@ -1103,7 +1101,7 @@ impl<'a> Typechecker<'a> { Type::primitive_boolean() }, )) - .with_same_source_span(e) + .with_same_source_loc(e) .has_attr(typ_expr_actual, attr.clone()), EffectSet::singleton(Effect::new(expr, attr)), ), @@ -1125,14 +1123,14 @@ impl<'a> Typechecker<'a> { Type::singleton_boolean(false) }, )) - .with_same_source_span(e) + .with_same_source_loc(e) .has_attr(typ_expr_actual, attr.clone()), ), } } None => TypecheckAnswer::fail( ExprBuilder::with_data(Some(Type::primitive_boolean())) - .with_same_source_span(e) + .with_same_source_loc(e) .has_attr(typ_expr_actual, attr.clone()), ), }) @@ -1158,7 +1156,7 @@ impl<'a> Typechecker<'a> { actual.then_typecheck(|actual_expr_ty, _| { TypecheckAnswer::success( ExprBuilder::with_data(Some(Type::primitive_boolean())) - .with_same_source_span(e) + .with_same_source_loc(e) // FIXME: `pattern` contains an `Arc>` that // could be cloned cheap, but this reallocated the // pattern vec. Need a different constructor. @@ -1195,7 +1193,7 @@ impl<'a> Typechecker<'a> { TypecheckAnswer::success( ExprBuilder::with_data(Some(type_of_is)) - .with_same_source_span(e) + .with_same_source_loc(e) .is_entity_type(expr_ty, entity_type.clone()), ) } @@ -1214,7 +1212,7 @@ impl<'a> Typechecker<'a> { TypecheckAnswer::success( ExprBuilder::with_data(Some(type_of_is)) - .with_same_source_span(e) + .with_same_source_loc(e) .is_entity_type(expr_ty, entity_type.clone()), ) } @@ -1223,7 +1221,7 @@ impl<'a> Typechecker<'a> { Some(Type::EntityOrRecord(EntityRecordKind::AnyEntity { .. })) => { TypecheckAnswer::success( ExprBuilder::with_data(Some(Type::primitive_boolean())) - .with_same_source_span(e) + .with_same_source_loc(e) .is_entity_type(expr_ty, entity_type.clone()), ) } @@ -1231,7 +1229,7 @@ impl<'a> Typechecker<'a> { // In either case a type error was already reported. _ => TypecheckAnswer::fail( ExprBuilder::with_data(Some(Type::primitive_boolean())) - .with_same_source_span(e) + .with_same_source_loc(e) .is_entity_type(expr_ty, entity_type.clone()), ), } @@ -1264,18 +1262,18 @@ impl<'a> Typechecker<'a> { type_errors.push(TypeError::empty_set_forbidden(e.clone())); TypecheckAnswer::fail( ExprBuilder::new() - .with_same_source_span(e) + .with_same_source_loc(e) .set(elem_expr_types), ) } Some(elem_lub) => TypecheckAnswer::success( ExprBuilder::with_data(Some(Type::set(elem_lub))) - .with_same_source_span(e) + .with_same_source_loc(e) .set(elem_expr_types), ), None => TypecheckAnswer::fail( ExprBuilder::new() - .with_same_source_span(e) + .with_same_source_loc(e) .set(elem_expr_types), ), } @@ -1319,7 +1317,7 @@ impl<'a> Typechecker<'a> { // PANIC SAFETY: can't have duplicate keys because the keys are the same as those in `map` which was already a BTreeMap #[allow(clippy::expect_used)] let expr = ExprBuilder::with_data(ty) - .with_same_source_span(e) + .with_same_source_loc(e) .record(map.keys().cloned().zip(record_attr_expr_tys)) .expect("this can't have duplicate keys because the keys are the same as those in `map` which was already a BTreeMap"); if is_success { @@ -1367,7 +1365,7 @@ impl<'a> Typechecker<'a> { if self.mode.is_strict() { let annotated_eq = ExprBuilder::with_data(Some(type_of_eq)) - .with_same_source_span(bin_expr) + .with_same_source_loc(bin_expr) .binary_app(*op, lhs_ty.clone(), rhs_ty.clone()); self.enforce_strict_equality( bin_expr, @@ -1379,7 +1377,7 @@ impl<'a> Typechecker<'a> { } else { TypecheckAnswer::success( ExprBuilder::with_data(Some(type_of_eq)) - .with_same_source_span(bin_expr) + .with_same_source_loc(bin_expr) .binary_app(*op, lhs_ty, rhs_ty), ) } @@ -1408,7 +1406,7 @@ impl<'a> Typechecker<'a> { ans_arg2.then_typecheck(|expr_ty_arg2, _| { TypecheckAnswer::success( ExprBuilder::with_data(Some(Type::primitive_boolean())) - .with_same_source_span(bin_expr) + .with_same_source_loc(bin_expr) .binary_app(*op, expr_ty_arg1, expr_ty_arg2), ) }) @@ -1446,7 +1444,7 @@ impl<'a> Typechecker<'a> { ans_arg2.then_typecheck(|expr_ty_arg2, _| { TypecheckAnswer::success( ExprBuilder::with_data(Some(Type::primitive_long())) - .with_same_source_span(bin_expr) + .with_same_source_loc(bin_expr) .binary_app(*op, expr_ty_arg1, expr_ty_arg2), ) }) @@ -1487,7 +1485,7 @@ impl<'a> Typechecker<'a> { if self.mode.is_strict() { let annotated_expr = ExprBuilder::with_data(Some(Type::primitive_boolean())) - .with_same_source_span(bin_expr) + .with_same_source_loc(bin_expr) .binary_app( *op, expr_ty_arg1.clone(), @@ -1508,7 +1506,7 @@ impl<'a> Typechecker<'a> { } else { TypecheckAnswer::success( ExprBuilder::with_data(Some(Type::primitive_boolean())) - .with_same_source_span(bin_expr) + .with_same_source_loc(bin_expr) .binary_app(*op, expr_ty_arg1, expr_ty_arg2), ) } @@ -1552,7 +1550,7 @@ impl<'a> Typechecker<'a> { if self.mode.is_strict() { let annotated_expr = ExprBuilder::with_data(Some(Type::primitive_boolean())) - .with_same_source_span(bin_expr) + .with_same_source_loc(bin_expr) .binary_app(*op, expr_ty_arg1.clone(), expr_ty_arg2.clone()); self.enforce_strict_equality( bin_expr, @@ -1564,7 +1562,7 @@ impl<'a> Typechecker<'a> { } else { TypecheckAnswer::success( ExprBuilder::with_data(Some(Type::primitive_boolean())) - .with_same_source_span(bin_expr) + .with_same_source_loc(bin_expr) .binary_app(*op, expr_ty_arg1, expr_ty_arg2), ) } @@ -1636,7 +1634,7 @@ impl<'a> Typechecker<'a> { ans_arg.then_typecheck(|arg_expr_ty, _| { TypecheckAnswer::success({ ExprBuilder::with_data(Some(Type::primitive_long())) - .with_same_source_span(mul_expr) + .with_same_source_loc(mul_expr) .mul(arg_expr_ty, *constant) }) }) @@ -1759,13 +1757,13 @@ impl<'a> Typechecker<'a> { if !lhs_typechecked || !rhs_typechecked { TypecheckAnswer::fail( ExprBuilder::with_data(Some(Type::primitive_boolean())) - .with_same_source_span(in_expr) + .with_same_source_loc(in_expr) .is_in(lhs_expr, rhs_expr), ) } else if left_is_unspecified && right_is_specified { TypecheckAnswer::success( ExprBuilder::with_data(Some(Type::singleton_boolean(false))) - .with_same_source_span(in_expr) + .with_same_source_loc(in_expr) .is_in(lhs_expr, rhs_expr), ) } else { @@ -1842,7 +1840,7 @@ impl<'a> Typechecker<'a> { // in the schema. _ => TypecheckAnswer::success( ExprBuilder::with_data(Some(Type::primitive_boolean())) - .with_same_source_span(in_expr) + .with_same_source_loc(in_expr) .is_in(lhs_expr, rhs_expr), ), } @@ -1985,7 +1983,7 @@ impl<'a> Typechecker<'a> { // in the descendants or not, so give it type boolean. None => { let in_expr = ExprBuilder::with_data(Some(Type::primitive_boolean())) - .with_same_source_span(in_expr) + .with_same_source_loc(in_expr) .is_in(lhs_expr, rhs_expr); if self.mode.is_partial() { TypecheckAnswer::success(in_expr) @@ -2012,7 +2010,7 @@ impl<'a> Typechecker<'a> { } else { let annotated_expr = ExprBuilder::with_data(Some(Type::primitive_boolean())) - .with_same_source_span(in_expr) + .with_same_source_loc(in_expr) .is_in(lhs_expr, rhs_expr); if self.mode.is_partial() { // In partial schema mode, undeclared entity types are @@ -2027,7 +2025,7 @@ impl<'a> Typechecker<'a> { // Still return `TypecheckFail` so that typechecking is not considered successful. Some(EntityType::Unspecified) => TypecheckAnswer::fail( ExprBuilder::with_data(Some(Type::primitive_boolean())) - .with_same_source_span(in_expr) + .with_same_source_loc(in_expr) .is_in(lhs_expr, rhs_expr), ), } @@ -2037,7 +2035,7 @@ impl<'a> Typechecker<'a> { // typechecking succeeds with type Boolean. TypecheckAnswer::success( ExprBuilder::with_data(Some(Type::primitive_boolean())) - .with_same_source_span(in_expr) + .with_same_source_loc(in_expr) .is_in(lhs_expr, rhs_expr), ) } @@ -2089,7 +2087,7 @@ impl<'a> Typechecker<'a> { // and by extension any particular action entity. TypecheckAnswer::success( ExprBuilder::with_data(Some(Type::False)) - .with_same_source_span(in_expr) + .with_same_source_loc(in_expr) .is_in(lhs_expr, rhs_expr), ) } @@ -2098,7 +2096,7 @@ impl<'a> Typechecker<'a> { // Still return `TypecheckFail` so that typechecking is not considered successful. EntityType::Unspecified => TypecheckAnswer::fail( ExprBuilder::with_data(Some(Type::primitive_boolean())) - .with_same_source_span(in_expr) + .with_same_source_loc(in_expr) .is_in(lhs_expr, rhs_expr), ), } @@ -2108,7 +2106,7 @@ impl<'a> Typechecker<'a> { // typechecking succeeds with type Boolean. TypecheckAnswer::success( ExprBuilder::with_data(Some(Type::primitive_boolean())) - .with_same_source_span(in_expr) + .with_same_source_loc(in_expr) .is_in(lhs_expr, rhs_expr), ) } @@ -2131,7 +2129,7 @@ impl<'a> Typechecker<'a> { Typechecker::entity_in_descendants(lhs, rhs_descendants, in_expr, lhs_expr, rhs_expr) } else { let annotated_expr = ExprBuilder::with_data(Some(Type::primitive_boolean())) - .with_same_source_span(in_expr) + .with_same_source_loc(in_expr) .is_in(lhs_expr, rhs_expr); if self.mode.is_partial() { TypecheckAnswer::success(annotated_expr) @@ -2170,7 +2168,7 @@ impl<'a> Typechecker<'a> { ) } else { let annotated_expr = ExprBuilder::with_data(Some(Type::primitive_boolean())) - .with_same_source_span(in_expr) + .with_same_source_loc(in_expr) .is_in(lhs_expr, rhs_expr); if self.mode.is_partial() { TypecheckAnswer::success(annotated_expr) @@ -2184,7 +2182,7 @@ impl<'a> Typechecker<'a> { // Still return `TypecheckFail` so that typechecking is not considered successful. TypecheckAnswer::fail( ExprBuilder::with_data(Some(Type::primitive_boolean())) - .with_same_source_span(in_expr) + .with_same_source_loc(in_expr) .is_in(lhs_expr, rhs_expr), ) } @@ -2210,7 +2208,7 @@ impl<'a> Typechecker<'a> { } else { Type::singleton_boolean(false) })) - .with_same_source_span(in_expr) + .with_same_source_loc(in_expr) .is_in(lhs_expr, rhs_expr), ) } @@ -2244,21 +2242,21 @@ impl<'a> Typechecker<'a> { Some(typ_arg) => { TypecheckAnswer::success(if typ_arg == &Type::singleton_boolean(true) { ExprBuilder::with_data(Some(Type::singleton_boolean(false))) - .with_same_source_span(unary_expr) + .with_same_source_loc(unary_expr) .not(typ_expr_arg) } else if typ_arg == &Type::singleton_boolean(false) { ExprBuilder::with_data(Some(Type::singleton_boolean(true))) - .with_same_source_span(unary_expr) + .with_same_source_loc(unary_expr) .not(typ_expr_arg) } else { ExprBuilder::with_data(Some(Type::primitive_boolean())) - .with_same_source_span(unary_expr) + .with_same_source_loc(unary_expr) .not(typ_expr_arg) }) } None => TypecheckAnswer::fail( ExprBuilder::with_data(Some(Type::primitive_boolean())) - .with_same_source_span(unary_expr) + .with_same_source_loc(unary_expr) .not(typ_expr_arg), ), }) @@ -2275,7 +2273,7 @@ impl<'a> Typechecker<'a> { ans_arg.then_typecheck(|typ_expr_arg, _| { TypecheckAnswer::success( ExprBuilder::with_data(Some(Type::primitive_long())) - .with_same_source_span(unary_expr) + .with_same_source_loc(unary_expr) .neg(typ_expr_arg), ) }) @@ -2502,7 +2500,7 @@ impl<'a> Typechecker<'a> { match typed_arg_exprs(type_errors) { Some(exprs) => TypecheckAnswer::fail( ExprBuilder::with_data(Some(ret_ty.clone())) - .with_same_source_span(ext_expr) + .with_same_source_loc(ext_expr) .call_extension_fn(fn_name.clone(), exprs), ), None => TypecheckAnswer::RecursionLimit, @@ -2525,7 +2523,7 @@ impl<'a> Typechecker<'a> { arg_exprs_effects.into_iter().unzip(); TypecheckAnswer::success( ExprBuilder::with_data(Some(ret_ty.clone())) - .with_same_source_span(ext_expr) + .with_same_source_loc(ext_expr) .call_extension_fn(fn_name.clone(), typed_arg_exprs), ) }, @@ -2537,7 +2535,7 @@ impl<'a> Typechecker<'a> { match typed_arg_exprs(type_errors) { Some(typed_args) => TypecheckAnswer::fail( ExprBuilder::with_data(None) - .with_same_source_span(ext_expr) + .with_same_source_loc(ext_expr) .call_extension_fn(fn_name.clone(), typed_args), ), None => TypecheckAnswer::RecursionLimit, diff --git a/cedar-policy-validator/src/validation_result.rs b/cedar-policy-validator/src/validation_result.rs index 129d924d7..3a2182fd0 100644 --- a/cedar-policy-validator/src/validation_result.rs +++ b/cedar-policy-validator/src/validation_result.rs @@ -15,6 +15,7 @@ */ use cedar_policy_core::ast::PolicyID; +use cedar_policy_core::parser::Loc; use miette::Diagnostic; use thiserror::Error; @@ -85,12 +86,12 @@ pub struct ValidationError<'a> { impl<'a> ValidationError<'a> { pub(crate) fn with_policy_id( id: &'a PolicyID, - source_span: Option, + source_loc: Option, error_kind: ValidationErrorKind, ) -> Self { Self { error_kind, - location: SourceLocation::new(id, source_span), + location: SourceLocation::new(id, source_loc), } } @@ -114,14 +115,14 @@ impl<'a> ValidationError<'a> { #[derive(Debug, Clone, Eq, PartialEq)] pub struct SourceLocation<'a> { policy_id: &'a PolicyID, - source_span: Option, + source_loc: Option, } impl<'a> SourceLocation<'a> { - pub(crate) fn new(policy_id: &'a PolicyID, source_span: Option) -> Self { + pub(crate) fn new(policy_id: &'a PolicyID, source_loc: Option) -> Self { Self { policy_id, - source_span, + source_loc, } } @@ -130,14 +131,14 @@ impl<'a> SourceLocation<'a> { self.policy_id } - pub fn source_span(&self) -> Option { - self.source_span + pub fn source_loc(&self) -> Option<&Loc> { + self.source_loc.as_ref() } } /// Enumeration of the possible diagnostic error that could be found by the /// verification steps. -#[derive(Debug, Diagnostic, Error)] +#[derive(Debug, Clone, Diagnostic, Error)] #[cfg_attr(test, derive(Eq, PartialEq))] #[non_exhaustive] pub enum ValidationErrorKind { @@ -210,7 +211,7 @@ impl ValidationErrorKind { } /// Structure containing details about an unrecognized entity type error. -#[derive(Debug, Error)] +#[derive(Debug, Clone, Error)] #[cfg_attr(test, derive(Eq, PartialEq))] #[error("unrecognized entity type `{actual_entity_type}`")] pub struct UnrecognizedEntityType { @@ -231,7 +232,7 @@ impl Diagnostic for UnrecognizedEntityType { } /// Structure containing details about an unrecognized action id error. -#[derive(Debug, Error)] +#[derive(Debug, Clone, Error)] #[cfg_attr(test, derive(Eq, PartialEq))] #[error("unrecognized action `{actual_action_id}`")] pub struct UnrecognizedActionId { @@ -252,7 +253,7 @@ impl Diagnostic for UnrecognizedActionId { } /// Structure containing details about an invalid action application error. -#[derive(Debug, Error)] +#[derive(Debug, Clone, Error)] #[cfg_attr(test, derive(Eq, PartialEq))] #[error("unable to find an applicable action given the policy head constraints")] pub struct InvalidActionApplication { @@ -278,7 +279,7 @@ impl Diagnostic for InvalidActionApplication { } /// Structure containing details about an unspecified entity error. -#[derive(Debug, Diagnostic, Error)] +#[derive(Debug, Clone, Diagnostic, Error)] #[cfg_attr(test, derive(Eq, PartialEq))] #[error("unspecified entity with id `{entity_id}`")] #[diagnostic(help("unspecified entities cannot be used in policies"))] diff --git a/cedar-policy/src/api.rs b/cedar-policy/src/api.rs index 7e49d45d0..bd9842c46 100644 --- a/cedar-policy/src/api.rs +++ b/cedar-policy/src/api.rs @@ -1503,7 +1503,7 @@ impl<'a> Diagnostic for ValidationResult<'a> { /// policy. The error contains a enumeration that specifies the kind of problem, /// and provides details specific to that kind of problem. The error also records /// where the problem was encountered. -#[derive(Debug, Error)] +#[derive(Debug, Clone, Error)] #[error("validation error on {location}: {}", self.error_kind())] pub struct ValidationError<'a> { location: SourceLocation<'static>, @@ -1535,16 +1535,16 @@ impl<'a> From> for ValidationError<' } } -// custom impl of `Diagnostic`: source location is from .location, everything else -// forwarded to .error_kind +// custom impl of `Diagnostic`: source location and source code are from +// .location, everything else forwarded to .error_kind impl<'a> Diagnostic for ValidationError<'a> { fn labels(&self) -> Option + '_>> { - let label = miette::LabeledSpan::underline(self.location.source_range?); + let label = miette::LabeledSpan::underline(self.location.source_loc.as_ref()?.span); Some(Box::new(std::iter::once(label))) } fn source_code(&self) -> Option<&dyn miette::SourceCode> { - self.error_kind.source_code() + Some(&self.location.source_loc.as_ref()?.src) } fn code<'s>(&'s self) -> Option> { @@ -1576,7 +1576,7 @@ impl<'a> Diagnostic for ValidationError<'a> { #[derive(Debug, Clone, Eq, PartialEq)] pub struct SourceLocation<'a> { policy_id: PolicyId, - source_range: Option, + source_loc: Option, phantom: PhantomData<&'a ()>, } @@ -1589,28 +1589,21 @@ impl<'a> SourceLocation<'a> { /// Get the start of the location. Returns `None` if this location does not /// have a range. pub fn range_start(&self) -> Option { - self.source_range.as_ref().map(miette::SourceSpan::offset) + self.source_loc.as_ref().map(parser::Loc::start) } /// Get the end of the location. Returns `None` if this location does not /// have a range. pub fn range_end(&self) -> Option { - self.source_range - .as_ref() - .map(|span| span.offset() + span.len()) + self.source_loc.as_ref().map(parser::Loc::end) } } impl<'a> std::fmt::Display for SourceLocation<'a> { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { write!(f, "policy `{}`", self.policy_id)?; - if let Some(source_range) = &self.source_range { - write!( - f, - " at offset {}-{}", - source_range.offset(), - source_range.offset() + source_range.len() - )?; + if let Some(loc) = &self.source_loc { + write!(f, " at offset {}-{}", loc.start(), loc.end())?; } Ok(()) } @@ -1619,10 +1612,10 @@ impl<'a> std::fmt::Display for SourceLocation<'a> { impl<'a> From> for SourceLocation<'static> { fn from(loc: cedar_policy_validator::SourceLocation<'a>) -> SourceLocation<'static> { let policy_id = PolicyId(loc.policy_id().clone()); - let source_range = loc.source_span(); + let source_loc = loc.source_loc().cloned(); Self { policy_id, - source_range, + source_loc, phantom: PhantomData, } } @@ -1642,7 +1635,7 @@ where .map(std::convert::Into::into) } -#[derive(Debug, Error)] +#[derive(Debug, Clone, Error)] #[error("validation warning on {location}: {kind}")] /// Warnings found in Cedar policies pub struct ValidationWarning<'a> { @@ -1675,16 +1668,16 @@ impl<'a> From> for ValidationWarni } } -// custom impl of `Diagnostic`: source location is from .location, everything else -// forwarded to .kind +// custom impl of `Diagnostic`: source location and source code are from +// .location, everything else forwarded to .kind impl<'a> Diagnostic for ValidationWarning<'a> { fn labels(&self) -> Option + '_>> { - let label = miette::LabeledSpan::underline(self.location.source_range?); + let label = miette::LabeledSpan::underline(self.location.source_loc.as_ref()?.span); Some(Box::new(std::iter::once(label))) } fn source_code(&self) -> Option<&dyn miette::SourceCode> { - self.kind.source_code() + Some(&self.location.source_loc.as_ref()?.src) } fn code<'s>(&'s self) -> Option> { diff --git a/cedar-policy/src/tests.rs b/cedar-policy/src/tests.rs index 7318d11df..c64ad11dd 100644 --- a/cedar-policy/src/tests.rs +++ b/cedar-policy/src/tests.rs @@ -3055,3 +3055,85 @@ mod policy_id_tests { assert_eq!(policy_id, "policy0"); } } + +mod error_source_tests { + use super::*; + use cool_asserts::assert_matches; + use miette::Diagnostic; + use serde_json::json; + + /// These errors should have both a source location (span) and attached source code. + #[test] + fn errors_have_source_location_and_source_code() { + // parse errors + let srcs = [ + r#"@one("two") @one("three") permit(principal, action, resource);"#, + r#"superforbid ( principal in Group::"bad", action, resource );"#, + r#"permit ( principal is User::"alice", action, resource );"#, + ]; + for src in srcs { + assert_matches!(PolicySet::from_str(src), Err(e) => { + assert!(e.labels().is_some(), "no source span for the parse error resulting from:\n {src}\nerror was:\n{:?}", miette::Report::new(e)); + assert!(e.source_code().is_some(), "no source code for the parse error resulting from:\n {src}\nerror was:\n{:?}", miette::Report::new(e)); + }); + } + + // evaluation errors + let srcs = [ + "1 + true", + "3 has foo", + "true && ([2, 3, 4] in [4, 5, 6])", + "ip(3)", + ]; + let req = Request::new(None, None, None, Context::empty(), None).unwrap(); + let entities = Entities::empty(); + for src in srcs { + let expr = Expression::from_str(src).unwrap(); + assert_matches!(eval_expression(&req, &entities, &expr), Err(_e) => { + /* TODO(#485): evaluation errors don't currently have source locations + assert!(e.labels().is_some(), "no source span for the evaluation error resulting from:\n {src}\nerror was:\n{:?}", miette::Report::new(e)); + assert!(e.source_code().is_some(), "no source code for the evaluation error resulting from:\n {src}\nerror was:\n{:?}", miette::Report::new(e)); + */ + }); + } + + // evaluation errors in policies + let srcs = [ + "permit ( principal, action, resource ) when { 1 + true };", + "permit ( principal, action, resource ) when { 3 has foo };", + "permit ( principal, action, resource ) when { true && ([2, 3, 4] in [4, 5, 6]) };", + "permit ( principal, action, resource ) when { ip(3) };", + ]; + let req = Request::new(None, None, None, Context::empty(), None).unwrap(); + let entities = Entities::empty(); + for src in srcs { + let pset = PolicySet::from_str(src).unwrap(); + let resp = Authorizer::new().is_authorized(&req, &pset, &entities); + for _err in resp.diagnostics().errors() { + /* TODO(#485): evaluation errors don't currently have source locations + assert!(err.labels().is_some(), "no source span for the evaluation error resulting from:\n {src}\nerror was:\n{:?}", miette::Report::new(err.clone())); + assert!(err.source_code().is_some(), "no source code for the evaluation error resulting from:\n {src}\nerror was:\n{:?}", miette::Report::new(err.clone())); + */ + } + } + + // validation errors + let validator = Validator::new( + Schema::from_json_value(json!({ "": { "actions": { "view": {} }, "entityTypes": {} }})) + .unwrap(), + ); + // same srcs as above + for src in srcs { + let pset = PolicySet::from_str(src).unwrap(); + let res = validator.validate(&pset, ValidationMode::Strict); + for err in res.validation_errors() { + assert!(err.labels().is_some(), "no source span for the validation error resulting from:\n {src}\nerror was:\n{:?}", miette::Report::new(err.clone())); + assert!(err.source_code().is_some(), "no source code for the validation error resulting from:\n {src}\nerror was:\n{:?}", miette::Report::new(err.clone())); + } + for warn in res.validation_warnings() { + assert!(warn.labels().is_some(), "no source span for the validation error resulting from:\n {src}\nerror was:\n{:?}", miette::Report::new(warn.clone())); + assert!(warn.source_code().is_some(), "no source code for the validation error resulting from:\n {src}\nerror was:\n{:?}", miette::Report::new(warn.clone())); + } + } + } +}