Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow macro blocks in included templates #376

Closed
msrd0 opened this issue Nov 6, 2020 · 11 comments · Fixed by #396
Closed

Allow macro blocks in included templates #376

msrd0 opened this issue Nov 6, 2020 · 11 comments · Fixed by #396

Comments

@msrd0
Copy link
Contributor

msrd0 commented Nov 6, 2020

I'm trying to have a nav.html template included in several other templates via {% include "nav.html" %} and was very supprised that I couldn't use macros in that template because macro blocks only allowed at the top level. Please allow macros to appear in included templates, this removes a lot of tedious and error-prone duplication.

@djc
Copy link
Owner

djc commented Nov 6, 2020

Are you aware of the import functionality? That could conceivably help with the organization here.

@msrd0
Copy link
Contributor Author

msrd0 commented Nov 6, 2020

I just searched through your book and I don't find any mentions of import, so I'm not sure what it does. My nav.html file roughly looks like this (but with lots more irrelevant html for styling and stuff):

{% macro entry(link, name) %}
<a href="{{link}}">{{name}}</a>
{% endmacro %}

<nav>
	{% call entry("/", "Dashboard") %}
	{% call entry("/profile", "Profile") %}
	<!-- lots more entries -->
</nav>

So I basically want to import/include the generated html in my other templates, not the macro itself.

@djc
Copy link
Owner

djc commented Nov 6, 2020

Huh, I must not have documented that yet. So if I read the code correctly, you can do {% import "macros.html" as macros %} and then you can call macros::entry("/", "Dashboard"), would that help?

I think the reason that macros can't be defined in includes is that currently includes are just very crudely directly parsed and code generated into the place where they're used. I'm not sure I'd want to change that.

@msrd0
Copy link
Contributor Author

msrd0 commented Nov 6, 2020

I just tried to move the macro into another file and then import that file, but that also doesn't work because just like macros, imports are only allowed at the top level.

@msrd0
Copy link
Contributor Author

msrd0 commented Nov 6, 2020

Just had another idea of moving the entire <nav> block into a second macro, so I don't need to include anything. However, previously I passed down a variable via {% let selected = "dashboard" %}. Now, when I try {% call nav::nav("dashboard") %} I get an error message:

error[E0277]: can't compare `&str` with `str`
  --> src/routing/mod.rs:29:10
   |
29 | #[derive(Template)]
   |          ^^^^^^^^ no implementation for `&str == str`
   |
   = help: the trait `std::cmp::PartialEq<str>` is not implemented for `&str`

which is weird because I've never had any non-literal strings, so every type should be &'static str.

@djc
Copy link
Owner

djc commented Nov 9, 2020

That sounds like a separate bug. Can you dump the codegen for it in a separate issue? (Or write it up as a test case, and I can mentor you through to fixing it yourself, if possible.)

msrd0 added a commit to msrd0/askama that referenced this issue Nov 9, 2020
msrd0 added a commit to msrd0/askama that referenced this issue Nov 9, 2020
@msrd0
Copy link
Contributor Author

msrd0 commented Nov 9, 2020

I added a test case in #379.

@djc
Copy link
Owner

djc commented Nov 9, 2020

So it looks like the problem is how we're taking references of local variables, which we have to be more principled about:

{
            let s = &"foo";
            writer.write_str("\n")?;
            {
                let s = &s;
                let other = &"bar";
                writer.write_str("\n")?;
                if s == "foo" {
                    writer.write_str("IT\'S FOO")?;
                } else if s == other {
                    write!(
                        writer,
                        "it\'s {expr0}",
                        expr0 = &::askama::MarkupDisplay::new_unsafe(&other, ::askama::Html),
                    )?;
                } else {
                    write!(
                        writer,
                        "it\'s not foo and not {expr1}",
                        expr1 = &::askama::MarkupDisplay::new_unsafe(&other, ::askama::Html),
                    )?;
                }
                writer.write_str("\n")?;
            }
            writer.write_str("\n")?;
        }
error[E0277]: can't compare `&&str` with `str`
  --> testing/tests/macro.rs:65:22
   |
65 |                 if s == "foo" {
   |                      ^^ no implementation for `&&str == str`
   |
   = help: the trait `std::cmp::PartialEq<str>` is not implemented for `&&str`
   = note: required because of the requirements on the impl of `std::cmp::PartialEq<&str>` for `&&&str`

error[E0277]: can't compare `&str` with `str`
  --> testing/tests/macro.rs:67:29
   |
67 |                 } else if s == other {
   |                             ^^ no implementation for `&str == str`
   |
   = help: the trait `std::cmp::PartialEq<str>` is not implemented for `&str`
   = note: required because of the requirements on the impl of `std::cmp::PartialEq<&str>` for `&&str`
   = note: required because of the requirements on the impl of `std::cmp::PartialEq<&&str>` for `&&&str`

The code generator is apparently a bit trigger-happy with taking references for local variables (for macro "arguments"), which maybe shouldn't happen quite as much. I'm not sure off the top of my head how we could solve this without breaking a bunch of other code, but that requires digging in a bit more. (Part of this is because binary operators are definitely less forgiving about deref coercion than other parts of the language.)

@vallentin
Copy link
Collaborator

no implementation for &str == str

I've been looking into this. I initially sought a solution, which would work similar to #392, as in it would trigger Rust's automatic dereferencing. I have a somewhat working solution, but it requires specialization. Personally, I'm not opposed to an "unstable" feature, but I feel like this should work on stable too.

I wanted to minimize the amount of code I needed to change, along with avoiding redoing how "trigger-happy" the generator is about borrowing. Though, I'll look into that next.

@djc
Copy link
Owner

djc commented Dec 5, 2020

Minimizing the amount of changed code at any stage is always appreciated -- keeping things separate will make them easier to review.

Sorry, but I want to stick to stable Rust.

Have you seen this article about a way to do something like specialization on stable? I experimented with it (see the specialization branch) a while back.

@vallentin
Copy link
Collaborator

Sorry, but I want to stick to stable Rust.

Don't get me wrong, I was agreeing with that sentiment.

Have you seen this article

I hadn't seen that article in particular. However, I did try something similar, which didn't end up working.

In short, the issue is that when you do e.g. impl ... for &i32, then automatic dereferencing kicks in and you're able to coerce &i32, &&i32, &&&i32 etc. back down to &i32. However, as soon as that &i32 changes to a T or &T, then goodbye automatic dereferencing, as &&&i32 matches both T/&T, so if you try to dereference it once, then it becomes &&i32 and not &i32.

Having non-generic impl ... for ... works. However, then it isn't generic, and as soon custom types are used, then we're back to square one of course.

I've tested various things, and it doesn't seem to be possible to:

  1. Have something that is generic without unstable features (along with no unsafe),
  2. Convert any T down to 1 level of indirection, without supplying the original base/non-referenced type,
  3. Which we can't do, as the parser can't infer the type of variables (specifically, those outside the template struct)

So next, I'll just go ahead and rework the generator, such that at most there is 1 level of indirection, because then it's as easy as dereferencing once.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants