[Tarantool-patches] [PATCH v3] error: add __concat method to error object

[Tarantool-patches] [PATCH v3] error: add __concat method to error object

[olegrok]

From: Oleg Babin <babinoleg@mail.ru>

Usually functions return pair {nil, err} and expected that err is string.
Let's make the behaviour of error object closer to string
and define __concat metamethod.

Closes #4489

The case of error "error_mt.__concat(): neither of args is an error"
is not covered by tests because of #4723
---
Changes in v3:
  - Improve concatenation logic
  - Add more test cases
Changes in v2:
  - Added tests

Issue: https://github.com/tarantool/tarantool/issues/4489
Branch: olegrok/4489-concat-for-errors

 src/lua/error.lua      | 11 +++++++
 test/box/misc.result   | 75 +++++++++++++++++++++++++++++++++++++++++-
 test/box/misc.test.lua | 24 +++++++++++++-
 3 files changed, 108 insertions(+), 2 deletions(-)

diff --git a/src/lua/error.lua b/src/lua/error.lua
index d091ee48b..7f249864a 100644
--- a/src/lua/error.lua
+++ b/src/lua/error.lua
@@ -160,9 +160,20 @@ local function error_index(err, key)
     return error_methods[key]
 end
 
+local function error_concat(lhs, rhs)
+    if ffi.istype('struct error', lhs) then
+        return tostring(lhs) .. rhs
+    elseif ffi.istype('struct error', rhs) then
+        return lhs .. tostring(rhs)
+    else
+       error('error_mt.__concat(): neither of args is an error')
+    end
+end
+
 local error_mt = {
     __index = error_index;
     __tostring = error_message;
+    __concat = error_concat;
 };
 
 ffi.metatype('struct error', error_mt);
diff --git a/test/box/misc.result b/test/box/misc.result
index 3ca4e31ac..5ac5e0f26 100644
--- a/test/box/misc.result
+++ b/test/box/misc.result
@@ -199,6 +199,79 @@ box.error.new()
 - error: 'Usage: box.error.new(code, args)'
 ...
 --
+-- gh-4489: box.error has __concat metamethod
+--
+test_run:cmd("push filter '(.builtin/.*.lua):[0-9]+' to '\\1'")
+---
+- true
+...
+e = box.error.new(box.error.UNKNOWN)
+---
+...
+'left side: ' .. e
+---
+- 'left side: Unknown error'
+...
+e .. ': right side'
+---
+- 'Unknown error: right side'
+...
+e .. nil
+---
+- error: 'builtin/error.lua: attempt to concatenate local ''rhs'' (a nil value)'
+...
+nil .. e
+---
+- error: 'builtin/error.lua: attempt to concatenate local ''lhs'' (a nil value)'
+...
+e .. box.NULL
+---
+- error: 'builtin/error.lua: attempt to concatenate ''string'' and ''void *'''
+...
+box.NULL .. e
+---
+- error: 'builtin/error.lua: attempt to concatenate ''void *'' and ''string'''
+...
+123 .. e
+---
+- 123Unknown error
+...
+e .. 123
+---
+- Unknown error123
+...
+e .. e
+---
+- Unknown errorUnknown error
+...
+e .. {}
+---
+- error: 'builtin/error.lua: attempt to concatenate local ''rhs'' (a table value)'
+...
+{} .. e
+---
+- error: 'builtin/error.lua: attempt to concatenate local ''lhs'' (a table value)'
+...
+-1ULL .. e
+---
+- error: 'builtin/error.lua: attempt to concatenate ''uint64_t'' and ''string'''
+...
+e .. -1ULL
+---
+- error: 'builtin/error.lua: attempt to concatenate ''string'' and ''uint64_t'''
+...
+1LL .. e
+---
+- error: 'builtin/error.lua: attempt to concatenate ''int64_t'' and ''string'''
+...
+e .. 1LL
+---
+- error: 'builtin/error.lua: attempt to concatenate ''string'' and ''int64_t'''
+...
+e = nil
+---
+...
+--
 -- System errors expose errno as a field.
 --
 _, err = require('fio').open('not_existing_file')
@@ -1065,7 +1138,7 @@ error()
 ---
 - error: null
 ...
---  A test case for bitwise operations 
+--  A test case for bitwise operations
 bit.lshift(1, 32)
 ---
 - 1
diff --git a/test/box/misc.test.lua b/test/box/misc.test.lua
index c3e992a48..e1c2f990f 100644
--- a/test/box/misc.test.lua
+++ b/test/box/misc.test.lua
@@ -59,6 +59,28 @@ e = box.error.new(box.error.CREATE_SPACE, "space", "error")
 e
 box.error.new()
 
+--
+-- gh-4489: box.error has __concat metamethod
+--
+test_run:cmd("push filter '(.builtin/.*.lua):[0-9]+' to '\\1'")
+e = box.error.new(box.error.UNKNOWN)
+'left side: ' .. e
+e .. ': right side'
+e .. nil
+nil .. e
+e .. box.NULL
+box.NULL .. e
+123 .. e
+e .. 123
+e .. e
+e .. {}
+{} .. e
+-1ULL .. e
+e .. -1ULL
+1LL .. e
+e .. 1LL
+e = nil
+
 --
 -- System errors expose errno as a field.
 --
@@ -281,7 +303,7 @@ dostring('return abc')
 dostring('return ...', 1, 2, 3)
 --  A test case for Bug#1043804 lua error() -> server crash
 error()
---  A test case for bitwise operations 
+--  A test case for bitwise operations
 bit.lshift(1, 32)
 bit.band(1, 3)
 bit.bor(1, 2)
-- 
2.23.0

Re: [Tarantool-patches] [PATCH v3] error: add __concat method to error object

[Sergey Ostanevich]

Hi!

Thanks for the patch!

I wonder if you're interested in coverage of all Lua types in tests?
On my side I found that decimals are not so fancy:

    tarantool> b=box.error.new(box.error.UNKNOWN)
    ---
    ...

    tarantool> a=require("decimal").new("1.5")
    ---
    ...

    tarantool> b..a
    ---
    - error: 'builtin/error.lua:165: attempt to concatenate ''string'' and
      ''struct 106'''
      ...

unlike the uuid:

    tarantool> z=require("uuid").new()
    ---
    ...

    tarantool> b..z
    ---
    - error: 'builtin/error.lua:165: attempt to concatenate ''string'' and
      ''struct tt_uuid'''
      ...

But this is not relevant to the patch, which is LGTM.

Regards,
Sergos


On 10 Jan 18:48, olegrok@tarantool.org wrote:
> From: Oleg Babin <babinoleg@mail.ru>
> 
> Usually functions return pair {nil, err} and expected that err is string.
> Let's make the behaviour of error object closer to string
> and define __concat metamethod.
> 
> Closes #4489
> 
> The case of error "error_mt.__concat(): neither of args is an error"
> is not covered by tests because of #4723
> ---
> Changes in v3:
>   - Improve concatenation logic
>   - Add more test cases
> Changes in v2:
>   - Added tests
> 
> Issue: https://github.com/tarantool/tarantool/issues/4489
> Branch: olegrok/4489-concat-for-errors
> 
>  src/lua/error.lua      | 11 +++++++
>  test/box/misc.result   | 75 +++++++++++++++++++++++++++++++++++++++++-
>  test/box/misc.test.lua | 24 +++++++++++++-
>  3 files changed, 108 insertions(+), 2 deletions(-)
> 
> diff --git a/src/lua/error.lua b/src/lua/error.lua
> index d091ee48b..7f249864a 100644
> --- a/src/lua/error.lua
> +++ b/src/lua/error.lua
> @@ -160,9 +160,20 @@ local function error_index(err, key)
>      return error_methods[key]
>  end
>  
> +local function error_concat(lhs, rhs)
> +    if ffi.istype('struct error', lhs) then
> +        return tostring(lhs) .. rhs
> +    elseif ffi.istype('struct error', rhs) then
> +        return lhs .. tostring(rhs)
> +    else
> +       error('error_mt.__concat(): neither of args is an error')
> +    end
> +end
> +
>  local error_mt = {
>      __index = error_index;
>      __tostring = error_message;
> +    __concat = error_concat;
>  };
>  
>  ffi.metatype('struct error', error_mt);
> diff --git a/test/box/misc.result b/test/box/misc.result
> index 3ca4e31ac..5ac5e0f26 100644
> --- a/test/box/misc.result
> +++ b/test/box/misc.result
> @@ -199,6 +199,79 @@ box.error.new()
>  - error: 'Usage: box.error.new(code, args)'
>  ...
>  --
> +-- gh-4489: box.error has __concat metamethod
> +--
> +test_run:cmd("push filter '(.builtin/.*.lua):[0-9]+' to '\\1'")
> +---
> +- true
> +...
> +e = box.error.new(box.error.UNKNOWN)
> +---
> +...
> +'left side: ' .. e
> +---
> +- 'left side: Unknown error'
> +...
> +e .. ': right side'
> +---
> +- 'Unknown error: right side'
> +...
> +e .. nil
> +---
> +- error: 'builtin/error.lua: attempt to concatenate local ''rhs'' (a nil value)'
> +...
> +nil .. e
> +---
> +- error: 'builtin/error.lua: attempt to concatenate local ''lhs'' (a nil value)'
> +...
> +e .. box.NULL
> +---
> +- error: 'builtin/error.lua: attempt to concatenate ''string'' and ''void *'''
> +...
> +box.NULL .. e
> +---
> +- error: 'builtin/error.lua: attempt to concatenate ''void *'' and ''string'''
> +...
> +123 .. e
> +---
> +- 123Unknown error
> +...
> +e .. 123
> +---
> +- Unknown error123
> +...
> +e .. e
> +---
> +- Unknown errorUnknown error
> +...
> +e .. {}
> +---
> +- error: 'builtin/error.lua: attempt to concatenate local ''rhs'' (a table value)'
> +...
> +{} .. e
> +---
> +- error: 'builtin/error.lua: attempt to concatenate local ''lhs'' (a table value)'
> +...
> +-1ULL .. e
> +---
> +- error: 'builtin/error.lua: attempt to concatenate ''uint64_t'' and ''string'''
> +...
> +e .. -1ULL
> +---
> +- error: 'builtin/error.lua: attempt to concatenate ''string'' and ''uint64_t'''
> +...
> +1LL .. e
> +---
> +- error: 'builtin/error.lua: attempt to concatenate ''int64_t'' and ''string'''
> +...
> +e .. 1LL
> +---
> +- error: 'builtin/error.lua: attempt to concatenate ''string'' and ''int64_t'''
> +...
> +e = nil
> +---
> +...
> +--
>  -- System errors expose errno as a field.
>  --
>  _, err = require('fio').open('not_existing_file')
> @@ -1065,7 +1138,7 @@ error()
>  ---
>  - error: null
>  ...
> ---  A test case for bitwise operations 
> +--  A test case for bitwise operations
>  bit.lshift(1, 32)
>  ---
>  - 1
> diff --git a/test/box/misc.test.lua b/test/box/misc.test.lua
> index c3e992a48..e1c2f990f 100644
> --- a/test/box/misc.test.lua
> +++ b/test/box/misc.test.lua
> @@ -59,6 +59,28 @@ e = box.error.new(box.error.CREATE_SPACE, "space", "error")
>  e
>  box.error.new()
>  
> +--
> +-- gh-4489: box.error has __concat metamethod
> +--
> +test_run:cmd("push filter '(.builtin/.*.lua):[0-9]+' to '\\1'")
> +e = box.error.new(box.error.UNKNOWN)
> +'left side: ' .. e
> +e .. ': right side'
> +e .. nil
> +nil .. e
> +e .. box.NULL
> +box.NULL .. e
> +123 .. e
> +e .. 123
> +e .. e
> +e .. {}
> +{} .. e
> +-1ULL .. e
> +e .. -1ULL
> +1LL .. e
> +e .. 1LL
> +e = nil
> +
>  --
>  -- System errors expose errno as a field.
>  --
> @@ -281,7 +303,7 @@ dostring('return abc')
>  dostring('return ...', 1, 2, 3)
>  --  A test case for Bug#1043804 lua error() -> server crash
>  error()
> ---  A test case for bitwise operations 
> +--  A test case for bitwise operations
>  bit.lshift(1, 32)
>  bit.band(1, 3)
>  bit.bor(1, 2)
> -- 
> 2.23.0
> 

Re: [Tarantool-patches] [PATCH v3] error: add __concat method to error object

[Oleg Babin]

Thanks for your review!

Yes, you are right but I think that gereral cases have been covered for 
"box.error" type.


I've created an issue - 
https://github.com/tarantool/tarantool/issues/4725 to generalize such 
approach and may be add "__concat" for decimals/uuid/...


Also "decimals" is not available in 1.10 if we want to backport this 
patch one day


On 14/01/2020 17:21, Sergey Ostanevich wrote:
> Hi!
> 
> Thanks for the patch!
> 
> I wonder if you're interested in coverage of all Lua types in tests?
> On my side I found that decimals are not so fancy:

---
Oleg Babin

Re: [Tarantool-patches] [PATCH v3] error: add __concat method to error object

[Alexander Turenko]

LGTM.

Just two minor comments re commit message.

WBR, Alexander Turenko.

On Fri, Jan 10, 2020 at 06:48:05PM +0300, olegrok@tarantool.org wrote:
> From: Oleg Babin <babinoleg@mail.ru>
> 
> Usually functions return pair {nil, err} and expected that err is string.

Curly braces can be interpreted as a table. I would use an apostrophe
instead.

> Let's make the behaviour of error object closer to string
> and define __concat metamethod.
> 
> Closes #4489
> 
> The case of error "error_mt.__concat(): neither of args is an error"
> is not covered by tests because of #4723

It is better to place all tags below commit message, not in the middle.
The only exception is docbot comment: it should be written after tags.

> ---
> Changes in v3:
>   - Improve concatenation logic
>   - Add more test cases
> Changes in v2:
>   - Added tests
> 
> Issue: https://github.com/tarantool/tarantool/issues/4489
> Branch: olegrok/4489-concat-for-errors
> 
>  src/lua/error.lua      | 11 +++++++
>  test/box/misc.result   | 75 +++++++++++++++++++++++++++++++++++++++++-
>  test/box/misc.test.lua | 24 +++++++++++++-
>  3 files changed, 108 insertions(+), 2 deletions(-)

Re: [Tarantool-patches] [PATCH v3] error: add __concat method to error object

[Oleg Babin]

Thanks!

I've changed commit message and force pushed it to the top of my branch:
https://github.com/tarantool/tarantool/tree/olegrok/4489-concat-for-errors

New commit message:

commit a9ad4bdfa51174223e0d10ef0b6a751fd15a6d8e (HEAD -> 
olegrok/4489-concat-for-errors, origin/olegrok/4489-concat-for-errors)
Author: Oleg Babin <babinoleg@mail.ru>
Date:   Fri Jan 10 18:35:03 2020 +0300

     error: add __concat method to error object

     Usually functions return pair `nil, err` and expected that err is 
string.
     Let's make the behaviour of error object closer to string
     and define __concat metamethod.

     The case of error "error_mt.__concat(): neither of args is an error"
     is not covered by tests because of #4723

     Closes #4489


On 15/01/2020 14:26, Alexander Turenko wrote:
> LGTM.
> 
> Just two minor comments re commit message.
> 

---
Oleg Babin

Re: [Tarantool-patches] [PATCH v3] error: add __concat method to error object

[Alexander Turenko]

Sergey O. is okay too, so I'm going to push the patch.

Pushed to master, 2.3, 2.2 and 1.10.

WBR, Alexander Turenko.

On Wed, Jan 15, 2020 at 03:35:42PM +0300, Oleg Babin wrote:
> Thanks!
> 
> I've changed commit message and force pushed it to the top of my branch:
> https://github.com/tarantool/tarantool/tree/olegrok/4489-concat-for-errors
> 
> New commit message:
> 
> commit a9ad4bdfa51174223e0d10ef0b6a751fd15a6d8e (HEAD ->
> olegrok/4489-concat-for-errors, origin/olegrok/4489-concat-for-errors)
> Author: Oleg Babin <babinoleg@mail.ru>
> Date:   Fri Jan 10 18:35:03 2020 +0300
> 
>     error: add __concat method to error object
> 
>     Usually functions return pair `nil, err` and expected that err is
> string.
>     Let's make the behaviour of error object closer to string
>     and define __concat metamethod.
> 
>     The case of error "error_mt.__concat(): neither of args is an error"
>     is not covered by tests because of #4723
> 
>     Closes #4489
> 
> 
> On 15/01/2020 14:26, Alexander Turenko wrote:
> > LGTM.
> > 
> > Just two minor comments re commit message.
> > 
> 
> ---
> Oleg Babin