Repository navigation
Add --warn-needless-override option to stubtest #13321
Description
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.
Activity
sobolevn commentedon Aug 4, 2022
CC @srittau and @AlexWaygood
AlexWaygood commentedon Aug 4, 2022
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:(Async)Iterablefrom__(a)iter__methods PyCQA/flake8-pyi#248 — currently this flake8-pyi check only checks that you don't returnIterablefrom__iter__methods, butstublintcould 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 ofIterator.Cc. @hauntsaninja as well, as the stubtest expert :)
sobolevn commentedon Aug 4, 2022
The same is true about not just methods, but attributes as well:
sobolevn commentedon Aug 4, 2022
I wrote a simple prototype (I was not able to go through all results, but top ~10 looks correct):
close,seek, etcsrittau commentedon Aug 4, 2022
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 commentedon Aug 4, 2022
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 commentedon Aug 4, 2022
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 commentedon Aug 4, 2022
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 commentedon Aug 4, 2022
Anyways, I will use the information I collected from
stubtestto send atypeshedPR 🙂I think I will finish it today / tomorrow. There are lots of methods to analyze!
JelleZijlstra commentedon Aug 4, 2022
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 commentedon Aug 4, 2022
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 commentedon Aug 6, 2022
Fair enough; I'm persuaded :)
+1 to both of these thoughts from @hauntsaninja as well.
sobolevn commentedon Aug 6, 2022
Ok then! I will send my prototype for everyone to review 🙂
AlexWaygood commentedon Aug 8, 2022
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 python/typeshed#8483 was merged, we've already had to revert a lot of
__eq__overrides:__eq__removal typeshed#8487And I think there may also be an issue with some of the
__delattr__deletions in python/typeshed#8483:decimal.Context.__delattr__be in the stub? typeshed#8504For dunder methods, can we take removals from typeshed one dunder method at a time, the same way we did with
__hash__?AlexWaygood commentedon Aug 8, 2022
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 commentedon Aug 8, 2022
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!