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

[Oleg Babin <olegrok@tarantool.org>]

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.