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

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

16.08.2021 16:03, Бабин Олег пишет:
> Thanks for your fixes.
>
>     Понедельник, 16 августа 2021, 10:53 +03:00 от Serge Petrenko
>     <sergepetrenko@tarantool.org>:
>
>     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
>
> I’m not sure that it’s correct. I’ve added a reference to SO 
> (https://stackoverflow.com/questions/61946190/lua-eq-on-tables-with-different-metatables 
> <https://stackoverflow.com/questions/61946190/lua-eq-on-tables-with-different-metatables>) 
> and it seems condition should be following
> mta and mta.__eq and mta == mtb. Otherwise tables have different __eq 
> metamathods and you should compare their content.
> I’m not sure that my proposal is completely correct and suggest to ask 
> Igor about it.

Actually the question you refer to describes Lua 5.3, while LuaJIT 
supports Lua 5.1
The rules for Lua 5.1 are as follows (taken from 
https://www.lua.org/manual/5.1/manual.html#2.4):

Use the __eq metamethod only when both objects have the same one.

So, in my case, if at least one metamethod is defined, try to use 
comparison operator.
If both tables have the same __eq metamethod, Lua will call it.
If the tables don't share the same metamethod, they will be compared like
Lua does usually: by reference. The result will be false then.


I've added one more test though, just in case:

diff --git a/test/app-tap/table.test.lua b/test/app-tap/table.test.lua
index c7a075aba..ae6fdfa2d 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(43)
+test:plan(44)

  do -- check basic table.copy (deepcopy)
      local example_table = {
@@ -258,12 +258,15 @@ do -- check table.equals
          __eq = function(a, b) -- luacheck: no unused args
              return true
          end}
-    local tbl_d = setmetatable({a=15}, mt)
-    local tbl_e = setmetatable({b=2, c=3}, mt)
+    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,
+    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,
+    test:is(table.equals({a = 15}, tbl_d), false,
+            "table.equals when metatables don't match")
+    local tbl_f = setmetatable({a = 15}, {__eq = function() return true 
end})
+    test:is(table.equals(tbl_d, tbl_f), false,
              "table.equals when metatables don't match")
  end




>               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
>     <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
>
> --
> Oleg Babin

-- 
Serge Petrenko