Repository: amaranth-lang/amaranth (python)

Issue:
Unfriendly "recursion depth exceeded" error message when forgetting `In` and `Out` on a `Component`'s signature
Consider the following Amaranth code, specifically the `rand` and `seq` lines in the `PageBuffer` `Component`:

```
from amaranth import Signal, Module
from amaranth.lib.wiring import Signature, In, Out, Component


class SeqSignature(Signature):
    def __init__(self):
        return super().__init__({
            "data": In(8),
            "addr": Out(10),
            "ready": Out(1),
            "valid": In(1)
        })


class RandSignature(Signature):
    def __init__(self):
        return super().__init__({
            "data": In(8),
            "addr": Out(14),
            "read_en": Out(1),
            "flush": Out(1),
            "valid": In(1)
        })


class PageBuffer(Component):
    rand: RandSignature().flip()
    seq: SeqSignature()

    def elaborate(self, plat):
        m = Module()

        return m


if __name__ == "__main__":
    m = PageBuffer()
```

On both Linux and Windows, the above snippet results in a recursion error along the lines of:

```
PS C:\msys64\home\William\Projects\FPGA\amaranth\efbutils\ufm_reader> python .\ufm_reader\page_buffer.py
Traceback (most recent call last):
  File "C:\msys64\home\William\Projects\FPGA\amaranth\efbutils\ufm_reader\ufm_reader\page_buffer.py", line 37, in <module>
    m = PageBuffer()
        ^^^^^^^^^^^^
  File "C:\msys64\home\William\Projects\FPGA\amaranth\amaranth\build\__editable__.amaranth-0.4.dev173+gf95fe45-py3-none-any\amaranth\lib\wiring.py", line 744, in __init__
    for name in self.signature.members:
                ^^^^^^^^^^^^^^
  File "C:\msys64\home\William\Projects\FPGA\amaranth\amaranth\build\__editable__.amaranth-0.4.dev173+gf95fe45-py3-none-any\amaranth\lib\wiring.py", line 761, in signature
    if (annot in (Value, Signal, Const) or
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\msys64\home\William\Projects\FPGA\amaranth\amaranth\build\__editable__.amaranth-0.4.dev173+gf95fe45-py3-none-any\amaranth\lib\wiring.py", line 489, in __eq__
    return other == self
           ^^^^^^^^^^^^^
  File "C:\msys64\home\William\Projects\FPGA\amaranth\amaranth\build\__editable__.amaranth-0.4.dev173+gf95fe45-py3-none-any\amaranth\lib\wiring.py", line 489, in __eq__
    return other == self
           ^^^^^^^^^^^^^
  File "C:\msys64\home\William\Projects\FPGA\amaranth\amaranth\build\__editable__.amaranth-0.4.dev173+gf95fe45-py3-none-any\amaranth\lib\wiring.py", line 489, in __eq__
    return other == self
           ^^^^^^^^^^^^^
  [Previous line repeated 495 more times]
RecursionError: maximum recursion depth exceeded in comparison
```

If I put `In()` and `Out` around the signature annotations, the error goes away:

```
class PageBuffer(Component):
    rand: In(RandSignature().flip())
    seq: Out(SeqSignature())
```

A more friendly error message along the lines of "you forgot `In` and `Out`" would be helpful to avoid debugging a misleading stack overflow error. On the other hand, RFC 2 [contains](https://amaranth-lang.org/rfcs/0002-interfaces.html#forwarding-interfaces) code examples where `In` and `Out` are omitted from the signature annotation. So it's not clear to me whether the initial code snippet should be erroring out or not.

Pull request description:
lib.wiring: fix equality of `FlippedSignature` with other object
Fixes #882.

Patch:
diff --git a/amaranth/lib/wiring.py b/amaranth/lib/wiring.py
index 182bb06..cc1977b 100644
--- a/amaranth/lib/wiring.py
+++ b/amaranth/lib/wiring.py
@@ -484,9 +484,11 @@ class FlippedSignature:
             return self.flip() == other.flip()
         else:
             # Delegate comparisons back to Signature (or its descendant) by flipping the arguments;
-            # equality must be reflexive but the implementation of __eq__ need not be, and we can
-            # take advantage of it here.
-            return other == self
+            # equality must be reflexive but the implementation of `__eq__` need not be, and we can
+            # take advantage of it here. This is done by returning `NotImplemented`, otherwise if
+            # the other object cannot be compared to a `FlippedSignature` either this will result
+            # in infinite recursion.
+            return NotImplemented
 
     # These methods do not access instance variables and so their implementation can be shared
     # between the normal and the flipped member collections.
@@ -758,7 +760,7 @@ class Component(Elaboratable):
             for name, annot in getattr(base, "__annotations__", {}).items():
                 if name.startswith("_"):
                     continue
-                if (annot in (Value, Signal, Const) or
+                if (annot is Value or annot is Signal or annot is Const or
                         (isinstance(annot, type) and issubclass(annot, ValueCastable)) or
                         isinstance(annot, Signature)):
                     if isinstance(annot, type):


Test patch:
diff --git a/tests/test_lib_wiring.py b/tests/test_lib_wiring.py
index b1bf74e..06e9bab 100644
--- a/tests/test_lib_wiring.py
+++ b/tests/test_lib_wiring.py
@@ -832,3 +832,15 @@ class ComponentTestCase(unittest.TestCase):
                 r"a signature member; did you mean 'val2: In\(MockValueCastable\)' or "
                 r"'val2: Out\(MockValueCastable\)'\?$"):
             C3().signature
+
+    def test_bug_882(self):
+        class PageBuffer(Component):
+            rand: Signature({}).flip()
+            other: Out(1)
+
+        with self.assertWarnsRegex(SyntaxWarning,
+                r"^Component '.+\.PageBuffer' has an annotation 'rand: Signature\({}\)\.flip\(\)', "
+                r"which is not a signature member; did you mean "
+                r"'rand: In\(Signature\({}\)\.flip\(\)\)' or "
+                r"'rand: Out\(Signature\({}\)\.flip\(\)\)'\?$"):
+            PageBuffer()