Repository: amaranth-lang/amaranth (python)

Issue:
Subtract of unsigned `Value`s should have signed shape
```
>>> a = Const(1) - Const(2)                                                                                                                                                                     
>>> a.shape()
unsigned(3)
```

The correct result here (`-1`) is obviously not representable in an unsigned shape.

Pull request:
hdl.ast: fix shape for subtraction.
Fixes #813.

Patch:
diff --git a/amaranth/hdl/ast.py b/amaranth/hdl/ast.py
index cbf2e60..da55380 100644
--- a/amaranth/hdl/ast.py
+++ b/amaranth/hdl/ast.py
@@ -117,9 +117,9 @@ class Shape:
             elif isinstance(obj, range):
                 if len(obj) == 0:
                     return Shape(0, obj.start < 0)
-                signed = obj.start < 0 or (obj.stop - obj.step) < 0
-                width  = max(bits_for(obj.start, signed),
-                             bits_for(obj.stop - obj.step, signed))
+                signed = obj[0] < 0 or obj[-1] < 0
+                width  = max(bits_for(obj[0], signed),
+                             bits_for(obj[-1], signed))
                 return Shape(width, signed)
             elif isinstance(obj, type) and issubclass(obj, Enum):
                 # For compatibility with third party enumerations, handle them as if they were
@@ -730,9 +730,12 @@ class Operator(Value):
                 return Shape(a_shape.width, True)
         elif len(op_shapes) == 2:
             a_shape, b_shape = op_shapes
-            if self.operator in ("+", "-"):
+            if self.operator == "+":
                 o_shape = _bitwise_binary_shape(*op_shapes)
                 return Shape(o_shape.width + 1, o_shape.signed)
+            if self.operator == "-":
+                o_shape = _bitwise_binary_shape(*op_shapes)
+                return Shape(o_shape.width + 1, True)
             if self.operator == "*":
                 return Shape(a_shape.width + b_shape.width, a_shape.signed or b_shape.signed)
             if self.operator == "//":
@@ -796,11 +799,11 @@ class Slice(Value):
             raise TypeError("Slice stop must be an integer, not {!r}".format(stop))
 
         n = len(value)
-        if start not in range(-(n+1), n+1):
+        if start not in range(-n, n+1):
             raise IndexError("Cannot start slice {} bits into {}-bit value".format(start, n))
         if start < 0:
             start += n
-        if stop not in range(-(n+1), n+1):
+        if stop not in range(-n, n+1):
             raise IndexError("Cannot stop slice {} bits into {}-bit value".format(stop, n))
         if stop < 0:
             stop += n


Test patch:
diff --git a/tests/test_hdl_ast.py b/tests/test_hdl_ast.py
index 65f2404..af4b175 100644
--- a/tests/test_hdl_ast.py
+++ b/tests/test_hdl_ast.py
@@ -117,6 +117,9 @@ class ShapeTestCase(FHDLTestCase):
         s7 = Shape.cast(range(-1, -1))
         self.assertEqual(s7.width, 0)
         self.assertEqual(s7.signed, True)
+        s8 = Shape.cast(range(0, 10, 3))
+        self.assertEqual(s8.width, 4)
+        self.assertEqual(s8.signed, False)
 
     def test_cast_enum(self):
         s1 = Shape.cast(UnsignedEnum)
@@ -440,7 +443,7 @@ class OperatorTestCase(FHDLTestCase):
     def test_sub(self):
         v1 = Const(0, unsigned(4)) - Const(0, unsigned(6))
         self.assertEqual(repr(v1), "(- (const 4'd0) (const 6'd0))")
-        self.assertEqual(v1.shape(), unsigned(7))
+        self.assertEqual(v1.shape(), signed(7))
         v2 = Const(0, signed(4)) - Const(0, signed(6))
         self.assertEqual(v2.shape(), signed(7))
         v3 = Const(0, signed(4)) - Const(0, unsigned(4))
@@ -448,7 +451,9 @@ class OperatorTestCase(FHDLTestCase):
         v4 = Const(0, unsigned(4)) - Const(0, signed(4))
         self.assertEqual(v4.shape(), signed(6))
         v5 = 10 - Const(0, 4)
-        self.assertEqual(v5.shape(), unsigned(5))
+        self.assertEqual(v5.shape(), signed(5))
+        v6 = 1 - Const(2)
+        self.assertEqual(v6.shape(), signed(3))
 
     def test_mul(self):
         v1 = Const(0, unsigned(4)) * Const(0, unsigned(6))
@@ -711,6 +716,9 @@ class SliceTestCase(FHDLTestCase):
         with self.assertRaisesRegex(IndexError,
                 r"^Slice start 4 must be less than slice stop 2$"):
             Slice(c, 4, 2)
+        with self.assertRaisesRegex(IndexError,
+                r"^Cannot start slice -9 bits into 8-bit value$"):
+            Slice(c, -9, -5)
 
     def test_repr(self):
         s1 = Const(10)[2]