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

[Alexander Turenko <alexander.turenko@tarantool.org>]

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.