Re: [Tarantool-patches] [PATCH 2/3] lua: table.deepcopy ignores __pairs metamethod

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the patch!

See 5 comments below.

1. Commit title uses narration, while it should use imperative.
For example, "lua: don't use pairs() in table.deepcopy".

On 13/02/2020 21:33, olegrok@tarantool.org wrote:
> From: Oleg Babin <babinoleg@mail.ru>
> 
> After 1d85144a9b4bbbb026402848efde1ab98bf72633

2. After commit hash please put its title in ().

> table.deepcopy changed the behaviour and started
> iterate through tables considering __pairs metamethod.
> In some cases it broke backward compatibility.
> To avoid such problem let's ignore __pairs
> and iterate through tables as it was before
> 
> Closes #4770

3. Unfortunately, things are not that simple. This patch
does not really close the issue, because pairs(space_object)
is still broken. But your commit probably is useful anyway in
case we will decide to keep 5.2 __pairs/__ipairs. And it would
unblock vshard development.

Summary: just use 'Part of #4770' instead of 'Closes'.

> Follow-up #4560
> ---
> Issue: https://github.com/tarantool/tarantool/issues/4770
> Branch: https://github.com/tarantool/tarantool/tree/olegrok/table-fixes
>  src/lua/table.lua           |  7 ++++++-
>  test/app-tap/table.test.lua | 26 +++++++++++++++++++++++++-
>  2 files changed, 31 insertions(+), 2 deletions(-)
> 
> diff --git a/src/lua/table.lua b/src/lua/table.lua
> index d83217dcb..4fa9c421c 100644
> --- a/src/lua/table.lua
> +++ b/src/lua/table.lua
> @@ -1,3 +1,8 @@
> +-- This pairs implementation doesn't trigger __pairs metamethod
> +local function internal_pairs(tbl)
> +    return next, tbl, nil
> +end

4. I would call it 'rawpairs', by analogue with 'rawset/get/*'
functions.

> +
>  local function table_deepcopy_internal(orig, cyclic)
>      cyclic = cyclic or {}
>      local copy = orig
> @@ -10,7 +15,7 @@ local function table_deepcopy_internal(orig, cyclic)
>                  copy = cyclic[orig]
>              else
>                  cyclic[orig] = copy
> -                for orig_key, orig_value in pairs(orig) do
> +                for orig_key, orig_value in internal_pairs(orig) do
>                      local key = table_deepcopy_internal(orig_key, cyclic)
>                      copy[key] = table_deepcopy_internal(orig_value, cyclic)
>                  end
> diff --git a/test/app-tap/table.test.lua b/test/app-tap/table.test.lua
> index 07894f69e..3faf2ed23 100755
> --- a/test/app-tap/table.test.lua
> +++ b/test/app-tap/table.test.lua
> @@ -241,4 +241,28 @@ do -- gh-4340: deepcopy doesn't handle __metatable correctly.
>      )
>  end
>  
> +do -- gh-4770: deepcopy uses __pairs for iteration over table.
> +    local original = { a = 1, b = 2 }
> +
> +    local function custom_pairs(self)
> +        local function step(tbl, k)
> +            local k, v = next(tbl, k)
> +            if v ~= nil then
> +                v = v + 1
> +            end
> +            return k, v
> +        end
> +        return step, self, nil

5. I propose you to just add 'assert(false)' as a body of
custom pairs. To ensure that it is never called by
deepcopy. Because anyway it is not called now in your
test.

> +    end
> +
> +    setmetatable(original, {__pairs = custom_pairs })
> +
> +    -- Don't use is deeply as it could use pairs for check
> +    local copy = table.deepcopy(original)
> +    test:is(original.a, copy.a,
> +            "checking that the first values is correctly copied")
> +    test:is(original.b, copy.b,
> +            "checking that the second values is correctly copied")
> +end
> +
>  os.exit(test:check() == true and 0 or 1)
>