amaranth-lang / amaranth

A modern hardware definition language and toolchain based on Python
https://amaranth-lang.org/docs/amaranth/
BSD 2-Clause "Simplified" License
1.54k stars 170 forks source link

Improve diagnostics on module.domain/module.domains mixup #1431

Open mcclure opened 3 months ago

mcclure commented 3 months ago

Modules have a ".domain" field [used primarily for reading back domains by name, usually aliased from m.d] and a ".domains" field [a list which is added to to define new clock domains]. I raised some questions in project chat about whether this is confusing, the conclusion was no because people have handled it so far, and it actually slightly idiomatic for amaranth (e.g. csr "field" vs "fields").

However, there is still one problem: It would be very easy for someone, especially a new user, to mistype "domains" as "domain", and not understand what they did wrong. This is exacerbated because the documentation currently cites m.d and m.domains but not m.domain.

@whitequark proposes a solution: Add improved diagnostics. Currently, if you try to use m.domains like m.domain`, it looks like this:

>>> m.domains["sync2"] = Signal().eq(Signal())
Traceback (most recent call last):
  File "/home/whitequark/Projects/glasgow/software/glasgow/support/arepl.py", line 47, in _run_code
    future = eval(code, self.locals)
             ^^^^^^^^^^^^^^^^^^^^^^^
  File "<console>", line 1, in <module>
TypeError: '_ModuleBuilderDomainSet' object does not support item assignment

And if you try to use m.domain like m.domains, it looks like:

>>> m.domain.sync2 = cd_sync2 = ClockDomain()
Traceback (most recent call last):
  File "/home/whitequark/Projects/glasgow/software/glasgow/support/arepl.py", line 47, in _run_code
    future = eval(code, self.locals)
             ^^^^^^^^^^^^^^^^^^^^^^^
  File "<console>", line 1, in <module>
  File "/home/whitequark/.local/pipx/venvs/glasgow/lib/python3.11/site-packages/amaranth/hdl/_dsl.py", line 132, in __setattr__
    raise AttributeError("Cannot assign 'd.{}' attribute; did you mean 'd.{} +='?"
AttributeError: Cannot assign 'd.sync2' attribute; did you mean 'd.sync2 +='?

Because the uses of m.d and m.domains are fully disjoint, performing an operation on the "wrong" object is detectable¹.

Expected behavior

In the TypeError raised when using m.domains "like m.d", have the informational string text include a phrase like "Hint: Did you mean to use the d field rather than the domains field?". In the AttributeError used when using m.d "like m.domains", the string should include a phrase like "Hint: Did you mean to use the domains field?" in addition to the suggestion about sync2 +=.

This should not need an RFC because it affects error messages only.


¹ I previously suggested using this property to collapse m.d and m.domains into one object, but after some discussion we concluded this was actually impossible.

whitequark commented 3 months ago

I previously suggested using this property to collapse m.d and m.domains into one object, but after some discussion we concluded this was actually impossible.

(Or, to the extent it's possible, it doesn't appear to have a benefit comparable to the effort required, and would have to depend on the clock domain rework anyway.)