Removed implicit borrowing of literals, calls, and more

[vallentin]

Fixes #404
Partially fixes the code commented by @clouds56 (i.e. it requires using offset.clone())

The truncate, indent, into_f64, and into_isize filters have been changed from taking &usize to usize.
This is a partial breaking change, as while e.g. {{ "hello"|truncate(3) }} is still valid.

Then given that fields are still implicitly borrowed, then {{ "hello"|truncate(self.len) }} would no longer be valid. However, given that calls aren't borrowed implicitly, then now explicitly cloning e.g. {{ "hello"|truncate(self.len.clone()) }} is possible as a workaround.

Which also means the comment by @clouds56 is possible. Before neither {% for i in v.iter().skip(5) %} nor {% for i in v.iter().skip(self.offset.clone()) %} were possible. Now both are possible!

Related: I added the test case previously mention in #401.

---

If desired, we could add special cases to visit_filter/_visit_args to allow, e.g. {{ "hello"|truncate(self.len) }}. Thus that in those cases, using fields won't be referenced.

---

The only remaining test case that broke by this change, was test_root_path_func_call, which I added a month ago in #393.

This doesn't work, given that ToString::to_string expects e.g. &i32 while it would be given a i32.

{{ ::std::string::ToString::to_string(123) }}

However, there is a workaround that does work within the template itself, by defining a temporary variable.

{% let i = 123 %}
{{ ::std::string::ToString::to_string(i) }}

---

Side note, it wasn't possible to use the abs filter before, given that expects an owned value. No change is needed to make it work now.

[vallentin]

Alternatively, we could also just force that _visit_args always borrows when used from the context of visit_filter. However, I haven't looked into that, so unsure whether or how much it might complicate the logic.

Pro: That way there would be no breaking changes.
Con: It might confuse users when reasoning about the implicit borrowing.

[djc]

IMO calling to_string() as a top-level expression is pretty pointless, since expressions will rely on Display anyway (so it's unnecessary).

This workaround isn't very discoverable, I think, so it doesn't seem very useful:

{% let i = 123 %}
{{ ::std::string::ToString::to_string(i) }}

I think this is a good change, though I still feel it probably warrants a breaking version number.

Alternatively, we could also just force that _visit_args always borrows when used from the context of visit_filter. However, I haven't looked into that, so unsure whether or how much it might complicate the logic.

Pro: That way there would be no breaking changes.
Con: It might confuse users when reasoning about the implicit borrowing.

Yeah, it's clever but it's probably too hard to reason about.

[vallentin]

IMO calling to_string() as a top-level expression is pretty pointless, since expressions will rely on Display anyway (so it's unnecessary).

I agree, I just took the example from the test case. The same would apply for user defined function, e.g. foo(i: &i32), then that now would fail with {{ self::foo(123) }}. But a workaround would be {% let i = 123 %} {{ self::foo(i) }}. However, in this case it being a user defined function, would make it a lot easier to just change &i32 to i32.

I think this is a good change, though I still feel it probably warrants a breaking version number.

Should we get a patch release out first, with all the prior non-breaking changes?

Yeah, it's clever but it's probably too hard to reason about.

It would boil down to that filters borrow literals/everything, whereas anything else doesn't borrow literals.

On one hand, I agree that it would be confusing. On the other hand, it technically only means that it's the behavior of e.g. function calls that have been changed to not borrow literals.

But yes, that would still be confusing and potentially inconsistent behavior that a user would have trouble reasoning with.

[djc]

Should we get a patch release out first, with all the prior non-breaking changes?

How much stuff do you still have on your list of things to fix? I'm inclined to just keep going for a while -- I don't think "breaking" releases are that bad, especially if they don't actually break much in practice.

[vallentin]

When this is merged, I can finally delete my fork, as this is the last thing committed on my fork! 😅

How much stuff do you still have on your list of things to fix?

I'll need to sort out my notes. It's a bit jumbled right now, between issues, stuff that can be refactored, and missing Jinja features.

For instance, sent you a message in relation to the ~ operator. This could easily be added, and wouldn't be a breaking change of course.

I don't think "breaking" releases are that bad

Sure. I meant more, if you wanted the patches and minor features added first. Then we could wait with merging this. Though I'm fine with either.