Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cannot detect all calls to await the same way you could with yield. #64236

Closed
bitwes opened this issue Aug 10, 2022 · 7 comments
Closed

Cannot detect all calls to await the same way you could with yield. #64236

bitwes opened this issue Aug 10, 2022 · 7 comments

Comments

@bitwes
Copy link

bitwes commented Aug 10, 2022

Godot version

v4.0.alpha13.official.59dcddc45

System information

2016 Macbook Pro MacOS 10.15.7

Issue description

In 3.x you could check the return value of a function call to see if a method called yield. As far as I can tell, there is no way to do the same in 4.0. The metadata for functions in 4.0 appear to contain a "coroutine" flag if the method calls await and contains a return. If a method only calls await the calling function can't know if an await occurred. There are situations when dynamically calling methods where this information is useful, especially when avoiding 'Redundant await' errors.

Here's what I've found from testing.

This method has an await and return so the usage flag for the return is 262150

func this_is_a_coroutine():
	return await get_tree().create_timer(1).timeout
{
 "args": [  ],
 "default_args": [ ],
 "flags": 1,
 "id": 0,
 "name": "this_is_a_coroutine",
 "return": {
  "class_name": "",  "hint": 0,  "hint_string": "",  "name": "",  "type": 0,
  "usage": 262150
 }
}

This method has an await but the usage value is 6.

func this_just_does_an_await():
	await get_tree().create_timer(1).timeout
{
 "args": [ ],
 "default_args": [ ],
 "flags": 1,
 "id": 0,
 "name": "this_just_does_an_await",
 "return": {
  "class_name": "",  "hint": 0,  "hint_string": "",  "name": "",  "type": 0,
  "usage": 6
 }
}

Use Case

In the GUT unit testing framework you can yield (now await) inside a test. In 3.x the framework can detect that a yield occurred by checking the return value of the method. GUT then yields to the "completed" signal if a yield occurred. While waiting for the signal, the framework gives animated feedback to the user, letting them know that the test suite has not frozen.

In 4.0 it appears that if you return within a method that uses await it will be marked as a coroutine. This isn't a practical requirement for tests (not your problem, I know) since tests shouldn't return a value. Since we cannot know for sure which tests contain an await, all tests must be called with await. This will cause a lot "redundant await" errors.

Besides the errors, this also prevents the framework from providing feedback to the user that a coroutine was called and it is waiting for it to finish. This can make the test suite appear to hang when it is really waiting for the test to finish.

Workarounds

These are all valid workarounds, but they require action by the end user and extra documentation which end users never read (again, not your problem, I know).

  • Disable errors for plugins
  • Disable the "redundant await" error
  • Require users of the framework to always return when using await.

Steps to reproduce

Described in issue description

Minimal reproduction project

Described in issue description.

@bitwes
Copy link
Author

bitwes commented Sep 27, 2022

If Callable had a way to indicate if it was "awaiting" that would work. If it also had a completed signal that would fill all the requirements of the 3.x way.

Currently in GUT I do this in a number of scenarios

script_result = script_inst.call(test_name)
if(_is_function_state(script_result)):
  yield(script_result, 'completed')

If Callable had an is_awaiting flag and a completed signal then you could do:

var call_this = Callable(script_inst, test_name)
call_this.call()
if(call_this.is_awaiting):
  await call_this.completed

If it only has the flag then you could wait until it finished using idle_frame

var call_this = Callable(script_inst, test_name)
call_this.call()
while(call_this.is_awaiting):
  await get_tree().idle_frame

I don't think having just the signal will work since it wouldn't be caught if the Callable does not encounter an await (based on my experience with signals in 3.x).

@bitwes
Copy link
Author

bitwes commented Sep 27, 2022

Here is the _is_function_state in case you were wondering.

func _is_function_state(script_result):
	return script_result != null and \
		   typeof(script_result) == TYPE_OBJECT and \
		   script_result is GDScriptFunctionState and \
		   script_result.is_valid()

@caimantilla
Copy link
Contributor

No progress on this issue? I'm kind of baffled that this isn't at least something you can check on Callable. It's even used in Escoria.
The alternative right now, for my use case, would be to create some custom callable container with an is_running property and a completed signal. This is a pretty sloppy workaround, though.

@jhlothamer
Copy link
Contributor

In my tests you can use await on a function that itself does no "awaiting". There's no errors and it's as if you didn't try to await on it at all. But if the function actually awaits on something, then the await happens in the call.

So, just always use await in this case. It should work.

@vnen
Copy link
Member

vnen commented Feb 23, 2023

This is one thing that I imagined some users would miss (though I didn't see this report until now, sorry) which is essentially manipulating the GDScriptFunctionState in some way, even if only checking if it's there.

For Godot 4.0 I purposely remove access to it (it's not even exposed anymore) because while it does have some benefits, it is an implementation detail that users shouldn't care about. More technically, it is a continuation, which is a pretty rare feature in languages AFAIK, and can also get pretty difficult to handle (especially with static types: if a function says it returns int it should return a GDScriptFunctionState as this can break many things).

I don't mean that this is not an issue. I just think there are better ways to solve this problem, one just being better introspection. Doing this in Callable may be tricky because the the "coroutine" thing is exclusivity of GDScript, so it should not leak out to core types (while other available languages may have it, each have their own implementation which does not translate well to the Godot core API).

Unfortunately we're just out of time for 4.0, so this will have to wait a bit longer. In the meantime I wouldn't mind hearing more suggestions or even more use cases so we can find a solution that encompasses them all.


PS: Regarding the usage flags you saw when testing, the big number is PROPERTY_USAGE_NIL_IS_VARIANT which is used to tell the NIL in the return type is actually Variant and not void. But in this case both of the methods should have it, the fact one does not is a bug.

@vnen vnen modified the milestones: 4.0, 4.1 Feb 23, 2023
@dalexeev
Copy link
Member

dalexeev commented Apr 18, 2023

especially when avoiding 'Redundant await' errors.

False positive REDUNDANT_AWAIT warnings have been fixed by #74949. If you don't know if a callable is a coroutine, you can use await. In the case of a non-coroutine, there will be no error, unlike yield in 3.x. But keep in mind that a function using await becomes a coroutine.

It is impossible to know whether a function is a coroutine or not, but this is more a matter of reflection/introspection in general, and not a limitation when using await at runtime. Also, you can implement helper classes/functions that were not possible in 3.x (this is mentioned in some of the proposals in the list below).

See also:

@bitwes
Copy link
Author

bitwes commented Jan 16, 2024

I think this is now resolved. The original issue seems less significant now (the messages GUT could provide), and nothing else has popped up in the meantime.

@bitwes bitwes closed this as completed Jan 16, 2024
@YuriSizov YuriSizov removed this from the 4.x milestone Jan 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: No status
Development

No branches or pull requests

8 participants