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

Internal methods can be wrongly exposed to GDScript when using signals or deferred calls #32585

Open
pouleyKetchoupp opened this issue Oct 6, 2019 · 3 comments

Comments

@pouleyKetchoupp
Copy link
Contributor

Godot version:
Master dev

OS/device including version:
Any platform

Issue description:
This is a discussion about a general issue in the engine.

When c++ classes use signal connections or deferred calls, they are currently required to expose the called methods to scripts, which can cause some random issues.
Here's an example: #31408

The problem is scripts end up with undocumented functions they can call or override, which are not supposed to be used outside of the c++ class, and this can cause random bugs in case they are overridden by mistake.

I would be willing to work on a PR but I'd like to check first what is the best way to go about it. I can see two different approaches.

1. Color code in GDScript for overridden functions
This would help GDScript users to realize they are using an existing function by mistake. It wouldn't solve the problem itself, but it seems like it might be an easy way to alleviate it for script users, since it would work for all existing functions right away. So it would be interesting as a short term solution.

image

2. Flag in ClassDB::bind_method to disable exposure to scripts
This could be a proper long term solution, but it would require to make some changes for all the different cases individually.
When methods are not meant to be exposed, ClassDB::bind_method could use a flag or argument to disable scripts exposure, and only make it available for c++ signals and deferred calls.
There's already a flag called METHOD_FLAG_NOSCRIPT in MethodFlags that doesn't seem to be used for anything. Maybe it could be used this way?
This flag could be used in Object::call in order to disable calls to scripts when the method is not supposed to be exposed to them.

Would it make sense to implement one of these solutions, or even both?

Let me know if this discussion should be in the proposal repository instead. I put it here because it's not about a specific issue to solve for my project, but rather to solve lots of bugs that could happen in the future on top of the existing one I mentioned.

@mwerezak
Copy link

mwerezak commented Oct 6, 2019

#1 seems like a hack rather than a real solution. Just try thinking about how you would document this to an average Godot user who hasn't seen this Issue. If your method name changes color you need to rename your function or risk messing up C++ code you know nothing about? Talk about awkward.

@pouleyKetchoupp
Copy link
Contributor Author

Your point about teaching users makes sense, but I don't see how that discards the whole proposal. The current state is that you can mess with c++ code with certain function names, and there's no way to know about it without investigating the engine, so any kind of information for the user is an improvement.

That said, based on your feedback I would suggest we also have a tool-tip when hovering the function name that would explain it's overriding another function. Or an alternative solution could be to display an icon near the function name with a tool-tip, instead of a different color.

The main point is for solution 1 to provide a general short-term helper, so that users can diagnose bugs like #31408 a bit more easily, while solution 2 would be the actual long-term fix.

@mwerezak
Copy link

mwerezak commented Oct 7, 2019

The part about teaching was not so much my point but rather an illustration of how counter this behaviour is to basic programming practices.

If a function is not part of a public API, users shouldn't be able to mess with it by overriding it's behavior, period. There's really good reasons why the world follows this principle and I'm not going to explain that here.

I didn't realize that there would be an issue with going straight to solution 2 for a fix. If you really need to, then make it produce a GDScript error to override these functions.

Edit: Or the editor could produce a warning if people feel that is better. IMO though scripts that do this really should not run.

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

3 participants