Add opt-in GDScript warning for when calling a coroutine without `await`

[mihe]

Describe the project you are working on

A 2D game, with several people working on it.

Describe the problem or limitation you are having in your project

Sometimes you forget to add await when calling a coroutine, and sometimes you change an existing method to await something, but forget to change some call site somewhere, resulting in the caller continuing execution when that might not be desirable.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

Adding an opt-in warning, like for example debug/gdscript/warnings/missing_await, where it's set to IGNORE by default, similar to how debug/gdscript/warnings/return_value_discarded works currently, would help resolve this problem.

Then you would (so long as there's adequate type information) get a warning for all places where you're calling a coroutine without await, with the ability to do @warning_ignore("missing_await") if you're absolutely sure that you want caller execution to continue at that particular call site.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

I'm guessing it would from an implementation standpoint be very similar to the current RETURN_VALUE_DISCARDED warning, where it just checks a couple of conditions in GDScriptAnalyzer::reduce_call and emits a warning if the CallNode isn't the child of an AwaitNode.

If this enhancement will not be used often, can it be worked around with a few lines of script?

Not that I'm aware of.

Is there a reason why this should be core and not an add-on in the asset library?

Same reason all the other GDScript warnings aren't.

[mihe]

Mildly related to #3901 and #11094 I guess, but those seem to focus more on implicitly/forcibly adding await with the help of an annotation rather than just emitting a warning when omitting it.

[dreed-sd]

This would benefit us greatly. Calling a function that internally uses await is essentially creating a new top-level coroutine, and represents dangling state that nothing is keeping track of anymore (in our code, that is). It is very important to be very intentional about it when you do this. On a large code base with a large number of developers working on it, it's way too easy to accidentally create this kind of dangling state, or top-level coroutine. We'd benefit greatly from it being an explicit decision to do so. By having this warning we could choose to require it.

[Jesusemora]

It would be better if there was a keyword to specify that we want to run a function without await.

Currently I use both, await halts execution of the code until the function finishes, so it can be used with timers, while running it without allows me to do multiple things at the same time from the same code, similar to tweens.

[dreed-sd]

It would be better if there was a keyword to specify that we want to run a function without await.

I agree it would be better, although I imagine much more controversial since there'd be a backward compatibility issue with it. Also though there are dynamic scenarios to consider, where a function is overridden to include an await when the base implementation does not -- the call site can't know until runtime if the func is a coroutine. For a restriction like this to work I think there'd also have to be a way to explicitly specify whether a func is a coroutine instead of it just being 'if it contains an await' -- I also think that would be better, but even more disruptive.

A warning will catch most mistakes. Although I have to admit, it won't catch the overridden scenario either.

[Jesusemora]

It would be better if there was a keyword to specify that we want to run a function without await.

I agree it would be better, although I imagine much more controversial since there'd be a backward compatibility issue with it. Also though there are dynamic scenarios to consider, where a function is overridden to include an await when the base implementation does not -- the call site can't know until runtime if the func is a coroutine. For a restriction like this to work I think there'd also have to be a way to explicitly specify whether a func is a coroutine instead of it just being 'if it contains an await' -- I also think that would be better, but even more disruptive.

A warning will catch most mistakes. Although I have to admit, it won't catch the overridden scenario either.

I don't think it needs to change anything in engine, just be a requirement in gdscript so we can catch it at a glance. It's similar to the proposal.

The problem I have with it being just a warning is the same I have with integer division, it is annoying when you are doing it on purpose.

As for backwards compat, we are going into godot 5, so It would not be a problem, they are already introducing the abstract keyword that I think would break some of my code if I ever upgraded those projects.

[mihe]

It would be better if there was a keyword to specify that we want to run a function without await.

Adding a new keyword to make the ignore case cleaner seems mostly orthogonal to the discussion of the warning itself, since I'm assuming plenty of people would object to being forced to use this proposed keyword all over the place when they do in fact just want execution to continue, meaning you'd still likely want/need an opt-in warning.

Anyway, I made a naive attempt at adding this warning myself: godotengine/godot#107936

they are already introducing the abstract keyword

For what it's worth, this is on track to becoming an @abstract annotation instead: godotengine/godot#107717