Repository: amaranth-lang/amaranth (python)

Issue:
Signals with private names
In some cases it is desirable to create a signal with a name that doesn't show up in the waveform viewer, such as when a signal is created to hold the results of intermediate computations, or to change shape, and it would just add noise. I propose adding a notion of a "private signal name".

I suggest changing naming rules so that `Signal(name="")` creates a signal with a private name. Such a signal is translated to a private wire in RTLIL (with a `$\d+` name), doesn't get displayed in the waveform viewer, and so on.

---

The FSM ongoing signals (not the state signal) should be changed to have private names in the 0.5 milestone.

Pull request description:
Allow Signals to be privately named using `name=""`
Closes #1223

Tested a couple toy designs. The inclusion of FSM names being private should be enough to prevent regressions.

This does remove a bit of diagnostic power from the FSM tests.

Patch:
diff --git a/amaranth/hdl/_ast.py b/amaranth/hdl/_ast.py
index 1844880..551fcc3 100644
--- a/amaranth/hdl/_ast.py
+++ b/amaranth/hdl/_ast.py
@@ -1884,7 +1884,8 @@ class Signal(Value, DUID, metaclass=_SignalMeta):
         If not specified, ``shape`` defaults to 1-bit and non-signed.
     name : str
         Name hint for this signal. If ``None`` (default) the name is inferred from the variable
-        name this ``Signal`` is assigned to.
+        name this ``Signal`` is assigned to. If the empty string, then this ``Signal`` is treated
+        as private and is generally hidden from view.
     init : int or integral Enum
         Reset (synchronous) or default (combinatorial) value.
         When this ``Signal`` is assigned to in synchronous context and the corresponding clock
@@ -1920,7 +1921,10 @@ class Signal(Value, DUID, metaclass=_SignalMeta):
 
         if name is not None and not isinstance(name, str):
             raise TypeError(f"Name must be a string, not {name!r}")
-        self.name = name or tracer.get_var_name(depth=2 + src_loc_at, default="$signal")
+        if name is None:
+            self.name = tracer.get_var_name(depth=2 + src_loc_at, default="$signal")
+        else:
+            self.name = name
 
         orig_shape = shape
         if shape is None:
@@ -2102,7 +2106,10 @@ class Signal(Value, DUID, metaclass=_SignalMeta):
         return SignalSet((self,))
 
     def __repr__(self):
-        return f"(sig {self.name})"
+        if self.name != "":
+            return f"(sig {self.name})"
+        else:
+            return "(sig)"
 
 
 @final
diff --git a/amaranth/hdl/_dsl.py b/amaranth/hdl/_dsl.py
index a74fd7c..2322a83 100644
--- a/amaranth/hdl/_dsl.py
+++ b/amaranth/hdl/_dsl.py
@@ -167,7 +167,7 @@ class FSM:
         if name not in self.encoding:
             self.encoding[name] = len(self.encoding)
             fsm_name = self._data["name"]
-            self._data["ongoing"][name] = Signal(name=f"{fsm_name}_ongoing_{name}")
+            self._data["ongoing"][name] = Signal(name="")
         return self._data["ongoing"][name]
 
 
@@ -462,7 +462,7 @@ class Module(_ModuleBuilderRoot, Elaboratable):
         if name not in fsm_data["encoding"]:
             fsm_name = fsm_data["name"]
             fsm_data["encoding"][name] = len(fsm_data["encoding"])
-            fsm_data["ongoing"][name] = Signal(name=f"{fsm_name}_ongoing_{name}")
+            fsm_data["ongoing"][name] = Signal(name="")
         try:
             _outer_case, self._statements = self._statements, {}
             self._ctrl_context = None
@@ -486,7 +486,7 @@ class Module(_ModuleBuilderRoot, Elaboratable):
                     if name not in ctrl_data["encoding"]:
                         fsm_name = ctrl_data["name"]
                         ctrl_data["encoding"][name] = len(ctrl_data["encoding"])
-                        ctrl_data["ongoing"][name] = Signal(name=f"{fsm_name}_ongoing_{name}")
+                        ctrl_data["ongoing"][name] = Signal(name="")
                     self._add_statement(
                         assigns=[FSMNextStatement(ctrl_data, name)],
                         domain=ctrl_data["domain"],
diff --git a/amaranth/hdl/_ir.py b/amaranth/hdl/_ir.py
index e02bae2..f3e9fc9 100644
--- a/amaranth/hdl/_ir.py
+++ b/amaranth/hdl/_ir.py
@@ -552,6 +552,8 @@ class Design:
         assigned_names = {name for name, conn, dir in self.ports if name is not None}
         for name, conn, dir in self.ports:
             if name is None:
+                if conn.name == "": # Nothing to name this port!
+                    raise TypeError("Signals with private names cannot be used in unnamed top-level ports")
                 name = _add_name(assigned_names, conn.name)
                 assigned_names.add(name)
             new_ports.append((name, conn, dir))
@@ -590,7 +592,7 @@ class Design:
                     frag_info.io_port_names[conn] = name
 
         for signal in frag_info.used_signals:
-            if signal not in frag_info.signal_names:
+            if signal not in frag_info.signal_names and signal.name != "": # Private name shouldn't be added.
                 frag_info.signal_names[signal] = _add_name(frag_info.assigned_names, signal.name)
         for port in frag_info.used_io_ports:
             if port not in frag_info.io_port_names:
diff --git a/amaranth/sim/core.py b/amaranth/sim/core.py
index 67c4f10..b54792e 100644
--- a/amaranth/sim/core.py
+++ b/amaranth/sim/core.py
@@ -4,6 +4,7 @@ import warnings
 from .._utils import deprecated
 from ..hdl._cd import *
 from ..hdl._ir import *
+from ..hdl._ast import Value, ValueLike
 from ._base import BaseEngine
 
 
@@ -238,5 +239,15 @@ class Simulator:
                     file.close()
             raise ValueError("Cannot start writing waveforms after advancing simulation time")
 
+        for trace in traces:
+            if isinstance(trace, ValueLike):
+                trace_cast = Value.cast(trace)
+                for trace_signal in trace_cast._rhs_signals():
+                    if trace_signal.name == "":
+                        if trace_signal is trace:
+                            raise TypeError("Cannot trace signal with private name")
+                        else:
+                            raise TypeError(f"Cannot trace signal with private name (within {trace!r})")
+
         return self._engine.write_vcd(vcd_file=vcd_file, gtkw_file=gtkw_file,
                                       traces=traces, fs_per_delta=fs_per_delta)


Test patch:
diff --git a/tests/test_hdl_ast.py b/tests/test_hdl_ast.py
index d233b23..576aa90 100644
--- a/tests/test_hdl_ast.py
+++ b/tests/test_hdl_ast.py
@@ -1180,6 +1180,8 @@ class SignalTestCase(FHDLTestCase):
         self.assertEqual(s1.name, "s1")
         s2 = Signal(name="sig")
         self.assertEqual(s2.name, "sig")
+        s3 = Signal(name="")
+        self.assertEqual(s3.name, "")
 
     def test_init(self):
         s1 = Signal(4, init=0b111, reset_less=True)
@@ -1294,6 +1296,8 @@ class SignalTestCase(FHDLTestCase):
     def test_repr(self):
         s1 = Signal()
         self.assertEqual(repr(s1), "(sig s1)")
+        s2 = Signal(name="")
+        self.assertEqual(repr(s2), "(sig)")
 
     def test_like(self):
         s1 = Signal.like(Signal(4))
diff --git a/tests/test_hdl_dsl.py b/tests/test_hdl_dsl.py
index c3e8c02..8048e54 100644
--- a/tests/test_hdl_dsl.py
+++ b/tests/test_hdl_dsl.py
@@ -603,8 +603,8 @@ class DSLTestCase(FHDLTestCase):
                 )
                 (case 1 )
             )
-            (eq (sig fsm_ongoing_FIRST) (== (sig fsm_state) (const 1'd0)))
-            (eq (sig fsm_ongoing_SECOND) (== (sig fsm_state) (const 1'd1)))
+            (eq (sig) (== (sig fsm_state) (const 1'd0)))
+            (eq (sig) (== (sig fsm_state) (const 1'd1)))
         )
         """)
         self.assertRepr(frag.statements["sync"], """
@@ -627,8 +627,7 @@ class DSLTestCase(FHDLTestCase):
             "(sig a)": "comb",
             "(sig fsm_state)": "sync",
             "(sig b)": "sync",
-            "(sig fsm_ongoing_FIRST)": "comb",
-            "(sig fsm_ongoing_SECOND)": "comb",
+            "(sig)": "comb",
         })
         fsm  = frag.find_generated("fsm")
         self.assertIsInstance(fsm.state, Signal)
@@ -659,8 +658,8 @@ class DSLTestCase(FHDLTestCase):
                 )
                 (case 1 )
             )
-            (eq (sig fsm_ongoing_FIRST) (== (sig fsm_state) (const 1'd0)))
-            (eq (sig fsm_ongoing_SECOND) (== (sig fsm_state) (const 1'd1)))
+            (eq (sig) (== (sig fsm_state) (const 1'd0)))
+            (eq (sig) (== (sig fsm_state) (const 1'd1)))
         )
         """)
         self.assertRepr(frag.statements["sync"], """
@@ -697,8 +696,8 @@ class DSLTestCase(FHDLTestCase):
                 )
                 (case 1 )
             )
-            (eq (sig fsm_ongoing_FIRST) (== (sig fsm_state) (const 1'd0)))
-            (eq (sig fsm_ongoing_SECOND) (== (sig fsm_state) (const 1'd1)))
+            (eq (sig) (== (sig fsm_state) (const 1'd0)))
+            (eq (sig) (== (sig fsm_state) (const 1'd1)))
         )
         """)
         self.assertRepr(frag.statements["sync"], """
@@ -743,10 +742,10 @@ class DSLTestCase(FHDLTestCase):
         self.maxDiff = 10000
         self.assertRepr(frag.statements["comb"], """
         (
-            (eq (sig b) (sig fsm_ongoing_SECOND))
-            (eq (sig a) (sig fsm_ongoing_FIRST))
-            (eq (sig fsm_ongoing_SECOND) (== (sig fsm_state) (const 1'd0)))
-            (eq (sig fsm_ongoing_FIRST) (== (sig fsm_state) (const 1'd1)))
+            (eq (sig b) (sig))
+            (eq (sig a) (sig))
+            (eq (sig) (== (sig fsm_state) (const 1'd0)))
+            (eq (sig) (== (sig fsm_state) (const 1'd1)))
         )
         """)
 
diff --git a/tests/test_hdl_ir.py b/tests/test_hdl_ir.py
index f968379..133c32f 100644
--- a/tests/test_hdl_ir.py
+++ b/tests/test_hdl_ir.py
@@ -962,6 +962,7 @@ class NamesTestCase(FHDLTestCase):
         o1 = Signal()
         o2 = Signal()
         o3 = Signal()
+        o4 = Signal(name="")
         i1 = Signal(name="i")
 
         f = Fragment()
@@ -980,6 +981,7 @@ class NamesTestCase(FHDLTestCase):
             "o1": (o1, PortDirection.Output),
             "o2": (o2, PortDirection.Output),
             "o3": (o3, PortDirection.Output),
+            "o4": (o4, PortDirection.Output),
         }
         design = f.prepare(ports)
         self.assertEqual(design.fragments[design.fragment].signal_names, SignalDict([
@@ -988,12 +990,20 @@ class NamesTestCase(FHDLTestCase):
             (o1, "o1"),
             (o2, "o2"),
             (o3, "o3"),
+            # (o4, "o4"), # Signal has a private name.
             (cd_sync.clk, "clk"),
-            (cd_sync.rst, "rst$6"),
+            (cd_sync.rst, "rst$7"),
             (cd_sync_norst.clk, "sync_norst_clk"),
-            (i1, "i$7"),
+            (i1, "i$8"),
         ]))
 
+    def test_wrong_private_unnamed_toplevel_ports(self):
+        s = Signal(name="")
+        f = Fragment()
+        with self.assertRaisesRegex(TypeError,
+                r"^Signals with private names cannot be used in unnamed top-level ports$"):
+            Design(f, ports=((None, s, None),), hierarchy=("top",))
+
     def test_assign_names_to_fragments(self):
         f = Fragment()
         f.add_subfragment(a := Fragment())
diff --git a/tests/test_sim.py b/tests/test_sim.py
index 978f8c0..9be4299 100644
--- a/tests/test_sim.py
+++ b/tests/test_sim.py
@@ -14,6 +14,7 @@ from amaranth.hdl._dsl import  *
 from amaranth.hdl._ir import *
 from amaranth.sim import *
 from amaranth.lib.memory import Memory
+from amaranth.lib.data import View, StructLayout
 
 from .utils import *
 from amaranth._utils import _ignore_deprecated
@@ -1042,6 +1043,21 @@ class SimulatorIntegrationTestCase(FHDLTestCase):
                 with sim.write_vcd(f):
                     pass
 
+    def test_vcd_private_signal(self):
+        sim = Simulator(Module())
+        with self.assertRaisesRegex(TypeError,
+                r"^Cannot trace signal with private name$"):
+            with open(os.path.devnull, "w") as f:
+                with sim.write_vcd(f, traces=(Signal(name=""),)):
+                    pass
+
+        sim = Simulator(Module())
+        with self.assertRaisesRegex(TypeError,
+                r"^Cannot trace signal with private name \(within \(cat \(sig x\) \(sig\)\)\)$"):
+            with open(os.path.devnull, "w") as f:
+                with sim.write_vcd(f, traces=(Cat(Signal(name="x"), Signal(name="")),)):
+                    pass
+
     def test_no_negated_boolean_warning(self):
         m = Module()
         a = Signal()