Make freed object different than null in comparison operators (reverted)

[vnen]

This is so everything is consistent, in particular with booleanization of objects. Now freed objects are considered a truthy value to agree with the fact they are different than null. Edit: I reversed this so freed objects are considered a falsy value instead, since I believe this is more intuitive and useful.

Also makes freed objects different from each other (if they are not the same reference).

Fix #59816

[RandomShaper]

I have a question:

    var a = Node.new()
    free(a)
    var b = Node.new()
    free(b)
    assert(a != b) # Is this guaranteed to succeed even if 'b' happened to take the same address as 'a'?

[vnen]

I have a question:

    var a = Node.new()
    free(a)
    var b = Node.new()
    free(b)
    assert(a != b) # Is this guaranteed to succeed even if 'b' happened to take the same address as 'a'?

It uses the ObjectID for comparison. Even if the address is the same, the ID should be different.

[vnen]

I'm still not sure if a freed Object should be truthy, given the following:

var a = Node.new()
a.free()

if a:
	# Do something with a

This would generate errors since a is not a valid object. The proper way to handle would be:

if is_instance_valid(a):
	# ...

Which is not as intuitive and require extra bookkeeping.

So maybe the best option is to make if a treat it as false if freed but a == null would also return false.

[RandomShaper]

Would one reason for a freed object not being falsy that on release you don't want the VM to be checking validity all the time, and therefore users are expected to use is_instance_valid() or, better, set their freed object vars to null after destruction?

Many other areas of Godot perform validity tests only on debug, to let the user warn something is not right, with the hope that they will fix it so release builds don't crash.

[vnen]

Would one reason for a freed object not being falsy that on release you don't want the VM to be checking validity all the time, and therefore users are expected to use is_instance_valid() or, better, set their freed object vars to null after destruction?

is_instance_valid() is doing the check anyway so moving this down won't make much of a difference since this is only important in the context of if obj anyway. Setting to null won't help in most cases where this happens because usually it is freed somewhere else (my example was just for context).

Many other areas of Godot perform validity tests only on debug, to let the user warn something is not right, with the hope that they will fix it so release builds don't crash.

That is more of an argument to having a freed object equivalent false. If the user writes if obj: then it is better if obj is valid when the block runs because those checks won't happen in release.

The main question is: do we want users to shoot themselves in their feet so they can learn proper memory handling or should we help them with an easier construct? IMO forcing the user to use is_instance_valid() sounds more annoying than just "hiding" it under the regular if check. If they want explicitness they can still use if obj == null.

[DaloLorn]

With regards to #73896 (comment):

I tend to agree with the sentiment that obj == null should evaluate to false instead of obj evaluating to true.

[vnen]

Updated the PR to consider a freed object to be falsy, so if freed_object: does not run the block and not freed_object returns true. I believe this is more intuitive way of handling this, acting as a shortcut for is_instance_valid() instead of forcing using the function everywhere.

[DaloLorn]

Yeah, this way you can also clearly distinguish between "this value was never set/was explicitly nulled" and "this value has been destroyed". I found this PR because I was considering an architecture for special abilities that might very well be interested in knowing whether a given object reference was ever assigned in the first place.

[RandomShaper]

I don't have a strong personal opinion, but the approach is consistent.

[AThousandShips]

I'm in support of this solution, and it helps in various contexts

[YuriSizov]

Thanks!

Reduz mentioned that this may make things slower now, but he didn't lean one way or another. So I'm fully trusting George and Pedro with this.

[vaartis]

If I may, I feel like deleted objects being not equal to null is, at the very least, a major breaking change. It also does not make sense to me to ever need to distinguish between a deleted object and null, since both mean that there is no more value there. Null means an absence of value, a deleted object is has become absent. If such distinction is really needed, I think it should be a separate function like is_object_deleted or something of that sort, instead of making it not equal to null which will probably subtly break a lot of people's projects in addition to being NOT how other engines do it (e.g. unity, where a deleted object is in fact equal to null).

[AThousandShips]

since both mean that there is no more value there.

They do not though, null means it's explicitly empty, this is true in other languages as well, please see the linked issue for the reasoning for this change, it's been extensively discussed, and been in the works for a long time in which objections could have been raised 🙂

You will also see this is a return to the correct behaviour used in 3.x

The previous behaviour was classed as a bug, and relying on it isn't safe and I'd not call that breaking compatibility, the behaviour was broken, the correct way to handle this before and after this change is to use is_instance_valid, not to check against null for this purpose, IMO

The change was also highlighted in the dev release so anyone can see it and adjust accordingly if they depended on the incorrect behaviour, but as this is not something people should have depended on I don't think it's valid to call it a breakage

[akien-mga]

After discussing with the GDScript and Core teams, we decided to revert this for now as there are still some aspects where we're not 100% sure what's the best behavior. We'll revisit this for a future release with more caution on how to avoid breaking user expectations and use cases, while fixing the inconsistency in the handling of freed objects.