A case came up where I'd like a non-async context manager to be able to decorate both regular and async functions.  I wonder why contextlib.ContextDecorator doesn't just support both cases.

The implementation seems straightforward, switching on iscoroutinefunction().

I wonder why do you need it? What is your use case?
A code snippet can help a lot.

I have a context manager used for timing sections of code or whole functions (when used as a decorator).  It's aware of the Trio async/await scheduler and is able to exclude periods of time where the task is not executing.  The context manager itself is not async.

Synopsis of decorator case:

  @PerfTimer('query')
  async def query_or_throw(self, q):
      return _parse_result(await self._send_query(q))

I'd like to just use the existing ContextDecorator to define the PerfTimer context manager, and have it wrap coroutine functions properly.  New API is not required.

Sorry, I still have no idea what is the code for PerfTimer class.

PerfTimer is a reusable, non-reentrant context manager implemented as a class.

There are already use cases of ContextDecorator described in the contextlib documentation, such as the track_entry_and_exit logger.  What reason would you have for such context managers to *not* wrap async functions properly?

Got you.

iscoroutinefunction() is not reliable: it detects async functions only but fails if we have a regular function that returns awaitable object.

I think AsyncContextDecorator is needed here to explicitly point that you want to wrap async function.

Another question is: sync or async context manager should be applied? I see use cases for both variants

My use case is for a non-async context manager, and if we take track_entry_and_exit as an example that's non-async as well.

The explicit approach (e.g. separate base class for decorators applied to sync vs. async functions) doesn't seem ideal, because it precludes having a single context manager for both cases.  track_entry_and_exit is an example where a context manager would want to decorate both types of functions.

I'm not sure how big of a problem the iscoroutinefunction() limitation is-- in Trio style we don't pass around coroutines by normal functions.  The `async` qualifier exists to make it clear when a function returns a coroutine, and ContextDecorator already doesn't work for the case of a regular function returning a coroutine.  I think the scope here is to enhance ContextDecorator to work with async functions which are properly qualified with `async`.

I was caught by this again, indirectly via @contextmanager.

If I define a non-async context manager using @contextmanager, I expect the decorator support to work on async functions.  I.e. the following should be equivalent:

```
async def foo():
    with non_async_cm:
        ...
```
```
@non_async_cm
async def foo():
    ...
```

Not only does the 2nd case not work, but the decorator is (effectively) silently ignored :(

There are two different axes of sync/async here, and it's important not to mix them up: the context manager could be sync or async, and the function that's being decorated could be sync or async. This issue is about the case where you have a sync context manager, and it's decorating an async function.

The current behavior is definitely super confusing and not at all useful.

In general, I'm also very skeptical about using iscoroutinefunction to automatically change semantics, because of the issue Andrew mentions about failing to handle complicated objects like functools.partial instances or sync functions that return awaitables. In this *specific* case though, I'm not sure... maybe it actually is OK?

What's special about this case is that when you write:

@sync_cm()
async def foo():
    ...

...the same person is writing both the @ line and the 'async def' line, and they're right next to each other in the source code. So the normal concern about some unexpected foreign object being passed in doesn't really apply. And in fact, if I did have an awaitable-returning-function:

@sync_cm()
def foo():
    return some_async_fn()

...then I think most people would actually *expect* this to be equivalent to:

def foo():
    with sync_cm():
        return some_async_fn()

Not very useful, but not surprising either. So maybe this is the rare case where switching on iscoroutinefunction is actually OK? The only cases where it would fail is if someone invokes the decorator form directly, e.g.

new_fn = sync_cm()(old_fn)

This seems pretty rare, but probably someone does do it, so I dunno.

If we think that's too magic, another option would be to make up some more explicit syntax, like:

@sync_cm().wrap_async
async def foo(): ...

...Oh, but I think this breaks the Guido's gut feeling rule (https://mail.python.org/pipermail/python-dev/2004-August/046711.html), so it would have to instead be:

@sync_cm.wrap_async()
async def foo(): ...

And even if we decide not to automatically switch on iscoroutinefunction, we should probably add a warning or something at least, because right now

@sync_cm()
async def ...

is definitely a mistake.

(Adding Nick to nosy as the contextlib maintainer.)

I still think that iscoroutinefunction should not be used for async/sync decoration type autodetection.

A day ago stuck with a case when decorator used iscoroutinefunction() for separation but returned a regular function wrapper that returns awaitable in turn.

As a result, the transparent decorator which should not modify a decorated function signature cannot be applied twice, e.g.

@retry
@retry
async def fetch(): 
    ...

This is very confusing behavior, the obvious fix is providing `sync_retry` and `async_retry` decorators to never mix the behavior.

Seems very close to the problem discussed here.

I wouldn't be OK with magic switching in the behaviour of ContextDecorator (that's not only semantically confusing, it's also going to make the contextlib function wrappers even slower than they already are).

I'm also entirely unclear on what you would expect a synchronous context manager to do when applied to an asynchronous function, as embedding an "await" call inside a synchronous with statement is unlikely to end well.

> I wouldn't be OK with magic switching in the behaviour of ContextDecorator (that's not only semantically confusing, it's also going to make the contextlib function wrappers even slower than they already are).

I hear you on the semantic confusion, but is a single check at definition time really that expensive? The runtime cost is zero.

> I'm also entirely unclear on what you would expect a synchronous context manager to do when applied to an asynchronous function, as embedding an "await" call inside a synchronous with statement is unlikely to end well.

It would be like:

async def blah():
    with something():
        await foo()

There's nothing weird about this; people write the long version all the time. You'd only do it when 'something()' doesn't do I/O, but there are lots of context managers that don't do I/O.

> I hear you on the semantic confusion, but is a single check at definition time really that expensive? 

Do you know how to make that single check 100% reliable?

@Yury: depends on what you mean by "100% reliable" :-). Like I said above, I'm normally super against automagic detection of sync-vs-async functions because of all the edge cases where it goes wrong, but in this specific case where people are writing a decorator one line above their def/async def, I think a simple iscoroutinefunction check will be pretty close to 100% matching what users expect.

Or, if we don't change the semantics, then we can still be 100% confident that if iscoroutinefunction returns true, then the user has made a mistake. (I.e., if we make this issue a warning, then it's possible we'll miss print a warning in some complicated cases, but we can be confident that all the warnings we do print are correct.)

> I think a simple iscoroutinefunction check will be pretty close to 100% matching what users expect.

Yes, "pretty close", but not reliable. :)  E.g. I can easily design a decorator that when applied first would break the proposed iscoroutinefunction logic.  While decorators like that aren't popular, I'd be wary about introducing a solution that can lead to hours of debugging in some weird (and maybe stupid) cases.

> Or, if we don't change the semantics, then we can still be 100% confident that if iscoroutinefunction returns true, then the user has made a mistake. (I.e., if we make this issue a warning, then it's possible we'll miss print a warning in some complicated cases, but we can be confident that all the warnings we do print are correct.)

+1 to implement a warning the way you suggest.

It isn't the simple case where the auto-detection idea concerns me, it's decorator stacking cases like:

    @outer
    @magic_detection
    @inner_forces_async
    def sync_native_api():
        ...

(With the original asyncio.coroutine being one example, but there are other situations where this comes up, like a wrapper that bundles synchronous calls up into an executor invocation)

That said, I'd be completely fine with a combination where:

* ContextDecorator grew a coroutine() classmethod (open to suggestions for bikeshed colours)
* The regular ContextDecorator constructor emitted a warning suggesting "cls.coroutine" when it was applied to a synchronous function

Then the original example would provide distinct spellings for the sync and async case, without the definition of PerfTimer needing to change:

  @PerfTimer.coroutine('query_async')
  async def query_or_throw(self, q):
      return _parse_result(await self._send_query(q))

  @PerfTimer('query_sync')
  def query_or_throw(self, q):
      return _parse_result(self._send_query_sync(q))

That way we're still refusing to guess in the face of ambiguity (does the user want the coroutine version of the context manager, or did they accidentally mix a potentially blocking synchronous context manager into their async code?), but the warning can be usefully explicit regarding how to resolve the ambiguity.

The query in #37743 highlights that generator functions and other wrapped iterator factories actually face a similar problem: they need the function body to contain "yield from wrapped(*args, **kwargs)" if the CM is going to be closed after the iterator terminates (or is closed), rather than closing immediately after the iterator is created.

bpo-40816 took the much simpler approach of introducing a separate contextlib.AsyncContextDecorator class.

> bpo-40816 took the much simpler approach of introducing a separate contextlib.AsyncContextDecorator class

How do I apply AsyncContextDecorator to the use case of wrapping either a sync or async function with a synchronous context manager?

You are correct that this proposal is technically rejected, rather than being a true duplicate of the AsyncContextManager addition.

The core problem with the automatic inference idea is that it isn't possible for the standard library to reliably distinguish at decoration time between "synchronous function that happens to return an awaitable" and "coroutine factory function written as a synchronous function that should nevertheless be treated like a coroutine function".

That ambiguity doesn't exist when ContextDecorator and AsyncContextDecorator are applied to distinct helper functions.

Within a given codebase, you may be able to write a "coroutine argument detection" function that is robust enough for that particular codebase (and iscoroutinefunction() may even be adequate for that purpose), in which case ContextDecorator and AsyncContextDecorator provide the building blocks for implementing the two halves of the solution, leaving only the auto-switching code as project specific.