Repository: amaranth-lang/amaranth (python)

Issue:
Constructing a `signed(0)`-shaped Const fails with `ValueError`.
While a signed shape with width 0 is allowed by the model, it has unclear semantics, and attempting to actually use it results in errors:

```
>>> Const(0, signed(0))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/mwk/amaranth/amaranth/hdl/ast.py", line 650, in __init__
    if self.signed and self.value >> (self.width - 1) & 1:
                       ~~~~~~~~~~~^^~~~~~~~~~~~~~~~~~
ValueError: negative shift count
```

Pull request description:
hdl: disallow `signed(0)` values with unclear semantics
Fixes #807.

Patch:
diff --git a/amaranth/hdl/ast.py b/amaranth/hdl/ast.py
index e1ee694..ed93347 100644
--- a/amaranth/hdl/ast.py
+++ b/amaranth/hdl/ast.py
@@ -78,11 +78,15 @@ class Shape:
         If ``False``, the value is unsigned. If ``True``, the value is signed two's complement.
     """
     def __init__(self, width=1, signed=False):
-        if not isinstance(width, int) or width < 0:
-            raise TypeError("Width must be a non-negative integer, not {!r}"
-                            .format(width))
+        if not isinstance(width, int):
+            raise TypeError(f"Width must be an integer, not {width!r}")
+        if not signed and width < 0:
+            raise TypeError(f"Width of an unsigned value must be zero or a positive integer, "
+                            f"not {width}")
+        if signed and width <= 0:
+            raise TypeError(f"Width of a signed value must be a positive integer, not {width}")
         self.width = width
-        self.signed = signed
+        self.signed = bool(signed)
 
     # The algorithm for inferring shape for standard Python enumerations is factored out so that
     # `Shape.cast()` and Amaranth's `EnumMeta.as_shape()` can both use it.
@@ -116,7 +120,7 @@ class Shape:
                 return Shape(obj)
             elif isinstance(obj, range):
                 if len(obj) == 0:
-                    return Shape(0, obj.start < 0)
+                    return Shape(0)
                 signed = obj[0] < 0 or obj[-1] < 0
                 width  = max(bits_for(obj[0], signed),
                              bits_for(obj[-1], signed))
@@ -860,9 +864,14 @@ class Part(Value):
         if not isinstance(stride, int) or stride <= 0:
             raise TypeError("Part stride must be a positive integer, not {!r}".format(stride))
 
+        value = Value.cast(value)
+        offset = Value.cast(offset)
+        if offset.shape().signed:
+            raise TypeError("Part offset must be unsigned")
+
         super().__init__(src_loc_at=src_loc_at)
-        self.value  = Value.cast(value)
-        self.offset = Value.cast(offset)
+        self.value  = value
+        self.offset = offset
         self.width  = width
         self.stride = stride
 
diff --git a/docs/lang.rst b/docs/lang.rst
index 6a1c110..9a87443 100644
--- a/docs/lang.rst
+++ b/docs/lang.rst
@@ -153,8 +153,8 @@ Shapes from ranges
 
 Casting a shape from a :class:`range` ``r`` produces a shape that:
 
-  * has a width large enough to represent both ``min(r)`` and ``max(r)``, and
-  * is signed if either ``min(r)`` or ``max(r)`` are negative, unsigned otherwise.
+  * has a width large enough to represent both ``min(r)`` and ``max(r)``, but not larger, and
+  * is signed if ``r`` contains any negative values, unsigned otherwise.
 
 Specifying a shape with a range is convenient for counters, indexes, and all other values whose width is derived from a set of numbers they must be able to fit:
 
@@ -184,6 +184,16 @@ Specifying a shape with a range is convenient for counters, indexes, and all oth
 
    Amaranth detects uses of :class:`Const` and :class:`Signal` that invoke such an off-by-one error, and emits a diagnostic message.
 
+.. note::
+
+   An empty range always casts to an ``unsigned(0)``, even if both of its bounds are negative.
+   This happens because, being empty, it does not contain any negative values.
+
+   .. doctest::
+
+      >>> Shape.cast(range(-1, -1))
+      unsigned(0)
+
 
 .. _lang-shapeenum:
 


Test patch:
diff --git a/tests/test_hdl_ast.py b/tests/test_hdl_ast.py
index 0cb8865..8745a53 100644
--- a/tests/test_hdl_ast.py
+++ b/tests/test_hdl_ast.py
@@ -42,11 +42,20 @@ class ShapeTestCase(FHDLTestCase):
         s3 = Shape(3, True)
         self.assertEqual(s3.width, 3)
         self.assertEqual(s3.signed, True)
+        s4 = Shape(0)
+        self.assertEqual(s4.width, 0)
+        self.assertEqual(s4.signed, False)
 
     def test_make_wrong(self):
         with self.assertRaisesRegex(TypeError,
-                r"^Width must be a non-negative integer, not -1$"):
-            Shape(-1)
+                r"^Width must be an integer, not 'a'$"):
+            Shape("a")
+        with self.assertRaisesRegex(TypeError,
+                r"^Width of an unsigned value must be zero or a positive integer, not -1$"):
+            Shape(-1, signed=False)
+        with self.assertRaisesRegex(TypeError,
+                r"^Width of a signed value must be a positive integer, not 0$"):
+            Shape(0, signed=True)
 
     def test_compare_non_shape(self):
         self.assertNotEqual(Shape(1, True), "hi")
@@ -87,7 +96,7 @@ class ShapeTestCase(FHDLTestCase):
 
     def test_cast_int_wrong(self):
         with self.assertRaisesRegex(TypeError,
-                r"^Width must be a non-negative integer, not -1$"):
+                r"^Width of an unsigned value must be zero or a positive integer, not -1$"):
             Shape.cast(-1)
 
     def test_cast_tuple_wrong(self):
@@ -116,7 +125,7 @@ class ShapeTestCase(FHDLTestCase):
         self.assertEqual(s6.signed, False)
         s7 = Shape.cast(range(-1, -1))
         self.assertEqual(s7.width, 0)
-        self.assertEqual(s7.signed, True)
+        self.assertEqual(s7.signed, False)
         s8 = Shape.cast(range(0, 10, 3))
         self.assertEqual(s8.width, 4)
         self.assertEqual(s8.signed, False)
@@ -386,7 +395,7 @@ class ConstTestCase(FHDLTestCase):
 
     def test_shape_wrong(self):
         with self.assertRaisesRegex(TypeError,
-                r"^Width must be a non-negative integer, not -1$"):
+                r"^Width of an unsigned value must be zero or a positive integer, not -1$"):
             Const(1, -1)
 
     def test_wrong_fencepost(self):
@@ -785,6 +794,11 @@ class BitSelectTestCase(FHDLTestCase):
         s = self.c.bit_select(self.s, 2)
         self.assertEqual(repr(s), "(part (const 8'd0) (sig s) 2 1)")
 
+    def test_offset_wrong(self):
+        with self.assertRaisesRegex(TypeError,
+                r"^Part offset must be unsigned$"):
+            self.c.bit_select(self.s.as_signed(), 1)
+
 
 class WordSelectTestCase(FHDLTestCase):
     def setUp(self):
@@ -817,6 +831,11 @@ class WordSelectTestCase(FHDLTestCase):
         s = self.c.word_select(self.s, 2)
         self.assertEqual(repr(s), "(part (const 8'd0) (sig s) 2 2)")
 
+    def test_offset_wrong(self):
+        with self.assertRaisesRegex(TypeError,
+                r"^Part offset must be unsigned$"):
+            self.c.word_select(self.s.as_signed(), 1)
+
 
 class CatTestCase(FHDLTestCase):
     def test_shape(self):
@@ -1012,7 +1031,7 @@ class SignalTestCase(FHDLTestCase):
 
     def test_shape_wrong(self):
         with self.assertRaisesRegex(TypeError,
-                r"^Width must be a non-negative integer, not -10$"):
+                r"^Width of an unsigned value must be zero or a positive integer, not -10$"):
             Signal(-10)
 
     def test_name(self):