Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
mcclure opened this issue Jun 28, 2024 · 1 comment
Open

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

mcclure opened this issue Jun 28, 2024 · 1 comment

Comments

@mcclure
Copy link
Contributor

mcclure commented Jun 28, 2024

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
Copy link
Member

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.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants