[Circuit] Detect overwriting of self.io in the presence of local references

[rsetaluri]

Often times it is desirable to both locally refer to self.io in a generator's __init__ function (i.e. io = self.io), as well as incrementally build the io, i.e. self.io += m.IO(...). However, since += overwrites the self.io object with a new IO object, it can lead to a bug where one does the following:

io = self.io = m.IO(...)
self.io += m.IO(...)  # now self.io refers to the updated IO, while io is out of date
io.x @= ...

To alleviate this I propose the following:

1. Keep __add__ as a function that returns a new IO object. Ideally it should check refcount (e.g. sys.getrefcount or gc.get_referrers) and if there is another referrer to self, then raise a warning or error.
2. Implement __iadd__ as a new function that modifies self directly and returns self, avoiding the problem altogether by not creating a new object.

[leonardt]

I would suspect that 2. is more "pythonic" and perhaps assumed behavior from someone familiar with Python, although I know we originally chose to keep IOs as immutable on principal. Perhaps we can try to enumerate the advantages and costs of keeping it immutable.

Perhaps an option 3. is that __add__ invalidates the two objects use to create the new object, so in the above example io.x @= would fail because the old io object is now invalidated. It seems there should be no reason for the user to mutate an intermediate io object (reading it would be fine if they just wanted to get access to port, unless we allow ports to be removed, in which case we may want to invalidate reads too)

[phanrahan]

I am starting to watch the issues as a way of catching up.

I don't like the idea of modifying self.io in an init. First, I believe we should try to be functional and single assignment as much as possible. Second, we should have clearly delimited stages. Class definition time and generators are the right place to define self.io (it really should be thought of as cls.io).

Is there a compelling use case for this feature?

[rsetaluri]

@phanrahan thanks for the feedback. I agree with the goal of being functional and single assignment as much as possible. As well as delimiting stages as much as possible (specify io, instantiate modules, wire stuff up).

However, there are certainly compelling cases for using at very least + to update io, for example io = m.IO(<functional signals>) + m.ClockIO(). += is admittedly less compelling, e.g.

io = m.IO(<common stuff>)
if flag0:
    io += m.IO(<stuff related to flag0>)
if flag1:
    io += m.IO(<stuff related to flag1>)
...

Of course, designers can always do the following:

ports = {<common stuff>}
if flag0:
    ports.update({<stuff related to flag0>})
if flag1:
    ports.update({<stuff related to flag1>})
io = m.IO(**ports)

In practice we've found designers like doing the io += more than staging ports. And there is a lot of existing code using this pattern.

[rsetaluri]

@leonardt I think we can do both (1) and (2) (and your option (3)) altogether (not mutually exclusive changes).

I do think invalidating both lhs and rhs after a + is actually a good idea. I can draft a change that does all 3 things.

[phanrahan]

Can we limit the += to the class definition? Once the class is defined it becomes immutable?

[rsetaluri]

Can we limit the += to the class definition? Once the class is defined it becomes immutable?

Yes, this is already the case. IO can not be updated after class definition.

To clarify something from above, the reason for self.io is that in the construction of generators, self is the circuit definition (i.e. the class). That is the metaclass <-> class relationship.

[phanrahan]

Sounds good.