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

Unexpose UtilityFunctions::is_instance_valid() #1513

Merged
merged 1 commit into from
Jul 15, 2024

Conversation

dsnopek
Copy link
Collaborator

@dsnopek dsnopek commented Jul 1, 2024

Fixes #1390

The is_instance_valid() function doesn't work as developers expect, and unless used very carefully, will cause crashes. Instead, developers should use ObjectDB::get_instance() with object ids to ensure that an instance is still valid.

This PR unexposes is_instance_valid() so that folks aren't tempted to use it.

I'm unsure if we should cherry-pick this or not. Technically it breaks source compatibility, but if developers are using this function, they probably have a bug that's waiting to be exposed. :-)

Note: Bindings aside from godot-cpp may be able to expose is_instance_valid() in a non-problematic way. However, since we're trying to match the Godot API and allow raw pointers to objects, we are stuck with this issue. If were able to disallow raw object pointers and require developers to use some kind of "object smart pointer object", that would be a different story.

Copy link
Contributor

@paddy-exe paddy-exe left a comment

Choose a reason for hiding this comment

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

LGTM. Makes sense to unexpose if the usage of the function will be breaking something down the line anyways. This should definitely be highlighted in the next release notes with a warning though to make people aware

@dsnopek dsnopek merged commit 0a1e31f into godotengine:master Jul 15, 2024
12 checks passed
@akien-mga akien-mga modified the milestones: 4.x, 4.3 Jul 22, 2024
@zhwei820
Copy link

zhwei820 commented Sep 2, 2024

So many change in downstream extensions..

@TokisanGames
Copy link

@zhwei820 It's broken.

See the bottom here: #1390 (comment)

ashtonmeuser added a commit to ashtonmeuser/godot-wasm that referenced this pull request Sep 12, 2024
@ashtonmeuser
Copy link

ashtonmeuser commented Sep 12, 2024

@dsnopek It seems that ObjectDB::get_instance() as suggested here and here isn't a perfect replacement. Starting with a Variant that represents a freed Object, we're unable to get an object ID in order to use ObjectDB::get_instance(). After ensuring a Variant is of type Object, an operator Object*() call causes a crash.

For example, the following dummy GDExtension library function and GDScript will produce a crash when supplying a freed Object:

// Must validate supplied object
godot_error MyLib::dummy(const Variant v) {
  if (v.get_type() != Variant::OBJECT) return ERR_CANT_CREATE;
  auto o = v.operator Object*(); // Crashes here if object was previously freed
  if (o == NULL) return ERR_CANT_CREATE;
  auto id = o->get_instance_id();
  auto x = ObjectDB::get_instance(id);
  if (x == NULL) return ERR_CANT_CREATE;
  return OK;
}
var o = Object.new()
var lib = MyLib.new()
assert(lib.dummy(o) == OK) # Expect object to be valid
o.free()
assert(lib.dummy(o) != OK) # Expect object to be invalid

Both assertion cases were previously correctly handled by UtilityFunctions::is_instance_valid() without error.

In a GDExtension context without UtilityFunctions::is_instance_valid(), I'm unable to find a way to properly validate a freed Object passed in as a Variant (ignoring super-hacky solutions e.g. v.stringify() == <Freed Object>).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaks compat enhancement This is an enhancement on the current functionality
Projects
None yet
8 participants