typing icon indicating copy to clipboard operation
typing copied to clipboard

Provide guidance on PEP 698's strict enforcement feature of typing.override

Open NeilGirdhar opened this issue 2 years ago • 5 comments

PEP 698 (typing.override) suggests that type-checkers provide a strict enforcement option that would mark method overrides. However, no guidance is provided about the methods __init__ and __new__. These methods are always implicit overrides (from object method). These method do not obey LSP (so their signature doesn't depend on any superclass), which means that override decoration only verifies that you haven't misspelled the method. Should they be exempt from this strict check?

(I'm not convinced one way or the other, but it would be nice to have some guidance on this.)

CC'ing @erictraut because we had a brief discussion about this.

NeilGirdhar avatar Mar 22 '23 14:03 NeilGirdhar

Cc. @stroxler

AlexWaygood avatar Mar 22 '23 14:03 AlexWaygood

On the same topic, consider this bad code

class  A:
    def f(self, x: int) -> None:
        pass


class  B:
    def f(self, x: int) -> None:
        pass


class  C(A, B):
    pass

In C, A.f overrides B.f (often by mistake). Should this implicit override be reported in strict mode?

Normally, if A and B both implement the same method (not just in the sense of same signature, but in the sense of behavior), then both A and B should inherit from an interface that defines that method:

class SomeInterface:
  def f(self, x: int) -> None:
     raise NotImplementedError

class A(SomeInterface):
  @override
  def f...

NeilGirdhar avatar Mar 22 '23 15:03 NeilGirdhar

I would very much favor that __new__ and __init__ be treated as special cases and exempt from strict @override checks - most type checkers (or at least Pyre) do not require compatibility of signatures for constructors, unlike on other methods.

stroxler avatar Mar 31 '23 02:03 stroxler

I don't have as much of an opinion about multiple inheritance; that's a messy problem that I haven't thought too much about.

In many cases preexisting conventions (which vary a bit per type checker) about method signature type compatibility might detect the problem - for example if A.f and B.f had different signatures (they are the same here) then at least some type checkers would probably complain about C.f not satisfying the signature of B.f because of incompatible types for A.f, which comes first in method resolution.

That said, I'm not sure Pyre would catch this with compatibility checks- I suspect that we only verify compatibility on actual definitions in C, not on collisions produced through multiple inheritance.

Regardless, the idea certainly has merit.

One potential reason not to do that would be that normal strict-override-errors should be trivial to autofix by injecting an @override decorator, whereas the one on C is far from trivial - you have to control at least one of A and B and have the ability to refactor the whole class hierarchy.

We probably wouldn't include this in Pyre for that reason, since we need to be able to auto-apply @override to millions of lines of existing code.

stroxler avatar Mar 31 '23 02:03 stroxler

I'm fine exempting __init__ and __new__. In my current implement of PEP 698, I didn't do this (for the reasons I noted here), but I will make that change.

As for the multiple inheritance example, I don't think that falls under the purview of PEP 698. Any class in Python can, in theory, be used as a base class for multiple inheritance. It would be unreasonable and inappropriate to expect that an author of a class anticipate the need to mark their methods with an @override descriptor to handle the (unlikely) case that it is overridden (obscured) by another "peer" class. See this issue for more details.

erictraut avatar Mar 31 '23 03:03 erictraut