Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
NeilGirdhar opened this issue Mar 22, 2023 · 5 comments
Labels
topic: other Other topics not covered

Comments

@NeilGirdhar
Copy link

NeilGirdhar commented Mar 22, 2023

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 NeilGirdhar added the topic: other Other topics not covered label Mar 22, 2023
@AlexWaygood
Copy link
Member

Cc. @stroxler

@NeilGirdhar
Copy link
Author

NeilGirdhar commented Mar 22, 2023

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...

@stroxler
Copy link
Contributor

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
Copy link
Contributor

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.

@erictraut
Copy link
Collaborator

erictraut commented Mar 31, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: other Other topics not covered
Projects
None yet
Development

No branches or pull requests

4 participants