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

[Sergey Bronnikov <sergeyb@tarantool.org>]

Hi, Vlad

thanks for review!

On 20:03 Sat 06 Jun , Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> See 11 comments below.
> 
> On 04/06/2020 10:39, sergeyb@tarantool.org wrote:
> > From: Sergey Bronnikov <sergeyb@tarantool.org>
> > 
> > Part of #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            | 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'.

the main idea was to supress rarely used variables inline and often used
variables globally in .luacheckrc. Suggested by Igor in at least [1]
and I'm agree with his approach.

1. https://lists.tarantool.org/pipermail/tarantool-patches/2020-May/017108.html

> 
> > +    "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.

Moved comments before code.

> > +    "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.

Fixed.
> 
> > +}> +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.

Fixed using rawset().

> 
> > +    },
> > +}
> > +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.

Answered in comment 1.

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

replaced 'm' to 'self'.

> > +    },
> > +}
> > 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.

change is required, see [1]

1. https://gitlab.com/tarantool/tarantool/-/jobs/589857153

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

change reverted
> 
> >          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.

change is required, see [1]

1. https://gitlab.com/tarantool/tarantool/-/jobs/589857153
> 
> >      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.

change is required, see [1]

src/lua/socket.lua:344:11: variable timeout was previously defined as an argument on line 340

1. https://gitlab.com/tarantool/tarantool/-/jobs/589857153

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

removed mode arg

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

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

-- 
sergeyb@