diff --git a/cedar-policy-cli/Cargo.toml b/cedar-policy-cli/Cargo.toml index c905916b4..36f865414 100644 --- a/cedar-policy-cli/Cargo.toml +++ b/cedar-policy-cli/Cargo.toml @@ -2,7 +2,7 @@ name = "cedar-policy-cli" edition = "2021" -version = "2.3.3" +version = "2.4.0" license = "Apache-2.0" categories = ["compilers", "config"] description = "CLI interface for the Cedar Policy language." @@ -11,15 +11,13 @@ homepage = "https://cedarpolicy.com" repository = "https://github.com/cedar-policy/cedar" [dependencies] -cedar-policy = { version = "=2.3.3", path = "../cedar-policy" } -cedar-policy-formatter = { version = "=2.3.3", path = "../cedar-policy-formatter" } -clap = { version = "4", features = ["derive"] } +cedar-policy = { version = "=2.4.0", path = "../cedar-policy" } +cedar-policy-formatter = { version = "=2.4.0", path = "../cedar-policy-formatter" } +clap = { version = "4", features = ["derive", "env"] } serde = { version = "1.0", features = ["derive"] } serde_json = "1.0" -anyhow = "1.0" - -[features] -default = [] +miette = { version = "5.9.0", features = ["fancy"] } +thiserror = "1.0" [dev-dependencies] assert_cmd = "2.0" diff --git a/cedar-policy-cli/src/err.rs b/cedar-policy-cli/src/err.rs new file mode 100644 index 000000000..c6d544af2 --- /dev/null +++ b/cedar-policy-cli/src/err.rs @@ -0,0 +1,39 @@ +/* + * Copyright 2023 Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +use std::error::Error; + +use miette::{Diagnostic, Report}; +use thiserror::Error; + +/// Internal adapter from Rust's standard [`Error`] to [`miette`]'s +/// [`Diagnostic`], with an attached diagnostic code. +#[derive(Debug, Diagnostic, Error)] +#[error(transparent)] +#[diagnostic(code(cedar_policy_cli::other_err))] +struct DiagnosticError(Box); + +/// Alternative to [`miette::IntoDiagnostic`] which attaches diagnostic codes +/// to adapted [`Error`]s for better-formatted output. +pub trait IntoDiagnostic { + fn into_diagnostic(self) -> Result; +} + +impl IntoDiagnostic for Result { + fn into_diagnostic(self) -> Result { + self.map_err(|err| DiagnosticError(Box::new(err)).into()) + } +} diff --git a/cedar-policy-cli/src/lib.rs b/cedar-policy-cli/src/lib.rs index bc738211e..7ff68311b 100644 --- a/cedar-policy-cli/src/lib.rs +++ b/cedar-policy-cli/src/lib.rs @@ -19,13 +19,14 @@ // omitted. #![allow(clippy::needless_return)] -use anyhow::{Context as _, Error, Result}; -use cedar_policy::*; -use cedar_policy_formatter::{policies_str_to_pretty, Config}; -use clap::{Args, Parser, Subcommand}; +mod err; + +use clap::{Args, Parser, Subcommand, ValueEnum}; +use miette::{miette, NamedSource, Report, Result, WrapErr}; use serde::{Deserialize, Serialize}; use std::{ collections::HashMap, + fmt::{self, Display}, fs::OpenOptions, path::Path, process::{ExitCode, Termination}, @@ -33,12 +34,54 @@ use std::{ time::Instant, }; +use cedar_policy::*; +use cedar_policy_formatter::{policies_str_to_pretty, Config}; + +use crate::err::IntoDiagnostic; + /// Basic Cedar CLI for evaluating authorization queries #[derive(Parser)] #[command(author, version, about, long_about = None)] // Pull from `Cargo.toml` pub struct Cli { #[command(subcommand)] pub command: Commands, + /// The output format to use for error reporting. + #[arg( + global = true, + short = 'f', + long = "error-format", + env = "CEDAR_ERROR_FORMAT", + default_value_t, + value_enum + )] + pub err_fmt: ErrorFormat, +} + +#[derive(Clone, Copy, Debug, Default, Eq, PartialEq, ValueEnum)] +pub enum ErrorFormat { + /// Human-readable error messages with terminal graphics and inline code + /// snippets. + #[default] + Human, + /// Plain-text error messages without fancy graphics or colors, suitable for + /// screen readers. + Plain, + /// Machine-readable JSON output. + Json, +} + +impl Display for ErrorFormat { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!( + f, + "{}", + match self { + ErrorFormat::Human => "human", + ErrorFormat::Plain => "plain", + ErrorFormat::Json => "json", + } + ) + } } #[derive(Subcommand, Debug)] @@ -71,7 +114,7 @@ pub struct ValidateArgs { pub struct CheckParseArgs { /// File containing the policy set #[clap(short, long = "policies", value_name = "FILE")] - pub policies_file: String, + pub policies_file: Option, } /// This struct contains the arguments that together specify a request. @@ -104,37 +147,41 @@ impl RequestArgs { match &self.request_json_file { Some(jsonfile) => { let jsonstring = std::fs::read_to_string(jsonfile) - .context(format!("failed to open request-json file {jsonfile}"))?; + .into_diagnostic() + .wrap_err_with(|| format!("failed to open request-json file {jsonfile}"))?; let qjson: RequestJSON = serde_json::from_str(&jsonstring) - .context(format!("failed to parse request-json file {jsonfile}"))?; + .into_diagnostic() + .wrap_err_with(|| format!("failed to parse request-json file {jsonfile}"))?; let principal = qjson .principal .map(|s| { - s.parse().context(format!( - "failed to parse principal in {jsonfile} as entity Uid" - )) + s.parse().wrap_err_with(|| { + format!("failed to parse principal in {jsonfile} as entity Uid") + }) }) .transpose()?; let action = qjson .action .map(|s| { - s.parse().context(format!( - "failed to parse action in {jsonfile} as entity Uid" - )) + s.parse().wrap_err_with(|| { + format!("failed to parse action in {jsonfile} as entity Uid") + }) }) .transpose()?; let resource = qjson .resource .map(|s| { - s.parse().context(format!( - "failed to parse resource in {jsonfile} as entity Uid" - )) + s.parse().wrap_err_with(|| { + format!("failed to parse resource in {jsonfile} as entity Uid") + }) }) .transpose()?; let context = Context::from_json_value( qjson.context, schema.and_then(|s| Some((s, action.as_ref()?))), - )?; + ) + .into_diagnostic() + .wrap_err_with(|| format!("failed to create a context from {jsonfile}"))?; Ok(Request::new(principal, action, resource, context)) } None => { @@ -142,8 +189,9 @@ impl RequestArgs { .principal .as_ref() .map(|s| { - s.parse() - .context(format!("failed to parse principal {s} as entity Uid")) + s.parse().wrap_err_with(|| { + format!("failed to parse principal {s} as entity Uid") + }) }) .transpose()?; let action = self @@ -151,7 +199,7 @@ impl RequestArgs { .as_ref() .map(|s| { s.parse() - .context(format!("failed to parse action {s} as entity Uid")) + .wrap_err_with(|| format!("failed to parse action {s} as entity Uid")) }) .transpose()?; let resource = self @@ -159,7 +207,7 @@ impl RequestArgs { .as_ref() .map(|s| { s.parse() - .context(format!("failed to parse resource {s} as entity Uid")) + .wrap_err_with(|| format!("failed to parse resource {s} as entity Uid")) }) .transpose()?; let context: Context = match &self.context_json_file { @@ -168,9 +216,12 @@ impl RequestArgs { Ok(f) => Context::from_json_file( f, schema.and_then(|s| Some((s, action.as_ref()?))), - )?, - Err(e) => Err(Error::from(e) - .context(format!("error while loading context from {jsonfile}")))?, + ) + .into_diagnostic() + .wrap_err_with(|| format!("failed to create a context from {jsonfile}"))?, + Err(e) => Err(e).into_diagnostic().wrap_err_with(|| { + format!("error while loading context from {jsonfile}") + })?, }, }; Ok(Request::new(principal, action, resource, context)) @@ -330,20 +381,20 @@ impl Termination for CedarExitCode { } pub fn check_parse(args: &CheckParseArgs) -> CedarExitCode { - match read_policy_file(&args.policies_file) { + match read_policy_set(args.policies_file.as_ref()) { Ok(_) => CedarExitCode::Success, Err(e) => { - println!("{:#}", e); + println!("Error: {e:?}"); CedarExitCode::Failure } } } pub fn validate(args: &ValidateArgs) -> CedarExitCode { - let pset = match read_policy_file(&args.policies_file) { + let pset = match read_policy_set(Some(&args.policies_file)) { Ok(pset) => pset, Err(e) => { - println!("{:#}", e); + println!("Error: {e:?}"); return CedarExitCode::Failure; } }; @@ -351,7 +402,7 @@ pub fn validate(args: &ValidateArgs) -> CedarExitCode { let schema = match read_schema_file(&args.schema_file) { Ok(schema) => schema, Err(e) => { - println!("{:#}", e); + println!("Error: {e:?}"); return CedarExitCode::Failure; } }; @@ -376,30 +427,31 @@ pub fn evaluate(args: &EvaluateArgs) -> (CedarExitCode, EvalResult) { None => None, Some(Ok(schema)) => Some(schema), Some(Err(e)) => { - println!("{:#}", e); + println!("Error: {e:?}"); return (CedarExitCode::Failure, EvalResult::Bool(false)); } }; let request = match args.request.get_request(schema.as_ref()) { Ok(q) => q, Err(e) => { - println!("error: {:#}", e); - return (CedarExitCode::Failure, EvalResult::Bool(false)); - } - }; - let expr = match Expression::from_str(&args.expression) { - Ok(expr) => expr, - Err(e) => { - println!("error while parsing the expression: {e}"); + println!("Error: {e:?}"); return (CedarExitCode::Failure, EvalResult::Bool(false)); } }; + let expr = + match Expression::from_str(&args.expression).wrap_err("failed to parse the expression") { + Ok(expr) => expr, + Err(e) => { + println!("Error: {e:?}"); + return (CedarExitCode::Failure, EvalResult::Bool(false)); + } + }; let entities = match &args.entities_file { None => Entities::empty(), Some(file) => match load_entities(file, schema.as_ref()) { Ok(entities) => entities, Err(e) => { - println!("error: {:#}", e); + println!("Error: {e:?}"); return (CedarExitCode::Failure, EvalResult::Bool(false)); } }, @@ -407,13 +459,16 @@ pub fn evaluate(args: &EvaluateArgs) -> (CedarExitCode, EvalResult) { let entities = match load_actions_from_schema(entities, &schema) { Ok(entities) => entities, Err(e) => { - println!("error: {:#}", e); + println!("Error: {e:?}"); return (CedarExitCode::Failure, EvalResult::Bool(false)); } }; - match eval_expression(&request, &entities, &expr) { + match eval_expression(&request, &entities, &expr) + .into_diagnostic() + .wrap_err("failed to evaluate the expression") + { Err(e) => { - println!("error while evaluating the expression: {e}"); + println!("Error: {e:?}"); return (CedarExitCode::Failure, EvalResult::Bool(false)); } Ok(result) => { @@ -424,8 +479,8 @@ pub fn evaluate(args: &EvaluateArgs) -> (CedarExitCode, EvalResult) { } pub fn link(args: &LinkArgs) -> CedarExitCode { - if let Err(msg) = link_inner(args) { - eprintln!("{:#}", msg); + if let Err(err) = link_inner(args) { + println!("Error: {err:?}"); CedarExitCode::Failure } else { CedarExitCode::Success @@ -433,15 +488,7 @@ pub fn link(args: &LinkArgs) -> CedarExitCode { } fn format_policies_inner(args: &FormatArgs) -> Result<()> { - let mut policies_str = String::new(); - match &args.file_name { - Some(path) => { - policies_str = std::fs::read_to_string(path)?; - } - None => { - std::io::Read::read_to_string(&mut std::io::stdin(), &mut policies_str)?; - } - }; + let policies_str = read_from_file_or_stdin(args.file_name.as_ref(), "policy set")?; let config = Config { line_width: args.line_width, indent_width: args.indent_width, @@ -451,8 +498,8 @@ fn format_policies_inner(args: &FormatArgs) -> Result<()> { } pub fn format_policies(args: &FormatArgs) -> CedarExitCode { - if let Err(msg) = format_policies_inner(args) { - eprintln!("{:#}", msg); + if let Err(err) = format_policies_inner(args) { + println!("Error: {err:?}"); CedarExitCode::Failure } else { CedarExitCode::Success @@ -466,16 +513,18 @@ fn create_slot_env(data: &HashMap) -> Result Result<()> { - let mut policies = read_policy_file(&args.policies_file)?; + let mut policies = read_policy_set(Some(&args.policies_file))?; let slotenv = create_slot_env(&args.arguments.data)?; - policies.link( - PolicyId::from_str(&args.template_id)?, - PolicyId::from_str(&args.new_id)?, - slotenv, - )?; + policies + .link( + PolicyId::from_str(&args.template_id)?, + PolicyId::from_str(&args.new_id)?, + slotenv, + ) + .into_diagnostic()?; let linked = policies .policy(&PolicyId::from_str(&args.new_id)?) - .context("Failed to add template-linked policy")?; + .ok_or_else(|| miette!("Failed to add template-linked policy"))?; println!("Template Linked Policy Added: {linked}"); let linked = TemplateLinked { template_id: args.template_id.clone(), @@ -547,11 +596,13 @@ impl From for LiteralTemplateLinked { fn add_template_links_to_set(path: impl AsRef, policy_set: &mut PolicySet) -> Result<()> { for template_linked in load_liked_file(path)? { let slot_env = create_slot_env(&template_linked.args)?; - policy_set.link( - PolicyId::from_str(&template_linked.template_id)?, - PolicyId::from_str(&template_linked.link_id)?, - slot_env, - )?; + policy_set + .link( + PolicyId::from_str(&template_linked.template_id)?, + PolicyId::from_str(&template_linked.link_id)?, + slot_env, + ) + .into_diagnostic()?; } Ok(()) } @@ -565,12 +616,19 @@ fn load_liked_file(path: impl AsRef) -> Result> { return Ok(vec![]); } }; - if f.metadata().context("Failed to read metadata")?.len() == 0 { + if f.metadata() + .into_diagnostic() + .wrap_err("Failed to read metadata")? + .len() + == 0 + { // File is empty, return empty set Ok(vec![]) } else { // File has contents, deserialize - serde_json::from_reader(f).context("Deserialization error") + serde_json::from_reader(f) + .into_diagnostic() + .wrap_err("Deserialization error") } } @@ -587,8 +645,9 @@ fn write_template_linked_file(linked: &[TemplateLinked], path: impl AsRef) .write(true) .truncate(true) .create(true) - .open(path)?; - Ok(serde_json::to_writer(f, linked)?) + .open(path) + .into_diagnostic()?; + serde_json::to_writer(f, linked).into_diagnostic() } pub fn authorize(args: &AuthorizeArgs) -> CedarExitCode { @@ -616,7 +675,7 @@ pub fn authorize(args: &AuthorizeArgs) -> CedarExitCode { if ans.diagnostics().errors().peekable().peek().is_some() { println!(); for err in ans.diagnostics().errors() { - println!("{}", err); + println!("{err}"); } } if args.verbose { @@ -635,7 +694,7 @@ pub fn authorize(args: &AuthorizeArgs) -> CedarExitCode { } Err(errs) => { for err in errs { - println!("{:#}", err); + println!("{err:?}"); } CedarExitCode::Failure } @@ -648,14 +707,20 @@ fn load_entities(entities_filename: impl AsRef, schema: Option<&Schema>) - .read(true) .open(entities_filename.as_ref()) { - Ok(f) => Entities::from_json_file(f, schema).context(format!( - "failed to parse entities from file {}", - entities_filename.as_ref().display() - )), - Err(e) => Err(e).context(format!( - "failed to open entities file {}", - entities_filename.as_ref().display() - )), + Ok(f) => Entities::from_json_file(f, schema) + .into_diagnostic() + .wrap_err_with(|| { + format!( + "failed to parse entities from file {}", + entities_filename.as_ref().display() + ) + }), + Err(e) => Err(e).into_diagnostic().wrap_err_with(|| { + format!( + "failed to open entities file {}", + entities_filename.as_ref().display() + ) + }), } } @@ -688,34 +753,69 @@ fn read_policy_and_links( policies_filename: impl AsRef, links_filename: Option>, ) -> Result { - let mut pset = read_policy_file(policies_filename.as_ref())?; + let mut pset = read_policy_set(Some(policies_filename.as_ref()))?; if let Some(links_filename) = links_filename { add_template_links_to_set(links_filename.as_ref(), &mut pset)?; } Ok(pset) } -fn read_policy_file(filename: impl AsRef) -> Result { - let src = std::fs::read_to_string(filename.as_ref()).context(format!( - "failed to open policy file {}", - filename.as_ref().display() - ))?; - let ps = PolicySet::from_str(&src).context(format!( - "failed to parse policies from file {}", - filename.as_ref().display() - ))?; +// Read from a file (when `filename` is a `Some`) or stdin (when `filename` is `None`) +fn read_from_file_or_stdin(filename: Option>, context: &str) -> Result { + let mut src_str = String::new(); + match filename.as_ref() { + Some(path) => { + src_str = std::fs::read_to_string(path) + .into_diagnostic() + .wrap_err_with(|| { + format!( + "failed to open {} file {}", + context, + path.as_ref().display() + ) + })?; + } + None => { + std::io::Read::read_to_string(&mut std::io::stdin(), &mut src_str) + .into_diagnostic() + .wrap_err_with(|| format!("failed to read {} from stdin", context))?; + } + }; + Ok(src_str) +} + +// Convenient wrapper around `read_from_file_or_stdin` to just read from a file +fn read_from_file(filename: impl AsRef, context: &str) -> Result { + read_from_file_or_stdin(Some(filename), context) +} + +fn read_policy_set( + filename: Option + std::marker::Copy>, +) -> miette::Result { + let context = "policy set"; + let ps_str = read_from_file_or_stdin(filename, context)?; + let ps = PolicySet::from_str(&ps_str) + .map_err(|err| { + let name = filename.map_or_else( + || "".to_owned(), + |n| n.as_ref().display().to_string(), + ); + Report::new(err).with_source_code(NamedSource::new(name, ps_str)) + }) + .wrap_err_with(|| format!("failed to parse {context}"))?; Ok(rename_from_id_annotation(ps)) } -fn read_schema_file(filename: impl AsRef) -> Result { - let schema_src = std::fs::read_to_string(filename.as_ref()).context(format!( - "failed to open schema file {}", - filename.as_ref().display() - ))?; - Schema::from_str(&schema_src).context(format!( - "failed to parse schema from file {}", - filename.as_ref().display() - )) +fn read_schema_file(filename: impl AsRef + std::marker::Copy) -> Result { + let schema_src = read_from_file(filename, "schema")?; + Schema::from_str(&schema_src) + .into_diagnostic() + .wrap_err_with(|| { + format!( + "failed to parse schema from file {}", + filename.as_ref().display() + ) + }) } fn load_actions_from_schema(entities: Entities, schema: &Option) -> Result { @@ -727,8 +827,11 @@ fn load_actions_from_schema(entities: Entities, schema: &Option) -> Resu .cloned() .chain(action_entities.iter().cloned()), ) - .context("failed to merge action entities with entity file"), - Err(e) => Err(e).context("failed to construct action entities"), + .into_diagnostic() + .wrap_err("failed to merge action entities with entity file"), + Err(e) => Err(e) + .into_diagnostic() + .wrap_err("failed to construct action entities"), }, None => Ok(entities), } @@ -737,12 +840,12 @@ fn load_actions_from_schema(entities: Entities, schema: &Option) -> Resu /// This uses the Cedar API to call the authorization engine. fn execute_request( request: &RequestArgs, - policies_filename: impl AsRef, + policies_filename: impl AsRef + std::marker::Copy, links_filename: Option>, entities_filename: impl AsRef, - schema_filename: Option>, + schema_filename: Option + std::marker::Copy>, compute_duration: bool, -) -> Result> { +) -> Result> { let mut errs = vec![]; let policies = match read_policy_and_links(policies_filename.as_ref(), links_filename) { Ok(pset) => pset, @@ -776,7 +879,7 @@ fn execute_request( let request = match request.get_request(schema.as_ref()) { Ok(q) => Some(q), Err(e) => { - errs.push(e.context("failed to parse request")); + errs.push(e.wrap_err("failed to parse request")); None } }; diff --git a/cedar-policy-cli/src/main.rs b/cedar-policy-cli/src/main.rs index 0b8ec2598..2e3274a31 100644 --- a/cedar-policy-cli/src/main.rs +++ b/cedar-policy-cli/src/main.rs @@ -16,14 +16,29 @@ #![forbid(unsafe_code)] +use clap::Parser; +use miette::ErrorHook; + use cedar_policy_cli::{ - authorize, check_parse, evaluate, format_policies, link, validate, CedarExitCode, Cli, Commands, + authorize, check_parse, evaluate, format_policies, link, validate, CedarExitCode, Cli, + Commands, ErrorFormat, }; -use clap::Parser; - fn main() -> CedarExitCode { - match Cli::parse().command { + let cli = Cli::parse(); + + let err_hook: Option = match cli.err_fmt { + ErrorFormat::Human => None, // This is the default. + ErrorFormat::Plain => Some(Box::new(|_| { + Box::new(miette::NarratableReportHandler::new()) + })), + ErrorFormat::Json => Some(Box::new(|_| Box::new(miette::JSONReportHandler::new()))), + }; + if let Some(err_hook) = err_hook { + miette::set_hook(err_hook).expect("failed to install error-reporting hook"); + } + + match cli.command { Commands::Authorize(args) => authorize(&args), Commands::Evaluate(args) => evaluate(&args).0, Commands::CheckParse(args) => check_parse(&args), diff --git a/cedar-policy-cli/tests/sample.rs b/cedar-policy-cli/tests/sample.rs index 82f2a5305..12479ae40 100644 --- a/cedar-policy-cli/tests/sample.rs +++ b/cedar-policy-cli/tests/sample.rs @@ -26,7 +26,7 @@ use cedar_policy_cli::{ fn run_check_parse_test(policies_file: impl Into, expected_exit_code: CedarExitCode) { let cmd = CheckParseArgs { - policies_file: policies_file.into(), + policies_file: Some(policies_file.into()), }; let output = check_parse(&cmd); assert_eq!(output, expected_exit_code, "{:#?}", cmd); diff --git a/cedar-policy-core/Cargo.toml b/cedar-policy-core/Cargo.toml index 295d281c6..5164614ce 100644 --- a/cedar-policy-core/Cargo.toml +++ b/cedar-policy-core/Cargo.toml @@ -3,7 +3,7 @@ name = "cedar-policy-core" edition = "2021" build = "build.rs" -version = "2.3.3" +version = "2.4.0" license = "Apache-2.0" categories = ["compilers", "config"] description = "Core implemenation of the Cedar Policy language." @@ -24,6 +24,7 @@ thiserror = "1.0" smol_str = { version = "0.2", features = ["serde"] } stacker = "0.1.15" arbitrary = { version = "1", features = ["derive"], optional = true } +miette = "5.9.0" # ipaddr extension requires ipnet ipnet = { version = "2.5.0", optional = true } diff --git a/cedar-policy-core/src/ast/entity.rs b/cedar-policy-core/src/ast/entity.rs index a5ae91b27..61cfbae77 100644 --- a/cedar-policy-core/src/ast/entity.rs +++ b/cedar-policy-core/src/ast/entity.rs @@ -15,7 +15,7 @@ */ use crate::ast::*; -use crate::parser::err::ParseError; +use crate::parser::err::ParseErrors; use crate::transitive_closure::TCNode; use crate::FromNormalizedStr; use itertools::Itertools; @@ -104,7 +104,7 @@ impl EntityUID { // GRCOV_BEGIN_COVERAGE /// Create an `EntityUID` with the given (unqualified) typename, and the given string as its EID. - pub fn with_eid_and_type(typename: &str, eid: &str) -> Result> { + pub fn with_eid_and_type(typename: &str, eid: &str) -> Result { Ok(Self { ty: EntityType::Concrete(Name::parse_unqualified_name(typename)?), eid: Eid(eid.into()), @@ -157,9 +157,9 @@ impl std::fmt::Display for EntityUID { // allow `.parse()` on a string to make an `EntityUID` impl std::str::FromStr for EntityUID { - type Err = Vec; + type Err = ParseErrors; - fn from_str(s: &str) -> Result> { + fn from_str(s: &str) -> Result { crate::parser::parse_euid(s) } } diff --git a/cedar-policy-core/src/ast/expr.rs b/cedar-policy-core/src/ast/expr.rs index cd6c92ed7..d615d47e8 100644 --- a/cedar-policy-core/src/ast/expr.rs +++ b/cedar-policy-core/src/ast/expr.rs @@ -17,7 +17,7 @@ use crate::{ ast::*, extensions::Extensions, - parser::{err::ParseError, SourceInfo}, + parser::{err::ParseErrors, SourceInfo}, }; use itertools::Itertools; use serde::{Deserialize, Serialize}; @@ -767,9 +767,9 @@ fn maybe_with_parens(expr: &Expr) -> String { } impl std::str::FromStr for Expr { - type Err = Vec; + type Err = ParseErrors; - fn from_str(s: &str) -> Result> { + fn from_str(s: &str) -> Result { crate::parser::parse_expr(s) } } diff --git a/cedar-policy-core/src/ast/literal.rs b/cedar-policy-core/src/ast/literal.rs index ea4118449..47177b4aa 100644 --- a/cedar-policy-core/src/ast/literal.rs +++ b/cedar-policy-core/src/ast/literal.rs @@ -71,7 +71,7 @@ impl std::fmt::Display for Literal { } impl std::str::FromStr for Literal { - type Err = Vec; + type Err = parser::err::ParseErrors; fn from_str(s: &str) -> Result { parser::parse_literal(s) diff --git a/cedar-policy-core/src/ast/name.rs b/cedar-policy-core/src/ast/name.rs index a0ec04733..f14ca4708 100644 --- a/cedar-policy-core/src/ast/name.rs +++ b/cedar-policy-core/src/ast/name.rs @@ -20,8 +20,10 @@ use itertools::Itertools; use serde::{Deserialize, Serialize}; use smol_str::SmolStr; +use crate::parser::err::ParseErrors; +use crate::FromNormalizedStr; + use super::PrincipalOrResource; -use crate::{parser::err::ParseError, FromNormalizedStr}; /// Arc::unwrap_or_clone() isn't stabilized as of this writing, but this is its implementation // @@ -61,7 +63,7 @@ impl Name { /// Create a `Name` with no path (no namespaces). /// Returns an error if `s` is not a valid identifier. - pub fn parse_unqualified_name(s: &str) -> Result> { + pub fn parse_unqualified_name(s: &str) -> Result { Ok(Self { id: s.parse()?, path: Arc::new(vec![]), @@ -109,9 +111,9 @@ impl std::fmt::Display for Name { // allow `.parse()` on a string to make a `Name` impl std::str::FromStr for Name { - type Err = Vec; + type Err = ParseErrors; - fn from_str(s: &str) -> Result> { + fn from_str(s: &str) -> Result { crate::parser::parse_name(s) } } @@ -247,9 +249,9 @@ impl std::fmt::Display for Id { // allow `.parse()` on a string to make an `Id` impl std::str::FromStr for Id { - type Err = Vec; + type Err = ParseErrors; - fn from_str(s: &str) -> Result> { + fn from_str(s: &str) -> Result { crate::parser::parse_ident(s) } } diff --git a/cedar-policy-core/src/ast/restricted_expr.rs b/cedar-policy-core/src/ast/restricted_expr.rs index cf611702d..7b8ee45aa 100644 --- a/cedar-policy-core/src/ast/restricted_expr.rs +++ b/cedar-policy-core/src/ast/restricted_expr.rs @@ -118,7 +118,7 @@ impl RestrictedExpr { } impl std::str::FromStr for RestrictedExpr { - type Err = Vec; + type Err = parser::err::ParseErrors; fn from_str(s: &str) -> Result { parser::parse_restrictedexpr(s) diff --git a/cedar-policy-core/src/entities.rs b/cedar-policy-core/src/entities.rs index 8e51ebe32..74cce7095 100644 --- a/cedar-policy-core/src/entities.rs +++ b/cedar-policy-core/src/entities.rs @@ -1093,14 +1093,18 @@ mod schema_based_parsing_tests { .expect("hr_contacts attr should exist"); assert!(matches!(hr_contacts.expr_kind(), &ExprKind::Set(_))); let contact = { - let ExprKind::Set(set) = hr_contacts.expr_kind() else { panic!("already checked it was Set") }; + let ExprKind::Set(set) = hr_contacts.expr_kind() else { + panic!("already checked it was Set") + }; set.iter().next().expect("should be at least one contact") }; assert!(matches!(contact.expr_kind(), &ExprKind::Record { .. })); let json_blob = parsed .get("json_blob") .expect("json_blob attr should exist"); - let ExprKind::Record { pairs } = json_blob.expr_kind() else { panic!("expected json_blob to be a Record") }; + let ExprKind::Record { pairs } = json_blob.expr_kind() else { + panic!("expected json_blob to be a Record") + }; let (_, inner1) = pairs .iter() .find(|(k, _)| k == "inner1") @@ -1114,7 +1118,9 @@ mod schema_based_parsing_tests { .find(|(k, _)| k == "inner3") .expect("inner3 attr should exist"); assert!(matches!(inner3.expr_kind(), &ExprKind::Record { .. })); - let ExprKind::Record { pairs: innerpairs } = inner3.expr_kind() else { panic!("already checked it was Record") }; + let ExprKind::Record { pairs: innerpairs } = inner3.expr_kind() else { + panic!("already checked it was Record") + }; let (_, innerinner) = innerpairs .iter() .find(|(k, _)| k == "innerinner") @@ -1168,7 +1174,9 @@ mod schema_based_parsing_tests { .expect("hr_contacts attr should exist"); assert!(matches!(hr_contacts.expr_kind(), &ExprKind::Set(_))); let contact = { - let ExprKind::Set(set) = hr_contacts.expr_kind() else { panic!("already checked it was Set") }; + let ExprKind::Set(set) = hr_contacts.expr_kind() else { + panic!("already checked it was Set") + }; set.iter().next().expect("should be at least one contact") }; assert!(matches!( @@ -1178,7 +1186,9 @@ mod schema_based_parsing_tests { let json_blob = parsed .get("json_blob") .expect("json_blob attr should exist"); - let ExprKind::Record { pairs } = json_blob.expr_kind() else { panic!("expected json_blob to be a Record") }; + let ExprKind::Record { pairs } = json_blob.expr_kind() else { + panic!("expected json_blob to be a Record") + }; let (_, inner1) = pairs .iter() .find(|(k, _)| k == "inner1") @@ -1192,7 +1202,9 @@ mod schema_based_parsing_tests { .find(|(k, _)| k == "inner3") .expect("inner3 attr should exist"); assert!(matches!(inner3.expr_kind(), &ExprKind::Record { .. })); - let ExprKind::Record { pairs: innerpairs } = inner3.expr_kind() else { panic!("already checked it was Record") }; + let ExprKind::Record { pairs: innerpairs } = inner3.expr_kind() else { + panic!("already checked it was Record") + }; let (_, innerinner) = innerpairs .iter() .find(|(k, _)| k == "innerinner") diff --git a/cedar-policy-core/src/entities/json/jsonvalue.rs b/cedar-policy-core/src/entities/json/jsonvalue.rs index 1364da02d..fc2977111 100644 --- a/cedar-policy-core/src/entities/json/jsonvalue.rs +++ b/cedar-policy-core/src/entities/json/jsonvalue.rs @@ -125,7 +125,7 @@ impl From<&EntityUID> for TypeAndId { } impl TryFrom for EntityUID { - type Error = Vec; + type Error = crate::parser::err::ParseErrors; fn try_from(e: TypeAndId) -> Result { Ok(EntityUID::from_components( @@ -172,13 +172,16 @@ impl JSONValue { Self::ExprEscape { __expr: expr } => { use crate::parser; let expr: Expr = parser::parse_expr(&expr).map_err(|errs| { - JsonDeserializationError::ExprParseError(parser::err::ParseError::WithContext { - context: format!( - "contents of __expr escape {} are not a valid Cedar expression", - expr - ), - errs, - }) + JsonDeserializationError::ExprParseError( + parser::err::WithContext { + context: format!( + "contents of __expr escape {} are not a valid Cedar expression", + expr + ), + errs, + } + .into(), + ) })?; Ok(RestrictedExpr::new(expr)?) } @@ -187,13 +190,13 @@ impl JSONValue { Ok(RestrictedExpr::val( EntityUID::try_from(entity.clone()).map_err(|errs| { JsonDeserializationError::EntityParseError( - parser::err::ParseError::WithContext { + parser::err::WithContext { context: format!( "contents of __entity escape {} do not make a valid entity reference", serde_json::to_string_pretty(&entity).unwrap_or_else(|_| format!("{:?}", &entity)) ), errs, - }, + }.into(), ) })?, )) @@ -291,13 +294,16 @@ impl FnAndArg { use crate::parser; Ok(RestrictedExpr::call_extension_fn( Name::from_normalized_str(&self.ext_fn).map_err(|errs| { - JsonDeserializationError::ExtnParseError(parser::err::ParseError::WithContext { - context: format!( - "in __extn escape, {:?} is not a valid function name", - &self.ext_fn, - ), - errs, - }) + JsonDeserializationError::ExtnParseError( + parser::err::WithContext { + context: format!( + "in __extn escape, {:?} is not a valid function name", + &self.ext_fn, + ), + errs, + } + .into(), + ) })?, vec![JSONValue::into_expr(*self.arg)?], )) diff --git a/cedar-policy-core/src/est.rs b/cedar-policy-core/src/est.rs index dc59c0e3a..2fc575009 100644 --- a/cedar-policy-core/src/est.rs +++ b/cedar-policy-core/src/est.rs @@ -106,7 +106,7 @@ impl Clause { impl TryFrom for Policy { type Error = ParseErrors; fn try_from(policy: cst::Policy) -> Result { - let mut errs = vec![]; + let mut errs = ParseErrors::new(); let effect = policy.effect.to_effect(&mut errs); let (principal, action, resource) = policy.extract_head(&mut errs); if let (Some(effect), Some(principal), Some(action), Some(resource), true) = @@ -123,11 +123,11 @@ impl TryFrom for Policy { }) .collect::, ParseErrors>>()?; let annotations = policy.annotations.into_iter().map(|node| { - let mut errs = vec![]; + let mut errs = ParseErrors::new(); let kv = node.to_kv_pair(&mut errs); match (errs.is_empty(), kv) { (true, Some((k, v))) => Ok((k, v)), - (false, _) => Err(ParseErrors(errs)), + (false, _) => Err(errs), (true, None) => Err(ParseError::ToAST("internal invariant violation: expected there to be an error if data is None here".to_string()).into()), } }).collect::>()?; @@ -140,7 +140,7 @@ impl TryFrom for Policy { annotations, }) } else { - Err(ParseErrors(errs)) + Err(errs) } } } @@ -148,7 +148,7 @@ impl TryFrom for Policy { impl TryFrom for Clause { type Error = ParseErrors; fn try_from(cond: cst::Cond) -> Result { - let mut errs = vec![]; + let mut errs = ParseErrors::new(); let expr: Result = match cond.expr { None => Err(ParseError::ToAST("clause should not be empty".to_string()).into()), Some(ASTNode { node, .. }) => match node { @@ -175,7 +175,7 @@ impl TryFrom for Clause { Some(false) => Clause::Unless(expr), }) } else { - Err(ParseErrors(errs)) + Err(errs) } } } diff --git a/cedar-policy-core/src/est/expr.rs b/cedar-policy-core/src/est/expr.rs index 240f2a322..b92a2d31c 100644 --- a/cedar-policy-core/src/est/expr.rs +++ b/cedar-policy-core/src/est/expr.rs @@ -19,7 +19,7 @@ use super::EstToAstError; use crate::ast; use crate::entities::{JSONValue, JsonDeserializationError, TypeAndId}; use crate::parser::cst; -use crate::parser::err::{ParseError, ParseErrors}; +use crate::parser::err::{ParseError, ParseErrors, WithContext}; use crate::parser::unescape; use crate::parser::ASTNode; use either::Either; @@ -597,10 +597,10 @@ impl TryFrom for ast::Expr { .next() .expect("already checked that len was 1"); let fn_name = fn_name.parse().map_err(|errs| - JsonDeserializationError::ExtnParseError(ParseError::WithContext { + JsonDeserializationError::ExtnParseError(WithContext { context: format!("expected valid operator or extension function name; got {fn_name}"), errs, - }) + }.into()) )?; Ok(ast::Expr::call_extension_fn( fn_name, @@ -1058,7 +1058,7 @@ fn interpret_primary(p: cst::Primary) -> Result, ParseEr }, cst::Primary::Ref(ASTNode { node, .. }) => match node { Some(cst::Ref::Uid { path, eid }) => { - let mut errs = vec![]; + let mut errs = ParseErrors::new(); let maybe_name = path.to_name(&mut errs); let maybe_eid = eid.as_valid_string(&mut errs); @@ -1071,7 +1071,7 @@ fn interpret_primary(p: cst::Primary) -> Result, ParseEr )), }))) } - _ => Err(ParseErrors(errs)), + _ => Err(errs), } } Some(cst::Ref::Ref { .. }) => { @@ -1090,7 +1090,7 @@ fn interpret_primary(p: cst::Primary) -> Result, ParseEr } (&[], Some(cst::Ident::Context)) => Ok(Either::Right(Expr::var(ast::Var::Context))), (path, Some(cst::Ident::Ident(id))) => Ok(Either::Left(ast::Name::new( - id.parse().map_err(ParseErrors)?, + id.parse()?, path.iter() .map(|ASTNode { node, .. }| { node.as_ref() @@ -1099,7 +1099,7 @@ fn interpret_primary(p: cst::Primary) -> Result, ParseEr "node should not be empty".to_string(), )]) }) - .and_then(|id| id.to_string().parse().map_err(ParseErrors)) + .and_then(|id| id.to_string().parse().map_err(Into::into)) }) .collect::, _>>()?, ))), @@ -1137,12 +1137,12 @@ fn interpret_primary(p: cst::Primary) -> Result, ParseEr .into_iter() .map(|node| match node.node { Some(cst::RecInit(k, v)) => { - let mut errs = vec![]; + let mut errs = ParseErrors::new(); let s = k .to_expr_or_special(&mut errs) .and_then(|es| es.into_valid_attr(&mut errs)); if !errs.is_empty() { - Err(ParseErrors(errs)) + Err(errs) } else { match (s, v.node) { (Some(s), Some(e)) => Ok((s, e.try_into()?)), diff --git a/cedar-policy-core/src/from_normalized_str.rs b/cedar-policy-core/src/from_normalized_str.rs index b72f21783..bf89e64a4 100644 --- a/cedar-policy-core/src/from_normalized_str.rs +++ b/cedar-policy-core/src/from_normalized_str.rs @@ -1,11 +1,11 @@ -use crate::parser::err::ParseError; +use crate::parser::err::{ParseError, ParseErrors}; use std::fmt::Display; use std::str::FromStr; /// Trait for parsing "normalized" strings only, throwing an error if a /// non-normalized string is encountered. See docs on the /// [`from_normalized_str`] trait function. -pub trait FromNormalizedStr: FromStr> + Display { +pub trait FromNormalizedStr: FromStr + Display { /// Create a `Self` by parsing a string, which is required to be normalized. /// That is, the input is required to roundtrip with the `Display` impl on `Self`: /// `Self::from_normalized_str(x).to_string() == x` must hold. @@ -18,17 +18,17 @@ pub trait FromNormalizedStr: FromStr> + Display { /// /// For the version that accepts whitespace and Cedar comments, use the /// actual `FromStr` implementations. - fn from_normalized_str(s: &str) -> Result> { + fn from_normalized_str(s: &str) -> Result { let parsed = Self::from_str(s)?; let normalized = parsed.to_string(); if normalized == s { // the normalized representation is indeed the one that was provided Ok(parsed) } else { - Err(vec![ParseError::ToAST(format!( + Err(ParseError::ToAST(format!( "{} needs to be normalized (e.g., whitespace removed): {s} The normalized form is {normalized}", Self::describe_self() - ))]) + )).into()) } } diff --git a/cedar-policy-core/src/parser.rs b/cedar-policy-core/src/parser.rs index 86646d998..876825fee 100644 --- a/cedar-policy-core/src/parser.rs +++ b/cedar-policy-core/src/parser.rs @@ -40,11 +40,17 @@ use crate::est; /// simple main function for parsing policies /// generates numbered ids -pub fn parse_policyset(text: &str) -> Result> { - let mut errs = Vec::new(); - text_to_cst::parse_policies(text)? - .to_policyset(&mut errs) - .ok_or(errs) +pub fn parse_policyset(text: &str) -> Result { + let mut errs = err::ParseErrors::new(); + let cst = text_to_cst::parse_policies(text)?; + let Some(ast) = cst.to_policyset(&mut errs) else { + return Err(errs); + }; + if errs.is_empty() { + Ok(ast) + } else { + Err(errs) + } } /// Like `parse_policyset()`, but also returns the (lossless) original text of @@ -52,24 +58,24 @@ pub fn parse_policyset(text: &str) -> Result Result<(HashMap, ast::PolicySet), err::ParseErrors> { - let mut errs = Vec::new(); - let cst = text_to_cst::parse_policies(text).map_err(err::ParseErrors)?; - let pset = cst - .to_policyset(&mut errs) - .ok_or_else(|| err::ParseErrors(errs.clone()))?; - // PANIC SAFETY Shouldn't be `none` since `parse_policies()` and `to_policyset()` didn't return `Err` - #[allow(clippy::expect_used)] - // PANIC SAFETY Indexing is safe because of how `SourceInfo` is constructed - #[allow(clippy::indexing_slicing)] - 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.info.0.clone()])) - .collect::>(); + let mut errs = err::ParseErrors::new(); + let cst = text_to_cst::parse_policies(text)?; + let Some(pset) = cst.to_policyset(&mut errs) else { + return Err(errs); + }; if errs.is_empty() { + // PANIC SAFETY Shouldn't be `none` since `parse_policies()` and `to_policyset()` didn't return `Err` + #[allow(clippy::expect_used)] + // PANIC SAFETY Indexing is safe because of how `SourceInfo` is constructed + #[allow(clippy::indexing_slicing)] + 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.info.0.clone()])) + .collect::>(); Ok((texts, pset)) } else { - Err(err::ParseErrors(errs)) + Err(errs) } } @@ -79,24 +85,25 @@ pub fn parse_policyset_and_also_return_policy_text( pub fn parse_policyset_to_ests_and_pset( text: &str, ) -> Result<(HashMap, ast::PolicySet), err::ParseErrors> { - let mut errs = Vec::new(); - let cst = text_to_cst::parse_policies(text).map_err(err::ParseErrors)?; - let pset = cst - .to_policyset(&mut errs) - .ok_or_else(|| err::ParseErrors(errs.clone()))?; - // PANIC SAFETY Shouldn't be `none` since `parse_policies()` and `to_policyset()` didn't return `Err` - #[allow(clippy::expect_used)] - let ests = cst - .with_generated_policyids() - .expect("shouldn't be None since parse_policies() and to_policyset() didn't return Err") - .map(|(id, policy)| match &policy.node { - Some(p) => Ok(Some((id, p.clone().try_into()?))), - None => Ok(None), - }) - .collect::>, err::ParseErrors>>()?; - match (errs.is_empty(), ests) { - (true, Some(ests)) => Ok((ests, pset)), - (_, _) => Err(err::ParseErrors(errs)), + let mut errs = err::ParseErrors::new(); + let cst = text_to_cst::parse_policies(text)?; + let Some(pset) = cst.to_policyset(&mut errs) else { + return Err(errs); + }; + if errs.is_empty() { + // PANIC SAFETY Shouldn't be `None` since `parse_policies()` and `to_policyset()` didn't return `Err` + #[allow(clippy::expect_used)] + let ests = cst + .with_generated_policyids() + .expect("missing policy set node") + .map(|(id, policy)| { + let p = policy.node.as_ref().expect("missing policy node").clone(); + Ok((id, p.try_into()?)) + }) + .collect::, err::ParseErrors>>()?; + Ok((ests, pset)) + } else { + Err(errs) } } @@ -106,15 +113,18 @@ pub fn parse_policyset_to_ests_and_pset( pub fn parse_policy_template( id: Option, text: &str, -) -> Result> { - let mut errs = Vec::new(); +) -> Result { + let mut errs = err::ParseErrors::new(); let id = match id { Some(id) => ast::PolicyID::from_string(id), None => ast::PolicyID::from_string("policy0"), }; - let r = text_to_cst::parse_policy(text)?.to_policy_template(id, &mut errs); + let cst = text_to_cst::parse_policy(text)?; + let Some(ast) = cst.to_policy_template(id, &mut errs) else { + return Err(errs); + }; if errs.is_empty() { - r.ok_or(errs).map(ast::Template::from) + Ok(ast) } else { Err(errs) } @@ -127,38 +137,38 @@ pub fn parse_policy_template_to_est_and_ast( id: Option, text: &str, ) -> Result<(est::Policy, ast::Template), err::ParseErrors> { - let mut errs = Vec::new(); + let mut errs = err::ParseErrors::new(); let id = match id { Some(id) => ast::PolicyID::from_string(id), None => ast::PolicyID::from_string("policy0"), }; - let cst = text_to_cst::parse_policy(text).map_err(err::ParseErrors)?; - let ast = cst - .to_policy_template(id, &mut errs) - .ok_or_else(|| err::ParseErrors(errs.clone()))?; - let est = cst.node.map(TryInto::try_into).transpose()?; - match (errs.is_empty(), est) { - (true, Some(est)) => Ok((est, ast)), - (_, _) => Err(err::ParseErrors(errs)), + let cst = text_to_cst::parse_policy(text)?; + let (Some(ast), Some(cst_node)) = (cst.to_policy_template(id, &mut errs), cst.node) else { + return Err(errs); + }; + if errs.is_empty() { + let est = cst_node.try_into()?; + Ok((est, ast)) + } else { + Err(errs) } } /// simple main function for parsing a policy. /// If `id` is Some, then the resulting policy will have that `id`. /// If the `id` is None, the parser will use "policy0". -pub fn parse_policy( - id: Option, - text: &str, -) -> Result> { - let mut errs = Vec::new(); +pub fn parse_policy(id: Option, text: &str) -> Result { + let mut errs = err::ParseErrors::new(); let id = match id { Some(id) => ast::PolicyID::from_string(id), None => ast::PolicyID::from_string("policy0"), }; - let r = text_to_cst::parse_policy(text)?.to_policy(id, &mut errs); - + let cst = text_to_cst::parse_policy(text)?; + let Some(ast) = cst.to_policy(id, &mut errs) else { + return Err(errs); + }; if errs.is_empty() { - r.ok_or(errs) + Ok(ast) } else { Err(errs) } @@ -171,90 +181,109 @@ pub fn parse_policy_to_est_and_ast( id: Option, text: &str, ) -> Result<(est::Policy, ast::StaticPolicy), err::ParseErrors> { - let mut errs = Vec::new(); + let mut errs = err::ParseErrors::new(); let id = match id { Some(id) => ast::PolicyID::from_string(id), None => ast::PolicyID::from_string("policy0"), }; - let cst = text_to_cst::parse_policy(text).map_err(err::ParseErrors)?; - let ast = cst - .to_policy(id, &mut errs) - .ok_or_else(|| err::ParseErrors(errs.clone()))?; - - let est = cst.node.map(TryInto::try_into).transpose()?; - match (errs.is_empty(), est) { - (true, Some(est)) => Ok((est, ast)), - (_, _) => Err(err::ParseErrors(errs)), + let cst = text_to_cst::parse_policy(text)?; + let (Some(ast), Some(cst_node)) = (cst.to_policy(id, &mut errs), cst.node) else { + return Err(errs); + }; + if errs.is_empty() { + let est = cst_node.try_into()?; + Ok((est, ast)) + } else { + Err(errs) } } /// Parse a policy or template (either one works) to its EST representation pub fn parse_policy_or_template_to_est(text: &str) -> Result { - let cst = text_to_cst::parse_policy(text).map_err(err::ParseErrors)?; - let est = cst.node.map(TryInto::try_into).transpose()?; - match est { - Some(est) => Ok(est), - None => Err(err::ParseErrors(vec![])), // theoretically this shouldn't happen if the `?`s above didn't already fail us out - } + let cst = text_to_cst::parse_policy(text)?; + // PANIC SAFETY Shouldn't be `none` since `parse_policy()` didn't return `Err` + #[allow(clippy::expect_used)] + let cst_node = cst.node.expect("missing policy or template node"); + cst_node.try_into() } /// parse an Expr /// /// Private to this crate. Users outside Core should use `Expr`'s `FromStr` impl /// or its constructors -pub(crate) fn parse_expr(ptext: &str) -> Result> { - let mut errs = Vec::new(); - text_to_cst::parse_expr(ptext)? - .to_expr(&mut errs) - .ok_or(errs) +pub(crate) fn parse_expr(ptext: &str) -> Result { + let mut errs = err::ParseErrors::new(); + let cst = text_to_cst::parse_expr(ptext)?; + let Some(ast) = cst.to_expr(&mut errs) else { + return Err(errs); + }; + if errs.is_empty() { + Ok(ast) + } else { + Err(errs) + } } /// parse a RestrictedExpr /// /// Private to this crate. Users outside Core should use `RestrictedExpr`'s /// `FromStr` impl or its constructors -pub(crate) fn parse_restrictedexpr( - ptext: &str, -) -> Result> { +pub(crate) fn parse_restrictedexpr(ptext: &str) -> Result { parse_expr(ptext) - .and_then(|expr| ast::RestrictedExpr::new(expr).map_err(|err| vec![err.into()])) + .and_then(|expr| ast::RestrictedExpr::new(expr).map_err(err::ParseErrors::from)) } /// parse an EntityUID /// /// Private to this crate. Users outside Core should use `EntityUID`'s `FromStr` /// impl or its constructors -pub(crate) fn parse_euid(euid: &str) -> Result> { - let mut errs = Vec::new(); - text_to_cst::parse_ref(euid)?.to_ref(&mut errs).ok_or(errs) +pub(crate) fn parse_euid(euid: &str) -> Result { + let mut errs = err::ParseErrors::new(); + let cst = text_to_cst::parse_ref(euid)?; + let Some(ast) = cst.to_ref(&mut errs) else { + return Err(errs); + }; + if errs.is_empty() { + Ok(ast) + } else { + Err(errs) + } } /// parse a Name /// /// Private to this crate. Users outside Core should use `Name`'s `FromStr` impl /// or its constructors -pub(crate) fn parse_name(name: &str) -> Result> { - let mut errs = Vec::new(); - text_to_cst::parse_name(name)? - .to_name(&mut errs) - .ok_or(errs) +pub(crate) fn parse_name(name: &str) -> Result { + let mut errs = err::ParseErrors::new(); + let cst = text_to_cst::parse_name(name)?; + let Some(ast) = cst.to_name(&mut errs) else { + return Err(errs); + }; + if errs.is_empty() { + Ok(ast) + } else { + Err(errs) + } } /// parse a string into an ast::Literal (does not support expressions) /// /// Private to this crate. Users outside Core should use `Literal`'s `FromStr` impl /// or its constructors -pub(crate) fn parse_literal(val: &str) -> Result> { - let mut errs = Vec::new(); - match text_to_cst::parse_primary(val)? - .to_expr(&mut errs) - .ok_or(errs)? - .into_expr_kind() - { - ast::ExprKind::Lit(v) => Ok(v), - _ => Err(vec![err::ParseError::ToAST( - "text is not a literal".to_string(), - )]), +pub(crate) fn parse_literal(val: &str) -> Result { + let mut errs = err::ParseErrors::new(); + let cst = text_to_cst::parse_primary(val)?; + let Some(ast) = cst.to_expr(&mut errs) else { + return Err(errs); + }; + if errs.is_empty() { + match ast.into_expr_kind() { + ast::ExprKind::Lit(v) => Ok(v), + _ => Err(err::ParseError::ToAST("text is not a literal".to_string()).into()), + } + } else { + Err(errs) } } @@ -268,23 +297,35 @@ pub(crate) fn parse_literal(val: &str) -> Result Result> { - let mut errs = Vec::new(); +pub fn parse_internal_string(val: &str) -> Result { + let mut errs = err::ParseErrors::new(); // we need to add quotes for this to be a valid string literal - text_to_cst::parse_primary(&format!(r#""{val}""#))? - .to_string_literal(&mut errs) - .ok_or(errs) + let cst = text_to_cst::parse_primary(&format!(r#""{val}""#))?; + let Some(ast) = cst.to_string_literal(&mut errs) else { + return Err(errs); + }; + if errs.is_empty() { + Ok(ast) + } else { + Err(errs) + } } /// parse an identifier /// /// Private to this crate. Users outside Core should use `Id`'s `FromStr` impl /// or its constructors -pub(crate) fn parse_ident(id: &str) -> Result> { - let mut errs = Vec::new(); - text_to_cst::parse_ident(id)? - .to_valid_ident(&mut errs) - .ok_or(errs) +pub(crate) fn parse_ident(id: &str) -> Result { + let mut errs = err::ParseErrors::new(); + let cst = text_to_cst::parse_ident(id)?; + let Some(ast) = cst.to_valid_ident(&mut errs) else { + return Err(errs); + }; + if errs.is_empty() { + Ok(ast) + } else { + Err(errs) + } } #[cfg(test)] diff --git a/cedar-policy-core/src/parser/cst_to_ast.rs b/cedar-policy-core/src/parser/cst_to_ast.rs index f1710a9f1..2682e13a4 100644 --- a/cedar-policy-core/src/parser/cst_to_ast.rs +++ b/cedar-policy-core/src/parser/cst_to_ast.rs @@ -35,7 +35,7 @@ // cases where there is a secondary conversion. This prevents any further // cloning. -use super::err::ParseError; +use super::err::{ParseError, ParseErrors}; use super::node::{ASTNode, SourceInfo}; use super::unescape::{to_pattern, to_unescaped_string}; use super::{cst, err}; @@ -50,9 +50,6 @@ use std::collections::{BTreeMap, HashSet}; use std::mem; use std::sync::Arc; -// shortcut for error parameter -type Errs<'a> = &'a mut Vec; - // for storing extension function names per callstyle struct ExtStyles<'a> { functions: HashSet<&'a ast::Name>, @@ -95,7 +92,7 @@ impl ASTNode> { } /// convert `cst::Policies` to `ast::PolicySet` - pub fn to_policyset(&self, errs: Errs<'_>) -> Option { + pub fn to_policyset(&self, errs: &mut ParseErrors) -> Option { let mut pset = ast::PolicySet::new(); let mut complete_set = true; for (policy_id, policy) in self.with_generated_policyids()? { @@ -143,7 +140,7 @@ impl ASTNode> { pub fn to_policy_or_template( &self, id: ast::PolicyID, - errs: Errs<'_>, + errs: &mut ParseErrors, ) -> Option> { let t = self.to_policy_template(id, errs)?; if t.slots().count() == 0 { @@ -155,7 +152,11 @@ impl ASTNode> { } /// Convert `cst::Policy` to an AST `InlinePolicy`. (Will fail if the CST is for a template) - pub fn to_policy(&self, id: ast::PolicyID, errs: Errs<'_>) -> Option { + pub fn to_policy( + &self, + id: ast::PolicyID, + errs: &mut ParseErrors, + ) -> Option { let tp = self.to_policy_template(id, errs)?; match ast::StaticPolicy::try_from(tp) { Ok(p) => Some(p), @@ -168,7 +169,11 @@ impl ASTNode> { /// Convert `cst::Policy` to `ast::Template`. Works for inline policies as /// well, which will become templates with 0 slots - pub fn to_policy_template(&self, id: ast::PolicyID, errs: Errs<'_>) -> Option { + pub fn to_policy_template( + &self, + id: ast::PolicyID, + errs: &mut ParseErrors, + ) -> Option { let (src, maybe_policy) = self.as_inner_pair(); // return right away if there's no data, parse provided error let policy = maybe_policy?; @@ -237,7 +242,7 @@ impl cst::Policy { /// get the head constraints from the `cst::Policy` pub fn extract_head( &self, - errs: Errs<'_>, + errs: &mut ParseErrors, ) -> ( Option, Option, @@ -280,7 +285,7 @@ impl cst::Policy { impl ASTNode> { /// Get the (k, v) pair for the annotation. Critically, this checks validity /// for the strings and does unescaping - pub fn to_kv_pair(&self, errs: Errs<'_>) -> Option<(ast::Id, SmolStr)> { + pub fn to_kv_pair(&self, errs: &mut ParseErrors) -> Option<(ast::Id, SmolStr)> { let maybe_anno = self.as_inner(); // return right away if there's no data, parse provided error let anno = maybe_anno?; @@ -308,7 +313,7 @@ impl ASTNode> { impl ASTNode> { /// Convert `cst::Ident` to `ast::Id`. Fails for reserved or invalid identifiers - pub fn to_valid_ident(&self, errs: Errs<'_>) -> Option { + pub fn to_valid_ident(&self, errs: &mut ParseErrors) -> Option { let maybe_ident = self.as_inner(); // return right away if there's no data, parse provided error let ident = maybe_ident?; @@ -338,7 +343,7 @@ impl ASTNode> { } /// effect - pub(crate) fn to_effect(&self, errs: Errs<'_>) -> Option { + pub(crate) fn to_effect(&self, errs: &mut ParseErrors) -> Option { let maybe_effect = self.as_inner(); // return right away if there's no data, parse provided error let effect = maybe_effect?; @@ -354,7 +359,7 @@ impl ASTNode> { } } } - pub(crate) fn to_cond_is_when(&self, errs: Errs<'_>) -> Option { + pub(crate) fn to_cond_is_when(&self, errs: &mut ParseErrors) -> Option { let maybe_cond = self.as_inner(); // return right away if there's no data, parse provided error let cond = maybe_cond?; @@ -371,7 +376,7 @@ impl ASTNode> { } } - fn to_var(&self, errs: Errs<'_>) -> Option { + fn to_var(&self, errs: &mut ParseErrors) -> Option { let maybe_ident = self.as_inner(); match maybe_ident { Some(cst::Ident::Principal) => Some(ast::Var::Principal), @@ -396,7 +401,7 @@ impl ast::Id { &self, e: ast::Expr, mut args: Vec, - errs: Errs<'_>, + errs: &mut ParseErrors, l: SourceInfo, ) -> Option { let mut adj_args = args.iter_mut().peekable(); @@ -439,7 +444,7 @@ enum PrincipalOrResource { } impl ASTNode> { - fn to_principal_constraint(&self, errs: Errs<'_>) -> Option { + fn to_principal_constraint(&self, errs: &mut ParseErrors) -> Option { match self.to_principal_or_resource_constraint(errs)? { PrincipalOrResource::Principal(p) => Some(p), PrincipalOrResource::Resource(_) => { @@ -451,7 +456,7 @@ impl ASTNode> { } } - fn to_resource_constraint(&self, errs: Errs<'_>) -> Option { + fn to_resource_constraint(&self, errs: &mut ParseErrors) -> Option { match self.to_principal_or_resource_constraint(errs)? { PrincipalOrResource::Principal(_) => { errs.push(err::ParseError::ToAST( @@ -463,7 +468,10 @@ impl ASTNode> { } } - fn to_principal_or_resource_constraint(&self, errs: Errs<'_>) -> Option { + fn to_principal_or_resource_constraint( + &self, + errs: &mut ParseErrors, + ) -> Option { let maybe_vardef = self.as_inner(); // return right away if there's no data, parse provided error let vardef = maybe_vardef?; @@ -516,7 +524,7 @@ impl ASTNode> { } } - fn to_action_constraint(&self, errs: Errs<'_>) -> Option { + fn to_action_constraint(&self, errs: &mut ParseErrors) -> Option { let maybe_vardef = self.as_inner(); let vardef = maybe_vardef?; @@ -577,13 +585,13 @@ impl ASTNode> { fn action_type_error_msg(euid: &EntityUID) -> ParseError { let msg = format!("Expected an EntityUID with the type `Action`. Got: {euid}"); - ParseError::ToCST(msg) + ParseError::ToAST(msg) } /// 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, -) -> Result> { +) -> Result { match a { ActionConstraint::Any => Ok(a), ActionConstraint::In(ref euids) => { @@ -604,7 +612,7 @@ fn action_constraint_contains_only_action_types( if euid_has_action_type(euid) { Ok(a) } else { - Err(vec![action_type_error_msg(euid)]) + Err(action_type_error_msg(euid).into()) } } } @@ -621,7 +629,7 @@ fn euid_has_action_type(euid: &EntityUID) -> bool { impl ASTNode> { /// to expr - fn to_expr(&self, errs: Errs<'_>) -> Option { + fn to_expr(&self, errs: &mut ParseErrors) -> Option { let (src, maybe_cond) = self.as_inner_pair(); // return right away if there's no data, parse provided error let cond = maybe_cond?; @@ -648,7 +656,7 @@ impl ASTNode> { } impl ASTNode> { - pub(crate) fn as_valid_string(&self, errs: Errs<'_>) -> Option<&SmolStr> { + pub(crate) fn as_valid_string(&self, errs: &mut ParseErrors) -> Option<&SmolStr> { let id = self.as_inner(); // return right away if there's no data, parse provided error let id = id?; @@ -684,7 +692,7 @@ pub(crate) enum ExprOrSpecial<'a> { } impl ExprOrSpecial<'_> { - fn into_expr(self, errs: Errs<'_>) -> Option { + fn into_expr(self, errs: &mut ParseErrors) -> Option { match self { Self::Expr(e) => Some(e), Self::Var(v, l) => Some(construct_expr_var(v, l)), @@ -709,7 +717,7 @@ impl ExprOrSpecial<'_> { } /// Variables, names (with no prefixes), and string literals can all be used as record attributes - pub(crate) fn into_valid_attr(self, errs: Errs<'_>) -> Option { + pub(crate) fn into_valid_attr(self, errs: &mut ParseErrors) -> Option { match self { Self::Var(var, _) => Some(construct_string_from_var(var)), Self::Name(name) => name.into_valid_attr(errs), @@ -731,7 +739,7 @@ impl ExprOrSpecial<'_> { } } - fn into_pattern(self, errs: Errs<'_>) -> Option> { + fn into_pattern(self, errs: &mut ParseErrors) -> Option> { match self { Self::StrLit(s, _) => match to_pattern(s) { Ok(pat) => Some(pat), @@ -763,7 +771,7 @@ impl ExprOrSpecial<'_> { } } /// to string literal - fn into_string_literal(self, errs: Errs<'_>) -> Option { + fn into_string_literal(self, errs: &mut ParseErrors) -> Option { match self { Self::StrLit(s, _) => match to_unescaped_string(s) { Ok(s) => Some(s), @@ -798,19 +806,19 @@ impl ExprOrSpecial<'_> { impl ASTNode> { /// to ref - fn to_ref(&self, var: ast::Var, errs: Errs<'_>) -> Option { + fn to_ref(&self, var: ast::Var, errs: &mut ParseErrors) -> Option { self.to_ref_or_refs::(errs, var).map(|x| x.0) } - fn to_ref_or_slot(&self, errs: Errs<'_>, var: ast::Var) -> Option { + fn to_ref_or_slot(&self, errs: &mut ParseErrors, var: ast::Var) -> Option { self.to_ref_or_refs::(errs, var) } - fn to_refs(&self, errs: Errs<'_>, var: ast::Var) -> Option { + fn to_refs(&self, errs: &mut ParseErrors, var: ast::Var) -> Option { self.to_ref_or_refs::(errs, var) } - fn to_ref_or_refs(&self, errs: Errs<'_>, var: ast::Var) -> Option { + fn to_ref_or_refs(&self, errs: &mut ParseErrors, var: ast::Var) -> Option { let maybe_expr = self.as_inner(); let expr = &*maybe_expr?.expr; match expr { @@ -826,10 +834,10 @@ impl ASTNode> { } /// convert `cst::Expr` to `ast::Expr` - pub fn to_expr(&self, errs: Errs<'_>) -> Option { + pub fn to_expr(&self, errs: &mut ParseErrors) -> Option { self.to_expr_or_special(errs)?.into_expr(errs) } - pub(crate) fn to_expr_or_special(&self, errs: Errs<'_>) -> Option> { + pub(crate) fn to_expr_or_special(&self, errs: &mut ParseErrors) -> Option> { let (src, maybe_expr) = self.as_inner_pair(); // return right away if there's no data, parse provided error let expr = &*maybe_expr?.expr; @@ -857,9 +865,9 @@ impl ASTNode> { /// or runtime data. trait RefKind: Sized { fn err_string() -> &'static str; - fn create_single_ref(e: EntityUID, errs: Errs<'_>) -> Option; - fn create_multiple_refs(es: Vec, errs: Errs<'_>) -> Option; - fn create_slot(errs: Errs<'_>) -> Option; + fn create_single_ref(e: EntityUID, errs: &mut ParseErrors) -> Option; + fn create_multiple_refs(es: Vec, errs: &mut ParseErrors) -> Option; + fn create_slot(errs: &mut ParseErrors) -> Option; } struct SingleEntity(pub EntityUID); @@ -869,18 +877,18 @@ impl RefKind for SingleEntity { "entity uid" } - fn create_single_ref(e: EntityUID, _errs: Errs<'_>) -> Option { + fn create_single_ref(e: EntityUID, _errs: &mut ParseErrors) -> Option { Some(SingleEntity(e)) } - fn create_multiple_refs(_es: Vec, errs: Errs<'_>) -> Option { + fn create_multiple_refs(_es: Vec, errs: &mut ParseErrors) -> Option { errs.push(err::ParseError::ToAST( "expected single entity uid, got a set of entity uids".to_string(), )); None } - fn create_slot(errs: Errs<'_>) -> Option { + fn create_slot(errs: &mut ParseErrors) -> Option { errs.push(err::ParseError::ToAST( "expected a single entity uid, got a template slot".to_string(), )); @@ -893,15 +901,15 @@ impl RefKind for EntityReference { "entity uid or template slot" } - fn create_slot(_: Errs<'_>) -> Option { + fn create_slot(_: &mut ParseErrors) -> Option { Some(EntityReference::Slot) } - fn create_single_ref(e: EntityUID, _errs: Errs<'_>) -> Option { + fn create_single_ref(e: EntityUID, _errs: &mut ParseErrors) -> Option { Some(EntityReference::euid(e)) } - fn create_multiple_refs(_es: Vec, errs: Errs<'_>) -> Option { + fn create_multiple_refs(_es: Vec, errs: &mut ParseErrors) -> Option { errs.push(err::ParseError::ToAST( "expected single entity uid or template slot, got a set of entity uids".to_string(), )); @@ -921,22 +929,22 @@ impl RefKind for OneOrMultipleRefs { "entity uid, set of entity uids, or template slot" } - fn create_slot(errs: Errs<'_>) -> Option { + fn create_slot(errs: &mut ParseErrors) -> Option { errs.push(err::ParseError::ToAST("Unexpected slot".to_string())); None } - fn create_single_ref(e: EntityUID, _errs: Errs<'_>) -> Option { + fn create_single_ref(e: EntityUID, _errs: &mut ParseErrors) -> Option { Some(OneOrMultipleRefs::Single(e)) } - fn create_multiple_refs(es: Vec, _errs: Errs<'_>) -> Option { + fn create_multiple_refs(es: Vec, _errs: &mut ParseErrors) -> Option { Some(OneOrMultipleRefs::Multiple(es)) } } impl ASTNode> { - fn to_expr_or_special(&self, errs: Errs<'_>) -> Option> { + fn to_expr_or_special(&self, errs: &mut ParseErrors) -> Option> { let (src, maybe_or) = self.as_inner_pair(); // return right away if there's no data, parse provided error let or = maybe_or?; @@ -957,7 +965,7 @@ impl ASTNode> { } } - fn to_ref_or_refs(&self, errs: Errs<'_>, var: ast::Var) -> Option { + fn to_ref_or_refs(&self, errs: &mut ParseErrors, var: ast::Var) -> Option { let maybe_or = self.as_inner(); let or = maybe_or?; match or.extended.len() { @@ -974,7 +982,7 @@ impl ASTNode> { } impl ASTNode> { - fn to_ref_or_refs(&self, errs: Errs<'_>, var: ast::Var) -> Option { + fn to_ref_or_refs(&self, errs: &mut ParseErrors, var: ast::Var) -> Option { let maybe_and = self.as_inner(); let and = maybe_and?; match and.extended.len() { @@ -989,10 +997,10 @@ impl ASTNode> { } } - fn to_expr(&self, errs: Errs<'_>) -> Option { + fn to_expr(&self, errs: &mut ParseErrors) -> Option { self.to_expr_or_special(errs)?.into_expr(errs) } - fn to_expr_or_special(&self, errs: Errs<'_>) -> Option> { + fn to_expr_or_special(&self, errs: &mut ParseErrors) -> Option> { let (src, maybe_and) = self.as_inner_pair(); // return right away if there's no data, parse provided error let and = maybe_and?; @@ -1015,7 +1023,7 @@ impl ASTNode> { } impl ASTNode> { - fn to_ref_or_refs(&self, errs: Errs<'_>, var: ast::Var) -> Option { + fn to_ref_or_refs(&self, errs: &mut ParseErrors, var: ast::Var) -> Option { let maybe_rel = self.as_inner(); match maybe_rel? { cst::Relation::Common { initial, extended } => match extended.len() { @@ -1045,10 +1053,10 @@ impl ASTNode> { } } - fn to_expr(&self, errs: Errs<'_>) -> Option { + fn to_expr(&self, errs: &mut ParseErrors) -> Option { self.to_expr_or_special(errs)?.into_expr(errs) } - fn to_expr_or_special(&self, errs: Errs<'_>) -> Option> { + fn to_expr_or_special(&self, errs: &mut ParseErrors) -> Option> { let (src, maybe_rel) = self.as_inner_pair(); // return right away if there's no data, parse provided error let rel = maybe_rel?; @@ -1108,7 +1116,7 @@ impl ASTNode> { } impl ASTNode> { - fn to_ref_or_refs(&self, errs: Errs<'_>, var: ast::Var) -> Option { + fn to_ref_or_refs(&self, errs: &mut ParseErrors, var: ast::Var) -> Option { let maybe_add = self.as_inner(); let add = maybe_add?; match add.extended.len() { @@ -1123,10 +1131,10 @@ impl ASTNode> { } } - fn to_expr(&self, errs: Errs<'_>) -> Option { + fn to_expr(&self, errs: &mut ParseErrors) -> Option { self.to_expr_or_special(errs)?.into_expr(errs) } - fn to_expr_or_special(&self, errs: Errs<'_>) -> Option> { + fn to_expr_or_special(&self, errs: &mut ParseErrors) -> Option> { let (src, maybe_add) = self.as_inner_pair(); // return right away if there's no data, parse provided error let add = maybe_add?; @@ -1151,7 +1159,7 @@ impl ASTNode> { } impl ASTNode> { - fn to_ref_or_refs(&self, errs: Errs<'_>, var: ast::Var) -> Option { + fn to_ref_or_refs(&self, errs: &mut ParseErrors, var: ast::Var) -> Option { let maybe_mult = self.as_inner(); let mult = maybe_mult?; match mult.extended.len() { @@ -1166,10 +1174,10 @@ impl ASTNode> { } } - fn to_expr(&self, errs: Errs<'_>) -> Option { + fn to_expr(&self, errs: &mut ParseErrors) -> Option { self.to_expr_or_special(errs)?.into_expr(errs) } - fn to_expr_or_special(&self, errs: Errs<'_>) -> Option> { + fn to_expr_or_special(&self, errs: &mut ParseErrors) -> Option> { let (src, maybe_mult) = self.as_inner_pair(); // return right away if there's no data, parse provided error let mult = maybe_mult?; @@ -1254,7 +1262,7 @@ impl ASTNode> { } impl ASTNode> { - fn to_ref_or_refs(&self, errs: Errs<'_>, var: ast::Var) -> Option { + fn to_ref_or_refs(&self, errs: &mut ParseErrors, var: ast::Var) -> Option { let maybe_unary = self.as_inner(); let unary = maybe_unary?; match &unary.op { @@ -1268,10 +1276,10 @@ impl ASTNode> { } } - fn to_expr(&self, errs: Errs<'_>) -> Option { + fn to_expr(&self, errs: &mut ParseErrors) -> Option { self.to_expr_or_special(errs)?.into_expr(errs) } - fn to_expr_or_special(&self, errs: Errs<'_>) -> Option> { + fn to_expr_or_special(&self, errs: &mut ParseErrors) -> Option> { let (src, maybe_unary) = self.as_inner_pair(); // return right away if there's no data, parse provided error let unary = maybe_unary?; @@ -1365,7 +1373,7 @@ impl ASTNode> { } } - fn to_ref_or_refs(&self, errs: Errs<'_>, var: ast::Var) -> Option { + fn to_ref_or_refs(&self, errs: &mut ParseErrors, var: ast::Var) -> Option { let maybe_mem = self.as_inner(); let mem = maybe_mem?; match mem.access.len() { @@ -1379,7 +1387,7 @@ impl ASTNode> { } } - fn to_expr_or_special(&self, errs: Errs<'_>) -> Option> { + fn to_expr_or_special(&self, errs: &mut ParseErrors) -> Option> { let (src, maybe_mem) = self.as_inner_pair(); // return right away if there's no data, parse provided error let mem = maybe_mem?; @@ -1593,7 +1601,7 @@ impl ASTNode> { } impl ASTNode> { - fn to_access(&self, errs: Errs<'_>) -> Option { + fn to_access(&self, errs: &mut ParseErrors) -> Option { let maybe_acc = self.as_inner(); // return right away if there's no data, parse provided error let acc = maybe_acc?; @@ -1620,7 +1628,7 @@ impl ASTNode> { } impl ASTNode> { - fn to_ref_or_refs(&self, errs: Errs<'_>, var: ast::Var) -> Option { + fn to_ref_or_refs(&self, errs: &mut ParseErrors, var: ast::Var) -> Option { let maybe_prim = self.as_inner(); let prim = maybe_prim?; let r: Result, String> = match prim { @@ -1661,10 +1669,10 @@ impl ASTNode> { } } - pub(crate) fn to_expr(&self, errs: Errs<'_>) -> Option { + pub(crate) fn to_expr(&self, errs: &mut ParseErrors) -> Option { self.to_expr_or_special(errs)?.into_expr(errs) } - fn to_expr_or_special(&self, errs: Errs<'_>) -> Option> { + fn to_expr_or_special(&self, errs: &mut ParseErrors) -> Option> { let (src, maybe_prim) = self.as_inner_pair(); // return right away if there's no data, parse provided error let prim = maybe_prim?; @@ -1676,7 +1684,7 @@ impl ASTNode> { #[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(v) = n.to_var(&mut Vec::new()) { + if let Some(v) = n.to_var(&mut ParseErrors::new()) { Some(ExprOrSpecial::Var(v, src.clone())) } else if let Some(n) = n.to_name(errs) { Some(ExprOrSpecial::Name(n)) @@ -1709,7 +1717,7 @@ impl ASTNode> { /// convert `cst::Primary` representing a string literal to a `SmolStr`. /// Fails (and adds to `errs`) if the `Primary` wasn't a string literal. - pub fn to_string_literal(&self, errs: Errs<'_>) -> Option { + pub fn to_string_literal(&self, errs: &mut ParseErrors) -> Option { let maybe_prim = self.as_inner(); // return right away if there's no data, parse provided error let prim = maybe_prim?; @@ -1727,7 +1735,7 @@ impl ASTNode> { } impl ASTNode> { - fn to_expr(&self, _errs: Errs<'_>) -> Option { + fn to_expr(&self, _errs: &mut ParseErrors) -> Option { let (src, s) = self.as_inner_pair(); s.map(|s| { ast::ExprBuilder::new() @@ -1742,7 +1750,7 @@ impl ASTNode> { impl ASTNode> { /// Build type constraints - fn to_type_constraint(&self, errs: Errs<'_>) -> Option { + fn to_type_constraint(&self, errs: &mut ParseErrors) -> Option { let (src, maybe_name) = self.as_inner_pair(); match maybe_name { Some(_) => { @@ -1755,7 +1763,7 @@ impl ASTNode> { } } - pub(crate) fn to_name(&self, errs: Errs<'_>) -> Option { + pub(crate) fn to_name(&self, errs: &mut ParseErrors) -> Option { let maybe_name = self.as_inner(); // return right away if there's no data, parse provided error let name = maybe_name?; @@ -1773,7 +1781,7 @@ impl ASTNode> { _ => None, } } - fn to_ident(&self, errs: Errs<'_>) -> Option<&cst::Ident> { + fn to_ident(&self, errs: &mut ParseErrors) -> Option<&cst::Ident> { let maybe_name = self.as_inner(); // return right away if there's no data, parse provided error let name = maybe_name?; @@ -1792,7 +1800,7 @@ impl ASTNode> { name.name.as_inner() } - fn to_var(&self, errs: Errs<'_>) -> Option { + fn to_var(&self, errs: &mut ParseErrors) -> Option { let name = self.to_ident(errs)?; match name { @@ -1813,7 +1821,7 @@ impl ASTNode> { impl ast::Name { /// Convert the `Name` into a `String` attribute, which fails if it had any namespaces - fn into_valid_attr(self, errs: Errs<'_>) -> Option { + fn into_valid_attr(self, errs: &mut ParseErrors) -> Option { if !self.path.is_empty() { errs.push(err::ParseError::ToAST( "A name with a path is not a valid attribute".to_string(), @@ -1824,7 +1832,12 @@ impl ast::Name { } } - fn into_func(self, args: Vec, errs: Errs<'_>, l: SourceInfo) -> Option { + fn into_func( + self, + args: Vec, + errs: &mut ParseErrors, + l: SourceInfo, + ) -> Option { // error on standard methods if self.path.is_empty() { let id = self.id.as_ref(); @@ -1853,7 +1866,7 @@ impl ast::Name { impl ASTNode> { /// convert `cst::Ref` to `ast::EntityUID` - pub fn to_ref(&self, errs: Errs<'_>) -> Option { + pub fn to_ref(&self, errs: &mut ParseErrors) -> Option { let maybe_ref = self.as_inner(); // return right away if there's no data, parse provided error let refr = maybe_ref?; @@ -1890,14 +1903,14 @@ impl ASTNode> { } } } - fn to_expr(&self, errs: Errs<'_>) -> Option { + fn to_expr(&self, errs: &mut ParseErrors) -> Option { self.to_ref(errs) .map(|euid| construct_expr_ref(euid, self.info.clone())) } } impl ASTNode> { - fn to_expr_or_special(&self, errs: Errs<'_>) -> Option> { + fn to_expr_or_special(&self, errs: &mut ParseErrors) -> Option> { let (src, maybe_lit) = self.as_inner_pair(); // return right away if there's no data, parse provided error let lit = maybe_lit?; @@ -1923,7 +1936,7 @@ impl ASTNode> { } impl ASTNode> { - fn to_init(&self, errs: Errs<'_>) -> Option<(SmolStr, ast::Expr)> { + fn to_init(&self, errs: &mut ParseErrors) -> Option<(SmolStr, ast::Expr)> { let (_src, maybe_lit) = self.as_inner_pair(); // return right away if there's no data, parse provided error let lit = maybe_lit?; @@ -2139,13 +2152,13 @@ mod tests { use super::*; use crate::{ ast::Expr, - parser::{err::MultipleParseErrors, *}, + parser::{err::ParseErrors, *}, }; use std::str::FromStr; #[test] fn show_expr1() { - let mut errs = Vec::new(); + let mut errs = ParseErrors::new(); let expr: ast::Expr = text_to_cst::parse_expr( r#" if 7 then 6 > 5 else !5 || "thursday" && ((8) >= "fish") @@ -2161,7 +2174,7 @@ mod tests { #[test] fn show_expr2() { - let mut errs = Vec::new(); + let mut errs = ParseErrors::new(); let expr: ast::Expr = text_to_cst::parse_expr( r#" [2,3,4].foo["hello"] @@ -2177,7 +2190,7 @@ mod tests { #[test] fn show_expr3() { // these exprs are ill-typed, but are allowed by the parser - let mut errs = Vec::new(); + let mut errs = ParseErrors::new(); let expr = text_to_cst::parse_expr( r#" "first".some_ident @@ -2226,7 +2239,7 @@ mod tests { #[test] fn show_expr4() { - let mut errs = Vec::new(); + let mut errs = ParseErrors::new(); let expr: ast::Expr = text_to_cst::parse_expr( r#" {"one":1,"two":2} has one @@ -2246,7 +2259,7 @@ mod tests { #[test] fn show_expr5() { - let mut errs = Vec::new(); + let mut errs = ParseErrors::new(); let expr = text_to_cst::parse_expr( r#" {"one":1,"two":2}.one @@ -2281,7 +2294,7 @@ mod tests { } // accessing a record with a non-identifier attribute - let mut errs = Vec::new(); + let mut errs = ParseErrors::new(); let expr: ast::Expr = text_to_cst::parse_expr( r#" {"this is a valid map key+.-_%()":1,"two":2}["this is a valid map key+.-_%()"] @@ -2301,7 +2314,7 @@ mod tests { #[test] fn show_expr6_idents() { - let mut errs = Vec::new(); + let mut errs = ParseErrors::new(); let expr = text_to_cst::parse_expr( r#" {if true then a else b:"b"} || @@ -2352,7 +2365,7 @@ mod tests { #[test] fn reserved_idents1() { - let mut errs = Vec::new(); + let mut errs = ParseErrors::new(); let parse = text_to_cst::parse_expr( r#" The::true::path::to::"enlightenment".false @@ -2366,7 +2379,7 @@ mod tests { assert!(errs.len() == 2); assert!(convert.is_none()); - let mut errs = Vec::new(); + let mut errs = ParseErrors::new(); let parse = text_to_cst::parse_expr( r#" if {if: true}.if then {"if":false}["if"] else {when:true}.permit @@ -2383,7 +2396,7 @@ mod tests { #[test] fn reserved_idents2() { - let mut errs = Vec::new(); + let mut errs = ParseErrors::new(); let parse = text_to_cst::parse_expr( r#" if {where: true}.like || {has:false}.in then {"like":false}["in"] else {then:true}.else @@ -2400,7 +2413,7 @@ mod tests { #[test] fn show_policy1() { - let mut errs = Vec::new(); + let mut errs = ParseErrors::new(); let parse = text_to_cst::parse_policy( r#" permit(principal:p,action:a,resource:r)when{w}unless{u}advice{"doit"}; @@ -2419,7 +2432,7 @@ mod tests { #[test] fn show_policy2() { - let mut errs = Vec::new(); + let mut errs = ParseErrors::new(); let parse = text_to_cst::parse_policy( r#" permit(principal,action,resource)when{true}; @@ -2436,7 +2449,7 @@ mod tests { #[test] fn show_policy3() { - let mut errs = Vec::new(); + let mut errs = ParseErrors::new(); let parse = text_to_cst::parse_policy( r#" permit(principal in User::"jane",action,resource); @@ -2455,7 +2468,7 @@ mod tests { #[test] fn show_policy4() { - let mut errs = Vec::new(); + let mut errs = ParseErrors::new(); let parse = text_to_cst::parse_policy( r#" forbid(principal in User::"jane",action,resource)unless{ @@ -2475,7 +2488,7 @@ mod tests { #[test] fn policy_annotations() { // common use-case - let mut errs = Vec::new(); + let mut errs = ParseErrors::new(); let policy = text_to_cst::parse_policy( r#" @anno("good annotation")permit(principal,action,resource); @@ -2490,7 +2503,7 @@ mod tests { ); // duplication is error - let mut errs = Vec::new(); + let mut errs = ParseErrors::new(); let policy = text_to_cst::parse_policy( r#" @anno("good annotation") @@ -2506,7 +2519,7 @@ mod tests { assert!(errs.len() == 1); // can have multiple annotations - let mut errs = Vec::new(); + let mut errs = ParseErrors::new(); let policyset = text_to_cst::parse_policies( r#" @anno1("first") @@ -2577,7 +2590,7 @@ mod tests { #[test] fn fail_head1() { - let mut errs = Vec::new(); + let mut errs = ParseErrors::new(); let parse = text_to_cst::parse_policy( r#" permit( @@ -2597,7 +2610,7 @@ mod tests { #[test] fn fail_head2() { - let mut errs = Vec::new(); + let mut errs = ParseErrors::new(); let parse = text_to_cst::parse_policy( r#" permit( @@ -2617,7 +2630,7 @@ mod tests { #[test] fn fail_head3() { - let mut errs = Vec::new(); + let mut errs = ParseErrors::new(); let parse = text_to_cst::parse_policy( r#" permit(principal,action,resource,context); @@ -2631,7 +2644,7 @@ mod tests { #[test] fn method_call2() { - let mut errs = Vec::new(); + let mut errs = ParseErrors::new(); let e = text_to_cst::parse_expr( r#" principal.contains(resource) @@ -2661,7 +2674,7 @@ mod tests { #[test] fn construct_record1() { - let mut errs = Vec::new(); + let mut errs = ParseErrors::new(); let e = text_to_cst::parse_expr( r#" {one:"one"} @@ -2750,7 +2763,7 @@ mod tests { #[test] fn construct_invalid_get() { - let mut errs = Vec::new(); + let mut errs = ParseErrors::new(); let e = text_to_cst::parse_expr( r#" {"one":1, "two":"two"}[0] @@ -2799,7 +2812,7 @@ mod tests { #[test] fn construct_has() { - let mut errs = Vec::new(); + let mut errs = ParseErrors::new(); let expr: ast::Expr = text_to_cst::parse_expr( r#" {"one":1,"two":2} has "arbitrary+ _string" @@ -2816,7 +2829,7 @@ mod tests { _ => panic!("should be a has expr"), } - let mut errs = Vec::new(); + let mut errs = ParseErrors::new(); let e = text_to_cst::parse_expr( r#" {"one":1,"two":2} has 1 @@ -2832,7 +2845,7 @@ mod tests { #[test] fn construct_like() { - let mut errs = Vec::new(); + let mut errs = ParseErrors::new(); let expr: ast::Expr = text_to_cst::parse_expr( r#" "354 hams" like "*5*" @@ -2860,7 +2873,7 @@ mod tests { assert!(e.is_none()); assert!(errs.len() == 1); - let mut errs = Vec::new(); + let mut errs = ParseErrors::new(); let expr: ast::Expr = text_to_cst::parse_expr( r#" "string\\with\\backslashes" like "string\\with\\backslashes" @@ -2918,7 +2931,7 @@ mod tests { _ => panic!("should be a like expr"), } - let mut errs = Vec::new(); + let mut errs = ParseErrors::new(); let expr: ast::Expr = text_to_cst::parse_expr( r#" "string\\*with\\*backslashes\\*and\\*stars" like "string\\\*with\\\*backslashes\\\*and\\\*stars" @@ -2987,7 +3000,7 @@ mod tests { // entities can be accessed using the same notation as records // ok - let mut errs = Vec::new(); + let mut errs = ParseErrors::new(); let expr: ast::Expr = text_to_cst::parse_expr( r#" User::"jane" has age @@ -3004,7 +3017,7 @@ mod tests { } // ok - let mut errs = Vec::new(); + let mut errs = ParseErrors::new(); let expr: ast::Expr = text_to_cst::parse_expr( r#" User::"jane" has "arbitrary+ _string" @@ -3021,7 +3034,7 @@ mod tests { } // not ok: 1 is not a valid attribute - let mut errs = Vec::new(); + let mut errs = ParseErrors::new(); let e = text_to_cst::parse_expr( r#" User::"jane" has 1 @@ -3033,7 +3046,7 @@ mod tests { assert!(errs.len() == 1); // ok - let mut errs = Vec::new(); + let mut errs = ParseErrors::new(); let expr: ast::Expr = text_to_cst::parse_expr( r#" User::"jane".age @@ -3050,7 +3063,7 @@ mod tests { } // ok - let mut errs = Vec::new(); + let mut errs = ParseErrors::new(); let expr: ast::Expr = text_to_cst::parse_expr( r#" User::"jane"["arbitrary+ _string"] @@ -3067,7 +3080,7 @@ mod tests { } // not ok: age is not a string literal - let mut errs = Vec::new(); + let mut errs = ParseErrors::new(); let e = text_to_cst::parse_expr( r#" User::"jane"[age] @@ -3081,7 +3094,7 @@ mod tests { #[test] fn relational_ops1() { - let mut errs = Vec::new(); + let mut errs = ParseErrors::new(); let e = text_to_cst::parse_expr( r#" 3 >= 2 >= 1 @@ -3129,7 +3142,7 @@ mod tests { #[test] fn arithmetic() { - let mut errs = Vec::new(); + let mut errs = ParseErrors::new(); let e = text_to_cst::parse_expr(r#" 2 + 4 "#) // the cst should be acceptable .expect("parse error") @@ -3242,7 +3255,7 @@ mod tests { #[test] fn template_tests() { for src in CORRECT_TEMPLATES { - let mut errs = Vec::new(); + let mut errs = ParseErrors::new(); let e = text_to_cst::parse_policy(src) .expect("parse_error") .to_policy_template(ast::PolicyID::from_string("i0"), &mut errs); @@ -3274,7 +3287,7 @@ mod tests { #[test] fn test_wrong_template_var() { for src in WRONG_VAR_TEMPLATES { - let mut errs = vec![]; + let mut errs = ParseErrors::new(); let e = text_to_cst::parse_policy(src) .expect("Parse Error") .to_policy_template(ast::PolicyID::from_string("id0"), &mut errs); @@ -3284,7 +3297,7 @@ mod tests { #[test] fn var_type() { - let mut errs = Vec::new(); + let mut errs = ParseErrors::new(); let e = text_to_cst::parse_policy( r#" permit(principal,action,resource); @@ -3460,7 +3473,7 @@ mod tests { Expr::mul(Expr::mul(Expr::var(ast::Var::Principal), 0), -1), ), ] { - let mut errs = Vec::new(); + let mut errs = ParseErrors::new(); let e = text_to_cst::parse_expr(es) .expect("should construct a CST") .to_expr(&mut errs) @@ -3481,7 +3494,7 @@ mod tests { // considered as a constant. "principal * --1", ] { - let mut errs = Vec::new(); + let mut errs = ParseErrors::new(); let e = text_to_cst::parse_expr(es) .expect("should construct a CST") .to_expr(&mut errs); @@ -3528,7 +3541,7 @@ mod tests { ), ), ] { - let mut errs = Vec::new(); + let mut errs = ParseErrors::new(); let e = text_to_cst::parse_expr(es) .expect("should construct a CST") .to_expr(&mut errs) @@ -3565,7 +3578,7 @@ mod tests { Expr::neg(Expr::val(9223372036854775807)), ), ] { - let mut errs = Vec::new(); + let mut errs = ParseErrors::new(); let e = text_to_cst::parse_expr(es) .expect("should construct a CST") .to_expr(&mut errs) @@ -3588,12 +3601,12 @@ mod tests { "Literal 9223372036854775808 is too large", ), ] { - let mut errs = Vec::new(); + let mut errs = ParseErrors::new(); let e = text_to_cst::parse_expr(es) .expect("should construct a CST") .to_expr(&mut errs); assert!(e.is_none()); - assert!(MultipleParseErrors(&errs).to_string().contains(em)); + assert!(errs.to_string().contains(em)); } } } diff --git a/cedar-policy-core/src/parser/err.rs b/cedar-policy-core/src/parser/err.rs index 98e19f588..3dab2c075 100644 --- a/cedar-policy-core/src/parser/err.rs +++ b/cedar-policy-core/src/parser/err.rs @@ -14,51 +14,177 @@ * limitations under the License. */ +use std::collections::HashMap; +use std::error::Error; +use std::fmt::{self, Display, Write}; +use std::iter; +use std::ops::{Deref, DerefMut}; + use lalrpop_util as lalr; -use std::fmt::Display; +use lazy_static::lazy_static; +use miette::{Diagnostic, LabeledSpan, Severity, SourceCode}; use thiserror::Error; +use crate::ast::RestrictedExpressionError; + +use crate::parser::fmt::join_with_conjunction; +use crate::parser::node::ASTNode; + +pub(crate) type RawLocation = usize; +pub(crate) type RawToken<'a> = lalr::lexer::Token<'a>; +pub(crate) type RawUserError = ASTNode; + +pub(crate) type RawParseError<'a> = lalr::ParseError, RawUserError>; +pub(crate) type RawErrorRecovery<'a> = lalr::ErrorRecovery, RawUserError>; + +type OwnedRawParseError = lalr::ParseError; + /// For errors during parsing -#[derive(Debug, Error, PartialEq, Clone)] +#[derive(Clone, Debug, Diagnostic, Error, PartialEq)] pub enum ParseError { - /// Error from the lalrpop parser, no additional information - #[error("{0}")] - ToCST(String), - /// Error in the cst -> ast transform, mostly well-formedness issues + /// Error from the CST parser. + #[error(transparent)] + #[diagnostic(transparent)] + ToCST(#[from] ToCSTError), + /// Error in the CST -> AST transform, mostly well-formedness issues. #[error("poorly formed: {0}")] + #[diagnostic(code(cedar_policy_core::parser::to_ast_err))] ToAST(String), - /// (Potentially) multiple errors. This variant includes a "context" for - /// what we were trying to do when we encountered these errors - #[error("error while {context}: {}", MultipleParseErrors(errs))] - WithContext { - /// What we were trying to do - context: String, - /// Error(s) we encountered while doing it - errs: Vec, - }, - /// Error concerning restricted expressions + /// Error concerning restricted expressions. #[error(transparent)] - RestrictedExpressionError(#[from] crate::ast::RestrictedExpressionError), + RestrictedExpressionError(#[from] RestrictedExpressionError), + /// One or more parse errors occurred while performing a task. + #[error(transparent)] + #[diagnostic(transparent)] + WithContext(#[from] WithContext), +} + +/// Error from the CST parser. +#[derive(Clone, Debug, Error, PartialEq)] +pub struct ToCSTError { + err: OwnedRawParseError, +} + +impl ToCSTError { + pub(crate) fn from_raw_parse_err(err: RawParseError<'_>) -> Self { + Self { + err: err.map_token(|token| token.to_string()), + } + } + + pub(crate) fn from_raw_err_recovery(recovery: RawErrorRecovery<'_>) -> Self { + Self::from_raw_parse_err(recovery.error) + } } -impl From> for ParseError { - fn from(e: lalr::ParseError) -> Self { - ParseError::ToCST(format!("{}", e)) +impl Display for ToCSTError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match &self.err { + OwnedRawParseError::InvalidToken { .. } => write!(f, "invalid token"), + OwnedRawParseError::UnrecognizedEof { .. } => write!(f, "unexpected end of input"), + OwnedRawParseError::UnrecognizedToken { + token: (_, token, _), + .. + } => write!(f, "unexpected token `{token}`"), + OwnedRawParseError::ExtraToken { + token: (_, token, _), + .. + } => write!(f, "extra token `{token}`"), + OwnedRawParseError::User { error } => write!(f, "{error}"), + } } } -impl From> for ParseError { - fn from(e: lalr::ErrorRecovery) -> Self { - e.error.into() +impl Diagnostic for ToCSTError { + fn code(&self) -> Option> { + Some(Box::new("cedar_policy_core::parser::to_cst_error")) } + + fn labels(&self) -> Option + '_>> { + let labeled_span = match &self.err { + OwnedRawParseError::InvalidToken { location } => { + LabeledSpan::underline(*location..*location) + } + OwnedRawParseError::UnrecognizedEof { location, expected } => { + LabeledSpan::new_with_span(expected_to_string(expected), *location..*location) + } + OwnedRawParseError::UnrecognizedToken { + token: (token_start, _, token_end), + expected, + } => LabeledSpan::new_with_span(expected_to_string(expected), *token_start..*token_end), + OwnedRawParseError::ExtraToken { + token: (token_start, _, token_end), + } => LabeledSpan::underline(*token_start..*token_end), + OwnedRawParseError::User { error } => LabeledSpan::underline(error.info.0.clone()), + }; + Some(Box::new(iter::once(labeled_span))) + } +} + +lazy_static! { + /// Keys mirror the token names defined in the `match` block of + /// `grammar.lalrpop`. + static ref FRIENDLY_TOKEN_NAMES: HashMap<&'static str, &'static str> = HashMap::from([ + ("TRUE", "`true`"), + ("FALSE", "`false`"), + ("IF", "`if`"), + ("PERMIT", "`permit`"), + ("FORBID", "`forbid`"), + ("WHEN", "`when`"), + ("UNLESS", "`unless`"), + ("IN", "`in`"), + ("HAS", "`has`"), + ("LIKE", "`like`"), + ("THEN", "`then`"), + ("ELSE", "`else`"), + ("PRINCIPAL", "`principal`"), + ("ACTION", "`action`"), + ("RESOURCE", "`resource`"), + ("CONTEXT", "`context`"), + ("PRINCIPAL_SLOT", "`?principal`"), + ("RESOURCE_SLOT", "`?resource`"), + ("IDENTIFIER", "identifier"), + ("NUMBER", "number"), + ("STRINGLIT", "string literal"), + ]); } -/// if you wrap a `Vec` in this struct, it gains a Display impl -/// that displays each parse error on its own line, indented. -#[derive(Debug, Error)] +fn expected_to_string(expected: &[String]) -> Option { + if expected.is_empty() { + return None; + } + + let mut expected_string = "expected ".to_owned(); + // PANIC SAFETY Shouldn't be `Err` since we're writing strings to a string + #[allow(clippy::expect_used)] + join_with_conjunction(&mut expected_string, "or", expected, |f, token| { + match FRIENDLY_TOKEN_NAMES.get(token.as_str()) { + Some(friendly_token_name) => write!(f, "{}", friendly_token_name), + None => write!(f, "{}", token.replace('"', "`")), + } + }) + .expect("failed to format expected tokens"); + Some(expected_string) +} + +/// Multiple related parse errors. +#[derive(Clone, Debug, Default, PartialEq)] pub struct ParseErrors(pub Vec); impl ParseErrors { + const DESCRIPTION_IF_EMPTY: &'static str = "unknown parse error"; + + /// Constructs a new, empty `ParseErrors`. + pub fn new() -> Self { + ParseErrors(Vec::new()) + } + + /// Constructs a new, empty `ParseErrors` with the specified capacity. + pub fn with_capacity(capacity: usize) -> Self { + ParseErrors(Vec::with_capacity(capacity)) + } + + // TODO(spinda): Can we get rid of this? /// returns a Vec with stringified versions of the ParserErrors pub fn errors_as_strings(&self) -> Vec { self.0 @@ -68,32 +194,125 @@ impl ParseErrors { } } -impl std::fmt::Display for ParseErrors { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - write!(f, "{}", MultipleParseErrors(&self.0)) +impl Display for ParseErrors { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self.first() { + Some(first_err) => Display::fmt(first_err, f), + None => write!(f, "{}", Self::DESCRIPTION_IF_EMPTY), + } + } +} + +impl Error for ParseErrors { + fn source(&self) -> Option<&(dyn Error + 'static)> { + self.first().and_then(Error::source) + } + + #[allow(deprecated)] + fn description(&self) -> &str { + match self.first() { + Some(first_err) => first_err.description(), + None => Self::DESCRIPTION_IF_EMPTY, + } + } + + #[allow(deprecated)] + fn cause(&self) -> Option<&dyn Error> { + self.first().and_then(Error::cause) + } +} + +impl Diagnostic for ParseErrors { + fn related<'a>(&'a self) -> Option + 'a>> { + let mut errs = self.iter().map(|err| err as &dyn Diagnostic); + errs.next().map(move |first_err| match first_err.related() { + Some(first_err_related) => Box::new(first_err_related.chain(errs)), + None => Box::new(errs) as Box>, + }) + } + + fn code<'a>(&'a self) -> Option> { + self.first().and_then(Diagnostic::code) + } + + fn severity(&self) -> Option { + self.first().and_then(Diagnostic::severity) + } + + fn help<'a>(&'a self) -> Option> { + self.first().and_then(Diagnostic::help) + } + + fn url<'a>(&'a self) -> Option> { + self.first().and_then(Diagnostic::url) + } + + fn source_code(&self) -> Option<&dyn SourceCode> { + self.first().and_then(Diagnostic::source_code) + } + + fn labels(&self) -> Option + '_>> { + self.first().and_then(Diagnostic::labels) + } + + fn diagnostic_source(&self) -> Option<&dyn Diagnostic> { + self.first().and_then(Diagnostic::diagnostic_source) + } +} + +impl AsRef> for ParseErrors { + fn as_ref(&self) -> &Vec { + &self.0 + } +} + +impl AsMut> for ParseErrors { + fn as_mut(&mut self) -> &mut Vec { + &mut self.0 + } +} + +impl AsRef<[ParseError]> for ParseErrors { + fn as_ref(&self) -> &[ParseError] { + self.0.as_ref() + } +} + +impl AsMut<[ParseError]> for ParseErrors { + fn as_mut(&mut self) -> &mut [ParseError] { + self.0.as_mut() + } +} + +impl Deref for ParseErrors { + type Target = Vec; + + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +impl DerefMut for ParseErrors { + fn deref_mut(&mut self) -> &mut Self::Target { + &mut self.0 } } impl From for ParseErrors { - fn from(e: ParseError) -> ParseErrors { - ParseErrors(vec![e]) + fn from(err: ParseError) -> Self { + vec![err].into() } } -/// Like [`ParseErrors`], but you don't have to own the `Vec` -#[derive(Debug, Error)] -pub struct MultipleParseErrors<'a>(pub &'a [ParseError]); +impl From for ParseErrors { + fn from(err: ToCSTError) -> Self { + ParseError::from(err).into() + } +} -impl<'a> std::fmt::Display for MultipleParseErrors<'a> { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - if self.0.is_empty() { - write!(f, "no errors found") - } else { - for err in self.0 { - write!(f, "\n {}", err)?; - } - Ok(()) - } +impl From for ParseErrors { + fn from(err: RestrictedExpressionError) -> Self { + ParseError::from(err).into() } } @@ -135,3 +354,48 @@ impl<'a> IntoIterator for &'a mut ParseErrors { self.0.iter_mut() } } + +/// One or more parse errors occurred while performing a task. +#[derive(Clone, Debug, Default, Error, PartialEq)] +#[error("{context}")] +pub struct WithContext { + /// What we were trying to do. + pub context: String, + /// Error(s) we encountered while doing it. + #[source] + pub errs: ParseErrors, +} + +impl Diagnostic for WithContext { + fn code<'a>(&'a self) -> Option> { + self.errs.code() + } + + fn severity(&self) -> Option { + self.errs.severity() + } + + fn help<'a>(&'a self) -> Option> { + self.errs.help() + } + + fn url<'a>(&'a self) -> Option> { + self.errs.url() + } + + fn source_code(&self) -> Option<&dyn SourceCode> { + self.errs.source_code() + } + + fn labels(&self) -> Option + '_>> { + self.errs.labels() + } + + fn related<'a>(&'a self) -> Option + 'a>> { + self.errs.related() + } + + fn diagnostic_source(&self) -> Option<&dyn Diagnostic> { + self.errs.diagnostic_source() + } +} diff --git a/cedar-policy-core/src/parser/fmt.rs b/cedar-policy-core/src/parser/fmt.rs index 565b72dcd..0248beaa6 100644 --- a/cedar-policy-core/src/parser/fmt.rs +++ b/cedar-policy-core/src/parser/fmt.rs @@ -14,9 +14,10 @@ * limitations under the License. */ +use std::fmt::{self, Write}; + use super::cst::*; use super::node::ASTNode; -use std::fmt; /// Helper struct to handle non-existent nodes struct View<'a, T>(&'a ASTNode>); @@ -427,6 +428,41 @@ impl std::fmt::Display for Slot { } } +/// Format an iterator as a natural-language string, separating items with +/// commas and a conjunction (e.g., "and", "or") between the last two items. +pub(crate) fn join_with_conjunction( + f: &mut W, + conjunction: &str, + items: impl IntoIterator, + fmt_item: impl Fn(&mut W, T) -> fmt::Result, +) -> fmt::Result { + let mut iter = items.into_iter().peekable(); + + if let Some(first_item) = iter.next() { + fmt_item(f, first_item)?; + + if let Some(second_item) = iter.next() { + match iter.peek() { + Some(_) => write!(f, ", "), + None => write!(f, " {conjunction} "), + }?; + + fmt_item(f, second_item)?; + + while let Some(item) = iter.next() { + match iter.peek() { + Some(_) => write!(f, ", "), + None => write!(f, ", {conjunction} "), + }?; + + fmt_item(f, item)?; + } + } + } + + Ok(()) +} + #[cfg(test)] mod test { use crate::parser::*; diff --git a/cedar-policy-core/src/parser/grammar.lalrpop b/cedar-policy-core/src/parser/grammar.lalrpop index fe0af268c..6026398d1 100644 --- a/cedar-policy-core/src/parser/grammar.lalrpop +++ b/cedar-policy-core/src/parser/grammar.lalrpop @@ -1,14 +1,18 @@ use std::str::FromStr; + +use lalrpop_util::{ParseError, ErrorRecovery}; + use crate::parser::*; +use crate::parser::err::{RawErrorRecovery, RawUserError}; use crate::parser::node::ASTNode as Node; -use lalrpop_util::{ParseError, ErrorRecovery}; -grammar<'err>(errors: &'err mut Vec, String>>); +grammar<'err>(errors: &'err mut Vec>); extern { - type Error = String; + type Error = RawUserError; } +// New tokens should be reflected in the `FRIENDLY_TOKEN_NAMES` map in err.rs. match { // Whitespace and comments r"\s*" => { }, // The default whitespace skipping is disabled an `ignore pattern` is specified @@ -55,15 +59,6 @@ match { "||", "&&", "+", "-", "*", "/", "%", "!", - - -} else { - // this will catch anything that is not above. - // This means that all tokens are allowed, so - // We never get the error of an unknown token. - // The other errors (i.e. UnrecognizedToken) are - // better. - r".", } Comma: Vec = { @@ -356,7 +351,7 @@ Literal: Node> = { =>? match u64::from_str(n) { Ok(n) => Ok(Node::new(Some(cst::Literal::Num(n)),l,r)), Err(e) => Err(ParseError::User { - error: format!("Integer parse fail: {e}"), + error: ASTNode::new(format!("integer parse error: {e}"),l,r), }), }, diff --git a/cedar-policy-core/src/parser/node.rs b/cedar-policy-core/src/parser/node.rs index a830b1743..00ba626b2 100644 --- a/cedar-policy-core/src/parser/node.rs +++ b/cedar-policy-core/src/parser/node.rs @@ -14,28 +14,103 @@ * limitations under the License. */ +use std::cmp::Ordering; +use std::error::Error; +use std::fmt::{self, Debug, Display}; +use std::hash::{Hash, Hasher}; +use std::ops::Range; + +use miette::{Diagnostic, LabeledSpan, Severity, SourceCode}; use serde::{Deserialize, Serialize}; /// Describes where in policy source code a node in the CST or expression AST /// occurs. -#[derive(Serialize, Deserialize, Hash, Debug, Clone, PartialEq, Eq)] -pub struct SourceInfo(pub std::ops::Range); +#[derive(Clone, Debug, Deserialize, Eq, Hash, PartialEq, Serialize)] +pub struct SourceInfo(pub Range); impl SourceInfo { - /// Get the start of range. - pub fn range_start(&self) -> usize { + /// Construct a new [`SourceInfo`] from a start offset and a length, in + /// bytes. + pub const fn new(start: usize, len: usize) -> Self { + SourceInfo(start..(start + len)) + } + + /// Construct a new zero-length [`SourceInfo`] pointing to a specific + /// offset. + pub const fn from_offset(offset: usize) -> Self { + SourceInfo(offset..offset) + } + + /// Get the start of range, in bytes. + pub const fn range_start(&self) -> usize { self.0.start } - /// Get the end of range. - pub fn range_end(&self) -> usize { + /// Get the end of range, in bytes. + pub const fn range_end(&self) -> usize { self.0.end } + + /// Get the length of the source range, in bytes. + /// + /// # Panics + /// + /// Panics if the end of the range is before the start. + pub const fn len(&self) -> usize { + assert!(self.range_start() <= self.range_end()); + self.range_end() - self.range_start() + } + + /// Tests whether this [`SourceInfo`] range is a zero-length offset. + pub const fn is_empty(&self) -> bool { + self.len() == 0 + } +} + +impl Display for SourceInfo { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + if self.is_empty() { + write!(f, "{}", self.range_start()) + } else { + write!(f, "[{}, {})", self.range_start(), self.range_end()) + } + } +} + +impl Ord for SourceInfo { + fn cmp(&self, other: &Self) -> Ordering { + self.range_start() + .cmp(&other.range_start()) + .then_with(|| self.len().cmp(&other.len())) + } +} + +impl PartialOrd for SourceInfo { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) + } +} + +impl From for SourceInfo { + fn from(offset: usize) -> Self { + SourceInfo::from_offset(offset) + } +} + +impl From> for SourceInfo { + fn from(range: Range) -> Self { + SourceInfo(range) + } +} + +impl From for Range { + fn from(info: SourceInfo) -> Self { + info.0 + } } /// Metadata for our syntax trees -// Note that these derives are likely to need explicit impls as we develop further -#[derive(Debug, Clone)] +#[derive(Clone, Deserialize, Serialize)] pub struct ASTNode { /// Main data represented pub node: N, @@ -47,16 +122,26 @@ pub struct ASTNode { impl ASTNode { /// Create a new Node from main data pub fn new(node: N, left: usize, right: usize) -> Self { - let info = SourceInfo(left..right); - ASTNode { node, info } + ASTNode::from_source(left..right, node) } /// Create a new Node from main data - pub fn from_source(node: N, info: SourceInfo) -> Self { - ASTNode { node, info } + pub fn from_source(info: impl Into, node: N) -> Self { + ASTNode { + node, + info: info.into(), + } + } + + /// Transform the inner value while retaining the attached source info. + pub fn map(self, f: impl FnOnce(N) -> M) -> ASTNode { + ASTNode { + node: f(self.node), + info: self.info, + } } - /// like Option.as_ref() + /// Converts from `&ASTNode` to `ASTNode<&N>`. pub fn as_ref(&self) -> ASTNode<&N> { ASTNode { node: &self.node, @@ -64,20 +149,97 @@ impl ASTNode { } } - /// map the main data, leaving the SourceInfo alone - pub fn map(self, f: impl FnOnce(N) -> D) -> ASTNode { + /// Converts from `&mut ASTNode` to `ASTNode<&mut N>`. + pub fn as_mut(&mut self) -> ASTNode<&mut N> { ASTNode { - node: f(self.node), - info: self.info, + node: &mut self.node, + info: self.info.clone(), } } - /// consume the Node, producing the main data and the SourceInfo + /// Consume the `ASTNode`, yielding the node and attached source info. pub fn into_inner(self) -> (N, SourceInfo) { (self.node, self.info) } } +impl ASTNode<&N> { + /// Converts a `ASTNode<&N>` to a `ASTNode` by cloning the inner value. + pub fn cloned(self) -> ASTNode { + self.map(|value| value.clone()) + } +} + +impl ASTNode<&N> { + /// Converts a `ASTNode<&N>` to a `ASTNode` by copying the inner value. + pub fn copied(self) -> ASTNode { + self.map(|value| *value) + } +} + +impl Debug for ASTNode { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + Debug::fmt(&self.node, f)?; + write!(f, " @ {}", self.info) + } +} + +impl Display for ASTNode { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + Display::fmt(&self.node, f) + } +} + +impl Error for ASTNode { + fn source(&self) -> Option<&(dyn Error + 'static)> { + self.node.source() + } + + fn description(&self) -> &str { + #[allow(deprecated)] + self.node.description() + } + + fn cause(&self) -> Option<&dyn Error> { + #[allow(deprecated)] + self.node.cause() + } +} + +impl Diagnostic for ASTNode { + fn code<'a>(&'a self) -> Option> { + self.node.code() + } + + fn severity(&self) -> Option { + self.node.severity() + } + + fn help<'a>(&'a self) -> Option> { + self.node.help() + } + + fn url<'a>(&'a self) -> Option> { + self.node.url() + } + + fn source_code(&self) -> Option<&dyn SourceCode> { + self.node.source_code() + } + + fn labels(&self) -> Option + '_>> { + self.node.labels() + } + + fn related<'a>(&'a self) -> Option + 'a>> { + self.node.related() + } + + fn diagnostic_source(&self) -> Option<&dyn Diagnostic> { + self.node.diagnostic_source() + } +} + // Ignore the metadata this node contains impl PartialEq for ASTNode { /// ignores metadata @@ -86,21 +248,27 @@ impl PartialEq for ASTNode { } } impl Eq for ASTNode {} +impl Hash for ASTNode { + /// ignores metadata + fn hash(&self, state: &mut H) { + self.node.hash(state); + } +} -/// Convenience methods on `ASTNode>` -impl ASTNode> { +/// Convenience methods on `ASTNode>` +impl ASTNode> { /// Similar to `.as_inner()`, but also gives access to the `SourceInfo` - pub fn as_inner_pair(&self) -> (&SourceInfo, Option<&T>) { + pub fn as_inner_pair(&self) -> (&SourceInfo, Option<&N>) { (&self.info, self.node.as_ref()) } - /// Get the inner data as `&T`, if it exists - pub fn as_inner(&self) -> Option<&T> { + /// Get the inner data as `&N`, if it exists + pub fn as_inner(&self) -> Option<&N> { self.node.as_ref() } /// `None` if the node is empty, otherwise a node without the `Option` - pub fn collapse(&self) -> Option> { + pub fn collapse(&self) -> Option> { self.node.as_ref().map(|node| ASTNode { node, info: self.info.clone(), @@ -111,7 +279,7 @@ impl ASTNode> { /// if no main data or if `f` returns `None`. pub fn apply(&self, f: F) -> Option where - F: FnOnce(&T, &SourceInfo) -> Option, + F: FnOnce(&N, &SourceInfo) -> Option, { f(self.node.as_ref()?, &self.info) } @@ -120,7 +288,7 @@ impl ASTNode> { /// Returns `None` if no main data or if `f` returns `None`. pub fn into_apply(self, f: F) -> Option where - F: FnOnce(T, SourceInfo) -> Option, + F: FnOnce(N, SourceInfo) -> Option, { f(self.node?, self.info) } diff --git a/cedar-policy-core/src/parser/text_to_cst.rs b/cedar-policy-core/src/parser/text_to_cst.rs index f97b6aab0..1970e5a44 100644 --- a/cedar-policy-core/src/parser/text_to_cst.rs +++ b/cedar-policy-core/src/parser/text_to_cst.rs @@ -29,92 +29,89 @@ lalrpop_mod!( "/src/parser/grammar.rs" ); +use lazy_static::lazy_static; + use super::*; -// "Global" data per-thread, initialized at first use -thread_local!(static POLICIES_PARSER: grammar::PoliciesParser = grammar::PoliciesParser::new()); -thread_local!(static POLICY_PARSER: grammar::PolicyParser = grammar::PolicyParser::new()); -thread_local!(static EXPR_PARSER: grammar::ExprParser = grammar::ExprParser::new()); -thread_local!(static REF_PARSER: grammar::RefParser = grammar::RefParser::new()); -thread_local!(static PRIMARY_PARSER: grammar::PrimaryParser = grammar::PrimaryParser::new()); -thread_local!(static NAME_PARSER: grammar::NameParser = grammar::NameParser::new()); -thread_local!(static IDENT_PARSER: grammar::IdentParser = grammar::IdentParser::new()); - -// This macro calls a generated parser (specified by the argument to the macro), -// collects errors that could be generated multiple ways, and returns a single -// Result where the error type is `err::ParseError`. -macro_rules! parse_collect_errors { - ($parser:ident, $text:ident) => { - $parser.with(|parser| { - // call generated parser - let mut errs = Vec::new(); - let result = parser.parse(&mut errs, $text); - - // convert both parser error types to the local error type - let mut errors: Vec = - errs.into_iter().map(err::ParseError::from).collect(); - let result = result.map_err(err::ParseError::from); - - // decide to return errors or success - match result { - Ok(parsed) => { - if !errors.is_empty() { - // In this case, `parsed` contains internal errors but could - // still be used. However, for now, we do not use `parsed` -- - // we just return the errors from this parsing phase and stop. - Err(errors) - } else { - Ok(parsed) - } - } - Err(e) => { - errors.push(e); - Err(errors) - } - } - }) +/// 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 +/// [`err::ParseErrors`]. +fn parse_collect_errors<'a, P, T>( + parser: &P, + parse: impl FnOnce( + &P, + &mut Vec>, + &'a str, + ) -> Result>, + text: &'a str, +) -> Result { + let mut errs = Vec::new(); + let result = parse(parser, &mut errs, text); + + let mut errors: err::ParseErrors = errs + .into_iter() + .map(|recovery| err::ToCSTError::from_raw_err_recovery(recovery).into()) + .collect(); + let parsed = match result { + Ok(parsed) => parsed, + Err(e) => { + errors.push(err::ToCSTError::from_raw_parse_err(e).into()); + return Err(errors); + } }; + if errors.is_empty() { + Ok(parsed) + } else { + Err(errors) + } +} + +// Thread-safe "global" parsers, initialized at first use +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(); + static ref REF_PARSER: grammar::RefParser = grammar::RefParser::new(); + static ref PRIMARY_PARSER: grammar::PrimaryParser = grammar::PrimaryParser::new(); + static ref NAME_PARSER: grammar::NameParser = grammar::NameParser::new(); + static ref IDENT_PARSER: grammar::IdentParser = grammar::IdentParser::new(); } /// Create CST for multiple policies from text pub fn parse_policies( text: &str, -) -> Result>, Vec> { - parse_collect_errors!(POLICIES_PARSER, text) +) -> 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>, Vec> { - parse_collect_errors!(POLICY_PARSER, text) +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>, Vec> { - parse_collect_errors!(EXPR_PARSER, text) +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>, Vec> { - parse_collect_errors!(REF_PARSER, text) +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>, Vec> { - parse_collect_errors!(PRIMARY_PARSER, text) +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>, Vec> { - parse_collect_errors!(NAME_PARSER, text) +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>, Vec> { - parse_collect_errors!(IDENT_PARSER, text) +pub fn parse_ident(text: &str) -> Result>, err::ParseErrors> { + parse_collect_errors(&*IDENT_PARSER, grammar::IdentParser::parse, text) } #[cfg(test)] @@ -668,7 +665,7 @@ mod tests { let policy_text = r#" //comment policy // comment policy 2 - @anno("good annotation") // comments after annotation + @anno("good annotation") // comments after annotation // comments after annotation 2 permit(principal //comment 1 == @@ -888,8 +885,8 @@ mod tests { #[test] fn policies6() { // test that an error doesn't stop the parser - let policies = POLICIES_PARSER.with(|p| { - p.parse( + let policies = POLICIES_PARSER + .parse( &mut Vec::new(), r#" // use a number to error @@ -900,8 +897,7 @@ mod tests { ) .expect("parser error") .node - .expect("no data") - }); + .expect("no data"); let success = policies .0 .into_iter() diff --git a/cedar-policy-core/src/parser/unescape.rs b/cedar-policy-core/src/parser/unescape.rs index 93e336450..ed65d2541 100644 --- a/cedar-policy-core/src/parser/unescape.rs +++ b/cedar-policy-core/src/parser/unescape.rs @@ -89,7 +89,10 @@ impl std::fmt::Display for UnescapeError { mod test { use super::to_unescaped_string; use crate::ast; - use crate::parser::{err::ParseError, text_to_cst}; + use crate::parser::{ + err::{ParseError, ParseErrors}, + text_to_cst, + }; #[test] fn test_string_escape() { @@ -125,7 +128,7 @@ mod test { #[test] fn test_pattern_escape() { // valid ASCII escapes - let mut errs = Vec::new(); + let mut errs = ParseErrors::new(); assert!( matches!(text_to_cst::parse_expr(r#""aa" like "\t\r\n\\\0\x42\*""#) .expect("failed parsing") @@ -141,7 +144,7 @@ mod test { ); // invalid ASCII escapes - let mut errs = Vec::new(); + let mut errs = ParseErrors::new(); assert!(text_to_cst::parse_expr(r#""abc" like "abc\xFF\xFEdef""#) .expect("failed parsing") .to_expr(&mut errs) @@ -152,7 +155,7 @@ mod test { )); // valid `\*` surrounded by chars - let mut errs = Vec::new(); + let mut errs = ParseErrors::new(); assert!( matches!(text_to_cst::parse_expr(r#""aaa" like "👀👀\*🤞🤞\*🤝""#) .expect("failed parsing") @@ -162,7 +165,7 @@ mod test { ); // invalid escapes - let mut errs = Vec::new(); + let mut errs = ParseErrors::new(); assert!(text_to_cst::parse_expr(r#""aaa" like "abc\d\bdef""#) .expect("failed parsing") .to_expr(&mut errs) diff --git a/cedar-policy-formatter/Cargo.toml b/cedar-policy-formatter/Cargo.toml index 5958cf0e5..6aa0b03c0 100644 --- a/cedar-policy-formatter/Cargo.toml +++ b/cedar-policy-formatter/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "cedar-policy-formatter" -version = "2.3.3" +version = "2.4.0" edition = "2021" license = "Apache-2.0" categories = ["compilers", "config"] @@ -10,10 +10,10 @@ homepage = "https://cedarpolicy.com" repository = "https://github.com/cedar-policy/cedar" [dependencies] -cedar-policy-core = { version = "=2.3.3", path = "../cedar-policy-core" } +cedar-policy-core = { version = "=2.4.0", path = "../cedar-policy-core" } pretty = "0.11" -anyhow = "1.0" logos = "0.12.1" itertools = "0.10" smol_str = { version = "0.2", features = ["serde"] } regex = { version= "1.8", features = ["unicode"] } +miette = { version = "5.9.0" } diff --git a/cedar-policy-formatter/src/pprint/fmt.rs b/cedar-policy-formatter/src/pprint/fmt.rs index 6877c7020..6334b1c07 100644 --- a/cedar-policy-formatter/src/pprint/fmt.rs +++ b/cedar-policy-formatter/src/pprint/fmt.rs @@ -14,7 +14,8 @@ * limitations under the License. */ -use anyhow::{anyhow, Context, Result}; +use miette::{miette, Result, WrapErr}; + use cedar_policy_core::ast::{PolicySet, Template}; use cedar_policy_core::parser::parse_policyset; use cedar_policy_core::parser::{err::ParseErrors, text_to_cst::parse_policies}; @@ -36,16 +37,14 @@ fn tree_to_pretty(t: &T, context: &mut config::Context<'_>) -> String { } fn soundness_check(ps: &str, ast: &PolicySet) -> Result<()> { - let formatted_ast = parse_policyset(ps) - .map_err(ParseErrors) - .context("formatter produces invalid policies")?; + let formatted_ast = parse_policyset(ps).wrap_err("formatter produces invalid policies")?; let (formatted_policies, policies) = ( formatted_ast.templates().collect::>(), ast.templates().collect::>(), ); if formatted_policies.len() != policies.len() { - return Err(anyhow!("missing formatted policies")); + return Err(miette!("missing formatted policies")); } for (f_p, p) in formatted_policies.into_iter().zip(policies.into_iter()) { @@ -63,24 +62,23 @@ fn soundness_check(ps: &str, ast: &PolicySet) -> Result<()> { .non_head_constraints() .eq_shape(p.non_head_constraints())) { - return Err(anyhow!(format!( + return Err(miette!( "policies differ:\nformatted: {}\ninput: {}", - f_p, p - ))); + f_p, + p + )); } } Ok(()) } pub fn policies_str_to_pretty(ps: &str, config: &Config) -> Result { - let cst = parse_policies(ps) - .map_err(ParseErrors) - .context("cannot parse input policies to CSTs")?; - let mut errs = Vec::new(); + let cst = parse_policies(ps).wrap_err("cannot parse input policies to CSTs")?; + let mut errs = ParseErrors::new(); let ast = cst .to_policyset(&mut errs) - .ok_or(ParseErrors(errs)) - .context("cannot parse input policies to ASTs")?; + .ok_or(errs) + .wrap_err("cannot parse input policies to ASTs")?; let tokens = get_token_stream(ps); let end_comment_str = &ps[tokens.last().unwrap().span.end..]; let mut context = config::Context { config, tokens }; diff --git a/cedar-policy-validator/Cargo.toml b/cedar-policy-validator/Cargo.toml index b2f204b5c..5c1e4c689 100644 --- a/cedar-policy-validator/Cargo.toml +++ b/cedar-policy-validator/Cargo.toml @@ -2,7 +2,7 @@ name = "cedar-policy-validator" edition = "2021" -version = "2.3.3" +version = "2.4.0" license = "Apache-2.0" categories = ["compilers", "config"] description = "Validator for the Cedar Policy language." @@ -11,7 +11,7 @@ homepage = "https://cedarpolicy.com" repository = "https://github.com/cedar-policy/cedar" [dependencies] -cedar-policy-core = { version = "=2.3.3", path = "../cedar-policy-core" } +cedar-policy-core = { version = "=2.4.0", path = "../cedar-policy-core" } serde = { version = "1.0", features = ["derive"] } serde_json = { version = "1.0", features = ["preserve_order"] } serde_with = "3.0" diff --git a/cedar-policy-validator/src/err.rs b/cedar-policy-validator/src/err.rs index 2aa88ef82..6a8e47e3a 100644 --- a/cedar-policy-validator/src/err.rs +++ b/cedar-policy-validator/src/err.rs @@ -18,7 +18,7 @@ use std::collections::HashSet; use cedar_policy_core::{ ast::{EntityUID, Name}, - parser::err::ParseError, + parser::err::{ParseError, ParseErrors}, transitive_closure, }; use itertools::Itertools; @@ -69,17 +69,17 @@ pub enum SchemaError { CycleInActionHierarchy, /// Parse errors occurring while parsing an entity type. #[error("Parse error in entity type: {}", Self::format_parse_errs(.0))] - EntityTypeParseError(Vec), + EntityTypeParseError(ParseErrors), /// Parse errors occurring while parsing a namespace identifier. #[error("Parse error in namespace identifier: {}", Self::format_parse_errs(.0))] - NamespaceParseError(Vec), + NamespaceParseError(ParseErrors), /// Parse errors occurring while parsing an extension type. #[error("Parse error in extension type: {}", Self::format_parse_errs(.0))] - ExtensionTypeParseError(Vec), + ExtensionTypeParseError(ParseErrors), /// Parse errors occurring while parsing the name of one of reusable /// declared types. #[error("Parse error in common type identifier: {}", Self::format_parse_errs(.0))] - CommonTypeParseError(Vec), + CommonTypeParseError(ParseErrors), /// The schema file included an entity type `Action` in the entity type /// list. The `Action` entity type is always implicitly declared, and it /// cannot currently have attributes or be in any groups, so there is no diff --git a/cedar-policy-validator/src/schema.rs b/cedar-policy-validator/src/schema.rs index 27ef28a26..6160d4499 100644 --- a/cedar-policy-validator/src/schema.rs +++ b/cedar-policy-validator/src/schema.rs @@ -26,7 +26,7 @@ use std::sync::Arc; use cedar_policy_core::{ ast::{Eid, Entity, EntityType, EntityUID, Id, Name, RestrictedExpr}, entities::{Entities, JSONValue, TCComputation}, - parser::err::ParseError, + parser::err::ParseErrors, transitive_closure::{compute_tc, TCNode}, FromNormalizedStr, }; @@ -34,6 +34,7 @@ use serde::{Deserialize, Serialize}; use serde_with::serde_as; use smol_str::SmolStr; +use crate::types::OpenTag; use crate::{ schema_file_format, types::{AttributeType, Attributes, EntityRecordKind, Type}, @@ -351,9 +352,10 @@ impl ValidatorNamespaceDef { Err(e) => return Err(e), }; } - Ok(Type::EntityOrRecord(EntityRecordKind::Record { - attrs: Attributes::with_required_attributes(required_attrs), - })) + Ok(Type::record_with_required_attributes( + required_attrs, + OpenTag::ClosedAttributes, + )) } JSONValue::Set(v) => match v.get(0) { //sets with elements of different types will be rejected elsewhere @@ -575,7 +577,7 @@ impl ValidatorNamespaceDef { pub(crate) fn parse_possibly_qualified_name_with_default_namespace( name_str: &SmolStr, default_namespace: Option<&Name>, - ) -> std::result::Result> { + ) -> std::result::Result { let name = Name::from_normalized_str(name_str)?; let qualified_name = if name.namespace_components().next().is_some() { @@ -601,7 +603,7 @@ impl ValidatorNamespaceDef { fn parse_unqualified_name_with_namespace( type_name: impl AsRef, namespace: Option, - ) -> std::result::Result> { + ) -> std::result::Result { let type_name = Id::from_normalized_str(type_name.as_ref())?; match namespace { Some(namespace) => Ok(Name::type_in_namespace(type_name, namespace)), @@ -663,8 +665,9 @@ impl ValidatorNamespaceDef { )) } else { Ok( - Self::parse_record_attributes(default_namespace, attributes)? - .map(Type::record_with_attributes), + Self::parse_record_attributes(default_namespace, attributes)?.map( + |attrs| Type::record_with_attributes(attrs, OpenTag::ClosedAttributes), + ), ) } } @@ -1042,7 +1045,7 @@ impl ValidatorSchema { fn record_attributes_or_none(ty: Type) -> Option { match ty { - Type::EntityOrRecord(EntityRecordKind::Record { attrs }) => Some(attrs), + Type::EntityOrRecord(EntityRecordKind::Record { attrs, .. }) => Some(attrs), _ => None, } } @@ -1064,7 +1067,7 @@ impl ValidatorSchema { } } - Type::EntityOrRecord(EntityRecordKind::Record { attrs }) => { + Type::EntityOrRecord(EntityRecordKind::Record { attrs, .. }) => { for (_, attr_ty) in attrs.iter() { Self::check_undeclared_in_type( &attr_ty.attr_type, @@ -1175,6 +1178,7 @@ impl ValidatorSchema { .context .iter() .map(|(k, v)| (k.clone(), v.clone())), + OpenTag::ClosedAttributes, ) }) } @@ -2224,7 +2228,7 @@ mod test { .expect("Error converting schema type to type.") .resolve_type_defs(&HashMap::new()) .unwrap(); - assert_eq!(ty, Type::record_with_attributes(None)); + assert_eq!(ty, Type::closed_record_with_attributes(None)); } #[test] diff --git a/cedar-policy-validator/src/type_error.rs b/cedar-policy-validator/src/type_error.rs index 186579a68..cba2cab2e 100644 --- a/cedar-policy-validator/src/type_error.rs +++ b/cedar-policy-validator/src/type_error.rs @@ -106,17 +106,19 @@ impl TypeError { } } - pub(crate) fn missing_attribute( + pub(crate) fn unsafe_attribute_access( on_expr: Expr, - missing: String, + attribute: String, suggestion: Option, + may_exist: bool, ) -> Self { Self { on_expr: Some(on_expr), source_location: None, - kind: TypeErrorKind::MissingAttribute(MissingAttribute { - missing, + kind: TypeErrorKind::UnsafeAttributeAccess(UnsafeAttributeAccess { + attribute, suggestion, + may_exist, }), } } @@ -218,8 +220,16 @@ pub enum TypeErrorKind { IncompatibleTypes(IncompatibleTypes), /// The typechecker detected an access to a record or entity attribute /// that it could not statically guarantee would be present. - #[error("Attribute not found in record or entity: {}", .0.missing)] - MissingAttribute(MissingAttribute), + #[error( + "Attribute not found in record or entity: {}{}", + .0.attribute, + if .0.may_exist { + ". There may be additional attributes that the validator is not able to reason about." + } else { + "" + } + )] + UnsafeAttributeAccess(UnsafeAttributeAccess), /// The typechecker could not conclude that an access to an optional /// attribute was safe. #[error("Unable to guarantee safety of access to optional attribute: {}", .0.optional)] @@ -273,9 +283,12 @@ pub struct TypesMustMatch { /// Structure containing details about a missing attribute error. #[derive(Debug, Hash, Eq, PartialEq)] -pub struct MissingAttribute { - missing: String, +pub struct UnsafeAttributeAccess { + attribute: String, suggestion: Option, + /// When this is true, the attribute might still exist, but the validator + /// cannot guarantee that it will. + may_exist: bool, } /// Structure containing details about an unsafe optional attribute error. diff --git a/cedar-policy-validator/src/typecheck.rs b/cedar-policy-validator/src/typecheck.rs index 30c0a9313..6d95f9d93 100644 --- a/cedar-policy-validator/src/typecheck.rs +++ b/cedar-policy-validator/src/typecheck.rs @@ -41,7 +41,9 @@ use crate::{ schema::{ is_action_entity_type, ActionHeadVar, HeadVar, PrincipalOrResourceHeadVar, ValidatorSchema, }, - types::{AttributeType, Effect, EffectSet, EntityRecordKind, Primitive, RequestEnv, Type}, + types::{ + AttributeType, Effect, EffectSet, EntityRecordKind, OpenTag, Primitive, RequestEnv, Type, + }, ValidationMode, }; @@ -802,7 +804,9 @@ impl<'a> Typechecker<'a> { type_errors: &mut Vec, ) -> TypecheckAnswer<'b> { let ExprKind::BinaryApp { op, arg1, arg2 } = bin_expr.expr_kind() else { - panic!("`strict_transform_binary` called with an expression kind other than `BinaryApp`"); + panic!( + "`strict_transform_binary` called with an expression kind other than `BinaryApp`" + ); }; // Binary operators `==`, `contains`, `containsAll`, and `containsAny` @@ -979,12 +983,19 @@ impl<'a> Typechecker<'a> { }, ) => Self::unify_strict_types(ety1, ety2), ( - Type::EntityOrRecord(EntityRecordKind::Record { attrs: attrs1 }), - Type::EntityOrRecord(EntityRecordKind::Record { attrs: attrs2 }), + Type::EntityOrRecord(EntityRecordKind::Record { + attrs: attrs1, + open_attributes: open1, + }), + Type::EntityOrRecord(EntityRecordKind::Record { + attrs: attrs2, + open_attributes: open2, + }), ) => { let keys1 = attrs1.attrs.keys().collect::>(); let keys2 = attrs2.attrs.keys().collect::>(); - keys1 == keys2 + open1 == open2 + && keys1 == keys2 && attrs1.iter().all(|(k, attr1)| { let attr2 = attrs2 .get_attr(k) @@ -1063,6 +1074,7 @@ impl<'a> Typechecker<'a> { ExprKind::Var(Var::Context) => TypecheckAnswer::success( ExprBuilder::with_data(Some(Type::record_with_attributes( request_env.context.clone(), + OpenTag::ClosedAttributes, ))) .with_same_source_info(e) .var(Var::Context), @@ -1471,7 +1483,7 @@ impl<'a> Typechecker<'a> { attr_ty.clone().map(|attr_ty| attr_ty.attr_type), ) .with_same_source_info(e) - .get_attr(typ_expr_actual, attr.clone()); + .get_attr(typ_expr_actual.clone(), attr.clone()); match attr_ty { Some(ty) => { // A safe access to an attribute requires either @@ -1495,10 +1507,11 @@ impl<'a> Typechecker<'a> { let borrowed = all_attrs.iter().map(|s| s.as_str()).collect::>(); let suggestion = fuzzy_search(attr, &borrowed); - type_errors.push(TypeError::missing_attribute( + type_errors.push(TypeError::unsafe_attribute_access( e.clone(), attr.to_string(), suggestion, + Type::may_have_attr(self.schema, typ_actual, attr), )); TypecheckAnswer::fail(annot_expr) } @@ -1701,7 +1714,10 @@ impl<'a> Typechecker<'a> { std::iter::zip(record_attrs, record_attr_tys); TypecheckAnswer::success( ExprBuilder::with_data(Some( - Type::record_with_required_attributes(record_type_entries), + Type::record_with_required_attributes( + record_type_entries, + OpenTag::ClosedAttributes, + ), )) .with_same_source_info(e) .record(t), diff --git a/cedar-policy-validator/src/typecheck/test_expr.rs b/cedar-policy-validator/src/typecheck/test_expr.rs index b2f0821e4..64af70adf 100644 --- a/cedar-policy-validator/src/typecheck/test_expr.rs +++ b/cedar-policy-validator/src/typecheck/test_expr.rs @@ -19,6 +19,8 @@ #![cfg(test)] // GRCOV_STOP_COVERAGE +use std::str::FromStr; + use cedar_policy_core::ast::{BinaryOp, EntityUID, Expr, PatternElem, SlotId, Var}; use serde_json::json; use smol_str::SmolStr; @@ -139,7 +141,7 @@ fn heterogeneous_set() { fn record_typechecks() { assert_typechecks_empty_schema( Expr::record([("foo".into(), Expr::val(1))]), - Type::record_with_required_attributes([("foo".into(), Type::primitive_long())]), + Type::closed_record_with_required_attributes([("foo".into(), Type::primitive_long())]), ) } @@ -499,6 +501,68 @@ fn entity_has_typechecks() { fn record_has_typechecks() { assert_typechecks_empty_schema( Expr::has_attr(Expr::var(Var::Context), "attr".into()), + Type::singleton_boolean(false), + ); + assert_typechecks_empty_schema( + Expr::has_attr(Expr::record([]), "attr".into()), + Type::singleton_boolean(false), + ); + assert_typechecks_empty_schema( + Expr::from_str("{a: 1} has a").unwrap(), + Type::singleton_boolean(true), + ); +} + +#[test] +fn record_lub_has_typechecks() { + assert_typechecks_empty_schema( + Expr::from_str("(if 1 > 0 then {a: 1} else {a: 2}) has a").unwrap(), + Type::singleton_boolean(true), + ); + assert_typechecks_empty_schema( + Expr::from_str("(if 1 > 0 then {a: 1} else {a: 2}) has b").unwrap(), + Type::singleton_boolean(false), + ); + assert_typechecks_empty_schema( + Expr::from_str("(if 1 > 0 then {a: 1} else {a: 2, b: 3}) has a").unwrap(), + Type::singleton_boolean(true), + ); + assert_typechecks_empty_schema( + Expr::from_str("(if 1 > 0 then {a: 1, b: 2} else {a: 1, c: 2}) has a").unwrap(), + Type::singleton_boolean(true), + ); + assert_typechecks_empty_schema( + Expr::from_str("(if 1 > 0 then {a: 1} else {}) has a").unwrap(), + Type::primitive_boolean(), + ); + assert_typechecks_empty_schema( + Expr::from_str("(if 1 > 0 then {a: 1, b: 2} else {a: 1, c: 2}) has b").unwrap(), + Type::primitive_boolean(), + ); + assert_typechecks_empty_schema( + Expr::from_str("(if 1 > 0 then (if 1 > 0 then {a: 1} else {}) else {}) has a").unwrap(), + Type::primitive_boolean(), + ); + assert_typechecks_empty_schema( + Expr::from_str("(if 1 > 0 then {a: true} else {a: false}) has b").unwrap(), + Type::singleton_boolean(false), + ); + assert_typechecks_empty_schema( + Expr::from_str("(if 1 > 0 then {a: true} else {a: false}) has a").unwrap(), + Type::singleton_boolean(true), + ); + + // These cases are imprecise. + assert_typechecks_empty_schema( + Expr::from_str("(if 1 > 0 then {a: 1} else {}) has c").unwrap(), + Type::primitive_boolean(), + ); + assert_typechecks_empty_schema( + Expr::from_str("(if 1 > 0 then {a: 1} else {b: 2}) has c").unwrap(), + Type::primitive_boolean(), + ); + assert_typechecks_empty_schema( + Expr::from_str("(if 1 > 0 then {a: 1} else {a : false}) has a").unwrap(), Type::primitive_boolean(), ); } @@ -535,10 +599,11 @@ fn record_get_attr_incompatible() { ); assert_typecheck_fails_empty_schema_without_type( Expr::get_attr(if_expr.clone(), attr.clone()), - vec![TypeError::missing_attribute( + vec![TypeError::unsafe_attribute_access( Expr::get_attr(if_expr, attr.clone()), attr.to_string(), None, + true, )], ); } @@ -568,7 +633,10 @@ fn record_get_attr_lub_typecheck_fails() { vec![TypeError::incompatible_types( if_expr, vec![ - Type::record_with_required_attributes([(attr, Type::singleton_boolean(true))]), + Type::closed_record_with_required_attributes([( + attr, + Type::singleton_boolean(true), + )]), Type::primitive_long(), ], )], @@ -580,10 +648,11 @@ fn record_get_attr_does_not_exist() { let attr: SmolStr = "foo".into(); assert_typecheck_fails_empty_schema_without_type( Expr::get_attr(Expr::record([]), attr.clone()), - vec![TypeError::missing_attribute( + vec![TypeError::unsafe_attribute_access( Expr::get_attr(Expr::record([]), attr.clone()), attr.to_string(), None, + false, )], ); } @@ -598,10 +667,11 @@ fn record_get_attr_lub_does_not_exist() { ); assert_typecheck_fails_empty_schema_without_type( Expr::get_attr(if_expr.clone(), attr.clone()), - vec![TypeError::missing_attribute( + vec![TypeError::unsafe_attribute_access( Expr::get_attr(if_expr, attr.clone()), attr.to_string(), None, + false, )], ); } @@ -686,7 +756,7 @@ fn contains_typecheck_fails() { vec![TypeError::expected_type( Expr::record([("foo".into(), Expr::val(1))]), Type::any_set(), - Type::record_with_attributes([( + Type::closed_record_with_attributes([( "foo".into(), AttributeType::new(Type::primitive_long(), true), )]), diff --git a/cedar-policy-validator/src/typecheck/test_namespace.rs b/cedar-policy-validator/src/typecheck/test_namespace.rs index 486b5a9a2..28bb3e835 100644 --- a/cedar-policy-validator/src/typecheck/test_namespace.rs +++ b/cedar-policy-validator/src/typecheck/test_namespace.rs @@ -349,10 +349,11 @@ fn multiple_namespaces_attributes() { schema, Expr::from_str("B::Foo::\"foo\".x").unwrap(), None, - vec![TypeError::missing_attribute( + vec![TypeError::unsafe_attribute_access( Expr::from_str("B::Foo::\"foo\".x").unwrap(), "x".to_string(), None, + false, )], ); } diff --git a/cedar-policy-validator/src/typecheck/test_optional_attributes.rs b/cedar-policy-validator/src/typecheck/test_optional_attributes.rs index 3ff53c899..a9c3a0333 100644 --- a/cedar-policy-validator/src/typecheck/test_optional_attributes.rs +++ b/cedar-policy-validator/src/typecheck/test_optional_attributes.rs @@ -538,7 +538,8 @@ fn record_optional_attrs() { "record": { "type": "Record", "attributes": { - "name": { "type": "String", "required": false} + "name": { "type": "String", "required": false}, + "other": { "type": "String", "required": true} } } } @@ -750,10 +751,11 @@ fn action_attrs_failing() { assert_policy_typecheck_fails( schema.clone(), failing_policy, - vec![TypeError::missing_attribute( + vec![TypeError::unsafe_attribute_access( Expr::get_attr(Expr::var(Var::Action), "canUndo".into()), "canUndo".to_string(), Some("isReadOnly".to_string()), + false, )], ); diff --git a/cedar-policy-validator/src/typecheck/test_policy.rs b/cedar-policy-validator/src/typecheck/test_policy.rs index 88f4d7015..c271e0c2c 100644 --- a/cedar-policy-validator/src/typecheck/test_policy.rs +++ b/cedar-policy-validator/src/typecheck/test_policy.rs @@ -244,7 +244,7 @@ fn policy_invalid_attribute() { r#"permit(principal, action in [Action::"delete_group", Action::"view_photo"], resource) when { resource.file_type == "jpg" };"# ).expect("Policy should parse."), vec![ - TypeError::missing_attribute(Expr::get_attr(Expr::var(Var::Resource), "file_type".into()), "file_type".into(), Some("name".into())) + TypeError::unsafe_attribute_access(Expr::get_attr(Expr::var(Var::Resource), "file_type".into()), "file_type".into(), Some("name".into()), false) ], ); } @@ -257,7 +257,7 @@ fn policy_invalid_attribute_2() { r#"permit(principal, action == Action::"view_photo", resource) when { principal.age > 21 };"# ).expect("Policy should parse."), vec![ - TypeError::missing_attribute(Expr::get_attr(Expr::var(Var::Principal), "age".into()), "age".into(), Some("name".into())) + TypeError::unsafe_attribute_access(Expr::get_attr(Expr::var(Var::Principal), "age".into()), "age".into(), Some("name".into()), false) ] ); } @@ -420,11 +420,12 @@ fn entity_lub_cant_access_attribute_not_shared() { assert_policy_typecheck_fails_simple_schema( p, vec![ - TypeError::missing_attribute( + TypeError::unsafe_attribute_access( Expr::from_str(r#"(if 1 > 0 then User::"alice" else Photo::"vacation.jpg").name"#) .unwrap(), "name".into(), None, + true, ), TypeError::types_must_match( Expr::from_str(r#"if 1 > 0 then User::"alice" else Photo::"vacation.jpg""#) @@ -444,10 +445,11 @@ fn entity_attribute_recommendation() { Some("0".to_string()), r#"permit(principal, action == Action::"view_photo", resource) when {resource.filetype like "*jpg" }; "# ).expect("Policy should parse"); - let expected = TypeError::missing_attribute( + let expected = TypeError::unsafe_attribute_access( Expr::get_attr(Expr::var(Var::Resource), "filetype".into()), "filetype".into(), Some("file_type".into()), + false, ); assert_policy_typecheck_fails_simple_schema(p, vec![expected]); } @@ -483,7 +485,7 @@ fn entity_record_lub_is_none() { TypeError::incompatible_types( Expr::from_str(r#"if 1 > 0 then User::"alice" else {name: "bob"}"#).unwrap(), [ - Type::record_with_required_attributes([("name".into(), Type::primitive_string())]), + Type::closed_record_with_required_attributes([("name".into(), Type::primitive_string())]), Type::named_entity_reference_from_str("User"), ] ) @@ -696,7 +698,7 @@ fn record_entity_lub_non_term() { vec![TypeError::incompatible_types( Expr::from_str(r#"if principal.bar then principal.foo else U::"b""#).unwrap(), [ - Type::record_with_required_attributes([( + Type::closed_record_with_required_attributes([( "foo".into(), Type::named_entity_reference_from_str("U"), )]), diff --git a/cedar-policy-validator/src/typecheck/test_strict.rs b/cedar-policy-validator/src/typecheck/test_strict.rs index fad24d495..6fb04eace 100644 --- a/cedar-policy-validator/src/typecheck/test_strict.rs +++ b/cedar-policy-validator/src/typecheck/test_strict.rs @@ -369,8 +369,8 @@ fn ext_struct_non_lit() { assert_strict_type_error( s, &q, - Expr::from_str(r#"ip(if context has foo then "a" else "b")"#).unwrap(), - Expr::from_str(r#"ip(if context has foo then "a" else "b")"#).unwrap(), + Expr::from_str(r#"ip(if 1 > 0 then "a" else "b")"#).unwrap(), + Expr::from_str(r#"ip(if 1 > 0 then "a" else "b")"#).unwrap(), Type::extension("ipaddr".parse().unwrap()), TypeErrorKind::NonLitExtConstructor, ) @@ -381,8 +381,8 @@ fn ext_struct_non_lit() { assert_strict_type_error( s, &q, - Expr::from_str(r#"decimal(if context has bar then "0.1" else "1.0")"#).unwrap(), - Expr::from_str(r#"decimal(if context has bar then "0.1" else "1.0")"#).unwrap(), + Expr::from_str(r#"decimal(if 1 > 0 then "0.1" else "1.0")"#).unwrap(), + Expr::from_str(r#"decimal(if 1 > 0 then "0.1" else "1.0")"#).unwrap(), Type::extension("decimal".parse().unwrap()), TypeErrorKind::NonLitExtConstructor, ) @@ -568,16 +568,32 @@ fn test_has_attr() { s.clone(), &q, Expr::from_str(r#"{name: "foo"} has bar"#).unwrap(), - Expr::from_str(r#"{name: "foo"} has bar"#).unwrap(), + Expr::from_str(r#"false"#).unwrap(), + Type::primitive_boolean(), + ); + assert_typechecks_strict( + s.clone(), + &q, + Expr::from_str(r#"{name: "foo"} has name"#).unwrap(), + Expr::from_str(r#"true"#).unwrap(), Type::primitive_boolean(), ); assert_types_must_match( s, &q, - Expr::from_str(r#"{name: 1 == "foo"} has bar"#).unwrap(), - Expr::from_str(r#"{name: 1 == "foo"} has bar"#).unwrap(), + Expr::from_str(r#"(if 1 == 2 then {name: 1} else {bar: 2}) has bar"#).unwrap(), + Expr::from_str(r#"(if 1 == 2 then {name: 1} else {bar: 2}) has bar"#).unwrap(), Type::primitive_boolean(), - [Type::primitive_long(), Type::primitive_string()], + [ + Type::closed_record_with_required_attributes([( + "name".into(), + Type::primitive_long(), + )]), + Type::closed_record_with_required_attributes([( + "bar".into(), + Type::primitive_long(), + )]), + ], ); }) } diff --git a/cedar-policy-validator/src/typecheck/test_type_annotation.rs b/cedar-policy-validator/src/typecheck/test_type_annotation.rs index 0afd78f81..59ff1119b 100644 --- a/cedar-policy-validator/src/typecheck/test_type_annotation.rs +++ b/cedar-policy-validator/src/typecheck/test_type_annotation.rs @@ -119,7 +119,7 @@ fn expr_typechecks_with_correct_annotation() { ("foo".into(), Expr::val(1)), ("bar".into(), Expr::val(false)), ]), - &ExprBuilder::with_data(Some(Type::record_with_required_attributes([ + &ExprBuilder::with_data(Some(Type::closed_record_with_required_attributes([ ("foo".into(), Type::primitive_long()), ("bar".into(), Type::singleton_boolean(false)), ]))) diff --git a/cedar-policy-validator/src/typecheck/test_unspecified_entity.rs b/cedar-policy-validator/src/typecheck/test_unspecified_entity.rs index b43fc50e1..8b758104e 100644 --- a/cedar-policy-validator/src/typecheck/test_unspecified_entity.rs +++ b/cedar-policy-validator/src/typecheck/test_unspecified_entity.rs @@ -87,10 +87,11 @@ fn spec_principal_unspec_resource() { .expect("Policy should parse."); assert_policy_typecheck_fails( policy, - vec![TypeError::missing_attribute( + vec![TypeError::unsafe_attribute_access( Expr::get_attr(Expr::var(Var::Resource), "name".into()), "name".to_string(), None, + true, )], ); } @@ -104,10 +105,11 @@ fn spec_resource_unspec_principal() { .expect("Policy should parse."); assert_policy_typecheck_fails( policy, - vec![TypeError::missing_attribute( + vec![TypeError::unsafe_attribute_access( Expr::get_attr(Expr::var(Var::Principal), "name".into()), "name".to_string(), None, + true, )], ); @@ -128,10 +130,11 @@ fn unspec_resource_unspec_principal() { .expect("Policy should parse."); assert_policy_typecheck_fails( policy, - vec![TypeError::missing_attribute( + vec![TypeError::unsafe_attribute_access( Expr::get_attr(Expr::var(Var::Principal), "name".into()), "name".to_string(), None, + true, )], ); @@ -142,10 +145,11 @@ fn unspec_resource_unspec_principal() { .expect("Policy should parse."); assert_policy_typecheck_fails( policy, - vec![TypeError::missing_attribute( + vec![TypeError::unsafe_attribute_access( Expr::get_attr(Expr::var(Var::Resource), "name".into()), "name".to_string(), None, + true, )], ); } diff --git a/cedar-policy-validator/src/types.rs b/cedar-policy-validator/src/types.rs index 4450677cc..f84807ec2 100644 --- a/cedar-policy-validator/src/types.rs +++ b/cedar-policy-validator/src/types.rs @@ -134,22 +134,28 @@ impl Type { } pub(crate) fn any_record() -> Type { - Type::record_with_required_attributes(None) + // OpenAttributes <: ClosedAttributes, so this makes `any_record` a + // super type of all records. + Type::record_with_attributes(None, OpenTag::OpenAttributes) } pub(crate) fn record_with_required_attributes( required_attrs: impl IntoIterator, + open_attributes: OpenTag, ) -> Type { Type::EntityOrRecord(EntityRecordKind::Record { attrs: Attributes::with_required_attributes(required_attrs), + open_attributes, }) } pub(crate) fn record_with_attributes( attrs: impl IntoIterator, + open_attributes: OpenTag, ) -> Type { Type::EntityOrRecord(EntityRecordKind::Record { attrs: Attributes::with_attributes(attrs), + open_attributes, }) } @@ -349,12 +355,19 @@ impl Type { /// attributes, so we return false. pub(crate) fn may_have_attr(schema: &ValidatorSchema, ty: &Type, attr: &str) -> bool { match ty { - // If the type of the expression is a entity reference type, and the - // attribute is not declared to exist in the schema, then we know - // the entity may not have the attribute. For an entity least upper - // bound resulting from multiple entity types, the type might have - // the attribute if any of the constituent entity types have the - // attribute in the schema. + // Never, being the bottom type, is a subtype of EntityOrRecord, so + // it could have any attributes. + Type::Never => true, + // An EntityOrRecord might have an open attributes record, in which + // case it could have any attribute. + Type::EntityOrRecord(k) if k.has_open_attributes_record() => true, + // In this case and all following `EntityOrRecord` cases, we know it + // does not have an open attributes record, so we know that an + // attribute may not exist if it is not explicitly listed in the + // type. For an entity, we look this up in the schema. For an + // entity least upper bound resulting from multiple entity + // types, the type might have the attribute if any of the + // constituent entity types have the attribute in the schema. Type::EntityOrRecord(EntityRecordKind::Entity(entity_lub)) => { entity_lub.lub_elements.iter().any(|entity| { schema @@ -366,10 +379,13 @@ impl Type { Type::EntityOrRecord(EntityRecordKind::ActionEntity { attrs, .. }) => { attrs.iter().any(|(found_attr, _)| attr.eq(found_attr)) } - // Other record or entity types always might have some attribute - // since the type may be the result of a least upper bound which - // could have dropped attributes from the type. - Type::EntityOrRecord(_) => true, + // A record will have an attribute if the attribute is in its + // attributes map. Records computed as a LUB may have an open + // attributes record, but that is handled by the first match case. + Type::EntityOrRecord(EntityRecordKind::Record { attrs, .. }) => { + attrs.get_attr(attr).is_some() + } + // No other types may have attributes. _ => false, } } @@ -424,7 +440,10 @@ impl Type { EntityRecordKind::ActionEntity { .. } => Type::json_type("ActionEntity"), }; match rk { - EntityRecordKind::Record { attrs } => { + EntityRecordKind::Record { + attrs, + open_attributes, + } => { let attr_json = attrs .iter() .map(|(attr, attr_ty)| { @@ -437,6 +456,12 @@ impl Type { }) .collect::>(); record_json.insert("attributes".to_string(), attr_json.into()); + if open_attributes.is_open() { + record_json.insert( + "additionalAttributes".to_string(), + open_attributes.is_open().into(), + ); + } } EntityRecordKind::ActionEntity { name, attrs } => { let attr_json = attrs @@ -505,7 +530,9 @@ impl Type { CoreSchemaType::EmptySet => matches!(self, Type::Set { .. }), // empty-set matches a set of any element type CoreSchemaType::Record { attrs } => match self { Type::EntityOrRecord(kind) => match kind { - EntityRecordKind::Record { attrs: self_attrs } => { + EntityRecordKind::Record { + attrs: self_attrs, .. + } => { attrs.iter().all(|(k, v)| { match self_attrs.get_attr(k) { Some(ty) => { @@ -620,7 +647,7 @@ impl TryFrom for cedar_policy_core::entities::SchemaType { ty: EntityType::Concrete(name), }) } - Type::EntityOrRecord(EntityRecordKind::Record { attrs }) => { + Type::EntityOrRecord(EntityRecordKind::Record { attrs, .. }) => { Ok(CoreSchemaType::Record { attrs: { attrs @@ -843,6 +870,18 @@ impl Attributes { }) } + // Determine if the attributes subtype while only allowing for depth + // subtyping. This forbids width subtyping, so there may not be attributes + // present in the subtype that do not exist in the super type. + pub(crate) fn is_subtype_depth_only( + &self, + schema: &ValidatorSchema, + other: &Attributes, + ) -> bool { + other.attrs.keys().collect::>() == self.attrs.keys().collect::>() + && self.is_subtype(schema, other) + } + pub(crate) fn least_upper_bound( schema: &ValidatorSchema, attrs0: &Attributes, @@ -868,6 +907,27 @@ impl IntoIterator for Attributes { } } +/// Used to tag record types to indicate if their attributes record is open or +/// closed. +#[derive(Hash, Ord, PartialOrd, Eq, PartialEq, Debug, Copy, Clone, Serialize)] +pub enum OpenTag { + // The attributes are open. A value of this type may have attributes other + // than those listed. + OpenAttributes, + // The attributes are closed. The attributes for a value of this type must + // exactly match the attributes listed in the type. + ClosedAttributes, +} + +impl OpenTag { + pub(crate) fn is_open(self) -> bool { + match self { + OpenTag::OpenAttributes => true, + OpenTag::ClosedAttributes => false, + } + } +} + /// Represents whether a type is an entity type, record type, or could be either /// /// The subtyping lattice for these types is that @@ -875,7 +935,14 @@ impl IntoIterator for Attributes { #[derive(Hash, Ord, PartialOrd, Eq, PartialEq, Debug, Clone, Serialize)] pub enum EntityRecordKind { /// A record type, with these attributes - Record { attrs: Attributes }, + Record { + /// The attributes that we know must exist (or may exist in the case of + /// optional attributes) for a record with this type along with the + /// types the attributes must have if they do exist. + attrs: Attributes, + /// Encodes whether the attributes for this record are open or closed. + open_attributes: OpenTag, + }, /// Any entity type AnyEntity, /// An entity reference type. An entity reference might be a reference to one @@ -904,9 +971,34 @@ impl EntityRecordKind { } } + /// Return `true` if this entity or record may have additional undeclared + /// attributes. + pub(crate) fn has_open_attributes_record(&self) -> bool { + match self { + // Records explicitly store this information. + EntityRecordKind::Record { + open_attributes, .. + } => open_attributes.is_open(), + // We know Actions never have additional attributes. This is true + // because the upper bound for any two action entities is + // `AnyEntity`, so if we have an ActionEntity here its attributes + // are known precisely. + EntityRecordKind::ActionEntity { .. } => false, + // The `AnyEntity` type has no declared attributes, but it is a + // super type of all other entity types which may have attributes, + // so it clearly may have additional attributes. + EntityRecordKind::AnyEntity => true, + // An entity LUB may not have an open attributes record. The record + // type returned by `get_attributes_type` _may_ be open, but even in + // that case we can account for all attributes that might exist by + // examining the elements of the LUB. + EntityRecordKind::Entity(_) => false, + } + } + pub(crate) fn get_attr(&self, schema: &ValidatorSchema, attr: &str) -> Option { match self { - EntityRecordKind::Record { attrs } => attrs.get_attr(attr).cloned(), + EntityRecordKind::Record { attrs, .. } => attrs.get_attr(attr).cloned(), EntityRecordKind::ActionEntity { attrs, .. } => attrs.get_attr(attr).cloned(), EntityRecordKind::AnyEntity => None, EntityRecordKind::Entity(lub) => { @@ -918,7 +1010,7 @@ impl EntityRecordKind { pub fn all_attrs(&self, schema: &ValidatorSchema) -> Vec { // Wish the clone here could be avoided, but `get_attribute_types` returns an owned `Attributes`. match self { - EntityRecordKind::Record { attrs } => attrs.attrs.keys().cloned().collect(), + EntityRecordKind::Record { attrs, .. } => attrs.attrs.keys().cloned().collect(), EntityRecordKind::ActionEntity { attrs, .. } => attrs.attrs.keys().cloned().collect(), EntityRecordKind::AnyEntity => vec![], EntityRecordKind::Entity(lub) => { @@ -934,9 +1026,42 @@ impl EntityRecordKind { ) -> Option { use EntityRecordKind::*; match (rk0, rk1) { - (Record { attrs: attrs0 }, Record { attrs: attrs1 }) => Some(Record { - attrs: Attributes::least_upper_bound(schema, attrs0, attrs1), - }), + ( + Record { + attrs: attrs0, + open_attributes: open0, + }, + Record { + attrs: attrs1, + open_attributes: open1, + }, + ) => { + let attrs = Attributes::least_upper_bound(schema, attrs0, attrs1); + + // Even though this function will never be called when the + // records are in a subtype relation, it is still possible that + // the LUB attribute set is the same the attribute key sets for + // `rk0` and `rk1`. This occurs when `rk0` and `rk1` have + // identical attribute keys sets with all corresponding + // attributes having a LUB while at least one pair of + // corresponding attributes is not in a subtype relation. + // E.g., Given `{a: true}` and `{a: false}`, the LUB is `{a: bool}`, + // and we know that `a` is the only attribute for this (closed) + // record even though neither is subtype of the other. + let open_attributes = if open0.is_open() + || open1.is_open() + || (attrs.keys().collect::>() + != (attrs0.keys().chain(attrs1.keys()).collect::>())) + { + OpenTag::OpenAttributes + } else { + OpenTag::ClosedAttributes + }; + Some(Record { + attrs, + open_attributes, + }) + } //We cannot take upper bounds of action entities because may_have_attr assumes the list of attrs it complete (ActionEntity { .. }, ActionEntity { .. }) => Some(AnyEntity), (Entity(lub0), Entity(lub1)) => Some(Entity(lub0.least_upper_bound(lub1))), @@ -968,8 +1093,28 @@ impl EntityRecordKind { ) -> bool { use EntityRecordKind::*; match (rk0, rk1) { - (Record { attrs: attrs0 }, Record { attrs: attrs1 }) => { - attrs0.is_subtype(schema, attrs1) + ( + Record { + attrs: attrs0, + open_attributes: open0, + }, + Record { + attrs: attrs1, + open_attributes: open1, + }, + ) => { + // Closed attributes subtype open attributes. A record type with + // open attributes may contain a value that is not in a record + // type with closed attributes, so open attribute record types + // can never subtype closed attribute record types. + (!open0.is_open() || open1.is_open()) + // When `rk1` has open attributes, width subtyping applies since + // there may be attributes in `rk0` that are not listed in + // `rk1`. When `rk1` is closed, a subtype of `rk1` may not have + // any attributes that are not listed in `rk1`, so we apply + // depth subtyping only. + && ((open1.is_open() && attrs0.is_subtype(schema, attrs1)) + || attrs0.is_subtype_depth_only(schema, attrs1)) } (ActionEntity { .. }, ActionEntity { .. }) => false, (Entity(lub0), Entity(lub1)) => lub0.is_subtype(lub1), @@ -1090,6 +1235,33 @@ mod test { assert!(!lub.lub_elements.is_empty()); Type::EntityOrRecord(EntityRecordKind::Entity(lub)) } + + pub(crate) fn open_record_with_required_attributes( + required_attrs: impl IntoIterator, + ) -> Type { + Type::EntityOrRecord(EntityRecordKind::Record { + attrs: Attributes::with_required_attributes(required_attrs), + open_attributes: OpenTag::OpenAttributes, + }) + } + + pub(crate) fn closed_record_with_required_attributes( + required_attrs: impl IntoIterator, + ) -> Type { + Type::record_with_required_attributes(required_attrs, OpenTag::ClosedAttributes) + } + + pub(crate) fn open_record_with_attributes( + attrs: impl IntoIterator, + ) -> Type { + Self::record_with_attributes(attrs, OpenTag::OpenAttributes) + } + + pub(crate) fn closed_record_with_attributes( + attrs: impl IntoIterator, + ) -> Type { + Self::record_with_attributes(attrs, OpenTag::ClosedAttributes) + } } fn assert_least_upper_bound(schema: ValidatorSchema, lhs: Type, rhs: Type, lub: Option) { @@ -1273,60 +1445,102 @@ mod test { } #[test] - fn test_record_lub() { + fn test_record_undef_lub() { assert_least_upper_bound_empty_schema( - Type::any_record(), - Type::any_record(), - Some(Type::record_with_attributes(None)), + Type::open_record_with_attributes(None), + Type::primitive_string(), + None, ); assert_least_upper_bound_empty_schema( - Type::any_record(), - Type::any_entity_reference(), + Type::closed_record_with_attributes(None), + Type::primitive_string(), None, ); assert_least_upper_bound_empty_schema( - Type::record_with_required_attributes([ - ("foo".into(), Type::False), - ("bar".into(), Type::primitive_long()), - ]), - Type::any_entity_reference(), + Type::closed_record_with_attributes(None), + Type::set(Type::primitive_boolean()), None, ); + } + + #[test] + fn test_record_lub() { + assert_least_upper_bound_empty_schema( + Type::closed_record_with_attributes(None), + Type::closed_record_with_attributes(None), + Some(Type::closed_record_with_attributes(None)), + ); + assert_least_upper_bound_empty_schema( + Type::closed_record_with_attributes(None), + Type::open_record_with_attributes(None), + Some(Type::open_record_with_attributes(None)), + ); + assert_least_upper_bound_empty_schema( + Type::open_record_with_attributes(None), + Type::closed_record_with_attributes(None), + Some(Type::open_record_with_attributes(None)), + ); + assert_least_upper_bound_empty_schema( + Type::open_record_with_attributes(None), + Type::open_record_with_attributes(None), + Some(Type::open_record_with_attributes(None)), + ); assert_least_upper_bound_empty_schema( - Type::record_with_required_attributes([ + Type::closed_record_with_required_attributes([ ("foo".into(), Type::False), ("bar".into(), Type::primitive_long()), ]), - Type::record_with_required_attributes([ + Type::closed_record_with_required_attributes([ ("foo".into(), Type::primitive_string()), ("bar".into(), Type::primitive_long()), ]), - Some(Type::record_with_required_attributes([( + Some(Type::open_record_with_required_attributes([( "bar".into(), Type::primitive_long(), )])), ); assert_least_upper_bound_empty_schema( - Type::record_with_required_attributes([ + Type::closed_record_with_required_attributes([("bar".into(), Type::primitive_long())]), + Type::closed_record_with_required_attributes([ + ("foo".into(), Type::primitive_string()), + ("bar".into(), Type::primitive_long()), + ]), + Some(Type::open_record_with_required_attributes([( + "bar".into(), + Type::primitive_long(), + )])), + ); + + assert_least_upper_bound_empty_schema( + Type::closed_record_with_required_attributes([ ("foo".into(), Type::False), ("bar".into(), Type::primitive_long()), ]), - Type::record_with_required_attributes([ + Type::closed_record_with_required_attributes([ ("foo".into(), Type::True), ("baz".into(), Type::primitive_long()), ]), - Some(Type::record_with_required_attributes([( + Some(Type::open_record_with_required_attributes([( + "foo".into(), + Type::primitive_boolean(), + )])), + ); + + assert_least_upper_bound_empty_schema( + Type::closed_record_with_required_attributes([("foo".into(), Type::False)]), + Type::closed_record_with_required_attributes([("foo".into(), Type::True)]), + Some(Type::closed_record_with_required_attributes([( "foo".into(), Type::primitive_boolean(), )])), ); assert_least_upper_bound_empty_schema( - Type::record_with_attributes([ + Type::closed_record_with_attributes([ ( "foo".into(), AttributeType::new(Type::primitive_long(), false), @@ -1336,7 +1550,7 @@ mod test { AttributeType::new(Type::primitive_long(), false), ), ]), - Type::record_with_attributes([ + Type::closed_record_with_attributes([ ( "foo".into(), AttributeType::new(Type::primitive_long(), true), @@ -1346,7 +1560,7 @@ mod test { AttributeType::new(Type::primitive_long(), false), ), ]), - Some(Type::record_with_attributes([ + Some(Type::closed_record_with_attributes([ ( "foo".into(), AttributeType::new(Type::primitive_long(), false), @@ -1358,12 +1572,10 @@ mod test { ])), ); - assert_least_upper_bound_empty_schema(Type::any_record(), Type::primitive_string(), None); - assert_least_upper_bound_empty_schema( - Type::record_with_attributes(None), - Type::primitive_string(), - None, + Type::closed_record_with_required_attributes([("a".into(), Type::primitive_long())]), + Type::closed_record_with_attributes([]), + Some(Type::open_record_with_attributes([])), ); } @@ -1543,11 +1755,24 @@ mod test { #[test] fn test_record_entity_lub() { - assert_least_upper_bound_attr_schema( + assert_least_upper_bound_empty_schema( Type::any_entity_reference(), Type::any_record(), None, ); + assert_least_upper_bound_empty_schema( + Type::closed_record_with_attributes(None), + Type::any_entity_reference(), + None, + ); + assert_least_upper_bound_empty_schema( + Type::closed_record_with_required_attributes([ + ("foo".into(), Type::False), + ("bar".into(), Type::primitive_long()), + ]), + Type::any_entity_reference(), + None, + ); assert_least_upper_bound_attr_schema( Type::named_entity_reference_from_str("foo"), Type::any_record(), @@ -1560,7 +1785,7 @@ mod test { ); assert_least_upper_bound_attr_schema( Type::named_entity_reference_from_str("buz"), - Type::record_with_required_attributes(vec![ + Type::closed_record_with_required_attributes(vec![ ("a".into(), Type::primitive_long()), ("b".into(), Type::primitive_long()), ("c".into(), Type::named_entity_reference_from_str("bar")), @@ -1603,7 +1828,7 @@ mod test { assert_least_upper_bound( schema, Type::named_entity_reference_from_str("U"), - Type::record_with_required_attributes([( + Type::closed_record_with_required_attributes([( "foo".into(), Type::named_entity_reference_from_str("U"), )]), @@ -1699,12 +1924,12 @@ mod test { assert_json_parses_to_schema_type(Type::named_entity_reference_from_str("Foo")); assert_json_parses_to_schema_type(Type::named_entity_reference_from_str("Foo::Bar")); assert_json_parses_to_schema_type(Type::named_entity_reference_from_str("Foo::Bar::Baz")); - assert_json_parses_to_schema_type(Type::record_with_attributes(None)); - assert_json_parses_to_schema_type(Type::record_with_attributes([( + assert_json_parses_to_schema_type(Type::closed_record_with_attributes(None)); + assert_json_parses_to_schema_type(Type::closed_record_with_attributes([( "a".into(), AttributeType::required_attribute(Type::primitive_boolean()), )])); - assert_json_parses_to_schema_type(Type::record_with_attributes([ + assert_json_parses_to_schema_type(Type::closed_record_with_attributes([ ( "a".into(), AttributeType::required_attribute(Type::primitive_boolean()), diff --git a/cedar-policy/Cargo.toml b/cedar-policy/Cargo.toml index c2fb3e58f..ad3e0ae58 100644 --- a/cedar-policy/Cargo.toml +++ b/cedar-policy/Cargo.toml @@ -2,7 +2,7 @@ name = "cedar-policy" edition = "2021" -version = "2.3.3" +version = "2.4.0" license = "Apache-2.0" categories = ["compilers", "config"] description = "Cedar is a language for defining permissions as policies, which describe who should have access to what." @@ -11,8 +11,8 @@ homepage = "https://cedarpolicy.com" repository = "https://github.com/cedar-policy/cedar" [dependencies] -cedar-policy-core = { version = "=2.3.3", path = "../cedar-policy-core" } -cedar-policy-validator = { version = "=2.3.3", path = "../cedar-policy-validator" } +cedar-policy-core = { version = "=2.4.0", path = "../cedar-policy-core" } +cedar-policy-validator = { version = "=2.4.0", path = "../cedar-policy-validator" } ref-cast = "1.0" serde = { version = "1.0", features = ["derive", "rc"] } serde_json = "1.0" diff --git a/cedar-policy/src/api.rs b/cedar-policy/src/api.rs index 3df0b618d..01eaa035d 100644 --- a/cedar-policy/src/api.rs +++ b/cedar-policy/src/api.rs @@ -734,16 +734,14 @@ impl From for SchemaError { Self::CycleInActionHierarchy } cedar_policy_validator::SchemaError::EntityTypeParseError(e) => { - Self::EntityTypeParse(ParseErrors(e)) - } - cedar_policy_validator::SchemaError::NamespaceParseError(e) => { - Self::NamespaceParse(ParseErrors(e)) + Self::EntityTypeParse(e) } + cedar_policy_validator::SchemaError::NamespaceParseError(e) => Self::NamespaceParse(e), cedar_policy_validator::SchemaError::CommonTypeParseError(e) => { - Self::CommonTypeParseError(ParseErrors(e)) + Self::CommonTypeParseError(e) } cedar_policy_validator::SchemaError::ExtensionTypeParseError(e) => { - Self::ExtensionTypeParse(ParseErrors(e)) + Self::ExtensionTypeParse(e) } cedar_policy_validator::SchemaError::ActionEntityTypeDeclared => { Self::ActionEntityTypeDeclared @@ -951,13 +949,33 @@ impl std::fmt::Display for EntityId { #[derive(Debug, Clone, Hash, PartialEq, Eq, PartialOrd, Ord, RefCast)] pub struct EntityTypeName(ast::Name); +impl EntityTypeName { + /// Get the basename of the `EntityTypeName` (ie, with namespaces stripped). + pub fn basename(&self) -> &str { + self.0.basename().as_ref() + } + + /// Get the namespace of the `EntityTypeName`, as components + pub fn namespace_components(&self) -> impl Iterator { + self.0.namespace_components().map(AsRef::as_ref) + } + + /// Get the full namespace of the `EntityTypeName`, as a single string. + /// + /// Examples: + /// - `foo::bar` --> the namespace is `"foo"` + /// - `bar` --> the namespace is `""` + /// - `foo::bar::baz` --> the namespace is `"foo::bar"` + pub fn namespace(&self) -> String { + self.0.namespace() + } +} + impl FromStr for EntityTypeName { type Err = ParseErrors; fn from_str(namespace_type_str: &str) -> Result { - ast::Name::from_normalized_str(namespace_type_str) - .map(EntityTypeName) - .map_err(ParseErrors) + ast::Name::from_normalized_str(namespace_type_str).map(EntityTypeName) } } @@ -977,9 +995,7 @@ impl FromStr for EntityNamespace { type Err = ParseErrors; fn from_str(namespace_str: &str) -> Result { - ast::Name::from_normalized_str(namespace_str) - .map(EntityNamespace) - .map_err(ParseErrors) + ast::Name::from_normalized_str(namespace_str).map(EntityNamespace) } } @@ -1048,9 +1064,7 @@ impl FromStr for EntityUid { // You can't actually `#[deprecated]` a trait implementation or trait // method. fn from_str(uid_str: &str) -> Result { - ast::EntityUID::from_normalized_str(uid_str) - .map(EntityUid) - .map_err(ParseErrors) + ast::EntityUID::from_normalized_str(uid_str).map(EntityUid) } } @@ -1351,7 +1365,7 @@ impl Template { /// If the `id` is None, the parser will use the default "policy0". /// The behavior around None may change in the future. pub fn parse(id: Option, src: impl AsRef) -> Result { - let ast = parser::parse_policy_template(id, src.as_ref()).map_err(ParseErrors)?; + let ast = parser::parse_policy_template(id, src.as_ref())?; Ok(Self { ast, lossless: LosslessPolicy::policy_or_template_text(src.as_ref()), @@ -1728,7 +1742,7 @@ impl Policy { /// If `id` is None, then "policy0" will be used. /// The behavior around None may change in the future. pub fn parse(id: Option, policy_src: impl AsRef) -> Result { - let inline_ast = parser::parse_policy(id, policy_src.as_ref()).map_err(ParseErrors)?; + let inline_ast = parser::parse_policy(id, policy_src.as_ref())?; let (_, ast) = ast::Template::link_static_policy(inline_ast); Ok(Self { ast, @@ -1912,9 +1926,7 @@ impl FromStr for Expression { /// create an Expression using Cedar syntax fn from_str(expression: &str) -> Result { - ast::Expr::from_str(expression) - .map_err(ParseErrors) - .map(Expression) + ast::Expr::from_str(expression).map(Expression) } } @@ -1971,9 +1983,7 @@ impl FromStr for RestrictedExpression { /// create a `RestrictedExpression` using Cedar syntax fn from_str(expression: &str) -> Result { - ast::RestrictedExpr::from_str(expression) - .map_err(ParseErrors) - .map(RestrictedExpression) + ast::RestrictedExpr::from_str(expression).map(RestrictedExpression) } } @@ -2346,6 +2356,37 @@ mod entity_uid_tests { let euid = EntityUid::from_type_name_and_id(entity_type_name, entity_id); assert_eq!(euid.id().as_ref(), "bobby"); assert_eq!(euid.type_name().to_string(), "Chess::Master"); + assert_eq!(euid.type_name().basename(), "Master"); + assert_eq!(euid.type_name().namespace(), "Chess"); + assert_eq!(euid.type_name().namespace_components().count(), 1); + } + + /// building an `EntityUid` from components, with no namespace + #[test] + fn entity_uid_no_namespace() { + let entity_id = EntityId::from_str("bobby").expect("failed at constructing EntityId"); + let entity_type_name = + EntityTypeName::from_str("User").expect("failed at constructing EntityTypeName"); + let euid = EntityUid::from_type_name_and_id(entity_type_name, entity_id); + assert_eq!(euid.id().as_ref(), "bobby"); + assert_eq!(euid.type_name().to_string(), "User"); + assert_eq!(euid.type_name().basename(), "User"); + assert_eq!(euid.type_name().namespace(), String::new()); + assert_eq!(euid.type_name().namespace_components().count(), 0); + } + + /// building an `EntityUid` from components, with many nested namespaces + #[test] + fn entity_uid_nested_namespaces() { + let entity_id = EntityId::from_str("bobby").expect("failed at constructing EntityId"); + let entity_type_name = EntityTypeName::from_str("A::B::C::D::Z") + .expect("failed at constructing EntityTypeName"); + let euid = EntityUid::from_type_name_and_id(entity_type_name, entity_id); + assert_eq!(euid.id().as_ref(), "bobby"); + assert_eq!(euid.type_name().to_string(), "A::B::C::D::Z"); + assert_eq!(euid.type_name().basename(), "Z"); + assert_eq!(euid.type_name().namespace(), "A::B::C::D"); + assert_eq!(euid.type_name().namespace_components().count(), 4); } /// building an `EntityUid` from components, including escapes @@ -2361,6 +2402,9 @@ mod entity_uid_tests { // the EntityId has the literal backslash characters in it assert_eq!(euid.id().as_ref(), r#"bobby\'s sister:\nVeronica"#); assert_eq!(euid.type_name().to_string(), "Hockey::Master"); + assert_eq!(euid.type_name().basename(), "Master"); + assert_eq!(euid.type_name().namespace(), "Hockey"); + assert_eq!(euid.type_name().namespace_components().count(), 1); } /// building an `EntityUid` from components, including backslashes @@ -2407,6 +2451,9 @@ mod entity_uid_tests { }; assert_eq!(euid.id().as_ref(), " hi there are spaces "); assert_eq!(euid.type_name().to_string(), "A::B::C"); // expect to have been normalized + assert_eq!(euid.type_name().basename(), "C"); + assert_eq!(euid.type_name().namespace(), "A::B"); + assert_eq!(euid.type_name().namespace_components().count(), 2); let policy = Policy::from_str( r#" @@ -2427,6 +2474,9 @@ permit(principal == A :: B " hi there are\n spaces and\n newlines " ); assert_eq!(euid.type_name().to_string(), "A::B::C::D"); // expect to have been normalized + assert_eq!(euid.type_name().basename(), "D"); + assert_eq!(euid.type_name().namespace(), "A::B::C"); + assert_eq!(euid.type_name().namespace_components().count(), 3); } #[test] @@ -2435,7 +2485,7 @@ permit(principal == A :: B assert!(matches!(result, Err(ParseErrors(_)))); let error = result.err().unwrap(); - assert!(error.to_string().contains("Unrecognized token `'`")); + assert!(error.to_string().contains("invalid token")); } /// parsing an `EntityUid` from string @@ -2490,6 +2540,9 @@ permit(principal == A :: B }; assert_eq!(parsed_euid.id().as_ref(), "hi"); assert_eq!(parsed_euid.type_name().to_string(), "A::B::C::D::E"); // expect to have been normalized + assert_eq!(parsed_euid.type_name().basename(), "E"); + assert_eq!(parsed_euid.type_name().namespace(), "A::B::C::D"); + assert_eq!(parsed_euid.type_name().namespace_components().count(), 4); } /// test that we can parse the `Display` output of `EntityUid` @@ -3130,14 +3183,20 @@ mod schema_based_parsing_tests { Some(Ok(EvalResult::Record(_))) )); { - let Some(Ok(EvalResult::Set(set))) = parsed.attr("hr_contacts") else { panic!("expected hr_contacts attr to exist and be a Set") }; + let Some(Ok(EvalResult::Set(set))) = parsed.attr("hr_contacts") else { + panic!("expected hr_contacts attr to exist and be a Set") + }; let contact = set.iter().next().expect("should be at least one contact"); assert!(matches!(contact, EvalResult::Record(_))); }; { - let Some(Ok(EvalResult::Record(rec))) = parsed.attr("json_blob") else { panic!("expected json_blob attr to exist and be a Record") }; + let Some(Ok(EvalResult::Record(rec))) = parsed.attr("json_blob") else { + panic!("expected json_blob attr to exist and be a Record") + }; let inner3 = rec.get("inner3").expect("expected inner3 attr to exist"); - let EvalResult::Record(rec) = inner3 else { panic!("expected inner3 to be a Record") }; + let EvalResult::Record(rec) = inner3 else { + panic!("expected inner3 to be a Record") + }; let innerinner = rec .get("innerinner") .expect("expected innerinner attr to exist"); @@ -3166,14 +3225,20 @@ mod schema_based_parsing_tests { )))) ); { - let Some(Ok(EvalResult::Set(set))) = parsed.attr("hr_contacts") else { panic!("expected hr_contacts attr to exist and be a Set") }; + let Some(Ok(EvalResult::Set(set))) = parsed.attr("hr_contacts") else { + panic!("expected hr_contacts attr to exist and be a Set") + }; let contact = set.iter().next().expect("should be at least one contact"); assert!(matches!(contact, EvalResult::EntityUid(_))); }; { - let Some(Ok(EvalResult::Record(rec))) = parsed.attr("json_blob") else { panic!("expected json_blob attr to exist and be a Record") }; + let Some(Ok(EvalResult::Record(rec))) = parsed.attr("json_blob") else { + panic!("expected json_blob attr to exist and be a Record") + }; let inner3 = rec.get("inner3").expect("expected inner3 attr to exist"); - let EvalResult::Record(rec) = inner3 else { panic!("expected inner3 to be a Record") }; + let EvalResult::Record(rec) = inner3 else { + panic!("expected inner3 to be a Record") + }; let innerinner = rec .get("innerinner") .expect("expected innerinner attr to exist");