#11795 Update the return type of __aexit__

[euresti]

Scope and purpose

Fixes #11795

Specifying Literal[False] tells the type checker that this context manager doesn't suppress exceptions.

Contributor Checklist:

This process applies to all pull requests - no matter how small.
Have a look at our developer documentation before submitting your Pull Request.

Below is a non-exhaustive list (as a reminder):

• The title of the PR should describe the changes and starts with the associated issue number, like “#9782 Remove twisted.news. #1234 Brief description”.
• A release notes news fragment file was create in src/twisted/newsfragments/ (see: Release notes fragments docs.)
• The automated tests were updated.
• Once all checks are green, request a review by leaving a comment that contains exactly the string please review.
  Our bot will trigger the review process, by applying the pending review label
  and requesting a review from the Twisted dev team.

[euresti]

please review

[adiroiban]

Many thanks for the changes.

Is this a bugfix or a feature?

We have a good set of automated tests and mypy is part of the test suite.

Why was an error similar to foo.py:3: error: Missing return statement not already detected by our tests.

Would it be possible to have a test that will make sure that we will not introduce a regression with future changes?

It looks like the code has coverage ... so I think that tests are executed.
it might be that we don't have annotations in the tests or we don't run mypy on the test code.

The normal dev process require an updated test for every code change.

I think that is ok to merge this without an updated test.

But I am leaving this for review, so that another Twisted dev can change the change and approve the "missing tests" policy exception for this PR.

Or if you can find a way to run a mypy test as part of the changes from this PR, then we can merge without the need for a review from another dev.

Thanks again!

[DMRobertson]

I note in passing that the annotations for exc_type, exc_val and exc_tb types look wrong here, see e.g. https://github.com/python/typeshed/blob/a7cf6144deeb3f99570f8b97fb5f5ecf16f20c01/stdlib/contextlib.pyi#L52-L54

[exarkun]

A good point in favor of having some kind of test for these changes.

[exarkun]

These annotations still look wrong.

[euresti]

Oh sorry. I was focusing so much on the testing I missed this comment. I can update that, but I'm not sure how to make mypy complain about this annotation in order to "test" it.

[exarkun]

Nor I :/

[euresti]

Done!

[euresti]

Thanks for the review.

Is this a bugfix or a feature?

I wasn't sure what to call it but saw that #11754 called it a feature. Though bugfix might be more reasonable and can change it to that.

Why was an error similar to foo.py:3: error: Missing return statement not already detected by our tests.

Currently mypy is tested by running mypy over the entire codebase. And this error only triggers in the following two ways:

async def blah() -> int:
    async with DeferredLock():
        return 15

or

async def blah(): -> int
    x = None
    async with DeferredLock():
        x = 15
    return x

The 2nd one complains because x might be None.

Weirdly the following doesn't get a complaint from mypy:

async def blah() -> None
   async with DeferredLock():
      x = 15
   y = x + 15

Which is the way our tests use it in src.twisted.test.test_defer.CoroutineContextVarsTests.test_asyncWithLock

Note: This seems to be a common pattern in mypy. Check out the following:

def missing_return(x: int) -> int:  # error: Missing return statement
    if x == 15:
        return 22

def incompatible_return(x: int) -> int:
    y = None
    if x == 15:
        y = 22
    return y  # error: Incompatible return value type (got "Optional[int]", expected "int")

def wat(x: int) -> int: # No Error
    if x == 15:
        y = 22
    return y

Hmm. I can trigger the error in that test (test_asyncWithLock) by adding d = None at the top of that test. Is that a reasonable thing to do? I don't know if other linters will get mad at me though.

As for full mypy testing, I've seen other projects use something like https://pypi.org/project/pytest-mypy-plugins/ or https://pypi.org/project/pytest-mypy-testing/ to test their stubs. I don't know if you'd rather have that, but I don't know anything about how this project is tested.

[adiroiban]

Thanks for the update.

If you change the test code and run the tox -e mypy using the "production" code from trunk and the failure is raised, I would say that the test changes are good enough.
It would help to add a comment that the value is initialized in order to help with mypy testing.

[euresti]

Here's the error before:

$ tox -e mypy
ROOT: will run in automatically provisioned tox, host .ve/bin/python3.10 is missing [requires (has)]: tox-wheel>=0.6.0, tox<4 (4.2.6)
ROOT: provision> .tox/.tox/bin/python -m tox -e mypy
mypy wheel-make: cleaning up build directory ...
mypy wheel-make: commands[0] | pip wheel /Users/david/src/twisted --no-deps --use-pep517 --wheel-dir /Users/david/src/twisted/.tox/dist
Processing /Users/david/src/twisted
  Installing build dependencies ... done
  Getting requirements to build wheel ... done
  Preparing metadata (pyproject.toml) ... done
Building wheels for collected packages: Twisted
  Building wheel for Twisted (pyproject.toml) ... done
  Created wheel for Twisted: filename=twisted-22.10.0.post0-py3-none-any.whl size=3134001 sha256=39e49cf401fdb3cf670f3de45e3e8658452199dcb0a15292b6c78f140afb6a4f
  Stored in directory: /private/var/folders/fr/q0q8chgs22712h22ltc3120c0000gn/T/pip-ephem-wheel-cache-wgj9ueg5/wheels/d3/50/16/420f1687791215aa8ac00e4bc08ef149305c89caf9033f2a7f
Successfully built Twisted
mypy inst-nodeps: /Users/david/src/twisted/.tox/.tmp/package/1/twisted-22.10.0.post0-py3-none-any.whl
mypy installed: alabaster==0.7.12,appdirs==1.4.4,astor==0.8.1,astroid==2.13.2,attrs==22.2.0,Automat==22.10.0,Babel==2.11.0,bcrypt==4.0.1,CacheControl==0.12.11,certifi==2022.12.7,cffi==1.15.1,charset-normalizer==2.1.1,click==8.1.3,click-default-group==1.2.2,ConfigArgParse==1.5.3,constantly==15.1.0,coverage==6.6.0b1,cryptography==39.0.0,cython-test-exception-raiser==1.0.2,dill==0.3.6,docutils==0.19,exceptiongroup==1.1.0,h2==4.1.0,hpack==4.0.0,hyperframe==6.0.1,hyperlink==21.0.0,hypothesis==6.62.0,idna==3.4,imagesize==1.4.1,incremental==22.10.0,isort==5.11.4,Jinja2==3.1.2,lazy-object-proxy==1.9.0,lockfile==0.12.2,lunr==0.6.2,MarkupSafe==2.1.1,mccabe==0.7.0,msgpack==1.0.4,mypy==0.981,mypy-extensions==0.4.3,mypy-zope==0.3.11,packaging==23.0,platformdirs==2.6.2,priority==1.3.0,pyasn1==0.4.8,pyasn1-modules==0.2.8,pycparser==2.21,pydoctor==22.9.1,pyflakes==2.5.0,Pygments==2.14.0,PyHamcrest==2.0.4,pylint==2.15.10,PyNaCl==1.5.0,pyOpenSSL==23.0.0,pyserial==3.5,pytz==2022.7,readthedocs-sphinx-ext==2.2.0,requests==2.28.1,service-identity==21.1.0,six==1.16.0,snowballstemmer==2.2.0,sortedcontainers==2.4.0,Sphinx==5.3.0,sphinx-rtd-theme==0.5.1,sphinxcontrib-devhelp==1.0.2,sphinxcontrib-htmlhelp==2.0.0,sphinxcontrib-jsmath==1.0.1,sphinxcontrib-qthelp==1.0.3,sphinxcontrib-serializinghtml==1.1.5,sphinxcontrib.applehelp==1.0.3,toml==0.10.2,tomli==2.0.1,tomlkit==0.11.6,towncrier==22.12.0,Twisted @ file:///Users/david/src/twisted/.tox/.tmp/package/1/twisted-22.10.0.post0-py3-none-any.whl,twistedchecker==0.7.4,types-cryptography==3.3.23.2,types-docutils==0.19.1.1,types-pyOpenSSL==23.0.0.0,types-setuptools==65.6.0.3,typing_extensions==4.4.0,urllib3==1.26.13,wrapt==1.14.1,zope.event==4.6,zope.interface==5.5.2,zope.schema==7.0.1
mypy run-test-pre: PYTHONHASHSEED='2503996487'
mypy run-test: commands[0] | mypy --cache-dir=/Users/david/src/twisted/.tox/mypy_cache --pretty src
src/twisted/test/test_defer.py:3528:25: error: Item "None" of "Optional[Deferred[DeferredLock]]" has no attribute "called"  [union-attr]
            self.assertTrue(d.called)
                            ^~~~~~~~
src/twisted/test/test_defer.py:3529:9: error: Incompatible types in "await" (actual type "Optional[Deferred[DeferredLock]]", expected type "Awaitable[Any]")  [misc]
            await d
            ^~~~~~~
Found 2 errors in 1 file (checked 850 source files)
ERROR: InvocationError for command /Users/david/src/twisted/.tox/mypy/bin/mypy --cache-dir=/Users/david/src/twisted/.tox/mypy_cache --pretty src (exited with code 1)
______________________________________________________________________________________________________ summary ______________________________________________________________________________________________________
ERROR:   mypy: commands failed
➜ 

[glyph]

This looks good to me. There's a broader conversation to be had about how to verify type annotations separately from behavior, and particularly how to verify that application code will get errors sometimes, but I think that this has been conscientiously verified as well as we can with our current tools. Let's land!

[euresti]

Is anything else missing?

[adiroiban]

I have enabled auto-merge for this since Glyph has approved it.

Many thanks David for the patch and sorry for the delay.