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

JavaClassWrapper: Give additional error when trying to call non-static method directly on the class #102494

Conversation

dsnopek
Copy link
Contributor

@dsnopek dsnopek commented Feb 6, 2025

Fixes #102019

I'm not sure this is the right fix.

JavaClass is still returning CALL_ERROR_INSTANCE_IS_NULL when a developer tries to call a non-static method directly on the class, which means the GDScript VM will still show its not-very-helpful message. But this adds an additional error giving more helpful information.

I went this way because the other CALL_ERROR_* values aren't any more appropriate to this situation. We could do CALL_ERROR_INVALID_METHOD, but that has the same confusion as on the issue, where the developer listed the methods, and saw that this one exists, but they can't call it on the class. And looking at other places where Godot uses CALL_ERROR_INSTANCE_IS_NULL, it really seems like the most appropriate error.

Another option would be to modify the GDScript VM's message to say something about non-static methods, but that would be kinda weird because GDScript itself can detect that you're calling a non-static method in the parser, before it even gets to the VM, so this can't come up with pure GDScript or native classes. So, I don't know that it makes sense to change that?

I'd love feedback on if doing it this way makes sense, or if we should do one of the alternatives.

@dsnopek dsnopek force-pushed the java-class-wrapper-error-non-static-methods branch from a12f545 to 96bde8f Compare February 6, 2025 18:46
@akien-mga akien-mga merged commit e01ab79 into godotengine:master Feb 10, 2025
20 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.

Error when calling exisiting function using JavaClassWrapper
5 participants