[Bind2] Refactor XMR references

[rsetaluri]

Avoids requiring setting instances as attributes on circuits to refer to them (similarly with self. for Generator2's). Instead getting the attribute will retrieve the instance by name.

For example,

class Foo(m.Circuit):
    my_placeholder_var = Bar(name="my_instance_name")

assert Foo.my_placeholder_var is Foo.my_instance_name

[codecov-commenter]

Codecov Report

Merging #1255 (7e054d4) into master (c865dd7) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 7e054d4 differs from pull request most recent head 65940f6. Consider uploading reports for the commit 65940f6 to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@           Coverage Diff           @@
##           master    #1255   +/-   ##
=======================================
  Coverage   85.67%   85.67%           
=======================================
  Files         161      161           
  Lines       17410    17416    +6     
=======================================
+ Hits        14916    14922    +6     
  Misses       2494     2494           

Impacted Files	Coverage Δ
magma/circuit.py	92.15% <100.00%> (+0.07%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[rsetaluri]

@leonardt my main concern is the performance cost of doing this lookup every time getattr is called. We could add an acceleration data structure (map from names to instances) to speed this up. Although, currently we don't enforce uniqueness of instance names, and that would implicitly enforce that. Open to suggestions here.

[leonardt]

Could we invert this, do __getattribute__ by default, and check instances list on AttributeError? That would optimize the common case (save a level of indirection in the function call stack)

[rsetaluri]

Could we invert this, do __getattribute__ by default, and check instances list on AttributeError? That would optimize the common case (save a level of indirection in the function call stack)

I like this option more. In general though, there is a problem about aliasing/colliding names, e.g.

class Foo(m.Circuit):
    Bar(name="bar")
    bar = 100

print (Foo.bar)  # prints "100"...

We should have a clear spec on what we want to do in this case, that is ideally independent of implementation concerns. Not sure I have a strong opinion on what we should do in this case. Alternatively, we could add a separate instance getter API, like Foo.get_instance(..).

[leonardt]

I think the above behavior is expected given normal Python class/object semantics so I'd advocate for preserving that and having a get API specifically for instances when there is this issue of collisions.

[rsetaluri]

Good point. See #1262

[leonardt]

Alternatively, we could try patching the instance name attributes at close time? Assuming the .open() pattern is used for modifications, this should be safe?

[rsetaluri]

@leonardt ended up going with your suggestion of defaulting to object.__getattribute__() and only searching for instances in the error case.