Repository: martinohmann/hcl-rs
Pull request #105 (2022-10-25 17:08:47)

fix!: sanitize identifiers upon creation
BREAKING CHANGE: `Identifier::new` is fallible now and the return value changed from `Identifier` to `Result<Identifier, Error>`. An infallible alternative is provided with `Identifier::sanitized`. The inner `String` field of `Identifier` is now private to prevent direct modification.

For most use cases this change shouldn't cause any issues big issues and it should be fairly easy to migrate code that may break.

`Identifier::new` is fallible in order to detect invalid identifiers.

Values created via any of `Identifier`'s `From` implementations are now automatically sanitized via `Identifier::sanitized`.

Furthermore `Identifier::unchecked` allows the creation of identifier without any validation. This can skip the sanitization if the caller already knows that the identifier is valid.


Patch:
diff --git a/src/error.rs b/src/error.rs
index d9e130c..ca72e15 100644
--- a/src/error.rs
+++ b/src/error.rs
@@ -37,6 +37,9 @@ pub enum Error {
     /// Represents errors due to invalid unicode code points that may occur when unescaping
     /// user-provided strings.
     InvalidUnicodeCodePoint(String),
+
+    /// Represents errors that resulted from identifiers that are not valid in HCL.
+    InvalidIdentifier(String),
 }
 
 impl Error {
@@ -75,6 +78,7 @@ impl Display for Error {
             Error::InvalidUnicodeCodePoint(u) => {
                 write!(f, "invalid unicode code point '\\u{}'", u)
             }
+            Error::InvalidIdentifier(ident) => write!(f, "invalid identifier `{}`", ident),
         }
     }
 }
diff --git a/src/macros.rs b/src/macros.rs
index dff4733..6383e73 100644
--- a/src/macros.rs
+++ b/src/macros.rs
@@ -367,7 +367,7 @@ macro_rules! block_internal {
 #[macro_export]
 macro_rules! block_label {
     ($ident:ident) => {
-        $crate::BlockLabel::Identifier(std::stringify!($ident).into())
+        $crate::BlockLabel::Identifier($crate::Identifier::unchecked(std::stringify!($ident)))
     };
 
     (($expr:expr)) => {
@@ -398,7 +398,7 @@ macro_rules! block_label {
 #[macro_export]
 macro_rules! object_key {
     ($ident:ident) => {
-        $crate::ObjectKey::Identifier(std::stringify!($ident).into())
+        $crate::ObjectKey::Identifier($crate::Identifier::unchecked(std::stringify!($ident)))
     };
 
     (#{$expr:expr}) => {
diff --git a/src/parser/mod.rs b/src/parser/mod.rs
index 2dbb09a..cd7fd05 100644
--- a/src/parser/mod.rs
+++ b/src/parser/mod.rs
@@ -283,7 +283,7 @@ fn parse_func_call(pair: Pair<Rule>) -> Result<FuncCall> {
     let mut pairs = pair.into_inner();
     let name = pairs.next().unwrap();
     let mut args = pairs.next().unwrap().into_inner();
-    let builder = FuncCall::builder(name.as_str());
+    let builder = FuncCall::builder(parse_ident(name));
 
     args.try_fold(builder, |builder, pair| match pair.as_rule() {
         Rule::ExpandFinal => Ok(builder.expand_final(true)),
@@ -354,7 +354,7 @@ fn parse_string(pair: Pair<Rule>) -> Result<String> {
 }
 
 fn parse_ident(pair: Pair<Rule>) -> Identifier {
-    Identifier::new(pair.as_str())
+    Identifier::unchecked(pair.as_str())
 }
 
 fn parse_heredoc(pair: Pair<Rule>) -> Heredoc {
diff --git a/src/structure/mod.rs b/src/structure/mod.rs
index ac6ed26..0852b70 100644
--- a/src/structure/mod.rs
+++ b/src/structure/mod.rs
@@ -73,7 +73,7 @@ pub use self::{
     template_expr::{Heredoc, HeredocStripMode, TemplateExpr},
     traversal::{Traversal, TraversalOperator},
 };
-use crate::{Map, Value};
+use crate::{Error, Map, Result, Value};
 use serde::{Deserialize, Serialize};
 use std::borrow::{Borrow, Cow};
 use std::fmt;
@@ -82,43 +82,153 @@ use std::ops;
 /// Represents an HCL identifier.
 #[derive(Deserialize, Serialize, Debug, Clone, PartialEq, Eq, Hash)]
 #[serde(rename = "$hcl::identifier")]
-pub struct Identifier(pub String);
+pub struct Identifier(String);
 
 impl Identifier {
-    /// Creates a new `Identifier` from something that can be converted to a `String`.
-    pub fn new<I>(ident: I) -> Self
+    /// Create a new `Identifier` after validating that it only contains characters that are
+    /// allowed in HCL identifiers.
+    ///
+    /// See [`Identifier::sanitized`][Identifier::sanitized] for an infallible alternative to this
+    /// function.
+    ///
+    /// # Example
+    ///
+    /// ```
+    /// # use hcl::Identifier;
+    /// assert!(Identifier::new("some_ident").is_ok());
+    /// assert!(Identifier::new("").is_err());
+    /// assert!(Identifier::new("1two3").is_err());
+    /// assert!(Identifier::new("with whitespace").is_err());
+    /// ```
+    ///
+    /// # Errors
+    ///
+    /// If `ident` contains characters that are not allowed in HCL identifiers or if it is empty an
+    /// error will be returned.
+    pub fn new<T>(ident: T) -> Result<Self>
     where
-        I: Into<String>,
+        T: Into<String>,
+    {
+        let ident = ident.into();
+
+        if ident.is_empty() {
+            return Err(Error::InvalidIdentifier(ident));
+        }
+
+        let mut chars = ident.chars();
+        let first = chars.next().unwrap();
+
+        if !is_id_start(first) || !chars.all(is_id_continue) {
+            return Err(Error::InvalidIdentifier(ident));
+        }
+
+        Ok(Identifier(ident))
+    }
+
+    /// Create a new `Identifier` after sanitizing the input if necessary.
+    ///
+    /// If `ident` contains characters that are not allowed in HCL identifiers will be sanitized
+    /// according to the following rules:
+    ///
+    /// - An empty `ident` results in an identifier containing a single underscore.
+    /// - Invalid characters in `ident` will be replaced with underscores.
+    /// - If `ident` starts with a character that is invalid in the first position but would be
+    ///   valid in the rest of an HCL identifier it is prefixed with an underscore.
+    ///
+    /// See [`Identifier::new`][Identifier::new] for a fallible alternative to this function if
+    /// you prefer rejecting invalid identifiers instead of sanitizing them.
+    ///
+    /// # Example
+    ///
+    /// ```
+    /// # use hcl::Identifier;
+    /// assert_eq!(Identifier::sanitized("some_ident").as_str(), "some_ident");
+    /// assert_eq!(Identifier::sanitized("").as_str(), "_");
+    /// assert_eq!(Identifier::sanitized("1two3").as_str(), "_1two3");
+    /// assert_eq!(Identifier::sanitized("with whitespace").as_str(), "with_whitespace");
+    /// ```
+    pub fn sanitized<T>(ident: T) -> Self
+    where
+        T: AsRef<str>,
+    {
+        let input = ident.as_ref();
+
+        if input.is_empty() {
+            return Identifier(String::from('_'));
+        }
+
+        let mut ident = String::with_capacity(input.len());
+
+        for (i, ch) in input.chars().enumerate() {
+            if i == 0 && is_id_start(ch) {
+                ident.push(ch);
+            } else if is_id_continue(ch) {
+                if i == 0 {
+                    ident.push('_');
+                }
+                ident.push(ch);
+            } else {
+                ident.push('_');
+            }
+        }
+
+        Identifier(ident)
+    }
+
+    /// Create a new `Identifier` without checking if it is valid.
+    ///
+    /// It is the caller's responsibility to ensure that the identifier is valid.
+    ///
+    /// For most use cases [`Identifier::new`][Identifier::new] or
+    /// [`Identifier::sanitized`][Identifier::sanitized] should be preferred.
+    ///
+    /// # Safety
+    ///
+    /// This function is not marked as unsafe because it does not cause undefined behaviour.
+    /// However, attempting to serialize an invalid identifier to HCL will produce invalid output.
+    pub fn unchecked<T>(ident: T) -> Self
+    where
+        T: Into<String>,
     {
         Identifier(ident.into())
     }
 
-    /// Consumes `self` and returns the `Identifier` as a `String`.
+    /// Consume `self` and return the wrapped `String`.
     pub fn into_inner(self) -> String {
         self.0
     }
 
-    /// Returns the `Identifier` as a `&str`.
+    /// Return a reference to the wrapped `str`.
     pub fn as_str(&self) -> &str {
         &self.0
     }
 }
 
+#[inline]
+fn is_id_start(ch: char) -> bool {
+    ch == '_' || unicode_ident::is_xid_start(ch)
+}
+
+#[inline]
+fn is_id_continue(ch: char) -> bool {
+    ch == '-' || unicode_ident::is_xid_continue(ch)
+}
+
 impl From<String> for Identifier {
     fn from(s: String) -> Self {
-        Identifier::new(s)
+        Identifier::sanitized(s)
     }
 }
 
 impl From<&str> for Identifier {
     fn from(s: &str) -> Self {
-        Identifier::new(s)
+        Identifier::sanitized(s)
     }
 }
 
 impl<'a> From<Cow<'a, str>> for Identifier {
     fn from(s: Cow<'a, str>) -> Self {
-        Identifier::new(s)
+        Identifier::sanitized(s)
     }
 }
 
diff --git a/src/structure/ser/block.rs b/src/structure/ser/block.rs
index e92c1ab..1597a9f 100644
--- a/src/structure/ser/block.rs
+++ b/src/structure/ser/block.rs
@@ -1,8 +1,8 @@
 use super::{
     body::BodySerializer, expression::ExpressionSerializer, structure::StructureSerializer,
-    SeqSerializer, StringSerializer,
+    IdentifierSerializer, SeqSerializer, StringSerializer,
 };
-use crate::{Attribute, Block, BlockLabel, Body, Error, Result, Structure};
+use crate::{Attribute, Block, BlockLabel, Body, Error, Identifier, Result, Structure};
 use serde::ser::{self, Impossible, Serialize};
 use std::fmt::Display;
 
@@ -352,7 +352,7 @@ impl ser::Serializer for BlockLabelSerializer {
         _variant_index: u32,
         variant: &'static str,
     ) -> Result<Self::Ok> {
-        Ok(BlockLabel::identifier(variant))
+        Identifier::new(variant).map(BlockLabel::Identifier)
     }
 
     fn serialize_newtype_variant<T>(
@@ -367,9 +367,9 @@ impl ser::Serializer for BlockLabelSerializer {
     {
         // Specialization for the `BlockLabel` type itself.
         match (name, variant) {
-            ("$hcl::block_label", "Identifier") => {
-                Ok(BlockLabel::identifier(value.serialize(StringSerializer)?))
-            }
+            ("$hcl::block_label", "Identifier") => Ok(BlockLabel::Identifier(
+                value.serialize(IdentifierSerializer)?,
+            )),
             (_, _) => value.serialize(self),
         }
     }
diff --git a/src/structure/ser/expression.rs b/src/structure/ser/expression.rs
index 2045313..60898c1 100644
--- a/src/structure/ser/expression.rs
+++ b/src/structure/ser/expression.rs
@@ -7,7 +7,7 @@ use super::{
     traversal::SerializeTraversalStruct,
     IdentifierSerializer, StringSerializer,
 };
-use crate::{Error, Expression, Number, Object, ObjectKey, RawExpression, Result};
+use crate::{Error, Expression, Identifier, Number, Object, ObjectKey, RawExpression, Result};
 use serde::ser::{self, Impossible, SerializeMap};
 use std::fmt::Display;
 
@@ -155,7 +155,7 @@ impl ser::Serializer for ExpressionSerializer {
             (_, _) => {
                 let mut object = Object::with_capacity(1);
                 object.insert(
-                    ObjectKey::Identifier(variant.into()),
+                    ObjectKey::Identifier(Identifier::new(variant)?),
                     value.serialize(self)?,
                 );
                 Ok(Expression::Object(object))
@@ -486,7 +486,7 @@ impl ser::Serializer for ObjectKeySerializer {
         _variant_index: u32,
         variant: &'static str,
     ) -> Result<Self::Ok> {
-        Ok(ObjectKey::Identifier(variant.into()))
+        Identifier::new(variant).map(ObjectKey::Identifier)
     }
 
     fn serialize_newtype_variant<T>(
diff --git a/src/structure/ser/mod.rs b/src/structure/ser/mod.rs
index 8150f2c..6fb3dbc 100644
--- a/src/structure/ser/mod.rs
+++ b/src/structure/ser/mod.rs
@@ -87,11 +87,11 @@ impl ser::Serializer for IdentifierSerializer {
     serialize_self! { some newtype_struct }
 
     fn serialize_char(self, value: char) -> Result<Self::Ok> {
-        Ok(Identifier::from(value.to_string()))
+        self.serialize_str(&value.to_string())
     }
 
     fn serialize_str(self, value: &str) -> Result<Self::Ok> {
-        Ok(value.into())
+        Identifier::new(value)
     }
 
     fn serialize_unit_variant(
@@ -100,14 +100,7 @@ impl ser::Serializer for IdentifierSerializer {
         _variant_index: u32,
         variant: &'static str,
     ) -> Result<Self::Ok> {
-        Ok(variant.into())
-    }
-
-    fn collect_str<T>(self, value: &T) -> Result<Self::Ok>
-    where
-        T: ?Sized + Display,
-    {
-        Ok(Identifier::from(value.to_string()))
+        self.serialize_str(variant)
     }
 }
 
diff --git a/src/structure/ser/template_expr.rs b/src/structure/ser/template_expr.rs
index 2fd5700..9ab09cb 100644
--- a/src/structure/ser/template_expr.rs
+++ b/src/structure/ser/template_expr.rs
@@ -124,7 +124,7 @@ impl ser::Serializer for HeredocSerializer {
         T: ?Sized + Serialize,
     {
         Ok(Heredoc {
-            delimiter: variant.into(),
+            delimiter: Identifier::new(variant)?,
             template: value.serialize(StringSerializer)?,
             strip: HeredocStripMode::None,
         })
diff --git a/src/structure/ser/traversal.rs b/src/structure/ser/traversal.rs
index 1236040..e962864 100644
--- a/src/structure/ser/traversal.rs
+++ b/src/structure/ser/traversal.rs
@@ -1,5 +1,5 @@
 use super::{expression::ExpressionSerializer, IdentifierSerializer, SeqSerializer};
-use crate::{Error, Expression, Result, Traversal, TraversalOperator};
+use crate::{Error, Expression, Identifier, Result, Traversal, TraversalOperator};
 use serde::ser::{self, Impossible, Serialize};
 
 pub struct TraversalSerializer;
@@ -112,7 +112,7 @@ impl ser::Serializer for TraversalOperatorSerializer {
         match (name, variant) {
             ("$hcl::traversal_operator", "AttrSplat") => Ok(TraversalOperator::AttrSplat),
             ("$hcl::traversal_operator", "FullSplat") => Ok(TraversalOperator::FullSplat),
-            (_, _) => Ok(TraversalOperator::GetAttr(variant.into())),
+            (_, _) => Identifier::new(variant).map(TraversalOperator::GetAttr),
         }
     }
 
diff --git a/src/template.rs b/src/template.rs
index 3bd05bc..0999efc 100644
--- a/src/template.rs
+++ b/src/template.rs
@@ -28,7 +28,7 @@
 //!
 //! let expected = Template::new()
 //!     .add_literal("Hello ")
-//!     .add_interpolation(Expression::Variable(Identifier::new("name")))
+//!     .add_interpolation(Expression::Variable(Identifier::new("name")?))
 //!     .add_literal("!");
 //!
 //! assert_eq!(expected, template);
@@ -60,12 +60,12 @@
 //!     .add_literal("\nBill of materials:\n")
 //!     .add_directive(
 //!         ForDirective::new(
-//!             Identifier::new("item"),
-//!             Expression::Variable(Identifier::new("items")),
+//!             Identifier::new("item")?,
+//!             Expression::Variable(Identifier::new("items")?),
 //!             Template::new()
 //!                 .add_literal("- ")
 //!                 .add_interpolation(
-//!                     Expression::Variable(Identifier::new("item"))
+//!                     Expression::Variable(Identifier::new("item")?)
 //!                 )
 //!                 .add_literal("\n")
 //!         )


Test patch:
diff --git a/src/de/tests.rs b/src/de/tests.rs
index 79ee868..4595ab2 100644
--- a/src/de/tests.rs
+++ b/src/de/tests.rs
@@ -203,7 +203,7 @@ fn deserialize_operation() {
             "unary",
             Operation::Unary(UnaryOp::new(
                 UnaryOperator::Not,
-                Identifier::new("variable"),
+                Identifier::unchecked("variable"),
             )),
         ))
         .add_attribute((
@@ -226,27 +226,27 @@ fn deserialize_for_expr() {
         .add_attribute((
             "list",
             ForExpr::new(
-                Identifier::new("item"),
-                Expression::Variable(Identifier::new("items")),
+                Identifier::unchecked("item"),
+                Expression::Variable(Identifier::unchecked("items")),
                 FuncCall::builder("func")
-                    .arg(Identifier::new("item"))
+                    .arg(Identifier::unchecked("item"))
                     .build(),
             )
-            .with_cond_expr(Identifier::new("item")),
+            .with_cond_expr(Identifier::unchecked("item")),
         ))
         .add_attribute((
             "object",
             ForExpr::new(
-                Identifier::new("value"),
-                Expression::Variable(Identifier::new("items")),
+                Identifier::unchecked("value"),
+                Expression::Variable(Identifier::unchecked("items")),
                 FuncCall::builder("tolower")
-                    .arg(Identifier::new("value"))
+                    .arg(Identifier::unchecked("value"))
                     .build(),
             )
-            .with_key_var(Identifier::new("key"))
+            .with_key_var(Identifier::unchecked("key"))
             .with_key_expr(
                 FuncCall::builder("toupper")
-                    .arg(Identifier::new("key"))
+                    .arg(Identifier::unchecked("key"))
                     .build(),
             )
             .with_grouping(true),
@@ -300,7 +300,7 @@ fn deserialize_terraform() {
                                         .add_attribute((
                                             "kms_master_key_id",
                                             Traversal::new(
-                                                Identifier::new("aws_kms_key"),
+                                                Identifier::unchecked("aws_kms_key"),
                                                 ["mykey", "arn"],
                                             ),
                                         ))
@@ -315,7 +315,10 @@ fn deserialize_terraform() {
                     "tags",
                     Expression::from_iter([
                         (
-                            ObjectKey::from(Traversal::new(Identifier::new("var"), ["dynamic"])),
+                            ObjectKey::from(Traversal::new(
+                                Identifier::unchecked("var"),
+                                ["dynamic"],
+                            )),
                             Expression::Null,
                         ),
                         (
@@ -393,7 +396,7 @@ fn issue_66() {
         .add_attribute((
             "a",
             Traversal::new(
-                Identifier::new("b"),
+                Identifier::unchecked("b"),
                 [Expression::String(String::from("c"))],
             ),
         ))
@@ -413,7 +416,7 @@ fn issue_81() {
         .add_attribute((
             "attr_splat",
             Traversal::new(
-                Identifier::new("module"),
+                Identifier::unchecked("module"),
                 [
                     TraversalOperator::GetAttr("instance".into()),
                     TraversalOperator::AttrSplat,
@@ -424,7 +427,7 @@ fn issue_81() {
         .add_attribute((
             "full_splat",
             Traversal::new(
-                Identifier::new("module"),
+                Identifier::unchecked("module"),
                 [
                     TraversalOperator::GetAttr("instance".into()),
                     TraversalOperator::FullSplat,
@@ -443,7 +446,7 @@ fn issue_83() {
         .add_attribute((
             "attr",
             Traversal::new(
-                Identifier::new("module"),
+                Identifier::unchecked("module"),
                 [
                     TraversalOperator::GetAttr("instance".into()),
                     TraversalOperator::LegacyIndex(0),
diff --git a/src/parser/tests.rs b/src/parser/tests.rs
index 9e69f0d..5037c95 100644
--- a/src/parser/tests.rs
+++ b/src/parser/tests.rs
@@ -293,12 +293,12 @@ fn parse_object_with_variable_expr_key() {
             "providers",
             Expression::from_iter([(
                 ObjectKey::Expression(Expression::from(Traversal::new(
-                    Identifier::new("aws"),
-                    [Identifier::new("eu-central-1")],
+                    Identifier::unchecked("aws"),
+                    [Identifier::unchecked("eu-central-1")],
                 ))),
                 Expression::from(Traversal::new(
-                    Identifier::new("aws"),
-                    [Identifier::new("eu-central-1")],
+                    Identifier::unchecked("aws"),
+                    [Identifier::unchecked("eu-central-1")],
                 )),
             )]),
         ))
@@ -370,17 +370,17 @@ fn parse_traversal_with_expression() {
         .add_attribute((
             "route_table_id",
             Expression::from(Traversal::new(
-                Identifier::new("aws_route_table"),
+                Identifier::unchecked("aws_route_table"),
                 [
                     TraversalOperator::GetAttr("private".into()),
                     TraversalOperator::Index(Expression::from(Operation::Binary(BinaryOp::new(
                         Traversal::new(
-                            Identifier::new("count"),
+                            Identifier::unchecked("count"),
                             [TraversalOperator::GetAttr("index".into())],
                         ),
                         BinaryOperator::Mod,
                         Traversal::new(
-                            Identifier::new("var"),
+                            Identifier::unchecked("var"),
                             [TraversalOperator::GetAttr("availability_zone_count".into())],
                         ),
                     )))),
@@ -398,7 +398,7 @@ fn parse_null_in_variable_expr() {
     let input = "foo = null_foo";
 
     let expected = Body::builder()
-        .add_attribute(("foo", Identifier::new("null_foo")))
+        .add_attribute(("foo", Identifier::unchecked("null_foo")))
         .build();
 
     expect_body(input, expected);
@@ -451,7 +451,7 @@ fn parse_hcl() {
                                         .add_attribute(Attribute::new(
                                             "kms_master_key_id",
                                             Traversal::new(
-                                                Identifier::new("aws_kms_key"),
+                                                Identifier::unchecked("aws_kms_key"),
                                                 ["mykey", "arn"],
                                             ),
                                         ))
@@ -499,7 +499,7 @@ fn unescape_strings() {
                     "heredoc",
                     TemplateExpr::Heredoc(
                         Heredoc::new(
-                            Identifier::new("EOS"),
+                            Identifier::unchecked("EOS"),
                             "            heredoc template with \\\n            escaped newline and \\\\backslash is not unescaped yet\n"
                         )
                         .with_strip_mode(HeredocStripMode::Indent)
@@ -550,11 +550,11 @@ fn parse_template_expr() {
 
             let expected_template = Template::new()
                 .add_literal("bar ")
-                .add_interpolation(Expression::Variable(Identifier::new("baz")))
+                .add_interpolation(Expression::Variable(Identifier::unchecked("baz")))
                 .add_literal(" ")
                 .add_directive(
                     IfDirective::new(
-                        Expression::Variable(Identifier::new("cond")),
+                        Expression::Variable(Identifier::unchecked("cond")),
                         Template::new().add_literal("qux"),
                     )
                     .with_if_strip(StripMode::Start)
diff --git a/src/ser/tests.rs b/src/ser/tests.rs
index 38ba017..eddff5a 100644
--- a/src/ser/tests.rs
+++ b/src/ser/tests.rs
@@ -196,7 +196,7 @@ fn serialize_traversal() {
         Attribute::new(
             "attr",
             Traversal::new(
-                Identifier::new("var"),
+                Identifier::unchecked("var"),
                 [
                     TraversalOperator::GetAttr("foo".into()),
                     TraversalOperator::FullSplat,
@@ -217,7 +217,7 @@ fn serialize_conditional() {
     expect_str(
         Attribute::new(
             "cond",
-            Conditional::new(Identifier::new("cond_var"), "yes", "no"),
+            Conditional::new(Identifier::unchecked("cond_var"), "yes", "no"),
         ),
         "cond = cond_var ? \"yes\" : \"no\"\n",
     );
@@ -240,31 +240,31 @@ fn serialize_for_expr() {
         .add_attribute((
             "list",
             ForExpr::new(
-                Identifier::new("item"),
-                Expression::Variable(Identifier::new("items")),
+                Identifier::unchecked("item"),
+                Expression::Variable(Identifier::unchecked("items")),
                 FuncCall::builder("func")
-                    .arg(Identifier::new("item"))
+                    .arg(Identifier::unchecked("item"))
                     .build(),
             )
-            .with_cond_expr(Identifier::new("item")),
+            .with_cond_expr(Identifier::unchecked("item")),
         ))
         .add_attribute((
             "object",
             ForExpr::new(
-                Identifier::new("value"),
-                Expression::Variable(Identifier::new("items")),
+                Identifier::unchecked("value"),
+                Expression::Variable(Identifier::unchecked("items")),
                 FuncCall::builder("tolower")
-                    .arg(Identifier::new("value"))
+                    .arg(Identifier::unchecked("value"))
                     .build(),
             )
-            .with_key_var(Identifier::new("key"))
+            .with_key_var(Identifier::unchecked("key"))
             .with_key_expr(
                 FuncCall::builder("toupper")
-                    .arg(Identifier::new("key"))
+                    .arg(Identifier::unchecked("key"))
                     .build(),
             )
             .with_cond_expr(Operation::Binary(BinaryOp::new(
-                Identifier::new("value"),
+                Identifier::unchecked("value"),
                 BinaryOperator::NotEq,
                 Expression::Null,
             )))
@@ -312,15 +312,18 @@ fn serialize_heredoc() {
                 .add_attribute((
                     "heredoc",
                     TemplateExpr::Heredoc(Heredoc::new(
-                        Identifier::new("HEREDOC"),
+                        Identifier::unchecked("HEREDOC"),
                         "foo\n  bar\nbaz\n",
                     )),
                 ))
                 .add_attribute((
                     "heredoc_indent",
                     TemplateExpr::Heredoc(
-                        Heredoc::new(Identifier::new("HEREDOC"), "    foo\n      bar\n    baz\n")
-                            .with_strip_mode(HeredocStripMode::Indent),
+                        Heredoc::new(
+                            Identifier::unchecked("HEREDOC"),
+                            "    foo\n      bar\n    baz\n",
+                        )
+                        .with_strip_mode(HeredocStripMode::Indent),
                     ),
                 ))
                 .build(),
@@ -431,7 +434,11 @@ fn roundtrip() {
                 .add_label("mybucket")
                 .add_attribute((
                     "count",
-                    Conditional::new(Traversal::new(Identifier::new("var"), ["enabled"]), 1, 0),
+                    Conditional::new(
+                        Traversal::new(Identifier::unchecked("var"), ["enabled"]),
+                        1,
+                        0,
+                    ),
                 ))
                 .add_attribute(("bucket", "mybucket"))
                 .add_attribute(("force_destroy", true))
@@ -444,7 +451,7 @@ fn roundtrip() {
                                         .add_attribute((
                                             "kms_master_key_id",
                                             Traversal::new(
-                                                Identifier::new("aws_kms_key"),
+                                                Identifier::unchecked("aws_kms_key"),
                                                 ["mykey", "arn"],
                                             ),
                                         ))
diff --git a/src/structure/ser/tests.rs b/src/structure/ser/tests.rs
index 244eed0..a18fe17 100644
--- a/src/structure/ser/tests.rs
+++ b/src/structure/ser/tests.rs
@@ -69,17 +69,17 @@ fn identity() {
     test_identity(
         TemplateExprSerializer,
         TemplateExpr::Heredoc(
-            Heredoc::new(Identifier::new("EOS"), "  ${foo}")
+            Heredoc::new(Identifier::unchecked("EOS"), "  ${foo}")
                 .with_strip_mode(HeredocStripMode::Indent),
         ),
     );
     test_identity(
         TemplateExprSerializer,
-        TemplateExpr::Heredoc(Heredoc::new(Identifier::new("EOS"), "${foo}")),
+        TemplateExpr::Heredoc(Heredoc::new(Identifier::unchecked("EOS"), "${foo}")),
     );
     test_identity(
         ConditionalSerializer,
-        Conditional::new(Identifier::new("some_cond_var"), "yes", "no"),
+        Conditional::new(Identifier::unchecked("some_cond_var"), "yes", "no"),
     );
     test_identity(
         OperationSerializer,
@@ -92,25 +92,25 @@ fn identity() {
     test_identity(
         ForExprSerializer,
         ForExpr::new(
-            Identifier::new("value"),
+            Identifier::unchecked("value"),
             vec![Expression::String(String::from("foo"))],
-            Identifier::new("other_value"),
+            Identifier::unchecked("other_value"),
         )
-        .with_key_var(Identifier::new("index"))
+        .with_key_var(Identifier::unchecked("index"))
         .with_cond_expr(Expression::Bool(true)),
     );
     test_identity(
         ForExprSerializer,
         ForExpr::new(
-            Identifier::new("value"),
+            Identifier::unchecked("value"),
             Expression::Object(Object::from([(
                 ObjectKey::from("k"),
                 Expression::String(String::from("v")),
             )])),
-            Identifier::new("other_value"),
+            Identifier::unchecked("other_value"),
         )
-        .with_key_var(Identifier::new("index"))
-        .with_key_expr(Identifier::new("other_key"))
+        .with_key_var(Identifier::unchecked("index"))
+        .with_key_expr(Identifier::unchecked("other_key"))
         .with_cond_expr(Expression::Bool(true))
         .with_grouping(true),
     );
@@ -208,9 +208,9 @@ fn custom() {
 
     test_serialize(
         ExpressionSerializer,
-        Conditional::new(Identifier::new("some_cond_var"), "yes", "no"),
+        Conditional::new(Identifier::unchecked("some_cond_var"), "yes", "no"),
         Expression::from(Conditional::new(
-            Identifier::new("some_cond_var"),
+            Identifier::unchecked("some_cond_var"),
             "yes",
             "no",
         )),
@@ -224,9 +224,9 @@ fn custom() {
 
     test_serialize(
         ExpressionSerializer,
-        TemplateExpr::Heredoc(Heredoc::new(Identifier::new("EOS"), "${foo}")),
+        TemplateExpr::Heredoc(Heredoc::new(Identifier::unchecked("EOS"), "${foo}")),
         Expression::from(TemplateExpr::Heredoc(Heredoc::new(
-            Identifier::new("EOS"),
+            Identifier::unchecked("EOS"),
             "${foo}",
         ))),
     );
@@ -234,19 +234,19 @@ fn custom() {
     test_serialize(
         ExpressionSerializer,
         ForExpr::new(
-            Identifier::new("value"),
+            Identifier::unchecked("value"),
             vec![Expression::String(String::from("foo"))],
-            Identifier::new("other_value"),
+            Identifier::unchecked("other_value"),
         )
-        .with_key_var(Identifier::new("index"))
+        .with_key_var(Identifier::unchecked("index"))
         .with_cond_expr(Expression::Bool(true)),
         Expression::from(
             ForExpr::new(
-                Identifier::new("value"),
+                Identifier::unchecked("value"),
                 vec![Expression::String(String::from("foo"))],
-                Identifier::new("other_value"),
+                Identifier::unchecked("other_value"),
             )
-            .with_key_var(Identifier::new("index"))
+            .with_key_var(Identifier::unchecked("index"))
             .with_cond_expr(Expression::Bool(true)),
         ),
     );
@@ -254,21 +254,21 @@ fn custom() {
     test_serialize(
         ExpressionSerializer,
         ForExpr::new(
-            Identifier::new("value"),
+            Identifier::unchecked("value"),
             vec![Expression::String(String::from("foo"))],
-            Identifier::new("other_value"),
+            Identifier::unchecked("other_value"),
         )
-        .with_key_var(Identifier::new("key"))
-        .with_key_expr(Expression::Variable(Identifier::new("key")))
+        .with_key_var(Identifier::unchecked("key"))
+        .with_key_expr(Expression::Variable(Identifier::unchecked("key")))
         .with_cond_expr(Expression::Bool(true)),
         Expression::from(
             ForExpr::new(
-                Identifier::new("value"),
+                Identifier::unchecked("value"),
                 vec![Expression::String(String::from("foo"))],
-                Identifier::new("other_value"),
+                Identifier::unchecked("other_value"),
             )
-            .with_key_var(Identifier::new("key"))
-            .with_key_expr(Expression::Variable(Identifier::new("key")))
+            .with_key_var(Identifier::unchecked("key"))
+            .with_key_expr(Expression::Variable(Identifier::unchecked("key")))
             .with_cond_expr(Expression::Bool(true)),
         ),
     );
@@ -276,11 +276,11 @@ fn custom() {
     test_serialize(
         ConditionalSerializer,
         (
-            Expression::Variable(Identifier::new("some_cond_var")),
+            Expression::Variable(Identifier::unchecked("some_cond_var")),
             Expression::String("yes".into()),
             Expression::String("no".into()),
         ),
-        Conditional::new(Identifier::new("some_cond_var"), "yes", "no"),
+        Conditional::new(Identifier::unchecked("some_cond_var"), "yes", "no"),
     );
 
     test_serialize(
diff --git a/src/structure/tests.rs b/src/structure/tests.rs
index 884c455..6a0a081 100644
--- a/src/structure/tests.rs
+++ b/src/structure/tests.rs
@@ -31,7 +31,7 @@ fn body_into_value() {
             "heredoc",
             TemplateExpr::Heredoc(
                 Heredoc::new(
-                    Identifier::new("EOS"),
+                    Identifier::unchecked("EOS"),
                     "  foo \\\n  bar ${baz}\\\\backslash",
                 )
                 .with_strip_mode(HeredocStripMode::Indent),