The auto-borrowing system in the context of filters accepting params by value

[couchand]

After reading the conversations on #423 and #492 I'm convinced that filters that take Copy types should heceforth accept their parameters by value. This makes filters very nicely line up with general API design of Copy types.

Passing function call return values and literals is exactly right: {{ 1|abs }} or {{ list.len()|abs }}.

There's just one thing: the very common case of passing template variables is now burdened by an apparently-unnecessary .clone(). What was previously the perfectly clear {{ field|filter }} is now (with the API change to take by value) {{ field.clone()|filter }}.

I'm not sure what the right solution is: obviously the auto-borrow is very useful and necessary in most cases. This wrinkle doesn't seem significant enough to justify introducing * and &. I don't think codegen-time has visibility into trait impls. Maybe all filters should just continue to accept their arguments by reference as a general practice? (What of abs, then?)

[vallentin]

I'm mainly just referencing stuff from the two PRs you linked.

The main reason to prefer pass-by-value compared to pass-by-reference. Is that given you can call .clone() then it's generally always possible to convert &T to T. However, given Askama doesn't have any take reference semantics (&), then it can be somewhat impossible to convert T to &T.

Additionally, if everything is auto-borrowed, then as soon as you throw local variables into the mix. Then soon you end up with &&&T types and worse, when variables are reassigned. Consider i is &&&i32, then now i + 1, i != 0, etc results in compile errors. In this case we can add .clone() to fix that, but I feel like we've just inversed where .clone() is needed then.

See also #392.

[djc]

It might make sense to have abs also take a reference, maybe I just missed that in review?

[couchand]

Something makes me unhappy that these are not interchangeable in a suitable environment:

• {{ 42 | abs }}
• {{ template_var | abs }}
• {% let local_var = 42 %}{{ local_var | abs }}
• {% let local_var = template_var %}{{ local_var | abs }}
• {{ answer.report() | abs }}

And likewise,

• {{ munge(42) }}
• {{ munge(template_var) }}
• {% let local_var = 42 %}{{ munge(local_var) }}
• {% let local_var = template_var %}{{ munge(local_var) }}
• {{ munge(answer.report()) }}

But I'm really sympathetic to the issues raised in the linked issues, particularly the use of iterator methods raised by @clouds56. So I understand the appeal of dropping auto-borrows in some cases.

I'm not really sympathetic to concerns about literals, myself. if {% if false %} doesn't work it would not be the worst restriction. Much more important is that references to the template fields are ergonomic when used with standard filters.

it can be somewhat impossible to convert T to &T.

Well, there is always Borrow::borrow, as pointed out by @msrd0 on #492. So it would seem the situation is generally symmetric. Though I'd guess that the majority case is referencing fields on the Template, counseling in favor of the filters taking the borrowed form.

See also #392.

I've never used macros and so I definitely don't understand the design issues there. But why would calling the macro add a level of reference? If all the code is generated into the same scope, wouldn't it be able to assume everything that needs to be borrowed is already a reference?

It might make sense to have abs also take a reference

I think this might be right. I'm coming back around to the idea that the filters should take references and everything should be a reference, even if it's a Copy type and the usual API design would be to take by-value. Something I only just noticed was that #423 also changed the filters truncate, indent, into_f64, and into_isize to take their arguments by-value.

[vallentin]

The main issue with using Borrow while having a generic type T, is that it causes inference issues. If the parameters are some concrete type, then e.g. using B: Borrow<i32> works great. However, B: Borrow<T> not so much.

Changing abs to taking &T instead of T could work. I mean, it would work, saying could because I'm pretty sure I tested it, in some other cases and had issues. But I don't recall the cases right now. That's not an excuse, I'm just mentioning, that I recall something. So if I can't recall it, then I'm all for changing abs to take &T.

Changing it to taking &T of course means that your first example of {{ 42|abs }} would need to be {{ 42.borrow()|abs }}. If .report() returns e.g. i32, then it would also need to be {{ answer.report().borrow()|abs }}.

In regards to Borrow, I'd love if we could generalize abs to something like:

pub fn abs<T, B>(number: B) -> Result<T>
where
    T: Signed,
    B: Borrow<T>,
{
    Ok(number.borrow().abs())
}

Then ideally be able to keep all your examples as is, and never need to do .clone() nor .borrow(). However, it causes type inference issues for T. If T was e.g. &i32, then it would no longer satisfy the T: Signed constraint.

It would also not solve the issue in relation to &&&T, as you can't .borrow() a &&&T into a &T.

If abs as defined as:

pub fn abs<B>(number: B) -> Result<i32>
where
    B: Borrow<i32>,
{
    Ok(number.borrow().abs())
}

Then all examples would work. So for any filter or function taking a concrete type, then it might be worth using Borrow, to minimize the explicit need for .clone()/.borrow().

[djc]

Maybe we can come up with a local trait MaybeDeref with blanket impls for T: Copy, &T where T: Copy, &&T: Copy, etc?

[vallentin]

It's not possible to have multiple blanket impls like that. Since impl<T> ... for T already matches both i32 and &&&i32. So attempting to have a impl<T> ... for &&&T would result in a "conflicting implementations" compile error.

I recall it being possible to do some magic with specialization, but yeah, specialization is still unstable.

Related comment: #376 (comment)

[couchand]

Post is a bit long, my apologies. Please stick with it, I get somewhere interesting by the end :).

---

I've thought a bit more about this and something occurred to me: in Rust, there is one and only one idiom for "this can take an owned or borrowed thing and you don't need to care which": method call syntax.

let owned = 42;
let borrowed = &owned;
assert_eq!(owned.abs(), borrowed.abs())

This kind of suggests that there just shouldn't be a filter like | abs, since the idiomatic Rust way to write the template would be to use .abs(). I suppose the fact that it was never possible to use until now and nobody complained is a good indication that it's not pulling its weight anyway... (See aside below)

So I've now completely reversed positions from my initial post -- I now think that all filters should take their (first) argument by reference. But what about the extra parameters like on the filters indent and truncate? Given how prevalent you'd expect literal usage with them (e.g. {{ text | truncate(4) }}) the subsequent arguments should probably be by value.

Now we have a weird mis-match, where the first argument of a filter (passed in through the pipe) is always by reference but subsequent arguments are by value. When we're passing literals or function call results into a filter, we'd really like them to be auto-borrowed for us. When we're passing Copy type template fields into function calls, we'd really like them to not be auto-borrowed. That is, in this environment:

#[derive(Template)]
struct Example {
    num: isize,
    text: String,
    object: Object,
}

struct Object;
impl Object {
    fn method(&self) -> isize { todo!() }
    fn munge(&self, other: &isize) -> isize { todo!() }
}

We want to write:

{{ num | abs }}
{{ 1 | abs }}
{{ object.method() | abs }}

And also:

{{ text | truncate(num) }}
{{ text | truncate(1) }}
{{ text | truncate(object.method()) }}

And also:

{% for i in items.iter().skip(num) %}{{ i }}{% endfor %}
{% for i in items.iter().skip(1) %}{{ i }}{% endfor %}
{% for i in items.iter().skip(object.method()) %}{{ i }}{% endfor %}

And I believe this system then implies that function calls that accept reference are now:

{{ object.munge(num.borrow()) }}
{{ object.munge(1isize.borrow()) }}
{{ object.munge(object.method.borrow()) }}

This all leads me to believe that the thing which determines whether we should be auto-borrowing is not whether it's a literal, call, or template field, but rather, how you're using the value.

---

Aside: should the filter syntax codegen to method call syntax? They kind of already look like methods if you squint. You could use any method in scope, and implement custom filters as traits. So Rusty! My oh my that would be a huge breaking change, though.

[msrd0]

in Rust, there is one and only one idiom for "this can take an owned or borrowed thing and you don't need to care which": method call syntax.

This is not true. There is a second one directly in the standard library, namely all macros that work with format, like format!, print!, write! etc. In a function call, this won't work, as the function invokation "eats" the variable that is not Copy:

let s: String = "foo".to_owned();
println("{}", s);
assert_eq!(s.as_str(), "foo"); // s cannot be used anymore

However, using macros from the standard library, this works, and further, also if we take a reference:

let s: String = "foo".to_owned();
println!("{}", s); // works, doesn't consume s
println!("{}", &s); // does exactly the same as above
assert_eq!(s.as_str(), "foo"); // s can still be used

Since askama is a macro, it is IMHO not only idiomatic to have it accept both references and owned values, but also in line with the macros implemented in the standard library.

[couchand]

Interesting angle, @msrd0. Though askama is a macro, it emits a trait implementation, so the code it's lowered to has to play by the rules of functions, not macros. I think the discussion above suggests that there's not really a way to emit code that will handle the distinction without intervention on the part of the template author. If you've got a suggestion of how to implement the more flexible behavior please mention it!

[msrd0]

Sure. Just looking at the expansion of above code, I was able to reproduce the idea behind it (Playground):

fn strlen(s: &str) -> usize {
    s.len()
}

macro_rules! strlen {
    ($s:expr) => {
        match (&$s,) {
            s => strlen(s.0),
        }
    };
}

fn main() {
    let s: String = "foo".to_owned();
    println!("{}", strlen!(s));
    println!("{}", strlen!(&s));
}

So askama could (if it had a filter strlen, which it doesn't, but was an easy example for my proof of concept) emit the code that above macro emits instead of calling the strlen function directly, and thereby work with both references and owned values.

[couchand]

That's a nice idea @msrd0, and seems to go to the idea that all filters should be written to take their argument by reference (since that pattern will not compile if the function takes a value).

---

On another note, how about the result of a filter? Looking into the code, I noticed that after #423, this no longer compiles: {{ foo|truncate(bar|abs) }} (edit: bad example, that doesn't fly because truncate requires a usize which isn't Signed). Perhaps we should remove the auto-borrow from a filter result, since it's roughly equivalent to a function call?