search

python / mypy

Optional static typing for Python
https://www.mypy-lang.org/
Other
18.23k stars 2.79k forks source link

Add `--warn-needless-override` option to `stubtest` #13321

Open sobolevn opened 2 years ago

sobolevn commented 2 years ago

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 commented 2 years ago

CC @srittau and @AlexWaygood

AlexWaygood commented 2 years ago

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:

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

sobolevn commented 2 years ago

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 commented 2 years ago

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'>
srittau commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

AlexWaygood commented 2 years ago

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:

Since https://github.com/python/typeshed/pull/8483 was merged, we've already had to revert a lot of __eq__ overrides:

And I think there may also be an issue with some of the __delattr__ deletions in https://github.com/python/typeshed/pull/8483:

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

AlexWaygood commented 2 years ago

In fact, for some special methods such as __eq__ and __format__ (see https://github.com/python/typeshed/pull/6877#issuecomment-1008350944), 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 commented 2 years ago

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!