amaranth-lang / amaranth

A modern hardware definition language and toolchain based on Python
https://amaranth-lang.org/docs/amaranth/
BSD 2-Clause "Simplified" License
1.49k stars 168 forks source link

View of ArrayLayout cannot be indexed with a negative `int` #1418

Closed whitequark closed 2 days ago

whitequark commented 6 days ago

To reproduce:

>>> from amaranth import *
>>> from amaranth.lib import data
>>> Signal(data.ArrayLayout(5, 2))[-1]
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File ".../lib/python3.12/site-packages/amaranth/lib/data.py", line 804, in __getitem__
    value = self.__target.word_select(key, Shape.cast(self.__layout.elem_shape).width)
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File ".../lib/python3.12/site-packages/amaranth/hdl/_ast.py", line 1269, in word_select
    return self[offset.value * width:(offset.value + 1) * width]
           ~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File ".../lib/python3.12/site-packages/amaranth/hdl/_ast.py", line 1183, in __getitem__
    return Slice(self, start, stop, src_loc_at=1)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File ".../lib/python3.12/site-packages/amaranth/hdl/_ast.py", line 1762, in __init__
    raise IndexError(f"Slice start {start} must be less than slice stop {stop}")
IndexError: Slice start 5 must be less than slice stop 0
whitequark commented 6 days ago

These lines:

https://github.com/amaranth-lang/amaranth/blob/94becb521ab7537f05d6facb1e32ea3e99a94873/amaranth/lib/data.py#L803-L804

should probably be something like:

            shape = self.__layout.elem_shape
            width = Shape.cast(self.__layout.elem_shape).width
            if isinstance(key, int):
                if key < 0:
                    key += self.__layout._length
                value = self.__target[key * width:key * width + width]
            else:
                value = self.__target.word_select(key, width)
whitequark commented 6 days ago

Actually that has the exact same problem as word_select... which we should probably improve the diagnostic for.