[stdlib] Deque - part 2 - add operator dunders and remaining traits

[avitkauskas]

This is a second part of Deque implementation adding the operator dunders and all remaining traits.

[JoeLoser]

Looks great, thank you for continuing this work! I left a few minor comments, but happy to see this moving along. When they're addressed, happy to sync this internally and land it.

Feel free to start adding a changelog entry if you'd like for your deque work, or would you prefer until it's fully finished from your perspective?

[JoeLoser]

Question It may be good to add a test for "index normalization" that we do in other collections, or explicitly note that we don't do this for deque. What do you think? Concretely, this would be adding a test case for a negative index (not -1), e.g. -2 in this case, or -3, etc.

[avitkauskas]

Can you please clarify this. We do not want to allow indexes out of range in Deque (Python raises IndexError in these cases). I use debug_assert() for that in the code (same as I see it used in the List implementation). Though I do not fully understand how this should work in Mojo, as the documentation says it's noop in production builds, then actually it means undefined behaviour for the indexes out of range in production builds. Not good in my point of view. Actually, if I try this code

    q = Deque(1, 2)
    e = q[-3]
    print(e)

with mojo run file.mojo it gives 5359310824, and if I try mojo run -g err.mojo it gives me 1. Why?
Why does the debug_assert not fail in debug compile with -g?

debug_assert(
   -len(self) <= normalized_idx < len(self),
    "index: ",
    normalized_idx,
    " is out of bounds for `Deque` of size: ",
    len(self),
)

In nightly tests _ = q[-3] does fail, but I cannot test it with assert_raises().

So, I can add a test case with -2 if it helps, but not with -3 as debug_assert() should 'somehow' save me from these out of range cases, but how? So what should be the desired behaviour in stdlib anyway?

[JoeLoser]

Unfortunately, the name debug_assert has nothing to do with "debug mode". To control the behavior of debug_assert, you'd need to specify -D ASSERT=all for example to enable assertions.

Right now, the testing for various debug_assert isn't ideal, but doable - see

mojo/stdlib/test/builtin/test_range_out_of_bounds.mojo

Line 36 in ce76c9e

# ensure out-of-bounds access calls abort through the assert handler

for example (note the test is using the not --crash in its RUN directive).

More broadly speaking, what our plan is with index normalization for various collections is something we need to figure out, so sticking with what you have is totally fine. Right now, due to the viralness of raises, we don't want to raise for out of bound accesses like in Python. Otherwise, it pushes all downstream callers to be in a potentially-raising context which won't be nice in the current state of Mojo.

[avitkauskas]

I tried to test the index out of range (to include it in the next PR part).
I included these lines in the test file:

# REQUIRES: has_not
# RUN: not --crash mojo -D ASSERT=all %s 2>&1

But I got an error:

FAIL: Mojo Standard Library :: collections/test_deque.mojo (1 of 1)
******************** TEST 'Mojo Standard Library :: collections/test_deque.mojo' FAILED ********************
Exit Code: 1

Command Output (stderr):
--
RUN: at line 14: not --crash mojo -D ASSERT=all /Users/alvis/repos/mojo/stdlib/test/collections/test_deque.mojo 2>&1
+ not --crash mojo -D ASSERT=all /Users/alvis/repos/mojo/stdlib/test/collections/test_deque.mojo
--

What else am I missing?

[gryznar]

Why it is called appendleft and not appendleft? Mojo style guide points that snake_case should be used :)

[avitkauskas]

This is how it named in Python: https://docs.python.org/3/library/collections.html#collections.deque
Do we want to follow Python API, or should we follow the Mojo guides?
Rust and C++ call it push_front and push_back, so lots of choices if we do not want to follow Python :)

[JoeLoser]

Let's follow the Python APIs for the time being and we can discuss this in our next team design meeting (CC: @ConnorGray).

[gryznar]

Same as above

[avitkauskas]

Please give me feedback on my comments to move forward. I cannot make any changes without your further guidance right now. Concerning the changelog entry, I will add it together with the last part that should follow after this PR.

[JoeLoser]

!sync

[modularbot]

✅🟣 This contribution has been merged 🟣✅

Your pull request has been merged to the internal upstream Mojo sources. It will be reflected here in the Mojo repository on the nightly branch during the next Mojo nightly release, typically within the next 24-48 hours.

We use Copybara to merge external contributions, click here to learn more.

[modularbot]

Landed in ab95268! Thank you for your contribution 🎉