Add support for "runtime" values #311

GuillaumeGomez · 2025-01-09T20:24:50Z

Fixes #48.

We can now provide values at runtime. It'll be useful for me for switching mdBook to rinja (or askama soon enough) as they need to allow to parse some files after mdBook is compiled if users provide more (not sure it'll be enough though, we'll see).

Funnily enough, it forced me to support the call::<> syntax with references and all.

GuillaumeGomez · 2025-01-10T13:35:24Z

Finally fixed CI. Sorry for the noise.

Kijewski · 2025-01-12T18:32:43Z

rinja/src/values.rs

+/// feature is enabled, it uses the [`BTreeMap`](alloc::collections::BTreeMap) type, otherwise
+/// it uses an empty type and does nothing.
+#[derive(Default)]
+pub struct Values(#[cfg(feature = "alloc")] HashMap<String, Box<dyn Any>>);


I kinda dislike the use of a HashMap in here. Unless there are quiet a lot key-value pairs, a simple slice &[(&str, &dyn Any)] will be excessively much faster. My guess is that a HashMap with many intermediate allocations won't be faster for less than 20 values. (I did not benchmark my guess, though. :) )

Maybe Values could be a trait instead?

Not using a map sounds good to me. I suppose by making Values a trait, we would let it up to users to implement it on whatever they want? But then the question is: should we implement on some types by default as well (like &(&str, &dyn Any) or HashMap<String, Box<dyn Any>>)?

I think it would be useful to implement it for some types, so it works "out of the box". But I would not seal the trait.

What do you mean by "seal the trait"?

mod private { // ← not `pub`, so you can't implement this trait outside of the crate pub trait Sealed {} } pub mod MyTrait: private::Sealed {}

https://predr.ag/blog/definitive-guide-to-sealed-traits-in-rust/#what-are-sealed-traits

Ah so making the trait impossible to implement outside of the crate. I didn't know there was a name for that. TIL, thanks!

Big question: making the Values trait fully generic over the key creates a lot of issues:

DynTemplate is not dyn-compatible anymore

We get unsolvable errors like:
4 | #[derive(Template)] | ^^^^^^^^ | | | expected `&K`, found `&str` | arguments to this method are incorrect

Would it be ok to force the key to be AsRef<str>?

Yes, I think it's a good idea to force the key to be str'ish. Maybe K: Borrow<str> would be the best solution? So the interface is the same as HashMap::get(), and because Borrow<str> implies that K's hash is the same as the borrowed str's hash.

Sounds good to me.

Kijewski · 2025-01-12T18:38:16Z

I am not 100% sure that I see the advantage over adding a field values?

GuillaumeGomez · 2025-01-12T22:00:52Z

Mostly because then, you don't need to add a values field to all impacted type. For example in docs.rs, we have a ~~csrp~~ csrf (EDIT: typo fix) thing we pass when rendering sometimes, but that means all docs.rs types need to have this field, even though so they can't set it and have no use for it.

Kijewski · 2025-01-12T22:13:42Z

Ah, yes, a csrf token sounds like a good use case.

GuillaumeGomez · 2025-01-15T00:00:21Z

Turned Values into a trait as discussed and implemented on some types. Sadly, even with the simplified version, there are still some frustrating limitations. For example, we cannot use &str as key for HashMap otherwise Values is not implemented. And when I tried to remove the for<'a> limitation, I encountered some lifetime issues I couldn't solve.

I'm very curious to see if you will be able to see a way to get around them.

Also, since DynTemplate cannot use generics, I currently commented out the _with_values methods on it.

Kijewski · 2025-01-15T19:14:28Z

I rebased the PR on the current master.

I implemented the idea somewhat differently. Please feel free to unpick the newest commit if you don't like it!

Instead of adding a VALUES variable, I implemented a "key" | value(ty) filter, and made the argument &dyn Values instead of being generic.

GuillaumeGomez · 2025-01-15T19:23:02Z

rinja/src/lib.rs

@@ -83,6 +84,7 @@ pub use rinja_derive::Template;
 pub use crate as shared;
 pub use crate::error::{Error, Result};
 pub use crate::helpers::PrimitiveType;
+pub use crate::values::{NO_VALUES, Value, Values};


I would also reexport get_value since people can always call rinja::get_value in their templates or even in their code (who knows?).

GuillaumeGomez · 2025-01-15T19:24:07Z

rinja_derive/src/generator/node.rs

@@ -1167,7 +1167,8 @@ impl<'a> Generator<'a, '_> {
                        idx
                    };
                    lines.write(format_args!(
-                        "(&&rinja::filters::Writable(expr{idx})).rinja_write(__rinja_writer)?;",
+                        "(&&&rinja::filters::Writable(expr{idx})).\


Why adding a new reference here btw?

I added the values as new argument to WriteWritable. This way it gets automagically passed on to any {{ template }}s. To make this work I need to tell Templates other FastWritables apart.

GuillaumeGomez · 2025-01-15T19:26:30Z

I think it's a much simpler approach, love it! Shouldn't have tried to force too much the trait argument approach... My turn now (some docs to update and some edges I'd like to soften). Thanks!

GuillaumeGomez · 2025-01-15T19:27:04Z

rinja_derive/src/generator/expr.rs

@@ -584,6 +586,7 @@ impl<'a> Generator<'a, '_> {
                return Ok(DisplayWrap::Unwrapped);
            }
        }
+        #[allow(clippy::literal_string_with_formatting_args)]


Oh that's a new lint I added haha.

Thank you, I like the lint! I forgot more than once to surround the string with format!(…). :)

Kijewski · 2025-01-15T19:31:31Z

My turn now (some docs to update and some edges I'd like to soften)

I'm happy that you like my approach, and I'm interested to see what you make out of it! :)

GuillaumeGomez · 2025-01-15T19:33:47Z

Seems like we broke rustc. :o

Kijewski · 2025-01-15T19:38:27Z

Haha, oops.

Can you open an issue? I guess you have more experience with that than me. (And I just noticed that it's 20:40 already and that I have not eaten all day.)

GuillaumeGomez · 2025-01-15T19:39:46Z

Off you go then. Enough computer for today for you! Eat well.

And yes, gonna do that. :)

GuillaumeGomez · 2025-01-15T21:22:54Z

While trying the new changes and in particular the new filter, I'm not a bit fan of the incoherence introduced: the argument is not argument an argument but a generic. What do you think if we instead added support for generics on filters? Or simpler (although I like the filter): we don't make this filter.

Kijewski · 2025-01-16T12:09:42Z

Yes, having a type as a function argument is a bit odd. Something like "key" | value::<u32> should be readable enough. Should we make the :: optional, so you can write "key" | value<u32>, too?

I wouldn't like a free function value("key") very much, because it shadows the method Self::value().

GuillaumeGomez · 2025-01-16T12:55:46Z

Yes, having a type as a function argument is a bit odd. Something like "key" | value::<u32> should be readable enough. Should we make the :: optional, so you can write "key" | value<u32>, too?

I'd prefer to keep the syntax Rust compatible as much as possible (and also I already implemented to parsing of ::<> so less work hehe).

I wouldn't like a free function value("key") very much, because it shadows the method Self::value().

Me neither. Using rinja::get_value sounds good enough to me.

If it sounds good to you, I'll make the changes.

Kijewski · 2025-01-16T13:09:53Z

If it sounds good to you, I'll make the changes.

Sure, go ahead! :)

GuillaumeGomez · 2025-01-16T23:50:02Z

Finally done! I special-cased the rinja::get_value path. I also needed to add even more generics support (for Expr::Call) so I think we support generics pretty much everywhere now. Talk about side-effects 😆 I also extended them to all them to be recursive which allows things like HashMap<String, Vec<u32>>.

I think I did pretty much everything I thought was missing. Thanks a lot for your help getting it to the finish line! :)

Kijewski · 2025-01-17T17:28:03Z

Would it be difficult to extract the turbofish implementation into its own PR, and then rebase this PR on it?

GuillaumeGomez · 2025-01-17T17:32:10Z

Hum, I think it should be mostly fine. The git history will be completely broken so I'm gonna have a good time. :')

GuillaumeGomez · 2025-01-23T14:24:57Z

Much smaller change now. Now two commits only and just value related changes. :)

Kijewski · 2025-01-23T22:30:20Z

rinja_derive/src/generator/expr.rs

+    fn _visit_ty(
+        &mut self,
+        ctx: &Context<'_>,
+        buf: &mut Buffer,
+        ty: &WithSpan<'_, Expr<'a>>,
+    ) -> Result<(), CompileError> {
+        match &**ty {
+            Expr::Var(ty) => {
+                buf.write(normalize_identifier(ty));
+            }
+            Expr::Path(ty) => {
+                self.visit_path(buf, ty);
+            }
+            Expr::Unary("&", rhs) => {
+                buf.write('&');
+                self._visit_ty(ctx, buf, rhs.as_ref())?;
+            }
+            Expr::BinOp(token @ ("<" | ">"), lhs, rhs) => {
+                self._visit_ty(ctx, buf, lhs.as_ref())?;
+                buf.write(token);
+                self._visit_ty(ctx, buf, rhs.as_ref())?;
+            }
+            _ => {
+                return Err(
+                    ctx.generate_error("expected a type name, not an expression", ty.span())
+                );
+            }
+        }
+        Ok(())
+    }
+


Suggested change

fn _visit_ty(

&mut self,

ctx: &Context<'_>,

buf: &mut Buffer,

ty: &WithSpan<'_, Expr<'a>>,

) -> Result<(), CompileError> {

match &**ty {

Expr::Var(ty) => {

buf.write(normalize_identifier(ty));

}

Expr::Path(ty) => {

self.visit_path(buf, ty);

}

Expr::Unary("&", rhs) => {

buf.write('&');

self._visit_ty(ctx, buf, rhs.as_ref())?;

}

Expr::BinOp(token @ ("<" | ">"), lhs, rhs) => {

self._visit_ty(ctx, buf, lhs.as_ref())?;

buf.write(token);

self._visit_ty(ctx, buf, rhs.as_ref())?;

}

_ => {

return Err(

ctx.generate_error("expected a type name, not an expression", ty.span())

);

}

}

Ok(())

}

The method is not used anymore.

Kijewski · 2025-01-23T22:37:27Z

I wonder why neither rustc nor clippy have a problem with the unused method? It calls itself, but nobody else calls it.

Except for the one comment, it look great!

GuillaumeGomez · 2025-01-23T22:44:00Z

It's super strange indeed... Gonna report that. In the meantime, fixed it.

Kijewski

👍

GuillaumeGomez · 2025-01-23T23:33:51Z

I wonder why neither rustc nor clippy have a problem with the unused method? It calls itself, but nobody else calls it.

It's because it starts with an underscore. XD

GuillaumeGomez requested a review from Kijewski January 9, 2025 20:24

GuillaumeGomez force-pushed the runtime-values branch 2 times, most recently from 7bc47be to 4dc727a Compare January 10, 2025 13:19

Kijewski reviewed Jan 12, 2025

View reviewed changes

GuillaumeGomez force-pushed the runtime-values branch 2 times, most recently from 86701f7 to fb8a28f Compare January 15, 2025 16:04

Kijewski force-pushed the runtime-values branch 2 times, most recently from 15ee05e to f16dcef Compare January 15, 2025 19:10

Kijewski force-pushed the runtime-values branch from f16dcef to 8e899b5 Compare January 15, 2025 19:15

GuillaumeGomez commented Jan 15, 2025

View reviewed changes

Kijewski force-pushed the runtime-values branch from 8e899b5 to 98db127 Compare January 15, 2025 19:25

GuillaumeGomez commented Jan 15, 2025

View reviewed changes

Kijewski force-pushed the runtime-values branch from 98db127 to b6b1461 Compare January 15, 2025 19:27

GuillaumeGomez force-pushed the runtime-values branch from 65e94e4 to ac72367 Compare January 16, 2025 23:46

GuillaumeGomez force-pushed the runtime-values branch from 18fd30e to 01ef507 Compare January 16, 2025 23:51

GuillaumeGomez mentioned this pull request Jan 17, 2025

Add support for generics syntax in parser #317

Merged

GuillaumeGomez force-pushed the runtime-values branch from 01ef507 to d192ced Compare January 23, 2025 14:24

Kijewski reviewed Jan 23, 2025

View reviewed changes

Kijewski mentioned this pull request Jan 23, 2025

derive: unify filter argument handling #318

Open

GuillaumeGomez added 2 commits January 23, 2025 23:43

Add support for "runtime" values

b726221

Add new chapter for runtime values

5257838

GuillaumeGomez force-pushed the runtime-values branch from d192ced to 5257838 Compare January 23, 2025 22:43

Kijewski approved these changes Jan 23, 2025

View reviewed changes

GuillaumeGomez mentioned this pull request Jan 23, 2025

Dead code doesn't trigger dead_code lint rust-lang/rust#135957

Closed

GuillaumeGomez merged commit cae09c5 into rinja-rs:master Jan 23, 2025
36 checks passed

GuillaumeGomez deleted the runtime-values branch January 23, 2025 23:08

Add support for "runtime" values #311

Add support for "runtime" values #311

Conversation

GuillaumeGomez commented Jan 9, 2025

GuillaumeGomez commented Jan 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kijewski Jan 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kijewski commented Jan 12, 2025

GuillaumeGomez commented Jan 12, 2025 • edited Loading

Kijewski commented Jan 12, 2025

GuillaumeGomez commented Jan 15, 2025

Kijewski commented Jan 15, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Jan 15, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kijewski commented Jan 15, 2025

GuillaumeGomez commented Jan 15, 2025

Kijewski commented Jan 15, 2025

GuillaumeGomez commented Jan 15, 2025

GuillaumeGomez commented Jan 15, 2025

Kijewski commented Jan 16, 2025

GuillaumeGomez commented Jan 16, 2025

Kijewski commented Jan 16, 2025

GuillaumeGomez commented Jan 16, 2025

Kijewski commented Jan 17, 2025

GuillaumeGomez commented Jan 17, 2025

GuillaumeGomez commented Jan 23, 2025

Choose a reason for hiding this comment

Kijewski commented Jan 23, 2025 • edited Loading

GuillaumeGomez commented Jan 23, 2025

Kijewski left a comment

Choose a reason for hiding this comment

GuillaumeGomez commented Jan 23, 2025

Kijewski Jan 12, 2025 •

edited

Loading

GuillaumeGomez commented Jan 12, 2025 •

edited

Loading

Kijewski commented Jan 23, 2025 •

edited

Loading