Repository: cedar-policy/cedar
Pull request #1398 (2024-12-30 21:26:05)

Enable the pedantic lint `needless_pass_by_value`
## Description of changes

Enable the pedantic lint `needless_pass_by_value` and resolve some new warnings

## Issue #, if available

## Checklist for requesting a review

The change in this PR is (choose one, and delete the other options):

- [ ] A breaking change requiring a major version bump to `cedar-policy` (e.g., changes to the signature of an existing API).
- [ ] A backwards-compatible change requiring a minor version bump to `cedar-policy` (e.g., addition of a new API).
- [ ] A bug fix or other functionality change requiring a patch to `cedar-policy`.
- [x] A change "invisible" to users (e.g., documentation, changes to "internal" crates like `cedar-policy-core`, `cedar-validator`, etc.)
- [ ] A change (breaking or otherwise) that only impacts unreleased or experimental code.

I confirm that this PR (choose one, and delete the other options):

- [ ] Updates the "Unreleased" section of the CHANGELOG with a description of my change (required for major/minor version bumps).
- [x] Does not update the CHANGELOG because my change does not significantly impact released code.

I confirm that [`cedar-spec`](https://github.com/cedar-policy/cedar-spec) (choose one, and delete the other options):

- [x] Does not require updates because my change does not impact the Cedar formal model or DRT infrastructure.
- [ ] Requires updates, and I have made / will make these updates myself. (Please include in your description a timeline or link to the relevant PR in `cedar-spec`, and how you have tested that your updates are correct.)
- [ ] Requires updates, but I do not plan to make them in the near future. (Make sure that your changes are hidden behind a feature flag to mark them as experimental.)
- [ ] I'm not sure how my change impacts `cedar-spec`. (Post your PR anyways, and we'll discuss in the comments.)

I confirm that [`docs.cedarpolicy.com`](https://docs.cedarpolicy.com/) (choose one, and delete the other options):

- [x] Does not require updates because my change does not impact the Cedar language specification.
- [ ] Requires updates, and I have made / will make these updates myself. (Please include in your description a timeline or link to the relevant PR in [`cedar-docs`](https://github.com/cedar-policy/cedar-docs). PRs should be targeted at a `staging-X.Y` branch, not `main`.)
- [ ] I'm not sure how my change impacts the documentation. (Post your PR anyways, and we'll discuss in the comments.)



Patch:
diff --git a/Cargo.toml b/Cargo.toml
index 20d2569e..ad556ee7 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 9a001c6b..089026f2 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<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
diff --git a/cedar-policy-core/src/est/expr.rs b/cedar-policy-core/src/est/expr.rs
index 49edc729..8e401b44 100644
--- a/cedar-policy-core/src/est/expr.rs
+++ b/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> {
diff --git a/cedar-policy-core/src/parser/cst_to_ast.rs b/cedar-policy-core/src/parser/cst_to_ast.rs
index 882ce448..88ceecc2 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<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()),
             )
diff --git a/cedar-policy-core/src/parser/err.rs b/cedar-policy-core/src/parser/err.rs
index 49b75052..ab96cf86 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<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)
     }
 
diff --git a/cedar-policy-formatter/src/pprint/doc.rs b/cedar-policy-formatter/src/pprint/doc.rs
index 22e92080..bb31f02a 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<Option<Unary>> {
                                         } 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 7ad9d448..ad8e3d50 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<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)
diff --git a/cedar-policy-validator/src/cedar_schema/fmt.rs b/cedar-policy-validator/src/cedar_schema/fmt.rs
index 5f20f11e..f559f66a 100644
--- a/cedar-policy-validator/src/cedar_schema/fmt.rs
+++ b/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}}")?;
                 }
diff --git a/cedar-policy-validator/src/cedar_schema/grammar.lalrpop b/cedar-policy-validator/src/cedar_schema/grammar.lalrpop
index 3db5e48a..91052835 100644
--- a/cedar-policy-validator/src/cedar_schema/grammar.lalrpop
+++ b/cedar-policy-validator/src/cedar_schema/grammar.lalrpop
@@ -90,14 +90,7 @@ match {
 
 }
 
-#[inline]
-AnyIdent: Node<AnyId> = {
-    <l:@L> <id:IDENTIFIER> <r:@R>
-        // `IDENTIFIER` should produce a valid `AnyId`
-        => Node::with_source_loc(AnyId::from_str(id).unwrap(), Loc::new(l..r, Arc::clone(src))),
-}
-
-// Annotations := {'@' IDENTIFIER '(' String ')'}
+// Annotations := {'@' AnyIdent '(' String ')'}
 Annotation: Node<(Node<AnyId>, Option<Node<SmolStr>>)> = {
     <l:@L> "@" <key:AnyIdent> <value: ("(" <STR> ")")?> <r:@R> => Node::with_source_loc((key, value), Loc::new(l..r, Arc::clone(src)))
 }
@@ -225,8 +218,15 @@ Comma<E>: Vec<E> = {
     },
 }
 
-// IDENT := ['_''a'-'z''A'-'Z']['_''a'-'z''A'-'Z''0'-'9']*
+// IDENT := ['_''a'-'z''A'-'Z']['_''a'-'z''A'-'Z''0'-'9']* - RESERVED
 Ident: Node<Id> = {
+    <id: AnyIdent> =>? Id::from_str(id.node.as_ref()).map(|i| Node::with_source_loc(i, id.loc.clone())).map_err(|err : cedar_policy_core::parser::err::ParseErrors| ParseError::User {
+        error: UserError::ReservedIdentifierUsed(Node::with_source_loc(id.node.to_smolstr(), id.loc.clone()))
+    }),
+}
+
+// AnyIdent := ['_''a'-'z''A'-'Z']['_''a'-'z''A'-'Z''0'-'9']*
+AnyIdent: Node<AnyId> = {
     <l:@L> NAMESPACE <r:@R>
         => Node::with_source_loc("namespace".parse().unwrap(), Loc::new(l..r, Arc::clone(src))),
     <l:@L> ENTITY <r:@R>
@@ -256,17 +256,9 @@ Ident: Node<Id> = {
     <l:@L> TYPE <r:@R>
         => Node::with_source_loc("type".parse().unwrap(), Loc::new(l..r, Arc::clone(src))),
     <l:@L> IN <r:@R>
-        =>? Err(ParseError::User {
-            error: UserError::ReservedIdentifierUsed(Node::with_source_loc("in".into(), Loc::new(l..r, Arc::clone(src))))
-        }),
+        => Node::with_source_loc("in".parse().unwrap(), Loc::new(l..r, Arc::clone(src))),
     <l:@L> <i:IDENTIFIER> <r:@R>
-        =>? Id::from_str(i)
-        .map(|id : Id| Node::with_source_loc(id, Loc::new(l..r, Arc::clone(src))))
-        .map_err(|err : cedar_policy_core::parser::err::ParseErrors|
-            ParseError::User {
-                error: UserError::ReservedIdentifierUsed(Node::with_source_loc(i.to_smolstr(), Loc::new(l..r, Arc::clone(src))))
-            }
-        )
+        => Node::with_source_loc(i.parse().unwrap(), Loc::new(l..r, Arc::clone(src))),
 }
 
 STR: Node<SmolStr> = {
diff --git a/cedar-policy-validator/src/entity_manifest/type_annotations.rs b/cedar-policy-validator/src/entity_manifest/type_annotations.rs
index 0c70e771..0bf4b071 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 36e87e63..a6d4e9c6 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<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
diff --git a/cedar-policy-validator/src/types.rs b/cedar-policy-validator/src/types.rs
index da4a162f..d8e22c4a 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<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),
         );
 
diff --git a/cedar-policy-validator/src/types/request_env.rs b/cedar-policy-validator/src/types/request_env.rs
index 22f18a53..dffe00f4 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<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()),
         }
     }


Test patch:
diff --git a/cedar-policy-validator/src/cedar_schema/test.rs b/cedar-policy-validator/src/cedar_schema/test.rs
index 4cf7628c..19a14e4c 100644
--- a/cedar-policy-validator/src/cedar_schema/test.rs
+++ b/cedar-policy-validator/src/cedar_schema/test.rs
@@ -1178,6 +1178,8 @@ mod translator_tests {
         ValidatorSchema,
     };
 
+    use super::SPECIAL_IDS;
+
     // We allow translating schemas that violate RFC 52 to `json_schema::Fragment`.
     // The violations are reported during further translation to `ValidatorSchema`
     #[test]
@@ -1999,6 +2001,28 @@ mod translator_tests {
         .expect("should translate to JSON schema");
         assert_eq!(serde_json::to_value(schema).unwrap(), json_value);
     }
+
+    #[test]
+    fn any_id() {
+        for id in SPECIAL_IDS {
+            test_translation(
+                &format!("@{id} entity User {{}};"),
+                serde_json::json!({
+                    "": {
+                        "entityTypes": {
+                            "User": {
+                                "annotations": {
+                                    id: "",
+                                }
+                            }
+                        },
+                        "actions": {},
+                    }
+                }),
+            )
+        }
+    }
+
     #[test]
     fn annotations() {
         // namespace annotations
@@ -2634,6 +2658,28 @@ mod entity_tags {
     }
 }
 
+#[cfg(test)]
+pub(crate) const SPECIAL_IDS: [&str; 18] = [
+    "principal",
+    "action",
+    "resource",
+    "context",
+    "true",
+    "false",
+    "permit",
+    "forbid",
+    "when",
+    "unless",
+    "in",
+    "has",
+    "like",
+    "is",
+    "if",
+    "then",
+    "else",
+    "__cedar",
+];
+
 // RFC 48 test cases
 #[cfg(test)]
 mod annotations {
@@ -2641,6 +2687,17 @@ mod annotations {
 
     use crate::cedar_schema::parser::parse_schema;
 
+    use super::SPECIAL_IDS;
+
+    // test if annotation keys can be any id
+    #[test]
+    fn any_id() {
+        for id in SPECIAL_IDS {
+            let schema_str = format!("@{id} entity User {{}};");
+            assert_matches!(parse_schema(&schema_str), Ok(_));
+        }
+    }
+
     #[test]
     fn no_keys() {
         assert_matches!(