Skip to content

Commit 383470b

Browse files
mitsuhikomitsuhiko
authored
loop.nextitem is now a lazy operation (#677)
1 parent f48b190 commit 383470b

13 files changed

+171
-80
lines changed

‎.cargo/config.toml

+1-1
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
[target.wasm32-wasi]
1+
[target.wasm32-wasip1]
22
runner = ["./scripts/wasmtime-wrapper.sh"]
33

44
[target.aarch64-unknown-linux-gnu]

‎.github/workflows/tests.yml

+1-1
Original file line numberDiff line numberDiff line change
@@ -109,7 +109,7 @@ jobs:
109109
- uses: dtolnay/rust-toolchain@master
110110
with:
111111
toolchain: stable
112-
targets: wasm32-wasi
112+
targets: wasm32-wasip1
113113
- uses: Swatinem/rust-cache@v2
114114
- name: Install WasmTime
115115
run: |

‎CHANGELOG.md

+3
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,9 @@ All notable changes to MiniJinja are documented here.
55
## 2.7.0
66

77
- Removed string interning. #675
8+
- `loop.nextitem` is now a lazy operation. This prevents issues when
9+
iterating over one-shot iterators combined with `{% break %}` and
10+
it now ensures that the iterator is not running "one item ahead". #677
811

912
## 2.6.0
1013

‎Makefile

+1-1
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ test: test-msrv test-cli
2626

2727
.PHONY: wasi-test
2828
wasi-test:
29-
@cd minijinja; cargo test --all-features --target=wasm32-wasi -- --nocapture
29+
@cd minijinja; cargo test --all-features --target=wasm32-wasip1 -- --nocapture
3030

3131
.PHONY: python-test
3232
python-test:

‎minijinja/src/syntax.rs

+5
Original file line numberDiff line numberDiff line change
@@ -746,6 +746,11 @@
746746
//! {%- endfor %}
747747
//! ```
748748
//!
749+
//! **Note on one-shot iterators:** if you break from a loop but you have
750+
//! accessed the `loop.nextitem` special variable, then you will lose one item.
751+
//! This is because accessing that attribute will peak into the iterator and
752+
//! there is no support for "putting values back".
753+
//!
749754
#![cfg_attr(
750755
feature = "custom_syntax",
751756
doc = r###"

‎minijinja/src/vm/context.rs

+4-13
Original file line numberDiff line numberDiff line change
@@ -1,29 +1,20 @@
11
use std::borrow::Cow;
22
use std::collections::{BTreeMap, HashSet};
33
use std::fmt;
4+
5+
#[cfg(any(feature = "macros", feature = "multi_template"))]
46
use std::sync::Arc;
57

68
use crate::environment::Environment;
79
use crate::error::{Error, ErrorKind};
8-
use crate::value::{Value, ValueIter};
9-
use crate::vm::loop_object::Loop;
10+
use crate::value::Value;
11+
use crate::vm::loop_object::LoopState;
1012

1113
#[cfg(feature = "macros")]
1214
use crate::vm::closure_object::Closure;
1315

1416
type Locals<'env> = BTreeMap<&'env str, Value>;
1517

16-
pub(crate) struct LoopState {
17-
pub(crate) with_loop_var: bool,
18-
pub(crate) recurse_jump_target: Option<usize>,
19-
// if we're popping the frame, do we want to jump somewhere? The
20-
// first item is the target jump instruction, the second argument
21-
// tells us if we need to end capturing.
22-
pub(crate) current_recursion_jump: Option<(usize, bool)>,
23-
pub(crate) iterator: ValueIter,
24-
pub(crate) object: Arc<Loop>,
25-
}
26-
2718
pub(crate) struct Frame<'env> {
2819
pub(crate) locals: Locals<'env>,
2920
pub(crate) ctx: Value,

‎minijinja/src/vm/loop_object.rs

+115-18
Original file line numberDiff line numberDiff line change
@@ -3,15 +3,126 @@ use std::sync::atomic::{AtomicUsize, Ordering};
33
use std::sync::{Arc, Mutex};
44

55
use crate::error::{Error, ErrorKind};
6-
use crate::value::{Enumerator, Object, Value};
6+
use crate::value::{Enumerator, Object, Value, ValueIter};
77
use crate::vm::state::State;
88

9+
pub(crate) struct LoopState {
10+
pub(crate) with_loop_var: bool,
11+
pub(crate) recurse_jump_target: Option<usize>,
12+
13+
// if we're popping the frame, do we want to jump somewhere? The
14+
// first item is the target jump instruction, the second argument
15+
// tells us if we need to end capturing.
16+
pub(crate) current_recursion_jump: Option<(usize, bool)>,
17+
18+
// Depending on if adjacent_loop_items is enabled or not, the iterator
19+
// is stored either on the loop state or in the loop object. This is
20+
// done because when the feature is disabled, we can avoid using a mutex.
21+
pub(crate) object: Arc<Loop>,
22+
#[cfg(not(feature = "adjacent_loop_items"))]
23+
iter: crate::value::ValueIter,
24+
}
25+
26+
impl LoopState {
27+
pub fn new(
28+
iter: ValueIter,
29+
depth: usize,
30+
with_loop_var: bool,
31+
recurse_jump_target: Option<usize>,
32+
current_recursion_jump: Option<(usize, bool)>,
33+
) -> LoopState {
34+
// for an iterator where the lower and upper bound are matching we can
35+
// consider them to have ExactSizeIterator semantics. We do however not
36+
// expect ExactSizeIterator bounds themselves to support iteration by
37+
// other means.
38+
let len = match iter.size_hint() {
39+
(lower, Some(upper)) if lower == upper => Some(lower),
40+
_ => None,
41+
};
42+
LoopState {
43+
with_loop_var,
44+
recurse_jump_target,
45+
current_recursion_jump,
46+
object: Arc::new(Loop {
47+
idx: AtomicUsize::new(!0usize),
48+
len,
49+
depth,
50+
#[cfg(feature = "adjacent_loop_items")]
51+
iter: Mutex::new(AdjacentLoopItemIterWrapper::new(iter)),
52+
last_changed_value: Mutex::default(),
53+
}),
54+
#[cfg(not(feature = "adjacent_loop_items"))]
55+
iter,
56+
}
57+
}
58+
59+
pub fn did_not_iterate(&self) -> bool {
60+
self.object.idx.load(Ordering::Relaxed) == 0
61+
}
62+
63+
pub fn next(&mut self) -> Option<Value> {
64+
self.object.idx.fetch_add(1, Ordering::Relaxed);
65+
#[cfg(feature = "adjacent_loop_items")]
66+
{
67+
self.object.iter.lock().unwrap().next()
68+
}
69+
#[cfg(not(feature = "adjacent_loop_items"))]
70+
{
71+
self.iter.next()
72+
}
73+
}
74+
}
75+
76+
#[cfg(feature = "adjacent_loop_items")]
77+
pub(crate) struct AdjacentLoopItemIterWrapper {
78+
prev_item: Option<Value>,
79+
current_item: Option<Value>,
80+
next_item: Option<Value>,
81+
iter: ValueIter,
82+
}
83+
84+
#[cfg(feature = "adjacent_loop_items")]
85+
impl AdjacentLoopItemIterWrapper {
86+
pub fn new(iterator: ValueIter) -> AdjacentLoopItemIterWrapper {
87+
AdjacentLoopItemIterWrapper {
88+
prev_item: None,
89+
current_item: None,
90+
next_item: None,
91+
iter: iterator,
92+
}
93+
}
94+
95+
pub fn next(&mut self) -> Option<Value> {
96+
self.prev_item = self.current_item.take();
97+
self.current_item = if let Some(ref next) = self.next_item.take() {
98+
Some(next.clone())
99+
} else {
100+
self.next_item = None;
101+
self.iter.next()
102+
};
103+
self.current_item.clone()
104+
}
105+
106+
pub fn next_item(&mut self) -> Value {
107+
if let Some(ref next) = self.next_item {
108+
next.clone()
109+
} else {
110+
self.next_item = self.iter.next();
111+
self.next_item.clone().unwrap_or_default()
112+
}
113+
}
114+
115+
pub fn prev_item(&self) -> Value {
116+
self.prev_item.clone().unwrap_or_default()
117+
}
118+
}
119+
9120
pub(crate) struct Loop {
10121
pub len: Option<usize>,
11122
pub idx: AtomicUsize,
12123
pub depth: usize,
13124
#[cfg(feature = "adjacent_loop_items")]
14-
pub value_triple: Mutex<(Option<Value>, Option<Value>, Option<Value>)>,
125+
pub iter: Mutex<AdjacentLoopItemIterWrapper>,
15126
pub last_changed_value: Mutex<Option<Vec<Value>>>,
16127
}
17128

@@ -107,23 +218,9 @@ impl Object for Loop {
107218
"depth" => Some(Value::from(self.depth + 1)),
108219
"depth0" => Some(Value::from(self.depth)),
109220
#[cfg(feature = "adjacent_loop_items")]
110-
"previtem" => Some(
111-
self.value_triple
112-
.lock()
113-
.unwrap()
114-
.0
115-
.clone()
116-
.unwrap_or(Value::UNDEFINED),
117-
),
221+
"previtem" => Some(self.iter.lock().unwrap().prev_item()),
118222
#[cfg(feature = "adjacent_loop_items")]
119-
"nextitem" => Some(
120-
self.value_triple
121-
.lock()
122-
.unwrap()
123-
.2
124-
.clone()
125-
.unwrap_or(Value::UNDEFINED),
126-
),
223+
"nextitem" => Some(self.iter.lock().unwrap().next_item()),
127224
_ => None,
128225
}
129226
}

‎minijinja/src/vm/mod.rs

+14-45
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,8 @@
11
use std::collections::BTreeMap;
22
use std::mem;
3-
use std::sync::atomic::{AtomicUsize, Ordering};
4-
use std::sync::{Arc, Mutex};
3+
4+
#[cfg(any(feature = "macros", feature = "multi_template"))]
5+
use std::sync::Arc;
56

67
use crate::compiler::instructions::{
78
Instruction, Instructions, LOOP_FLAG_RECURSIVE, LOOP_FLAG_WITH_LOOP_VAR, MAX_LOCALS,
@@ -12,8 +13,8 @@ use crate::output::{CaptureMode, Output};
1213
use crate::utils::{untrusted_size_hint, AutoEscape, UndefinedBehavior};
1314
use crate::value::namespace_object::Namespace;
1415
use crate::value::{ops, value_map_with_capacity, Kwargs, ObjectRepr, Value, ValueMap};
15-
use crate::vm::context::{Frame, LoopState, Stack};
16-
use crate::vm::loop_object::Loop;
16+
use crate::vm::context::{Frame, Stack};
17+
use crate::vm::loop_object::LoopState;
1718
use crate::vm::state::BlockStack;
1819

1920
#[cfg(feature = "macros")]
@@ -498,24 +499,7 @@ impl<'env> Vm<'env> {
498499
ctx_ok!(self.push_loop(state, a, *flags, pc, next_loop_recursion_jump.take()));
499500
}
500501
Instruction::Iterate(jump_target) => {
501-
let l = state.ctx.current_loop().unwrap();
502-
l.object.idx.fetch_add(1, Ordering::Relaxed);
503-
504-
let next = {
505-
#[cfg(feature = "adjacent_loop_items")]
506-
{
507-
let mut triple = l.object.value_triple.lock().unwrap();
508-
triple.0 = triple.1.take();
509-
triple.1 = triple.2.take();
510-
triple.2 = l.iterator.next();
511-
triple.1.clone()
512-
}
513-
#[cfg(not(feature = "adjacent_loop_items"))]
514-
{
515-
l.iterator.next()
516-
}
517-
};
518-
match next {
502+
match state.ctx.current_loop().unwrap().next() {
519503
Some(item) => stack.push(assert_valid!(item)),
520504
None => {
521505
pc = *jump_target;
@@ -525,7 +509,7 @@ impl<'env> Vm<'env> {
525509
}
526510
Instruction::PushDidNotIterate => {
527511
let l = state.ctx.current_loop().unwrap();
528-
stack.push(Value::from(l.object.idx.load(Ordering::Relaxed) == 0));
512+
stack.push(Value::from(l.did_not_iterate()));
529513
}
530514
Instruction::Jump(jump_target) => {
531515
pc = *jump_target;
@@ -984,15 +968,7 @@ impl<'env> Vm<'env> {
984968
current_recursion_jump: Option<(usize, bool)>,
985969
) -> Result<(), Error> {
986970
#[allow(unused_mut)]
987-
let mut iterator = ok!(state.undefined_behavior().try_iter(iterable));
988-
// for an iterator where the lower and upper bound are matching we can
989-
// consider them to have ExactSizeIterator semantics. We do however not
990-
// expect ExactSizeIterator bounds themselves to support iteration by
991-
// other means.
992-
let len = match iterator.size_hint() {
993-
(lower, Some(upper)) if lower == upper => Some(lower),
994-
_ => None,
995-
};
971+
let mut iter = ok!(state.undefined_behavior().try_iter(iterable));
996972
let depth = state
997973
.ctx
998974
.current_loop()
@@ -1001,20 +977,13 @@ impl<'env> Vm<'env> {
1001977
let recursive = flags & LOOP_FLAG_RECURSIVE != 0;
1002978
let with_loop_var = flags & LOOP_FLAG_WITH_LOOP_VAR != 0;
1003979
ok!(state.ctx.push_frame(Frame {
1004-
current_loop: Some(LoopState {
980+
current_loop: Some(LoopState::new(
981+
iter,
982+
depth,
1005983
with_loop_var,
1006-
recurse_jump_target: if recursive { Some(pc) } else { None },
1007-
current_recursion_jump,
1008-
object: Arc::new(Loop {
1009-
idx: AtomicUsize::new(!0usize),
1010-
len,
1011-
depth,
1012-
#[cfg(feature = "adjacent_loop_items")]
1013-
value_triple: Mutex::new((None, None, iterator.next())),
1014-
last_changed_value: Mutex::default(),
1015-
}),
1016-
iterator,
1017-
}),
984+
if recursive { Some(pc) } else { None },
985+
current_recursion_jump
986+
)),
1018987
..Frame::default()
1019988
}));
1020989
Ok(())

‎minijinja/tests/inputs/loop_break_one_shot_iter.txt

+5
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
{}
2+
---
3+
normal iteration does not lose items:
4+
{% for item in one_shot_iterator %}{{ item }}{% if item == 1 %}{% break %}{% endif %}{% endfor %}
5+
{%- for item in one_shot_iterator %}{{ item }}{% endfor %}

‎minijinja/tests/inputs/loop_break_one_shot_iter_peek.txt

+5
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
{}
2+
---
3+
peeking loses items:
4+
{% for item in one_shot_iterator %}{{ item }}(next: {{ loop.nextitem }}){% if item == 0 %}{% break %}{% endif %}{% endfor %}
5+
{%- for item in one_shot_iterator %}{{ item }}(next: {{ loop.nextitem }}){% endfor %}

‎minijinja/tests/inputs/loop_controls.txt

+1-1
Original file line numberDiff line numberDiff line change
@@ -4,4 +4,4 @@
44
{%- if item is odd %}{% continue %}{% endif %}
55
{%- if item > 5 %}{% break %}{% endif %}
66
{{- item }}
7-
{%- endfor %}
7+
{%- endfor %}

‎minijinja/tests/snapshots/test_templates__vm@loop_break_one_shot_iter.txt.snap

+8
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
---
2+
source: minijinja/tests/test_templates.rs
3+
description: "normal iteration does not lose items:\n{% for item in one_shot_iterator %}{{ item }}{% if item == 1 %}{% break %}{% endif %}{% endfor %}\n{%- for item in one_shot_iterator %}{{ item }}{% endfor %}"
4+
info: {}
5+
input_file: minijinja/tests/inputs/loop_break_one_shot_iter.txt
6+
---
7+
normal iteration does not lose items:
8+
012

‎minijinja/tests/snapshots/test_templates__vm@loop_break_one_shot_iter_peek.txt.snap

+8
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
---
2+
source: minijinja/tests/test_templates.rs
3+
description: "peeking loses items:\n{% for item in one_shot_iterator %}{{ item }}(next: {{ loop.nextitem }}){% if item == 0 %}{% break %}{% endif %}{% endfor %}\n{%- for item in one_shot_iterator %}{{ item }}(next: {{ loop.nextitem }}){% endfor %}"
4+
info: {}
5+
input_file: minijinja/tests/inputs/loop_break_one_shot_iter_peek.txt
6+
---
7+
peeking loses items:
8+
0(next: 1)2(next: )

0 commit comments

Comments
 (0)