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

[Sergey Bronnikov <sergeyb@tarantool.org>]

Igor,

thanks for review!
I have corrected patches according to your comments.

On 01:54 Fri 24 Apr , Igor Munkin wrote:
> Sergey,
> 
> Thanks for the patch! Please consider the comments I left below.
> 
> On 21.04.20, sergeyb@tarantool.org wrote:
> > From: Sergey Bronnikov <sergeyb@tarantool.org>
> > 
> > Closes #4681
> > 
> > Reviewed-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
> > Co-authored-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
> > ---
> >  .luacheckrc                     |  8 ++++++++
> >  src/box/lua/console.lua         |  6 +++---
> >  src/box/lua/feedback_daemon.lua |  2 +-
> >  src/box/lua/key_def.lua         |  2 +-
> >  src/box/lua/load_cfg.lua        | 25 +++++++++++------------
> >  src/box/lua/net_box.lua         | 26 ++++++++----------------
> >  src/box/lua/schema.lua          | 35 ++++++++++++++++-----------------
> >  src/box/lua/tuple.lua           |  8 +++-----
> >  src/box/lua/upgrade.lua         | 19 +++++++++---------
> >  9 files changed, 62 insertions(+), 69 deletions(-)
> > 
> > diff --git a/.luacheckrc b/.luacheckrc
> > index 60aedc842..64692b27c 100644
> > --- a/.luacheckrc
> > +++ b/.luacheckrc
> > @@ -25,7 +25,15 @@ exclude_files = {
> >      ".git/**/*.lua",
> >  }
> >  
> > +files["**/*.lua"] = {
> > +    globals = {"box", "_TARANTOOL", "help", "tutorial"},
> > +    ignore = {"212/self", "122"}
> > +}
> 
> Well, I can't figure out why these global suppressions are set in this
> patch.
> By the way these suppressions don't work for extra/dist/tarantoolctl.in.

I'll move these globals to a patch specific to src/lua/.

> Furthermore, IMHO, global suppression for (W122)[Setting a read-only
> field of a global variable] is a bad practice.

requested by Vladislav in [1] (comment 5):
"This error is actually ridiculous in such a language as Lua.
Can we ignore it globally? The same for error 122."

[1] https://lists.tarantool.org/pipermail/tarantool-patches/2020-April/015404.html

> >  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"}}
> > +files["src/box/lua/console.lua"] = {ignore = {"212"}}
> 
> What is the reason to fix 3 (W211)[Unused local variable] warnings, but
> leave and suppress 9 (W212)[Unused argument] warnings (at the same time
> you fix this type of warnings in load_cfg.lua)?
> 
> > +files["src/box/lua/load_cfg.lua"] = {ignore = {"542"}}
> 
> It's a single empty branch warning, why it can't be fixed / suppressed
> inline? Here is the diff:

I thought we don't want to use inline supressions at all.
Well, applied patch below on a branch.

> ================================================================================
> 
> diff --git a/.luacheckrc b/.luacheckrc
> index 64692b27c..0c1f85766 100644
> --- a/.luacheckrc
> +++ b/.luacheckrc
> @@ -29,11 +29,11 @@ files["**/*.lua"] = {
>      globals = {"box", "_TARANTOOL", "help", "tutorial"},
>      ignore = {"212/self", "122"}
>  }
> +
>  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"}}
>  files["src/box/lua/console.lua"] = {ignore = {"212"}}
> -files["src/box/lua/load_cfg.lua"] = {ignore = {"542"}}
>  files["src/box/lua/net_box.lua"] = {ignore = {"431", "432", "231", "411", "212"}}
>  files["src/box/lua/schema.lua"] = {ignore = {"431", "432", "542", "212"}}
> diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
> index 0926c7380..38c128e85 100644
> --- a/src/box/lua/load_cfg.lua
> +++ b/src/box/lua/load_cfg.lua
> @@ -414,7 +414,7 @@ local function prepare_cfg(cfg, default_cfg, template_cfg, modify_cfg, prefix)
>          elseif v == "" or v == nil then
>              -- "" and NULL = ffi.cast('void *', 0) set option to default value
>              v = default_cfg[k]
> -        elseif template_cfg[k] == 'any' then
> +        elseif template_cfg[k] == 'any' then -- luacheck: ignore
>              -- any type is ok
>          elseif type(template_cfg[k]) == 'table' then
>              if type(v) ~= 'table' then
> 
> ================================================================================
> 
> > +files["src/box/lua/net_box.lua"] = {ignore = {"431", "432", "231", "411", "212"}}
> 
> Again, I see suppressed (W212)[Unused argument] warnings *but*:
> * they are well localized
> * most of them will *never* be fixed and should be commented and
>   protected from inquisitive persons
> * others can be fixed with a trivial changes
> 
> Here is the diff:

Applied, thanks!

> ================================================================================
> 
> diff --git a/.luacheckrc b/.luacheckrc
> index 64692b27c..b05260d29 100644
> --- a/.luacheckrc
> +++ b/.luacheckrc
> @@ -35,5 +35,5 @@ files["src/lua/fio.lua"] = {ignore = {"231"}}
>  files["src/lua/init.lua"] = {globals = {"dostring"}}
>  files["src/box/lua/console.lua"] = {ignore = {"212"}}
>  files["src/box/lua/load_cfg.lua"] = {ignore = {"542"}}
> -files["src/box/lua/net_box.lua"] = {ignore = {"431", "432", "231", "411", "212"}}
> +files["src/box/lua/net_box.lua"] = {ignore = {"431", "432", "231", "411"}}
>  files["src/box/lua/schema.lua"] = {ignore = {"431", "432", "542", "212"}}
> diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
> index 1e351bb31..8b3802749 100644
> --- a/src/box/lua/net_box.lua
> +++ b/src/box/lua/net_box.lua
> @@ -56,18 +56,18 @@ local E_PROC_LUA             = box.error.PROC_LUA
>  -- utility tables
>  local is_final_state         = {closed = 1, error = 1}
>  
> -local function decode_nil(raw_data, raw_data_end)
> +local function decode_nil(raw_data, raw_data_end) -- luacheck: no unused args
>      return nil, raw_data_end
>  end
>  local function decode_data(raw_data)
>      local response, raw_end = decode(raw_data)
>      return response[IPROTO_DATA_KEY], raw_end
>  end
> -local function decode_tuple(raw_data, raw_data_end, format)
> +local function decode_tuple(raw_data, raw_data_end, format) -- luacheck: no unused args
>      local response, raw_end = internal.decode_select(raw_data, nil, format)
>      return response[1], raw_end
>  end
> -local function decode_get(raw_data, raw_data_end, format)
> +local function decode_get(raw_data, raw_data_end, format) -- luacheck: no unused args
>      local body, raw_end = internal.decode_select(raw_data, nil, format)
>      if body[2] then
>          return nil, raw_end, box.error.MORE_THAN_ONE_TUPLE
> @@ -110,7 +110,7 @@ local method_encoder = {
>      max     = internal.encode_select,
>      count   = internal.encode_call,
>      -- inject raw data into connection, used by console and tests
> -    inject = function(buf, id, bytes)
> +    inject = function(buf, id, bytes) -- luacheck: no unused args
>          local ptr = buf:reserve(#bytes)
>          ffi.copy(ptr, bytes, #bytes)
>          buf.wpos = ptr + #bytes
> @@ -176,7 +176,7 @@ end
>  -- Default action on push during a synchronous request -
>  -- ignore.
>  --
> -local function on_push_sync_default(...) end
> +local function on_push_sync_default() end
>  
>  --
>  -- Basically, *transport* is a TCP connection speaking one of
> @@ -1216,7 +1216,7 @@ function remote_methods:execute(query, parameters, sql_opts, netbox_opts)
>                           sql_opts or {})
>  end
>  
> -function remote_methods:prepare(query, parameters, sql_opts, netbox_opts)
> +function remote_methods:prepare(query, parameters, sql_opts, netbox_opts) -- luacheck: no unused args
>      check_remote_arg(self, "prepare")
>      if type(query) ~= "string" then
>          box.error(box.error.SQL_PREPARE, "expected string as SQL statement")
> @@ -1581,7 +1581,7 @@ this_module.self = {
>      timeout = function(self) return self end,
>      wait_connected = function(self) return true end,
>      is_connected = function(self) return true end,
> -    call = function(_box, proc_name, args, opts)
> +    call = function(_box, proc_name, args)
>          check_remote_arg(_box, 'call')
>          check_call_args(args)
>          args = args or {}
> @@ -1598,7 +1598,7 @@ this_module.self = {
>              return handle_eval_result(pcall(proc, unpack(args)))
>          end
>      end,
> -    eval = function(_box, expr, args, opts)
> +    eval = function(_box, expr, args)
>          check_remote_arg(_box, 'eval')
>          check_eval_args(args)
>          args = args or {}
> 
> ================================================================================
> 
> Moreover a single (W231)[Local variable is set but never accessed]
> warning is also supressed (but you fixed others in this file). Here is
> the diff:

Applied, thanks!

> ================================================================================
> 
> diff --git a/.luacheckrc b/.luacheckrc
> index 64692b27c..b23ebfc60 100644
> --- a/.luacheckrc
> +++ b/.luacheckrc
> @@ -35,5 +35,5 @@ files["src/lua/fio.lua"] = {ignore = {"231"}}
>  files["src/lua/init.lua"] = {globals = {"dostring"}}
>  files["src/box/lua/console.lua"] = {ignore = {"212"}}
>  files["src/box/lua/load_cfg.lua"] = {ignore = {"542"}}
> -files["src/box/lua/net_box.lua"] = {ignore = {"431", "432", "231", "411", "212"}}
> +files["src/box/lua/net_box.lua"] = {ignore = {"431", "432", "411", "212"}}
>  files["src/box/lua/schema.lua"] = {ignore = {"431", "432", "542", "212"}}
> diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
> index 1e351bb31..bcfec92bc 100644
> --- a/src/box/lua/net_box.lua
> +++ b/src/box/lua/net_box.lua
> @@ -749,13 +749,12 @@ local function create_transport(host, port, user, password, callback,
>              return iproto_schema_sm()
>          end
>          encode_auth(send_buf, new_request_id(), user, password, salt)
> -        local err, hdr, body_rpos, body_end = send_and_recv_iproto()
> +        local err, hdr, body_rpos = send_and_recv_iproto()
>          if err then
>              return error_sm(err, hdr)
>          end
>          if hdr[IPROTO_STATUS_KEY] ~= 0 then
> -            local body
> -            body, body_end = decode(body_rpos)
> +            local body = decode(body_rpos)
>              return error_sm(E_NO_CONNECTION, body[IPROTO_ERROR_KEY])
>          end
>          set_state('fetch_schema')
> 
> ================================================================================
> 
> > +files["src/box/lua/schema.lua"] = {ignore = {"431", "432", "542", "212"}}
> 
> Again, it's a single empty branch warning, that can be suppressed
> inline. Here is the diff:

Applied, thanks!

> ================================================================================
> 
> diff --git a/.luacheckrc b/.luacheckrc
> index 64692b27c..fa715fded 100644
> --- a/.luacheckrc
> +++ b/.luacheckrc
> @@ -36,4 +36,4 @@ files["src/lua/init.lua"] = {globals = {"dostring"}}
>  files["src/box/lua/console.lua"] = {ignore = {"212"}}
>  files["src/box/lua/load_cfg.lua"] = {ignore = {"542"}}
>  files["src/box/lua/net_box.lua"] = {ignore = {"431", "432", "231", "411", "212"}}
> -files["src/box/lua/schema.lua"] = {ignore = {"431", "432", "542", "212"}}
> +files["src/box/lua/schema.lua"] = {ignore = {"431", "432", "212"}}
> diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
> index de47b807e..5fad272ab 100644
> --- a/src/box/lua/schema.lua
> +++ b/src/box/lua/schema.lua
> @@ -259,7 +259,7 @@ local function check_param_table(table, template)
>          if template[k] == nil then
>              box.error(box.error.ILLEGAL_PARAMS,
>                        "unexpected option '" .. k .. "'")
> -        elseif template[k] == 'any' then
> +        elseif template[k] == 'any' then -- luacheck: ignore
>              -- any type is ok
>          elseif (string.find(template[k], ',') == nil) then
>              -- one type
> 
> ================================================================================
> 
> And the following simple diff fixes all remaining (W212)[Unused
> argument] warnings reported for schema.lua:

Applied.

> ================================================================================
> 
> diff --git a/.luacheckrc b/.luacheckrc
> index 64692b27c..bfe144f64 100644
> --- a/.luacheckrc
> +++ b/.luacheckrc
> @@ -36,4 +36,4 @@ files["src/lua/init.lua"] = {globals = {"dostring"}}
>  files["src/box/lua/console.lua"] = {ignore = {"212"}}
>  files["src/box/lua/load_cfg.lua"] = {ignore = {"542"}}
>  files["src/box/lua/net_box.lua"] = {ignore = {"431", "432", "231", "411", "212"}}
> -files["src/box/lua/schema.lua"] = {ignore = {"431", "432", "542", "212"}}
> +files["src/box/lua/schema.lua"] = {ignore = {"431", "432", "542"}}
> diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
> index de47b807e..537443eeb 100644
> --- a/src/box/lua/schema.lua
> +++ b/src/box/lua/schema.lua
> @@ -1229,12 +1229,12 @@ end
>  
>  local iterator_t = ffi.typeof('struct iterator')
>  ffi.metatype(iterator_t, {
> -    __tostring = function(iterator)
> +    __tostring = function()
>          return "<iterator state>"
>      end;
>  })
>  
> -local iterator_gen = function(param, state)
> +local iterator_gen = function(param, state) -- luacheck: no unused args
>      --[[
>          index:pairs() mostly conforms to the Lua for-in loop conventions and
>          tries to follow the best practices of Lua community.
> @@ -1268,7 +1268,7 @@ local iterator_gen = function(param, state)
>      end
>  end
>  
> -local iterator_gen_luac = function(param, state)
> +local iterator_gen_luac = function(param, state) -- luacheck: no unused args
>      local tuple = internal.iterator_next(state)
>      if tuple ~= nil then
>          return state, tuple -- new state, value
> @@ -1773,9 +1773,9 @@ local function wrap_schema_object_mt(name)
>          __pairs = global_mt.__pairs
>      }
>      local mt_mt = {}
> -    mt_mt.__newindex = function(t, k, v)
> +    mt_mt.__newindex = function(self, k, v)
>          mt_mt.__newindex = nil
> -        mt.__index = function(t, k)
> +        mt.__index = function(self, k)
>              return mt[k] or box.schema[name][k]
>          end
>          rawset(mt, k, v)
> @@ -2484,7 +2484,7 @@ local function revoke(uid, name, privilege, object_type, object_name, options)
>      end
>  end
>  
> -local function drop(uid, opts)
> +local function drop(uid)
>      -- recursive delete of user data
>      local _vpriv = box.space[box.schema.VPRIV_ID]
>      local spaces = box.space[box.schema.VSPACE_ID].index.owner:select{uid}
> @@ -2563,7 +2563,7 @@ box.schema.user.drop = function(name, opts)
>              box.error(box.error.DROP_USER, name,
>                        "the user is active in the current session")
>          end
> -        return drop(uid, opts)
> +        return drop(uid)
>      end
>      if not opts.if_exists then
>          box.error(box.error.NO_SUCH_USER, name)
> 
> ================================================================================
> 
> <snipped>
> 
> > diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
> > index 07fa54c38..1e351bb31 100644
> > --- a/src/box/lua/net_box.lua
> > +++ b/src/box/lua/net_box.lua
> 
> <snipped>
> 
> > @@ -425,7 +421,7 @@ local function create_transport(host, port, user, password, callback,
> >          state = new_state
> >          last_errno = new_errno
> >          last_error = new_error
> > -        callback('state_changed', new_state, new_errno, new_error)
> > +        callback('state_changed', new_state, new_error)
> 
> Please adjust the corresponding comment.
> 
> >          state_cond:broadcast()
> >          if state == 'error' or state == 'error_reconnect' or
> >             state == 'closed' then
> 
> <snipped>
> 
> > @@ -954,7 +945,7 @@ local function new_sm(host, port, opts, connection, greeting)
> >      local remote = {host = host, port = port, opts = opts, state = 'initial'}
> >      local function callback(what, ...)
> >          if what == 'state_changed' then
> > -            local state, errno, err = ...
> > +            local state, err = ...
> >              local was_connected = remote._is_connected
> >              if state == 'active' then
> >                  if not was_connected then
> > diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
> > index 85fcca562..de47b807e 100644
> > --- a/src/box/lua/schema.lua
> > +++ b/src/box/lua/schema.lua
> 
> <snipped>
> 
> > @@ -582,9 +580,9 @@ end
> >  --
> >  local function format_field_resolve(format, path, what)
> >      assert(type(path) == 'number' or type(path) == 'string')
> > -    local idx = nil
> > +    local idx
> >      local relative_path = nil
> > -    local field_name = nil
> > +    local field_name
> 
> You can simply move it down to its initialization.

Done

> >      -- Path doesn't require resolve.
> >      if type(path) == 'number' then
> >          idx = path
> 

<snipped>