Godot: Improve CSharpScript cross-language compatibility and editor support

[opl-]

Godot version:
227494b

OS/device including version:
All

Issue description (updated):

CSharpScript::get_base_script is currently not implemented, which prevents the editor and other languages from correctly determining the inheritance chain and any information derived from it. The method needs to be implemented to fix this.

See @neikeq's comment below for his vision on how this should be implemented.

---

Issue description (original):

I've been looking into the C# side of things for #40147 and while looking at the implementation of the mono module, I noticed an issue related to the lack of inheritance support in the CSharpScript class:

Currently the CSharpScript class assumes responsibility for the entire user class and all of its inheritance chain going up. In contrast, as an example, GDScript creates a new GDScript class instance for each script in the inheritance chain.

One of the problems this causes is that CSharpScript::get_base_script currently returns Ref<Script>(), which causes several issues:

• Without inheritance information, other scripting languages like GDScript are unable to take advantage of things like type hints or specifying variables using more generic types. (For example, for the inheritance chain Node -> MyBaseNode -> MyDerivedNode, the user could declare a variable of type MyBaseNode and assign an instance of MyDerivedNode to it.) These assumptions are not possible to make without correct inheritance information.

• The editor currently relies on Script::get_base_script for things like the inspector sections and the Create New Node dialog, which currently uses it to determine the tree layout. (This could possibly be bypassed by using the ScriptServer to determine the inheritance chain instead.)

Unless this was done for a specific reason, it should be changed. This would require refactoring methods like CSharpScript::get_script_property_list to go up the inheritance chain (like GDScript does). Additionally, since for instances initialized from mono the CSharpScript instances don't have a path and neither would the ones for the base classes (at least under the current implementation), the ResourceLoader::load caching can't be reused for scripts, so a new cache internal to the CSharpLanguage would likely have to be created as suggested in one of the comments in the code. This could potentially replace the existing CSharpLanguage::script_list field.

As a side note, this could also be a good opportunity to look at #15661 again, which suggests using the fully qualified class names to refer to C# scripts instead of the paths to their source code files, which would remove the need to determine the source file path altogether.

Steps to reproduce:
N/A

Minimal reproduction project:
N/A

---

Pinging @neikeq as the maintainer of the Mono module.

[neikeq]

Currently the CSharpScript class assumes responsibility for the entire user class and all of its inheritance chain going up. In contrast, as an example, GDScript creates a new GDScript class instance for each script in the inheritance chain.

Yes, and I prefer to keep it this way. Having a CSharpScript for each base class is an unnecessary overhead. Specially if the base classes are never used separately as scripts.

One of the problems this causes is that CSharpScript::get_base_script currently returns Ref<Script>(), which causes several issues

Script::get_base_script indeed needs to be implemented. It should either return the script representing the file associated with the base class or one that wraps a class (like CSharpScript::create_for_managed_type does).

This would require refactoring methods like CSharpScript::get_script_property_list to go up the inheritance chain (like GDScript does).

CSharpScript::get_script_property_list does include properties from the base classes, but we don't walk up the inheritance chain with CSharpScript but the GDMonoClass directly: GDMonoClass *top = script_class; while (top && top != native) { /* ... */ top = top->get_parent_class(); }. The member_info list used by get_script_property_list is filled in _update_exports and _update_member_info_no_exports.

As a side note, this could also be a good opportunity to look at #15661 again, which suggests using the fully qualified class names to refer to C# scripts instead of the paths to their source code files

This is something I've wanted for a long time, but implementing it properly will require core changes which are up to @reduz. I discussed with him about adding the possibility to reference scripts by a qualified name instead of a file and he agreed to make it possible. This was about a year ago IIRC so I don't if he changed his opinion.

[opl-]

I see, thank you for confirming that this is indeed by design.

If I understand correctly, the solution you're proposing is to implement CSharpScript::get_base_script to return a newly constructed (or previously cached) CSharpScript instance with all of the properties parsed again for that instance. Looking at the uses of that method, it appears to currently be primarily used by the editor, so I'd say that is indeed not an unreasonable design. I'll update the issue to include this information.