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

has_method does not work on class #22838

Open
DavidSichma opened this issue Oct 7, 2018 · 14 comments
Open

has_method does not work on class #22838

DavidSichma opened this issue Oct 7, 2018 · 14 comments
Assignees
Milestone

Comments

@DavidSichma
Copy link
Contributor

Godot version:
current master

OS/device including version:
Windows, don't think it matters

Issue description:
Investigating #22833 I found out that has_method does not work with classes.
Consider the following script:

extends Node

class Test:
	static func foo():
		print("foo")

func _ready():
	Test.foo() # works as expected
	print(Test.has_method("foo")) # false although Test has this method

In #22833 this bug makes it impossible to use a static func of a class in array.sort_custom().

Steps to reproduce:
Call has_method on a class.

Minimal reproduction project:
sfhm.zip

@DavidSichma
Copy link
Contributor Author

DavidSichma commented Oct 7, 2018

Changing this line in object.h

bool has_method(const StringName &p_method) const;

and making this bool virtual seems to (partially) fix the issue. The Script class has its own has_method so it should work if correctly implemented.

As I didn't test it with VisualScript and C# I am not comfortable committing this change.

@akien-mga akien-mga added this to the 3.1 milestone Oct 7, 2018
@akien-mga
Copy link
Member

CC @reduz @vnen @neikeq

@neikeq
Copy link
Contributor

neikeq commented Oct 7, 2018

The scripting API doesn't support static method. As such, has_method is not static. You need an instance to access it.

Changing this line in object.h

bool has_method(const StringName &p_method) const;

and making this bool virtual seems to fix the issue. The Script class has its own has_method so it should work if correctly implemented.

I don't understand how that would change anything. Could you give more details?

@DavidSichma
Copy link
Contributor Author

DavidSichma commented Oct 8, 2018

In Objects there is this method defined:

bool has_method(const StringName &p_method) const;

If I get it right this method checks if the method is either specified in an atached script or in the class.

However there is also a has_method() defined in the Script class which inherits Object. The implementations of the derived classes seem work for static functions (tested only for GDScript).

virtual bool has_method(const StringName &p_method) const = 0;

This already caused duplicate issues: #10785

If I understand c++ correctly, correct my if I'm wrong, the following happens:

  • Object has has_method which is not virtual and can not be overridden.
  • Script classes have their own has_method which causes the Object's implementation to be hidden but not overriden.
  • Calling has_method() on any object whose type isn't specified will call the Object's version.
  • Changing to virtual will essentialy make the Object's version possible to be overridden. This way the correct method will be called.

This makes it possible to check for static method in inner classes. The inner classes are of type GDScript and their has_method works for both static and non static functions (I don't know if this is intended behavior but it just works in this context).

Note that I only tested this with GDScript. I have not testet it in VisualScript and C# nor do I plan learning C# just for that. Also I don't know if this could potentialy create other issues with has_method I can't foresee.

@DavidSichma
Copy link
Contributor Author

Ok, changing has_method to virtual won't solve all problems.

The biggest one is that just overriding will get rid of the functionality in object's implementation. With this "fix" has_method on any script will not find

  • free()
  • methods in an attached script (though attaching a script to a script might be very rare)
  • methods defined not in the script itself. For example has_method can not be found.

Furthermore static functions of parents can not be found, although they are callable.

Advice would be appreciated. Or if someone wants to try it themself, go for it.

@DavidSichma
Copy link
Contributor Author

If someone is interested: Here are the test cases I could think of:

extends Node

class Foo:
	static func foo():
		return "foo"
	func foo_non_static():
		return "foo-non-static"

class Bar extends Foo:
	static func bar():
		return "bar"
	func bar_non_static():
		return "bar-non-static"

class scrpt:
	func script_func():
		return "script_func"

static func fun():
	return "fun"

func fun_non_static():
	return "fun_non_static"

func _ready():
	print("Test on self")
	_test(self, self.fun(), "fun")
	_test(self, self.fun_non_static(), "fun_non_static")
	
	print("Test on inner class")
	_test(Foo, Foo.foo(), "foo")
	_test(Foo, Foo.foo_non_static(), "foo_non_static")
	
	print("Test on instance of inner class")
	var fo = Foo.new()
	_test(fo, fo.foo(), "foo")
	_test(fo, fo.foo_non_static(), "foo_non_static")

	print("Test on inherited inner class")
	_test(Bar, Bar.foo(), "foo")
	_test(Bar, Bar.foo_non_static(), "foo_non_static")

	print("Test on instance of inherited inner class")
	var ba = Bar.new()
	_test(ba, ba.foo(), "foo")
	_test(ba, ba.foo_non_static(), "foo_non_static")
	
	print("Adding script to instance of inner class")
	fo.set_script(scrpt)
	_test(fo, fo.script_func(), "script_func")
	print("Adding script to inner class")
	Foo.set_script(scrpt)
	_test(Foo, Foo.script_func(), "script_func")
	
	print("Foo has " + ("" if Foo.has_method("has_method") else "no ") + "class methods")

func _test(var obj, var call, var method_name):
	var res = "Calling %s() on %s: \"%s\" and method can%s be found" % [method_name, obj, call, "" if obj.has_method(method_name) else " not"]
	print(res)

@neikeq
Copy link
Contributor

neikeq commented Oct 9, 2018

First of all, Script::has_method is very likely unrelated to Object::has_method. If I were to go the virtual has_method way, I would rename the method in Script to something else first and then add the has_method override.

The biggest one is that just overriding will get rid of the functionality in object's implementation

This can be fixed by calling the object's implementation if the method was not found.

Furthermore static functions of parents can not be found, although they are callable.

This should be simple as well. Just calling base->has_method(p_method) as well should be enough.

This won't work in any other language (except VisualScript perhaps), unless they have a way to get the Script from a type.
The reason it works in GDScript is likely because a Type identifier seems to represent the Script itself (the language is designed to be well integrated with Godot). I would be surprised if it wasn't the same for VisualScript.
Theorically we could provide a method GetScriptFromType(typeof(Foo)) or GetScriptFromType<Foo>() in C# which would return the Script so you can call HasMethod on it. Not sure about other languages.

akien-mga added a commit that referenced this issue Oct 9, 2018
…od exists."

This reverts commit 6415454.

That patch was correct but Object::has_method is not a reliable way to check
if we can use the given method, as it doesn't support inner classes (#22838).
@akien-mga
Copy link
Member

Note: I've reverted 6415454 with c730957 due to this bug. Once we have a solution for this issue, we could re-add 6415454.

@DavidSichma
Copy link
Contributor Author

Thanks for the tip! I made the changes and it works fine.

I have renamed Script::has_method to Script::defines_method as this method just lists methods defined in this very script. I also updated the calls, so no Object::has_method will be called by accident.

@reduz
Copy link
Member

reduz commented Nov 1, 2018

This will be properly fixable when methods become actual members. @vnen is going to work on this for Godot 3.2, so kicking there.

@reduz reduz modified the milestones: 3.1, 3.2 Nov 1, 2018
@akien-mga akien-mga modified the milestones: 3.2, 4.0 Jan 9, 2020
@YuriSizov
Copy link
Contributor

Will this be a part of 4.0 or was it just postponed to the latest available version? I guess it is related to the introduction of a Callable type? Also, what is going to be the solution? Will has_method work with both static and instanced methods or will we get a separate method for static checks?

@willnationsdev
Copy link
Contributor

Will this be a part of 4.0 or was it just postponed to the latest available version? I guess it is related to the introduction of a Callable type?

Yes, I believe this is being postponed till 4.0 (or, if not then, 4.1) since reduz's "when methods become actual members" comment refers to the introduction of the Callable type happening in 4.0.

I suspect the solution will just be changing the implementation of has_method() to support it. It would involve having the Script check static functions on a would-be ScriptInstance that hasn't yet been created, so the way that data is stored will likely be updated (probably by virtue of the Callable changes).

Will has_method work with both static and instanced methods or will we get a separate method for static checks?

My guess would be that it would be the same method for both, but it would just be updated so that instance and static methods work on ScriptInstances and static methods work on Scripts.

@Zylann
Copy link
Contributor

Zylann commented Jun 26, 2021

Just FYI, this problem occurs too with regular classes, not just class:

	var klass = CanvasItem
	# prints `false`
	print(klass.has_method("hide"))

Arguably here you could use ClassDB instead (and in fact you have to because CanvasItem cannot be instanced), but that only works with engine classes, so it misses a point that "class objects" could be managed with the same interface.

@akien-mga
Copy link
Member

Is this still reproducible in 4.0? (I believe so and it might be tracked in newer issues too, but I didn't check.)

@akien-mga akien-mga modified the milestones: 4.0, 4.x Feb 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants