diff --git a/Cargo.toml b/Cargo.toml index 20d2569ef..ad556ee7a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -43,6 +43,7 @@ rust-2018-idioms = "deny" # means we'll see it in local runs. [workspace.lints.clippy] nursery = { level = "warn", priority = -1 } +needless_pass_by_value = "warn" # These lints may be worth enforcing, but cause a lot of noise at the moment. use_self = "allow" diff --git a/cedar-policy-cli/src/lib.rs b/cedar-policy-cli/src/lib.rs index 9a001c6b9..089026f20 100644 --- a/cedar-policy-cli/src/lib.rs +++ b/cedar-policy-cli/src/lib.rs @@ -947,7 +947,7 @@ fn translate_policy_inner(args: &TranslatePolicyArgs) -> Result { let translate = match args.direction { PolicyTranslationDirection::CedarToJson => translate_policy_to_json, }; - read_from_file_or_stdin(args.input_file.clone(), "policy").and_then(translate) + read_from_file_or_stdin(args.input_file.as_ref(), "policy").and_then(translate) } pub fn translate_policy(args: &TranslatePolicyArgs) -> CedarExitCode { @@ -984,7 +984,7 @@ fn translate_schema_inner(args: &TranslateSchemaArgs) -> Result { SchemaTranslationDirection::JsonToCedar => translate_schema_to_cedar, SchemaTranslationDirection::CedarToJson => translate_schema_to_json, }; - read_from_file_or_stdin(args.input_file.clone(), "schema").and_then(translate) + read_from_file_or_stdin(args.input_file.as_ref(), "schema").and_then(translate) } pub fn translate_schema(args: &TranslateSchemaArgs) -> CedarExitCode { @@ -1377,7 +1377,7 @@ fn load_entities(entities_filename: impl AsRef, schema: Option<&Schema>) - /// This will rename template-linked policies to the id of their template, which may /// cause id conflicts, so only call this function before instancing /// templates into the policy set. -fn rename_from_id_annotation(ps: PolicySet) -> Result { +fn rename_from_id_annotation(ps: &PolicySet) -> Result { let mut new_ps = PolicySet::new(); let t_iter = ps.templates().map(|t| match t.annotation("id") { None => Ok(t.clone()), @@ -1443,7 +1443,7 @@ fn read_cedar_policy_set( Report::new(err).with_source_code(NamedSource::new(name, ps_str)) }) .wrap_err_with(|| format!("failed to parse {context}"))?; - rename_from_id_annotation(ps) + rename_from_id_annotation(&ps) } /// Read a policy set, static policy or policy template, in Cedar JSON (EST) syntax, from the file given diff --git a/cedar-policy-core/src/est/expr.rs b/cedar-policy-core/src/est/expr.rs index 49edc729c..8e401b44f 100644 --- a/cedar-policy-core/src/est/expr.rs +++ b/cedar-policy-core/src/est/expr.rs @@ -1511,7 +1511,7 @@ impl TryFrom<&Node>> for Expr { extract_single_argument(args, "containsAny()", &access.loc)?, )), "isEmpty" => { - require_zero_arguments(args, "isEmpty()", &access.loc)?; + require_zero_arguments(&args, "isEmpty()", &access.loc)?; Either::Right(Expr::is_empty(left)) } "getTag" => Either::Right(Expr::get_tag( @@ -1584,7 +1584,7 @@ pub fn extract_single_argument( /// Return a wrong arity error if the iterator has any elements. pub fn require_zero_arguments( - args: impl ExactSizeIterator, + args: &impl ExactSizeIterator, fn_name: &'static str, loc: &Loc, ) -> Result<(), ParseErrors> { diff --git a/cedar-policy-core/src/parser/cst_to_ast.rs b/cedar-policy-core/src/parser/cst_to_ast.rs index 882ce4480..88ceecc2a 100644 --- a/cedar-policy-core/src/parser/cst_to_ast.rs +++ b/cedar-policy-core/src/parser/cst_to_ast.rs @@ -490,7 +490,7 @@ impl ast::UnreservedId { "containsAny" => extract_single_argument(args.into_iter(), "containsAny", loc) .map(|arg| construct_method_contains_any(e, arg, loc.clone())), "isEmpty" => { - require_zero_arguments(args.into_iter(), "isEmpty", loc)?; + require_zero_arguments(&args.into_iter(), "isEmpty", loc)?; Ok(construct_method_is_empty(e, loc.clone())) } "getTag" => extract_single_argument(args.into_iter(), "getTag", loc) @@ -984,7 +984,7 @@ impl Node> { }); let (target, field) = flatten_tuple_2(maybe_target, maybe_field)?; Ok(ExprOrSpecial::Expr { - expr: construct_exprs_extended_has(target, field, self.loc.clone()), + expr: construct_exprs_extended_has(target, &field, &self.loc), loc: self.loc.clone(), }) } @@ -1941,7 +1941,7 @@ fn construct_expr_has_attr(t: ast::Expr, s: SmolStr, loc: Loc) -> ast::Expr { fn construct_expr_get_attr(t: ast::Expr, s: SmolStr, loc: Loc) -> ast::Expr { ast::ExprBuilder::new().with_source_loc(loc).get_attr(t, s) } -fn construct_exprs_extended_has(t: ast::Expr, attrs: NonEmpty, loc: Loc) -> ast::Expr { +fn construct_exprs_extended_has(t: ast::Expr, attrs: &NonEmpty, loc: &Loc) -> ast::Expr { let (first, rest) = attrs.split_first(); let has_expr = construct_expr_has_attr(t.clone(), first.to_owned(), loc.clone()); let get_expr = construct_expr_get_attr(t, first.to_owned(), loc.clone()); @@ -1973,7 +1973,7 @@ fn construct_exprs_extended_has(t: ast::Expr, attrs: NonEmpty, loc: Loc has_expr, construct_expr_has_attr(get_expr.clone(), attr.to_owned(), loc.clone()), std::iter::empty(), - &loc, + loc, ), construct_expr_get_attr(get_expr, attr.to_owned(), loc.clone()), ) diff --git a/cedar-policy-core/src/parser/err.rs b/cedar-policy-core/src/parser/err.rs index 49b750523..ab96cf869 100644 --- a/cedar-policy-core/src/parser/err.rs +++ b/cedar-policy-core/src/parser/err.rs @@ -834,11 +834,12 @@ impl ParseErrors { /// Flatten a `Vec` into a single `ParseErrors`, returning /// `None` if the input vector is empty. - pub(crate) fn flatten(v: Vec) -> Option { - let (first, rest) = v.split_first()?; - let mut first = first.clone(); - rest.iter() - .for_each(|errs| first.extend(errs.iter().cloned())); + pub(crate) fn flatten(errs: impl IntoIterator) -> Option { + let mut errs = errs.into_iter(); + let mut first = errs.next()?; + for inner in errs { + first.extend(inner); + } Some(first) } diff --git a/cedar-policy-formatter/src/pprint/doc.rs b/cedar-policy-formatter/src/pprint/doc.rs index 22e920801..bb31f02a5 100644 --- a/cedar-policy-formatter/src/pprint/doc.rs +++ b/cedar-policy-formatter/src/pprint/doc.rs @@ -415,7 +415,7 @@ impl Doc for Node> { } else { RcDoc::text("-") }, - comment.get(i as usize)?.clone(), + comment.get(i as usize)?, RcDoc::nil(), )) }) diff --git a/cedar-policy-formatter/src/pprint/utils.rs b/cedar-policy-formatter/src/pprint/utils.rs index 7ad9d4481..ad8e3d507 100644 --- a/cedar-policy-formatter/src/pprint/utils.rs +++ b/cedar-policy-formatter/src/pprint/utils.rs @@ -14,6 +14,8 @@ * limitations under the License. */ +use std::borrow::Borrow; + use itertools::Itertools; use pretty::RcDoc; @@ -132,11 +134,11 @@ pub fn get_comment_in_range<'src>( /// trailing comment, then it will introduce a newline at the end. pub fn add_comment<'src>( d: RcDoc<'src>, - comment: Comment<'src>, + comment: impl Borrow>, next_doc: RcDoc<'src>, ) -> RcDoc<'src> { - let leading_comment = comment.leading_comment(); - let trailing_comment = comment.trailing_comment(); + let leading_comment = comment.borrow().leading_comment(); + let trailing_comment = comment.borrow().trailing_comment(); let leading_comment_doc = get_leading_comment_doc_from_str(leading_comment); let trailing_comment_doc = get_trailing_comment_doc_from_str(trailing_comment, next_doc); leading_comment_doc.append(d).append(trailing_comment_doc) diff --git a/cedar-policy-validator/src/cedar_schema/fmt.rs b/cedar-policy-validator/src/cedar_schema/fmt.rs index 5f20f11e1..f559f66a2 100644 --- a/cedar-policy-validator/src/cedar_schema/fmt.rs +++ b/cedar-policy-validator/src/cedar_schema/fmt.rs @@ -95,21 +95,22 @@ impl Display for json_schema::RecordType { } } -/// Create a non-empty with borrowed contents from a slice -fn non_empty_slice(v: &[T]) -> Option> { - NonEmpty::collect(v.iter()) -} - -fn fmt_vec(f: &mut std::fmt::Formatter<'_>, ets: NonEmpty) -> std::fmt::Result { - let contents = ets.iter().map(T::to_string).join(", "); - write!(f, "[{contents}]") +fn fmt_non_empty_slice( + f: &mut std::fmt::Formatter<'_>, + (head, tail): (&T, &[T]), +) -> std::fmt::Result { + write!(f, "[{head}")?; + for e in tail { + write!(f, ", {e}")?; + } + write!(f, "]") } impl Display for json_schema::EntityType { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - if let Some(non_empty) = non_empty_slice(&self.member_of_types) { + if let Some(non_empty) = self.member_of_types.split_first() { write!(f, " in ")?; - fmt_vec(f, non_empty)?; + fmt_non_empty_slice(f, non_empty)?; } let ty = &self.shape; @@ -128,18 +129,14 @@ impl Display for json_schema::EntityType { impl Display for json_schema::ActionType { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - if let Some(parents) = self - .member_of - .as_ref() - .and_then(|refs| non_empty_slice(refs.as_slice())) - { + if let Some(parents) = self.member_of.as_ref().and_then(|refs| refs.split_first()) { write!(f, " in ")?; - fmt_vec(f, parents)?; + fmt_non_empty_slice(f, parents)?; } if let Some(spec) = &self.applies_to { match ( - non_empty_slice(spec.principal_types.as_slice()), - non_empty_slice(spec.resource_types.as_slice()), + spec.principal_types.split_first(), + spec.resource_types.split_first(), ) { // One of the lists is empty // This can only be represented by the empty action @@ -151,9 +148,9 @@ impl Display for json_schema::ActionType { (Some(ps), Some(rs)) => { write!(f, " appliesTo {{")?; write!(f, "\n principal: ")?; - fmt_vec(f, ps)?; + fmt_non_empty_slice(f, ps)?; write!(f, ",\n resource: ")?; - fmt_vec(f, rs)?; + fmt_non_empty_slice(f, rs)?; write!(f, ",\n context: {}", &spec.context.0)?; write!(f, "\n}}")?; } diff --git a/cedar-policy-validator/src/entity_manifest/type_annotations.rs b/cedar-policy-validator/src/entity_manifest/type_annotations.rs index 0c70e771e..0bf4b071e 100644 --- a/cedar-policy-validator/src/entity_manifest/type_annotations.rs +++ b/cedar-policy-validator/src/entity_manifest/type_annotations.rs @@ -69,7 +69,7 @@ impl RootAccessTrie { match key { EntityRoot::Literal(lit) => slice.to_typed( request_type, - &Type::euid_literal(lit.clone(), schema).ok_or_else(|| { + &Type::euid_literal(lit, schema).ok_or_else(|| { MismatchedMissingEntityError { entity: lit.clone(), } @@ -77,7 +77,7 @@ impl RootAccessTrie { schema, )?, EntityRoot::Var(Var::Action) => { - let ty = Type::euid_literal(request_type.action.clone(), schema) + let ty = Type::euid_literal(&request_type.action, schema) .ok_or_else(|| MismatchedMissingEntityError { entity: request_type.action.clone(), })?; diff --git a/cedar-policy-validator/src/typecheck.rs b/cedar-policy-validator/src/typecheck.rs index 36e87e63e..a6d4e9c65 100644 --- a/cedar-policy-validator/src/typecheck.rs +++ b/cedar-policy-validator/src/typecheck.rs @@ -184,12 +184,11 @@ impl<'a> Typechecker<'a> { // explicit that `expect_type` will be called for every element of // request_env without short circuiting. let policy_condition = &t.condition(); - for requeste in self - .unlinked_request_envs() - .flat_map(|env| self.link_request_env(env, t)) - { - let check = typecheck_fn(&requeste, policy_condition); - result_checks.push((requeste, check)) + for unlinked_e in self.unlinked_request_envs() { + for linked_e in self.link_request_env(&unlinked_e, t) { + let check = typecheck_fn(&linked_e, policy_condition); + result_checks.push((linked_e, check)) + } } result_checks } @@ -208,7 +207,7 @@ impl<'a> Typechecker<'a> { let mut policy_checks = Vec::new(); for t in policy_templates.iter() { let condition_expr = t.condition(); - for linked_env in self.link_request_env(request.clone(), t) { + for linked_env in self.link_request_env(&request, t) { let mut type_errors = Vec::new(); let empty_prior_capability = CapabilitySet::new(); let ty = self.expect_type( @@ -276,11 +275,11 @@ impl<'a> Typechecker<'a> { /// Given a request environment and a template, return new environments /// formed by linking template slots with possible entity types. - fn link_request_env<'b>( + fn link_request_env<'b, 'c>( &'b self, - env: RequestEnv<'b>, + env: &'c RequestEnv<'b>, t: &'b Template, - ) -> Box> + 'b> { + ) -> Box> + 'c> { match env { RequestEnv::UndeclaredAction => Box::new(std::iter::once(RequestEnv::UndeclaredAction)), RequestEnv::DeclaredAction { @@ -469,7 +468,7 @@ impl<'a> Typechecker<'a> { // detected by a different part of the validator, so a ValidationError is // not generated here. We still return `TypecheckFail` so that // typechecking is not considered successful. - match Type::euid_literal((**euid).clone(), self.schema) { + match Type::euid_literal(euid.as_ref(), self.schema) { // The entity type is undeclared, but that's OK for a // partial schema. The attributes record will be empty if we // try to access it later, so all attributes will have the diff --git a/cedar-policy-validator/src/types.rs b/cedar-policy-validator/src/types.rs index da4a162f2..d8e22c4a9 100644 --- a/cedar-policy-validator/src/types.rs +++ b/cedar-policy-validator/src/types.rs @@ -119,10 +119,10 @@ impl Type { /// Construct a type for a literal EUID. This type will be a named entity /// type for the type of the [`EntityUID`]. - pub(crate) fn euid_literal(entity: EntityUID, schema: &ValidatorSchema) -> Option { + pub(crate) fn euid_literal(entity: &EntityUID, schema: &ValidatorSchema) -> Option { if entity.entity_type().is_action() { schema - .get_action_id(&entity) + .get_action_id(entity) .map(Type::entity_reference_from_action_id) } else { schema @@ -2534,7 +2534,7 @@ mod test { #[test] fn test_action_entity_lub() { let action_view_ty = - Type::euid_literal(r#"Action::"view""#.parse().unwrap(), &action_schema()).unwrap(); + Type::euid_literal(&r#"Action::"view""#.parse().unwrap(), &action_schema()).unwrap(); assert_least_upper_bound( action_schema(), @@ -2551,7 +2551,7 @@ mod test { action_schema(), ValidationMode::Strict, action_view_ty.clone(), - Type::euid_literal(r#"Action::"edit""#.parse().unwrap(), &action_schema()).unwrap(), + Type::euid_literal(&r#"Action::"edit""#.parse().unwrap(), &action_schema()).unwrap(), Ok(action_view_ty.clone()), ); @@ -2561,14 +2561,16 @@ mod test { action_schema(), ValidationMode::Permissive, action_view_ty.clone(), - Type::euid_literal(r#"ns::Action::"move""#.parse().unwrap(), &action_schema()).unwrap(), + Type::euid_literal(&r#"ns::Action::"move""#.parse().unwrap(), &action_schema()) + .unwrap(), Ok(Type::any_entity_reference()), ); assert_least_upper_bound( action_schema(), ValidationMode::Strict, action_view_ty.clone(), - Type::euid_literal(r#"ns::Action::"move""#.parse().unwrap(), &action_schema()).unwrap(), + Type::euid_literal(&r#"ns::Action::"move""#.parse().unwrap(), &action_schema()) + .unwrap(), Err(LubHelp::EntityType), ); diff --git a/cedar-policy-validator/src/types/request_env.rs b/cedar-policy-validator/src/types/request_env.rs index 22f18a533..dffe00f41 100644 --- a/cedar-policy-validator/src/types/request_env.rs +++ b/cedar-policy-validator/src/types/request_env.rs @@ -98,7 +98,7 @@ impl<'a> RequestEnv<'a> { /// [`Type`] of the `action` for this request environment pub fn action_type(&self, schema: &ValidatorSchema) -> Option { match self.action_entity_uid() { - Some(action) => Type::euid_literal(action.clone(), schema), + Some(action) => Type::euid_literal(action, schema), None => Some(Type::any_entity_reference()), } }