Warn about unused Awaitables (and potentially other objects)

[JelleZijlstra]

This code:

async def f() -> None:
    pass
async def g() -> None:
    f()

is incorrect because f() isn't awaited. This will produce a runtime warning with the right debug settings, but mypy doesn't give any errors.

It would be nice if mypy could catch this, since I think it's a pretty common error in async code. This could be generalized to warning on other types or function return values that shouldn't be ignored (maybe open?), similar to __attribute__((warn_unused_result)) in GCC.

[gvanrossum]

I'm pretty sure we've got another similar issue somewhere, but I can't find it. (About results that should never be discarded -- perhaps uncalled functions?)

I'm not sure how easy this would be to do -- especially not the generalization, which probably would require us to invent some acceptable syntax for marking functions whose result should never be ignored.

[Daenyth]

I think skipping generalization and focusing on Awaitable first would still catch bugs. Once there's a second case people want to specialize it'll be easier to see what needs to be made general

[njsmith]

Awaitable is a bit special too, because "is the value used" isn't quite the right check. For example, here's one of the cpython bots that had a missing await:

python/cpython#9168 (comment)

The code was something like f"Sorry, {get_participants()}, ...", but should have been f"Sorry, {await get_participants()}, ...". So the awaitable value was used, and was even used in a type-valid way (pretty much any python object can be interpolated into an f-string), but clearly not what was intended. On the other hand, in asyncio, passing an awaitable into a function like asyncio.gather is a totally normal way to consume it.

I was looking at this recently in the context of Trio, where the rules for using awaitables are much simpler: basically just, any call returning an awaitable should have an await present, full stop. This is obviously too simple to become a general always-on feature in mypy, since it won't work for asyncio users, but it'd be nice to have it as an optional feature or a plugin or something. I made some more detailed notes here on what exactly we'd want and how it might work: python-trio/trio#671

Is it possible to have a mypy plugin that adds a check like this? And can anyone point me at roughly where in the type checking process we'd want to be to perform a check like that? (I assume somewhere in mypy/checker.py?)

[ilevkivskyi]

@njsmith

Is it possible to have a mypy plugin that adds a check like this?

I am not sure writing it as a plugin is the best solution. This will need addition of at least one new plugin hook. I think a better idea is to just add a flag to mypy. This looks like a common scenario. Also the implementation in mypy is very simple. We can add a flag is_awaited on CallExpr that will be populated during semantic analysis (by enclosing visit_await). An then an error can be given in visit_call_expr in checkexpr.py if the inferred resulting type is Awaitable, but the is_awaited flag is not set.

[njsmith]

That would probably be the easiest approach, yeah. But... this check really is only useful to projects using trio, not to projects using asyncio. Is mypy interested in carrying features like that?

[ilevkivskyi]

Is mypy interested in carrying features like that?

I have heard similar things from other people. We can start with something, and then maybe generalize the flag to be tunable, for example:

• --enforce-await=none (default) does nothing
• --enforce-await=global (hard to implement) will complain only if a coroutine is never awaited
• --enforce-await=local (easy to implement) will complain if a coroutine is not immediately awaited.

But we can start just with a binary flag --enforce-await and then iterate on the result later.

[JukkaL]

I don't like the idea of just adding another opt-in warning for this. I'd much prefer if we could have something that is enabled by default, even if it doesn't catch all errors. There's a risk that the majority of users would not realize that such an option exists.

What about always rejecting code such as the original example, as the first iteration of this feature:

async def f() -> None:
    pass
async def g() -> None:
    f()  # Should be an error

Is there ever a good reason to write f() without doing anything with the result? This wouldn't reject things like x = f(), only bare f(). x = f() would likely generate an error already if the code does anything with x later on.

We could later warn about things like "{}".format(should_be_awaited()) -- again, this is something that would hardly ever be right. We could special case some common operations that work with arbitrary objects, such as str.format, % formatting, str(x) and f-strings. We could extend this to check for things like "%s" % foo.method as well (i.e., forgot to call a method).

Later, we could consider adding a trio-specific stricter check (possible as a plugin).

[njsmith]

I'd much prefer if we could have something that is enabled by default, even if it doesn't catch all errors. There's a risk that the majority of users would not realize that such an option exists.

In Trio's case, we'd probably make it enabled by default in our official project template and advertise it prominently in the docs (we already have a whole discussion of this issue right in the tutorial). But yeah, I know what you mean.

Is there ever a good reason to write f() without doing anything with the result?

If f returns a coroutine object, then you're correct, a bare f() is always a mistake. If f() returns a Future, then a bare f() is a relatively normal thing to write. So I guess a check like this keyed off Coroutine rather than Awaitable would make sense.

We could later [...] later [...]

I hear where you're coming from. But, it is frustrating that mypy has all the information to do a very straightforward, precise, and high-coverage check here, but it's blocked because we can't get at it :-/. Would there be any simple, acceptable change to the plugin interface that would make it possible to implement this as a plugin relatively soon?

It looks to me like the two main blockers are:

• You can define hooks that run on function and method calls, but any given function/method can only have 1 such hook. Here we want a hook that runs on all functions+methods, so we can do that, but if we do then it will effectively disable any other plugins that want to use those hooks. Maybe there could be some way to run multiple hooks in sequence? For the function and method hooks in particular it doesn't look like this would be terribly complicated semantically. This does create some issue about what order to run hooks in. (Ideally the Awaitable check would run last, I guess, so it can see the inferred ret_type after all other hooks have run?) But in practice it may not matter much. For this plugin, the only time it would matter would be if another plugin changed a ret_type from Awaitable to non-Awaitable, or vice-versa, and that seems rare.

• There's no way for the hook to tell whether a given call is the immediate child of an await node. I guess this could be hacked into the Context?

[JukkaL]

Would there be any simple, acceptable change to the plugin interface that would make it possible to implement this as a plugin relatively soon?

Small plugin API changes are typically not controversial, so this is a possibility.

Maybe there could be some way to run multiple hooks in sequence?

That would be one way to do it, but ordering issues would make this a bit awkward, as you point out. Another option would to add a new "check only" hook that is called after the signature hooks have already run and we have a final signature. The hook wouldn't be able to change the signature but it can perform additional checks, and it's guaranteed to run after the signature-related hooks. We could allow multiple plugins to define these hooks for a function.

There's no way for the hook to tell whether a given call is the immediate child of an await node. I guess this could be hacked into the Context?

We'd need another attribute in the relevant context named tuple, I guess.

Can you create a new issue for the plugin feature? We can then focus on the non-trio/non-plugin aspects in this issue.

[njsmith]

Can you create a new issue for the plugin feature? We can then focus on the non-trio/non-plugin aspects in this issue.

OK, forked this off into #5650 and replied there.

[Dreamsorcerer]

* `--enforce-await=local` (easy to implement) will complain if a coroutine is not _immediately_ awaited.

What about a local check, with the added assumption that a function expecting an Awaitable is likely to await on it? Plus a tunable unknown state when not enough typing information is available (parameter is Any).

This would automatically make functions like asyncio.gather() work without needing to special case them. I'm imagining that cases where a function expects an Awaitable and then doesn't await on it are going to be very rare (plus if they are within the user's code it would still highlight an error within that function).

For example:

async def do_something(): ...

async def func_with_await(foo: Awaitable):
    return foo  # Error: foo not awaited

async def unknown_func(foo: Any):
    print(foo)

async def main():
    # These start as awaited=False
    a = do_something()
    b = do_something()
    c = do_something()
    d = do_something()
    e = do_something()

    await func_with_await(a)  # a.awaited=True
    asyncio.create_task(b)  # b.awaited=True
    await unknown_func(c)  # c.awaited=Unknown
    print(d)  # d.awaited=False
    # Error: e.awaited=False

Whenever the awaited state is False it should error, and if True it should not error. When it is in the unknown state, this should error depending on a tunable flag (similar to the various Any warnings that are configurable).

[Dreamsorcerer]

Is there likely to be any movement on this? I've been hitting this problem a lot lately, trying to refactor a bunch of sync code to be async and struggling to find every location that needs an await.

[JelleZijlstra]

As you see, not much is happening.

I did implement this check in Quora's typechecker, pyanalyze (https://github.com/quora/pyanalyze); you can see the relevant tests at https://github.com/quora/pyanalyze/blob/26e622e249d5b101323647a29ede386d4de06f1e/pyanalyze/test_name_check_visitor.py#L1868. It is not nearly as well-tested as mypy, though, but feel free to give it a try.

[jstasiak]

I have a generic proof of concept in #8951. It should help in this case too. I've meant to make a proper PR out of it for some time now, I'll try to get it done eventually.

[erictraut]

We had a similar discussion in this pyright issue. As others have mentioned in this thread, there are subtleties involved. I ended up implementing a check in pyright that covers the most common error cases (including the one at the top of this thread) while avoiding most false positives.

If someone is interested in implementing a similar check in mypy, I'm happy to share the learnings from pyright.

[Dreamsorcerer]

Thanks, I'll take a look at some of those.

Would be nice if we can atleast decide on the correct approach and remove the needs-discussion label though. Is my proposal above (#2499 (comment)) reasonable? Or any issues with it? I think it would work 99% of the time and seems like a relatively easy approach to implement (although I'm not really familiar with the mypy codebase).

[AlexWaygood]

There has recently been movement on this in the form of #12279

[Dreamsorcerer]

Looks like a good start. Judging by the examples, I assume this just checks that the return value is assigned to a variable or awaited. Which will catch the simplest mistakes like:

async def f():
    db.insert(...)

But, it won't yet check if the assigned variable is actually awaited.

[Daenyth]

What about this design:

• always warn when an awaitable is not awaited
• provide a cast-like method (implemented as identity) to signal to mypy "no, really, I don't want to await this" and locally silence the warning
• if an awaitable is not awaited but is passed to a method whose parameter is typed to receive an awaitable, do not warn

[Dreamsorcerer]

* provide a `cast`-like method (implemented as `identity`) to signal to mypy "no, really, I don't want to await this" and locally silence the warning

I think you have basically just described the purpose of # type: ignore comments.

* if an awaitable is not awaited but is passed to a method whose parameter is _typed_ to receive an awaitable, do not warn

This is what I proposed above (#2499 (comment)), with an option to change whether an error occurs when passed to a function with Any.

[graingert]

anyio has the concept of DeprecatedAwaitables that warn when you await them - they are list, float and Noneish subclasses otherwise. Is the advice to do this?

class DeprecatedAwaitable:
     if not TYPE_CHECKING:
        def __await__(self):
            warnings.warn(..., stacklevel=2)
            return None
            yield

[jhance]

I think trying to find whether all possible control flows await an awaitable is difficult for a type checker. How does it know if I pass it to a function whether that function will await it or not?

The statement that passing it to a function is likely to await it is not true. For example appending to a list

[Dreamsorcerer]

I think trying to find whether all possible control flows await an awaitable is difficult for a type checker. How does it know if I pass it to a function whether that function will await it or not?

The statement that passing it to a function is likely to await it is not true. For example appending to a list

How frequently do you think a function will explicitly declare the type as Awaitable and then not await on it (either directly or through another function)? My assumption is that this will be very rare in real world use cases, likewise a typesafe function that doesn't accept an Awaitable (e.g. object) is very unlikely to await on it. If this works in real world use cases 99% of the time, then it's definitely worth putting in.

If a function accepts Any then we have no idea if it will await or not, so having a strictness setting to warn or not in these cases is helpful. We can default to not warning to avoid false positives, but people looking for something stricter/safer can enable warnings in this case (see the different examples in my comment).

[graingert]

closed in #12279 ?

see docs here https://mypy.readthedocs.io/en/stable/error_code_list2.html#check-that-awaitable-return-value-is-used-unused-awaitable

[Dreamsorcerer]

That just handles unused awaitable expressions. Something as simple as a = foo() won't trigger the warning.

I think we still need to have something like #2499 (comment) to cover a lot more cases. It's a bit more complex as you need to track the awaited state through a function in order to see if it was awaited by the end, but would catch a lot more mistakes.

[jstasiak]

a = foo() where a remains unused should be covered by flake8 or other linters already under the unused variable umbrella (granted, maybe it'd be nice to have it in Mypy as well)

[Dreamsorcerer]

a = foo() where a remains unused should be covered by flake8 or other linters already under the unused variable umbrella (granted, maybe it'd be nice to have it in Mypy as well)

That only works if the variable is unused. You can then do print(a) or similar and flake8 will no longer catch it as unused and mypy will not catch it as unawaited.

[tback]

Can mypy be configured to require type hints for unawaited assigns?

async def do_something(): ...

async def main():
    # These start as awaited=False
    a = do_something()  # ERROR
    b : Awaitable = do_something()  # OK
    c = await do_something()  # OK

[Dreamsorcerer]

That is not an unusual thing to do though, so not sure how many people would find such an option useful.

[tback]

My reasoning behind this request is to make delayed calls an active decision and to prevent errors from forgotten awaits.
I'd assume that this would provide a robust solution for a couple of "simple" use cases while also being kind of easy to implement.