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

Core: Fix Object::has_method() for script static methods #82767

Merged

Conversation

dalexeev
Copy link
Member

@dalexeev dalexeev commented Oct 4, 2023

@RandomShaper
Copy link
Member

My take on this very interesting issue:

My guts tell me that Script::has_method() belongs to a different concern that Object::has_method(). In other words, a script resource has methods appropriate to its nature (dealing with it as a script), so you can't say that a script happening to define some method for the object it will be attached to means that the script resource itself provides that method. In yet other words, Object::has_method() is intended to check the interface of the object itself, while Script::has_method() is intended to check the interface of the script inside, only that both happened to get the same name, unfortunately.

Then, there has to be a way for an object to include the interface defined by any attached script in its own, for purposes like calling, property setting/getting, and checking the interface for a method by name. This last point has motivated me to check the current implementation of Object::has_method(), to see if it actually includes whatever interface the attached script enrichens it with. For this discussion, these are the relevant lines:

	if (script_instance && script_instance->has_method(p_method)) {
		return true;
	}

So, script-wise it will only check the methods in the script if there's an instance. My first thought was to add code to find a static method in the attached script. But that's not right, because the object the Callable is based on, and so it does the validity check on it, is the class, the Script.

@RandomShaper
Copy link
Member

Since I needed to test my hypothesis, I've done the changes and finally PRd them as #82770.

@akien-mga
Copy link
Member

To corroborate RandomShaper's review, the compiler warning treated as error in this PR also exposes the same problem:

in core/object/script_language.h:
'has_method' overrides a member function but is not marked 'override' [-Werror,-Winconsistent-missing-override]

Script::has_method is virtual and it's the one that GDScript and CSharpScript override. It's not the same as Object::has_method, they just unfortunately share the same name (and this warning thankfully does it job at pointing out that this is creating an unexpected relationship).

@dalexeev dalexeev force-pushed the core-make-object-has-method-virtual branch from 6140ce2 to 042217f Compare October 4, 2023 09:40
@dalexeev dalexeev changed the title Core: Make Object::has_method() virtual Core: Fix Object::has_method() for script static methods Oct 4, 2023
@dalexeev
Copy link
Member Author

dalexeev commented Oct 4, 2023

Yes, I was confused that this method is called the same in Variant, Object and Script.

Here's another solution (virtual multi-level _has_method()): dalexeev@6a9944e. But I think that this is overkill, and it might conflict with GDExtension in the future, if we decide to expose the method.

Copy link
Member

@RandomShaper RandomShaper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current version of this PR includes the best of all the ideas discussed.

Just a note for the next compat breakage: rename Script::has_(_static)?method() to Script::has_script_(_static)?method() (I think the already existing Script::has_script_signal() and others are named like that to disambiguate).

@dalexeev dalexeev force-pushed the core-make-object-has-method-virtual branch from 042217f to ed0b3c0 Compare October 4, 2023 16:46
@dalexeev dalexeev requested a review from a team as a code owner October 4, 2023 16:46
@dalexeev dalexeev requested a review from dsnopek October 4, 2023 16:46
Copy link
Contributor

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The GDExtension related changes look good to me! Thanks :-)

@akien-mga akien-mga merged commit 5cee7b0 into godotengine:master Oct 5, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Callables of static methods are not valid if accessed via the class's name
4 participants