Re: [Tarantool-patches] [PATCH v2 1/2] lua: introduce table.equals method

[Serge Petrenko via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

13.08.2021 23:13, Oleg Babin пишет:
> Thanks for your fixes. Seems I found one more issue.
>
> ```
>
> mt = {__eq = function(a, b) return a.a == b.a end}
>
> t1 = setmetatable({a = 3}, mt)
>
> t2 = setmetatable({a = 3}, mt)
>
> tarantool> t1 == t2 -- metamethod is triggered
> ---
> - true
> ...
>
> tarantool> t1 == {a = 3} -- tables have different metatables
> ---
> - false
> ...
>
> tarantool> {a = 3} == t1
> ---
> - false
> ...
>
> tarantool> table.equals(t1, t2) -- OK
> ---
> - true
> ...
>
> tarantool> table.equals(t1, {a = 3}) -- Seems also ok
> ---
> - false
> ...
>
> tarantool> table.equals({a = 3}, t1) -- It's strange
> ---
> - true
> ...
>
> ```
>
>
> Seems you can use two tables if they have the same metatable.
>
> Probably here it's described better 
> https://stackoverflow.com/questions/32637684/equality-operator-on-mixed-types-in-lua
>
>

Thanks for a thorough review!

I've force pushed the following fixes:

=============================================

diff --git a/src/lua/table.lua b/src/lua/table.lua
index 3d5e69e97..edd60d1be 100644
--- a/src/lua/table.lua
+++ b/src/lua/table.lua
@@ -65,8 +65,11 @@ local function table_equals(a, b)
      if type(a) ~= 'table' or type(b) ~= 'table' then
          return type(a) == type(b) and a == b
      end
-    local mt = getmetatable(a)
-    if mt and mt.__eq then
+    local mta = getmetatable(a)
+    local mtb = getmetatable(b)
+    -- Let Lua decide what should happen when at least one of the 
tables has a
+    -- metatable.
+    if mta and mta.__eq or mtb and mtb.__eq then
          return a == b
      end
      for k, v in pairs(a) do
diff --git a/test/app-tap/table.test.lua b/test/app-tap/table.test.lua
index ec81593f3..029b923fb 100755
--- a/test/app-tap/table.test.lua
+++ b/test/app-tap/table.test.lua
@@ -8,7 +8,7 @@ yaml.cfg{
      encode_invalid_as_nil  = true,
  }
  local test = require('tap').test('table')
-test:plan(40)
+test:plan(43)

  do -- check basic table.copy (deepcopy)
      local example_table = {
@@ -254,6 +254,14 @@ do -- check table.equals
      test:ok(table.equals(tbl_a, tbl_c),
              "table.equals for shallow copied tables after modification")
      test:is(table.equals(tbl_a, tbl_b), false, "table.equals does a 
deep check")
+    local mt = {__eq = function(a, b) return true end}
+    local tbl_d = setmetatable({a=15}, mt)
+    local tbl_e = setmetatable({b=2, c=3}, mt)
+    test:ok(table.equals(tbl_d, tbl_e), "table.equals respects __eq")
+    test:is(table.equals(tbl_d, {a=15}), false,
+            "table.equals when metatables don't match")
+    test:is(table.equals({a=15}, tbl_d), false,
+            "table.equals when metatables don't match")
  end

  os.exit(test:check() == true and 0 or 1)

=============================================

>
> On 13.08.2021 13:22, Serge Petrenko wrote:
>>
>>
>> 13.08.2021 08:30, Oleg Babin пишет:
>>> Thanks for your patch.
>>>
>>>
>>> It seems it works in quite strange way:
>>>
>>> ```
>>>
>>> tarantool> table.equals({a = box.NULL}, {})
>>> ---
>>> - true
>>> ...
>>>
>>> tarantool> table.equals({}, {a = box.NULL})
>>> ---
>>> - false
>>> ...
>>>
>>> ```
>>>
>>>
>>> I can change arguments order to get different result. Expected false 
>>> in both cases.
>>>
>>> For tap it was considered as buggy behaviour 
>>> https://github.com/tarantool/tarantool/issues/4125
>>>
>>>
>>
>> ====================================
>>
>> Good catch! Thanks!
>>
>> Check out the diff:
>>
>> diff --git a/src/lua/table.lua b/src/lua/table.lua
>> index 5f35a30f6..3d5e69e97 100644
>> --- a/src/lua/table.lua
>> +++ b/src/lua/table.lua
>> @@ -63,7 +63,7 @@ end
>>  -- @return true when the two tables are equal (false otherwise).
>>  local function table_equals(a, b)
>>      if type(a) ~= 'table' or type(b) ~= 'table' then
>> -        return a == b
>> +        return type(a) == type(b) and a == b
>>      end
>>      local mt = getmetatable(a)
>>      if mt and mt.__eq then
>> diff --git a/test/app-tap/table.test.lua b/test/app-tap/table.test.lua
>> index a3c9aa123..ec81593f3 100755
>> --- a/test/app-tap/table.test.lua
>> +++ b/test/app-tap/table.test.lua
>> @@ -8,7 +8,7 @@ yaml.cfg{
>>      encode_invalid_as_nil  = true,
>>  }
>>  local test = require('tap').test('table')
>> -test:plan(38)
>> +test:plan(40)
>>
>>  do -- check basic table.copy (deepcopy)
>>      local example_table = {
>> @@ -227,6 +227,10 @@ do -- check table.equals
>>      test:ok(table.equals({}, {}), "table.equals for empty tables")
>>      test:is(table.equals({}, {1}), false, "table.equals with one 
>> empty table")
>>      test:is(table.equals({1}, {}), false, "table.equals with one 
>> empty table")
>> +    test:is(table.equals({key = box.NULL}, {key = nil}), false,
>> +            "table.equals for box.NULL and nil")
>> +    test:is(table.equals({key = nil}, {key = box.NULL}), false,
>> +            "table.equals for box.NULL and nil")
>>      local tbl_a = {
>>          first = {
>>              1,
>>
>> ====================================
>>> On 13.08.2021 02:30, Serge Petrenko via Tarantool-patches wrote:
>>>> Introduce table.equals for comparing tables.
>>>> The method respects __eq metamethod, if provided.
>>>>
>>>> Needed-for #5894
>>>> ---
>>>>
>>

-- 
Serge Petrenko