Mypy issues in QROM / SelectSwapQROM

[tanujkhattar]

https://github.com/quantumlib/Qualtran/pull/986 adds 3 type ignores to qrom bloqs to make mypy happy; but its not clear why these issues arise in the first place.

Run check/mypy
qualtran/bloqs/data_loading/qrom.py:56: error: Definition of "controlled" in base class "Bloq" is incompatible with definition in base class "Gate"  [misc]
qualtran/bloqs/data_loading/select_swap_qrom.py:65: error: Definition of "controlled" in base class "Bloq" is incompatible with definition in base class "Gate"  [misc]
qualtran/bloqs/data_loading/select_swap_qrom.py:253: error: Missing return statement  [return]
Found 3 errors in 2 files (checked 436 source files)

This issue is to unblock the PR and track the potential fixes of these mypy issues at a later point.

• [ ] For the first two, there is some nuance going on with multiple inheritance and type-ignores in controlled method of GateWithRegisters. Both QROM and SelectSwapQROM derive from GateWithRegisters and QROMBase (which is a new abstract base class). The only place where the controlled method should come from is the GateWithRegisters base class; but due to multiple inheritance mypy starts complaining. This issue doesn't arise for all other bloqs that only derive from GateWithRegisters. For some reason, having multiple parent classes is making mypy unhappy.

• [ ] For the third issue, we have used the pattern of yielding operations without an explicit "return" statement when a function returns a cirq.OP_TREE forever in cirq; but for some reason its making mypy unhappy here.

cc @dstrain115

[dstrain115]

For any subclasses of GateWithRegisters, you will need to add a #type: ignore[misc]. This is because Bloq and cirq.Gate have conflicting definitions for the controlled method, so the double inheritance is not 100% valid accoridng to type checking.

For the third one, you probably need to change the return type to Iterator or Generator.

Let me know if you need help with this.

[tanujkhattar]

I thought cirq.OP_TREE is defined to be a union of Operation and an iterator over OpTree so we shouldn't nee to change the return type to an iterator?

https://github.com/quantumlib/Cirq/blob/e4b6ab2f1f01b3c2d06456eb7244430e691474ef/cirq-core/cirq/ops/op_tree.py#L56

Either way, it looks like https://github.com/python/mypy/issues/731 is now fixed so we should update the definition of cirq.OP_TREE to be a recursive type (cc @pavoljuhas can we do this before the next release?)

[pavoljuhas]

Either way, it looks like python/mypy#731 is now fixed so we should update the definition of cirq.OP_TREE to be a recursive type (cc @pavoljuhas can we do this before the next release?)

The problem with OP_TREE, whether defined as the current Union[Operation, OpTree] or a recursive Union[Operation, Iterable[OP_TREE]] type, is that it does not guarantee such types are iterable. You would need to add isinstance(optree, Iterable) to pass mypy for every iteration over values that are returned as OP_TREE-s.

A proper way would be to redefine OP_TREE as Union[Iterable[Operation], Iterable[OP_TREE]], but I suspect that would necessitate a lot of changes to the cirq code and would be a breaking API change.

If I check cirq with mypy-1.10.0 as in qualtran I get a similar "Missing return statement" error for functions that yield after declaring an OP_TREE return type.

I suggest to change the return type of SelectSwapQROM.decompose_from_registers to Iterator[cirq.OP_TREE], there are already similar constructs in qualtran for example in TwoBitCSwap.decompose_from_registers.