feat: lua script inheritance & feat: is_abstract by illarn · Pull Request #225 · gilzoide/lua-gdextension

illarn · 2026-04-13T23:35:54Z

Goal

Make it possible to inherit from lua classes (#164) and add abstract lua script property (since now with script inheritance it has effect)

Changes

Added get_class_script utility
Added base_script (reference to the script being extended) in the LuaScriptMetadata
Added all godot function implementations for in-editor inheritance to work in LuaScript and LuaScriptLanguage
Implemented is_abstract property in LuaScriptMetadata and LuaScript
Moved get_func implementation from LuaScriptInstance to LuaScript get_property and added parent value check

illarn · 2026-04-14T00:49:12Z

@gilzoide hello! I'm in the making of this PR, seems almost done, I'll try finishing it this week. If you have free time feel free to check it and give feedback. I've tested it and all in-editor stuff seems to work (signal inheritance, node selection, method/property inheritance, exported property inheritance)

Btw I've short-sightedly branched it from my main instead of upstream, so it also shows changes from the other PR, sorry for the inconvenience

illarn · 2026-04-19T08:10:10Z

@gilzoide hello! I've finished this PR, added an inheritance.lua test, tested all the in-editor functionality, everything seems to work fine

Things that can be improved:

adding reload on parent source change. I tried to make it work with _set_source_code, but searching for children was painfully slow (we can store script inheritance tree to solve this, although maybe godot has some hook for this purpose, couldn't find it)
adding super. Idk if it's better to implement this as a keyword, method or smth like BaseClass.method(derived, ...), but I think it would be pretty handy
adding custom error message when trying to call :new() on an abstract script, right now it's just invalid method new

I think this is ready to merge as is (unless I missed smth), but if you feel that something else is required pls let me know

gilzoide · 2026-04-20T18:18:31Z

+}
+
+StringName LuaScript::_get_instance_base_script_type() const {
+	return _get_base_script() != nullptr ? metadata.base_class : "";


This doesn't look right. Shouldn't you return something from _get_base_script() in case _get_base_script() != nullptr?

This doesn't look right. Shouldn't you return something from _get_base_script() in case _get_base_script() != nullptr?

I thought leaving empty string as a false indicating that there's no base_script makes more sense since the method name specifies script_type. I can change it if you think returning base_type when script doesn't extend another script makes more sense

gilzoide · 2026-04-20T19:05:21Z

 	if (script.is_valid() && script->_is_valid()) {
 		result["name"] = script->_get_global_name();
-		result["base_type"] = script->_get_instance_base_type();
+		result["base_type"] = script->_get_instance_base_script_type().is_empty() ? script->get_instance_base_type() : script->_get_instance_base_script_type();


Why not stick to script->_get_instance_base_type() here, since the implementation already handles base scripts correctly?

Why not stick to script->_get_instance_base_type() here, since the implementation already handles base scripts correctly?

I don't think I understand the question. If I get base_type here in script_class_cache all the scripts would inherit from that type, not base script. Did you mean stick to _get_instance_base_script_type? If so then yes, if I don't return "" this would get much cleaner, I should do it that way

gilzoide · 2026-04-20T19:09:18Z

+					if (extends_script == nullptr) {
+						WARN_PRINT(String("Specified base class '%s' does not exist, using RefCounted") % Array::make(extends));
+					} else {
+						base_class = extends;


Why not get the extends_script->get_metadata().base_class here instead and let LuaScript::_get_instance_base_type just return it? So that base_class always has the name of a valid native GDCLASS.
Remember that in this branch extends is the result of extends_script->to_string().

Didn't want to deal with cross inclusions at that moment and then forgot about it, so yeah, I should fix it

gilzoide · 2026-04-20T19:15:02Z

+Ref<Script> get_class_script(const String& class_name) {
+	Array global_classes = ProjectSettings::get_singleton()->get_global_class_list();
+	for (int i = 0; i < global_classes.size(); i++) {
+		Dictionary cls = global_classes[i];
+		if (String(cls["class"]) == class_name) {
+			return ResourceLoader::get_singleton()->load(cls["path"], "Script");
+		}
+	}
+
+	return nullptr;
+}


This implementation only accepts global classes declared with class_name. I think this feature is unfinished without the support for loading scripts by path. You should at least support paths relative to res:// and uid://, although supporting relative paths from the current script would be a nice touch, such as is supported by GDScript.

This implementation only accepts global classes declared with class_name. I think this feature is unfinished without the support for loading scripts by path. You should at least support paths relative to res:// and uid://, although supporting relative paths from the current script would be a nice touch, such as is supported by GDScript.

I don't really understand this. If I'm trying to get class's script how can it return a script which isn't declared with class_name? Or are you talking about some code which uses this function?

I just googled and understood what you were talking about, didn't know that you can extend scripts by their path. I agree that this should be implemented, I'll think about how I can do it

gilzoide · 2026-04-20T19:21:47Z

I think the inheritance test should be more comprehensive. You should at least test that overriding methods in the derived class supersedes methods from the base class. Also test that get_script_property_list, get_script_method_list and the like return members from both Base and Derived classes.

Co-authored-by: Gil Reis <gilzoide@gmail.com>

illarn · 2026-04-24T16:16:23Z

@gilzoide hello! I have fixed everything from the feedback (apart from this, didn't understand the comment). Most notably:

inheritance from absolute/relative path works
metadata base class now always contains base class name
inheritance test expanded, added shadowing test, abs/rel derived test, base test, editor callbacks test (get methods/property/signal list)

I haven't moved get_property from LuaScript for reasons explained in the reply, but if you still think it's not fit there pls explain how should it be realized

illarn · 2026-04-24T18:21:31Z

also for some reason with luajit accessing godot Array after retrieving a callable results in a crash with no error. Have no idea why this happens, but only happens on this branch and with luajit. that's why CI test is failing. I'll take a look at why this might happen, LuaScript::get_property is my suspect, but can't be sure, extremely weird case

local node = Control:new()

-- Comment any of the 2 lines or reverse order = no crash, but this exact setup = crash
local methods = node.queue_free
local a = #Array({})
-- terminate called without an active exception
-- make: *** [Makefile:51: test] Aborted (core dumped)

gilzoide

Hey @illarn, thanks a lot for taking the time to update the code. Sorry it took me so long to review again, these last weeks have been busy here and I didn't want to rush this. There are still things that should be addressed, I've added comments in the relevant sections.

Again, thanks a lot for taking the time to implement this feature! Please let me know if there's any help I can further provide. I'll try to debug the crash with LuaJIT you mentioned, maybe I can find something that could help.

gilzoide · 2026-05-02T14:46:00Z

 						WARN_PRINT(String("Specified base class '%s' does not exist, using RefCounted") % Array::make(extends));
 					} else {
-						base_class = extends;
+						base_class = static_cast<Ref<LuaScript>>(extends_script)->get_metadata().base_class;


Wouldn't this crash in case extends_script is a Script but not a LuaScript?
If base scripts for Lua should always be a LuaScript and not just a plain Script (such as an instance of GDScript), make explicit checks and add error messages if necessary.

you're right, I can just get_instance_base_type() on base script and it will work with any script, fixed

gilzoide · 2026-05-02T14:53:42Z

 		if (Ref<LuaScript> base = get_base_script(); base != nullptr)
-			if (const Variant property = base->_get_property_default_value(p_property))
-				return property;
+			return property;


You should still call base->_get_property_default_value(p_property) here.

gilzoide · 2026-05-02T17:56:16Z

Hey @illarn, I found what's causing the crash: the Variant.__len metamethod implementation is using sol::variadic_args wrong, which will at least cause an error to be printed and at most crash the app. I just fixed it in #227, you can merge tip of branch main into yours.

illarn · 2026-05-03T17:00:43Z

@gilzoide Hello! Thank you for the feedback, I've returned get_func implementation to LuaScriptProperty as I understood it from your comment (can't reply to it for some reason) and fixed all the other problems. The luajit crash has also been fixed with #227, so everything should be ready for merging

gilzoide

Hey @illarn, thanks a lot for the updates, and sorry again for taking so long to review. I think we are really close now!

Apart from the inline comments, another thing I'd like to comment overall is about LuaScriptMetadata::base_script being a Ref<Script>. If pretty much every feature in LuaScript that traverses the base script chain casts the base script to Ref<LuaScript>, shouldn't we force that Lua scripts only extend other Lua scripts? Both GDScript and CSharpScript support only themselves as base and Godot doesn't traverse the script chain in general, it's a responsibility for the script / script instance.

All that said, I'd say we should either:

Force that Lua scripts extend only Lua scripts (or native classes, of course)
Add real support for extending scripts of other languages

The simplest is option 1, the official script languages already do this, so I don't think we need to bother trying to support the general case. But if you want to try option 2, it's up to you.

gilzoide · 2026-05-17T14:12:11Z

+	Ref<Script> current = script;
+	while (current.is_valid()) {
+		if (current == metadata.base_script)
+			return true;
+		current = current->get_base_script();
+	}
+
+	return false;


This implementation is incorrect, we should iterate over the base script chain for this instead of script's. As a reference, take a look at GDScript::inherits_script.

gilzoide · 2026-05-17T14:15:20Z

+	const LuaScriptMetadata metadata = p_instance->script->get_metadata();
+
+	// a) try calling `_get`
+	if (const LuaScriptMethod *_get = metadata.methods.getptr(string_names->_get)) {


We should use LuaScript::get_method here, in case the _get method is defined in a super class.

illarn added 5 commits April 13, 2026 14:30

WIP

2c07136

Merge branch 'main' into feat/script-inheritance

933ca22

added godot script inheritance required implementations

e1dabfd

removed leftover members from the LuaScriptMetadata

10ee856

deletion fix

68a5e79

illarn changed the title ~~Feat: lua script inheritance~~ Feat: lua script inheritance and is_abstract Apr 13, 2026

illarn changed the title ~~Feat: lua script inheritance and is_abstract~~ Feat: lua script inheritance & Feat: is_abstract Apr 13, 2026

improved code style consistency

def76a9

illarn added 4 commits April 15, 2026 01:56

Merge branch 'main' into feat/script-inheritance

a3d12a5

added lua script inheritance fallback for get method

5a8654c

WIP

b9eff83

fixed lua script inheritance + added tests

ecb102b

illarn marked this pull request as ready for review April 19, 2026 07:47

illarn mentioned this pull request Apr 19, 2026

Extending a lua class doesn't work #164

Open

fixed call_func

f56f7d2

illarn changed the title ~~Feat: lua script inheritance & Feat: is_abstract~~ feat: lua script inheritance & feat: is_abstract Apr 20, 2026

fix: duplicate methods, signals and properties in LuaScript

70b0b46

gilzoide requested changes Apr 20, 2026

View reviewed changes

illarn and others added 10 commits April 20, 2026 22:35

Update src/script-language/LuaScript.cpp

832f31e

Co-authored-by: Gil Reis <gilzoide@gmail.com>

Update src/script-language/LuaScript.cpp

f620ca8

Co-authored-by: Gil Reis <gilzoide@gmail.com>

Update src/script-language/LuaScript.cpp

3e0b41e

Co-authored-by: Gil Reis <gilzoide@gmail.com>

fixes from feedback

3c28815

fixes from feedback

d11eebc

fixes from feedback

ae0011b

fixes from feedback

850428a

fixes from feedback

553e67b

inheritance test improvement WIP

499a0a0

freeze fix

71d07a2

illarn added 2 commits April 24, 2026 18:56

inheritance fixes

7057902

inheritance test improvement

f535f76

inheritance test condition fix

36a0b85

gilzoide requested changes May 2, 2026

View reviewed changes

illarn added 3 commits May 3, 2026 15:29

Merge branch 'main' into feat/script-inheritance

b0de1cf

fixes from feedback

45460e9

returned get_func implementation to LuaScriptInstance

f50ad1b

gilzoide requested changes May 17, 2026

View reviewed changes

Uh oh!

Conversation

illarn commented Apr 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Goal

Changes

Uh oh!

illarn commented Apr 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

illarn commented Apr 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

illarn commented Apr 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

illarn commented Apr 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gilzoide left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gilzoide commented May 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

illarn commented May 3, 2026

Uh oh!

gilzoide left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gilzoide May 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

illarn commented Apr 13, 2026 •

edited

Loading

illarn commented Apr 14, 2026 •

edited

Loading

illarn commented Apr 19, 2026 •

edited

Loading

illarn commented Apr 24, 2026 •

edited

Loading

illarn commented Apr 24, 2026 •

edited

Loading

gilzoide commented May 2, 2026 •

edited

Loading

gilzoide left a comment •

edited

Loading

gilzoide May 17, 2026 •

edited

Loading