Re: [Tarantool-patches] [PATCH v2 4/6] Fix luacheck warnings in extra/dist/tarantoolctl.in

[Igor Munkin <imun@tarantool.org>]

Sergey,

Thanks for the patch! I started with this one, since it's simple and I
can point the general issues related to the fix.

I see the following approach to fix the errors and warning found by
luacheck in *Lua sources*:

* If the fix is obvious and simple -- please do it.
* If the warning/erro fix leads to code complication or reduce
  readability (e.g. empty if branch) -- let's add an inline ignore
  comment, since it unlikely to be fixed and just burdens .luacheckrc.
* If none of the above -- add the corresponding rule to .luacheckrc.

I left inline comments for your patch and added the fix I made for
tarantoolctl at the end. Please consider them.

On 08.04.20, Sergey Bronnikov wrote:
> From: Sergey Bronnikov <sergeyb@tarantool.org>
> 
> Closes #4681
> ---
>  extra/dist/tarantoolctl.in | 20 +++++++++-----------
>  1 file changed, 9 insertions(+), 11 deletions(-)
> 
> diff --git a/extra/dist/tarantoolctl.in b/extra/dist/tarantoolctl.in
> index f5a912ccd..3bcbe5b79 100755
> --- a/extra/dist/tarantoolctl.in
> +++ b/extra/dist/tarantoolctl.in

<snipped>

> @@ -64,7 +63,7 @@ end
>  
>  local function check_user_level()
>      local uid = os.getenv('UID')
> -    local udir = nil
> +    local udir

Minor: I looked at the code nearby and it looks like this initialization
can be moved below, since udir is unused prior it (consider the changes
below).

>      if uid == 0 or os.getenv("NOTIFY_SOCKET") then
>          return nil
>      end
> @@ -420,7 +419,7 @@ local cat_formats = setmetatable({
>      json = cat_json_cb,
>      lua  = cat_lua_cb,
>  }, {
> -    __index = function(self, cmd)
> +    __index = function(cmd)

Side-note: I see you reverted this change, but the fix proposed by Oleg
is a good one. I guess we can apply it here.

>          error(("Unknown formatter '%s'"):format(cmd))
>      end
>  })

<snipped>

> -- 
> 2.23.0
> 

I tried to fix the issues by myself and dumped my step-by-step activity
below. At first let's see what luacheck reports for tarantoolctl source:

| $ luacheck --codes extra/dist/tarantoolctl.in | tail -1
| Total: 35 warnings / 0 errors in 1 file
| $ luacheck --codes extra/dist/tarantoolctl.in | \
|         grep -vP '^(Total:|Checking|$)' | cut -f 6 -d ' ' | sort | uniq -c
|       1 (W112)
|       5 (W113)
|       1 (W122)
|       2 (W211)
|       3 (W212)
|       4 (W213)
|       3 (W311)
|       2 (W411)
|      13 (W431)
|       1 (W542)

According to the list of warnings[1] W113 is reported when accessing an
undefined global variable is detected. At the same time W112 is reported
when mutating an undefined global variable is detected. Considering
output these are well known Tarantool globals: box and _TARANTOOL. Let's
suppress them (now with CLI option and globally in .luacheckrc further):
| $ luacheck --codes extra/dist/tarantoolctl.in --globals box --globals _TARANTOOL | \
|         grep -vP '^(Total:|Checking|$)' | cut -f 6 -d ' ' | sort | uniq -c
|       1 (W122)
|       2 (W211)
|       3 (W212)
|       4 (W213)
|       3 (W311)
|       2 (W411)
|      13 (W431)
|       1 (W542)

Well, 6 false-positive warnings are suppressed, 29 others left. Let's
see the remaining ones:

| $ luacheck --codes extra/dist/tarantoolctl.in --globals box --globals _TARANTOOL
| Checking extra/dist/tarantoolctl.in               29 warnings
|
|     extra/dist/tarantoolctl.in:49:7: (W211) unused variable 'lua_arguments'
|     extra/dist/tarantoolctl.in:67:11: (W311) value assigned to variable 'udir' is unused
|     extra/dist/tarantoolctl.in:127:34: (W431) shadowing upvalue 'default_file' on line 38
|     extra/dist/tarantoolctl.in:136:22: (W411) variable 'msg' was previously defined on line 130
|     extra/dist/tarantoolctl.in:217:5: (W122) setting read-only field '?' of global 'arg'
|     extra/dist/tarantoolctl.in:423:24: (W212) unused argument 'self'
|     extra/dist/tarantoolctl.in:440:15: (W431) shadowing upvalue 'console_sock' on line 42
|     extra/dist/tarantoolctl.in:573:11: (W431) shadowing upvalue 'console_sock' on line 42
|     extra/dist/tarantoolctl.in:634:11: (W431) shadowing upvalue 'console_sock' on line 42
|     extra/dist/tarantoolctl.in:695:31: (W431) shadowing upvalue 'uri' on line 9
|     extra/dist/tarantoolctl.in:759:11: (W431) shadowing upvalue 'console_sock' on line 42
|     extra/dist/tarantoolctl.in:793:11: (W311) value assigned to variable 'code' is unused
|     extra/dist/tarantoolctl.in:819:11: (W431) shadowing upvalue 'status' on line 750
|     extra/dist/tarantoolctl.in:851:11: (W211) unused variable 'spaces'
|     extra/dist/tarantoolctl.in:852:21: (W411) variable 'spaces' was previously defined on line 851
|     extra/dist/tarantoolctl.in:865:59: (W542) empty if branch
|     extra/dist/tarantoolctl.in:878:9: (W213) unused loop variable 'id'
|     extra/dist/tarantoolctl.in:895:11: (W431) shadowing upvalue 'uri' on line 9
|     extra/dist/tarantoolctl.in:904:9: (W213) unused loop variable 'id'
|     extra/dist/tarantoolctl.in:926:9: (W213) unused loop variable 'i'
|     extra/dist/tarantoolctl.in:940:11: (W311) value assigned to variable 'key' is unused
|     extra/dist/tarantoolctl.in:957:30: (W212) unused argument 'command'
|     extra/dist/tarantoolctl.in:957:39: (W212) unused argument 'program'
|     extra/dist/tarantoolctl.in:1324:9: (W213) unused loop variable 'no'
|     extra/dist/tarantoolctl.in:1333:31: (W431) shadowing upvalue 'commands' on line 1019
|     extra/dist/tarantoolctl.in:1335:18: (W431) shadowing upvalue 'self_name' on line 30
|     extra/dist/tarantoolctl.in:1387:15: (W431) shadowing upvalue 'command_name' on line 32
|     extra/dist/tarantoolctl.in:1388:15: (W431) shadowing upvalue 'positional_arguments' on line 47
|     extra/dist/tarantoolctl.in:1388:37: (W431) shadowing upvalue 'keyword_arguments' on line 48
|
| Total: 29 warnings / 0 errors in 1 file

Considering your patch you've already fixed all W211[unused local
variable], W212[unused argument] (considering the fix proposed by Oleg)
and W213[unused loop variable] warnings. The fixes are obvious and
good. Furthermore, you've successfully fixed all W311[value assigned to
a local variable is unused] warnings (but I fixed one of them in a
slightly different way below).

If I didn't miss anything, only the following warnings are left:
| $ luacheck --codes extra/dist/tarantoolctl.in --globals box --globals _TARANTOOL | \
|         grep -vP '^(Total:|Checking|$)' | cut -f 6 -d ' ' | sort | uniq -c
|       1 (W122)
|       1 (W411)
|      13 (W431)
|       1 (W542)

* extra/dist/tarantoolctl.in:136:22: (W411) variable 'msg' was previously defined on line 130
  Both msg values are well-localized, so I guess we can simply rename
  both occurences (consider the changes below).
* extra/dist/tarantoolctl.in:865:59: (W542) empty if branch
  This warning is also false-positive, since the way the branches are
  organized makes code clearer. Thereby this warning looks unlikely to
  be fixed, so I added an inline suppression.

The remaining errors (read-only arg table modification and upvalue
shadowing) can be placed to .luacheckrc to be fixed in future, since
fixes seem to be non-trivial and some of them introduce changes to
function contracts/behaviour.

Omitting few lines to be added to .luacheckrc, here are my changes:

================================================================================

diff --git a/extra/dist/tarantoolctl.in b/extra/dist/tarantoolctl.in
index f5a912ccd..a80ecc057 100755
--- a/extra/dist/tarantoolctl.in
+++ b/extra/dist/tarantoolctl.in
@@ -46,7 +46,6 @@ local instance_dir
 local default_cfg
 local positional_arguments
 local keyword_arguments
-local lua_arguments = arg
 local language
 
 -- function for printing usage reference
@@ -64,13 +63,12 @@ end
 
 local function check_user_level()
     local uid = os.getenv('UID')
-    local udir = nil
     if uid == 0 or os.getenv("NOTIFY_SOCKET") then
         return nil
     end
     -- local dir configuration
     local pwd = os.getenv('PWD')
-    udir = pwd and pwd .. '/.tarantoolctl'
+    local udir = pwd and pwd .. '/.tarantoolctl' or nil
     udir = udir and fio.stat(udir) and udir or nil
     -- or home dir configuration
     local homedir = os.getenv('HOME')
@@ -127,15 +125,15 @@ end
 local function load_default_file(default_file)
     if default_file then
         local env = setmetatable({}, { __index = _G })
-        local ufunc, msg = loadfile(default_file)
+        local ufunc, loaderr = loadfile(default_file)
         -- if load fails - show the last 10 lines of the log file
         if not ufunc then
-            log.error("Failed to load defaults file: %s", msg)
+            log.error("Failed to load defaults file: %s", loaderr)
         end
         debug.setfenv(ufunc, env)
-        local state, msg = pcall(ufunc)
+        local state, err = pcall(ufunc)
         if not state then
-            log.error('Failed to execute defaults file: %s', msg)
+            log.error('Failed to execute defaults file: %s', err)
         end
         default_cfg = env.default_cfg
         instance_dir = env.instance_dir
@@ -420,7 +418,7 @@ local cat_formats = setmetatable({
     json = cat_json_cb,
     lua  = cat_lua_cb,
 }, {
-    __index = function(self, cmd)
+    __index = function(_, cmd)
         error(("Unknown formatter '%s'"):format(cmd))
     end
 })
@@ -790,7 +788,7 @@ end
 local function eval()
     local console_sock_path = uri.parse(console_sock).service
     local filename = arg[3]
-    local code = nil
+    local code
     if filename == nil then
         if stdin_isatty() then
             log.error("Usage:")
@@ -848,7 +846,6 @@ end
 -- xlog / snap file, so even when it stops on LSN >= @a opts.to on
 -- a current file a next file will be processed.
 local function filter_xlog(gen, param, state, opts, cb)
-    local spaces = opts.spaces
     local from, to, spaces = opts.from, opts.to, opts.space
     local show_system, replicas = opts['show-system'], opts.replica
 
@@ -862,7 +859,7 @@ local function filter_xlog(gen, param, state, opts, cb)
         elseif (lsn < from) or (lsn >= to) or
            (not spaces and sid and sid < 512 and not show_system) or
            (spaces and (sid == nil or not find_in_list(sid, spaces))) or
-           (replicas and not find_in_list(rid, replicas)) then
+           (replicas and not find_in_list(rid, replicas)) then -- luacheck: ignore
             -- pass this tuple
         else
             cb(record)
@@ -875,7 +872,7 @@ local function cat()
     local cat_format = opts.format
     local format_cb = cat_formats[cat_format]
     local is_printed = false
-    for id, file in ipairs(positional_arguments) do
+    for _, file in ipairs(positional_arguments) do
         log.error("Processing file '%s'", file)
         local gen, param, state = xlog.pairs(file)
         filter_xlog(gen, param, state, opts, function(record)
@@ -901,7 +898,7 @@ local function play()
     if not remote:wait_connected() then
         error(("Error while connecting to host '%s'"):format(uri))
     end
-    for id, file in ipairs(positional_arguments) do
+    for _, file in ipairs(positional_arguments) do
         log.info(("Processing file '%s'"):format(file))
         local gen, param, state = xlog.pairs(file)
         filter_xlog(gen, param, state, opts, function(record)
@@ -923,7 +920,7 @@ local function play()
 end
 
 local function find_arg(name_arg)
-    for i, key in ipairs(arg) do
+    for _, key in ipairs(arg) do
         if key:find(name_arg) ~= nil then
             return key
         end
@@ -937,7 +934,7 @@ local function rocks()
     local util = require("luarocks.util")
     local command_line = require("luarocks.cmd")
     local options = keyword_arguments
-    local key = nil
+    local key
     if options["only-server"] ~= nil then
         key = find_arg("only%-server")
     else
@@ -954,7 +951,7 @@ local function rocks()
     end
 
     -- Tweak help messages
-    util.see_help = function(command, program)
+    util.see_help = function()
         -- TODO: print extended help message here
         return "See Tarantool documentation for help."
     end
@@ -1321,7 +1318,7 @@ local function usage_command(name, cmd)
     if type(header) == 'string' then
         header = { header }
     end
-    for no, line in ipairs(header) do
+    for _, line in ipairs(header) do
         log.error("    " .. line, name)
     end
 end

================================================================================

[1]: https://luacheck.readthedocs.io/en/stable/warnings.html

-- 
Best regards,
IM