Fix __exit__ return type of various context managers

[Enegg]

Changes

Fixes #847.

A few context managers which can suppress exceptions had their __exit__ method's return type incorrectly annotated as bool | None, where according to typing docs it should be bool.

I've also included changes for bool | None -> None in places where bool is never returned. This shouldn't have any effect.

Checklist

If this is a user-facing code change, like a bugfix or a new feature, please ensure that
you've fulfilled the following conditions (where applicable):

• You've added tests (in tests/) added which would fail without your patch
• You've updated the documentation (in docs/, in case of behavior changes or new
  features)
• You've added a new changelog entry (in docs/versionhistory.rst).

If this is a trivial change, like a typo fix or a code reformatting, then you can ignore
these instructions.

Updating the changelog

If there are no entries after the last release, use **UNRELEASED** as the version.
If, say, your patch fixes issue #123, the entry should look like this:

- Fix big bad boo-boo in task groups
  (`#123 <https://github.com/agronholm/anyio/issues/123>`_; PR by @yourgithubaccount)

If there's no issue linked, just link to your pull request instead by updating the
changelog after you've created the PR.

[Enegg]

There's one more context manager which could have its return type adjusted:
https://github.com/Enegg/anyio/blob/Enegg/issue847/src/anyio/from_thread.py#L126-L134
Its return type is dependent on the type of the async_cm.__exit__ that the class proxies. This would require a type var to be used in AbstractAsyncContextManager[T_co, <ExitT>].
I haven't included that yet as I am not sure if that's a backwards compatible change (Abstract[Async]ContextManager didn't always have a second generic variable iirc)

[agronholm]

Just a little change request, and a couple minor questions.

[agronholm]

Did the pre-commit checks pass for you locally?

[Enegg]

Did the pre-commit checks pass for you locally?

I did not run pre-commit locally. I see that mypy is failing in three places unrelated to my PR.
As for

src/anyio/_backends/_trio.py:190: error: Incompatible return value type (got "bool | None", expected "bool") [return-value]

This seems to be a problem in Trio itself. trio.open_nursery is typed as returning -> AbstractAsyncContextManager[Nursery], which due to a missing second generic type defaults to bool | None. The actual context manager returned from that function is properly typed as -> bool.
https://github.com/python-trio/trio/blob/main/src/trio/_core/_run.py#L1029

[agronholm]

Did the pre-commit checks pass for you locally?

I did not run pre-commit locally. I see that mypy is failing in three places unrelated to my PR. As for

src/anyio/_backends/_trio.py:190: error: Incompatible return value type (got "bool | None", expected "bool") [return-value]

This seems to be a problem in Trio itself. trio.open_nursery is typed as returning -> AbstractAsyncContextManager[Nursery], which due to a missing second generic type defaults to bool | None. The actual context manager returned from that function is properly typed as -> bool. https://github.com/python-trio/trio/blob/main/src/trio/_core/_run.py#L1029

Why am I not seeing any pre-commit failures on latest master then?

$ pre-commit run -a
yamllint.................................................................Passed
check toml...............................................................Passed
check yaml...............................................................Passed
debug statements (python)................................................Passed
fix end of files.........................................................Passed
mixed line ending........................................................Passed
trim trailing whitespace.................................................Passed
ruff.....................................................................Passed
ruff-format..............................................................Passed
mypy.....................................................................Passed
rst ``code`` is two backticks............................................Passed
rst directives end with two colons.......................................Passed
rst ``inline code`` next to normal text..................................Passed

[Enegg]

Okay, correction:
Those places are related to the PR.

Mypy is failing as it now sees code within with CancelScope differently - the three other places it fails in have a return statement within the with block. Any type checker given a suppressing cm will now assume the return may not be reached on all code paths (due to an exception), but the code outside the cm block is still reachable (as the cm may suppress the exception).

With that said, this becomes a non-trivial problem typing-wise; I can see that those CancelScopes do not have a deadline set, which means they won't actually be suppressing; there's hardly any way to communicate that to a type checker.

[Enegg]

Fixing 2 out of the 4 reports would require either # type: ignores, or refactoring the code flow so a return doesn't happen inside a CancelScope. The test case could be fixed by removing NoReturn. The 4th is a trio issue.

@agronholm, what would you have me do?

[agronholm]

Fixing 2 out of the 4 reports would require either # type: ignores, or refactoring the code flow so a return doesn't happen inside a CancelScope. The test case could be fixed by removing NoReturn. The 4th is a trio issue.

@agronholm, what would you have me do?

Dang, this turned out to be way harder than I thought. Could we reduce the scope of this PR to just fix the non-problematic CMs?

[Enegg]

Could we reduce the scope of this PR to just fix the non-problematic CMs?

Done, although this PR no longer fixes the issue I've opened.

I presume you'd still want to "fix" the issue at some point to be on pair with trio? (trio.CancelScope.__exit__ returns bool)

[agronholm]

Could we reduce the scope of this PR to just fix the non-problematic CMs?

Done, although this PR no longer fixes the issue I've opened.

I presume you'd still want to "fix" the issue at some point to be on pair with trio? (trio.CancelScope.__exit__ returns bool)

Before I do anything, let's take stock on the remaining changes you wanted to make. Which classes where did you still need to fix and what were the issues there?

[Enegg]

I wanted to fix CancelScope, which currently allows a footgun.
It turned out to be a non-trival fix, as depending on its usage (shield-only or not), one can expect a non-suppressing vs suppressing behavior, and the library already makes use of this duality in a few places.

Changing the return type from bool | None to bool removes this footgun:

with move_on_after(5):
    await sleep(10)
    result = 123
    
print(result)  # -> bool | None => no issues, -> bool => result may be undefined

but at the same time reports an issue for shield-only scopes (which never suppress at runtime):

with CancelScope(shield=True):
    await sleep(10)
    result = 123

print(result)  # this should be fine, but is now reported as potentially undefined

The issues reported by mypy were at places where a shield-only CancelScope was used, and the code flow depended on it not having suppressing behavior (a return happening within its block and the like).

[agronholm]

I wanted to fix CancelScope, which currently allows a footgun. It turned out to be a non-trival fix, as depending on its usage (shield-only or not), one can expect a non-suppressing vs suppressing behavior, and the library already makes use of this duality in a few places.

Changing the return type from bool | None to bool removes this footgun:

with move_on_after(5):
    await sleep(10)
    result = 123
    
print(result)  # -> bool | None => no issues, -> bool => result may be undefined

but at the same time reports an issue for shield-only scopes (which never suppress at runtime):

with CancelScope(shield=True):
    await sleep(10)
    result = 123

print(result)  # this should be fine, but is now reported as potentially undefined

The issues reported by mypy were at places where a shield-only CancelScope was used, and the code flow depended on it not having suppressing behavior (a return happening within its block and the like).

This will still suppress a cancellation exception:

with CancelScope(shield=True) as scope:
    scope.cancel()
    await sleep(10)
    result = 123

[Enegg]

This will still suppress a cancellation exception:

with CancelScope(shield=True) as scope:
    scope.cancel()
    await sleep(10)
    result = 123

Yes, but I meant specifically a CancelScope which only shields. For example:
https://github.com/Enegg/anyio/blob/dc82e5018c0f0237dbac4a81916c3f4c1d6f0731/src/anyio/to_process.py#L174-L179
(this was also one of the places where mypy reported issues)

[agronholm]

There is no way to tell a type checker that a cancel scope won't suppress an exception. The way I see, it, bool seems to be the only correct return type annotation.

[Enegg]

Technically there is, but it's hardly practical.

We could make CancelScope generic (probably over bool) and create overloads, like

@overload
def __init__(self: CancelScope[Literal[False]], shield: bool = ...) -> None: ...
@overload
def __init__(self: CancelScope[bool], shield: bool = ..., deadline: float = ...) -> None: ...

def cancel(self: CancelScope[Literal[True]] | CancelScope[bool]) -> None: ...  # not bindable on shield-only scope

@deadline.setter
def deadline(self: CancelScope[Literal[True]] | CancelScope[bool], value: float) -> None: ...

One could still create a cancellable CancelScope without initial deadline with cs = CancelScope[bool](...)

Either way, I think that the benefits of type checkers reporting reachability problems outweigh the false positive on CancelScope(shield=True).
It'd also be on pair with Trio, which CancelScope does return bool.

[agronholm]

Technically there is, but it's hardly practical.

We could make CancelScope generic (probably over bool) and create overloads, like

@overload
def __init__(self: CancelScope[Literal[False]], shield: bool = ...) -> None: ...
@overload
def __init__(self: CancelScope[bool], shield: bool = ..., deadline: float = ...) -> None: ...

def cancel(self: CancelScope[Literal[True]] | CancelScope[bool]) -> None: ...  # not bindable on shield-only scope

@deadline.setter
def deadline(self: CancelScope[Literal[True]] | CancelScope[bool], value: float) -> None: ...

One could still create a cancellable CancelScope without initial deadline with cs = CancelScope[bool](...)

Either way, I think that the benefits of type checkers reporting reachability problems outweigh the false positive on CancelScope(shield=True). It'd also be on pair with Trio, which CancelScope does return bool.

I don't understand your reasoning for these overloads. "not bindable on shield-only scope"? What does that even mean?
Are we in agreement on the fact that a type checker cannot determine from the constructed CancelScope whether it will suppress exceptions or not?

[agronholm]

Oh, wait, you meant that you would do with CancelScope[False](): ... and it would mark it as non-suppressing?

[agronholm]

If that's the case, then I think I got it. But I'm not very keen on that – I might consider it if Trio implements it, but not otherwise. At any rate, switching to bool on CancelScope.__exit__() seems like something we should do for now.

[Enegg]

But I'm not very keen on that

I am not suggesting going the Generic way (because it's not ergonomic), was just showing you it can technically be done.

switching to bool on CancelScope.__exit__() seems like something we should do for now

I agree with this.

Oh, wait, you meant that you would do with CancelScope[False](): ... and it would mark it as non-suppressing?

More or less. The point of the overloads is to determine the suppressing behavior during CS creation, so passing in a deadline automatically defaults to suppressing, and not otherwise.

"not bindable on shield-only scope"? What does that even mean?

Because of the extra annotation on self (CancelScope[Literal[True]] | CancelScope[bool]), a type checker will report an error on an attempt to use .cancel off a CancelScope[Literal[False]], and allow it otherwise:

[agronholm]

Ok, I'll merge this some time later then, provided the changes we spoke of have been added. Thanks!

[Enegg]

provided the changes we spoke of have been added

I have undone the changes I've made on CancelScope.__exit__, as they were causing mypy to report a few issues in places where CancelScope(shield=True) was used in the library (where the code flow is sound at runtime).

I can reintroduce them by silencing the issues with # type: ignore. Are you fine with that?

[agronholm]

Can you show me the mypy errors?

[Enegg]

I'm working with pyright on my side (which generates a lot of errors for the repo), and pre-commit only runs on staged files.
I'll redo the changes to CancelScope, commit/push and have mypy produce the errors after the checks run. Is that fine?

[agronholm]

Okay.

[Enegg]

OK, so:

src/anyio/to_process.py:38: error: Missing return statement [return]
src/anyio/_backends/_asyncio.py:2450: error: Missing return statement [return]

run_sync and run_sync_in_worker_thread have a return within with CancelScope(shield=...). When seen as a suppressing CM, the return may not be reached due to an exception, and the flow would continue reaching the end of the function (which implicitly returns None, which is incompatible with T_Retval)

run_sync_in_worker_thread seems to use the value of the scope later down, but only to grab its parent scope

tests/test_taskgroups.py:844: error: Implicit return in function which does not return [misc]

task is typed as NoReturn, but with a suppressing CM the control flow may not reach pytest.fail and instead hit an implicit return. Could be fixed by replacing NoReturn with None.

[agronholm]

Ok, I think I can deal with these (later).

[agronholm]

I couldn't figure out a better way to deal with two of these errors than to add those type ignore comments.

[Enegg]

That's fine, one of the purposes of # type: ignore is to be an escape hatch for situations the current type system cannot express, and this is clearly one right now.

If we wanted to conform to the current type system, the functionality of CancelScope would need to be split into two objects/CMs - one providing a shield (with __exit__ -> None), and one for cancellation (with __exit__ -> bool).