Revamp Filter API

[epage]

Goals

• Base support for named arguments (for support parser, see Support for named arguments in filters #92)
• Filter writers write signature in declarative way, simplifying error reporting and parameter extraction
• Performance by moving more processing to parse-time
• Allow a tool to generate documentation for all the included filters.
• Offload argument-list parsing error handling from the user

Steps

1. Get filters out of runtime

Modify FilterCall to capture the relevant filter, avoiding the need to do this in the runtime

2. Change filter APIs

Like tags/blocks, we'll make filters a two-trait process. The first trait replaces the FilterCall and processes the arguments into the order desired by the filter, erroring if needed. The second trait is generated by this and is captured into the FilterChain.

We'll also have an introspection trait that reports name, description, and each argument with name, description, and whether it is required, optional, positional only, or keyword only.

Rough sketches

trait FilterReflection {
    fn name(&self) -> &' static str;
    fn description(&self) -> &'static str;
    // Not sure on return type
    // Goal is to return name and description
    fn required_parameters(&self) -> &'static [(&'static str, &'statci str)];
    fn optional_parameters(&self) -> &'static [(&'static str, &'statci str)];
    // TODO does liquid have positional-only or keyword-only parameters?
    fn positional_parameters(&self) -> (&'static str, &'statci str);
    fn keyword_parameters(&self) -> (&'static str, &'statci str);
}

trait FilterParser: FilterReflection {
    // In the ideal case, this will re-arrange Expressions into a way friendly for `dyn Filter`, reporting errors for invalid combinations of parameters (too few, too many, etc)
    // This will have the added benefit of having moved these checks from Render time to Parse time which is very helpful for application that Render the same template multiple times (like Cobalt)
    // Unsure on exact signature here
    fn parse(&self, Vec<Expression>, HashMap<&str, Expression>) -> Box<dyn Filter>;
}

trait Filter: Debug {
    // This will evaluate the expressions and evaluate the filter.
    fn filter(&self, input: &Value, context: &Contect) -> Value;
}

Open Questions

• Need to support positional or keyword only?
• How does liquid-ruby compose a mixture of positional and keyword arguments?

Making things simple

We'll have a derive macro that takes a Parameters struct and generates an Arguments struct and helpers for taking the arguments and parsing them into the Arguments struct.

We also want to keep the current optimization for when a Filter has no configuration (100% of filters today; in the future some filters like a markdown filter could have state).

Rough sketch

#[derive(FilterParameters)]
#[parameters(arguments="SliceArguments")]
struct SliceParameters {
    #[parameter(description="")]
    offset: Expression,
    #[parameter(description="")]
    length: Option<Expression>,
}

• Support a #[parameter(rename="")] in case the member's name is insufficient
• Support a #[parameter(positional=true)] and #[parameter(keyword=true)]

This generates

• static (no self) functions on SliceParameters relevant for FilterReflection
• A static function for converting FilterParser::parse's parameters into a SliceParameter
• A static function for converting SliceParameters to SliceArguments

The reason for the static methods is to allow us to maintain our current optimization wherein a struct will be full of a functions points and implement FilterParser, allowing a non-Boxed Copy variant.

TODO Anything more we can help with

[epage]

@Goncalerta I might do Step 1 of this issue as part of my effort to start to lock down Context's API but you might find Step 2 interesting.

[epage]

Got #302 in which takes care of Step 1 and expanded on some of my ideas for Step 2. Still in rough shape.

[Goncalerta]

I'm thinking about FilterReflection being a structure instead and adding a .reflection() function on FilterParser that returns the reflection. What are your thoughts on that?

[epage]

Before I weigh in, I'd be interested to know what is leading to the idea of a FilterReflection struct? Does this have any other impact on the rough sketch?

Also, at this point, would you like to be added as a maintainer on github. if nothing else, it will let you edit the original issue.

[Goncalerta]

Before I weigh in, I'd be interested to know what is leading to the idea of a FilterReflection struct? Does this have any other impact on the rough sketch?

If I understood correctly how FilterReflection will be used, each function will basically be a getter to a fixed value such as, for example:

fn name(&self) -> &' static str {
    "filter_name"
}

It seemed to me that a struct would be more appropriate for this:

FilterReflection {
  name: "filter_name",
  description: "this is a filter",
  ... 
}

Because every member would be a static constant, the whole structure could then be itself a static constant passed in during the registration of the filter. It just felt more "right" to me, at least at a first glance.

However, to be honest I'm not really sure exactly where and how you intend to use the information from FilterReflection. What makes you need it in the first place? Could you expand more on this?

Also, at this point, would you like to be added as a maintainer on github. if nothing else, it will let you edit the original issue.

Well, I'd be glad to. Thank you!

[epage]

Well, I'd be glad to. Thank you!

I think I did it correctly. I've not messed with permissions too much before

[epage]

However, to be honest I'm not really sure exactly where and how you intend to use the information from FilterReflection. What makes you need it in the first place? Could you expand more on this?

So I'm going to go in priority order, even if some things are only tangentially related.

Ideally, the information we use to generate the reflection is also used by a macro to code-generate the mapping from the user's parameters (positional and keyword) to named parameter Expressions stored in a struct which would then be mapped to named Values that the code-gen would pass to the user's actual function. This would consolidate a lot of logic, including how to map keyword parameters and error reporting, and make writing filters easier because you have "named parameters" rather than a &[] and a &HashMap. Worst case scenario, we don't code-generate the logic but provide a helper function that reads the FilterReflection and processes it. This will be a little slower for parse times but will probably get us moving forward faster.

It also seems like it'd be "convenient" if tags, filters, and blocks all included their names. Then a user only needs to pass one thing in to register them. The upside for blocks is we'd then have a &'static str of the end<name> without having to trust the user. The downside is they can't register a plugin under a different name without creating a wrapper. I think doing so is rare enough that it won't be a problem.

The last is due to Liquid's flexibility. Cobalt will be using the jekyll variant of the language. cargo tarball and cargo generate are using a completely custom variant of the language. Someone else might use the standard variant. To help with that, I want to provide introspection on what all plugins exist and a quick reference for them. This will let tools report that information back to the user and will be much easier to maintain than each tool maintaining a site documenting its version of liquid.

[epage]

If I understood correctly how FilterReflection will be used, each function will basically be a getter to a fixed value such as, for example:

Because every member would be a static constant, the whole structure could then be itself a static constant passed in during the registration of the filter. It just felt more "right" to me, at least at a first glance.

That is true that it is all static which does lower the value.

I'll step you through some of my thought process on this.

tl;dr

The trade offs are very minor. See "Future Compatibility" for why I think I have a slight aesthetic bias towards a trait but would be interested in your thoughts after reading this.

Future Compatibility

We can go two directions with our struct, pub members or getters/setters.

For pub members, we'd need to have a private member to prevent construction and deconstruction / pattern matching to allow us to add new fields in the future without breaking compatibility. We'd then either need to provide a impl Default to populate the private / new fields but this lowers the correctness because the user could not provide one of minimum fields we do want. Alternatively, we could provide a new but that gets unwieldy with a lot of members.

Getters/setters end up having a bit of boiler plate.

Since you mentioned the struct being static / const, both routes are dependent on function calls and so all of this is dependent on the status of the const_fn feature (not been tracking too closely, maybe enough of it is stable now?).

One benefit of a trait (and getters get this as well) is that we can provide default implementations of some things, including implementing them in terms of other functions ("inherent trait functions").

Ease of use

For some weird reason, I lean slightly towards a trait on this. Not sure why.

One of our goals is to simplify the common case with macros. I'd love for our existing filters to not need too much of a change from how they look today though I expect that'd be a lot of complex macro work that might not be worth it and we might need a compromise between the ideal and no macros.

On the other hand, it'd be really nice if we provide macros that compose so that if someone doesn't want all of our built-in logic, they can easily customize it. For example, they might want a macro for parsing parameters into a struct and evaluating the struct. On the other hand, they might want a configurable filter. They have to bypass our optimizations (struct of function pointers) and instead have a Box<dyn Trait>.

So with that in mind, some design considerations. As a user of macros, one of the things I find annoying is when they generate hidden code I need to access (enums, structs, fns). Implementing a trait is easy because you can just point someone to the trait to describe what will be implemented. I think this is even true for your struct idea because I don't think the user would need to access the generated struct at all since we'll have a generated trait to do that for them.

Implementation cost

So the above reminded me of the "struct of function pointers" optimization we use to avoid the cost of Box. If that optimization is still relevant, then we are effectively implementing both the full trait and the struct solution (just function pointers instead of str). That is a bit annoying.

Performance

Our priorities for performance

1. render time
2. parse time
3. Constructing a parser

And the remaining bits that I'm unsure how much or if we care about
4. Reporting errors
5. Providing help for CLIs

Getting the name from FilterReflection is (3) and (4) and the rest is (5). I remember wanting to turn the parsed filter/tag/block name into the 'static version. I think it was for error reporting (as an insignificant performance improvement) but there is a chance it was for a priority case; I just don't remember.

For getting the name, since we don't need to reuse the lookup, it turns into a vtable call vs a vtable + struct lookup which there isn't really a difference there and is all unnoticeable anyways within our application.

[epage]

Just added the goal I forgot about "Offload argument-list parsing error handling from the user".