Keats / tera

A template engine for Rust based on Jinja2/Django
http://keats.github.io/tera/
MIT License
3.54k stars 283 forks source link

Error when including a template that extends another #385

Open Arnaz87 opened 5 years ago

Arnaz87 commented 5 years ago

Test project showcasing the error: https://github.com/Arnaz87/tera-test

And the traceback:

thread 'main' panicked at 'internal error: entered unreachable code: render_node -> unexpected node: Extends(WS { left: false, right: false }, "parent-holder.txt")', /home/arnaud/.cargo/git/checkouts/tera-ae1ac1f255426cfc/68dfeb5/src/renderer/processor.rs:812:18
stack backtrace:
   0: std::sys::unix::backtrace::tracing::imp::unwind_backtrace
             at src/libstd/sys/unix/backtrace/tracing/gcc_s.rs:49
   1: std::sys_common::backtrace::_print
             at src/libstd/sys_common/backtrace.rs:71
   2: std::panicking::default_hook::{{closure}}
             at src/libstd/sys_common/backtrace.rs:59
             at src/libstd/panicking.rs:211
   3: std::panicking::default_hook
             at src/libstd/panicking.rs:227
   4: std::panicking::rust_panic_with_hook
             at src/libstd/panicking.rs:491
   5: std::panicking::continue_panic_fmt
             at src/libstd/panicking.rs:398
   6: std::panicking::begin_panic_fmt
             at src/libstd/panicking.rs:353
   7: tera::renderer::processor::Processor::render_node
             at /home/arnaud/.cargo/git/checkouts/tera-ae1ac1f255426cfc/68dfeb5/src/renderer/processor.rs:812
   8: tera::renderer::processor::Processor::render_body
             at /home/arnaud/.cargo/git/checkouts/tera-ae1ac1f255426cfc/68dfeb5/src/renderer/processor.rs:150
   9: tera::renderer::processor::Processor::render_node
             at /home/arnaud/.cargo/git/checkouts/tera-ae1ac1f255426cfc/68dfeb5/src/renderer/processor.rs:804
  10: tera::renderer::processor::Processor::render
             at /home/arnaud/.cargo/git/checkouts/tera-ae1ac1f255426cfc/68dfeb5/src/renderer/processor.rs:858
  11: tera::renderer::Renderer::render
             at /home/arnaud/.cargo/git/checkouts/tera-ae1ac1f255426cfc/68dfeb5/src/renderer/mod.rs:54
  12: tera::tera::Tera::render
             at /home/arnaud/.cargo/git/checkouts/tera-ae1ac1f255426cfc/68dfeb5/src/tera.rs:309
  13: tera_test::main
             at src/main.rs:7
  14: std::rt::lang_start::{{closure}}
             at /rustc/9fda7c2237db910e41d6a712e9a2139b352e558b/src/libstd/rt.rs:74
  15: std::panicking::try::do_call
             at src/libstd/rt.rs:59
             at src/libstd/panicking.rs:310
  16: __rust_maybe_catch_panic
             at src/libpanic_unwind/lib.rs:102
  17: std::rt::lang_start_internal
             at src/libstd/panicking.rs:289
             at src/libstd/panic.rs:398
             at src/libstd/rt.rs:58
  18: std::rt::lang_start
             at /rustc/9fda7c2237db910e41d6a712e9a2139b352e558b/src/libstd/rt.rs:74
  19: main
  20: __libc_start_main
  21: _start
Keats commented 5 years ago

Ah perfect, I guess that's a reproduction of https://github.com/Keats/tera/issues/370

Not sure what the behaviour should be though, probably an error message instead of a panic

Keats commented 5 years ago

The main issue with allowing extends in include is that you can end up with situations where the included template is in the inheritance tree of the template including it so you end up with an infinite loop.

In practice include in Tera should almost never be used anyway, macros are preferred.

Arnaz87 commented 5 years ago

Isn't it better to explicitly detect infinite loops?

How can macros be used in this case?

Arnaz87 commented 5 years ago

In our project we need potentially complex documents in other bigger, also complex documents, I don't know macros very well but I don't know if they are powerful enough to cover that use case.

naturallymitchell commented 5 years ago

With a configurable recursion depth limit in Tera and app-level prevention of insane template structures, I think we'd handle the complexity well from both ends.

Keats commented 5 years ago

I never use include myself, it's only there because it sometimes is easier to include some bits than using a macro.

Do you have an example of complex include? Macros are like functions returning text so you can do everything allowed in Tera aside from inheritance

naturallymitchell commented 5 years ago

the software as a whole is fairly complex. would you run it if installation was simple and easy enough to quickly try this out?

Keats commented 5 years ago

Is it not possible to get a sample included file? I was hesitating removing include completely` for v1 actually so it would be useful to have some info on how it is used. If it's only possible through the software, I'll have a look later if you put the URL

naturallymitchell commented 5 years ago

I'm pretty sure this is our use case that causes the bug: https://github.com/lighttouch-themes/subscription-menu/blob/master/layouts/base-example.html

but we use include a lot in other places

Keats commented 5 years ago

That indeed uses include a lot, way more than I thought it would be used :o It doesn't seem like that repo has included templates extending a base layout though, they look like regular includes

naturallymitchell commented 5 years ago

we have that reproduced for https://github.com/foundpatterns/lighttouch-base/issues/140 in

Arnaz87 commented 5 years ago

It doesn't uses inheritance in includes yet precisely because it's broken, hence the issue

Keats commented 5 years ago

I am personally not planning to implement that feature, is one of you interested in doing it?

naturallymitchell commented 5 years ago

is one of you interested in doing it?

yes, Arnaud said he will code it

if you have any tips ahead, that's cool

Keats commented 5 years ago

I think that's not going to be trivial as allowing extends means you have to render the include as actual template with the current context (eg an include in a loop). Maybe creating a new Processor and just calling render on that would work but then you need to account for recursion and potentially other unforeseen complications.

Arnaz87 commented 5 years ago

https://github.com/foundpatterns/tera/commit/e4a4ef9c586ce6f93e83cacc2adec0a8b6c2d77b

Is there anything wrong with this approach? It was doing something with the call stack and pushing an include frame that I removed, is that important? I think the included template then doesn't have access to locals declared in the including template, when previously it had, is that right?

It's still missing the recursion check, but I want to know if it's going the right way.

No tests are failing with these changes (Except all the error cases, wtf)