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.