Introduce AppKey for type safety

[Dreamsorcerer]

Will create some PRs in other projects shortly to test it all out properly. But, this looks promising so far. It's less intrusive than the previous proposal and still works with Request.config_dict.

[asvetlov]

If it makes linter problems, we can drop inheritance from Mapping / MutableMapping.
Strictly speaking, the proposed structure is a heterogeneous container; typing.Mapping[K, V] is more likely a homogenous version.
Mypy should still be smart enough to infer types from __getitem__ / __setitem__, isn't it?
The support of runtime collections.abc check is still desirable. It can be done either by inheritance or by direct collections.abc.mapping.register() call -- both works to me.

[Dreamsorcerer]

If it makes linter problems, we can drop inheritance from Mapping / MutableMapping.

I'll just drop a type: ignore in, should work otherwise. I was just hoping to get a better approach from python/mypy#11949

[Dreamsorcerer]

So, it doesn't work as nicely as I'd like.

AppKey("foo", Iterable[int]) won't work as it's not a concrete class. So, instead we probably have to do it like the Stash implementation as AppKey[Iterable[int]]("foo").

Another issue is that the typing errors from mypy are rather difficult to work with. For example, this code in aiohttp-jinja2:

app[aiohttp_jinja2.APP_CONTEXT_PROCESSORS_KEY] = (
    aiohttp_jinja2.request_processor,
    processor,
)

Produces:

Cannot infer type argument 1 of "__setitem__" of "Application"  [misc]

But, it works if you write it as:

f: Tuple[aiohttp_jinja2._ContextProcessor, ...] = (
    aiohttp_jinja2.request_processor,
    processor,
)
app[aiohttp_jinja2.APP_CONTEXT_PROCESSORS_KEY] = f

Even with simple examples like:

k = web.AppKey[int]("k")
app[k] = "foo"

Mypy just produces: Cannot infer type argument 1 of "__setitem__" of "Application" [misc]

Which is rather awkward for the user to figure out the problem. The type of both items is defined as a TypeVar (def __setitem__(self, key: AppKey[_T], value: _T)), so mypy only tells you if they are not the same type, but it doesn't tell you that the value type is the problem or what the expected type is.

[Dreamsorcerer]

Anyway, changes for aiohttp-jinja2: https://github.com/aio-libs/aiohttp-jinja2/pull/534/files

[Dreamsorcerer]

AppKey("foo", Iterable[int]) won't work as it's not a concrete class.

Weirdly, mypy stop complaining about this once the parameter is defined with Optional. I'd rather like to keep this syntax, so I've added support for both.

[lgtm-com]

This pull request introduces 2 alerts when merging a7cd6b7 into 30e9d7f - view on LGTM.com

new alerts:

• 2 for Variable defined multiple times

[lgtm-com]

This pull request introduces 2 alerts when merging 5c9aa9c into 30e9d7f - view on LGTM.com

new alerts:

• 2 for Variable defined multiple times

[lgtm-com]

This pull request introduces 2 alerts when merging de82d2b into 30e9d7f - view on LGTM.com

new alerts:

• 2 for Variable defined multiple times

[lgtm-com]

This pull request introduces 2 alerts when merging 7fbdff9 into 30e9d7f - view on LGTM.com

new alerts:

• 2 for Variable defined multiple times

[codecov]

Codecov Report

Merging #6498 (ab43733) into master (af7bf90) will decrease coverage by 0.02%.
The diff coverage is 90.32%.

@@            Coverage Diff             @@
##           master    #6498      +/-   ##
==========================================
- Coverage   93.36%   93.34%   -0.03%     
==========================================
  Files         104      104              
  Lines       30496    30623     +127     
  Branches     3067     3080      +13     
==========================================
+ Hits        28472    28584     +112     
- Misses       1850     1863      +13     
- Partials      174      176       +2     

Flag	Coverage Δ
unit	93.26% <89.67%> (-0.03%)	⬇️

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

Impacted Files	Coverage Δ
aiohttp/web_app.py	94.86% <78.12%> (-2.48%)	⬇️
aiohttp/helpers.py	95.75% <86.20%> (-1.19%)	⬇️
aiohttp/web.py	99.16% <100.00%> (+<0.01%)	⬆️
tests/test_web_app.py	99.47% <100.00%> (+0.05%)	⬆️
tests/test_web_functional.py	97.96% <100.00%> (+<0.01%)	⬆️
aiohttp/connector.py	93.93% <0.00%> (+<0.01%)	⬆️
tests/test_connector.py	95.72% <0.00%> (+0.01%)	⬆️
tests/test_test_utils.py	98.31% <0.00%> (+0.02%)	⬆️

---

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 af7bf90...ab43733. Read the comment docs.

[lgtm-com]

This pull request introduces 2 alerts when merging 4a38645 into 58da337 - view on LGTM.com

new alerts:

• 2 for Variable defined multiple times

[asvetlov]

LGTM.
@Dreamsorcerer please feel free to merge when you decide that the PR is ready.

[Dreamsorcerer]

Right, I'm going to merge this and hope that mypy improves its error message in future. I opened an issue for it: python/mypy#12012

[patchback]

Backport to 3.9: 💔 cherry-picking failed — conflicts found

❌ Failed to cleanly apply a1d4dac on top of patchback/backports/3.9/a1d4dac1d63ad64295d357c8097650e1ef63b9c9/pr-6498

Backporting merged PR #6498 into master

1. Ensure you have a local repo clone of your fork. Unless you cloned it
   from the upstream, this would be your origin remote.
2. Make sure you have an upstream repo added as a remote too. In these
   instructions you'll refer to it by the name upstream. If you don't
   have it, here's how you can add it:

   $ git remote add upstream https://github.com/aio-libs/aiohttp.git

3. Ensure you have the latest copy of upstream and prepare a branch
   that will hold the backported code:

   $ git fetch upstream
   $ git checkout -b patchback/backports/3.9/a1d4dac1d63ad64295d357c8097650e1ef63b9c9/pr-6498 upstream/3.9

4. Now, cherry-pick PR Introduce AppKey for type safety #6498 contents into that branch:

   $ git cherry-pick -x a1d4dac1d63ad64295d357c8097650e1ef63b9c9

   If it'll yell at you with something like fatal: Commit a1d4dac1d63ad64295d357c8097650e1ef63b9c9 is a merge but no -m option was given., add -m 1 as follows intead:

   $ git cherry-pick -m1 -x a1d4dac1d63ad64295d357c8097650e1ef63b9c9

5. At this point, you'll probably encounter some merge conflicts. You must
   resolve them in to preserve the patch from PR Introduce AppKey for type safety #6498 as close to the
   original as possible.
6. Push this branch to your fork on GitHub:

   $ git push origin patchback/backports/3.9/a1d4dac1d63ad64295d357c8097650e1ef63b9c9/pr-6498

7. Create a PR, ensure that the CI is green. If it's not — update it so that
   the tests and any other checks pass. This is it!
   Now relax and wait for the maintainers to process your pull request
   when they have some cycles to do reviews. Don't worry — they'll tell you if
   any improvements are necessary when the time comes!

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

[eoghanmurray]

What if I don't care about types (because I'm using Python, not Typescript)?
I want to do

app['redis'] = RedisCluster()
app['port'] = 30000

Without having to figure out why my app is warning about AppKeys etc.?
Specifically IMO the warning UserWarning: It is recommended to use web.AppKey instances for keys. would be an appropriate warning given by mypy or some tool as it is running type checking over an app, it is inappropriate for an application startup warning.

[Dreamsorcerer]

Firstly, it's a warning, just filter it out if you decide it's not relevant for you. Secondly, it's not possible to create a warning in mypy for something like this (especially retroactively, while maintaining backwards-compatibility).

I'll add a note to that section with examples to filter out the warning, but it's just a standard warning. We added a custom class, so you can filter out that specific warning very easily.

[Dreamsorcerer]

Also, even if you don't use mypy, a lot of users still want annotations etc. in their IDE via pyright or similar.

[Dreamsorcerer]

https://aiohttp--7839.org.readthedocs.build/en/7839/web_advanced.html#application-s-config

[eoghanmurray]

Thanks for your response!

Just filter it out

To do that I have to figure out what the warning means, and whether it's something that I need to fix. In this case the fix (using a WebKey class when both setting and retrieving, and figuring out types for values which would then need to be maintained) seems quite un-pythonic and would make the code more mysterious.

I did already try to filter it out before posting here (I didn't previously know if filtering warnings was possible in Python) but the following code taken from SO didn't work

with warnings.catch_warnings():
    warnings.simplefilter("ignore")
    app['redis'] = RedisCluster()
    app['port'] = 30000

As I now have learned that you need to set up the 'ignore' before app = web.Application() is initialized.

I just think that this is something that should be highlighted as best practices in the documentation and doesn't merit a warning.
With thanks for your time.

[Dreamsorcerer]

with warnings.catch_warnings():
    warnings.simplefilter("ignore")
    app["foo"] = 3

Works fine for me.

Anyway, if you don't care at all, filter it globally, examples in the above link. I'd use catch_warnings() if you're only trying to maintain backwards-compatibility with something (which is likely what homeassistant will be doing).

[eoghanmurray]

My version is 3.9.0b0 which doesn't appear to have #7677 in it "Only display warning when using -Wdefault, -We, -X dev or similar."

So basically my 'observations' are redundant I think (I didn't have warnings explicitly turned on).

[Dreamsorcerer]

Ah, yes, I didn't realise your were on b0. I did mention that the warning shouldn't appear unless you're explicitly enabling them with -Wall or similar. That was a bug, as I thought it would be ignored by default.

rc0 is building now.

[eoghanmurray]

I apologize for wasting your time, 3.9.0b1 works perfect in this respect.