From 32b623db670dece6d6e5df80a043e01a3edc4ca6 Mon Sep 17 00:00:00 2001
From: Yuya Nishihara <yuya@tcha.org>
Date: Fri, 22 Mar 2024 22:35:00 +0900
Subject: [PATCH] templater: propagate error from formatted string property

---
 cli/src/template_builder.rs | 16 ++++++++--------
 cli/src/templater.rs        | 13 +++++++++----
 2 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/cli/src/template_builder.rs b/cli/src/template_builder.rs
index e8493696f..a4043a9a4 100644
--- a/cli/src/template_builder.rs
+++ b/cli/src/template_builder.rs
@@ -181,14 +181,6 @@ impl<'a> IntoTemplateProperty<'a> for CoreTemplatePropertyKind<'a> {
         match self {
             CoreTemplatePropertyKind::String(property) => Some(property),
             _ => {
-                // TODO: Runtime property evaluation error will be materialized
-                // as string here. Ideally, the error should propagate because
-                // the caller expects a value, not a template to render. Some
-                // ideas to work around the problem:
-                // 1. stringify property without using Template type (works only for
-                //    non-template expressions)
-                // 2. add flag to propagate property error as io::Error::other() (e.g.
-                //    Template::format(formatter, propagate_err))
                 let template = self.try_into_template()?;
                 Some(Box::new(PlainTextFormattedProperty::new(template)))
             }
@@ -1631,6 +1623,7 @@ mod tests {
         env.add_keyword("description", || {
             L::wrap_string(Literal("description 1".to_owned()))
         });
+        env.add_keyword("bad_string", || L::wrap_string(new_error_property("Bad")));
 
         insta::assert_snapshot!(env.render_ok(r#""".len()"#), @"0");
         insta::assert_snapshot!(env.render_ok(r#""foo".len()"#), @"3");
@@ -1643,6 +1636,13 @@ mod tests {
             env.render_ok(r#""description 123".contains(description.first_line())"#),
             @"true");
 
+        // inner template error should propagate
+        insta::assert_snapshot!(env.render_ok(r#""foo".contains(bad_string)"#), @"<Error: Bad>");
+        insta::assert_snapshot!(
+            env.render_ok(r#""foo".contains("f" ++ bad_string) ++ "bar""#), @"<Error: Bad>bar");
+        insta::assert_snapshot!(
+            env.render_ok(r#""foo".contains(separate("o", "f", bad_string))"#), @"<Error: Bad>");
+
         insta::assert_snapshot!(env.render_ok(r#""".first_line()"#), @"");
         insta::assert_snapshot!(env.render_ok(r#""foo\nbar".first_line()"#), @"foo");
 
diff --git a/cli/src/templater.rs b/cli/src/templater.rs
index 922f20863..4aa2f9291 100644
--- a/cli/src/templater.rs
+++ b/cli/src/templater.rs
@@ -435,10 +435,8 @@ impl<T: Template> TemplateProperty for PlainTextFormattedProperty<T> {
     fn extract(&self) -> Result<Self::Output, TemplatePropertyError> {
         let mut output = vec![];
         let mut formatter = PlainTextFormatter::new(&mut output);
-        let mut wrapper = TemplateFormatter::new(&mut formatter, format_property_error_inline);
-        self.template
-            .format(&mut wrapper)
-            .expect("write() to PlainTextFormatter should never fail");
+        let mut wrapper = TemplateFormatter::new(&mut formatter, propagate_property_error);
+        self.template.format(&mut wrapper)?;
         Ok(String::from_utf8(output).map_err(|err| err.utf8_error())?)
     }
 }
@@ -771,6 +769,13 @@ fn format_property_error_inline(
     })
 }
 
+fn propagate_property_error(
+    _formatter: &mut dyn Formatter,
+    err: TemplatePropertyError,
+) -> io::Result<()> {
+    Err(io::Error::other(err.0))
+}
+
 /// Creates function that renders a template to buffer and returns the buffer
 /// only if it isn't empty.
 ///