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

Compatibility with Python 3.7 #284

Closed
Dreamsorcerer opened this issue Sep 22, 2022 · 21 comments
Closed

Compatibility with Python 3.7 #284

Dreamsorcerer opened this issue Sep 22, 2022 · 21 comments

Comments

@Dreamsorcerer
Copy link

Dreamsorcerer commented Sep 22, 2022

Is there any option to say that we need compatibility with Python 3.7? Or are we expected to add any incompatible error codes to the ignore list?

e.g. Y022 is not possible to fix until Python 3.9 is the minimum supported version (in our library).

@AlexWaygood
Copy link
Collaborator

AlexWaygood commented Sep 22, 2022

e.g. Y022 is not possible to fix until Python 3.9 is the minimum supported version (in our library).

In stubs, you can use PEP 585 syntax on Python versions < 3.9. flake8-pyi only supports stub files.

@Dreamsorcerer
Copy link
Author

Yep, just realised that, never mind... :P

@AlexWaygood
Copy link
Collaborator

Out of curiosity, did you only just add dependabot to multidict or something? That's a huge version bump!

@Dreamsorcerer
Copy link
Author

Looks like it's been trying to update since February (might have been broken or something before that): https://github.com/aio-libs/multidict/pulls?q=is%3Apr+flake8-pyi+is%3Aclosed
Main problem is just becoming aware that a PR is blocked due to a change in the dependency. We get so many emails about dependabot PRs, but none of those emails will tell you that a PR is blocked.

@Dreamsorcerer
Copy link
Author

Dreamsorcerer commented Dec 19, 2022

I'm reopening this, as it seems this isn't quite correct. After releasing multidict 6.0.3 we've been getting type errors in aiohttp if mypy is run with python <3.10.

Specifically, testing locally, these are the changes I've needed to revert to get the type checking passing again:
https://github.com/aio-libs/multidict/pull/798/files#diff-7a7ab10ef362af2c297ef848720b82cc38d7d40ce944923bcb8a2a5551a67631
Triggering Y026 and Y027.

The | syntax seems to be fine on any Python version, but other changes such as importing from collections.abc are not.

@AlexWaygood
Copy link
Collaborator

I'm reopening this, as it seems this isn't quite correct. After releasing multidict 6.0.3 we've been getting type errors in aiohttp if mypy is run with python <3.10.

This is somewhat surprising. We've been using PEP 585 syntax in typeshed for many months now (and mypy uses the same version of typeshed for checking code on Python 3.7 as it does on other versions of Python). If there were a compatibility issue here, I would have thought that we would have heard about it by now.

In any case, this seems more like a mypy bug than a flake8-pyi bug. Type checkers should allow modern syntax in stub files; if they don't, that should be reported as a bug to the type checker.

Still, I'm curious to see if I can reproduce this. Can you give me the full details of the Python version you're running mypy on, any environment variables you're setting, the flags you're passing to mypy, and the code mypy is failing on?

@Dreamsorcerer
Copy link
Author

Dreamsorcerer commented Dec 24, 2022

Here's an example of the CI failing: https://github.com/aio-libs/aiohttp/actions/runs/3733253503/jobs/6333799339

That's running on Python 3.9, it seems running mypy on any version of Python <3.10 results in errors.
Mypy config: https://github.com/aio-libs/aiohttp/blob/master/.mypy.ini
Everything else you need is in the repo too.

You can test locally by cloning aiohttp and type checking that. You'll find errors with multidict==6.0.3, no errors with the reverted changes in multidict==6.0.4.

I don't think it's a problem with the syntax, but the object implementations. e.g. If mypy is type checking with Python 3.7, then it doesn't recognise collections.abc.MutableMapping to be a Generic. So, we need to stick to typing.MutableMapping (or use typing_extensions), even in stubs.

@Dreamsorcerer
Copy link
Author

Which come to think of it, is exactly what is being done in typeshed, with sys.version_info checks.

@AlexWaygood
Copy link
Collaborator

Which come to think of it, is exactly what is being done in typeshed, with sys.version_info checks.

We import MutableMapping from collections.abc even in our 3.7 branches in typeshed.

@Dreamsorcerer
Copy link
Author

Yes, I take that back, just checked that. So, I'm lost why it is happening.

@AlexWaygood
Copy link
Collaborator

I'll see if I can spend some time looking at your repo, but tbh I have a lot of other things on right now, so there's a significantly higher chance I'll get to it if you're able to minimise the repro a little :)

@Dreamsorcerer
Copy link
Author

Dreamsorcerer commented Dec 24, 2022

On Python <3.10:

pip install aiohttp --upgrade  # Get dependencies installed
git clone https://github.com/aio-libs/aiohttp.git
cd aiohttp/
mypy  # No errors with latest multidict
pip install multidict==6.0.3
mypy  # Errors

@AlexWaygood
Copy link
Collaborator

"It is easy to reproduce this issue" ≠ "I have a minimal example".

@Dreamsorcerer
Copy link
Author

Uhh, no, I can't figure out a minimal reproducer, which is probably a bad sign...

@AlexWaygood
Copy link
Collaborator

Okay, thanks for trying. I'll see if I can figure out what's going on.

@JelleZijlstra
Copy link
Collaborator

Is there something left to do here? Doesn't sound like there's a reproducible flake8-pyi bug.

@Dreamsorcerer
Copy link
Author

It was reproducible with our repo: #284 (comment)
Just couldn't find a standalone reproducer.

@JelleZijlstra
Copy link
Collaborator

That sounds like a bug in mypy, not flake8-pyi.

@Dreamsorcerer
Copy link
Author

I'll leave that to you to decide, but it makes sense to me that mypy would give an error because collections.abc.MutableMapping is not Generic in Python 3.7 (as I mentioned at #284 (comment)).

@JelleZijlstra
Copy link
Collaborator

The log you link there no longer exists, unfortunately. However, collections.abc.MutableMapping should be generic for mypy on all versions (all it looks at is typeshed, where collections.abc.MutableMapping is an alias for https://github.com/python/typeshed/blob/7544b60db7b08249b1fef3663f0a68736660cae8/stdlib/typing.pyi#L636). If there is something in mypy giving false positive errors, then that's a mypy bug.

@AlexWaygood
Copy link
Collaborator

AlexWaygood commented Jun 17, 2023

Indeed — even in a .py file, mypy believes collections.abc.MutableMapping to be subscriptable at runtime on Python 3.7, as it should (as Jelle says, it's just going off what we tell it in the typeshed stubs): https://mypy-play.net/?mypy=latest&python=3.7&gist=2cb96b8e999e1e4e633af3cdd5a66556

I never got round to investigating the weirdness in your CI, I'm afraid, but I'm 95% sure that it wasn't a bug in flake8-pyi. As Jelle says, my best guess is that it's a bug in mypy

@AlexWaygood AlexWaygood closed this as not planned Won't fix, can't repro, duplicate, stale Jun 17, 2023
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

No branches or pull requests

3 participants