Re: [Tarantool-patches] [PATCH v4 4/10] Fix luacheck warnings in src/lua/

[Igor Munkin <imun@tarantool.org>]

Sergey,

Thanks for the patch! I left several comments below, please consider
them.

On 21.04.20, sergeyb@tarantool.org wrote:
> From: Sergey Bronnikov <sergeyb@tarantool.org>
> 
> Closes #4681
> 
> Reviewed-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
> Reviewed-by: Igor Munkin <imun@tarantool.org>
> 
> Co-authored-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
> Co-authored-by: Igor Munkin <imun@tarantool.org>
> ---
>  .luacheckrc            |  3 ++
>  src/lua/argparse.lua   |  6 ++--
>  src/lua/buffer.lua     |  4 +--
>  src/lua/clock.lua      |  2 +-
>  src/lua/crypto.lua     |  4 +--
>  src/lua/csv.lua        |  5 ++-
>  src/lua/digest.lua     |  2 +-
>  src/lua/env.lua        |  2 +-
>  src/lua/fiber.lua      |  4 +--
>  src/lua/fio.lua        | 25 +++++++-------
>  src/lua/help.lua       |  7 ++--
>  src/lua/httpc.lua      |  3 --
>  src/lua/init.lua       |  7 ++--
>  src/lua/msgpackffi.lua | 22 +++++--------
>  src/lua/socket.lua     | 65 ++++++++++++++++++-------------------
>  src/lua/string.lua     |  1 -
>  src/lua/swim.lua       | 10 +++---
>  src/lua/tap.lua        | 74 ++++++++++++++++++++----------------------
>  src/lua/trigger.lua    |  3 --
>  19 files changed, 117 insertions(+), 132 deletions(-)
> 
> diff --git a/.luacheckrc b/.luacheckrc
> index 0b7dc2dfe..60aedc842 100644
> --- a/.luacheckrc
> +++ b/.luacheckrc
> @@ -26,3 +26,6 @@ exclude_files = {
>  }
>  
>  files["extra/dist/tarantoolctl.in"] = {ignore = {"212/self", "122", "431"}}
> +files["src/lua/swim.lua"] = {ignore = {"431"}}
> +files["src/lua/fio.lua"] = {ignore = {"231"}}
> +files["src/lua/init.lua"] = {globals = {"dostring"}}

There are several "(W212) unused argument self" warnings for crypto.lua
(and other chunks like digest.lua, errno.lua, etc), but no corresponding
suppression in .luacheckrc like the one you added for tarantoolctl.in in
the previous patch.

Furthermore I see no reason to leave suppressions list unsorted.

> diff --git a/src/lua/argparse.lua b/src/lua/argparse.lua
> index faa0ae130..f58985425 100644
> --- a/src/lua/argparse.lua
> +++ b/src/lua/argparse.lua
> @@ -90,8 +90,8 @@ local function convert_parameter(name, convert_from, convert_to)
>      return convert_from
>  end
>  
> -local function parameters_parse(t_in, options)
> -    local t_out, t_in = {}, t_in or {}
> +local function parameters_parse(t__in, options)
> +    local t_out, t_in = {}, t__in or {}

Moved the related thread from v3 series review[1]:
| > > diff --git a/src/lua/argparse.lua b/src/lua/argparse.lua
| > > index faa0ae130..f58985425 100644
| > > --- a/src/lua/argparse.lua
| > > +++ b/src/lua/argparse.lua
| > > @@ -90,8 +90,8 @@ local function convert_parameter(name, convert_from, convert_to)
| > >      return convert_from
| > >  end
| > >
| > > -local function parameters_parse(t_in, options)
| > > -    local t_out, t_in = {}, t_in or {}
| > > +local function parameters_parse(t__in, options)
| > > +    local t_out, t_in = {}, t__in or {}
| >
| > I guess you can just give t__in a proper name, e.g. args, params, etc.
|
| Agree with you, replaced it with 'param'.
|

I see no corresponding changes in v4.

>  
>      -- Prepare a lookup table for options. An option name -> a
>      -- type name to convert a parameter into or true (which means

<snipped>

> diff --git a/src/lua/fio.lua b/src/lua/fio.lua
> index 83fddaa0a..b2b4b4c77 100644
> --- a/src/lua/fio.lua
> +++ b/src/lua/fio.lua

<snipped>

> @@ -407,20 +407,20 @@ fio.rmtree = function(path)
>      if type(path) ~= 'string' then
>          error("Usage: fio.rmtree(path)")
>      end
> -    local status, err
> +    local status

This was also mentioned in the v3 series review[2]:
| This value is unused (and the corresponding warning is reported).
| Consider the changes below:
|
| ================================================================================
|
| diff --git a/src/lua/fio.lua b/src/lua/fio.lua
| index b2b4b4c77..e271eccf6 100644
| --- a/src/lua/fio.lua
| +++ b/src/lua/fio.lua
| @@ -407,7 +407,6 @@ fio.rmtree = function(path)
|      if type(path) ~= 'string' then
|          error("Usage: fio.rmtree(path)")
|      end
| -    local status
|      path = fio.abspath(path)
|      local ls, err = fio.listdir(path)
|      if err ~= nil then
| @@ -427,7 +426,8 @@ fio.rmtree = function(path)
|              end
|          end
|      end
| -    status, err = fio.rmdir(path)
| +    local _
| +    _, err = fio.rmdir(path)
|      if err ~= nil then
|          return false, string.format("failed to remove %s: %s", path, err)
|      end
|
| ================================================================================

I see no reason to suppress it in the .luacheckrc instead of fixing a
single occurense.

>      path = fio.abspath(path)
>      local ls, err = fio.listdir(path)
>      if err ~= nil then
>          return nil, err
>      end
> -    for i, f in ipairs(ls) do
> +    for _, f in ipairs(ls) do
>          local tmppath = fio.pathjoin(path, f)
>          local st = fio.lstat(tmppath)
>          if st then
>              if st:is_dir() then
> -                st, err = fio.rmtree(tmppath)
> +                _, err = fio.rmtree(tmppath)
>              else
> -                st, err = fio.unlink(tmppath)
> +                _, err = fio.unlink(tmppath)
>              end
>              if err ~= nil  then
>                  return nil, err

<snipped>

> diff --git a/src/lua/help.lua b/src/lua/help.lua
> index 54ff1b5d0..1b822828b 100644
> --- a/src/lua/help.lua
> +++ b/src/lua/help.lua

<snipped>

> @@ -22,7 +19,7 @@ setmetatable(help, { __call = help_call })
>  
>  local screen_id = 1;
>  
> -local function tutorial_call(table, action)
> +local function tutorial_call(action)

This change breaks the behaviour. Consider the following:
| $ ./src/tarantool
| Tarantool 2.4.0-192-g89f3b427d
| type 'help' for interactive help
| tarantool> tutorial('start')
| ---
| - error: 'builtin/help.lua:32: Usage: tutorial("start" | "next" | "prev" | 1 .. 20)'
| ...
|
| tarantool> getmetatable(tutorial).__call('start')
| ---
| - |
|   Tutorial -- Screen #1 -- Hello, Moon
|   ====================================
|
|   Welcome to the Tarantool tutorial.
|   It will introduce you to Tarantool’s Lua application server
|   and database server, which is what’s running what you’re seeing.
|   This is INTERACTIVE -- you’re expected to enter requests
|   based on the suggestions or examples in the screen’s text.
|
|   The first request is:
|
|   5.1, "Olá", "Lua"
|   ------------------
|
|   This isn’t your grandfather’s "Hello World" ...
|   the decimal literal 5.1 and the two strings inside
|   single quotes ("Hello Moon" in Portuguese) will just
|   be repeated, without need for a print() statement.
|
|   Take that one-line request and enter it below after the
|   "tarantool>" prompt, then type Enter.
|   You’ll see the response:
|   ---
|   - 5.1
|   - Olá
|   - Lua
|   ...
|   Then you’ll get a chance to repeat -- perhaps entering
|   something else such as "Longer String",-1,-3,0.
|   When you’re ready to go to the next screen, enter <tutorial("next")>.
| ...
|
| tarantool>

Long story short: table arg is similar to self here (see more here[3])
and cannot be simply dropped. Nevertheless it looks more convenient to
replace it with self.

I guess it would be nice to add a test for it (or at least file a
follow-up issue requesting the test).

>      if action == 'start' then
>          screen_id = 1;
>      elseif action == 'next' or action == 'more' then

<snipped>

> -- 
> 2.23.0
> 

[1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-April/016113.html
[2]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-April/016022.html
[3]: https://www.lua.org/manual/5.1/manual.html#2.8

-- 
Best regards,
IM