8000 Fix itertools chain by rng-dynamics · Pull Request #3788 · RustPython/RustPython · GitHub
[go: up one dir, main page]
Skip to content

Fix itertools chain #3788

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

Merged
merged 6 commits into from
Jun 15, 2022
Merged

Fix itertools chain #3788

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a9d16e2
test: itertools.chain evaluate lazliy
rng-dynamics Jun 10, 2022
4c11720
feat: itertools.chain evaluate lazily
rng-dynamics Jun 12, 2022
33bcc89
test: itertools.chain stop iteration on error
rng-dynamics Jun 14, 2022
ec18a12
refactor
rng-dynamics Jun 14, 2022
76ef074
Release source when PyIterReturn::StopIteration
rng-dynamics Jun 14, 2022
480f5f1
fix tests
rng-dynamics Jun 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions extra_tests/snippets/stdlib_itertools.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,18 @@
with assert_raises(TypeError):
next(x)

# iterables are lazily evaluted
x = chain.from_iterable(itertools.repeat(range(2)))
assert next(x) == 0
assert next(x) == 1
assert next(x) == 0
assert next(x) == 1

x = chain(1, [2])
with assert_raises(TypeError):
next(x)
with assert_raises(StopIteration):
next(x)

# itertools.count tests

Expand Down
86 changes: 46 additions & 40 deletions vm/src/stdlib/itertools.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ mod decl {
rc::PyRc,
};
use crate::{
builtins::{int, PyGenericAlias, PyInt, PyIntRef, PyTuple, PyTupleRef, PyTypeRef},
builtins::{int, PyGenericAlias, PyInt, PyIntRef, PyList, PyTuple, PyTupleRef, PyTypeRef},
convert::ToPyObject,
function::{ArgCallable, FuncArgs, OptionalArg, OptionalOption, PosArgs},
identifier,
Expand All @@ -25,19 +25,18 @@ mod decl {
#[pyclass(name = "chain")]
#[derive(Debug, PyPayload)]
struct PyItertoolsChain {
iterables: Vec<PyObjectRef>,
cur_idx: AtomicCell<usize>,
cached_iter: PyRwLock<Option<PyIter>>,
source: PyRwLock<Option<PyIter>>,
active: PyRwLock<Option<PyIter>>,
}

#[pyimpl(with(IterNext))]
impl PyItertoolsChain {
#[pyslot]
fn slot_new(cls: PyTypeRef, 8000 args: FuncArgs, vm: &VirtualMachine) -> PyResult {
let args_list = PyList::from(args.args);
PyItertoolsChain {
iterables: args.args,
cur_idx: AtomicCell::new(0),
cached_iter: PyRwLock::new(None),
source: PyRwLock::new(Some(args_list.to_pyobject(vm).get_iter(vm)?)),
active: PyRwLock::new(None),
}
.into_ref_with_type(vm, cls)
.map(Into::into)
Expand All @@ -46,13 +45,12 @@ mod decl {
#[pyclassmethod]
fn from_iterable(
cls: PyTypeRef,
iterable: PyObjectRef,
source: PyObjectRef,
vm: &VirtualMachine,
) -> PyResult<PyRef<Self>> {
PyItertoolsChain {
iterables: iterable.try_to_value(vm)?,
cur_idx: AtomicCell::new(0),
cached_iter: PyRwLock::new(None),
source: PyRwLock::new(Some(source.get_iter(vm)?)),
active: PyRwLock::new(None),
}
.into_ref_with_type(vm, cls)
}
Expand All @@ -65,37 +63,45 @@ mod decl {
impl IterNextIterable for PyItertoolsChain {}
impl IterNext for PyItertoolsChain {
fn next(zelf: &Py<Self>, vm: &VirtualMachine) -> PyResult<PyIterReturn> {
loop {
let pos = zelf.cur_idx.load();
if pos >= zelf.iterables.len() {
break;
}
let cur_iter = if zelf.cached_iter.read().is_none() {
// We need to call "get_iter" outside of the lock.
let iter = zelf.iterables[pos].clone().get_iter(vm)?;
*zelf.cached_iter.write() = Some(iter.clone());
iter
} else if let Some(cached_iter) = (*zelf.cached_iter.read()).clone() {
cached_iter
} else {
// Someone changed cached iter to None since we checked.
continue;
};

// We need to call "next" outside of the lock.
match cur_iter.next(vm) {
Ok(PyIterReturn::Return(ok)) => return Ok(PyIterReturn::Return(ok)),
Ok(PyIterReturn::StopIteration(_)) => {
zelf.cur_idx.fetch_add(1);
*zelf.cached_iter.write() = None;
}
Err(err) => {
return Err(err);
let next = || {
let source = zelf.source.read().clone();
match source {
None => {
return Ok(PyIterReturn::StopIteration(None));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
match source {
None => {
return Ok(PyIterReturn::StopIteration(None));
let source = if let Some(source) = source {
source
} else {
return Ok(PyIterReturn::StopIteration(None))
};

I have different suggestion to @fanninpm's one. Let's keep indent depth shallower

Copy link
Contributor
@fanninpm fanninpm Jun 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@youknowone that suggestion has a few syntax errors in lines 68 and 71.

EDIT: I stand corrected regarding line 68.

}
Some(source) => loop {
let active = zelf.active.read().clone();
match active {
None => match source.next(vm) {
Ok(PyIterReturn::Return(ok)) => {
*zelf.active.write() = Some(ok.get_iter(vm)?);
}
Ok(PyIterReturn::StopIteration(_)) => {
return Ok(PyIterReturn::StopIteration(None));
}
Err(err) => {
return Err(err);
}
},
Some(active) => match active.next(vm) {
Ok(PyIterReturn::Return(ok)) => {
return Ok(PyIterReturn::Return(ok));
}
Ok(PyIterReturn::StopIteration(_)) => {
*zelf.active.write() = None;
}
Err(err) => {
return Err(err);
}
},
}
},
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📎 "It looks like you're matching variants of an Option. Would you like help?" 📎

if let Some(source) = zelf.source.read().clone() {
    loop {
        if let Some(active) = zelf.active.read().clone() {
            match active.next(vm) {
                Ok(PyIterReturn::Return(ok)) => {
                    return Ok(PyIterReturn::Return(ok));
                }
                Ok(PyIterReturn::StopIteration(_)) => {
                    *zelf.active.write() = None;
                }
                Err(err) => {
                    return Err(err);
                }
            }
        } else {
            match source.next(vm) {
                Ok(PyIterReturn::Return(ok)) => {
                    *zelf.active.write() = Some(ok.get_iter(vm)?);
                }
                Ok(PyIterReturn::StopIteration(_)) => {
                    return Ok(PyIterReturn::StopIteration(None));
                }
                Err(err) => {
                    return Err(err);
                }
            }
        }
    }
} else {
    Ok(PyIterReturn::StopIteration(None))
}

}

Ok(PyIterReturn::StopIteration(None))
};
next().map_err(|err| {
*zelf.source.write() = None;
err
})
}
}

Expand Down
0