Repository: amaranth-lang/amaranth (python)

Issue:
Indexing into signals with layouts could be improved
Motivating example:
If you have `s = Signal(Layout)`, you can index a specific field with `s.field` or `s[field]`;  or index a slice with `s.as_value()[0:4]`. If you try to index it directly as `s[0:4]` you get `KeyError: slice(0, 4, None)`. 

This is a bit of a sharp edge for a new user because `s[0:4]` looks like valid amaranth syntax and would be valid for `s = Signal(8)`. A useful piece of context not provided by the error message is that `s.as_value()[0:4]` is how you would accomplish the intention of `s[0:4]`.

Possible resolutions: 
1) Since layouts already have a custom `__getitem__`  method, an extra error message could be added there, along the lines of:
```python
 if isinstance(key, slice): 
      raise KeyError("Signals with layouts cannot be indexed with slices, use signal.as_value()")
```
2) Make it possible to index into a signal directly, so that `s[0:4]` and `s.field` are both valid. Since layout members must be strings, there shouldn't be any possible collisions between integer slices and layout members (ie, you could have a field named `"0:4"` but not a field named `0:4`. I'm not sure exactly what this implementation could look like.

3) Do nothing. The existing Amaranth documentation already explains all of this.

Pull request:
Add a diagnostic for slicing `data.View`
This is meaningless for a view but meaningful for the underlying value.

Fixes #1375.

Patch:
diff --git a/amaranth/hdl/_dsl.py b/amaranth/hdl/_dsl.py
index a73f20f..c66df2e 100644
--- a/amaranth/hdl/_dsl.py
+++ b/amaranth/hdl/_dsl.py
@@ -654,6 +654,8 @@ class Module(_ModuleBuilderRoot, Elaboratable):
         if name == None:
             self._anon_submodules.append((submodule, src_loc))
         else:
+            if name == "":
+                raise NameError("Submodule name must not be empty")
             if name in self._named_submodules:
                 raise NameError(f"Submodule named '{name}' already exists")
             self._named_submodules[name] = (submodule, src_loc)
diff --git a/amaranth/lib/data.py b/amaranth/lib/data.py
index 302aaf1..623d327 100644
--- a/amaranth/lib/data.py
+++ b/amaranth/lib/data.py
@@ -792,6 +792,9 @@ class View(ValueCastable):
         :exc:`TypeError`
             If :meth:`.ShapeCastable.__call__` does not return a value or a value-castable object.
         """
+        if isinstance(key, slice):
+            raise TypeError(
+                "View cannot be indexed with a slice; did you mean to call `.as_value()` first?")
         if isinstance(self.__layout, ArrayLayout):
             if not isinstance(key, (int, Value, ValueCastable)):
                 raise TypeError(


Test patch:
diff --git a/tests/test_hdl_dsl.py b/tests/test_hdl_dsl.py
index a255ff6..ecbed9d 100644
--- a/tests/test_hdl_dsl.py
+++ b/tests/test_hdl_dsl.py
@@ -887,6 +887,12 @@ class DSLTestCase(FHDLTestCase):
         with self.assertRaisesRegex(NameError, r"^Submodule named 'foo' already exists$"):
             m1.submodules.foo = m2
 
+    def test_submodule_named_empty(self):
+        m1 = Module()
+        m2 = Module()
+        with self.assertRaisesRegex(NameError, r"^Submodule name must not be empty$"):
+            m1.submodules[""] = m2
+
     def test_submodule_get(self):
         m1 = Module()
         m2 = Module()
diff --git a/tests/test_lib_data.py b/tests/test_lib_data.py
index 7eb90a1..697f366 100644
--- a/tests/test_lib_data.py
+++ b/tests/test_lib_data.py
@@ -670,6 +670,12 @@ class ViewTestCase(FHDLTestCase):
                 r"with a value$"):
             Signal(data.StructLayout({}))[Signal(1)]
 
+    def test_index_wrong_slice(self):
+        with self.assertRaisesRegex(TypeError,
+                r"^View cannot be indexed with a slice; did you mean to call `.as_value\(\)` "
+                r"first\?$"):
+            Signal(data.StructLayout({}))[0:1]
+
     def test_getattr(self):
         v = Signal(data.UnionLayout({
             "a": unsigned(2),