[Tarantool-patches] [PATCH 5/6] Fix luacheck warnings in src/lua/

[Vladislav Shpilevoy]

Thanks for the patch!

See 11 comments below.

On 04/06/2020 10:39, sergeyb at tarantool.org wrote:
> From: Sergey Bronnikov <sergeyb at tarantool.org>
> 
> Part of #4681
> 
> Reviewed-by: Vladislav Shpilevoy <v.shpilevoy at tarantool.org>
> Reviewed-by: Igor Munkin <imun at tarantool.org>
> 
> Co-authored-by: Vladislav Shpilevoy <v.shpilevoy at tarantool.org>
> Co-authored-by: Igor Munkin <imun at tarantool.org>
> ---
>  .luacheckrc            | 28 ++++++++++++++++++++++++----
>  src/lua/argparse.lua   |  3 ++-
>  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        | 30 ++++++++++++++----------------
>  src/lua/help.lua       |  7 ++-----
>  src/lua/httpc.lua      |  3 ---
>  src/lua/init.lua       |  4 ++--
>  src/lua/msgpackffi.lua | 22 +++++++++-------------
>  src/lua/socket.lua     | 16 ++++++++--------
>  src/lua/string.lua     |  1 -
>  src/lua/swim.lua       |  2 +-
>  src/lua/tap.lua        |  4 ----
>  src/lua/trigger.lua    |  3 ---
>  19 files changed, 73 insertions(+), 73 deletions(-)
> 
> diff --git a/.luacheckrc b/.luacheckrc
> index b917eb927..fb8b9dfb3 100644
> --- a/.luacheckrc
> +++ b/.luacheckrc
> @@ -1,9 +1,14 @@
>  std = "luajit"
>  globals = {"box", "_TARANTOOL"}
>  ignore = {
> -    "212/self", -- Unused argument <self>.
> -    "411",      -- Redefining a local variable.
> -    "431",      -- Shadowing an upvalue.
> +    "143/debug",  -- Accessing an undefined field of a global variable <debug>.

1. Why are global variables ignored sometimes using '143/<name>', sometimes
via adding to the global 'globals', sometimes via the per-file 'globals'?
'debug', 'string', and 'table' are globals, available in all the sources,
just like 'box'.

> +    "143/string", -- Accessing an undefined field of a global variable <string>.
> +    "143/table",  -- Accessing an undefined field of a global variable <table>.
> +    "212/self",   -- Unused argument <self>.

2. This is one of the main reasons, why non-standardized alignment applied
to everything is evil. Diff split into patches does not make much sense, when a
next patch anyway should rewrite 100% of the previous one just to make alignment
correct. I would propose to make the alignment correct right away, from the first
patch. Or remove it. Or make it less strict, so not all lines are required to be
equaly aligned. Whatever helps not to change already fine lines.

> +    "411",        -- Redefining a local variable.
> +    "421",        -- Shadowing a local variable.
> +    "431",        -- Shadowing an upvalue.
> +    "432",        -- Shadowing an upvalue argument.
>  }
> @@ -27,3 +32,18 @@ files["extra/dist/tarantoolctl.in"] = {
>          "122", -- https://github.com/tarantool/tarantool/issues/4929
>      },
>  }
> +files["src/lua/help.lua"] = {
> +    globals = {"help", "tutorial"}, -- globals defined for interactive mode.

3. Lets use single style for all comments: start sentences from
capital letters. Instead of switching between lowcase and normal
writing from time to time.

> +}> +files["src/lua/init.lua"] = {
> +    globals = {"dostring"}, -- miscellaneous global function definition.
> +    ignore = {
> +        "122/os",      -- set tarantool specific behaviour for os.exit.
> +        "142/package", -- add custom functions into Lua package namespace.

4. os and package changes can be done using rawset() function. It does not
produce a warning, and behaves in the above cases the same. However this
should be consulted with other reviewers.

Another option - add 'os' and 'package' to the list of globals. It also fixes
the warnings.

> +    },
> +}
> +files["src/lua/swim.lua"] = {
> +    ignore = {
> +        "212/m", -- respect swim module code style.

5. Once again I wonder why do we ignore unused arguments sometimes globaly in
'ignore = {...}', sometimes per-file like here, sometimes using inlined luacheck
comments.

In this particular case there is even a fourth option - rename 'm' to 'self' in
the single problematic line. Like you did for many similar places.

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

6. Unnecessary diff. At least when I check on top of the branch.

> diff --git a/src/lua/init.lua b/src/lua/init.lua
> index ff3e74c3c..aea7a7491 100644
> --- a/src/lua/init.lua
> +++ b/src/lua/init.lua
> @@ -144,9 +144,9 @@ local function gen_search_func(path_fn, templates, need_traverse)
>  
>          local searchpaths = {}
>  
> -        for _, path in ipairs(paths) do
> +        for _, p in ipairs(paths) do
>              for _, template in pairs(templates) do
> -                table.insert(searchpaths, fio.pathjoin(path, template))
> +                table.insert(searchpaths, fio.pathjoin(p, template))
>              end

7. Ditto.

>          end
>  
> @@ -603,7 +599,7 @@ local function check_offset(offset, len)
>      if offset == nil then
>          return 1
>      end
> -    local offset = ffi.cast('ptrdiff_t', offset)
> +    offset = ffi.cast('ptrdiff_t', offset)

8. Ditto.

>      if offset < 1 or offset > len then
>          error(string.format("offset = %d is out of bounds [1..%d]",
>              tonumber(offset), len))
> diff --git a/src/lua/socket.lua b/src/lua/socket.lua
> index bbdef01e3..2a2c7a32c 100644
> --- a/src/lua/socket.lua
> +++ b/src/lua/socket.lua
> @@ -1044,7 +1044,7 @@ local function tcp_connect(host, port, timeout)
>          boxerrno(0)
>          return s
>      end
> -    local timeout = timeout or TIMEOUT_INFINITY
> +    timeout = timeout or TIMEOUT_INFINITY

9. Ditto.

>      local stop = fiber.clock() + timeout
>      local dns = getaddrinfo(host, port, timeout, { type = 'SOCK_STREAM',
>          protocol = 'tcp' })
> @@ -1319,7 +1319,7 @@ local function lsocket_tcp_getpeername(self)
>      return peer.host, tostring(peer.port), peer.family:match("AF_(.*)"):lower()
>  end
>  
> -local function lsocket_tcp_settimeout(self, value, mode)
> +local function lsocket_tcp_settimeout(self, value, mode) -- luacheck: no unused args

10. Mode is really ignored, and this is not some kind of built-in
function with 'fixed' signature. And it is not used 'virtually',
when there is a variable keeping pointer at one function among
many having the same signature. So what is a purpose of keeping it?

>      check_socket(self)
>      self.timeout = value
>      -- mode is effectively ignored
> @@ -1475,7 +1475,7 @@ local function lsocket_tcp_receive(self, pattern, prefix)
>          local result = { prefix }
>          local deadline = fiber.clock() + (self.timeout or TIMEOUT_INFINITY)
>          repeat
> -            local data = read(self, LIMIT_INFINITY, timeout, check_infinity)
> +            data = read(self, LIMIT_INFINITY, timeout, check_infinity)

11. Unnecessary diff.

>              if data == nil then
>                  if not errno_is_transient[self._errno] then
>                      return nil, socket_error(self)