Implement Range for OpStack

[cyberbono3]

Closes #322

[jan-ferdinand]

Thanks for taking this on! Unfortunately, there is a logic error in the implementation. The documentation of field stack on OpStack reads:

/// The underlying, actual stack. When manually accessing, be aware of reversed indexing:
/// while `op_stack[0]` is the top of the stack, `op_stack.stack[0]` is the lowest element in
/// the stack.

Consequently, accessing op_stack[0..2] (equivalent to op_stack.index(0..2) or op_stack.index_mut(0..2) depending on context and determined by the compiler) should give op_stack.stack[op_stack.stack.len() - 2..op_stack.stack.len()].reverse() or similar. Note that this probably (a) doesn't compile and (b) is likely to contain off-by-one errors.

[jan-ferdinand]

This file is only generated due to a bug in proptest. It has purposefully not been commited.

[cyberbono3]

I have added processor.txt into .gitignore. Does it sound good to you ?

[cyberbono3]

@jan-ferdinand Sorry for delay. Thanks for your clarifications. Any thoughts on my update?

[cyberbono3]

@jan-ferdinand would you like to change stack to queue ?

[jan-ferdinand]

No; conceptually, it is still a stack. This should probably be reflected in the doc comment. How about:

    /// The underlying, actual stack.
    ///
    /// Even though the type suggests a queue, it is actually used as a stack. The
    /// decision to use a [VecDeque] is motivated by indexing using ranges,
    /// allowing, for example:
    ///
    /// ```no_compile
    /// let my_slice = op_stack[3..=5];
    /// ```
    ///
    /// Note that only `*_front` methods should be called on this stack. Pushing to
    /// or popping from the back is almost certainly a logic error.

[jan-ferdinand]

It seems we have another fundamental issue going on. 😫 Consider the following implementation:

impl Index<Range<usize>> for OpStack {
    type Output = [BFieldElement];

    fn index(&self, range: Range<usize>) -> &Self::Output {
        let (front, back) = self.stack.as_slices();

        if front.is_empty() {
            &back[range]
        } else if back.is_empty() {
            &front[range]
        } else {
            panic!("the underlying op stack is not contiguous")
        }
    }
}

It is super easy to trigger the panic:

#[test]
fn indexing_with_range_produces_expected_slices() {
    let mut op_stack = test_stack();
    assert_eq!(op_stack[1..3], bfe_array![1, 2]); // 💥
}

/// An [`OpStack`] with [`BFieldElement`] `i` at index `i` and length
/// 2·[`OpStackElement::COUNT`].
fn test_stack() -> OpStack {
    let mut op_stack = OpStack::default();
    for i in (0..OpStackElement::COUNT).rev() {
        op_stack.push(bfe!(i));
    }

    op_stack
}

For impl IndexMut<_> for OpStack, we can easily avoid this by calling self.stack.make_contiguous() in the index method. However, for immutable access, this is impossible.

These are the options I see:

• Give up. (😢)
• Only support IndexMut<_> in combination with ranges. (😕)
• Use a VecDeque and ensure that it is always contiguous. This might impact performance heavily and requires benchmarks.
• Find (or build) a sort of reverse Vec.

I'm sorry that this seemingly simple issue is turning into a bit of a rabbit hole. I appreciate the effort you're putting in. At the same time, I want to emphasize that you should feel free to drop this increasingly complex endeavor at any point. 🙂

[jan-ferdinand]

No; conceptually, it is still a stack. This should probably be reflected in the doc comment. How about:

    /// The underlying, actual stack.
    ///
    /// Even though the type suggests a queue, it is actually used as a stack. The
    /// decision to use a [VecDeque] is motivated by indexing using ranges,
    /// allowing, for example:
    ///
    /// ```no_compile
    /// let my_slice = op_stack[3..=5];
    /// ```
    ///
    /// Note that only `*_front` methods should be called on this stack. Pushing to
    /// or popping from the back is almost certainly a logic error.

[jan-ferdinand]

Suggested change

	&self.stack.as_slices().0[range.start..range.end]
	let (front, back) = self.stack.as_slices();
	if front.is_empty() {
	&back[range]
	} else if back.is_empty() {
	&front[range]
	} else {
	panic!("the underlying op stack is not contiguous")
	}

[cyberbono3]

It seems we have another fundamental issue going on. 😫 Consider the following implementation:

impl Index<Range<usize>> for OpStack {
    type Output = [BFieldElement];

    fn index(&self, range: Range<usize>) -> &Self::Output {
        let (front, back) = self.stack.as_slices();

        if front.is_empty() {
            &back[range]
        } else if back.is_empty() {
            &front[range]
        } else {
            panic!("the underlying op stack is not contiguous")
        }
    }
}

It is super easy to trigger the panic:

#[test]
fn indexing_with_range_produces_expected_slices() {
    let mut op_stack = test_stack();
    assert_eq!(op_stack[1..3], bfe_array![1, 2]); // 💥
}

/// An [`OpStack`] with [`BFieldElement`] `i` at index `i` and length
/// 2·[`OpStackElement::COUNT`].
fn test_stack() -> OpStack {
    let mut op_stack = OpStack::default();
    for i in (0..OpStackElement::COUNT).rev() {
        op_stack.push(bfe!(i));
    }

    op_stack
}

For impl IndexMut<_> for OpStack, we can easily avoid this by calling self.stack.make_contiguous() in the index method. However, for immutable access, this is impossible.

These are the options I see:

• Give up. (😢)
• Only support IndexMut<_> in combination with ranges. (😕)
• Use a VecDeque and ensure that it is always contiguous. This might impact performance heavily and requires benchmarks.
• Find (or build) a sort of reverse Vec.

I'm sorry that this seemingly simple issue is turning into a bit of a rabbit hole. I appreciate the effort you're putting in. At the same time, I want to emphasize that you should feel free to drop this increasingly complex endeavor at any point. 🙂

Yeah, you are right. I have identified, that IndexMut<_> with self.stack.make_contiguous() requires Index<_> implementation. That is impossible to make it work. So I am willing to drop this endeavour.

Regarding reverse Vec, from my perspective, it introduces some additional complexity that vector has to be sorted every time. Thus, the best option is to leave as it is.

Thanks for your guidance!

[jan-ferdinand]

Thank you for trying, and sorry for so many dead ends. 😕