Repository: amaranth-lang/amaranth
Pull request #739 (2023-01-21 14:04:00)

hdl.ast: allow IntEnum/IntFlag in Cat.
Example code that can make use of such changes:

```py
class Something(IntEnum):
    A = 0b1000
    B = 0b0000

...

class Component(Elaboratable):
    def elaborate(self, platform):
        m = Module()
        with m.Switch(...):
             with m.Case(Cat(Something.A, Something.B)):
                 ...
```

The first commit is more important to me than the second one (you can always work it around using `Case(Cat(...)._as_const())`), but they are pretty much independent, so if it is desirable to only merge one of them, you can do it.

codecov-commenter: # [Codecov](https://codecov.io/gh/amaranth-lang/amaranth/pull/739?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=amaranth-lang) Report
> Merging [#739](https://codecov.io/gh/amaranth-lang/amaranth/pull/739?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=amaranth-lang) (ada976d) into [main](https://codecov.io/gh/amaranth-lang/amaranth/commit/91d45136828c55c2c035cd12ea41dd41ce1c1669?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=amaranth-lang) (91d4513) will **decrease** coverage by `0.09%`.
> The diff coverage is `55.55%`.

```diff
@@            Coverage Diff             @@
##             main     #739      +/-   ##
==========================================
- Coverage   81.10%   81.02%   -0.09%     
==========================================
  Files          49       49              
  Lines        6543     6547       +4     
  Branches     1571     1573       +2     
==========================================
- Hits         5307     5305       -2     
- Misses       1041     1043       +2     
- Partials      195      199       +4     
```


| [Impacted Files](https://codecov.io/gh/amaranth-lang/amaranth/pull/739?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=amaranth-lang) | Coverage Δ | |
|---|---|---|
| [amaranth/hdl/dsl.py](https://codecov.io/gh/amaranth-lang/amaranth/pull/739?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=amaranth-lang#diff-YW1hcmFudGgvaGRsL2RzbC5weQ==) | `99.20% <0.00%> (-0.54%)` | :arrow_down: |
| [amaranth/hdl/ast.py](https://codecov.io/gh/amaranth-lang/amaranth/pull/739?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=amaranth-lang#diff-YW1hcmFudGgvaGRsL2FzdC5weQ==) | `89.40% <71.42%> (-0.19%)` | :arrow_down: |
| [amaranth/\_toolchain/cxx.py](https://codecov.io/gh/amaranth-lang/amaranth/pull/739?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=amaranth-lang#diff-YW1hcmFudGgvX3Rvb2xjaGFpbi9jeHgucHk=) | `95.23% <0.00%> (-4.77%)` | :arrow_down: |
| [amaranth/build/run.py](https://codecov.io/gh/amaranth-lang/amaranth/pull/739?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=amaranth-lang#diff-YW1hcmFudGgvYnVpbGQvcnVuLnB5) | `21.73% <0.00%> (ø)` | |

:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=amaranth-lang)

whitequark: Please split this PR into two and add tests.
whitequark: So there are actually two distinct changes in this PR that I'd like to have addressed separately:
  1. Fixing a bug in value casting code that would treat `IntEnum` values as integers and not enumerations. This is currently unspecified, but since the [documentation](https://amaranth-lang.org/docs/amaranth/latest/lang.html#shapes-from-enumerations) lists that the Enum behavior specializes the `int` behavior (widens the result), I think they should be indeed treated as enumerations.
  2. Changing the behavior of the diagnostic that prints a warning if a bare integer is used in a `Cat` context. This is actually still a problem with enums because it's not possible to specify width of an enumeration explicitly. That's tracked in #730 though.

In addition you only have tests for `IntEnum` while the change is applied to `Enum` too.

Could you please split this into two commits? One introducing the change (1), adding a test for bare `Enum`, and adding a line into both of the sections of documentation [here](https://amaranth-lang.org/docs/amaranth/latest/lang.html#value-casting) to mention that if a value is both `int` and `Enum`, `Enum` behavior takes preference. Another introducing the change (2) silencing the warning for `IntEnum`, the way it's already silenced for `Enum`.
Arusekk: ad 1: Sure.
ad 2: ...and you got me on this one.  The whole PR is mainly because I managed to make a proof-of-concept implementation of an enum with width; would you like me to add it in a next PR?
```py
class BitEnum(ValueCastable, Enum):
    """
    A helper class that defines Amaranth enums with fixed width
    """
    def __init_subclass__(cls, *, width):
        cls.__width = width

    @ValueCastable.lowermethod
    def as_value(self):
        return Const(self.value, self.__width)

class Color(BitEnum, width=5):
    RED  = 0b00001
    BLUE = 0b00010
```
Another option would be to insert such logic to Const and define enums as `class Color(Const, Enum, width=5):`, possibly with an extra check that the subclass is a subclass of Enum, to still prevent reckless Const subclassing.

(actually the above does not work due to Python incorrectly assuming that `ValueCastable` is a data type, not a mix-in class, because `ValueCastable` defines a `__new__` method—maybe the checks could be moved to `Value.cast`? the below does work, however)
<details>

```py
class ValueCastableHack(int, ValueCastable):
    @ValueCastable.lowermethod
    def as_value(self):
        raise NotImplementedError("width information lost!")


class BitEnum(ValueCastableHack, Enum):
    """
    A helper class that defines Amaranth enums with fixed width
    """
    def __init_subclass__(cls, *, width):
        cls.__width = width

    @ValueCastable.lowermethod
    def as_value(self):
        return Const(self.value, self.__width)

class Color(BitEnum, width=5):
    RED  = 0b00001
    BLUE = 0b00010
```

</details>
whitequark: > would you like me to add it in a next PR?

I think such a change needs an RFC first. Please do write one (#693 can be used as an example re: format) and submit it together with your implementation as the proposed one, especially if you do already have it.

We don't have to wait until the RFC is merged to merge these changes though.

Patch:
diff --git a/amaranth/hdl/ast.py b/amaranth/hdl/ast.py
index 4dcf4c0..586fbb3 100644
--- a/amaranth/hdl/ast.py
+++ b/amaranth/hdl/ast.py
@@ -168,12 +168,12 @@ class Value(metaclass=ABCMeta):
         while True:
             if isinstance(obj, Value):
                 return obj
-            elif isinstance(obj, int):
-                return Const(obj)
-            elif isinstance(obj, Enum):
-                return Const(obj.value, Shape.cast(type(obj)))
             elif isinstance(obj, ValueCastable):
                 new_obj = obj.as_value()
+            elif isinstance(obj, Enum):
+                return Const(obj.value, Shape.cast(type(obj)))
+            elif isinstance(obj, int):
+                return Const(obj)
             else:
                 raise TypeError("Object {!r} cannot be converted to an Amaranth value".format(obj))
             if new_obj is obj:
@@ -860,7 +860,7 @@ class Cat(Value):
         super().__init__(src_loc_at=src_loc_at)
         self.parts = []
         for index, arg in enumerate(flatten(args)):
-            if isinstance(arg, int) and arg not in [0, 1]:
+            if isinstance(arg, int) and not isinstance(arg, Enum) and arg not in [0, 1]:
                 warnings.warn("Argument #{} of Cat() is a bare integer {} used in bit vector "
                               "context; consider specifying explicit width using C({}, {}) instead"
                               .format(index + 1, arg, arg, bits_for(arg)),
diff --git a/docs/lang.rst b/docs/lang.rst
index 0c2a47e..06c0c46 100644
--- a/docs/lang.rst
+++ b/docs/lang.rst
@@ -235,6 +235,9 @@ Casting a value from an integer ``i`` is a shorthand for ``Const(i)``:
    >>> Value.cast(5)
    (const 3'd5)
 
+.. note::
+
+   If a value subclasses :class:`enum.IntEnum` or its class otherwise inherits from both :class:`int` and :class:`Enum`, it is treated as an enumeration.
 
 Values from enumeration members
 -------------------------------
@@ -247,6 +250,10 @@ Casting a value from an enumeration member ``m`` is a shorthand for ``Const(m.va
    (const 2'd1)
 
 
+.. note::
+
+   If a value subclasses :class:`enum.IntEnum` or its class otherwise inherits from both :class:`int` and :class:`Enum`, it is treated as an enumeration.
+
 .. _lang-signals:
 
 Signals


Test patch:
diff --git a/tests/test_hdl_ast.py b/tests/test_hdl_ast.py
index 55e5a88..b0cd9b2 100644
--- a/tests/test_hdl_ast.py
+++ b/tests/test_hdl_ast.py
@@ -23,6 +23,12 @@ class StringEnum(Enum):
     BAR = "b"
 
 
+class TypedEnum(int, Enum):
+    FOO = 1
+    BAR = 2
+    BAZ = 3
+
+
 class ShapeTestCase(FHDLTestCase):
     def test_make(self):
         s1 = Shape()
@@ -199,6 +205,11 @@ class ValueTestCase(FHDLTestCase):
         self.assertIsInstance(e2, Const)
         self.assertEqual(e2.shape(), signed(2))
 
+    def test_cast_typedenum(self):
+        e1 = Value.cast(TypedEnum.FOO)
+        self.assertIsInstance(e1, Const)
+        self.assertEqual(e1.shape(), unsigned(2))
+
     def test_cast_enum_wrong(self):
         with self.assertRaisesRegex(TypeError,
                 r"^Only enumerations with integer values can be used as value shapes$"):
@@ -781,6 +792,24 @@ class CatTestCase(FHDLTestCase):
             warnings.filterwarnings(action="error", category=SyntaxWarning)
             Cat(0, 1, 1, 0)
 
+    def test_enum(self):
+        class Color(Enum):
+            RED  = 1
+            BLUE = 2
+        with warnings.catch_warnings():
+            warnings.filterwarnings(action="error", category=SyntaxWarning)
+            c = Cat(Color.RED, Color.BLUE)
+        self.assertEqual(repr(c), "(cat (const 2'd1) (const 2'd2))")
+
+    def test_intenum(self):
+        class Color(int, Enum):
+            RED  = 1
+            BLUE = 2
+        with warnings.catch_warnings():
+            warnings.filterwarnings(action="error", category=SyntaxWarning)
+            c = Cat(Color.RED, Color.BLUE)
+        self.assertEqual(repr(c), "(cat (const 2'd1) (const 2'd2))")
+
     def test_int_wrong(self):
         with self.assertWarnsRegex(SyntaxWarning,
                 r"^Argument #1 of Cat\(\) is a bare integer 2 used in bit vector context; "