[Tarantool-patches] [PATCH v2] error: Add __concat method to error object

[Tarantool-patches] [PATCH v2] error: Add __concat method to error object

[Oleg Babin]

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

Changes in v2:
  - Added tests

---
 src/lua/error.lua      |  8 +++++++
 test/box/misc.result   | 47 +++++++++++++++++++++++++++++++++++++++++-
 test/box/misc.test.lua | 17 ++++++++++++++-
 3 files changed, 70 insertions(+), 2 deletions(-)

diff --git a/src/lua/error.lua b/src/lua/error.lua
index 28fc0377d..b44fab7b6 100644
--- a/src/lua/error.lua
+++ b/src/lua/error.lua
@@ -150,9 +150,17 @@ local function error_index(err, key)
     return error_methods[key]
 end
 
+local function error_concat(lhs, rhs)
+    if lhs == nil or rhs == nil then
+        error("attempt to concatenate struct error and nil")
+    end
+    return tostring(lhs) .. tostring(rhs)
+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 b2930515b..db184c037 100644
--- a/test/box/misc.result
+++ b/test/box/misc.result
@@ -196,6 +196,51 @@ 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 struct error and nil'
+...
+nil .. e
+---
+- error: 'builtin/error.lua: attempt to concatenate struct error and nil'
+...
+e .. box.NULL
+---
+- error: 'builtin/error.lua: attempt to concatenate struct error and nil'
+...
+box.NULL .. e
+---
+- error: 'builtin/error.lua: attempt to concatenate struct error and nil'
+...
+123 .. e
+---
+- 123Unknown error
+...
+e .. 123
+---
+- Unknown error123
+...
+e = nil
+---
+...
 ----------------
 -- # box.stat
 ----------------
@@ -1037,7 +1082,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 cc223b2ef..6104b935b 100644
--- a/test/box/misc.test.lua
+++ b/test/box/misc.test.lua
@@ -59,6 +59,21 @@ 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 = nil
+
 ----------------
 -- # box.stat
 ----------------
@@ -271,7 +286,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 v2] error: Add __concat method to error object

[Alexander Turenko]

On Mon, Nov 11, 2019 at 05:50:09PM +0300, Oleg Babin 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
> ---
> 
> Changes in v2:
>   - Added tests
> 
> ---
>  src/lua/error.lua      |  8 +++++++
>  test/box/misc.result   | 47 +++++++++++++++++++++++++++++++++++++++++-
>  test/box/misc.test.lua | 17 ++++++++++++++-
>  3 files changed, 70 insertions(+), 2 deletions(-)
> 
> diff --git a/src/lua/error.lua b/src/lua/error.lua
> index 28fc0377d..b44fab7b6 100644
> --- a/src/lua/error.lua
> +++ b/src/lua/error.lua
> @@ -150,9 +150,17 @@ local function error_index(err, key)
>      return error_methods[key]
>  end
>  
> +local function error_concat(lhs, rhs)
> +    if lhs == nil or rhs == nil then
> +        error("attempt to concatenate struct error and nil")

Nit: If we want to follow string concatenation errors, then it should be
'struct error and nil' or 'nil and struct error' depending of what
values lhs and rhs have. See below possible resolution.

> +    end
> +    return tostring(lhs) .. tostring(rhs)
> +end
> +
>  local error_mt = {
>      __index = error_index;
>      __tostring = error_message;
> +    __concat = error_concat;
>  };
>  
>  ffi.metatype('struct error', error_mt);

It is the same as
https://github.com/tarantool/tarantool/issues/4489#issuecomment-530448023

I'll cite my answer from this issue:

 | I would say that it should not allow more then a string: say,
 | `'' .. box.NULL` gives an error, so
 | `box.error.new(box.error.CFG, 'foo', 'bar') .. box.NULL` should too.
 | At the first glance it looks that we should allow only an error as
 | the first argument and a string, a number or an error as the second
 | one.
 | 
 | I think it is harmless, so I'm okay in general.

Yep, the example with box.NULL works correct with your implementation.
But not with ffi.new('char'). Not even with {}.

Also I was a bit out of context and don't know that a metamethod of a
right value can be invoked if there are no metamethod in a left one. So
'we should allow only an error as the first argument' is not correct:
the first and the seconds arguments should be symmetric.

To sum up I propose to follow logic of concatenation operation of a
string: it allows only certain data types to be concatenated with it:
strings and numbers.

We maybe can reuse string's checks:

 | if lhs is an error:
 |     return tostring(lhs) .. rhs
 | else if rhs is an error:
 |     return lhs .. tostring(rhs)
 | else:
 |     return error('error_mt.__concat(): neither of args is an error')

Here we handle everything as I see: nils, tables (with and without its
own __concat), numbers, cdata and userdata (again, with and without
__concat).

Anyway, let's test an implementation more carefully.

The only flaw I see that if a concatenation error occurs, then it will
state an error of concatenate a string with something (or something with
a string) rather then struct error.

WBR, Alexander Turenko.

Re: [Tarantool-patches] [PATCH v2] error: Add __concat method to error object

[Sergey Ostanevich]

Hi!

Thanks for the patch!
LGTM.

Sergos


On 11 Nov 17:50, Oleg Babin 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
> ---
> 
> Changes in v2:
>   - Added tests
> 
> ---
>  src/lua/error.lua      |  8 +++++++
>  test/box/misc.result   | 47 +++++++++++++++++++++++++++++++++++++++++-
>  test/box/misc.test.lua | 17 ++++++++++++++-
>  3 files changed, 70 insertions(+), 2 deletions(-)
> 
> diff --git a/src/lua/error.lua b/src/lua/error.lua
> index 28fc0377d..b44fab7b6 100644
> --- a/src/lua/error.lua
> +++ b/src/lua/error.lua
> @@ -150,9 +150,17 @@ local function error_index(err, key)
>      return error_methods[key]
>  end
>  
> +local function error_concat(lhs, rhs)
> +    if lhs == nil or rhs == nil then
> +        error("attempt to concatenate struct error and nil")
> +    end
> +    return tostring(lhs) .. tostring(rhs)
> +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 b2930515b..db184c037 100644
> --- a/test/box/misc.result
> +++ b/test/box/misc.result
> @@ -196,6 +196,51 @@ 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 struct error and nil'
> +...
> +nil .. e
> +---
> +- error: 'builtin/error.lua: attempt to concatenate struct error and nil'
> +...
> +e .. box.NULL
> +---
> +- error: 'builtin/error.lua: attempt to concatenate struct error and nil'
> +...
> +box.NULL .. e
> +---
> +- error: 'builtin/error.lua: attempt to concatenate struct error and nil'
> +...
> +123 .. e
> +---
> +- 123Unknown error
> +...
> +e .. 123
> +---
> +- Unknown error123
> +...
> +e = nil
> +---
> +...
>  ----------------
>  -- # box.stat
>  ----------------
> @@ -1037,7 +1082,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 cc223b2ef..6104b935b 100644
> --- a/test/box/misc.test.lua
> +++ b/test/box/misc.test.lua
> @@ -59,6 +59,21 @@ 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 = nil
> +
>  ----------------
>  -- # box.stat
>  ----------------
> @@ -271,7 +286,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 v2] error: Add __concat method to error object

[Oleg Babin]

Thanks for your review!

As we discussed verbally I improve test and consider cases of tables and 
ffi objects

We will not overcomplicate logic with proper error message construction: 
user will see "attempt to concatenate ''type' and ''string''' "


On 10/01/2020 15:32, Alexander Turenko wrote:
> On Mon, Nov 11, 2019 at 05:50:09PM +0300, Oleg Babin 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
>> ---
>>
>> Changes in v2:
>>    - Added tests
>>
>> ---
>>   src/lua/error.lua      |  8 +++++++
>>   test/box/misc.result   | 47 +++++++++++++++++++++++++++++++++++++++++-
>>   test/box/misc.test.lua | 17 ++++++++++++++-
>>   3 files changed, 70 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/lua/error.lua b/src/lua/error.lua
>> index 28fc0377d..b44fab7b6 100644
>> --- a/src/lua/error.lua
>> +++ b/src/lua/error.lua
>> @@ -150,9 +150,17 @@ local function error_index(err, key)
>>       return error_methods[key]
>>   end
>>   
>> +local function error_concat(lhs, rhs)
>> +    if lhs == nil or rhs == nil then
>> +        error("attempt to concatenate struct error and nil")
> Nit: If we want to follow string concatenation errors, then it should be
> 'struct error and nil' or 'nil and struct error' depending of what
> values lhs and rhs have. See below possible resolution.
>
>> +    end
>> +    return tostring(lhs) .. tostring(rhs)
>> +end
>> +
>>   local error_mt = {
>>       __index = error_index;
>>       __tostring = error_message;
>> +    __concat = error_concat;
>>   };
>>   
>>   ffi.metatype('struct error', error_mt);
> It is the same as
> https://github.com/tarantool/tarantool/issues/4489#issuecomment-530448023
>
> I'll cite my answer from this issue:
>
>   | I would say that it should not allow more then a string: say,
>   | `'' .. box.NULL` gives an error, so
>   | `box.error.new(box.error.CFG, 'foo', 'bar') .. box.NULL` should too.
>   | At the first glance it looks that we should allow only an error as
>   | the first argument and a string, a number or an error as the second
>   | one.
>   |
>   | I think it is harmless, so I'm okay in general.
>
> Yep, the example with box.NULL works correct with your implementation.
> But not with ffi.new('char'). Not even with {}.
>
> Also I was a bit out of context and don't know that a metamethod of a
> right value can be invoked if there are no metamethod in a left one. So
> 'we should allow only an error as the first argument' is not correct:
> the first and the seconds arguments should be symmetric.
>
> To sum up I propose to follow logic of concatenation operation of a
> string: it allows only certain data types to be concatenated with it:
> strings and numbers.
>
> We maybe can reuse string's checks:
>
>   | if lhs is an error:
>   |     return tostring(lhs) .. rhs
>   | else if rhs is an error:
>   |     return lhs .. tostring(rhs)
>   | else:
>   |     return error('error_mt.__concat(): neither of args is an error')
>
> Here we handle everything as I see: nils, tables (with and without its
> own __concat), numbers, cdata and userdata (again, with and without
> __concat).
>
> Anyway, let's test an implementation more carefully.
>
> The only flaw I see that if a concatenation error occurs, then it will
> state an error of concatenate a string with something (or something with
> a string) rather then struct error.
>
> WBR, Alexander Turenko.