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

[Sergey Ostanevich <sergos@tarantool.org>]

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
>