Skip to content

Commit

Permalink
hdl.ast: warn on fencepost error in Signal(range(x), reset=x).
Browse files Browse the repository at this point in the history
Also, relax the language reference inset from "warning" to "note"
since this is no longer something developers have to keep in mind
explicitly.
  • Loading branch information
whitequark committed Mar 13, 2023
1 parent 32eabd9 commit 80343d1
Show file tree
Hide file tree
Showing 3 changed files with 33 additions and 4 deletions.
15 changes: 15 additions & 0 deletions amaranth/hdl/ast.py
Expand Up @@ -632,6 +632,13 @@ def __init__(self, value, shape=None, *, src_loc_at=0):
elif isinstance(shape, int):
shape = Shape(shape, signed=self.value < 0)
else:
if isinstance(shape, range) and self.value == shape.stop:
warnings.warn(
message="Value {!r} equals the non-inclusive end of the constant "
"shape {!r}; this is likely an off-by-one error"
.format(self.value, shape),
category=SyntaxWarning,
stacklevel=2)
shape = Shape.cast(shape, src_loc_at=1 + src_loc_at)
self.width = shape.width
self.signed = shape.signed
Expand Down Expand Up @@ -1023,6 +1030,14 @@ def __init__(self, shape=None, *, name=None, reset=0, reset_less=False,
self.reset = reset.value
self.reset_less = bool(reset_less)

if isinstance(orig_shape, range) and self.reset == orig_shape.stop:
warnings.warn(
message="Reset value {!r} equals the non-inclusive end of the signal "
"shape {!r}; this is likely an off-by-one error"
.format(self.reset, orig_shape),
category=SyntaxWarning,
stacklevel=2)

self.attrs = OrderedDict(() if attrs is None else attrs)

if decoder is None and isinstance(orig_shape, type) and issubclass(orig_shape, Enum):
Expand Down
4 changes: 2 additions & 2 deletions docs/lang.rst
Expand Up @@ -163,7 +163,7 @@ Specifying a shape with a range is convenient for counters, indexes, and all oth

.. _lang-exclrange:

.. warning::
.. note::

Python ranges are *exclusive* or *half-open*, meaning they do not contain their ``.stop`` element. Because of this, values with shapes cast from a ``range(stop)`` where ``stop`` is a power of 2 are not wide enough to represent ``stop`` itself:

Expand All @@ -175,7 +175,7 @@ Specifying a shape with a range is convenient for counters, indexes, and all oth
>>> fencepost.value
0

Be mindful of this edge case!
Amaranth detects uses of :class:`Const` and :class:`Signal` that invoke such an off-by-one error, and emits a diagnostic message.


.. _lang-shapeenum:
Expand Down
18 changes: 16 additions & 2 deletions tests/test_hdl_ast.py
Expand Up @@ -357,6 +357,12 @@ def test_shape_wrong(self):
r"^Width must be a non-negative integer, not -1$"):
Const(1, -1)

def test_wrong_fencepost(self):
with self.assertWarnsRegex(SyntaxWarning,
r"^Value 10 equals the non-inclusive end of the constant shape "
r"range\(0, 10\); this is likely an off-by-one error$"):
Const(10, range(10))

def test_normalization(self):
self.assertEqual(Const(0b10110, signed(5)).value, -10)

Expand Down Expand Up @@ -961,8 +967,10 @@ def test_shape(self):
self.assertEqual(s8.shape(), signed(5))
s9 = Signal(range(-20, 16))
self.assertEqual(s9.shape(), signed(6))
s10 = Signal(range(0))
self.assertEqual(s10.shape(), unsigned(0))
with warnings.catch_warnings():
warnings.filterwarnings(action="ignore", category=SyntaxWarning)
s10 = Signal(range(0))
self.assertEqual(s10.shape(), unsigned(0))
s11 = Signal(range(1))
self.assertEqual(s11.shape(), unsigned(1))

Expand Down Expand Up @@ -1006,6 +1014,12 @@ def test_reset_wrong_too_wide(self):
r"^Reset value -2 will be truncated to the signal shape signed\(1\)$"):
Signal(signed(1), reset=-2)

def test_reset_wrong_fencepost(self):
with self.assertWarnsRegex(SyntaxWarning,
r"^Reset value 10 equals the non-inclusive end of the signal shape "
r"range\(0, 10\); this is likely an off-by-one error$"):
Signal(range(0, 10), reset=10)

def test_attrs(self):
s1 = Signal()
self.assertEqual(s1.attrs, {})
Expand Down

0 comments on commit 80343d1

Please sign in to comment.