Skip to content
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

disallow subclassing any #9491

Closed
wants to merge 20 commits into from
Closed

Conversation

Avasam
Copy link
Sponsor Collaborator

@Avasam Avasam commented Jan 11, 2023

Until they're all fixed. This serves as a tracker. (look at the CI results)

Quick link to latest CI result [2020/01/14]: https://github.com/python/typeshed/actions/runs/3918898421/jobs/6699566039

@Avasam
Copy link
Sponsor Collaborator Author

Avasam commented Jan 11, 2023

fpdf2 is blocked by fontTools not being marked as typed. The library does seem to type its function parameters from what I can see. Maybe they just didn't think to add a py.typed marker? See fonttools/fonttools#2943

@Avasam
Copy link
Sponsor Collaborator Author

Avasam commented Jan 11, 2023

tqdm is blocked by #8974 (tensorflow) and the lack of dask stubs. rich provides its own typing.
Although it might be possible to just redefine the classes in tqdm's stubs if they're simple enough.

@AlexWaygood
Copy link
Member

AlexWaygood commented Jan 11, 2023

tqdm is blocked by #8974 (tensorflow) and the lack of dask stubs. rich provides its own typing.
Although it might be possible to just redefine the classes in tqdm's stubs if they're simple enough.

Those parts of the tqdm stubs are all optional extensions to tqdm. I disagree with adding e.g. rich as a dependency for the whole stub just so that we can type an optional extension :/

@Avasam
Copy link
Sponsor Collaborator Author

Avasam commented Jan 11, 2023

Those parts of the tqdm stubs are all optional extensions to tqdm. I disagree with adding e.g. rich as a dependency for the whole stub just so that we can type an optional extension :/

I haven't looked at tqdm in details yet, but it's possible the type are quite simple (seeing how they seem to be some kind of callback interface). We may be able to do without depending on any external stub.

@github-actions

This comment has been minimized.

@AlexWaygood
Copy link
Member

Is there an equivalent pyright setting that we should also enable?

@Avasam
Copy link
Sponsor Collaborator Author

Avasam commented Jan 12, 2023

Is there an equivalent pyright setting that we should also enable?

Pyright has reportUntypedBaseClass, but I think it only cares about Unknown. It's also already turned on.

@Avasam
Copy link
Sponsor Collaborator Author

Avasam commented Jan 12, 2023

Not sure how I can fix this one in stdlib/distutils
https://github.com/python/typeshed/actions/runs/3898147001/jobs/6656579675#step:5:17

stdlib/distutils/command/check.pyi:9: error: Class cannot subclass "_Reporter" (has type "Any") [misc]

@JelleZijlstra
Copy link
Member

Right, that is a part of the stdlib that depends on a third-party package (docutils). Given that distutils is deprecated anyway, I would just type ignore that one and not spend more time on it.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@AlexWaygood
Copy link
Member

AlexWaygood commented Jan 13, 2023

One thing I worry about a little bit here: some of these have been pretty difficult to fix, and some others have been easy but non-obvious (e.g. the fix for sqlalchemy was just to delete a bunch of code 😉).

The audit you've been doing here has been really great for improving the quality of typeshed's stubs, so whatever we decide here, you've done some fantastic work! But if we disallow subclassing Any altogether, does that maybe make it overly difficult for contributors to add new stubs to typeshed? As we've seen, there are a lot of different reasons people might have for subclassing Any, especially in stubs that have just been added to typeshed, which are usually incomplete.

I'm undecided on this point — very interested in hearing other people's thoughts! In general, I'm extremely on board with making our tests stricter wherever possible, so I'm somewhat torn :)

@Avasam
Copy link
Sponsor Collaborator Author

Avasam commented Jan 13, 2023

My thoughts:
In cases too complex. You can suppress the error and you're not any worse that you were before this change. As long as it's marked as incomplete.

_BaseClass: TypeAlias = Incomplete  # Explicitly marked as incomplete

# Some comment explaining why
class SomeClass(_BaseClass): ...  # type: ignore[misc]

Which is exactly what happened in pillow.

Now you'd at least get the warning from mypy. Which is helpful.

@AlexWaygood
Copy link
Member

AlexWaygood commented Jan 13, 2023

Yup. But my worry is that if we say "just type: ignore it away" every time somebody comes across a tricky case — then in that case, there's maybe little point in having mypy emit an error at all. It's not like we were ever going out of our way to subclass Any beforehand — all of these were, in a sense, special cases that were tricky or non-obvious to solve for some reason, and we always knew that we were "doing a bad thing" by subclassing Any. It's just that nobody had the time or inclination to go through them and fix the fixable ones (until now!).

So are there any cases where having mypy emit the error would have changed the way we would have approached writing the stub in the first instance? I'm not sure.

@github-actions

This comment has been minimized.

@Avasam Avasam marked this pull request as ready for review February 1, 2023 03:55
@Avasam
Copy link
Sponsor Collaborator Author

Avasam commented Feb 1, 2023

So are there any cases where having mypy emit the error would have changed the way we would have approached writing the stub in the first instance? I'm not sure.

For what its worth: the regression tests do fail on sublcassing any. I had to type: ignore one in https://github.com/python/typeshed/pull/9596/files?diff=unified&w=1#diff-9889184b00f6a93854ecfa4d52c58c2a2275a5b6c413ef48728b125846b69b3bR16

There are still the following that would need to be completed first:

@github-actions

This comment has been minimized.

@srittau
Copy link
Collaborator

srittau commented Feb 1, 2023

I share @AlexWaygood's concerns. There are valid reason to subclass Any and it's often useful as a temporary workaround. I don't see what we gain by enabling the flag in typeshed. That doesn't mean that replacing all the Anys hasn't been very valuable!

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants