Merge pull request #7 from woodruffw/ww/better-expr

src/action.rs

@@ -11,7 +11,7 @@ use std::collections::HashMap;
 
 use serde::Deserialize;
 
-use crate::common::{BoE, Env};
+use crate::common::{expr::BoE, Env};
 
 /// A GitHub Actions action definition.
 #[derive(Deserialize)]

src/common.rs

@@ -1,9 +1,11 @@
 //! Shared models and utilities.
 
-use std::{borrow::Cow, collections::HashMap, fmt::Display};
+use std::{collections::HashMap, fmt::Display};
 
 use serde::{Deserialize, Deserializer, Serialize};
 
+pub mod expr;
+
 /// `permissions` for a workflow, job, or step.
 #[derive(Deserialize, Debug, PartialEq)]
 #[serde(rename_all = "kebab-case", untagged)]
@@ -76,100 +78,6 @@ impl Display for EnvValue {
     }
 }
 
-/// Represents a GitHub Actions expression.
-///
-/// This type performs no syntax checking on the underlying expression,
-/// meaning that it might be invalid. The underlying expression may also
-/// be "curly" or "bare" depending on its origin; use an appropriate
-/// method like [`Expression::as_curly`] to access a specific form.
-#[derive(Serialize, Deserialize)]
-#[serde(transparent)]
-pub struct Expression(String);
-
-impl Expression {
-    /// Returns the underlying expression with any whitespace trimmed.
-    /// This is unlikely to be necessary in practice, but can happen
-    /// if a user encapsulates the expression in a YAML string with
-    /// additional whitespace.
-    fn trimmed(&self) -> &str {
-        self.0.trim()
-    }
-
-    /// Returns whether the underlying inner expression is "curly", i.e.
-    /// includes the `${{ ... }}` expression delimiters.
-    fn is_curly(&self) -> bool {
-        self.trimmed().starts_with("${{") && self.trimmed().ends_with("}}")
-    }
-
-    /// Construct an `Expression` from the given value if and only if
-    /// the value is already a "curly" expression.
-    pub fn from_curly(value: String) -> Option<Self> {
-        let expr = Self(value);
-        if !expr.is_curly() {
-            return None;
-        }
-
-        Some(expr)
-    }
-
-    /// Construct an `Expression` from the given value if and only if
-    /// the value is already a "bare" expression.
-    pub fn from_bare(value: String) -> Option<Self> {
-        let expr = Self(value);
-        if expr.is_curly() {
-            return None;
-        }
-
-        Some(expr)
-    }
-
-    /// Returns the "curly" form of this expression, i.e. `${{ expr }}`.
-    pub fn as_curly(&self) -> Cow<'_, str> {
-        if self.is_curly() {
-            Cow::Borrowed(self.trimmed())
-        } else {
-            Cow::Owned(format!("${{{{ {expr} }}}}", expr = self.trimmed()))
-        }
-    }
-
-    /// Returns the "bare" form of this expression, i.e. `expr` if
-    /// the underlying expression is `${{ expr }}`.
-    pub fn as_bare(&self) -> &str {
-        if self.is_curly() {
-            self.trimmed()
-                .strip_prefix("${{")
-                .unwrap()
-                .strip_suffix("}}")
-                .unwrap()
-                .trim()
-        } else {
-            self.trimmed()
-        }
-    }
-}
-
-/// A "literal or expr" type, for places in GitHub Actions where a
-/// key can either have a literal value (array, object, etc.) or an
-/// expression string.
-#[derive(Serialize, Deserialize)]
-#[serde(untagged)]
-pub enum LoE<T> {
-    Literal(T),
-    Expr(Expression),
-}
-
-impl<T> Default for LoE<T>
-where
-    T: Default,
-{
-    fn default() -> Self {
-        Self::Literal(T::default())
-    }
-}
-
-/// A `bool` literal or an actions expression.
-pub type BoE = LoE<bool>;
-
 /// A "scalar or vector" type, for places in GitHub Actions where a
 /// key can have either a scalar value or an array of values.
 ///
@@ -230,7 +138,7 @@ mod tests {
 
     use crate::common::{BasePermission, Permission};
 
-    use super::{Expression, Permissions};
+    use super::Permissions;
 
     #[test]
     fn test_permissions() {
@@ -248,36 +156,4 @@ mod tests {
             )]))
         );
     }
-
-    #[test]
-    fn test_expression() {
-        let expr = Expression("${{ foo }}".to_string());
-        assert_eq!(expr.as_curly(), "${{ foo }}");
-        assert_eq!(expr.as_bare(), "foo");
-
-        let expr = Expression("foo".to_string());
-        assert_eq!(expr.as_curly(), "${{ foo }}");
-        assert_eq!(expr.as_bare(), "foo");
-
-        let expr = Expression(" \t ${{ foo  }} \t\n".to_string());
-        // NOTE: whitespace within the curly is preserved. Worth changing?
-        assert_eq!(expr.as_curly(), "${{ foo  }}");
-        assert_eq!(expr.as_bare(), "foo");
-
-        let expr = Expression("  foo \t\n".to_string());
-        assert_eq!(expr.as_curly(), "${{ foo }}");
-        assert_eq!(expr.as_bare(), "foo");
-    }
-
-    #[test]
-    fn test_expression_from_curly() {
-        assert!(Expression::from_curly("${{ foo }}".into()).is_some());
-        assert!(Expression::from_curly("foo".into()).is_none());
-    }
-
-    #[test]
-    fn test_expression_from_bare() {
-        assert!(Expression::from_bare("${{ foo }}".into()).is_none());
-        assert!(Expression::from_bare("foo".into()).is_some());
-    }
 }

src/common/expr.rs

@@ -0,0 +1,141 @@
+//! GitHub Actions expression parsing and handling.
+
+use serde::{Deserialize, Serialize};
+
+/// An explicit GitHub Actions expression, fenced by `${{ <expr> }}`.
+#[derive(Debug, PartialEq, Serialize)]
+pub struct ExplicitExpr(String);
+
+impl ExplicitExpr {
+    /// Construct an `ExplicitExpr` from the given string, consuming it
+    /// in the process.
+    ///
+    /// Returns `None` if the input is not a valid explicit expression.
+    pub fn from_curly(expr: impl Into<String>) -> Option<Self> {
+        // Invariant preservation: we store the full string, but
+        // we expect it to be a well-formed expression.
+        let expr = expr.into();
+        let trimmed = expr.trim();
+        if !trimmed.starts_with("${{") || !trimmed.ends_with("}}") {
+            return None;
+        }
+
+        Some(ExplicitExpr(expr))
+    }
+
+    /// Return the original string underlying this expression, including
+    /// its exact whitespace and curly delimiters.
+    pub fn as_raw(&self) -> &str {
+        &self.0
+    }
+
+    /// Return the "curly" form of this expression, with leading and trailing
+    /// whitespace removed.
+    ///
+    /// Whitespace *within* the expression body is not removed or normalized.
+    pub fn as_curly(&self) -> &str {
+        self.as_raw().trim()
+    }
+
+    /// Return the "bare" form of this expression, i.e. the `body` within
+    /// `${{ body }}`. Leading and trailing whitespace within
+    /// the expression body is removed.
+    pub fn as_bare(&self) -> &str {
+        return self
+            .as_curly()
+            .strip_prefix("${{")
+            .and_then(|e| e.strip_suffix("}}"))
+            .map(|e| e.trim())
+            .expect("invariant violated: ExplicitExpr must be an expression");
+    }
+}
+
+impl<'de> Deserialize<'de> for ExplicitExpr {
+    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+    where
+        D: serde::Deserializer<'de>,
+    {
+        let raw = String::deserialize(deserializer)?;
+
+        let Some(expr) = Self::from_curly(raw) else {
+            return Err(serde::de::Error::custom(
+                "invalid expression: expected '${{' and '}}' delimiters",
+            ));
+        };
+
+        Ok(expr)
+    }
+}
+
+/// A "literal or expr" type, for places in GitHub Actions where a
+/// key can either have a literal value (array, object, etc.) or an
+/// expression string.
+#[derive(Debug, Deserialize, PartialEq, Serialize)]
+#[serde(untagged)]
+pub enum LoE<T> {
+    // Observe that `Expr` comes first, since `LoE<String>` should always
+    // attempt to parse as an expression before falling back on a literal
+    // string.
+    Expr(ExplicitExpr),
+    Literal(T),
+}
+
+impl<T> Default for LoE<T>
+where
+    T: Default,
+{
+    fn default() -> Self {
+        Self::Literal(T::default())
+    }
+}
+
+/// A convenience alias for a `bool` literal or an actions expression.
+pub type BoE = LoE<bool>;
+
+#[cfg(test)]
+mod tests {
+    use super::{ExplicitExpr, LoE};
+
+    #[test]
+    fn test_expr_invalid() {
+        let cases = &[
+            "not an expression",
+            "${{ missing end ",
+            "missing beginning }}",
+        ];
+
+        for case in cases {
+            let case = format!("\"{case}\"");
+            assert!(serde_yaml::from_str::<ExplicitExpr>(&case).is_err());
+        }
+    }
+
+    #[test]
+    fn test_expr() {
+        let expr = "\"  ${{ foo }} \\t \"";
+        let expr: ExplicitExpr = serde_yaml::from_str(&expr).unwrap();
+        assert_eq!(expr.as_bare(), "foo");
+    }
+
+    #[test]
+    fn test_loe() {
+        let lit = "\"normal string\"";
+        assert_eq!(
+            serde_yaml::from_str::<LoE<String>>(lit).unwrap(),
+            LoE::Literal("normal string".to_string())
+        );
+
+        let expr = "\"${{ expr }}\"";
+        assert!(matches!(
+            serde_yaml::from_str::<LoE<String>>(expr).unwrap(),
+            LoE::Expr(_)
+        ));
+
+        // Invalid expr deserializes as string.
+        let invalid = "\"${{ invalid \"";
+        assert_eq!(
+            serde_yaml::from_str::<LoE<String>>(invalid).unwrap(),
+            LoE::Literal("${{ invalid ".to_string())
+        );
+    }
+}

src/workflow/job.rs

@@ -5,7 +5,8 @@ use std::collections::HashMap;
 use serde::{Deserialize, Serialize};
 use serde_yaml::Value;
 
-use crate::common::{BoE, Env, LoE, Permissions};
+use crate::common::expr::{BoE, LoE};
+use crate::common::{Env, Permissions};
 
 use super::{Concurrency, Defaults};
 
@@ -155,7 +156,7 @@ pub enum Secrets {
 #[cfg(test)]
 mod tests {
     use crate::{
-        common::{EnvValue, LoE},
+        common::{expr::LoE, EnvValue},
         workflow::job::{Matrix, Secrets},
     };
 

src/workflow/mod.rs

@@ -11,7 +11,7 @@ use std::collections::HashMap;
 
 use serde::Deserialize;
 
-use crate::common::{BoE, Env, Permissions};
+use crate::common::{expr::BoE, Env, Permissions};
 
 pub mod event;
 pub mod job;