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

Prevent instantiating classes that aren't exposed #102440

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dsnopek
Copy link
Contributor

@dsnopek dsnopek commented Feb 4, 2025

Originally from #99534

Currently, if you call ClassDB::can_instantiate("...") for an internal class, it will return true, but most normal ways of instantiating the class in GDScript, GDExtension or C# won't work. However, they still can be created via via ClassDB::instantiate("..."). There's also no indication to GDExtension or scripting from ClassDB that these classes can't be instantiated normally because we don't expose ClassDB::class_is_exposed().

For example:

func _ready() -> void:
	# Prints: true
	print(ClassDB.class_exists("AnimationNodeStartState"))

	# Prints: true
	print(ClassDB.can_instantiate("AnimationNodeStartState"))

	# Prints: <AnimationNodeStartState#12345>
	print(ClassDB.instantiate("AnimationNodeStartState"))

	# ERROR!
	AnimationNodeStartState.new()

Do we even want internal classes to be instantiatable via ClassDB::instantiate("...")?

This PR answers no, and makes it so that internal classes return false for ClassDB::can_instantiate() and can't be created via ClassDB::instantiate("...").

I don't think there's anything that depends on the old behavior, but I'm not 100% sure, so treat this one with caution.

@dsnopek dsnopek force-pushed the classdb-cannot-instantiate-unexposed-classes branch from 46d8ca0 to be81b06 Compare February 4, 2025 23:09
@RandomShaper
Copy link
Member

Following you latest comment on the issue, I agree with this approach.

Aside, I'm wondering if we're using two different terms to mean the same across the API and implementation: "internal" and "not exposed". If that's true, we may want to harominze the terminology.

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.

2 participants