From 5cfef325b03b25ad96d6c229a5ec3fd6a32f700d Mon Sep 17 00:00:00 2001 From: Dirkjan Ochtman Date: Wed, 15 Dec 2021 14:08:45 +0100 Subject: Use a separate trait for object safety (#579) This is 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... --- askama/src/lib.rs | 77 +++++++++++++++++++++++++++++++++++++----- askama_actix/src/lib.rs | 5 ++- askama_shared/src/generator.rs | 22 +++--------- testing/tests/ext.rs | 12 +++---- testing/tests/simple.rs | 7 ++-- 5 files changed, 84 insertions(+), 39 deletions(-) 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 { - 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; + /// 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 DynTemplate for T { + fn dyn_render(&self) -> Result { + ::render(self) + } + + fn dyn_render_into(&self, writer: &mut dyn std::fmt::Write) -> Result<()> { + ::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 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(()) 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!( - ::extension(), - Some("html") - ); + assert_eq!(VariablesTemplate::EXTENSION, Some("html")); } #[derive(Template)] -- cgit