Add --warn-needless-override option to stubtest

[sobolevn]

Feature

Imagine this file with implementation:

class A:
   def do_some(self) -> None:
       print('A')

class B(A):
   def do_some(self) -> None:
       print('B')

Auto-stub creators will create a stub like:

class A:
   def do_some(self) -> None: ...

class B(A):
   def do_some(self) -> None: ...

Which technically is right, but not quite.
Since do_some has the same signature in both A and B (only runtime implementation is different), we don't actually need it to be duplicated. We should ideally want just:

class A:
   def do_some(self) -> None: ...

class B(A): ...

Because we always want minimal correct stubs.
But, right now we only do this by hand.

Pitch

I propose adding --warn-needless-override options (with whatever name) that can warn us about needless overrides of parent methods in child classes.

I will send a PR with the initial imlementation.

[sobolevn]

CC @srittau and @AlexWaygood

[AlexWaygood]

I've been wondering about something like this, and I love the idea — but I'm not sure it belongs in stubtest. Stubtest is a test that verifies the correctness of a stub. But this feels more like a stylistic thing that a linter should warn about.

What if we created a separate script, stublint, that was similar to stubtest but had more linter-esque checks in it? Other checks this linting script could potentially include are:

• Prefer unions over overloads PyCQA/flake8-pyi#245
• Warn about overloads that make other overloads useless PyCQA/flake8-pyi#246
• A better version of Introduce Y045: Ban returning (Async)Iterable from __(a)iter__ methods PyCQA/flake8-pyi#248 — currently this flake8-pyi check only checks that you don't return Iterable from __iter__ methods, but stublint could do a better job here, as it would be able harness the power of the type checker to tell in general whether the type being returned from an __iter__ method is a subtype of Iterator.

Cc. @hauntsaninja as well, as the stubtest expert :)

[sobolevn]

The same is true about not just methods, but attributes as well:

class A:
  a: int

class B(A):
  a: int  # should be flagged

[sobolevn]

I wrote a simple prototype (I was not able to go through all results, but top ~10 looks correct):

error: _compression.DecompressReader.readable redefinition
Stub: at line 62
<mypy.nodes.FuncDef object at 0x111094e10>
Runtime: at line 33 in file /Users/sobolev/.pyenv/versions/3.8.9/lib/python3.8/_compression.py
<class '_compression.DecompressReader'>

error: _compression.DecompressReader.close redefinition
Stub: at line 58
<mypy.nodes.FuncDef object at 0x1110949d0>
Runtime: at line 33 in file /Users/sobolev/.pyenv/versions/3.8.9/lib/python3.8/_compression.py
<class '_compression.DecompressReader'>

error: _compression.DecompressReader.seekable redefinition
Stub: at line 66
<mypy.nodes.FuncDef object at 0x110fb8260>
Runtime: at line 33 in file /Users/sobolev/.pyenv/versions/3.8.9/lib/python3.8/_compression.py
<class '_compression.DecompressReader'>

error: _compression.DecompressReader.tell redefinition
Stub: at line 67
<mypy.nodes.FuncDef object at 0x110fb8370>
Runtime: at line 33 in file /Users/sobolev/.pyenv/versions/3.8.9/lib/python3.8/_compression.py
<class '_compression.DecompressReader'>

error: _decimal.Context.__delattr__ redefinition
Stub: at line 94
<mypy.nodes.FuncDef object at 0x10fbdcd00>
Runtime:
<class 'decimal.Context'>

error: _decimal.Decimal.__hash__ redefinition
Stub: at line 99
<mypy.nodes.FuncDef object at 0x10ff36370>
Runtime:
<class 'decimal.Decimal'>

error: _decimal.Decimal.__eq__ redefinition
Stub: at line 95
<mypy.nodes.FuncDef object at 0x10fbdce10>
Runtime:
<class 'decimal.Decimal'>

error: _decimal.DecimalTuple.__annotations__ redefinition
Stub: at line 83
<mypy.nodes.Var object at 0x113aba740>
Runtime:
<class 'decimal.DecimalTuple'>

error: _decimal.DecimalTuple.__doc__ redefinition
Stub: at line 80
<mypy.nodes.Var object at 0x113aba440>
Runtime:
<class 'decimal.DecimalTuple'>

error: _dummy_thread.LockType.__init__ redefinition
Stub: at line 89
<mypy.nodes.FuncDef object at 0x10fbdc9d0>
Runtime: at line 88 in file /Users/sobolev/.pyenv/versions/3.8.9/lib/python3.8/_dummy_thread.py
<class '_dummy_thread.LockType'>

• This method: https://github.com/python/typeshed/blob/7953848101f734c590eaac2918034cebe609ab49/stdlib/_compression.pyi#L23 overrides this one: https://github.com/python/typeshed/blob/7953848101f734c590eaac2918034cebe609ab49/stdlib/io.pyi#L62 And the same for close, seek, etc

[srittau]

While I agree that this doesn't seem like a good fit for stubtest, I am not a fan of yet another linter. Would a check like this be hard to integrate into flake8-pyi?

[sobolevn]

I don't think it is possible. We need is_same_type() function to be sure that types are the same.

I guess we can do some cheap-by-name-checks in flake8-pyi, but that's it.

[AlexWaygood]

Would a check like this be hard to integrate into flake8-pyi?

flake8-pyi could potentially do something like this on a per-module basis, but it would mean substantially duplicating a lot of the work mypy already does in constructing symbol tables and inheritance trees. That feels silly to me -- flake8-pyi isn't a type checker, and we shouldn't pretend to be one. Also, flake8-pyi only ever looks at code one file at a time, so if a class inherits from a class in another module, flake8-pyi loses all information about the base class.

One way around this would be if flake8-pyi added mypy as a runtime dependency. Then we could harness mypy to build the stubs, and then do some linting in flake8-pyi using information based on mypy's build. But that might involve using some mypy internal implementation details, which (as we know all too well at flake8-pyi!) would be fragile and prone to breakage.

[sobolevn]

One way around this would be if flake8-pyi added mypy as a runtime dependency.

I've done this a couple of times. See https://github.com/wemake-services/typed-linter but, this is pretty hard to implement. You need to somehow match python's AST and mypy nodes (see https://github.com/wemake-services/typed-linter/blob/master/typed_linter/contrib/mypy/traversal/mypy_ast.py). You need to get expression's type information from mypy (it is statefull and very buggy for outsiders, see https://github.com/wemake-services/typed-linter/blob/master/typed_linter/contrib/mypy/type_reveal.py).

And it is just not worth it 😞

[sobolevn]

Anyways, I will use the information I collected from stubtest to send a typeshed PR 🙂
I think I will finish it today / tomorrow. There are lots of methods to analyze!

[JelleZijlstra]

I would prefer to implement this kind of check in stubtest over adding yet another tool. It's slightly outside stubtest's original use case, but "finding problems in stubs that need type checker support to detect" seems like a reasonable expansion.

[hauntsaninja]

I'm happy to implement things like this as opt-in checks in stubtest. One other option to throw out there, I think this kind of thing might be a better fit for a stub only lint in mypy proper. E.g. it's much more ergonomic to type-ignore with error code such a lint than allowlist it.

I think I'd also like to see the stubtest / mypy diff before we merge 30 typeshed PRs. There are ways in which I can imagine this going wrong, e.g. we should probably accept unannotated redefinitions, since a human would want to go and check whether the implementation actually takes the same set of types as the unannotated parent. There might also be cases where the presence of properties or methods on the class itself may matter to type checkers.

[AlexWaygood]

I would prefer to implement this kind of check in stubtest over adding yet another tool. It's slightly outside stubtest's original use case, but "finding problems in stubs that need type checker support to detect" seems like a reasonable expansion.

Fair enough; I'm persuaded :)

One other option to throw out there, I think this kind of thing might be a better fit for a stub only lint in mypy proper. [...] I think I'd also like to see the stubtest / mypy diff before we merge 30 typeshed PRs.

+1 to both of these thoughts from @hauntsaninja as well.

[sobolevn]

Ok then! I will send my prototype for everyone to review 🙂

[AlexWaygood]

I think I'd also like to see the stubtest / mypy diff before we merge 30 typeshed PRs. There are ways in which I can imagine this going wrong, e.g. we should probably accept unannotated redefinitions, since a human would want to go and check whether the implementation actually takes the same set of types as the unannotated parent. There might also be cases where the presence of properties or methods on the class itself may matter to type checkers.

I think we need to be especially careful with dunder methods when doing this kind of thing. Type checkers special-case a lot of dunder methods so that their very existence on a subclass causes them to treat the subclass specially. I've previously caused regressions related to this special-casing:

• Enum values on enum class can't be reassigned or deleted typeshed#7385).

Since python/typeshed#8483 was merged, we've already had to revert a lot of __eq__ overrides:

• Revert __eq__ removal typeshed#8487

And I think there may also be an issue with some of the __delattr__ deletions in python/typeshed#8483:

• Should decimal.Context.__delattr__ be in the stub? typeshed#8504

For dunder methods, can we take removals from typeshed one dunder method at a time, the same way we did with __hash__?

[AlexWaygood]

In fact, for some special methods such as __eq__ and __format__ (see python/typeshed#6877 (comment)), it might be good to have stubtest do the opposite to what's being proposed here: to ensure overrides are present on subclasses, even if the signature on the override is the same as on the base class.

[sobolevn]

Yes, I agree - special-casing is quite hard. I think that special PRs for that is the way to go.

__delattr__ is now added into the set of things to ignore. Thanks for catching this!