Repository: amaranth-lang/amaranth (python)

Issue:
Deprecate and remove `ValueCastable.lowermethod`
This isn't actually serving any useful purpose.

- Deprecation PR: N/A
- Removal PR: N/A (for milestone 0.6)

Pull request:
hdl._ast: deprecate `ValueCastable.lowermethod`.
Fixes #1114.

Patch:
diff --git a/amaranth/hdl/_ast.py b/amaranth/hdl/_ast.py
index 6355654..e19a280 100644
--- a/amaranth/hdl/_ast.py
+++ b/amaranth/hdl/_ast.py
@@ -483,7 +483,14 @@ class Value(metaclass=ABCMeta):
         Returns
         -------
         :class:`Value`, :pc:`signed(len(self))`, :ref:`assignable <lang-assignable>`
+
+        Raises
+        ------
+        ValueError
+            If :pc:`len(self) == 0`.
         """
+        if len(self) == 0:
+            raise ValueError("Cannot create a 0-width signed value")
         return Operator("s", [self])
 
     def __bool__(self):
@@ -900,9 +907,9 @@ class Value(metaclass=ABCMeta):
 
         Returns
         -------
-        :class:`Value`, :pc:`unsigned(len(self) + amount)`
+        :class:`Value`, :pc:`unsigned(max(len(self) + amount, 0))`
             If :pc:`self` is unsigned.
-        :class:`Value`, :pc:`signed(len(self) + amount)`
+        :class:`Value`, :pc:`signed(max(len(self) + amount, 1))`
             If :pc:`self` is signed.
         """
         if not isinstance(amount, int):
@@ -974,6 +981,8 @@ class Value(metaclass=ABCMeta):
         if amount < 0:
             return self.shift_left(-amount)
         if self.shape().signed:
+            if amount >= len(self):
+                amount = len(self) - 1
             return self[amount:].as_signed()
         else:
             return self[amount:] # unsigned
@@ -1253,9 +1262,6 @@ class ValueCastable:
         if cls.shape is ValueCastable.shape:
             raise TypeError(f"Class '{cls.__name__}' deriving from 'ValueCastable' must override "
                             "the 'shape' method")
-        if not hasattr(cls.as_value, "_ValueCastable__memoized"):
-            raise TypeError(f"Class '{cls.__name__}' deriving from 'ValueCastable' must decorate "
-                            "the 'as_value' method with the 'ValueCastable.lowermethod' decorator")
 
     # The signatures and definitions of these methods are weird because they are present here for
     # documentation (and error checking above) purpose only and should not affect control flow.
@@ -1324,17 +1330,10 @@ class ValueCastable:
         """
         return super().shape(*args, **kwargs) # :nocov:
 
+    # TODO(amaranth-0.6): remove
     @staticmethod
+    @deprecated("`ValueCastable.lowermethod` is no longer required and will be removed in Amaranth 0.6")
     def lowermethod(func):
-        """Decorator to memoize lowering methods.
-
-        Ensures the decorated method is called only once, with subsequent method calls returning
-        the object returned by the first first method call.
-
-        This decorator is required to decorate the :pc:`as_value` method of :pc:`ValueCastable`
-        subclasses. This is to ensure that Amaranth's view of representation of all values stays
-        internally consistent.
-        """
         @functools.wraps(func)
         def wrapper_memoized(self, *args, **kwargs):
             # Use `in self.__dict__` instead of `hasattr` to avoid interfering with custom
diff --git a/amaranth/hdl/_rec.py b/amaranth/hdl/_rec.py
index a02b69b..f033fad 100644
--- a/amaranth/hdl/_rec.py
+++ b/amaranth/hdl/_rec.py
@@ -178,7 +178,6 @@ class Record(ValueCastable):
                 raise AttributeError("{} does not have a field '{}'. Did you mean one of: {}?"
                                      .format(reference, item, ", ".join(self.fields))) from None
 
-    @ValueCastable.lowermethod
     def as_value(self):
         return Cat(self.fields.values())
 
diff --git a/amaranth/lib/data.py b/amaranth/lib/data.py
index 1083fa3..978f83e 100644
--- a/amaranth/lib/data.py
+++ b/amaranth/lib/data.py
@@ -638,7 +638,6 @@ class View(ValueCastable):
         """
         return self.__orig_layout
 
-    @ValueCastable.lowermethod
     def as_value(self):
         """Get underlying value.
 
diff --git a/amaranth/lib/enum.py b/amaranth/lib/enum.py
index 998d54e..84b922d 100644
--- a/amaranth/lib/enum.py
+++ b/amaranth/lib/enum.py
@@ -232,7 +232,6 @@ class EnumView(ValueCastable):
         """Returns the underlying enum type."""
         return self.enum
 
-    @ValueCastable.lowermethod
     def as_value(self):
         """Returns the underlying value."""
         return self.target


Test patch:
diff --git a/tests/test_hdl_ast.py b/tests/test_hdl_ast.py
index 83bbb77..4b14f6b 100644
--- a/tests/test_hdl_ast.py
+++ b/tests/test_hdl_ast.py
@@ -343,7 +343,7 @@ class ValueTestCase(FHDLTestCase):
         self.assertRepr(Const(256, signed(9)).shift_left(-5),
                         "(s (slice (const 9'sd-256) 5:9))")
         self.assertRepr(Const(256, signed(9)).shift_left(-15),
-                        "(s (slice (const 9'sd-256) 9:9))")
+                        "(s (slice (const 9'sd-256) 8:9))")
 
     def test_shift_left_wrong(self):
         with self.assertRaisesRegex(TypeError,
@@ -367,12 +367,20 @@ class ValueTestCase(FHDLTestCase):
                         "(slice (const 9'd256) 1:9)")
         self.assertRepr(Const(256, unsigned(9)).shift_right(5),
                         "(slice (const 9'd256) 5:9)")
+        self.assertRepr(Const(256, unsigned(9)).shift_right(15),
+                        "(slice (const 9'd256) 9:9)")
         self.assertRepr(Const(256, signed(9)).shift_right(1),
                         "(s (slice (const 9'sd-256) 1:9))")
         self.assertRepr(Const(256, signed(9)).shift_right(5),
                         "(s (slice (const 9'sd-256) 5:9))")
+        self.assertRepr(Const(256, signed(9)).shift_right(7),
+                        "(s (slice (const 9'sd-256) 7:9))")
+        self.assertRepr(Const(256, signed(9)).shift_right(8),
+                        "(s (slice (const 9'sd-256) 8:9))")
+        self.assertRepr(Const(256, signed(9)).shift_right(9),
+                        "(s (slice (const 9'sd-256) 8:9))")
         self.assertRepr(Const(256, signed(9)).shift_right(15),
-                        "(s (slice (const 9'sd-256) 9:9))")
+                        "(s (slice (const 9'sd-256) 8:9))")
 
     def test_shift_right_wrong(self):
         with self.assertRaisesRegex(TypeError,
@@ -516,6 +524,11 @@ class OperatorTestCase(FHDLTestCase):
         self.assertEqual(repr(v), "(s (const 4'd1))")
         self.assertEqual(v.shape(), signed(4))
 
+    def test_as_signed_wrong(self):
+        with self.assertRaisesRegex(ValueError,
+                r"^Cannot create a 0-width signed value$"):
+            Const(0, 0).as_signed()
+
     def test_pos(self):
         self.assertRepr(+Const(10), "(const 4'd10)")
 
@@ -1027,7 +1040,6 @@ class ArrayTestCase(FHDLTestCase):
 
     def test_index_value_castable(self):
         class MyValue(ValueCastable):
-            @ValueCastable.lowermethod
             def as_value(self):
                 return Signal()
 
@@ -1315,7 +1327,6 @@ class MockValueCastable(ValueCastable):
     def shape(self):
         return Value.cast(self.dest).shape()
 
-    @ValueCastable.lowermethod
     def as_value(self):
         return self.dest
 
@@ -1327,9 +1338,10 @@ class MockValueCastableChanges(ValueCastable):
     def shape(self):
         return unsigned(self.width)
 
-    @ValueCastable.lowermethod
-    def as_value(self):
-        return Signal(self.width)
+    with _ignore_deprecated():
+        @ValueCastable.lowermethod
+        def as_value(self):
+            return Signal(self.width)
 
 
 class MockValueCastableCustomGetattr(ValueCastable):
@@ -1339,29 +1351,16 @@ class MockValueCastableCustomGetattr(ValueCastable):
     def shape(self):
         assert False
 
-    @ValueCastable.lowermethod
-    def as_value(self):
-        return Const(0)
+    with _ignore_deprecated():
+        @ValueCastable.lowermethod
+        def as_value(self):
+            return Const(0)
 
     def __getattr__(self, attr):
         assert False
 
 
 class ValueCastableTestCase(FHDLTestCase):
-    def test_not_decorated(self):
-        with self.assertRaisesRegex(TypeError,
-                r"^Class 'MockValueCastableNotDecorated' deriving from 'ValueCastable' must "
-                r"decorate the 'as_value' method with the 'ValueCastable.lowermethod' decorator$"):
-            class MockValueCastableNotDecorated(ValueCastable):
-                def __init__(self):
-                    pass
-
-                def shape(self):
-                    pass
-
-                def as_value(self):
-                    return Signal()
-
     def test_no_override(self):
         with self.assertRaisesRegex(TypeError,
                 r"^Class 'MockValueCastableNoOverrideAsValue' deriving from 'ValueCastable' must "
diff --git a/tests/test_lib_wiring.py b/tests/test_lib_wiring.py
index 98f48c8..1682f78 100644
--- a/tests/test_lib_wiring.py
+++ b/tests/test_lib_wiring.py
@@ -4,7 +4,6 @@ import unittest
 from types import SimpleNamespace as NS
 
 from amaranth import *
-from amaranth.hdl._ast import ValueCastable
 from amaranth.lib import data, enum
 from amaranth.lib.wiring import Flow, In, Out, Member
 from amaranth.lib.wiring import SignatureError, SignatureMembers, FlippedSignatureMembers
diff --git a/tests/test_sim.py b/tests/test_sim.py
index fd90e8d..01e55d3 100644
--- a/tests/test_sim.py
+++ b/tests/test_sim.py
@@ -728,7 +728,6 @@ class SimulatorIntegrationTestCase(FHDLTestCase):
 
     def test_value_castable(self):
         class MyValue(ValueCastable):
-            @ValueCastable.lowermethod
             def as_value(self):
                 return Signal()