GDScript: Fix subclass methods not inheriting RPC info

[anvilfolk]

Fixes #80087, which has an excellent explanation of the origins of the issue thanks to @Dragoncraft89.

UPDATED

General issue description

The way that RPC configurations were populated required the full compilation of the base class. Specifically, the order of operations is the following:

GDScriptCompiler::compile() // Compiles the main class
|-> GDScriptCompiler::_populate_class_members() // Prepares base class member info, followed by current & inner classes
|-> GDScriptCompiler::_compile_class() // Actually compiles the class to byte code
    |-> GDScript::_init_rpc_methods_properties() // At the end of compilation, obtains RPC information from base classes
|-> GDScriptCache::finish_compiling() // Finally compiles dependencies, which includes base classes

So notice that since RPC information requires the base class to be compiled, that information isn't available until after, when GDScriptCache::finish_compiling() is called. However, the core issue of #78146 was that _populate_class_members() was calling for the compilation of the base class, which introduced fundamental cyclic dependency problems during compilation.

We cannot cause the compilation of another class during compilation of the current class. Specifically, _compile_class() cannot cause another compile() or get_full_script() call to anywhere else.

Solution

The solution is to populate RPC information in a similar manner to how _populate_class_members() currently works, after #79205. In essence, RPC configuration should only depend on:

1. Information available from shallow scripts, i.e. parsed & analyzed (but not compiled) scripts,
2. The base class

Crucially, it cannot depend on data obtained from the compilation (meaning _compile_class()) of another class. Current code requires GDScriptFunctions, which result from compilation.

Implementation

All the RPC information was actually from simple parsing of the annotation and stored in FunctionNode. It turns out _populate_class_members() was already using GDScripts and their associated ClassNode, which contains members, including functions.

All RPC logic was moved into _populate_class_members(), which was already recursively being called for base scripts, traversing its ClassNode members, and continuing with inner scripts. In essence, doing everything we needed it to do.

The function was also renamed to _prepare_compilation(), which is more descriptive of all it does now. Comments were added indicating that the function cannot cause compilation to occur. It's strictly a pre-compilation step. The order is now:

GDScriptCompiler::compile() // Compiles the main class
|-> GDScriptCompiler::_prepare_compilation() // Prepares base class member info & RPC info, followed by current & inner classes
|-> GDScriptCompiler::_compile_class() // Actually compiles the class to byte code
|-> GDScriptCache::finish_compiling() // Finally compiles dependencies, which includes base classes

MRP

Here is a project that fails before this PR and is fixed after, and which uses inner class RPCs. Basically a copy of the original issue report. To run it, you can use the following commands from the appropriate directories:

godot-binary --path ./godot-test/ server.tscn
godot-binary --path ./godot-test/ client.tscn

I doubt it addresses #80597 since that one is present in 4.1.1 but this regression was introduced by #79205, which isn't in 4.1.

[dev-mico]

nice work, i'm planning on debuggin + making a sample project for #80597 next week, I'll make sure to ping you if I can figure it out.

[anvilfolk]

After some testing and thinking about it some more, I don't think this approach is going to work. I think it fairly fundamentally relies on GDScriptCache::finish_compiling() which _compile_class() doesn't use, and that's what's used for inner classes. I suspect adapting compile() for inner classes would result in cyclic compilation errors.

I'm going to investigate moving RPC info storage into structures that don't need the class to be compiled, and which can be resolved at analyze-time. That way at compile time we don't need to check compiled superclasses, we can just access data from analyzed superclasses.

[ryanabx]

I'm going to investigate moving RPC info storage into structures that don't need the class to be compiled, and which can be resolved at analyze-time. That way at compile time we don't need to check compiled superclasses, we can just access data from analyzed superclasses.

Out of curiosity, are all annotations currently reliant on the class being compiled?

[Dragoncraft89]

@anvilfolk I dont think you can call RPC functions on inner classes. At least I can't manage to do it.

[anvilfolk]

I think I've finally come up with a fix that works in the general case, and also does a nice little refactor of the code to boot :) If folks using RPCs could please test this PR that would be wonderful! I've only run some tests locally, but they aren't super complex.

@anvilfolk I dont think you can call RPC functions on inner classes. At least I can't manage to do it.

@Dragoncraft89 the issue is that you can have a "normal" class extend an inner class, and the inner class can extend from anything, etc, and any point in that hierarchy may have RPC info being inherited from somewhere! I'll attach a project to the OP with an example!

[Dragoncraft89]

So I just tested some cases and they all work:

• Inheritance Parent -> Child
• Inheritance Grandparent -> Parent -> Child
• The above cases with an inner class
• Without inheritance

Additionally I tested this on my project that prompted me to open the original bug report. This works as well.

Well done @anvilfolk !

[anvilfolk]

Thank you so much for testing! ❤️ My favorite PRs are the ones that fix an issue and end up with less code, hehehehe! Hopefully we'll get some reviews here soon and it'll get merged so the issue is fixed in master.

[adamscott]

Reviewed during the GDScript Team Meeting. Everything looks fine and that's a nasty bug removed from our codebase. Good job, Ocean!

[anvilfolk]

Latest push was just fixing the comments @AThousandShips flagged! Apologies on always forgetting the comment guidelines, I'll try to be better at it :)

[AThousandShips]

It's something that takes time to get used to! That's why I'm here ^-^

[akien-mga]

Thanks!