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 18:41, Бабин Олег пишет:
> Ok. Thanks for your changes. Don’t forget to changelog entry and 
> docbot request and LGTM then.
>

Thanks for your answers!
Yep, sure. Done.

>     Понедельник, 16 августа 2021, 18:36 +03:00 от Serge Petrenko
>     <sergepetrenko@tarantool.org>:
>
>
>     16.08.2021 16:03, Бабин Олег пишет:
>     > Thanks for your fixes.
>     >
>     > Понедельник, 16 августа 2021, 10:53 +03:00 от Serge Petrenko
>     > <sergepetrenko@tarantool.org
>     </compose?To=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>
>     >
>     <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
>     <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>
>     > <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
>
> --
> Oleg Babin

-- 
Serge Petrenko