Repository: djc/askama
Pull request #579 (2021-12-15 08:31:57)

Use a separate trait for object safety
This is a relatively major change to the main trait's API. For context,
I always started from the concept of monomorphized traits, but later
several contributors asked about object safety. At that point I made
`Template` object-safe, and then even later added a `SizedTemplate`
to make some things easier for people who don't need object safety.

However, having object-safety in the primary trait is bad for
performance (a substantial number of calls into the virtual `Write`
trait is relatively slow), and I don't think those who don't need
object safety should pay for the cost of having it.

Additionally, I feel using associated consts for the extension and
size hint is more idiomatic than having accessor methods. I don't
know why I didn't use these from the start -- maybe associated
consts didn't exist yet, or I didn't yet know how/when to use them.
Askama is pretty old at this point...

Kijewski: Maybe lose the `Template` requirement for `DynTemplate`? E.g.

```rs
pub trait DynTemplate {
    /// Helper method which allocates a new `String` and renders into it
    fn dyn_render(&self) -> Result<String>;

    /// Renders the template to the given `writer` buffer
    fn dyn_render_into(&self, writer: &mut dyn std::fmt::Write) -> Result<()>;

    /// Helper function to inspect the template's extension
    fn extension(&self) -> Option<&'static str>;

    /// Provides an conservative estimate of the expanded length of the rendered template
    fn size_hint(&self) -> usize;
}

impl<T: Template> DynTemplate for T {
    fn dyn_render(&self) -> Result<String> {
        <Self as Template>::render(self)
    }

    fn dyn_render_into(&self, writer: &mut dyn std::fmt::Write) -> Result<()> {
        <Self as Template>::render_into(self, writer)
    }

    fn extension(&self) -> Option<&'static str> {
        Self::EXTENSION
    }

    fn size_hint(&self) -> usize {
        Self::SIZE_HINT
    }
}
```

Otherwise DynTemplate isn't object-safe because all super-traits need to be object-safe, or am I misunderstanding the requirement in https://doc.rust-lang.org/reference/items/traits.html#object-safety? 
djc: Ohh, good call, thanks! I thought we had some tests for this...
vallentin: > I feel using associated consts for the extension and
size hint is more idiomatic than having accessor methods. I don't
know why I didn't use these from the start -- maybe associated
consts didn't exist yet

Your assumption is right, they weren't stable yet. But yes, it seems to be more idiomatic to use associated constants, a bunch of `::consts` modules, have also moved into `f32`, `i32`, etc types, in the standard library

I'm still waiting for `associated_type_defaults`, I always randomly need it :)
Kijewski: I don't know if this diff is too ugly, but it could prevent accidental shadowing of the identifier `W` if it's used in the user's code:

```diff
diff --git a/askama/src/lib.rs b/askama/src/lib.rs
index c116f55..185e8cc 100644
--- a/askama/src/lib.rs
+++ b/askama/src/lib.rs
@@ -84,7 +84,7 @@ pub trait Template {
     }
 
     /// Renders the template to the given `writer` buffer
-    fn render_into<W: std::fmt::Write + ?Sized>(&self, writer: &mut W) -> Result<()>;
+    fn render_into(&self, writer: &mut (impl std::fmt::Write + ?Sized)) -> Result<()>;
 
     /// The template's extension, if provided
     const EXTENSION: Option<&'static str>;
@@ -179,9 +179,9 @@ mod tests {
     fn dyn_template() {
         struct Test;
         impl Template for Test {
-            fn render_into<W: std::fmt::Write + ?Sized>(
+            fn render_into(
                 &self,
-                writer: &mut W,
+                writer: &mut (impl std::fmt::Write + ?Sized),
             ) -> askama_shared::Result<()> {
                 Ok(writer.write_str("test")?)
             }
diff --git a/askama_shared/src/generator.rs b/askama_shared/src/generator.rs
index bf553fb..ad7e1fe 100644
--- a/askama_shared/src/generator.rs
+++ b/askama_shared/src/generator.rs
@@ -128,7 +128,7 @@ impl<'a, S: std::hash::BuildHasher> Generator<'a, S> {
     ) -> Result<(), CompileError> {
         self.write_header(buf, "::askama::Template", None)?;
         buf.writeln(
-            "fn render_into<W: ::std::fmt::Write + ?Sized>(&self, writer: &mut W) -> \
+            "fn render_into(&self, writer: &mut (impl ::std::fmt::Write + ?Sized)) -> \
              ::askama::Result<()> {",
         )?;
```
djc: A manual implementation of `Template` of a type with a type parameter named `W` seems pretty unlikely, but it doesn't hurt. 👍 
djc: Thanks for all the feedback! I think this is ready to go, and then I'll likely do a release soon.

Patch:
diff --git a/askama/src/lib.rs b/askama/src/lib.rs
index 82a186b..185e8cc 100644
--- a/askama/src/lib.rs
+++ b/askama/src/lib.rs
@@ -73,26 +73,59 @@ use std::path::Path;
 pub use askama_escape::{Html, Text};
 
 /// Main `Template` trait; implementations are generally derived
+///
+/// If you need an object-safe template, use [`DynTemplate`].
 pub trait Template {
     /// Helper method which allocates a new `String` and renders into it
     fn render(&self) -> Result<String> {
-        let mut buf = String::with_capacity(self.size_hint());
+        let mut buf = String::with_capacity(Self::SIZE_HINT);
         self.render_into(&mut buf)?;
         Ok(buf)
     }
+
+    /// Renders the template to the given `writer` buffer
+    fn render_into(&self, writer: &mut (impl std::fmt::Write + ?Sized)) -> Result<()>;
+
+    /// The template's extension, if provided
+    const EXTENSION: Option<&'static str>;
+
+    /// Provides a conservative estimate of the expanded length of the rendered template
+    const SIZE_HINT: usize;
+}
+
+/// Object-safe wrapper trait around [`Template`] implementers
+///
+/// This trades reduced performance (mostly due to writing into `dyn Write`) for object safety.
+pub trait DynTemplate {
+    /// Helper method which allocates a new `String` and renders into it
+    fn dyn_render(&self) -> Result<String>;
+
     /// Renders the template to the given `writer` buffer
-    fn render_into(&self, writer: &mut dyn std::fmt::Write) -> Result<()>;
+    fn dyn_render_into(&self, writer: &mut dyn std::fmt::Write) -> Result<()>;
+
     /// Helper function to inspect the template's extension
     fn extension(&self) -> Option<&'static str>;
-    /// Provides an conservative estimate of the expanded length of the rendered template
+
+    /// Provides a conservative estimate of the expanded length of the rendered template
     fn size_hint(&self) -> usize;
 }
 
-pub trait SizedTemplate {
-    /// Helper function to inspect the template's extension
-    fn extension() -> Option<&'static str>;
-    /// Provides an conservative estimate of the expanded length of the rendered template
-    fn size_hint() -> usize;
+impl<T: Template> DynTemplate for T {
+    fn dyn_render(&self) -> Result<String> {
+        <Self as Template>::render(self)
+    }
+
+    fn dyn_render_into(&self, writer: &mut dyn std::fmt::Write) -> Result<()> {
+        <Self as Template>::render_into(self, writer)
+    }
+
+    fn extension(&self) -> Option<&'static str> {
+        Self::EXTENSION
+    }
+
+    fn size_hint(&self) -> usize {
+        Self::SIZE_HINT
+    }
 }
 
 pub use crate::shared::filters;
@@ -137,3 +170,31 @@ pub mod mime {
     note = "file-level dependency tracking is handled automatically without build script"
 )]
 pub fn rerun_if_templates_changed() {}
+
+#[cfg(test)]
+mod tests {
+    use super::{DynTemplate, Template};
+
+    #[test]
+    fn dyn_template() {
+        struct Test;
+        impl Template for Test {
+            fn render_into(
+                &self,
+                writer: &mut (impl std::fmt::Write + ?Sized),
+            ) -> askama_shared::Result<()> {
+                Ok(writer.write_str("test")?)
+            }
+
+            const EXTENSION: Option<&'static str> = Some("txt");
+
+            const SIZE_HINT: usize = 4;
+        }
+
+        fn render(t: &dyn DynTemplate) -> String {
+            t.dyn_render().unwrap()
+        }
+
+        assert_eq!(render(&Test), "test");
+    }
+}
diff --git a/askama_actix/src/lib.rs b/askama_actix/src/lib.rs
index 2496d92..baf9fd6 100644
--- a/askama_actix/src/lib.rs
+++ b/askama_actix/src/lib.rs
@@ -11,13 +11,12 @@ pub trait TemplateToResponse {
 
 impl<T: askama::Template> TemplateToResponse for T {
     fn to_response(&self) -> HttpResponse {
-        let mut buffer = BytesMut::with_capacity(self.size_hint());
+        let mut buffer = BytesMut::with_capacity(T::SIZE_HINT);
         if self.render_into(&mut buffer).is_err() {
             return ErrorInternalServerError("Template parsing error").error_response();
         }
 
-        let ctype =
-            askama::mime::extension_to_mime_type(self.extension().unwrap_or("txt")).to_string();
+        let ctype = askama::mime::extension_to_mime_type(T::EXTENSION.unwrap_or("txt")).to_string();
         HttpResponse::Ok()
             .content_type(ctype.as_str())
             .body(buffer.freeze())
diff --git a/askama_shared/src/generator.rs b/askama_shared/src/generator.rs
index 41b81df..56c82aa 100644
--- a/askama_shared/src/generator.rs
+++ b/askama_shared/src/generator.rs
@@ -128,7 +128,7 @@ impl<'a, S: std::hash::BuildHasher> Generator<'a, S> {
     ) -> Result<(), CompileError> {
         self.write_header(buf, "::askama::Template", None)?;
         buf.writeln(
-            "fn render_into(&self, writer: &mut dyn ::std::fmt::Write) -> \
+            "fn render_into(&self, writer: &mut (impl ::std::fmt::Write + ?Sized)) -> \
              ::askama::Result<()> {",
         )?;
 
@@ -160,25 +160,13 @@ impl<'a, S: std::hash::BuildHasher> Generator<'a, S> {
         buf.writeln("Ok(())")?;
         buf.writeln("}")?;
 
-        buf.writeln("fn extension(&self) -> Option<&'static str> {")?;
+        buf.writeln("const EXTENSION: ::std::option::Option<&'static ::std::primitive::str> = ")?;
         buf.writeln(&format!("{:?}", self.input.extension()))?;
-        buf.writeln("}")?;
+        buf.writeln(";")?;
 
-        buf.writeln("fn size_hint(&self) -> usize {")?;
+        buf.writeln("const SIZE_HINT: ::std::primitive::usize = ")?;
         buf.writeln(&format!("{}", size_hint))?;
-        buf.writeln("}")?;
-
-        buf.writeln("}")?;
-
-        self.write_header(buf, "::askama::SizedTemplate", None)?;
-
-        buf.writeln("fn size_hint() -> usize {")?;
-        buf.writeln(&format!("{}", size_hint))?;
-        buf.writeln("}")?;
-
-        buf.writeln("fn extension() -> Option<&'static str> {")?;
-        buf.writeln(&format!("{:?}", self.input.extension()))?;
-        buf.writeln("}")?;
+        buf.writeln(";")?;
 
         buf.writeln("}")?;
         Ok(())


Test patch:
diff --git a/testing/tests/ext.rs b/testing/tests/ext.rs
index 5ed4e72..7a494fa 100644
--- a/testing/tests/ext.rs
+++ b/testing/tests/ext.rs
@@ -8,7 +8,7 @@ struct PathHtml;
 fn test_path_ext_html() {
     let t = PathHtml;
     assert_eq!(t.render().unwrap(), "foo.html");
-    assert_eq!(t.extension(), Some("html"));
+    assert_eq!(PathHtml::EXTENSION, Some("html"));
 }
 
 #[derive(Template)]
@@ -19,7 +19,7 @@ struct PathJinja;
 fn test_path_ext_jinja() {
     let t = PathJinja;
     assert_eq!(t.render().unwrap(), "foo.jinja");
-    assert_eq!(t.extension(), Some("jinja"));
+    assert_eq!(PathJinja::EXTENSION, Some("jinja"));
 }
 
 #[derive(Template)]
@@ -30,7 +30,7 @@ struct PathHtmlJinja;
 fn test_path_ext_html_jinja() {
     let t = PathHtmlJinja;
     assert_eq!(t.render().unwrap(), "foo.html.jinja");
-    assert_eq!(t.extension(), Some("html"));
+    assert_eq!(PathHtmlJinja::EXTENSION, Some("html"));
 }
 
 #[derive(Template)]
@@ -41,7 +41,7 @@ struct PathHtmlAndExtTxt;
 fn test_path_ext_html_and_ext_txt() {
     let t = PathHtmlAndExtTxt;
     assert_eq!(t.render().unwrap(), "foo.html");
-    assert_eq!(t.extension(), Some("txt"));
+    assert_eq!(PathHtmlAndExtTxt::EXTENSION, Some("txt"));
 }
 
 #[derive(Template)]
@@ -52,7 +52,7 @@ struct PathJinjaAndExtTxt;
 fn test_path_ext_jinja_and_ext_txt() {
     let t = PathJinjaAndExtTxt;
     assert_eq!(t.render().unwrap(), "foo.jinja");
-    assert_eq!(t.extension(), Some("txt"));
+    assert_eq!(PathJinjaAndExtTxt::EXTENSION, Some("txt"));
 }
 
 #[derive(Template)]
@@ -63,5 +63,5 @@ struct PathHtmlJinjaAndExtTxt;
 fn test_path_ext_html_jinja_and_ext_txt() {
     let t = PathHtmlJinjaAndExtTxt;
     assert_eq!(t.render().unwrap(), "foo.html.jinja");
-    assert_eq!(t.extension(), Some("txt"));
+    assert_eq!(PathHtmlJinjaAndExtTxt::EXTENSION, Some("txt"));
 }
diff --git a/testing/tests/simple.rs b/testing/tests/simple.rs
index e66485a..dcd324e 100644
--- a/testing/tests/simple.rs
+++ b/testing/tests/simple.rs
@@ -1,6 +1,6 @@
 #![allow(clippy::blacklisted_name)]
 
-use askama::{SizedTemplate, Template};
+use askama::Template;
 
 use std::collections::HashMap;
 
@@ -26,10 +26,7 @@ fn test_variables() {
          Iñtërnâtiônàlizætiøn is important\n\
          in vars too: Iñtërnâtiônàlizætiøn"
     );
-    assert_eq!(
-        <VariablesTemplate as SizedTemplate>::extension(),
-        Some("html")
-    );
+    assert_eq!(VariablesTemplate::EXTENSION, Some("html"));
 }
 
 #[derive(Template)]