Enable the pedantic lint needless_pass_by_value

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"

cedar-policy-cli/src/lib.rs

@@ -947,7 +947,7 @@ fn translate_policy_inner(args: &TranslatePolicyArgs) -> Result<String> {
     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<String> {
         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<Path>, 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<PolicySet> {
+fn rename_from_id_annotation(ps: &PolicySet) -> Result<PolicySet> {
     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

cedar-policy-core/src/est/expr.rs

@@ -1511,7 +1511,7 @@ impl TryFrom<&Node<Option<cst::Member>>> 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<T>(
 
 /// Return a wrong arity error if the iterator has any elements.
 pub fn require_zero_arguments<T>(
-    args: impl ExactSizeIterator<Item = T>,
+    args: &impl ExactSizeIterator<Item = T>,
     fn_name: &'static str,
     loc: &Loc,
 ) -> Result<(), ParseErrors> {

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<Option<cst::Relation>> {
                 });
                 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<SmolStr>, loc: Loc) -> ast::Expr {
+fn construct_exprs_extended_has(t: ast::Expr, attrs: &NonEmpty<SmolStr>, 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<SmolStr>, 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()),
             )

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

@@ -834,11 +834,12 @@ impl ParseErrors {
 
     /// Flatten a `Vec<ParseErrors>` into a single `ParseErrors`, returning
     /// `None` if the input vector is empty.
-    pub(crate) fn flatten(v: Vec<ParseErrors>) -> Option<Self> {
-        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<Item = ParseErrors>) -> Option<Self> {
+        let mut errs = errs.into_iter();
+        let mut first = errs.next()?;
+        for inner in errs {
+            first.extend(inner);
+        }
         Some(first)
     }
 

cedar-policy-formatter/src/pprint/doc.rs

@@ -415,7 +415,7 @@ impl Doc for Node<Option<Unary>> {
                                         } else {
                                             RcDoc::text("-")
                                         },
-                                        comment.get(i as usize)?.clone(),
+                                        comment.get(i as usize)?,
                                         RcDoc::nil(),
                                     ))
                                 })

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<Comment<'src>>,
     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)

cedar-policy-validator/src/cedar_schema/fmt.rs

@@ -95,21 +95,22 @@ impl<N: Display> Display for json_schema::RecordType<N> {
     }
 }
 
-/// Create a non-empty with borrowed contents from a slice
-fn non_empty_slice<T>(v: &[T]) -> Option<NonEmpty<&T>> {
-    NonEmpty::collect(v.iter())
-}
-
-fn fmt_vec<T: Display>(f: &mut std::fmt::Formatter<'_>, ets: NonEmpty<T>) -> std::fmt::Result {
-    let contents = ets.iter().map(T::to_string).join(", ");
-    write!(f, "[{contents}]")
+fn fmt_non_empty_slice<T: Display>(
+    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<N: Display> Display for json_schema::EntityType<N> {
     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<N: Display> Display for json_schema::EntityType<N> {
 
 impl<N: Display> Display for json_schema::ActionType<N> {
     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<N: Display> Display for json_schema::ActionType<N> {
                 (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}}")?;
                 }

cedar-policy-validator/src/entity_manifest/type_annotations.rs

@@ -69,15 +69,15 @@ 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(),
                                     }
                                 })?,
                                 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(),
                                     })?;

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<dyn Iterator<Item = RequestEnv<'b>> + 'b> {
+    ) -> Box<dyn Iterator<Item = RequestEnv<'b>> + '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

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<Type> {
+    pub(crate) fn euid_literal(entity: &EntityUID, schema: &ValidatorSchema) -> Option<Type> {
         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),
         );
 

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<Type> {
         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()),
         }
     }