hdl.ast: implement ShapeCastable.__subclasshook__.

[ghost]

This PR intends to eventually implement ValueCastable.__subclasshook__ too. When it does so: fixes #753.

This is a draft with just the ShapeCastable side of things working — as best I can tell — almost as well as we'd like. There might be some more cases to handle in the workarounds. What workarounds? See python/cpython#81062. This is a bit uglier than I'd like.

• I haven't yet investigated if defining our own metaclass for these (instead of using ABCMeta) would be easier in sum or not, but I plan to.
• Note that __subclasshook__ doesn't get enough information to differentiate different enum subclasses. This fails objection (2b) from #753 (comment).
  • While I'm here, is it our intention that an empty Python Enum is shape-castable? Unlike Amaranth's, they are—they cast to unsigned(0).

[whitequark]

Really impressed with you figuring out how to do this so quickly, I don't think I'd have managed with the amount of Python bugs involved.

[whitequark]

This is so cursed.

[whitequark]

This is the only part of the PR/fix that gives me pause--this part is kind of ... Extremely obscure. Is there any better way to make this check? I was hoping that overriding __instancecheck__ but not __subclasscheck__ would be a viable solution, but having thought about it more that seems even worse than this.

I can see having to just accept this but it doesn't have to make me happy :s

[ghost]

Re: the type(obj).mro() suggestion (per @mwkmwkmwk)—this is almost OK, but unfortunately still not:

>>> from amaranth.lib.enum import Enum as AmaranthEnum
>>> class EnumA(AmaranthEnum): X = 0
... 
>>> EnumA.__class__.__mro__
(<class 'amaranth.lib.enum.EnumMeta'>, <class 'amaranth.hdl.ast.ShapeCastable'>, <class 'enum.EnumType'>, <class 'type'>, <class 'object'>)
>>> type(EnumA).mro()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: unbound method type.mro() needs an argument
>>>

mro() is defined on classes, not metaclasses: i.e. it's an instance method on type. For all call-sites, where the object we're checking may have a metaclass as its type()—which includes passing any enum class—this will fail. All call-sites may receive enum classes.

(It's worth noting this behaviour isn't due to any interaction with ABCMeta.)

We can use type(obj).__mro__, which contains 50% fewer underscores than what the PR currently proposes, so that's a start. (I've also found one site where we can actually use isinstance without causing recursive bad.)

One 'solution' is to make a helper and be done with it. The naming is awkward: ShapeCastable.actual_instance(o)? _utils.in_type_mro(o, c)? c in _utils.tmro(o)?

Another is to reduce the number of places we have to do this to less than 3, and then it's under threshold for refactoring!

More thoughts below.

[whitequark]

• I haven't yet investigated if defining our own metaclass for these (instead of using ABCMeta) would be easier in sum or not, but I plan to.

I was thinking of using ABCMeta since that seems to be the "proper" solution for our use case, but I'm open to using our own metaclass. (Though I'm a little worried at the proliferation of custom metaclasses, which we until recently entirely avoided.)

• While I'm here, is it our intention that an empty Python Enum is shape-castable? Unlike Amaranth's, they are—they cast to unsigned(0).

Oh, that's just an error on my part. Amaranth enums with no defined shape are intended to be a complete drop-in replacement for Python enums. IIRC the empty enum isn't shape-castable because of inheritance concerns? but due to the policy I mention that clearly must be fixed as a matter of principle even if there are consequences we have to deal with elsewhere.

This also resolves objection 2b which is convenient.

[ghost]

I was thinking of using ABCMeta since that seems to be the "proper" solution for our use case, but I'm open to using our own metaclass. (Though I'm a little worried at the proliferation of custom metaclasses, which we until recently entirely avoided.)

At the very least, it turns out adding our own ABCMeta subclass gives us a way to put a workaround in one place. Unfortunately, we have to use a descriptor :))))))

We need ABCMeta's calls to cls.__subclasses__() (and it turns out cls.__subclasscheck__() too, from __instancecheck__) actually call with the receiver being cls, regardless of whether cls is a class or metaclass. Without a descriptor, it's either an instance method and can only be called against classes (current behaviour), or a class method and the metaclass where it's defined is always the receiver.

Pros:

• What the workaround is doing is a lot more clear.
  • We don't need strange code in ShapeCastable.__subclasshook__ to side-step the issue, nor do we have to vet our behaviour vis-à-vis ABCMeta.__subclasscheck__ to make sure our results line up with what it would be doing if we ever returned to it.
  • We don't need to include a different bespoke workaround in EnumMeta either.

Cons:

• Now we have custom metaclasses with descriptors.
• It may not be clear where we need to use TypePatched/ABCMetaPatched.
  • Frankly, the errors that result without its use are a good tell.
  • (If we really do manage to not (further) proliferate metaclasses then it's not such a big deal!)

[codecov]

Codecov Report

Merging #830 (d1ecd56) into main (11d5bb1) will increase coverage by 0.09%.
The diff coverage is 97.77%.

@@            Coverage Diff             @@
##             main     #830      +/-   ##
==========================================
+ Coverage   83.75%   83.85%   +0.09%     
==========================================
  Files          54       54              
  Lines        7677     7704      +27     
  Branches     1867     1871       +4     
==========================================
+ Hits         6430     6460      +30     
+ Misses       1048     1045       -3     
  Partials      199      199              

Files Changed	Coverage Δ
amaranth/lib/data.py	99.00% <93.33%> (-0.33%)	⬇️
amaranth/hdl/ast.py	90.58% <100.00%> (+0.61%)	⬆️
amaranth/lib/wiring.py	100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[ghost]

This also resolves objection 2b which is convenient.

Actually, 2b is kind of half-resolved already, and half-not-touched by this. Revisiting it:

Given class X(amaranth.lib.enum.Enum): pass, issubclass(X, ShapeCastable) == True will always be the case, but the enum X is not actually shape-castable since Shape.cast(X) will raise TypeError.

It turns out issubclass(X, ShapeCastable) is always easy to get right, even in the presence of e.g. non-const-castable members. It ends up directly handing X to __subclasshook__, so we can inspect its members at will.

But what does issubclass(X, ShapeCastable) even mean? isinstance(X, ShapeCastable) — which is what we're saying implies Shape.cast(X) will succeed — only gets given type(X). That is EnumMeta, which leaves us with very little.

[ghost]

Continuing from thoughts in #830 (comment):

There's a number of places where isinstance(shape, ShapeCastable) had to be changed to (what is now) ShapeCastable in type(obj).__mro__ because they then do obj(...), obj.as_shape() or obj.const(...). These couldn't stay as isinstance checks because they're actually asking about the Python class MRO, rather than how shape-y it could be: _SignalMeta.__call__ wants to know if it can call its argument to wrap the returned signal, Layout.cast wants to know if its argument can have as_shape called on it, etc.

ISTM that we can clarify things: isinstance(obj, ShapeCastable) answers the question "will Shape.cast(obj) succeed?", but not whether the methods expected of a bona fide ShapeCastable subclass are available on obj. Luckily, object-oriented programming has a solution for us.

We can consider a different ABC for actual subclassing use—a terrible name would be CustomShapeCastable—which has the expected @abstractmethods. This also gives us a nicer division of responsibilities: ShapeCastable exists only to answer the question "can you Shape.cast(obj)?", and nothing about the availability of particular methods on obj. User and internal code alike can ask isinstance(obj, ABetterNameThanCustomShapeCastable) if they're wanting to attempt a method call that that base class promises. We mark ShapeCastable final at this stage.

An alternate solution proposes even bigger hacks where we magically insert a base class into subclasses of ShapeCastable such that internal code can ask isinstance(obj, RealImplementationOfShapeCastable) (i.e. instead of ShapeCastable in type(obj).__mro__), but it seems like more magic for less clarity.

[ghost]

A proof of concept of the CustomShapeCastable split can be seen here: charlottia/amaranth@castable-protocols...charlottia:amaranth:castable-protocols-split. We no longer have to do any manual type(x).__mro__ checks.

[ghost]

I was thinking of using ABCMeta since that seems to be the "proper" solution for our use case, but I'm open to using our own metaclass. (Though I'm a little worried at the proliferation of custom metaclasses, which we until recently entirely avoided.)

I've realized that, even if we abandon the idea of using ABCMeta at the ShapeCastable level, we still use it (for @abstractmethod) in Layout. We'd have to drop it there, too, otherwise we get a metaclass conflict.

I tried this. And you know what? It's so much less annoying. We have zero workarounds, except for implementing the abstract machinery ourselves. (But that sits in __init_subclass__ on the class itself—it's easy to see.) Because we override __instancecheck__ in the ShapeCastableMeta, we can distinguish enum subclasses. etc. etc. etc.

Will clean this up and push, but I think this is probably what the solution should look like, without a compelling reason to still want ABCMeta in this particular class hierarchy.

[ghost]

OK, I'm way happier with this. See 9b1aa95. I've included the "CustomShapeCastable" thing in this branch now too, but still think the name sucks and it's definitely an optional part of this change anyway.

Turns out avoiding ABCMeta lets you just say what you mean. We lose two things:

• @abstractmethod. We reimplement this in two places with __init_subclass__ — CustomShapeCastable (what we had before on ShapeCastable), and now Layout too.
  • We could refactor this in any number of ways if we wanted, and probably should — just had to fix one bug (9f1d935).
• The caching ABC does on the type checks.

I'm not sure how significant the latter may turn out to be.

[whitequark]

If we add a CustomShapeCastable alongside ShapeCastable, then this should affect ValueCastable as well; and that would need an RFC since it's a breaking change...

[ghost]

If we add a CustomShapeCastable alongside ShapeCastable, then this should affect ValueCastable as well; and that would need an RFC since it's a breaking change...

Hmmm, OK. Let's do this without breaking anything.

An alternate solution proposes even bigger hacks where we magically insert a base class into subclasses of ShapeCastable such that internal code can ask isinstance(obj, RealImplementationOfShapeCastable) (i.e. instead of ShapeCastable in type(obj).__mro__), but it seems like more magic for less clarity.

This doesn't seem that hacky or magical anymore. (Dropping ABCMeta and related hacks refunded quite a bit of magic budget.) It's quite straight-forward and means the diff to users is now "there's an extra type we can use to check specifically for the user-defined methods", without any loss or modifying existing code. All internal uses of CustomShapeCastable are solely those isinstance checks that require differentiating the two.

One question about this approach is whether we export CustomShapeCastable or not. I think we should: user code doing tricks may want to know whether the interface is available on a given object, and it will appear in live objects' MROs either way.

Currently just fixing up the coverage (/getting coverage diffs running locally); if this approach looks OK to you, I'll repeat for ValueCastable.

---

tl;dr:

1. How are you feeling about our own metaclass over ABCMeta here?
   1. Does the subclass insertion approach seem fine?
   2. Do we export the inserted subclass?
2. Refactor out abstract method machinery now, or later?

[ghost]

Today seemed like a fine day to keep diving deep, so I explored the ValueCastable add, as well as refactoring them. This was mostly for my own enjoyment, but it could well be useful or interesting, so I've pushed it to a different branch: main...charlottia:amaranth:castable-protocols-valuecastable. Notes on this stage are in the added commit: charlottia@3ddcb90 post-rebase: charlottia@a77c005.

[ghost]

Noting: I feel like this does (or will) overlap fairly substantially with #778.

[ghost]

Rebased this (and main...charlottia:amaranth:castable-protocols-valuecastable) post-RFC 22.

[whitequark]

Thank you for this work, @charlottia. We have decided to go with a new approach based on RFC 35, and this PR is now superseded by #983.

[ghost]

Got it, thanks!