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

Breaking change in Record #533

Closed
anuejn opened this issue Nov 7, 2020 · 10 comments · Fixed by #541
Closed

Breaking change in Record #533

anuejn opened this issue Nov 7, 2020 · 10 comments · Fixed by #541
Labels
Milestone

Comments

@anuejn
Copy link
Contributor

anuejn commented Nov 7, 2020

abbebf8 introduced a breaking change in Record:
Some (all?) operators are not overloaded on it anymore because they are looked up on the class rather than on the instance so __getattr__() doesnt forward them.

pre abbebf8:

>>> Record([("a", 1)])
(rec <unnamed> a)

post abbebf8:

>>> Record([("a", 1)]) + 1
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: unsupported operand type(s) for +: 'Record' and 'int'
@whitequark whitequark added the bug label Nov 7, 2020
@whitequark whitequark added this to the 0.3 milestone Nov 7, 2020
@whitequark
Copy link
Member

Hm, I wonder what we should do here.

  • This could be done with a metaclass, but that may cause issues for people multiple-inheriting from Record (does anyone do that?);
  • We could also simply enumerate and assign all relevant methods on UserValue, which would also have a desirable side effect of not introducing anything new into the UserValue even if it is introduced in Value.

I think the second approach is better.

@awygle
Copy link
Contributor

awygle commented Nov 7, 2020

I agree with whitequark, the second option is better at this point.

We also need to enhance the Record test suite to catch stuff like this in the future.

@anuejn
Copy link
Contributor Author

anuejn commented Nov 8, 2020

I also have downstream code for which the old behavior of UserValue is quite handy and desirable. Maybe It is desirable to keep something like UserValue (with its implementation based on ValueCastable for those usecases?

@whitequark
Copy link
Member

whitequark commented Nov 8, 2020

Maybe It is desirable to keep something like UserValue

Under no circumstances I will keep UserValue with the old behavior (that adds methods on Value to it as we get more of those) as it has been one of the major design mistakes and backward compatibility hazards.

Your link doesn't work, so I can't offer you any alternative migration path.

@anuejn
Copy link
Contributor Author

anuejn commented Nov 8, 2020

Ah sorry for the broken link, here is a working one. Recommendations are appreciated :).

@whitequark
Copy link
Member

At a glance: isn't that yet another place where Interfaces would help?

@anuejn
Copy link
Contributor Author

anuejn commented Nov 8, 2020

At a glance: isn't that yet another place where Interfaces would help?

No, not really. The code tries to wrap a convenient interface around an Instance by parsing the verilog definition with yosys (and with this getting the direction of the ports) and then exposing the inputs / outputs as attributes.

Moreover, it gives you the ability to access the ports in a hierarchical way so that instance.somevalue is equivalent to instance.some.value which is quite handy with large Instances (e.g. the ps7 block of the zynq where you can pass individual axi ports around using this technique).
This however has one problematic aspect: If we have an Instance, that has both a port named some and a port named somevalue, we cant just return a Signal for instance.some because instance.some.value also needs to resolve to a port.
This means that I need to have something that looks just like a Signal but also has the ability to override __getattr__()

@whitequark
Copy link
Member

Moreover, it gives you the ability to access the ports in a hierarchical way [...] which is quite handy with large Instances (e.g. the ps7 block of the zynq where you can pass individual axi ports around using this technique).

This is exactly the kind of problem interfaces are supposed to solve: being able to pass an AXI or a Wishbone bus around, and also integrate it into a bundle of buses, or a module interface.

so that instance.somevalue is equivalent to instance.some.value

cant just return a Signal for instance.some because instance.some.value also needs to resolve to a port

I'm fine with this not being easy to do, because this is something I want to actively discourage. Elsewhere in nMigen and nMigen-SoC, I made sure that objects that override __getattr__ have no attributes of their own that can cause conflicts and backward compatibility hazards: m.d.<domain> is the prime example, and both UserValue and the current Record implementation are the worst offenders. PackedStruct would inherit from ValueCastable (which has no methods of its own except for as_value), and Interface is just its own thing.

For wrapping PS7, I would suggest using a prefix or a suffix for all composite buses that puts them in a namespace different from individual ports. You could also use a prefix or a suffix only in case of a conflict, but that seems harder to use to me.

@anuejn
Copy link
Contributor Author

anuejn commented Nov 8, 2020

This is exactly the kind of problem interfaces are supposed to solve: being able to pass an AXI or a Wishbone bus around, and also integrate it into a bundle of buses, or a module interface.

I have exactly that (with an out of tree Interface implementation) and use the code linked above to make building said Interface nicer. But maybe I should just get rid of that step in the middle (that now that I think of it doesnt really ease things a lot). Probably my code is just a bit too cursed and not really worth the introduced hazards.

@whitequark
Copy link
Member

Probably my code is just a bit too cursed and not really worth the introduced hazards.

This is basically what I was trying to say without being rude. I understand why you did it (I used this sort of thing too in the past), it's just I think other approaches are preferable.

whitequark pushed a commit that referenced this issue Nov 9, 2020
Commit abbebf8 used __getattr__ to proxy Value methods called on 
Record. However, that did not proxy operators like __add__ because
Python looks up the special operator methods directly on the class
and does not run __getattr__ if they are missing.

Instead of using __getattr__, explicitly enumerate and wrap every
Value method that should be proxied. This also ensures backwards
compatibility if more methods are added to Value later.

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

Successfully merging a pull request may close this issue.

3 participants