MiniJinja 2.0

[mitsuhiko]

The work for MiniJinja 2 is taking place in the main branch. Maintenance releases for 1.x will be worked on in minijinja-1.x.

• Make ValueKind #[non_exhaustive]. This prevents adding new value types to it.
• Remove the ExactSizeIterator bound for the iterator interface
• Simplify object system (refs Objects 2.0 #442)
• 2.0: Separate Iterators from Sequences #454
• remove minijinja-stack-ref (TODO: consider bringing it back if it still brings value with the new object system)
• Restore WASI Support #468
• 2.0: Rename Value::from_serializable #469
• Review the From implementations for Value (particularly in relation to collections)

[mitsuhiko]

So turns out that ExactSizeIterator was thankfully not expose on the public iteration method, just the internal one. So that is actually not needed which is great.

[SergioBenitez]

One suggestion, if the desire is to separate call_method() and call(), is to call the former call() and the latter invoke(). To me that makes a bit more sense, but it's just a naming issue.

The other relevant idea is to remove call_method() entirely and instead call get_value("method").invoke(). I believe that this already happens, but it wasn't clear to me why this wasn't the standard/only way to call methods.

[SergioBenitez]

I've just upgraded my WIP SSG to the minijinja 2.0 branch and wanted to offer a bit of a usage report. Note that I'm upgrading from the previous seq/map revamp and not the original system.

Positives

• Above everything else, the new system is really nice. It's awesome to have a single from_object and from_dyn_object that works for everything. I don't have to think about what kind of object I'm getting when writing filters or tests, which is really quite nice.

• The mapped_enumeration() method is really, really nice. It's allowed me to get rid of every Vec creation, which I assume translates into performance wins, though I haven't measured yet. I also added an internal mapped_reversible_enumeration(), which was necessary in a few cases.

• All of the pros from the original object revamp also apply, in particular the massive performance wins. I suspect minijinja itself is faster, as in my use of minijinja, making the SSG overall faster.

Considerations

• Having to manually write an Object::repr() method feels very error-prone. I think I would rather either the method be mandatory (i.e, no default) or the default impl at least consider objects with Enumeration::Range to be Seq by default. In general it feels like there's something odd here with respect to having to define the "repr" at all, which is something I brought up earlier. I wonder if looking at the cases where the repr() is meaningful can yield insight into a more elegant API.

• Calling Value::from(&str) feels like an unfortunate allocation, especially when the string is 'static. I think it'd be interesting to explore making ValueRepr::String an Either<&'static str, Arc<str>>.

• I got tripped up with the new Value::is_true() functionality for objects (which I wrote...). I'm not really sure what it should do, quite honestly. But maybe this is the right change to make?

  -            ValueRepr::Object(ref x) => x.enumeration().len().map_or(false, |x| x != 0),
  +            ValueRepr::Object(ref x) => x.enumeration().len().map_or(true, |x| x != 0),

• In my application, I have several ways I want to use a single value as a minijinja Object. For example, I have an Arc<T> that internally contains a Vec<A> and a Vec<B>. I want to create three objects from the single Arc<T>: one with keys a and b, and second for the sequence Vec<A>, and a third for the sequence Vec<B>, and I want to do so with zero allocations. The idea would be to proxy the second two objects through the Arc<T> Object. This isn't directly possible as we can only have a single Object impl for Arc<T>. So to work-around this, I create wrappers around the T: A(SIte) and B(Site). If these are #[repr(transparent)], then we can cast an Arc<T> to either an Arc<A> or Arc<B> safely. Thus we can have Object impls for A and B which give us the desired functionality.

  This feels like it will be a common scenario. In my application, I create 5 such variations of different types. I've created a macro to safely create those variants: declare_variation!(A of T) which produces a type A with a method A::new(t: Arc<T>) -> Arc<A> which lets you do Value::from_dyn_object(A::new(self.clone()) to get a variation. I wonder if including this macro, or something like it, in minijinja would be a good idea.

[mitsuhiko]

Having to manually write an Object::repr() method feels very error-prone. I think I would rather either the method be mandatory (i.e, no default) or the default impl at least consider objects with Enumeration::Range to be Seq by default. In general it feels like there's something odd here with respect to having to define the "repr" at all, which is something I brought up earlier. I wonder if looking at the cases where the repr() is meaningful can yield insight into a more elegant API.

I have to say that I don't like Enumeration::Range either but that's a slightly different issue. I am planning of just changing it to Enumeration::Sized(usize) since any offset not 0 violates the behavior of a sequence. I find it hard to understand why the enumeration would map to the object's repr/shape. The original object model of Jinja follows the Python one including the iteration system. The shape of an object is not defined by it's iteration, but by an explicit protocol. But this is not really set in stone, I want to see what feels best.

I generally believe that the default of Map is reasonable because that's what most objects are.

Calling Value::from(&str) feels like an unfortunate allocation, especially when the string is 'static. I think it'd be interesting to explore making ValueRepr::String an Either<&'static str, Arc>.

Might be worth checking again, but last time I tried it, it resulted in a significant loss in performance.

[mitsuhiko]

On the earlier conversation:

The other relevant idea is to remove call_method() entirely and instead call get_value("method").invoke(). I believe that this already happens, but it wasn't clear to me why this wasn't the standard/only way to call methods.

These are separate operations in MiniJinja so that foo['x'] and foo.x() can resolve to distinct namespaces. The solution for this problem in Jinja2 is very complex (two separate syntaxes) and not necessary if full compatibility with Python's object model is not required.

[mitsuhiko]

In general I really quite like the object model now. There are some warts but they are easy enough to work out. The main point I’m unsure at the moment is the iterator stuff but that would be easy enough to work out.

The explicit repr seems fine to me because most of the time it comes from the utilities automatically or the object is a map anyways but I will consider making it implied later down the road.

[mitsuhiko]

Current state:

• Having to manually write an Object::repr() method feels very error-prone. I think I would rather either the method be mandatory (i.e, no default) or the default impl at least consider objects with Enumeration::Range to be Seq by default. In general it feels like there's something odd here with respect to having to define the "repr" at all, which is something I brought up earlier. I wonder if looking at the cases where the repr() is meaningful can yield insight into a more elegant API.

I'm quite conflicted on this still. At the moment it still defaults to Map but I am going to play around with trying to see what auto detection does.

• I got tripped up with the new Value::is_true() functionality for objects (which I wrote...). I'm not really sure what it should do, quite honestly. But maybe this is the right change to make?

This is largely resolved now. The current logic is:

• Enumeration::NonEnumerable has no size and is always considered truthy
• for all other enumerations, if they are not empty the object is truthy

I reintroduced Value::from_iterator. There is definitely some overlap with Value::from_object_iter and I am not super happy with the mismatching names. The difference is that the former really only should be used for generator type situations with no known upper bound, and Value::from_object_iter looks more like a sequence. It also reports as iterator, but it does stringify into a sequence for debug printing.

The reversing code is entirely gone for now. If that is worthwhile retaining I will find a way to bring it back.