Repository: martinohmann/hcl-rs
Pull request #109 (2022-10-27 06:22:29)

fix!: change type of `Expression::Variable` variant
BREAKING CHANGE: The type of `Expression`'s `Variable` variant changed from `Identifier` to `Variable`. You can create a variable from an identifier via `Variable::from(identifier)` or by using `Variable::new` and `Variable::sanitized`.

This change is necessary to better distiguish between variables and bare identifiers and to be able to clean up some trait bounds.


Patch:
diff --git a/src/parser/mod.rs b/src/parser/mod.rs
index 42290d3..8ef6c9b 100644
--- a/src/parser/mod.rs
+++ b/src/parser/mod.rs
@@ -184,7 +184,7 @@ fn parse_expr_term(pair: Pair<Rule>) -> Result<Expression> {
         Rule::TemplateExpr => Expression::TemplateExpr(Box::new(parse_template_expr(inner(pair)))),
         Rule::Tuple => parse_expressions(pair).map(Expression::Array)?,
         Rule::Object => parse_object(pair).map(Expression::Object)?,
-        Rule::Variable => Expression::Variable(parse_ident(pair)),
+        Rule::Variable => Expression::Variable(Variable::from(parse_ident(pair))),
         Rule::FunctionCall => Expression::FuncCall(Box::new(parse_func_call(pair)?)),
         Rule::Parenthesis => Expression::Parenthesis(Box::new(parse_expression(inner(pair))?)),
         Rule::ForExpr => Expression::from(parse_for_expr(inner(pair))?),
diff --git a/src/structure/de.rs b/src/structure/de.rs
index ebc4345..0a2112a 100644
--- a/src/structure/de.rs
+++ b/src/structure/de.rs
@@ -1101,6 +1101,14 @@ impl<'de> IntoDeserializer<'de, Error> for Identifier {
     }
 }
 
+impl<'de> IntoDeserializer<'de, Error> for Variable {
+    type Deserializer = NewtypeStructDeserializer<String>;
+
+    fn into_deserializer(self) -> Self::Deserializer {
+        self.into_inner().into_deserializer()
+    }
+}
+
 pub struct NewtypeStructDeserializer<T, E = Error> {
     value: T,
     marker: PhantomData<E>,
diff --git a/src/structure/expression.rs b/src/structure/expression.rs
index 0bbf108..efe1110 100644
--- a/src/structure/expression.rs
+++ b/src/structure/expression.rs
@@ -31,7 +31,7 @@ pub enum Expression {
     /// A quoted string or heredoc that embeds a program written in the template sub-language.
     TemplateExpr(Box<TemplateExpr>),
     /// Represents a variable name identifier.
-    Variable(Identifier),
+    Variable(Variable),
     /// Represents an attribute or element traversal.
     Traversal(Box<Traversal>),
     /// Represents a function call.
@@ -219,9 +219,9 @@ impl From<TemplateExpr> for Expression {
     }
 }
 
-impl From<Identifier> for Expression {
-    fn from(ident: Identifier) -> Self {
-        Expression::Variable(ident)
+impl From<Variable> for Expression {
+    fn from(variable: Variable) -> Self {
+        Expression::Variable(variable)
     }
 }
 
diff --git a/src/structure/ser/expression.rs b/src/structure/ser/expression.rs
index 60898c1..9a8f31d 100644
--- a/src/structure/ser/expression.rs
+++ b/src/structure/ser/expression.rs
@@ -7,7 +7,9 @@ use super::{
     traversal::SerializeTraversalStruct,
     IdentifierSerializer, StringSerializer,
 };
-use crate::{Error, Expression, Identifier, Number, Object, ObjectKey, RawExpression, Result};
+use crate::{
+    Error, Expression, Identifier, Number, Object, ObjectKey, RawExpression, Result, Variable,
+};
 use serde::ser::{self, Impossible, SerializeMap};
 use std::fmt::Display;
 
@@ -119,7 +121,9 @@ impl ser::Serializer for ExpressionSerializer {
                 value.serialize(StringSerializer)?,
             )))
         } else if name == "$hcl::identifier" {
-            Ok(Expression::Variable(value.serialize(IdentifierSerializer)?))
+            Ok(Expression::Variable(Variable::from(
+                value.serialize(IdentifierSerializer)?,
+            )))
         } else {
             value.serialize(self)
         }
diff --git a/src/template.rs b/src/template.rs
index 0999efc..01cc61b 100644
--- a/src/template.rs
+++ b/src/template.rs
@@ -21,14 +21,14 @@
 //! #
 //! # fn main() -> Result<(), Box<dyn Error>> {
 //! use hcl::template::Template;
-//! use hcl::{Expression, Identifier, TemplateExpr};
+//! use hcl::{TemplateExpr, Variable};
 //!
-//! let expr = TemplateExpr::QuotedString(String::from("Hello ${name}!"));
+//! let expr = TemplateExpr::from("Hello ${name}!");
 //! let template = Template::from_expr(&expr)?;
 //!
 //! let expected = Template::new()
 //!     .add_literal("Hello ")
-//!     .add_interpolation(Expression::Variable(Identifier::new("name")?))
+//!     .add_interpolation(Variable::new("name")?)
 //!     .add_literal("!");
 //!
 //! assert_eq!(expected, template);
@@ -43,7 +43,7 @@
 //! # use std::error::Error;
 //! #
 //! # fn main() -> Result<(), Box<dyn Error>> {
-//! use hcl::{Expression, Identifier};
+//! use hcl::{Identifier, Variable};
 //! use hcl::template::{ForDirective, StripMode, Template};
 //! use std::str::FromStr;
 //!
@@ -61,12 +61,10 @@
 //!     .add_directive(
 //!         ForDirective::new(
 //!             Identifier::new("item")?,
-//!             Expression::Variable(Identifier::new("items")?),
+//!             Variable::new("items")?,
 //!             Template::new()
 //!                 .add_literal("- ")
-//!                 .add_interpolation(
-//!                     Expression::Variable(Identifier::new("item")?)
-//!                 )
+//!                 .add_interpolation(Variable::new("item")?)
 //!                 .add_literal("\n")
 //!         )
 //!         .with_for_strip(StripMode::End)
@@ -245,10 +243,13 @@ impl Interpolation {
     }
 }
 
-impl From<Expression> for Interpolation {
-    fn from(expr: Expression) -> Self {
+impl<T> From<T> for Interpolation
+where
+    T: Into<Expression>,
+{
+    fn from(expr: T) -> Self {
         Interpolation {
-            expr,
+            expr: expr.into(),
             strip: StripMode::default(),
         }
     }


Test patch:
diff --git a/src/de/tests.rs b/src/de/tests.rs
index 4595ab2..ab54dfe 100644
--- a/src/de/tests.rs
+++ b/src/de/tests.rs
@@ -1,7 +1,7 @@
 use super::*;
 use crate::{
     BinaryOp, BinaryOperator, Block, Body, Expression, ForExpr, FuncCall, Identifier, ObjectKey,
-    Operation, Traversal, TraversalOperator, UnaryOp, UnaryOperator,
+    Operation, Traversal, TraversalOperator, UnaryOp, UnaryOperator, Variable,
 };
 use pretty_assertions::assert_eq;
 use serde::Deserialize;
@@ -203,7 +203,7 @@ fn deserialize_operation() {
             "unary",
             Operation::Unary(UnaryOp::new(
                 UnaryOperator::Not,
-                Identifier::unchecked("variable"),
+                Variable::unchecked("variable"),
             )),
         ))
         .add_attribute((
@@ -227,26 +227,26 @@ fn deserialize_for_expr() {
             "list",
             ForExpr::new(
                 Identifier::unchecked("item"),
-                Expression::Variable(Identifier::unchecked("items")),
+                Variable::unchecked("items"),
                 FuncCall::builder("func")
-                    .arg(Identifier::unchecked("item"))
+                    .arg(Variable::unchecked("item"))
                     .build(),
             )
-            .with_cond_expr(Identifier::unchecked("item")),
+            .with_cond_expr(Variable::unchecked("item")),
         ))
         .add_attribute((
             "object",
             ForExpr::new(
                 Identifier::unchecked("value"),
-                Expression::Variable(Identifier::unchecked("items")),
+                Variable::unchecked("items"),
                 FuncCall::builder("tolower")
-                    .arg(Identifier::unchecked("value"))
+                    .arg(Variable::unchecked("value"))
                     .build(),
             )
             .with_key_var(Identifier::unchecked("key"))
             .with_key_expr(
                 FuncCall::builder("toupper")
-                    .arg(Identifier::unchecked("key"))
+                    .arg(Variable::unchecked("key"))
                     .build(),
             )
             .with_grouping(true),
@@ -300,7 +300,7 @@ fn deserialize_terraform() {
                                         .add_attribute((
                                             "kms_master_key_id",
                                             Traversal::new(
-                                                Identifier::unchecked("aws_kms_key"),
+                                                Variable::unchecked("aws_kms_key"),
                                                 ["mykey", "arn"],
                                             ),
                                         ))
@@ -316,7 +316,7 @@ fn deserialize_terraform() {
                     Expression::from_iter([
                         (
                             ObjectKey::from(Traversal::new(
-                                Identifier::unchecked("var"),
+                                Variable::unchecked("var"),
                                 ["dynamic"],
                             )),
                             Expression::Null,
@@ -395,10 +395,7 @@ fn issue_66() {
     let expected = Body::builder()
         .add_attribute((
             "a",
-            Traversal::new(
-                Identifier::unchecked("b"),
-                [Expression::String(String::from("c"))],
-            ),
+            Traversal::new(Variable::unchecked("b"), [Expression::from("c")]),
         ))
         .build();
 
@@ -416,7 +413,7 @@ fn issue_81() {
         .add_attribute((
             "attr_splat",
             Traversal::new(
-                Identifier::unchecked("module"),
+                Variable::unchecked("module"),
                 [
                     TraversalOperator::GetAttr("instance".into()),
                     TraversalOperator::AttrSplat,
@@ -427,7 +424,7 @@ fn issue_81() {
         .add_attribute((
             "full_splat",
             Traversal::new(
-                Identifier::unchecked("module"),
+                Variable::unchecked("module"),
                 [
                     TraversalOperator::GetAttr("instance".into()),
                     TraversalOperator::FullSplat,
@@ -446,7 +443,7 @@ fn issue_83() {
         .add_attribute((
             "attr",
             Traversal::new(
-                Identifier::unchecked("module"),
+                Variable::unchecked("module"),
                 [
                     TraversalOperator::GetAttr("instance".into()),
                     TraversalOperator::LegacyIndex(0),
diff --git a/src/parser/tests.rs b/src/parser/tests.rs
index 5037c95..698d7ad 100644
--- a/src/parser/tests.rs
+++ b/src/parser/tests.rs
@@ -293,11 +293,11 @@ fn parse_object_with_variable_expr_key() {
             "providers",
             Expression::from_iter([(
                 ObjectKey::Expression(Expression::from(Traversal::new(
-                    Identifier::unchecked("aws"),
+                    Variable::unchecked("aws"),
                     [Identifier::unchecked("eu-central-1")],
                 ))),
                 Expression::from(Traversal::new(
-                    Identifier::unchecked("aws"),
+                    Variable::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::unchecked("aws_route_table"),
+                Variable::unchecked("aws_route_table"),
                 [
                     TraversalOperator::GetAttr("private".into()),
                     TraversalOperator::Index(Expression::from(Operation::Binary(BinaryOp::new(
                         Traversal::new(
-                            Identifier::unchecked("count"),
+                            Variable::unchecked("count"),
                             [TraversalOperator::GetAttr("index".into())],
                         ),
                         BinaryOperator::Mod,
                         Traversal::new(
-                            Identifier::unchecked("var"),
+                            Variable::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::unchecked("null_foo")))
+        .add_attribute(("foo", Variable::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::unchecked("aws_kms_key"),
+                                                Variable::unchecked("aws_kms_key"),
                                                 ["mykey", "arn"],
                                             ),
                                         ))
@@ -550,11 +550,11 @@ fn parse_template_expr() {
 
             let expected_template = Template::new()
                 .add_literal("bar ")
-                .add_interpolation(Expression::Variable(Identifier::unchecked("baz")))
+                .add_interpolation(Variable::unchecked("baz"))
                 .add_literal(" ")
                 .add_directive(
                     IfDirective::new(
-                        Expression::Variable(Identifier::unchecked("cond")),
+                        Variable::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 eddff5a..9bdfeb6 100644
--- a/src/ser/tests.rs
+++ b/src/ser/tests.rs
@@ -2,7 +2,7 @@ use super::*;
 use crate::{
     Attribute, BinaryOp, BinaryOperator, Block, BlockLabel, Body, Conditional, Expression, ForExpr,
     FuncCall, Heredoc, HeredocStripMode, Identifier, Object, ObjectKey, Operation, RawExpression,
-    TemplateExpr, Traversal, TraversalOperator,
+    TemplateExpr, Traversal, TraversalOperator, Variable,
 };
 use pretty_assertions::assert_eq;
 use serde_json::json;
@@ -196,7 +196,7 @@ fn serialize_traversal() {
         Attribute::new(
             "attr",
             Traversal::new(
-                Identifier::unchecked("var"),
+                Variable::unchecked("var"),
                 [
                     TraversalOperator::GetAttr("foo".into()),
                     TraversalOperator::FullSplat,
@@ -217,7 +217,7 @@ fn serialize_conditional() {
     expect_str(
         Attribute::new(
             "cond",
-            Conditional::new(Identifier::unchecked("cond_var"), "yes", "no"),
+            Conditional::new(Variable::unchecked("cond_var"), "yes", "no"),
         ),
         "cond = cond_var ? \"yes\" : \"no\"\n",
     );
@@ -241,30 +241,30 @@ fn serialize_for_expr() {
             "list",
             ForExpr::new(
                 Identifier::unchecked("item"),
-                Expression::Variable(Identifier::unchecked("items")),
+                Variable::unchecked("items"),
                 FuncCall::builder("func")
-                    .arg(Identifier::unchecked("item"))
+                    .arg(Variable::unchecked("item"))
                     .build(),
             )
-            .with_cond_expr(Identifier::unchecked("item")),
+            .with_cond_expr(Variable::unchecked("item")),
         ))
         .add_attribute((
             "object",
             ForExpr::new(
                 Identifier::unchecked("value"),
-                Expression::Variable(Identifier::unchecked("items")),
+                Variable::unchecked("items"),
                 FuncCall::builder("tolower")
-                    .arg(Identifier::unchecked("value"))
+                    .arg(Variable::unchecked("value"))
                     .build(),
             )
             .with_key_var(Identifier::unchecked("key"))
             .with_key_expr(
                 FuncCall::builder("toupper")
-                    .arg(Identifier::unchecked("key"))
+                    .arg(Variable::unchecked("key"))
                     .build(),
             )
             .with_cond_expr(Operation::Binary(BinaryOp::new(
-                Identifier::unchecked("value"),
+                Variable::unchecked("value"),
                 BinaryOperator::NotEq,
                 Expression::Null,
             )))
@@ -411,7 +411,7 @@ fn serialize_nested_expression() {
     expect_str(
         Attribute::new(
             "attr",
-            Expression::Parenthesis(Box::new(Expression::Variable("foo".into()))),
+            Expression::Parenthesis(Box::new(Expression::from(Variable::unchecked("foo")))),
         ),
         "attr = (foo)\n",
     );
@@ -435,7 +435,7 @@ fn roundtrip() {
                 .add_attribute((
                     "count",
                     Conditional::new(
-                        Traversal::new(Identifier::unchecked("var"), ["enabled"]),
+                        Traversal::new(Variable::unchecked("var"), ["enabled"]),
                         1,
                         0,
                     ),
@@ -451,7 +451,7 @@ fn roundtrip() {
                                         .add_attribute((
                                             "kms_master_key_id",
                                             Traversal::new(
-                                                Identifier::unchecked("aws_kms_key"),
+                                                Variable::unchecked("aws_kms_key"),
                                                 ["mykey", "arn"],
                                             ),
                                         ))
@@ -471,14 +471,12 @@ fn roundtrip() {
                         ),
                         (
                             ObjectKey::Identifier("team".into()),
-                            Expression::TemplateExpr(Box::new(TemplateExpr::QuotedString(
-                                "${var.team}".into(),
-                            ))),
+                            Expression::TemplateExpr(Box::new(TemplateExpr::from("${var.team}"))),
                         ),
                         (
                             ObjectKey::Identifier("environment".into()),
-                            Expression::Parenthesis(Box::new(Expression::Variable(
-                                "environment".into(),
+                            Expression::Parenthesis(Box::new(Expression::from(
+                                Variable::unchecked("environment"),
                             ))),
                         ),
                     ]),
diff --git a/src/structure/ser/tests.rs b/src/structure/ser/tests.rs
index a18fe17..b154836 100644
--- a/src/structure/ser/tests.rs
+++ b/src/structure/ser/tests.rs
@@ -79,7 +79,7 @@ fn identity() {
     );
     test_identity(
         ConditionalSerializer,
-        Conditional::new(Identifier::unchecked("some_cond_var"), "yes", "no"),
+        Conditional::new(Variable::unchecked("some_cond_var"), "yes", "no"),
     );
     test_identity(
         OperationSerializer,
@@ -94,7 +94,7 @@ fn identity() {
         ForExpr::new(
             Identifier::unchecked("value"),
             vec![Expression::String(String::from("foo"))],
-            Identifier::unchecked("other_value"),
+            Variable::unchecked("other_value"),
         )
         .with_key_var(Identifier::unchecked("index"))
         .with_cond_expr(Expression::Bool(true)),
@@ -107,10 +107,10 @@ fn identity() {
                 ObjectKey::from("k"),
                 Expression::String(String::from("v")),
             )])),
-            Identifier::unchecked("other_value"),
+            Variable::unchecked("other_value"),
         )
         .with_key_var(Identifier::unchecked("index"))
-        .with_key_expr(Identifier::unchecked("other_key"))
+        .with_key_expr(Variable::unchecked("other_key"))
         .with_cond_expr(Expression::Bool(true))
         .with_grouping(true),
     );
@@ -208,9 +208,9 @@ fn custom() {
 
     test_serialize(
         ExpressionSerializer,
-        Conditional::new(Identifier::unchecked("some_cond_var"), "yes", "no"),
+        Conditional::new(Variable::unchecked("some_cond_var"), "yes", "no"),
         Expression::from(Conditional::new(
-            Identifier::unchecked("some_cond_var"),
+            Variable::unchecked("some_cond_var"),
             "yes",
             "no",
         )),
@@ -236,7 +236,7 @@ fn custom() {
         ForExpr::new(
             Identifier::unchecked("value"),
             vec![Expression::String(String::from("foo"))],
-            Identifier::unchecked("other_value"),
+            Variable::unchecked("other_value"),
         )
         .with_key_var(Identifier::unchecked("index"))
         .with_cond_expr(Expression::Bool(true)),
@@ -244,7 +244,7 @@ fn custom() {
             ForExpr::new(
                 Identifier::unchecked("value"),
                 vec![Expression::String(String::from("foo"))],
-                Identifier::unchecked("other_value"),
+                Variable::unchecked("other_value"),
             )
             .with_key_var(Identifier::unchecked("index"))
             .with_cond_expr(Expression::Bool(true)),
@@ -256,19 +256,19 @@ fn custom() {
         ForExpr::new(
             Identifier::unchecked("value"),
             vec![Expression::String(String::from("foo"))],
-            Identifier::unchecked("other_value"),
+            Variable::unchecked("other_value"),
         )
         .with_key_var(Identifier::unchecked("key"))
-        .with_key_expr(Expression::Variable(Identifier::unchecked("key")))
+        .with_key_expr(Variable::unchecked("key"))
         .with_cond_expr(Expression::Bool(true)),
         Expression::from(
             ForExpr::new(
                 Identifier::unchecked("value"),
                 vec![Expression::String(String::from("foo"))],
-                Identifier::unchecked("other_value"),
+                Variable::unchecked("other_value"),
             )
             .with_key_var(Identifier::unchecked("key"))
-            .with_key_expr(Expression::Variable(Identifier::unchecked("key")))
+            .with_key_expr(Variable::unchecked("key"))
             .with_cond_expr(Expression::Bool(true)),
         ),
     );
@@ -276,11 +276,11 @@ fn custom() {
     test_serialize(
         ConditionalSerializer,
         (
-            Expression::Variable(Identifier::unchecked("some_cond_var")),
+            Expression::from(Variable::unchecked("some_cond_var")),
             Expression::String("yes".into()),
             Expression::String("no".into()),
         ),
-        Conditional::new(Identifier::unchecked("some_cond_var"), "yes", "no"),
+        Conditional::new(Variable::unchecked("some_cond_var"), "yes", "no"),
     );
 
     test_serialize(