Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH 0/3] lua: table fixes
@ 2020-02-13 20:33 olegrok
  2020-02-13 20:33 ` [Tarantool-patches] [PATCH 1/3] lua: fix incorrect table.deepcopy __metatable handling olegrok
                   ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: olegrok @ 2020-02-13 20:33 UTC (permalink / raw)
  To: tarantool-patches, imun, v.shpilevoy; +Cc: Oleg Babin

From: Oleg Babin <babinoleg@mail.ru>

This patchset fixes three problems.
Two first are bugs in table.deepcopy function.
The third is similar to the second
(because the root of problem is a __pairs
metamethod) that I found then worked on the
second commit. 

Branch: https://github.com/tarantool/tarantool/tree/olegrok/table-fixes

Oleg Babin (3):
  lua: fix incorrect table.deepcopy __metatable handling
  lua: table.deepcopy ignores __pairs metamethod
  tap: is_deeply ignores __pairs metamethod

 src/lua/table.lua           |  9 ++++++--
 src/lua/tap.lua             |  9 +++++---
 test/app-tap/table.test.lua | 44 ++++++++++++++++++++++++++++++++++++-
 test/app-tap/tap.result     |  6 +++--
 test/app-tap/tap.test.lua   | 24 +++++++++++++++++++-
 5 files changed, 83 insertions(+), 9 deletions(-)

-- 
2.23.0

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [Tarantool-patches] [PATCH 1/3] lua: fix incorrect table.deepcopy __metatable handling
  2020-02-13 20:33 [Tarantool-patches] [PATCH 0/3] lua: table fixes olegrok
@ 2020-02-13 20:33 ` olegrok
  2020-02-13 22:50   ` Vladislav Shpilevoy
  2020-02-20 11:12   ` Igor Munkin
  2020-02-13 20:33 ` [Tarantool-patches] [PATCH 2/3] lua: table.deepcopy ignores __pairs metamethod olegrok
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 13+ messages in thread
From: olegrok @ 2020-02-13 20:33 UTC (permalink / raw)
  To: tarantool-patches, imun, v.shpilevoy; +Cc: Oleg Babin

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.
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
         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.
+    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)
-- 
2.23.0

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [Tarantool-patches] [PATCH 2/3] lua: table.deepcopy ignores __pairs metamethod
  2020-02-13 20:33 [Tarantool-patches] [PATCH 0/3] lua: table fixes olegrok
  2020-02-13 20:33 ` [Tarantool-patches] [PATCH 1/3] lua: fix incorrect table.deepcopy __metatable handling olegrok
@ 2020-02-13 20:33 ` olegrok
  2020-02-13 22:50   ` Vladislav Shpilevoy
  2020-02-20 11:00   ` Igor Munkin
  2020-02-13 20:33 ` [Tarantool-patches] [PATCH 3/3] tap: is_deeply " olegrok
  2020-02-13 22:50 ` [Tarantool-patches] [PATCH 0/3] lua: table fixes Vladislav Shpilevoy
  3 siblings, 2 replies; 13+ messages in thread
From: olegrok @ 2020-02-13 20:33 UTC (permalink / raw)
  To: tarantool-patches, imun, v.shpilevoy; +Cc: Oleg Babin

From: Oleg Babin <babinoleg@mail.ru>

After 1d85144a9b4bbbb026402848efde1ab98bf72633
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
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
+
 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
@@ -8,7 +8,7 @@ yaml.cfg{
     encode_invalid_as_nil  = true,
 }
 local test = require('tap').test('table')
-test:plan(33)
+test:plan(35)
 
 do -- check basic table.copy (deepcopy)
     local example_table = {
@@ -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
+    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)
-- 
2.23.0

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [Tarantool-patches] [PATCH 3/3] tap: is_deeply ignores __pairs metamethod
  2020-02-13 20:33 [Tarantool-patches] [PATCH 0/3] lua: table fixes olegrok
  2020-02-13 20:33 ` [Tarantool-patches] [PATCH 1/3] lua: fix incorrect table.deepcopy __metatable handling olegrok
  2020-02-13 20:33 ` [Tarantool-patches] [PATCH 2/3] lua: table.deepcopy ignores __pairs metamethod olegrok
@ 2020-02-13 20:33 ` olegrok
  2020-02-13 22:50   ` Vladislav Shpilevoy
  2020-02-20 10:57   ` Igor Munkin
  2020-02-13 22:50 ` [Tarantool-patches] [PATCH 0/3] lua: table fixes Vladislav Shpilevoy
  3 siblings, 2 replies; 13+ messages in thread
From: olegrok @ 2020-02-13 20:33 UTC (permalink / raw)
  To: tarantool-patches, imun, v.shpilevoy; +Cc: Oleg Babin

From: Oleg Babin <babinoleg@mail.ru>

After 1d85144a9b4bbbb026402848efde1ab98bf72633
is_deeply could use __pairs metamethod when
iterated through tables. It led to situation
when two similar tables could be considered
as different.
To eliminate such situation default pairs
was replaced with "pure" pairs that ignores
__pairs metamethod

Follow-up #4770, #4560
---
 src/lua/tap.lua           |  9 ++++++---
 test/app-tap/tap.result   |  6 ++++--
 test/app-tap/tap.test.lua | 24 +++++++++++++++++++++++-
 3 files changed, 33 insertions(+), 6 deletions(-)

diff --git a/src/lua/tap.lua b/src/lua/tap.lua
index 94b080d5a..23ca79f0b 100644
--- a/src/lua/tap.lua
+++ b/src/lua/tap.lua
@@ -77,7 +77,10 @@ local function skip(test, message, extra)
 end
 
 
-local nan = 0/0
+-- This pairs implementation doesn't trigger __pairs metamethod
+local function internal_pairs(tbl)
+    return next, tbl, nil
+end
 
 local function cmpdeeply(got, expected, extra)
     if type(expected) == "number" or type(got) == "number" then
@@ -107,7 +110,7 @@ local function cmpdeeply(got, expected, extra)
     local path = extra.path or '/'
     local visited_keys = {}
 
-    for i, v in pairs(got) do
+    for i, v in internal_pairs(got) do
         visited_keys[i] = true
         extra.path = path .. '/' .. i
         if not cmpdeeply(v, expected[i], extra) then
@@ -116,7 +119,7 @@ local function cmpdeeply(got, expected, extra)
     end
 
     -- check if expected contains more keys then got
-    for i, v in pairs(expected) do
+    for i, v in internal_pairs(expected) do
         if visited_keys[i] ~= true and (extra.strict or v ~= box.NULL) then
             extra.expected = 'key ' .. tostring(i)
             extra.got = 'nil'
diff --git a/test/app-tap/tap.result b/test/app-tap/tap.result
index 12bf86ec2..ecfdf11d1 100644
--- a/test/app-tap/tap.result
+++ b/test/app-tap/tap.result
@@ -137,7 +137,7 @@ not ok - failed subtests
   failed: 1
   ...
     # is_deeply
-    1..20
+    1..22
     ok - 1 and 1
     ok - abc and abc
     ok - empty tables
@@ -203,10 +203,12 @@ not ok - failed subtests
       strict: true
       ...
     ok - {a = box.NULL} and {a = box.NULL} strict true
+    ok - is_deeply ignores __pairs metamethod
+    ok - is_deeply ignores __pairs metamethod
     # is_deeply: end
 not ok - failed subtests
   ---
-  planned: 20
+  planned: 22
   failed: 8
   ...
     # like
diff --git a/test/app-tap/tap.test.lua b/test/app-tap/tap.test.lua
index e2a78f630..757781636 100755
--- a/test/app-tap/tap.test.lua
+++ b/test/app-tap/tap.test.lua
@@ -131,7 +131,7 @@ end)
 
 
 test:test('is_deeply', function(t)
-    t:plan(20)
+    t:plan(22)
 
     t:is_deeply(1, 1, '1 and 1')
     t:is_deeply('abc', 'abc', 'abc and abc')
@@ -166,6 +166,28 @@ test:test('is_deeply', function(t)
     t:is_deeply({a = box.NULL}, {a = box.NULL},
                 '{a = box.NULL} and {a = box.NULL} strict true')
     t.strict = false
+
+    --
+    -- gh-4770: is_deeply uses __pairs for iteration through the 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
+    end
+
+    setmetatable(original, {__pairs = custom_pairs })
+    t:is_deeply(original, {a = 1, b = 2},
+            'is_deeply ignores __pairs metamethod')
+    t:is_deeply({a = 1, b = 2}, original,
+            'is_deeply ignores __pairs metamethod')
 end)
 
 
-- 
2.23.0

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Tarantool-patches] [PATCH 3/3] tap: is_deeply ignores __pairs metamethod
  2020-02-13 20:33 ` [Tarantool-patches] [PATCH 3/3] tap: is_deeply " olegrok
@ 2020-02-13 22:50   ` Vladislav Shpilevoy
  2020-02-20 10:57   ` Igor Munkin
  1 sibling, 0 replies; 13+ messages in thread
From: Vladislav Shpilevoy @ 2020-02-13 22:50 UTC (permalink / raw)
  To: olegrok, tarantool-patches, imun; +Cc: Oleg Babin

Thanks for the patch!

Comments are basically the same as for 2/3 patch
about table.deepcopy: use imperative; rename to rawpairs;
add commit title in () after commit hash; use 'Part of'
for 4770, because it is not closed;

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Tarantool-patches] [PATCH 2/3] lua: table.deepcopy ignores __pairs metamethod
  2020-02-13 20:33 ` [Tarantool-patches] [PATCH 2/3] lua: table.deepcopy ignores __pairs metamethod olegrok
@ 2020-02-13 22:50   ` Vladislav Shpilevoy
  2020-02-20 11:00   ` Igor Munkin
  1 sibling, 0 replies; 13+ messages in thread
From: Vladislav Shpilevoy @ 2020-02-13 22:50 UTC (permalink / raw)
  To: olegrok, tarantool-patches, imun; +Cc: Oleg Babin

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)
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Tarantool-patches] [PATCH 1/3] lua: fix incorrect table.deepcopy __metatable handling
  2020-02-13 20:33 ` [Tarantool-patches] [PATCH 1/3] lua: fix incorrect table.deepcopy __metatable handling olegrok
@ 2020-02-13 22:50   ` Vladislav Shpilevoy
  2020-02-20 11:12   ` Igor Munkin
  1 sibling, 0 replies; 13+ messages in thread
From: Vladislav Shpilevoy @ 2020-02-13 22:50 UTC (permalink / raw)
  To: olegrok, tarantool-patches, imun; +Cc: Oleg Babin

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)
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Tarantool-patches] [PATCH 0/3] lua: table fixes
  2020-02-13 20:33 [Tarantool-patches] [PATCH 0/3] lua: table fixes olegrok
                   ` (2 preceding siblings ...)
  2020-02-13 20:33 ` [Tarantool-patches] [PATCH 3/3] tap: is_deeply " olegrok
@ 2020-02-13 22:50 ` Vladislav Shpilevoy
  2020-02-15 10:05   ` Oleg Babin
  3 siblings, 1 reply; 13+ messages in thread
From: Vladislav Shpilevoy @ 2020-02-13 22:50 UTC (permalink / raw)
  To: olegrok, tarantool-patches, imun; +Cc: Oleg Babin

Hi! Thanks for the patch!

On 13/02/2020 21:33, olegrok@tarantool.org wrote:
> From: Oleg Babin <babinoleg@mail.ru>
> 
> This patchset fixes three problems.
> Two first are bugs in table.deepcopy function.
> The third is similar to the second
> (because the root of problem is a __pairs
> metamethod) that I found then worked on the
> second commit. 
> 
> Branch: https://github.com/tarantool/tarantool/tree/olegrok/table-fixes

Usually we put issue number into branch name. Take a
look at branch list on GitHub for examples.

Also you need to put web links at the affected issues
here.

Additionally, we have a new rule, that behaviour changing
tickets should be reflected in changelog. This ticket changes
behaviour. So you need to add a changelog label. Like this:

@ChangeLog
- table.deepcopy now correctly handles __metatable attribute of
  a metatable (gh-4340).

I was also thinking about adding a docbot request, but seems like
table.* extensions are not documented anyway.

> 
> Oleg Babin (3):
>   lua: fix incorrect table.deepcopy __metatable handling
>   lua: table.deepcopy ignores __pairs metamethod
>   tap: is_deeply ignores __pairs metamethod
> 
>  src/lua/table.lua           |  9 ++++++--
>  src/lua/tap.lua             |  9 +++++---
>  test/app-tap/table.test.lua | 44 ++++++++++++++++++++++++++++++++++++-
>  test/app-tap/tap.result     |  6 +++--
>  test/app-tap/tap.test.lua   | 24 +++++++++++++++++++-
>  5 files changed, 83 insertions(+), 9 deletions(-)
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Tarantool-patches] [PATCH 0/3] lua: table fixes
  2020-02-13 22:50 ` [Tarantool-patches] [PATCH 0/3] lua: table fixes Vladislav Shpilevoy
@ 2020-02-15 10:05   ` Oleg Babin
  2020-02-15 15:35     ` Vladislav Shpilevoy
  0 siblings, 1 reply; 13+ messages in thread
From: Oleg Babin @ 2020-02-15 10:05 UTC (permalink / raw)
  To: Vladislav Shpilevoy, tarantool-patches, imun; +Cc: Oleg Babin

Hi! Thanks for your comments!

I asked Igor M. about this patchset and he said
that final decision has not yet been made and patch with 
__pairs/__ipairs metamethods could be reverted. Then two my patches will 
not be needed. Therefore I will wait final decision.

And I have a question about
> Additionally, we have a new rule, that behaviour changing
> tickets should be reflected in changelog. This ticket changes
> behaviour. So you need to add a changelog label. Like this:

Why I should get it from you but not from tarantool developers guide 
https://www.tarantool.io/en/doc/2.2/dev_guide/developer_guidelines/ ?

On 14/02/2020 01:50, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
> 
> On 13/02/2020 21:33, olegrok@tarantool.org wrote:
>> From: Oleg Babin <babinoleg@mail.ru>
>>
>> This patchset fixes three problems.
>> Two first are bugs in table.deepcopy function.
>> The third is similar to the second
>> (because the root of problem is a __pairs
>> metamethod) that I found then worked on the
>> second commit.
>>
>> Branch: https://github.com/tarantool/tarantool/tree/olegrok/table-fixes
> 
> Usually we put issue number into branch name. Take a
> look at branch list on GitHub for examples.
> 
> Also you need to put web links at the affected issues
> here.
> 
> Additionally, we have a new rule, that behaviour changing
> tickets should be reflected in changelog. This ticket changes
> behaviour. So you need to add a changelog label. Like this:
> 
> @ChangeLog
> - table.deepcopy now correctly handles __metatable attribute of
>    a metatable (gh-4340).
> 
> I was also thinking about adding a docbot request, but seems like
> table.* extensions are not documented anyway.
> 
>>
>> Oleg Babin (3):
>>    lua: fix incorrect table.deepcopy __metatable handling
>>    lua: table.deepcopy ignores __pairs metamethod
>>    tap: is_deeply ignores __pairs metamethod
>>
>>   src/lua/table.lua           |  9 ++++++--
>>   src/lua/tap.lua             |  9 +++++---
>>   test/app-tap/table.test.lua | 44 ++++++++++++++++++++++++++++++++++++-
>>   test/app-tap/tap.result     |  6 +++--
>>   test/app-tap/tap.test.lua   | 24 +++++++++++++++++++-
>>   5 files changed, 83 insertions(+), 9 deletions(-)
>>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Tarantool-patches] [PATCH 0/3] lua: table fixes
  2020-02-15 10:05   ` Oleg Babin
@ 2020-02-15 15:35     ` Vladislav Shpilevoy
  0 siblings, 0 replies; 13+ messages in thread
From: Vladislav Shpilevoy @ 2020-02-15 15:35 UTC (permalink / raw)
  To: Oleg Babin, tarantool-patches, imun; +Cc: Oleg Babin

> I asked Igor M. about this patchset and he said
> that final decision has not yet been made and patch with __pairs/__ipairs metamethods could be reverted. Then two my patches will not be needed. Therefore I will wait final decision.

Ok. It was also decided, that debug.* usage to get a metatable
is not acceptable, so we will port deepcopy() to C.

> 
> And I have a question about
>> Additionally, we have a new rule, that behaviour changing
>> tickets should be reflected in changelog. This ticket changes
>> behaviour. So you need to add a changelog label. Like this:
> 
> Why I should get it from you but not from tarantool developers guide https://www.tarantool.io/en/doc/2.2/dev_guide/developer_guidelines/ ?

This is not a question for me.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Tarantool-patches] [PATCH 3/3] tap: is_deeply ignores __pairs metamethod
  2020-02-13 20:33 ` [Tarantool-patches] [PATCH 3/3] tap: is_deeply " olegrok
  2020-02-13 22:50   ` Vladislav Shpilevoy
@ 2020-02-20 10:57   ` Igor Munkin
  1 sibling, 0 replies; 13+ messages in thread
From: Igor Munkin @ 2020-02-20 10:57 UTC (permalink / raw)
  To: olegrok; +Cc: Oleg Babin, tarantool-patches, v.shpilevoy

Oleg,

Thanks for the patch, but since we reverted the corresponding
breakage[1] I guess the changes are irrelevant. Discarding it.

On 13.02.20, olegrok@tarantool.org wrote:
> From: Oleg Babin <babinoleg@mail.ru>
> 
> After 1d85144a9b4bbbb026402848efde1ab98bf72633
> is_deeply could use __pairs metamethod when
> iterated through tables. It led to situation
> when two similar tables could be considered
> as different.
> To eliminate such situation default pairs
> was replaced with "pure" pairs that ignores
> __pairs metamethod
> 
> Follow-up #4770, #4560
> ---
>  src/lua/tap.lua           |  9 ++++++---
>  test/app-tap/tap.result   |  6 ++++--
>  test/app-tap/tap.test.lua | 24 +++++++++++++++++++++++-
>  3 files changed, 33 insertions(+), 6 deletions(-)
> 

<snipped>

> -- 
> 2.23.0
> 

[1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-February/014304.html

-- 
Best regards,
IM

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Tarantool-patches] [PATCH 2/3] lua: table.deepcopy ignores __pairs metamethod
  2020-02-13 20:33 ` [Tarantool-patches] [PATCH 2/3] lua: table.deepcopy ignores __pairs metamethod olegrok
  2020-02-13 22:50   ` Vladislav Shpilevoy
@ 2020-02-20 11:00   ` Igor Munkin
  1 sibling, 0 replies; 13+ messages in thread
From: Igor Munkin @ 2020-02-20 11:00 UTC (permalink / raw)
  To: olegrok; +Cc: Oleg Babin, tarantool-patches, v.shpilevoy

Oleg,

Thanks for the patch, but since we reverted the corresponding
breakage[1] I guess the changes are irrelevant. Discarding it.

On 13.02.20, olegrok@tarantool.org wrote:
> From: Oleg Babin <babinoleg@mail.ru>
> 
> After 1d85144a9b4bbbb026402848efde1ab98bf72633
> 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
> 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(-)
> 

<snipped>

> -- 
> 2.23.0
> 

[1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-February/014304.html

-- 
Best regards,
IM

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Tarantool-patches] [PATCH 1/3] lua: fix incorrect table.deepcopy __metatable handling
  2020-02-13 20:33 ` [Tarantool-patches] [PATCH 1/3] lua: fix incorrect table.deepcopy __metatable handling olegrok
  2020-02-13 22:50   ` Vladislav Shpilevoy
@ 2020-02-20 11:12   ` Igor Munkin
  1 sibling, 0 replies; 13+ messages in thread
From: Igor Munkin @ 2020-02-20 11:12 UTC (permalink / raw)
  To: olegrok; +Cc: Oleg Babin, tarantool-patches, v.shpilevoy

Oleg,

Thanks for the patch. I totally agree with Vlad regarding the debug.*
usage and porting table.deepcopy to C. Propose also to discard this
patch.

On 13.02.20, 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.
> 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(-)
> 

<snipped>

> -- 
> 2.23.0
> 

-- 
Best regards,
IM

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2020-02-20 11:17 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-13 20:33 [Tarantool-patches] [PATCH 0/3] lua: table fixes olegrok
2020-02-13 20:33 ` [Tarantool-patches] [PATCH 1/3] lua: fix incorrect table.deepcopy __metatable handling olegrok
2020-02-13 22:50   ` Vladislav Shpilevoy
2020-02-20 11:12   ` Igor Munkin
2020-02-13 20:33 ` [Tarantool-patches] [PATCH 2/3] lua: table.deepcopy ignores __pairs metamethod olegrok
2020-02-13 22:50   ` Vladislav Shpilevoy
2020-02-20 11:00   ` Igor Munkin
2020-02-13 20:33 ` [Tarantool-patches] [PATCH 3/3] tap: is_deeply " olegrok
2020-02-13 22:50   ` Vladislav Shpilevoy
2020-02-20 10:57   ` Igor Munkin
2020-02-13 22:50 ` [Tarantool-patches] [PATCH 0/3] lua: table fixes Vladislav Shpilevoy
2020-02-15 10:05   ` Oleg Babin
2020-02-15 15:35     ` Vladislav Shpilevoy

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox