Inconsistency in boolean validation of [Freed Object]

[h0lley]

Godot version

v4.0.alpha5.official [d7d528c]

System information

Ubuntu

Issue description

I'm aware we ought to use is_instance_valid() to validate an Object.
However in Godot 4 things may be a little different since comparison with null is now possible.
While [Freed Object] == null and not [Freed Object] was false in Godot 3, it is now true in 4.

However, [Freed Object] still validates to true in an if, while null validates to false.
This now results in very strange behavior:

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

if lele and not lele:
    print("this prints")

or:

if lele == null:
    print(lele, " is null")

if lele:
    print(lele, " is true")

if not null:
    print("null is false")

[Freed Object] is null
[Freed Object] is true
null is false

Many new users try comparison with null first before learning about is_instance_valid() (why this doesn't work in Godot 3 is a very common question). Since this yields the expected result now, they'll likely stick to it and never learn about is_instance_valid(). Treating [Freed Object] like null will however end up in users assuming if [Freed Object]: will validate to false, which it does not.

So here's the two solutions I can see (either or):

• Behavior of Godot 3 should be restored
  • Meaning [Freed Object] == null and not [Freed Object] should both be false again
• if [Freed Object]: should validate to false as well
  • Which would reduce complexity as we could actually do away with is_instance_valid()

Steps to reproduce

Run example code in a _ready function.

Minimal reproduction project

No response

[Calinou]

cc @vnen

[dalexeev]

I also wanted to report this. I hope this will be helpful.

print	3.4.2 Freed Object	3.4.2 null	4.0a5 Freed Object	4.0a5 null	Option 1	Option 2
value	[Deleted Object]	Null	[Freed Object]	null	[Freed Object]	[Freed Object]
not value	False	True	true	true	false	true
"+" if value else "-"	+	-	+	-	+	-
is_instance_valid(value)	False	False	false	false	false	false
value == null	False	True	true	true	false	true
typeof(value) == TYPE_NIL	False	True	false	true	false	true
typeof(value) == TYPE_OBJECT	True	False	true	false	true	false
value is Object	True	False	true	false	true	false
value is Node	Error	False	Error	false	???	false

Details

v3.4.2.stable.official [45eaa2d]

tool
extends EditorScript

func test(value):
    print("-----")
    print("value                         ", value)
    print("not value                     ", not value)
    print('"+" if value else "-"         ', "+" if value else "-")
    print("is_instance_valid(value)      ", is_instance_valid(value))
    print("value == null                 ", value == null)
    print("typeof(value) == TYPE_NIL     ", typeof(value) == TYPE_NIL)
    print("typeof(value) == TYPE_OBJECT  ", typeof(value) == TYPE_OBJECT)
    print("value is Object               ", value is Object)
    print("value is Node                 ", value is Node)

func _run():
    var object = ClassDB.instance("Object")
    object.free()
    test(object)

    var node = Node.new()
    node.free()
    test(node)

    test(null)

-----
value                         [Deleted Object]
not value                     False
"+" if value else "-"         +
is_instance_valid(value)      False
value == null                 False
typeof(value) == TYPE_NIL     False
typeof(value) == TYPE_OBJECT  True
value is Object               True
 res://_dev/_run.gd:14 - Left operand of 'is' was already freed.
-----
value                         [Deleted Object]
not value                     False
"+" if value else "-"         +
is_instance_valid(value)      False
value == null                 False
typeof(value) == TYPE_NIL     False
typeof(value) == TYPE_OBJECT  True
value is Object               True
 res://_dev/_run.gd:14 - Left operand of 'is' was already freed.
-----
value                         Null
not value                     True
"+" if value else "-"         -
is_instance_valid(value)      False
value == null                 True
typeof(value) == TYPE_NIL     True
typeof(value) == TYPE_OBJECT  False
value is Object               False
value is Node                 False

v4.0.alpha5.official [d7d528c]

@tool
extends EditorScript

func test(value):
    print("-----")
    print("value                         ", value)
    print("not value                     ", not value)
    print('"+" if value else "-"         ', "+" if value else "-")
    print("is_instance_valid(value)      ", is_instance_valid(value))
    print("value == null                 ", value == null)
    print("typeof(value) == TYPE_NIL     ", typeof(value) == TYPE_NIL)
    print("typeof(value) == TYPE_OBJECT  ", typeof(value) == TYPE_OBJECT)
    print("value is Object               ", value is Object)
    print("value is Node                 ", value is Node)

func _run():
    var object = ClassDB.instantiate("Object")
    object.free()
    test(object)

    var node = Node.new()
    node.free()
    test(node)

    test(null)

-----
value                         [Freed Object]
not value                     true
"+" if value else "-"         +
is_instance_valid(value)      false
value == null                 true
typeof(value) == TYPE_NIL     false
typeof(value) == TYPE_OBJECT  true
value is Object               true
  res://_dev/_run.gd:14 - Left operand of 'is' is a previously freed instance.
-----
value                         [Freed Object]
not value                     true
"+" if value else "-"         +
is_instance_valid(value)      false
value == null                 true
typeof(value) == TYPE_NIL     false
typeof(value) == TYPE_OBJECT  true
value is Object               true
  res://_dev/_run.gd:14 - Left operand of 'is' is a previously freed instance.
-----
value                         null
not value                     true
"+" if value else "-"         -
is_instance_valid(value)      false
value == null                 true
typeof(value) == TYPE_NIL     true
typeof(value) == TYPE_OBJECT  false
value is Object               false
value is Node                 false

I don't know how the is operator should behave in this case. Should inheritance be checked for freed objects?

print	Freed Object	Freed Node	null
value is Object	true	true	false
value is Node	false	true	false

It's probably not possible, but it's strange that is works for Object but doesn't work for child classes. In my understanding, is should be a safe operator.

• if [Freed Object]: should validate to false as well

I don't think this is the right solution, if [Freed Object]: and if not [Freed Object]: should not both be false (since not [Freed Object] should be false).

[h0lley]

I don't think this is the right solution, if [Freed Object]: and if not [Freed Object]: should not both be false (since not [Freed Object] should be false).

not [Freed Object] would be true as is currently the case, just like not null is true.
the two solutions I've listed are either or.

[dalexeev]

The error is still reproduced in 4.0 beta 1.

[dalexeev]

Related?

# 3.x
print(1 if ""  else 2) # 2
print(1 if !"" else 2) # 1

# 4.0 beta 3
print(1 if ""  else 2) # 2
print(1 if !"" else 2) # 2

[vmedea]

if [Freed Object]: should validate to false as well

I agree this is a weird situation (I came here from google, searching whether this behavior changed in 4). But also it can be useful to distingish a no-longer-existent object from a variable intentionally set (or initialized) to null. Say, we're tracking a target node, keeping this as a variable on the player. We'd want to know when it disappears. So I'm not sure it's a good idea to get rid of this distinction completely.

[vnen]

The main question is whether freed_object == null returns true or false. The rest should follow from this via transitivity (that is if freed_object == null and null == false then freed_object == false).

I do like the idea of having this to be true, since usually you are expecting a valid instance when you do the test. This avoids an extra check. It is also more beginner friendly since everyone hits the "freed object" problem and gets confused, this wouldn't happen anymore.

OTOH it might be a bug in the game logic if you are dealing with freed objects, so you may want to catch that and fix the source of the bug. If this is true then this check would be pretty much impossible. In fact, is_instance_valid() would become pointless.

[vnen]

Actually, it was pointed out to me that is_same(freed_object, null) is always false, so that would be one way to check for freed objects, even if freed_object == null is true.

[dalexeev]

null != false if I remember correctly. Transitivity is not always satisfied (at least for NAN, but there may be other cases). is_instance_valid() is still required, since Freed Object causes an error in 4.0 in some cases, unlike null (passing a parameter, the is operator, is_instance_of(), #73501, etc).

[vnen]

null != false if I remember correctly.

Well, that would be actually correct as it is, but I should have been clearer that I mean this as a conversion to bool, in particular when using as a condition to if, not a straight equality comparison.

The NAN case is very specific. We could consider freed the same as NAN: a missing value that always fail comparison. But I believe that this is a bit too much and can get quite confusing. Also, currently what happens is: freed1 == freed2, so different freed objects are evaluated to be equal (also with is_same(freed1, freed2)). I think they can be equal if actually the same (unlike NAN), but not equal to a different one.

To be extra clear, I'm not proposing to remove is_instance_valid() either way.

I believe it is established that if null: does not run the block. Then should if freed: run the block or not? If freed == null is true then the block should not run. That is, the should have the same "truthyness" value.

Same applies to not null (currently true) which is one of the major things in this issue that motivates to fix it now. At the moment if freed: runs the block and if not freed: also runs the block. That is clearly wrong.

Now, we could make so freed != null while keeping if freed: to not run the block. This would make if freed: have different behavior than if freed == null:, which may be acceptable, but I also think it's confusing. I´d rather see users complaining that they are getting "freed object" errors than they complaining if freed: is different than if if freed == null: while the same as if null:

---

In the end, I think I'll go with if freed: to run the block (so "truthy") and also freed != null. It is the most clear IMO, without hiding details from the users even if they end up hitting issues more often. At least they will be aware and able to fix those (or work around them).

[anvilfolk]

And then we just need to make sure in the documentation that if an object evaluates to true, but further down the line the code throws errors, then they need to use is_instance_valid() and more generally think about objects being freed somewhere else impacting code here :)

[dalexeev]

Now, we could make so freed != null while keeping if freed: to not run the block. This would make if freed: have different behavior than if freed == null:, which may be acceptable, but I also think it's confusing.

0, 0.0, "", Color(), etc. are also not equal to null or false, but are implicitly cast to false in the if context.

I don't mind if Freed Object is booleaned to false for convenience, but not freed should obviously be true in this case.

Moreover, this can be used not only with if, but also like here: var has_property = maybe_freed and maybe_freed.property.

Comparing a Freed Object to null is basically the wrong situation. I think hiding the error from the user here is a disservice. By making freed == null true, we only delay the moment the user realizes this, because in other situations there is still a difference between null and Freed Object.

It's sad that the user can shoot themselves in the foot by adding == null. If we want to prevent this, then the correct solution is to make the behavior the same as in 3.x IMO. That is, to make the error noticeable as early as possible. And recommend using is_instance_valid().

[akien-mga]

Reopening as #73896 is being reverted for now for 4.3.