Added context manager mix-in classes

[agronholm]

Changes

This adds two new mix-in classes: ContextManagerMixin and AsyncContextManagerMixin. They're meant to enable users to write context manager classes in the same way as @contextmanager and @asynccontextmanager, respectively. The aim is to enable users to more easily embed cancel scopes and task groups into other context manager classes.

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.

[agronholm]

Open issues:

• Can't type annotate to return Self
• Should T be co/contravariant? (typeshed uses a covariant typevar in AbstractAsyncContextManager)
• This may require a new documentation section
• We should probably check for corner cases in the @asynccontextmanager implementation
• Reentrancy (must consider the case where the CM is entered twice before exiting) (disallowed re-entrancy)

[smurfix]

Well, there's no requirement for a context manager to return Self and the wrapper shouldn't enforce that. It should accept / forward whatever type __contextmanager__ is declared to return.

On the other hand, what this wrapper does not do is to ensure that the context is not entered twice, and that really is a bug.

[agronholm]

Well, there's no requirement for a context manager to return Self and the wrapper shouldn't enforce that. It should accept / forward whatever type contextmanager is declared to return.

I've already updated the docstring before you posted this. This version was never intended to be pushed to GH.

On the other hand, what this wrapper does not do is to ensure that the context is not entered twice, and that really is a bug.

Hm, I see what you mean. But rather than raise an error here, perhaps reentrancy should be properly supported instead?

[agronholm]

On second thought, I don't think it's feasible to support reentrancy, as there's no way to track which generator to use in __aexit__().

[agronholm]

I've thus disallowed reentrancy.

[smurfix]

I have been doing something like this in my own codebase for ages by now.

One problem, though: in a subclass, how do I "cleanly" call the superclass's __asynccontextmanager__? The best way I can come up with is

async with asynccontextmanager(super().__asynccontextmanager__)():
    ...

which is … meh. Anyway, whichever way we come up with needs documentation.

[agronholm]

I have been doing something like this in my own codebase for ages by now.

One problem, though: in a subclass, how do I "cleanly" call the superclass's __asynccontextmanager__? The best way I can come up with is

async with asynccontextmanager(super().__asynccontextmanager__)():
    ...

which is … meh. Anyway, whichever way we come up with needs documentation.

This is a very good point and we need to have an answer before going forward with this.

[agronholm]

I think we need to start by thinking what the ideal solution would look like, and then see if we can implement that.

[agronholm]

@Zac-HD mentioned elsewhere that he wanted to enforce the use of @asynccontextmanager on this special method, for the benefit of PEP 789 linter checks. If we do that, then this gets considerably cleaner: async with super().__asynccontextmanager__() as obj:

[smurfix]

Right.

Ideally we'd be able to simply do async with super() but that'd require some interesting core CPython changes AFAICT.

[agronholm]

I pushed those changes, but I encountered a different problem: How can we override the type parameter when subclassing? The pre-commit checks fail for this reason, and I don't know how to fix this.

[smurfix]

Huh. pyright doesn't understand this either, which frankly surprises me somewhat.
Maybe ask there?

[agronholm]

Huh. pyright doesn't understand this either, which frankly surprises me somewhat. Maybe ask there?

Doesn't understand what? The parent class pins the type variable to itself, and that class does not have the child_started and child_finished attributes, so it seems pretty clear to me why the mypy checks fail here. What I don't understand is how to improve the type annotations to get around this.

[agronholm]

There appears to be no way to pin the type variable to Self, which is exactly what we'd need here.

[tapetersen]

The problem here is that DummyContextManager binds the typevar concretely in its definition which sets the type for __enter__ unless we override that as well.

If it was allowed/defined and possible we would want to parametrize it directly with Self as:

class DummyContextManager(ContextManagerMixin[Self]):
    ...
    @contextmanager
    def __contextmanager__(self) -> Generator[Self]:
        ...

What does work here is the old workaround with a typevar with a bound as:

_TSelf = TypeVar("_TSelf", bound="DummyContextManager")
class DummyContextManager(ContextManagerMixin[_TSelf]):
    def __init__(self, handle_exc: bool = False) -> None:
        self.started = False
        self.finished = False
        self.handle_exc = handle_exc

    @contextmanager
    def __contextmanager__(self: _TSelf) -> Generator[_TSelf]:
        self.started = True
        try:
            yield self
        except RuntimeError:
            if not self.handle_exc:
                raise

        self.finished = True

That's of course not ideal, needing a declared typevar with correct bound for every class more or less, but the case of further extending the contextmanagers overriding the returned type maybe isn't that common either?

[agronholm]

The problem here is that DummyContextManager binds the typevar concretely in its definition which sets the type for __enter__ unless we override that as well.

If it was allowed/defined and possible we would want to parametrize it directly with Self as:

class DummyContextManager(ContextManagerMixin[Self]):
    ...
    @contextmanager
    def __contextmanager__(self) -> Generator[Self]:
        ...

What does work here is the old workaround with a typevar with a bound as:

_TSelf = TypeVar("_TSelf", bound="DummyContextManager")
class DummyContextManager(ContextManagerMixin[_TSelf]):
    def __init__(self, handle_exc: bool = False) -> None:
        self.started = False
        self.finished = False
        self.handle_exc = handle_exc

    @contextmanager
    def __contextmanager__(self: _TSelf) -> Generator[_TSelf]:
        self.started = True
        try:
            yield self
        except RuntimeError:
            if not self.handle_exc:
                raise

        self.finished = True

That's of course not ideal, needing a declared typevar with correct bound for every class more or less, but the case of further extending the contextmanagers overriding the returned type maybe isn't that common either?

By "does work" do you mean you get the correct type out of reveal_type() in an inherited CM? Can you show me?

[agronholm]

I don't think forcing context manager authors to use a typevar just to enable proper inheritance is a good solution to the problem. Let's see what the pyright folks have to say about this.

[tapetersen]

As in I checked out your branch but there are some issues with pyproject.toml parsing (the license checks but also some things about the tox config) so it was a bit hard to get the pre-commit to run.

Looking closer I can't get it to work without explicitly passing through the leaf-class as a typevar.

I suspect that getting this to work without explicit annotation when inheriting is not really possible with current typing system.

(The return type of __aenter__ in the mixin is basically dependent on a possibly non-self type-parameter in a future child-class)

Have added it as a pull request to your branch anyway but it's not really any better except maybe the default (and maybe demonstrating that it works if you specify the return type explicitly for every child)

[agronholm]

As in I checked out your branch but there are some issues with pyproject.toml parsing (the license checks but also some things about the tox config) so it was a bit hard to get the pre-commit to run.

What license checks? Pre-commit has no issues running over here, nor on CI.

I suspect that getting this to work without explicit annotation when inheriting is not really possible with current typing system.

That was my conclusion too.

[agronholm]

I've worked around this by adding specialized versions of the mix-in classes for yielding self. I'll post the code once I have 100% coverage again.

[agronholm]

It was just an old version of tox (4.5) that was picked up from outside the venv due to vscode's restoration of env-vars that messed up some things.

May still not hurt to add a requires in the tox section as specified here: https://tox.wiki/en/latest/config.html#pyproject-toml-native Wouldn't have solved this as the old version couldn't parse it anyway but for other incompatibilites it may be a good idea.

I originally skipped specifying that for precisely that reason.

[graingert]

oh this was slightly wrong it should have used pop_all()

I think it's worth including an (Async)ExitStack instruction on how to do this

[graingert]

I think you can alternatively put _ContextManagerMixin__cm on the _SupportsCtxMgr protocol

[graingert]

might be worth a test for a class that implements both AsyncContextManagerMixin and ContextManagerMixin

[agronholm]

I'm not sure why? The mix-in classes have no overlap whatsoever.

[agronholm]

Oh, I just realized: that's the problem. One could enter the CM synchronously AND asynchronously without first exiting.