Re: [Tarantool-patches] [PATCH 1/3] lua: fix incorrect table.deepcopy __metatable handling

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

Thanks for the patch!

See 3 comments below.

On 13/02/2020 21:33, olegrok@tarantool.org wrote:
> From: Oleg Babin <babinoleg@mail.ru>
> 
> Before this patch we got metatabe of original table
> using "getmetatable" function. It leads to the error
> if original table contained __metatable that could be
> e.g. a string or a number.

1. What error and why? (I know why, because I googled this,
but it should be in the commit message).

> To fix this problem getmetatable was replaced to debug.getmetatable.
> 
> Closes #4340
> ---


> Issue: https://github.com/tarantool/tarantool/issues/4340
>  src/lua/table.lua           |  2 +-
>  test/app-tap/table.test.lua | 20 +++++++++++++++++++-
>  2 files changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/src/lua/table.lua b/src/lua/table.lua
> index 7e8ba7aae..d83217dcb 100644
> --- a/src/lua/table.lua
> +++ b/src/lua/table.lua
> @@ -2,7 +2,7 @@ local function table_deepcopy_internal(orig, cyclic)
>      cyclic = cyclic or {}
>      local copy = orig
>      if type(orig) == 'table' then
> -        local mt, copy_function = getmetatable(orig), nil
> +        local mt, copy_function = debug.getmetatable(orig), nil

2. Please, add a comment why do you use debug instead of normal
getmetatable().

>          if mt then copy_function = mt.__copy end
>          if copy_function == nil then
>              copy = {}
> diff --git a/test/app-tap/table.test.lua b/test/app-tap/table.test.lua
> index 60c095fdf..07894f69e 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(31)
> +test:plan(33)
>  
>  do -- check basic table.copy (deepcopy)
>      local example_table = {
> @@ -223,4 +223,22 @@ do -- check usage of not __copy metamethod on second level + shallow
>      )
>  end
>  
> +do -- gh-4340: deepcopy doesn't handle __metatable correctly.

3. Please, put comment on a separate line, before 'do'. The code
above, which places comment and code on the same line, is
incorrect. Probably because it is old, and at that time there
was no such rule.

> +    local original = {
> +        content = 'string'
> +    }
> +    setmetatable(original, { __metatable = 'protection' })
> +    local copy = table.deepcopy(original)
> +    test:is(
> +            copy.content,
> +            original.content,
> +            "checking that original string was copied"
> +    )
> +    test:is(
> +            getmetatable(copy),
> +            'protection',
> +            "checking that __metatable was correctly copied"
> +    )
> +end
> +
>  os.exit(test:check() == true and 0 or 1)
>