Use a separate trait for object safety

[djc]

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.

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...

[Kijewski]

The change looks sensible to me. 👍

[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 --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.

[Kijewski]

LGTM except for the two last comments.

[djc]

I was waiting for @vallentin's approval, but I guess it's fine in this case. :)

[vallentin]

I didn't have any additional "deny comments", which is why I just added regular comments. So it was fine with me :)