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.

Sign up for GitHub

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

Jump to bottom

Iterating over elements of an iterator inside a macro moves the inner collection #708

Open
msrd0 opened this issue Jul 26, 2022 · 6 comments · May be fixed by #714
Open

Iterating over elements of an iterator inside a macro moves the inner collection #708

msrd0 opened this issue Jul 26, 2022 · 6 comments · May be fixed by #714

Comments

@msrd0
Copy link
Contributor

msrd0 commented Jul 26, 2022

For some reason, askama won't use references of the inner collection when using macros, resulting in a compiler error

error[E0507]: cannot move out of `foo.list` which is behind a shared reference
   --> src/main.rs:
    |
280 |     #[derive(Template)]
    |              ^^^^^^^^
    |              |
    |              `foo.list` moved due to this method call
    |              move occurs because `foo.list` has type `BTreeSet<std::string::String>`, which does not implement the `Copy` trait
    |
note: this function takes ownership of the receiver `self`, which moves `foo.list`
   --> /usr/lib/rustlib/src/rust/library/core/src/iter/traits/collect.rs:261:18
    |
261 |     fn into_iter(self) -> Self::IntoIter;
    |                  ^^^^
    = note: this error originates in the derive macro `Template` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0507]: cannot move out of `foo.list` which is behind a shared reference
   --> src/main.rs:
    |
280 |     #[derive(Template)]
    |              ^^^^^^^^
    |              |
    |              `foo.list` moved due to this method call
    |              move occurs because `foo.list` has type `BTreeSet<std::string::String>`, which does not implement the `Copy` trait
    |
    = note: this error originates in the derive macro `Template` (in Nightly builds, run with -Z macro-backtrace for more info)

When taking the code from the macro body and placing it at the call side, it works as expected.

This is a minimal template to reproduce the problem:

{%- macro list(list) -%}
<ul>
	{%- for item in list %}
	<li>{{item}}</li>
	{%- endfor %}
</ul>
<ol>
	{%- for item in list %}
	<li>{{item}}</li>
	{%- endfor %}
</ol>
{%- endmacro -%}

{%- for foo in foos %}
{% call list(foo.list) %}
{%- endfor %}
#[derive(Template)]
#[template(path = "list.j2", print = "code")]
struct List {
	foos: Vec<Foo>
}

struct Foo {
	list: BTreeSet<String>
}
Generated code by askama

impl ::askama::Template for List {
    fn render_into(&self, writer: &mut (impl ::std::fmt::Write + ?Sized)) -> ::askama::Result<()> {
        include_bytes! ("/path/to/list.j2") ;
        {
            let mut _did_loop = false;
            let _iter = (&self.foos).into_iter();
            for (foo, _loop_item) in ::askama::helpers::TemplateLoop::new(_iter) {
                _did_loop = true;
                writer.write_str("\n")?;
                {
                    writer.write_str("<ul>")?;
                    {
                        let mut _did_loop = false;
                        let _iter = (foo.list).into_iter();
                        for (item, _loop_item) in ::askama::helpers::TemplateLoop::new(_iter) {
                            _did_loop = true;
                            ::std::write!(
                                writer,
                                "\n\t<li>{expr0}</li>",
                                expr0 = &::askama::MarkupDisplay::new_unsafe(&(item), ::askama::Html),
                            )?;
                        }
                        if !_did_loop {
                        }
                    }
                    writer.write_str("\n</ul>\n<ol>")?;
                    {
                        let mut _did_loop = false;
                        let _iter = (foo.list).into_iter();
                        for (item, _loop_item) in ::askama::helpers::TemplateLoop::new(_iter) {
                            _did_loop = true;
                            ::std::write!(
                                writer,
                                "\n\t<li>{expr1}</li>",
                                expr1 = &::askama::MarkupDisplay::new_unsafe(&(item), ::askama::Html),
                            )?;
                        }
                        if !_did_loop {
                        }
                    }
                    writer.write_str("\n</ol>")?;
                }
            }
            if !_did_loop {
            }
        }
        ::askama::Result::Ok(())
    }
    const EXTENSION: ::std::option::Option<&'static ::std::primitive::str> =
    Some("j2")
    ;
    const SIZE_HINT: ::std::primitive::usize =
    0
    ;
    const MIME_TYPE: &'static ::std::primitive::str =
    "application/octet-stream"
    ;
}
impl ::std::fmt::Display for List {
    #[inline]
    fn fmt(&self, f: &mut ::std::fmt::Formatter) -> ::std::fmt::Result {
        ::askama::Template::render_into(self, f).map_err(|_| ::std::fmt::Error {})
    }
}

I'm using askama version 0.11.1 from crates.io.
@djc
Copy link
Owner

djc commented Jul 28, 2022

Yeah, I guess we should maybe take a reference of whatever we're passing to the macro? Would you be able to submit a PR?

@msrd0
Copy link
Contributor Author

msrd0 commented Jul 31, 2022

I can try to do a PR but I need some more details on how you think this should be addressed. It sounds like the problem is with the macro somehow having a different logic to decide if the iterator needs to be taken over a reference or not than the template outside the macro? Can you tell me how this works in askama and point me to the relevant code?

@msrd0
Copy link
Contributor Author

msrd0 commented Jul 31, 2022

By looking briefly at your loop code, it looks like askama "forgets" that self is involved and therefore doesn't take a reference. The behaviour can be replicated like this:

{%- for foo in foos %}
{%- set list = foo.list %}
<ul>
	{%- for item in list %}
	<li>{{item}}</li>
	{%- endfor %}
</ul>
<ol>
	{%- for item in list %}
	<li>{{item}}</li>
	{%- endfor %}
</ol>
{%- endfor %}

This produces the exact same error message. Maybe looping over a variable should always loop over that variable's reference?

@djc
Copy link
Owner

djc commented Aug 1, 2022

I think this is the relevant PR: 2302637. We may want to tweak that logic? Navigating whether we want to pass references or ownership is often tricky. I think it could make sense that, when referencing a self field, we want to pass a reference only to the macro.

@djc
Copy link
Owner

djc commented Aug 1, 2022

c7697cb contains similar changes for the macro calling code, I think.

msrd0 added a commit to msrd0/askama that referenced this issue Aug 6, 2022
@msrd0
Add failing test case for djc#708
da6ac75
@msrd0 msrd0 linked a pull request Aug 6, 2022 that will close this issue
Draft
@msrd0
Copy link
Contributor Author

msrd0 commented Aug 6, 2022

I tried to implement your solution, but either I didn't get it quite right or just always taking a reference of macro parameters doesn't work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[DONT MERGE] Always take reference when accessing value passed as argument to a macro msrd0/askama
2 participants
@djc @msrd0