-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
ClientSession.timeout has an incorrect typing #6917
Comments
I'll certainly take a look over it, if you create a PR.
A |
A I'll try to prepare a PR later today then, or later this week if I won't be able to do so today. |
No, it literally just replaces the type, thus throwing away type safety:
There are certainly times where casting makes more sense, but both are inherently unsafe and are ignoring the actual issue. |
That's weird, I remember getting an error before when trying to straight up "replace" a type like you just shown. Strange. |
I have created #6923 to address this issue. |
This is a follow-up PR to #6917, which fixes the overly inclusive typing of `ClientSession.timeout` attribute, which can never actually end up as an `object` instance. This leftover probably exists from the time when there was the `_sentinel` value being assigned to the attribute (see "3.8 branch `self._timeout` assignment" from the linked issue), hence why it ended up this way. For untyped code, there's absolutely no change — the `self._timeout` attribute was never actually assigned the sentinel value to. Most of the explanation already exists in #6917, here's the test code snippet from there again: ```py import asyncio import aiohttp async def main: session = aiohttp.ClientSession(timeout=aiohttp.ClientTimeout(total=10)) # read back the total time attribute total_time = session.timeout.total # "object" type of "Union[object, ClientTimeout]" has no attribute "total" print(total_time) asyncio.run(main()) ``` For typed code, the current solution would be to `assert isinstance(session.timeout, aiohttp.ClientTimeout)` everywhere the attribute is being accessed. This PR removes this "unnecessary necessity". Co-authored-by: Sviatoslav Sydorenko <wk@sydorenko.org.ua> Fixes #6917 PR #6923 (cherry picked from commit 78fa040)
This is a follow-up PR to #6917, which fixes the overly inclusive typing of `ClientSession.timeout` attribute, which can never actually end up as an `object` instance. This leftover probably exists from the time when there was the `_sentinel` value being assigned to the attribute (see "3.8 branch `self._timeout` assignment" from the linked issue), hence why it ended up this way. For untyped code, there's absolutely no change — the `self._timeout` attribute was never actually assigned the sentinel value to. Most of the explanation already exists in #6917, here's the test code snippet from there again: ```py import asyncio import aiohttp async def main: session = aiohttp.ClientSession(timeout=aiohttp.ClientTimeout(total=10)) # read back the total time attribute total_time = session.timeout.total # "object" type of "Union[object, ClientTimeout]" has no attribute "total" print(total_time) asyncio.run(main()) ``` For typed code, the current solution would be to `assert isinstance(session.timeout, aiohttp.ClientTimeout)` everywhere the attribute is being accessed. This PR removes this "unnecessary necessity". Co-authored-by: Sviatoslav Sydorenko <wk@sydorenko.org.ua> Fixes #6917 PR #6923 (cherry picked from commit 78fa040)
…om the `ClientSession.timeout` type (#6924) **This is a backport of PR #6923 as merged into master (78fa040).** <!-- Thank you for your contribution! --> ## What do these changes do? <!-- Please give a short brief about these changes. --> This is a follow-up PR to #6917, which fixes the overly inclusive typing of `ClientSession.timeout` attribute, which can never actually end up as an `object` instance. This leftover probably exists from the time when there was the `_sentinel` value being assigned to the attribute (see "3.8 branch `self._timeout` assignment" from the linked issue), hence why it ended up this way. ## Are there changes in behavior for the user? <!-- Outline any notable behaviour for the end users. --> For untyped code, there's absolutely no change - the `self._timeout` attribute was never actually assigned the sentinel value to. Most of the explanation already exists in #6917, here's the test code snippet from there again: ```py import asyncio import aiohttp async def main: session = aiohttp.ClientSession(timeout=aiohttp.ClientTimeout(total=10)) # read back the total time attribute total_time = session.timeout.total # "object" type of "Union[object, ClientTimeout]" has no attribute "total" print(total_time) asyncio.run(main()) ``` For typed code, the current solution would be to `assert isinstance(session.timeout, aiohttp.ClientTimeout)` everywhere the attribute is being accessed. This PR removes this "unnecessary necessity". ## Related issue number <!-- Are there any issues opened that will be resolved by merging this change? --> #6917 ## Checklist - [x] I think the code is well written - [ ] Unit tests for the changes exist - [x] Documentation reflects the changes (documentation never mentioned an `object` instance being accessible from there even) - [ ] If you provide code modification, please add yourself to `CONTRIBUTORS.txt` * The format is <Name> <Surname>. * Please keep alphabetical order, the file is sorted by names. - [ ] Add a new news fragment into the `CHANGES` folder * name it `<issue_id>.<type>` for example (588.bugfix) * if you don't have an `issue_id` change it to the pr id after creating the pr * ensure type is one of the following: * `.feature`: Signifying a new feature. * `.bugfix`: Signifying a bug fix. * `.doc`: Signifying a documentation improvement. * `.removal`: Signifying a deprecation or removal of public API. * `.misc`: A ticket has been closed, but it is not of interest to users. * Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files." Co-authored-by: DevilXD <DevilXD@users.noreply.github.com>
…om the `ClientSession.timeout` type (#6925) **This is a backport of PR #6923 as merged into master (78fa040).** <!-- Thank you for your contribution! --> ## What do these changes do? <!-- Please give a short brief about these changes. --> This is a follow-up PR to #6917, which fixes the overly inclusive typing of `ClientSession.timeout` attribute, which can never actually end up as an `object` instance. This leftover probably exists from the time when there was the `_sentinel` value being assigned to the attribute (see "3.8 branch `self._timeout` assignment" from the linked issue), hence why it ended up this way. ## Are there changes in behavior for the user? <!-- Outline any notable behaviour for the end users. --> For untyped code, there's absolutely no change - the `self._timeout` attribute was never actually assigned the sentinel value to. Most of the explanation already exists in #6917, here's the test code snippet from there again: ```py import asyncio import aiohttp async def main: session = aiohttp.ClientSession(timeout=aiohttp.ClientTimeout(total=10)) # read back the total time attribute total_time = session.timeout.total # "object" type of "Union[object, ClientTimeout]" has no attribute "total" print(total_time) asyncio.run(main()) ``` For typed code, the current solution would be to `assert isinstance(session.timeout, aiohttp.ClientTimeout)` everywhere the attribute is being accessed. This PR removes this "unnecessary necessity". ## Related issue number <!-- Are there any issues opened that will be resolved by merging this change? --> #6917 ## Checklist - [x] I think the code is well written - [ ] Unit tests for the changes exist - [x] Documentation reflects the changes (documentation never mentioned an `object` instance being accessible from there even) - [ ] If you provide code modification, please add yourself to `CONTRIBUTORS.txt` * The format is <Name> <Surname>. * Please keep alphabetical order, the file is sorted by names. - [ ] Add a new news fragment into the `CHANGES` folder * name it `<issue_id>.<type>` for example (588.bugfix) * if you don't have an `issue_id` change it to the pr id after creating the pr * ensure type is one of the following: * `.feature`: Signifying a new feature. * `.bugfix`: Signifying a bug fix. * `.doc`: Signifying a documentation improvement. * `.removal`: Signifying a deprecation or removal of public API. * `.misc`: A ticket has been closed, but it is not of interest to users. * Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files." Co-authored-by: DevilXD <DevilXD@users.noreply.github.com>
Describe the bug
The
aiohttp.ClientSession.timeout
attribute has a type ofUnion[object, aiohttp.ClientTimeout]
, however the code logic will never actually assign a bareobject
type to theself._timeout
attribute, making this typing quite over-inclusive. Trying to use this attribute in typed code results in having to usecast(aiohttp.ClientTimeout, session.timeout)
, which is far from ideal considering one can just fix the typing in the library.I ran into this while using Python 3.8.10, but the exact same explanation above applies to the current master branch (and the version I'm using of course), as shown by the snippets below.
3.8 branch
__init__
parameter:aiohttp/aiohttp/client.py
Line 217 in 6243204
3.8 branch
self._timeout
assignment:aiohttp/aiohttp/client.py
Lines 261 to 290 in 6243204
Note the
# type: ignore
comment onL278
there - it's because thetimeout is sentinel
check does not narrow down thetimeout
type. The correct way to go about this would be to use acast
there instead of ignoring the issue like that.3.8 branch
timeout
attribute declaration:aiohttp/aiohttp/client.py
Lines 1029 to 1032 in 6243204
Master branch
__init__
parameter:aiohttp/aiohttp/client.py
Line 215 in 52fa599
Master branch
self._timeout
assignment:aiohttp/aiohttp/client.py
Lines 260 to 263 in 52fa599
Due to a different handling of the
sentinel
value via anEnum
member, nocast
is needed here.Master branch
timeout
attribute declaration:aiohttp/aiohttp/client.py
Lines 1008 to 1011 in 52fa599
The attribute type is still over-inclusive here though.
The solution would be quite simple:
Please let me know if you'd welcome a PR for this. I'd like to get this backported back to 3.8 (that I'm using) if possible, but if not, just fixing it in the master branch so that it's correct going forward would be good enough for me.
To Reproduce
Utilize some kind of a type checker like MyPy.
Expected behavior
The attribute having only the
aiohttp.ClientTimeout
type and not requiringcast
usage when accessing the attribute during library usage in user code.Logs/tracebacks
Python Version
Python 3.8.10
aiohttp Version
Version: 3.8.1
multidict Version
Version: 6.0.2
yarl Version
Version: 1.7.2
OS
Windows
Related component
Client
Additional context
Related issues and PRs:
#4191
#4193
Code of Conduct
The text was updated successfully, but these errors were encountered: