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

Docs state that object.free() will make any reference to the object return null. This is not true. #35534

Closed
koodikulma-fi opened this issue Jan 24, 2020 · 11 comments

Comments

@koodikulma-fi
Copy link

koodikulma-fi commented Jan 24, 2020

Godot version:
3.2. beta5

OS/device including version:
Win10

Issue description:
The docs state that calling free():

Deletes the object from memory. Any pre-existing reference to the freed object will now return null.

However, after an object has been freed, references to it will not return null but instead an object (printing it will give [Deleted Object]). So !!object_ref returns true and not false which it does for null. I think it would be much more useful to actually return null (--> false as a boolean), like is clearly documented. Edit: --> So, only two states: null and [Object], so that there's no need to handle the third [Deleted Object] case (which is practically null, but currently technically not).

As this is such a basic thing, I'm probably missing something obvious. Anyhow, the latter part of the doc description was added in 3.1 (not there in 3.0), but this happens at least in 3.1.2 stable official and 3.2 beta5, and any earlier I've tried.

Steps to reproduce:

  1. Create an Object instance and refer to it: eg. var my_object = some_object_class.new()
  2. Free the object, eg. my_object.free()
  3. Check what it returns: eg. print(my_object) or print(!!my_object)

Minimal reproduction project:
FreedButNotNull.zip

Workarounds:
In my case, now considering to either do a special validation, eg. is_instance_valid(object), or attaching to the virtual _notify method of the important objects that might be deleted to check for NOTIFICATION_PREDELETE to tell others so they don't need to use is_instance_valid. For Nodes especially, the _notify is quite crowded, so it's not an optimal workaround (and for Nodes _exit_tree is not always an alternative: it might be called more often). So, hoping this is a bug.

@koodikulma-fi
Copy link
Author

koodikulma-fi commented Jan 29, 2020

Sorry, this is kind of a duplicate, couldn't find it at first: #20549

Anyway. To reiterate my view: It seems the dead object still has some uses - purely as a reference. So dead_obj_1 == dead_obj_2 returns false, while another_ref_to_dead_obj_1 == dead_obj_1 returns true.

.. In either case, the what the docs state is either plain wrong or at least confusing.

@Zireael07
Copy link
Contributor

The docs will probably become completely true with the merging of #35667

@KoBeWi
Copy link
Member

KoBeWi commented Apr 24, 2020

In 58cbec8 they print as [Freed Object]. Looks like the documentation is wrong or outdated.

@koodikulma-fi
Copy link
Author

My guess is that the documentation is ahead of implementation - since this was added recently, and there's a potential solution pending as Zireael07 pointed out.

@Zireael07
Copy link
Contributor

The one I pointed out was cancelled, and later another fix was made.

@akien-mga
Copy link
Member

Duplicate of #20549.

@akien-mga
Copy link
Member

Or well, not a duplicate if we consider that this one is about invalid docs while the other is a feature request.

@koodikulma-fi
Copy link
Author

Nice to know it's being worked on. Well, at the time of opening #20549, the docs were up to date in regards to this. :) I'd like this feature, but before it appears, I guess this issue is about invalid docs. (So anyone wondering about it, might end up reading this discussion and conclude that the feature will likely(?) appear later.)

@akien-mga
Copy link
Member

It's not clear whether #20549 will be implemented. What has been fixed is the problem of dangling Variants where a reference to a freed object could become a reference to a valid, different object.

@SekoiaTree
Copy link
Contributor

This should probably closed seeing as the mentionned PR is merged.

@KoBeWi
Copy link
Member

KoBeWi commented Aug 25, 2020

Yep, this is resolved.

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

6 participants