Modify Array/Dictionary::operator== to do real key/value comparison

[touilleMan]

Here is my attempt to fix the current infamous dictionary comparison behavior (see #27615)

Basically the issue is currently two Dictionary object only considered equal if they point on the same underlying hashmap.
This is very error prone given this behavior leaks into GDScript:

var a = {}
var b = {}
var c = a
print(a == b)  # false
print(a == c)  # true

There is already PR #29222 that tries to fix this by modifying Variant comparison. However I believe this is not the right way to go given it only fix GDScript, but not GDNative !

Currently GDNative's godot_dictionary_operator_equal is pretty useless (I would even say it is extremely error prone, I've personally been bitten multiple time by this function) and so GDNative users has to implement their own comparison (see https://github.com/touilleMan/godot-python/blob/72df9d1b38120aeec3c702004e14dcb82e95bde5/tools/builtins_templates/dictionary.tmpl.pxi#L180-L185) which is both cumbersome and inefficient.

Given I couldn't really figure out the usecase for this underlying-hashmap-only comparison, I've recompiled a modified version of Godot with Dictionary::operator== and Dictionary::operator!= removed (see touilleMan@2409aa6).
The only place those operator are used are in variant_op.cpp in the OP_EQUAL and OP_NOT_EQUAL operators.
So I guess I can officially say the current implementation of Dictionary::operator== is useless \o/

Of course, I think we should do the same for Array::operator==, I'll update this PR with those change if we agree on the idea ;-)

Bugsquad edit:

• Fixes Inspector can't show 3rd level of nesting for runtime dictionaries/arrays (fixed in master) #25375.
• Fixes Dictionaries are compared by reference with ==, unlike Arrays which are compared by value #27615.
• Fixes Dictionary values in instanced scene are not updated from parent #29221.

[pouleyKetchoupp]

From what you discovered about the use of == operators in the code base, it does seem like this is a better solution than PR #29222.

Two things I would like to mention:

• In that PR I've added a fix in project_settings_editor.cpp because deep dictionary comparison was causing a regression:
  Dictionary values saved for instanced scene only when different from parent #29222 (comment)

• Reduz had mentioned a potential issue with cyclical inclusions, which I figured is probably the case where the same dictionary is added inside of itself (for the most simple case), and it would cause an infinite loop.
  Dictionary values saved for instanced scene only when different from parent #29222 (comment)

[touilleMan]

@pouleyKetchoupp thanks a lot for you inputs ;-)

In that PR I've added a fix in project_settings_editor.cpp because deep dictionary comparison was causing a regression:

I'll had to this PR your change related to this issue then 👍

Reduz had mentioned a potential issue with cyclical inclusions, which I figured is probably the case where the same dictionary is added inside of itself (for the most simple case), and it would cause an infinite loop.

That's a really good point !
In fact recursion is already a trouble in the current code base, the following code cause a segfault on Godot 3.2:

var recursive_a_1 = [1]
recursive_a_1.append(recursive_a_1)
print("recursive_a_1 == recursive_a_1 ==> ", recursive_a_1 == recursive_a_1)

This is interesting given doing print("recursive: ", recursive_a_1) works as expected (it output recursive: [1, [...]]). This works because Variant::stringify keeps List<const void *> stack to track the recursivity.

So I've modified the PR to handle recursive check with a similar stack system.

However I fell like this code would benefit a lot from unit testing... but there is no such thing in Godot (well there is the https://github.com/godotengine/godot-tests repo, but it doesn't seems really used)

Here is the tests I've done manually:

var x1 = {}
var y1 = {}
x1[1] = y1
y1[1] = y1

var x2 = {}
var y2 = {}
x2[1] = y2
y2[1] = y2

print("x1 == x2 ==> ", x1 == x2)
# Test dictionary

var d1 = {1: 1}
var d2 = {1: 1}
var d1_ref2 = d1
var other_d = {1: 2}

print("d1 == d2 ==> ", d1 == d2)
print("d1 == d1_ref2 ==> ", d1 == d1_ref2)
print("d1 != other_d ==> ", d1 != other_d)

var nested1_d1 = {1: d1, 2: 2}
var nested2_d1 = {1: d1, 2: 2}
var nested3_d2 = {1: d2, 2: 2}
var other_d_nested = {1: other_d, 2: 2}

print("nested1_d1 == nested2_d1 ==> ", nested1_d1 == nested2_d1)
print("nested1_d1 == nested3_d2 ==> ", nested1_d1 == nested3_d2)
print("nested1_d1 != other_d_nested ==> ", nested1_d1 != other_d_nested)

var recursive_d_1 = {1: 1}
recursive_d_1[2] = recursive_d_1	
print("recursive_d_1 == recursive_d_1 ==> ", recursive_d_1 == recursive_d_1)

var recursive_d_2 = {1: 1}
recursive_d_2[2] = recursive_d_2
print("recursive_d_1 == recursive_d_2 ==> ", recursive_d_1 == recursive_d_2)

var cross_recursive_d_1 = {1: 1}
var cross_recursive_d_2 = {1: 1}
cross_recursive_d_1[2] = cross_recursive_d_2
cross_recursive_d_2[2] = cross_recursive_d_1
print("cross_recursive_d_1 == cross_recursive_d_2 ==> ", cross_recursive_d_1 == cross_recursive_d_2)

if false:
	var key_recursive_d_1 = {1: 1}
	var key_recursive_d_2 = {1: 1}
	# BUG !!!! This Crash here !
	key_recursive_d_1[key_recursive_d_2] = 1
	key_recursive_d_2[key_recursive_d_1] = 1
	print("key_recursive_d_1 == key_recursive_d_2 ==> ", key_recursive_d_1 == key_recursive_d_2)

if false:
	var x = ['x']
	var y = ['y']
	var z = ['z']
	y.append(y)
	x.append(y)
	x.append(y)
	# BUG !!!!! this prints "[x, [y, [...]], [...]]" but it should be "[x, [y, [...]], [y, [...]]]"
	print(x)

# Test array

var a1 = [1]
var a2 = [1]
var a1_ref2 = a1
var other_a = [2]

print("a1 == a2 ==> ", a1 == a2)
print("a1 == a1_ref2 ==> ", a1 == a1_ref2)
print("a1 != other_a ==> ", a1 != other_a)

var nested1_a1 = [a1, 2]
var nested2_a1 = [a1, 2]
var nested3_a2 = [a2, 2]
var other_a_nested = [other_a, 2]

print("nested1_a1 == nested2_a1 ==> ", nested1_a1 == nested2_a1)
print("nested1_a1 == nested3_a2 ==> ", nested1_a1 == nested3_a2)
print("nested1_a1 != other_a_nested ==> ", nested1_a1 != other_a_nested)

var recursive_a_1 = [1]
recursive_a_1.append(recursive_a_1)
print("recursive: ", recursive_a_1)
print("recursive_a_1 == recursive_a_1 ==> ", recursive_a_1 == recursive_a_1)

var recursive_a_2 = [1]
recursive_a_2.append(recursive_a_2)
print("recursive_a_1 == recursive_a_2 ==> ", recursive_a_1 == recursive_a_2)

var cross_recursive_a_1 = [1]
var cross_recursive_a_2 = [1]
cross_recursive_a_1.append(cross_recursive_a_2)
cross_recursive_a_2.append(cross_recursive_a_1)
print("cross_recursive_a_1 == cross_recursive_a_2 ==> ", cross_recursive_a_1 == cross_recursive_a_2)

As you can see I've also discovered two unrelated bugs ;-)

[RandomShaper]

What's the status of this?
I haven't reviewed the code thoroughly, but the change is very welcome.

[touilleMan]

@RandomShaper It should be ready to merge, my only concern is there is currently no place to add unittests in godot :'(

[Calinou]

It should be ready to merge, my only concern is there is currently no place to add unittests in godot :'(

See #30743.

[touilleMan]

I've rebased this PR ;-)

[touilleMan]

@akien-mga sorry to directly ping you, but I'm not sure what can be done to get this and #35813 merged...

[pouleyKetchoupp]

Just tested on master.
This PR fixes #29221 and possibly some other issues related to failing dictionary comparison.

[fire]

This branch has conflicts that must be resolved

Can this be rebased?

[touilleMan]

@fire I've rebased the PR

However it seems to have an issue related to variant_call.cpp when running the binary:

$  .\bin\godot.windows.opt.tools.64.exe
ERROR: Wrong argument name count supplied for method:  Dictionary::hash
   at: (core\variant_call.cpp:571)
ERROR: Wrong argument name count supplied for method:  Array::hash
   at: (core\variant_call.cpp:571)

I suspect this is linked to what @reduz said in #42780:

Note: while it should build ok this PR breaks release target (I think its broken already..), a subsequent PR refactoring method binds should fix it.

[Zylann]

Hang on, this basically replaces the behavior of == to compare by contents instead of reference? Or am I misunderstanding?
Because when this happened for arrays, we ended up with the impossibility to compare by ref, leading to #33627 and personally I didn't like that when it was introduced.
(see also discussion on #27615 (comment) and #27615 (comment))
Maybe it makes sense if you come from Python, but every other languages I used does not do that, and at least offers the option to compare either by ref or by value, instead of completely replacing it. Besides, this is not only about GDScript since you want to do this to core which will affect every language.

[touilleMan]

Hang on, this basically replaces the behavior of == to compare by contents instead of reference?

The idea is to replace the behavior of == in GDScript and in GDNative xxx_operator_equal, the simplest way to do that was to rewrite the C++'s operator== for Array and Dictionary (those operators are really scarcely used in the codebase).
It also seems a good idea to have a coherent comparison model: either GDScript ==, GDNative xxx_operator_equal and C++ operator== all do pointer comparison, either they all do value comparison.
Having a mix of that seems seems like a recipe for nasty bug in my opinion ;-)

Because when this happened for arrays, we ended up with the impossibility to compare by ref, leading to #33627 and personally I didn't like that when it was introduced.

Array.id()/Dictionary.id() is pretty close to your need (the only trouble is id() returns NULL when the array/dictionary is empty, so you cannot directly do is_different = a.id() != b.id())
I guess we could expose this id() method in GDScript (or a sightly modified version to overcome the NULL issue).

Maybe it makes sense if you come from Python, but every other languages I used does not do that, and at least offers the option to compare either by ref or by value, instead of completely replacing it.

In Python you can do a is b to test if a and b point to the same underlying object.
The point I guess is what's the least surprising behavior for == for GDScript users.
Given GDScript is inspired by Python I would say the == as value comparison is the most logical behavior.
Beside == is used for strings comparison, I guess it would be really confusing to do pointer comparison there so we have not much choice if we want to give == a coherent behavior ^^

But I think the most important thing is to have both kind of comparisons (per value and per pointer) available and documented (I'm sick of having to do that or that).

[aaronfranke]

@touilleMan This PR needs to be rebased on the latest master when you have the time.

[akien-mga]

The doc changes in the CI build seem to be due to changes in the printing of arrays:
https://github.com/godotengine/godot/pull/35816/checks?check_run_id=3442432599
https://github.com/godotengine/godot/pull/35816/files#diff-8fa43a0158f72fe267697c83f42d65e567c538c003320e7a9417faffd51f8bfeR1648

Might be a rebase mistake as those changed recently IINM.

[touilleMan]

@akien-mga Thanks for the tip ! I'll have a look at this ;-)

[touilleMan]

@akien-mga the last error in the build involve the documentation:

iff --git a/doc/classes/CodeEdit.xml b/doc/classes/CodeEdit.xml
index 93f72d45ae..60efd5d3a5 100644
--- a/doc/classes/CodeEdit.xml
+++ b/doc/classes/CodeEdit.xml
@@ -494,7 +494,6 @@
                <member name="line_length_guidelines" type="int[]" setter="set_line_length_guidelines" getter="get_line_length_guidelines" default="[]">
                        Draws vertical lines at the provided columns. The first entry is considered a main hard guideline and is draw more prominently
                </member>
-               <member name="structured_text_bidi_override_options" type="Array" setter="set_structured_text_bidi_override_options" getter="get_structured_text_bidi_override_options" override="true" default="[]" />
                <member name="symbol_lookup_on_click" type="bool" setter="set_symbol_lookup_on_click_enabled" getter="is_symbol_lookup_on_click_enabled" default="false">
                        Set when a validated word from [signal symbol_validate] is clicked, the [signal symbol_lookup] should be emitted.
                </member>

It is due to new array comparison behavior in here:

godot/editor/doc_tools.cpp

Line 309 in 84faf39

if (!default_value_valid || !base_default_value_valid || default_value == base_default_value) {

Here, when dealing with CodeEdit::structured_text_bidi_override_options, default_value and base_default_value are both an empty array. However before this PR the default_value == base_default_value would still be false (given comparison was done on pointers and not on values).

My guess is the per-pointer comparison was not intended here given this part of the code aims at not duplicating parent attributes that have been left untouched by children.

So my last commit 2351380d74f7604a9707f8e7b5892cec10dbb929 corrects the documentation, this PR should be finally ready to merge now ;-)

[RandomShaper]

I guess the commits could benefit from some squashing.

[touilleMan]

@RandomShaper rebased&squashed, ready for merge ;-)

[akien-mga]

The documentation for Dictionary (and maybe Array) should be updated to reflect these changes. At least Dictionary's description still has an explicit warning that Dictionary comparisons are not based on contents.

IIUC, we now lack a way to compare Arrays and Dictionaries by reference. It would be good to add one so that we can close all related issues / proposals as solved.