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

Activity

1. NeilGirdhar

   added

   topic: otherOther topics not coveredOther topics not covered

   on Mar 22, 2023
2. AlexWaygood commented on Mar 22, 2023

   

   AlexWaygood

   on Mar 22, 2023

   Member

   More actions

   Cc. @stroxler
3. NeilGirdhar commented on Mar 22, 2023

   

   NeilGirdhar

   on Mar 22, 2023

   Last edited by NeilGirdhar

   Author

   More actions

   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...
4. stroxler commented on Mar 31, 2023

   

   stroxler

   on Mar 31, 2023

   Contributor

   More actions

   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.

   👍React with 👍2Reacted by Sergio Castillo and Josh Smith
5. stroxler commented on Mar 31, 2023

   

   stroxler

   on Mar 31, 2023

   Contributor

   More actions

   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.

   👍React with 👍1Reacted by Neil Girdhar
6. erictraut commented on Mar 31, 2023

   

   erictraut

   on Mar 31, 2023

   Last edited by erictraut

   Collaborator

   More actions

   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.

   👍React with 👍1Reacted by Steven Troxler