[External] [stdlib] Fix UB in reversed(Dict.values()) and `reversed…

…(Dict.items())` (#40974)

[External] [stdlib] Fix UB in `reversed(Dict.values())` and
`reversed(Dict.items())`

Finally found the culprit in the flakyness that plagued us since a few
week in the `test_reversed.mojo`.

### The actual bug:

When iterating over a list in reverse order, we should start at
`len(my_list) - 1` not at `len(my_list)`.
That triggered an out of bounds access and thus was undefined behavior.

### The effect on our CI
As you know, we have been seeing flakyness lately. It was documented a
number of times and always related to `reverse`:
* #2866 (comment)
* #2369

### Why was it passing sometimes?
This is because there were `Optional[...]` in the List. Thus if the flag
of the `Optional` says that no element is present, it's just skipped
(the dict doesn't have an entry at this index). So the list of the Dict
would often look like this: `["a", "b", "c", "d"] None`
but the last `None` is actually memory that we don't have access to.
Sometimes it's then skipped in the iteration making the tests pass.
Sometimes it would cause segfaults because the test dict worked with
strings. Sometimes we would get `wrong variant type` since we don't know
what happens to the memory between None check and access.

### Why wasn't it found earlier?

First of all, our Dict implementation is too complexe for what it does
and thus is very good at hiding bugs.

Well we did have `debug_assert` before getting the element of the
`List`, but this `debug_assert` looked like this in the dict iterator:
```mojo
            @parameter
            if forward:
                debug_assert(
                    self.index < self.src[]._reserved, "dict iter bounds"
                )
            else:
                debug_assert(self.index >= 0, "dict iter bounds")
```
So one bound was checked when reading in one direction and the other
bound was checked in the other direction. A better `debug_assert` would
have been
```mojo
debug_assert(0 <= self.index < self.src[]._reserved, "dict iter bounds")
```
When I worked on my PR #2718 the
condition `self.index < self.src[]._reserved` didn't trigger anything
since it was in the wrong branch, it was never executed.

Also before, `__get_ref` didn't have any bounds checks, even when
assertions were enabled.

A recent commit
8d0870e
adds `unsafe_get()` in List and make `__get_ref` use it. It also adds
`debug_assert` to `unsafe_get()`, which means that now `__get_ref` has
bounds checks if run with assertions enabled. This allowed me to catch
the out of bounds access when updating
#2718 making the fail
deterministic and debuggable.

Since we have this, the `debug_assert` in `dict.mojo` isn't necessary
anymore.

### Consequences on ongoing work:
* This fix have been also added to
#2718
* The PR #2701 that we did with
@jayzhan211 was actually correct. It was just using
`reverse(Dict.items())` which was buggy at the time. After the fix is
merged, we can re-revert this PR.
* #2794 is not necessary anymore
since the implementation by @jayzhan211 was correct.
* The real cause of #2866 was
found, the issue has already been closed though.
* #2369 can be closed for good.
* #2832 can be closed for good.

### Closing thoughts
* We really need to run the unit tests with assertions enabled and add
assertions whenever necessary
* The dict implementation is a bit too complicated. For example,
`self._reserved` is the length of the internal list. There is no need to
store the length of the list twice. Let's drop this variable and use
`len(self._entries)` instead. I guess this is a relic of the time when
`List` wasn't completely flushed out. If had done so, it would have been
ovious that we can't do `my_list.__get_ref(len(my_list))`
* Iterating manually over a list like this is bug-prone. The
implementation we have especially is, since
```mojo
                @parameter
                if forward:
                    self.index += 1
                else:
                    self.index -= 1
```
is done twice in the code, it should only be done once. While there is
no bug, code duplication and complexity hides bugs.
* We should iterate over the list with a list iterator, not with a
custom-made iterator. This will remove a lot of code in the `dict.mojo`.

Co-authored-by: Gabriel de Marmiesse <gabriel.demarmiesse@datadoghq.com>
Closes #2896
MODULAR_ORIG_COMMIT_REV_ID: b65009dc51f1e3027f91b5b61a5b7003cb022b87

stdlib/src/builtin/reversed.mojo

@@ -193,5 +193,5 @@ fn reversed[
     """
     var src = Reference(value)[].src
     return _DictEntryIter[K, V, dict_mutability, dict_lifetime, False](
-        src[]._reserved, 0, src
+        src[]._reserved - 1, 0, src
     )

stdlib/src/collections/dict.mojo

@@ -85,15 +85,6 @@ struct _DictEntryIter[
     @always_inline
     fn __next__(inout self) -> Self.ref_type:
         while True:
-
-            @parameter
-            if forward:
-                debug_assert(
-                    self.index < self.src[]._reserved, "dict iter bounds"
-                )
-            else:
-                debug_assert(self.index >= 0, "dict iter bounds")
-
             var opt_entry_ref = self.src[]._entries.__get_ref(self.index)
             if opt_entry_ref[]:
 
@@ -187,7 +178,7 @@ struct _DictValueIter[
         var src = self.iter.src
         return _DictValueIter(
             _DictEntryIter[K, V, dict_mutability, dict_lifetime, False](
-                src[]._reserved, 0, src
+                src[]._reserved - 1, 0, src
             )
         )