GDScript: logical and/or between non boolean value cause a parser error

[ThakeeNathees]

Godot version: 7d972b8

OS/device including version: windows10

Issue description:
if both operands of the and/or operator are compile time known non boolean values, the parser throws error (where it shouldn't)

func _ready():	
	if "" or []:
		pass
## Parse Error: Invalid operands "String" and "Array" for "or" operator.

Minimal reproduction project:
N/A

[lyuma]

Confirmed on latest master 85186bc

[qarmin]

I'm not sure if this should be fixed.
What exactly means if ""? Is true when is empty or always false because it is not bool?
I like Rust approach when everything must be clear(if can only check bool condition) and "" will need to be changed to !"".is_empty()
But this will broke compatibilty and will need to add warning to 3.x branch to fix project earlier.

[vnen]

I don't think this is a bug, the logical operators should only accept booleans.

[vnen]

Closing as it's not a bug.

[vnen]

Reopening for discussion, since this worked on 3.x.

Is this really wanted? I can't see a lot of use cases for this TBH, I feel it's more likely to happen as an user error than anything else. Note that if "": should still work, just using boolean operations that doesn't work anymore.

cc @KoBeWi

[KoBeWi]

A few examples of such conditions in my code:

if last_action and Input.is_action_just_released(last_action): #last_action is String

if not menu.last_action or action != menu.last_action:

if in_water and randi() % 100 == 0: #in_water is array

if prev_crouch and not crouch and (check_raycast(Vector2(-15, -45), Vector2(-15, 0)) or check_raycast(Vector2(0, -45)) or check_raycast(Vector2(15, -45), Vector2(15, 0))): #check_raycast() is helper for intersect_ray(), which returns Dictionary

if animator.assigned_animation and  (target_animation.begins_with(animator.assigned_animation) or animator.assigned_animation.begins_with(target_animation)):

if is_inside_tree() and map_rect and not map_rect.has_point(global_position): #map_rect is Rect2

if not projectile_data: #Dictionary

Another common thing to do is if some_object and some_object.some_variable, where some_object might be null.

The main rationale behind keeping the old behavior is that these different objects do evaluate to boolean. If it's possible to do if "string", forbidding if not "string" doesn't make sense.

Another problem, this is more personal, is that long time ago I suggested that GDScript should treat non-booleans
like Ruby or LUA, i.e. anything is "true" if exists and "false" if it's null. I didn't like that in Godot an empty Dictionary or Array or Vector2 is "false". But eventually I got used to it, found it convenient and adapted into my code. And now, if suddenly we can't use these values for boolean operations, it would turn out to be a big mistake.

That said, is there any reason not to keep the old behavior? Does it impact performance or something? If there is a strong reason against supporting objects in boolean operations then I guess I could maybe live with that (in pain, but still ;_;). This mostly affects legacy projects. I'd have to add != "" or .is_empty() or == null in lots of places, but for new projects it's just minor inconvenience (it adds unnecessary comparisons and increases verbosity, but the code is more explicit I guess).

[vnen]

I'd argue that most of those would be more readable using explicit comparisons. E.g. using if not last_action.is_empty() and ... instead of just if last_action and ....

There's not really any strong reason for not supporting these though, so we might keep the status quo, even if it's not ideal. The only thing is that we have to add all possible combinations of types in core/variant/variant_op.cpp so GDScript (and potentially other scripting languages) can recognize those are valid.

[KoBeWi]

I don't think it's any less readable. Compare:

if some_object and some_bool:

if some_object != null and some_bool == true:

You wouldn't normally use some_bool == true, no? It doesn't matter whether it's boolean or not if it's inside a condition.

Anyways, keeping the status quo would be appreciated.

[vnen]

I don't think it's any less readable. Compare:

if some_object and some_bool:

if some_object != null and some_bool == true:

You wouldn't normally use some_bool == true, no? It doesn't matter whether it's boolean or not if it's inside a condition.

The thing is that booleans are usually named using a convention that makes them obvious in this context, so something like if is_in_floor is quite easy to understand. But something like if last_action can be ambiguous (is it trying to check if this is the last action in a potential list?). Also, if used in a condition I first assume it's a boolean, so something like if in_water makes me think it's a boolean, not an array. Same thing for object IMO, if some_object != null is usually better to read than just if some_object, even though for some reason objects were added to the new logical operators.

In any case, feel free to open a PR adding the missing combinations in the logical operators.

[Vaasref]

It goes beyond being easy to read. This issue is about not having a arbitrary language.

var arr:Array = []
if not arr:#error because you cannot use something that is not a bool with "not"
  print("array empty")
if arr:#no error
  print("not empty")
var other_condition:bool = true
if arr and other_condition:#error because you cannot use something that is not a bool with "and"
  print("not empty")

But it gets better !
GDScript is not supposed to force typing on you, is it ? So if my variable is not type then I need to also check for non-nullity before checking for emptiness, right ?
Nope, since it is not typed then I can just use the simpler check because ... reason ? (I mean if its not a bug then there must be a reason right ?)

var arr = []
if arr and arr[0] == 0:
  print("arr[0] is 0")

Typing your data you want to be more sure of the data it contains to not have to be as explicit when trying to use that data.

Now for the addressing the "less readable" argument.
Do you really think that :

var arr:Array = []
if arr and arr[0] == 0:
  print("arr[0] is 0")

... is less readable than :

var arr:Array = []
if arr.is_empty() and arr[0] == 0:
  print("arr[0] is 0")

I take that example because that is an extremely common occurrence for me, checking if something exists or is null before actually using it. How can it not be readable ? Why would you force the use of a more explicit when it is already more than obvious in that common case.
I'm not against being more explicit at time when it is warranted, but here it seems to me like a case of Enterprise Edition coding.
If you cannot understand a simple check of a object right before calling one of its function then I thing there is no amount of "!= null" that will help you and you should better get some sleep because you won't write any good code until then.

Me: I want python
Mom: No, we have python at home 
Python at home: ...

The previous version of GDScript was already an ersatz of python, but ok, it was only a bit rougher and mostly due to the way classes and built-in types were different.
But now it's really going backward on a very common syntax like that doesn't even work.

So yeah, that may not be a "bug" but that's clearly an oversight and the discrepancy in behavior makes it an issue and not just a matter of opinion about readability.

[h0lley]

I think a language should either be strict about if/and/or/not only taking boolean, or it should fully support truthy/falsey validation.

For a high level scripting language like GDScript I'd personally prefer the latter.
Users who want to be safe can always use == to ensure explicit boolean comparison.
Users / projects / cases where loose validation is fine should be able to use if/and/or/not with the same types.

When if "": is supported, then so should be if true and "":
When if []: is supported, then so should be if true and []: and if not []:

Got curious and made a breakdown:

	if in 3.x	not in 3.x	and/or in 3.x	if in 4.0.alpha	not in 4.0.alpha	and/or in 4.0.alpha
""	false	true	supported	false	⚠️ false, and only works with literal	⚠️ not supported
"a"	true	false	supported	true	false, ⚠️ only works with literal	⚠️ not supported
0	false	true	supported	false	true	supported
1	true	false	supported	true	false	supported
0.0	false	true	supported	false	true	supported
1.0	true	false	supported	true	false	supported
[]	false	true	supported	false	⚠️ not supported	⚠️ not supported
[1]	true	false	supported	true	⚠️ not supported	⚠️ not supported
{}	false	true	supported	false	⚠️ not supported	⚠️ not supported
{"a": 1}	true	false	supported	true	⚠️ not supported	⚠️ not supported
^""	false	true	supported	false	⚠️ false, and only works with literal	⚠️ not supported
^"a"	true	false	supported	true	false, ⚠️ only works with literal	⚠️ not supported
&""	n/a	n/a	n/a	true	false, ⚠️ only works with literal	⚠️ not supported
&"a"	n/a	n/a	n/a	⚠️ false	false, ⚠️ only works with literal	⚠️ not supported
Vector2.ZERO	false	true	supported	false	⚠️ false, and only works with literal	⚠️ not supported
Vector2.ONE	true	false	supported	true	false, ⚠️ only works with literal	⚠️ not supported
null	false	true	supported	false	true	supported
[Object]	true	false	supported	true	false	supported
[Freed Object]	true	false	supported	true	⚠️ true	supported

String and Vector only working with not as literals as well as not "" and not Vector2.ZERO validating to false seem to be separate bugs. Also [Freed Object] behaving weirdly, that's reported here: #59816

Types like Vector3, AABB, Quaternion, Rect2, etc. seem to be suffering from the same issues as Vector2.

No idea what's going on with NodePath and StringName. They are probably not really relevant in this table but still weird how they behave.

[KoBeWi]

Maybe it will help if we do what I suggested way back in #20634, which is treating everything except false and null as true? Right now we have lots of these weird cases like Vector.ZERO or [] that have custom bool evaluators and, as seen in this issue, this breaks with operators and has many inconsistencies. If we do my approach, then you'd need to only check the type of Variant and only evaluate bool (true/false). NIL would always be false and everything else true. idk if it's simpler to implement 🤔

The code would boil down to (ignorant pseudocode):

if variant.type == bool:
    return variant.operator bool()
else if variant.type == NIL
    return false
else
    return true

btw seems like Signal has also some boolean logic (valid = true, invalid = false), and probably Callable and RID too.

EDIT:
Forgot that null object is not NIL, so I guess that would be another exception.

[h0lley]

I think it's helpful when things that cannot be null validate to false when their content is empty/0, like an empty string, a dictionary without items, or a vector of magnitude 0.

when an user writes something like if arr:, then it's obvious that they mean to do if not arr.empty(): - I don't see how that could go against anyone's expectations, and find that it makes for better readability.

if, for an example, an array would always validate to true (as it can neither be false nor null), then there would no longer be an application for things like if arr:, at which point we could just as well do away with truthy/falsey validation altogether and have if take bool only.

also, I am for [Freed Object] validating to false as that would allow us to do away with is_instance_valid(). I don't know if most, but I can tell that many new users expect to be able to treat a [Freed Object] as null.

[KoBeWi]

I think it's helpful when things that cannot be null validate to false when their content is empty/0, like an empty string, a dictionary without items, or a vector of magnitude 0.

Yeah I was against it initially, but then incorporated it in my code and it's indeed convenient. But it was a trap, as this issue appeared and it broke lots of my code xd And it doesn't seem like restoring the 3.x behavior is easy/possible. Maybe the solution is to completely forbid non-booleans (maybe except objects) in conditions.

Personally I'm fine either way, as I started using is_empty() etc anyway, just to avoid such bugs.

also, I am for [Freed Object] validating to false as that would allow us to do away with is_instance_valid(). I don't know if most, but I can tell that many new users expect to be able to treat a [Freed Object] as null.

[Freed Object] is like that for technical reasons. Previously it was inconsistent between release and debug and it led to many bugs. IIRC making it evaluate to false implies some performance penalties due to validation.

[tavurth]

For my use case this is very unusable in the current state.

I'm very used to languages taking anything passed to a conditional if and automatically transforming to boolean.

if not true:
  pass

Works, whereas:

if not {}:
   pass

Or even:

if not self.name:
   pass

Fails with a compile error.

If we want to be so strict, we should offer a converter function type similar to in nim. Where I can explicitly convert my args of an if to bool.

Due to GDScripts "Open to Everyone" approach, I would say this is a bad idea.

Most JS & Python users are used to and or not reducing the operands to bool by default. Why should we force them an extra annoying step to coerce these values? It's GDscript, not GDc.

This is particularly visible for me since in GD3.x this worked, and it's now a regression.

[Vaasref]

Thanks to give that issue some attention. I really need some features from Godot 4 but the amount of refactoring that issue would imply made it very impractical.
It's fine to have to have to refactor code when features purposefully changed, but refactoring for a regression due to an oversight doesn't feel right.

[tavurth]

@Vaasref I'm sure this regression will be fixed soon, in some cases what I've done is simply added a helper to my Utils singleton:

func truthy(item) -> bool:
	return item and item != null

Please note this covers the following tests (port from Godot 3.x)

func test():
	assert(truthy("") == false)
	assert(truthy("a") == true)
	assert(truthy(0) == false)
	assert(truthy(1) == true)
	assert(truthy(0.0) == false)
	assert(truthy(1.0) == true)
	assert(truthy([]) == false)
	assert(truthy([1]) == true)
	assert(truthy({}) == false)
	assert(truthy({"a": 1}) == true)
	assert(truthy(null) == false)
	assert(truthy([Object]) == true)
	assert(truthy(Vector2.ONE) == true)
	assert(truthy(Vector2.ZERO) == false)

But does not cover the following Godot 4.x tests:

	assert(truthy(&"") == true)
	assert(truthy(&"a") == true)

	var a = Node.new()
	a.free()
	assert(truthy(a) == true)

In that way it can be easily refactored later when fixed.

IMO Godot 4.x is now worth it, and have ported my main projects over already.