Remove overlapping slots and fix broken slots inheritance

[ariebovenberg]

What do these changes do?

There were a few __slots__ related mistakes:

python -m slotscheck aiohttp -v
ERROR: 'aiohttp.web_protocol:RequestHandler' defines overlapping slots.
       - _reading_paused (aiohttp.base_protocol:BaseProtocol)
ERROR: 'aiohttp.web_response:StreamResponse' has slots but superclass does not.
       - aiohttp.helpers:CookieMixin

Fixing the second error was more problematic than it seems. The straightforward solution of adding __slots__ = ('_cookies', ) was not possible, because this triggered a TypeError: multiple bases have instance lay-out conflict in HTTPException since non-emptly slots cannot be combined with an Exception case class:

>>> class CookieMixin:
...  __slots__ = ('_cookies', )
...
>>> class HTTPException(CookieMixin, Exception):
...  pass
...
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: multiple bases have instance lay-out conflict

There are several possible solutions:

1. put empty slots in CookieMixin, since it's an abstract class. This is the currently implemented solution.
2. leave the situation as-is. The __slots__ in StreamResponse will be mostly ineffective.
3. remove __slots__ from StreamResponse. They aren't saving memory or preventing __dict__ from being created.

Are there changes in behavior for the user?

StreamResponse will no longer have a __dict__, so arbitrary attributes cannot be set. This is not documented, so likely not a problem

Related issue number

No issue. This is a small change.

Checklist

• I think the code is well written
• Unit tests for the changes exist n/a (see questions below)
• Documentation reflects the changes n/a
• 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."

Questions

• regarding tests: probably not needed. I discovered the slots issue with slotscheck, a tool I wrote. Of course, If you like, I can add it to tox/CI as I've done for instagram/LibCST and dry-python/returns.

[Dreamsorcerer]

Would be great if you could add this test to the linter step in CI. Then we can avoid introducing new overlaps in future.

[ariebovenberg]

@Dreamsorcerer You mean in workflows/ci.yaml?

    - name: Run linters
      run: |
        make mypy
        make slotscheck  # after adding slotscheck to Makefile

Alternatively I can add the hook to the .pre-commit-config.yaml. Whichever you prefer.

[Dreamsorcerer]

@Dreamsorcerer You mean in workflows/ci.yaml?

    - name: Run linters
      run: |
        make mypy
        make slotscheck  # after adding slotscheck to Makefile

That looks fine to me. Not sure it needs to go in the Makefile. But, I'd make it a separate step (and probably rename that existing step to 'Run Mypy'). I meant lint job earlier, rather than linter step..

[ariebovenberg]

That looks fine to me. Not sure it needs to go in the Makefile. But, I'd make it a separate step (and probably rename that existing step to 'Run Mypy'). I meant lint job earlier, rather than linter step.

Can do. I'm curious though: what's the reasoning behind what goes in pre-commit vs GitHub workflow?

[codecov]

Codecov Report

Merging #6547 (32ce395) into master (21aff1f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #6547   +/-   ##
=======================================
  Coverage   93.36%   93.36%           
=======================================
  Files         104      104           
  Lines       30506    30509    +3     
  Branches     3068     3069    +1     
=======================================
+ Hits        28482    28485    +3     
  Misses       1850     1850           
  Partials      174      174           

Flag	Coverage Δ
unit	93.29% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
aiohttp/web_protocol.py	88.31% <ø> (ø)
aiohttp/web_response.py	98.41% <ø> (ø)
aiohttp/helpers.py	96.94% <100.00%> (+<0.01%)	⬆️
tests/test_helpers.py	98.96% <100.00%> (+<0.01%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 21aff1f...32ce395. Read the comment docs.

[Dreamsorcerer]

That looks fine to me. Not sure it needs to go in the Makefile. But, I'd make it a separate step (and probably rename that existing step to 'Run Mypy'). I meant lint job earlier, rather than linter step.

Can do. I'm curious though: what's the reasoning behind what goes in pre-commit vs GitHub workflow?

Probably a question for someone else. I'd assume that pre-commit is being run as a git hook, so it makes sense to have primarily fast checks which are likely to catch mistakes. If it only catches an error for 1 in 500 PRs or something, it's probably best not delaying a developer's commit. But, not sure if that's what has been done so far, I didn't set up anything in pre-commit and don't really mind which way it goes in.

[ariebovenberg]

That looks fine to me. Not sure it needs to go in the Makefile. But, I'd make it a separate step (and probably rename that existing step to 'Run Mypy'). I meant lint job earlier, rather than linter step.

Can do. I'm curious though: what's the reasoning behind what goes in pre-commit vs GitHub workflow?

Probably a question for someone else. I'd assume that pre-commit is being run as a git hook, so it makes sense to have primarily fast checks which are likely to catch mistakes. If it only catches an error for 1 in 500 PRs or something, it's probably best not delaying a developer's commit. But, not sure if that's what has been done so far, I didn't set up anything in pre-commit and don't really mind which way it goes in.

Indeed, problems with slots probably won't occur in most PRs. I'll add it to the GH workflow for now.

[asvetlov]

Thanks!