Re: [Tarantool-patches] [PATCH] Fix luacheck warnings in src/lua/*.lua

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the patch!

See 14 comments below, diff in the end of the email,
and on top of the branch in a separate commit.

On 03/04/2020 11:39, Sergey Bronnikov wrote:
> GitHub branch: https://github.com/tarantool/tarantool/tree/ligurio/gh-4681-fix-luacheck-warnings
> 
> How-to check:
> 
> $ tarantoolctl rocks install luacheck
> $ .rocks/bin/luacheck src/lua/*.lua

1. This patch clearly relates to 4681. Please, add a reference
to it. As

    'Part of #4681'

2. I am still getting warnings, when run the check:

Checking src/lua/load_ffi_defs.lua                9 warnings

    src/lua/load_ffi_defs.lua:8:1: setting non-standard global variable ffi
    src/lua/load_ffi_defs.lua:10:1: accessing undefined variable ffi
    src/lua/load_ffi_defs.lua:1607:4: accessing undefined variable ffi
    src/lua/load_ffi_defs.lua:1607:23: accessing undefined variable ffi
    src/lua/load_ffi_defs.lua:1608:5: accessing undefined variable ffi
    src/lua/load_ffi_defs.lua:1624:5: accessing undefined variable ffi
    src/lua/load_ffi_defs.lua:1639:1: accessing undefined variable ffi
    src/lua/load_ffi_defs.lua:1701:1: accessing undefined variable ffi
    src/lua/load_ffi_defs.lua:1730:1: accessing undefined variable ffi

> ---
>  src/lua/argparse.lua   |  6 ++--
>  src/lua/buffer.lua     |  4 +--
>  src/lua/clock.lua      |  2 +-
>  src/lua/crypto.lua     | 18 +++++-----
>  src/lua/csv.lua        |  5 ++-
>  src/lua/digest.lua     |  6 ++--
>  src/lua/env.lua        |  2 +-
>  src/lua/errno.lua      |  2 +-
>  src/lua/fio.lua        | 28 ++++++++--------
>  src/lua/help.lua       | 24 +++++++-------
>  src/lua/httpc.lua      |  3 --
>  src/lua/init.lua       | 17 +++++-----
>  src/lua/msgpackffi.lua | 15 ++++-----
>  src/lua/socket.lua     | 66 ++++++++++++++++++-------------------
>  src/lua/string.lua     |  1 -
>  src/lua/swim.lua       | 20 +++++------
>  src/lua/tap.lua        | 75 ++++++++++++++++++++----------------------
>  src/lua/trigger.lua    |  3 --
>  18 files changed, 142 insertions(+), 155 deletions(-)
> 
> diff --git a/src/lua/buffer.lua b/src/lua/buffer.lua
> index 9aac82b39..5979980ed 100644
> --- a/src/lua/buffer.lua
> +++ b/src/lua/buffer.lua
> @@ -182,7 +182,7 @@ local ibuf_methods = {
>      unused = ibuf_unused;
>  }
>  
> -local function ibuf_tostring(ibuf)
> +local function ibuf_tostring(ibuf) -- luacheck: ignore 212

3. You can remove this argument. In Lua it is ok to call a
function and pass more arguments than it has.

>      return '<ibuf>'
>  end
>  local ibuf_mt = {> diff --git a/src/lua/env.lua b/src/lua/env.lua
> index dd1616a84..d851586f0 100644
> --- a/src/lua/env.lua
> +++ b/src/lua/env.lua
> @@ -28,7 +28,7 @@ os.environ = function()
>  end
>  
>  os.setenv = function(key, value)
> -    local rv = nil
> +    local rv = nil -- luacheck: ignore 311

4. Just remove '= nil'.

>      if value ~= nil then
>          rv = ffi.C.setenv(key, value, 1)
>      else
> diff --git a/src/lua/errno.lua b/src/lua/errno.lua
> index db800ce30..c91cad4b7 100644
> --- a/src/lua/errno.lua
> +++ b/src/lua/errno.lua
> @@ -17,5 +17,5 @@ return setmetatable({
>  }, {
>      __index = errno_list,
>      __newindex = function() error("Can't create new errno constants") end,
> -    __call = function(self, ...) return ffi.errno(...) end
> +    __call = function(self, ...) return ffi.errno(...) end -- luacheck: ignore 212

5. This error is actually ridiculous in such a language as Lua.
Can we ignore it globally?

The same for error 122.

>  })
> diff --git a/src/lua/fio.lua b/src/lua/fio.lua
> index 4692e1026..dd71c6742 100644
> --- a/src/lua/fio.lua
> +++ b/src/lua/fio.lua
> @@ -306,7 +306,7 @@ fio.abspath = function(path)
>          error("Usage: fio.abspath(path)")
>      end
>      path = path
> -    local joined_path = ''
> +    local joined_path = '' -- luacheck: ignore 311

6. "= ''" can be removed.

>      local path_tab = {}
>      if string.sub(path, 1, 1) == '/' then
>          joined_path = path> @@ -430,7 +430,6 @@ fio.copytree = function(from, to)
>      if type(from) ~= 'string' or type(to) ~= 'string' then
>          error('Usage: fio.copytree(from, to)')
>      end
> -    local status, reason
>      local st = fio.stat(from)
>      if not st then
>          return false, string.format("Directory %s does not exist", from)
> @@ -444,14 +443,14 @@ fio.copytree = function(from, to)
>      end
>  
>      -- create tree of destination
> -    status, reason = fio.mktree(to)
> +    local status, reason = fio.mktree(to) -- luacheck: ignore 231

7. You could keep this line untouched, but add the
warning ignorance to the original line above. To
change as few lines as possible to keep git blame
clean.

> diff --git a/src/lua/help.lua b/src/lua/help.lua
> index 54ff1b5d0..768f4dec4 100644
> --- a/src/lua/help.lua
> +++ b/src/lua/help.lua
> @@ -7,22 +7,22 @@ local doc = require('help.en_US')
>  -- corresponds to a tarantool version a user runs.
>  local DOCUMENTATION_VERSION = '2.1'
>  
> -help = { doc.help }
> -tutorial = {}
> -tutorial[1] = help[1]
> +help = { doc.help } -- luacheck: ignore 111
> +tutorial = {} -- luacheck: ignore 111

8. This is insane. Should every global usage of these variables
be accompanied by these comments? Can they be ignored globally?
Or at least for this file?

> +tutorial[1] = help[1] -- luacheck: ignore 112 113
>  
> -local help_function_data = {};
> -local help_object_data = {}
> +local help_function_data = {}; -- luacheck: ignore 211
> +local help_object_data = {} -- luacheck: ignore 211
>  
> -local function help_call(table, param)
> -    return help
> +local function help_call(table, param) -- luacheck: ignore 212
> +    return help -- luacheck: ignore 113
>  end
>  
> -setmetatable(help, { __call = help_call })
> +setmetatable(help, { __call = help_call }) -- luacheck: ignore 113
>  
>  local screen_id = 1;
>  
> -local function tutorial_call(table, action)
> +local function tutorial_call(table, action) -- luacheck: ignore 212
>      if action == 'start' then
>          screen_id = 1;
>      elseif action == 'next' or action == 'more' then
> @@ -46,9 +46,9 @@ local function tutorial_call(table, action)
>      return (res:gsub('<version>', DOCUMENTATION_VERSION))
>  end
>  
> -setmetatable(tutorial, { __call = tutorial_call })
> +setmetatable(tutorial, { __call = tutorial_call }) -- luacheck: ignore 113
>  
>  return {
> -    help = help;
> -    tutorial = tutorial;
> +    help = help; -- luacheck: ignore 113
> +    tutorial = tutorial; -- luacheck: ignore 113
>  }
> diff --git a/src/lua/msgpackffi.lua b/src/lua/msgpackffi.lua
> index f775f2d41..471b72c84 100644
> --- a/src/lua/msgpackffi.lua
> +++ b/src/lua/msgpackffi.lua
> @@ -291,7 +291,7 @@ local function encode(obj)
>      return r
>  end
>  
> -local function encode_ibuf(obj, ibuf)
> +local function encode_ibuf(obj, ibuf) -- luacheck: ignore 211

9. This function is just unused.

>      encode_r(ibuf, obj, 0)
>  end
>  
> @@ -320,7 +320,7 @@ local decode_r
>  
>  -- See similar constants in utils.cc
>  local DBL_INT_MAX = 1e14 - 1
> -local DBL_INT_MIN = -1e14 + 1
> +local DBL_INT_MIN = -1e14 + 1 -- luacheck: ignore 211

10. Also really unused, and can be removed.

>  
>  local function decode_u8(data)
>      local num = ffi.cast(uint8_ptr_t, data[0])[0]
> @@ -465,8 +465,7 @@ end
>  local function decode_array(data, size)
>      assert (type(size) == "number")
>      local arr = {}
> -    local i
> -    for i=1,size,1 do
> +    for i=1,size,1 do -- luacheck: ignore 213

11. Seems like you decided to use '_' to ignore a loop
variable in all the similar places above and below.
Why didn't you apply it here?

>          table.insert(arr, decode_r(data))
>      end
> diff --git a/src/lua/socket.lua b/src/lua/socket.lua
> index a334ad45b..c5691a425 100644
> --- a/src/lua/socket.lua
> +++ b/src/lua/socket.lua
> @@ -1311,7 +1311,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: ignore 212

12. Mode is the last argument and can be removed.

>      check_socket(self)
>      self.timeout = value
>      -- mode is effectively ignored
> diff --git a/src/lua/swim.lua b/src/lua/swim.lua
> index 01eeb7eae..7fd2e8250 100644
> --- a/src/lua/swim.lua
> +++ b/src/lua/swim.lua
> @@ -1,5 +1,5 @@
>  local ffi = require('ffi')
> -local uuid = require('uuid')
> +local uuid_ = require('uuid')

13. Yeah, this line is basically the reason I decided to review
the patch. I deliberately shadowed this global name in some
functions, because the name is good, the library is not used
in these functions, and this is our convention - we don't call
a library 'name + _'. For example, we don't call them 'fiber_',
or 'ffi_', or 'console_', and so on. Is it possible to ignore
this error?

I would better expect from luacheck some real help. Like usage
of undeclared variables, duplicate keys in declared tables,
unused functions. Complaints about other staff just make the
code dirty with all these 'luacheck: ignore', IMO.

Most of the warning fixes in this patch made the code worse.
For example, using '_' as a variable name sometimes makes it
harder to understand what is in that value so as it is ignored
and why.

I think we should have a config where the useless or harmful
warnings are ignored. It seems luacheck supports configuration
stored in a special file.

>  local buffer = require('buffer')
>  local msgpack = require('msgpack')
>  local crypto = require('crypto')
> diff --git a/src/lua/tap.lua b/src/lua/tap.lua
> index 94b080d5a..04497386e 100644
> --- a/src/lua/tap.lua
> +++ b/src/lua/tap.lua
> @@ -53,7 +53,7 @@ local function ok(test, cond, message, extra)
>      io.write(string.format("not ok - %s\n", message))
>      extra = extra or {}
>      if test.trace then
> -        local frame = debug.getinfo(3, "Sl")
> +        debug.getinfo(3, "Sl")

14. Why is that call needed, if its result is not used?

>          extra.trace = traceback()
>          extra.filename = extra.trace[#extra.trace].filename
>          extra.line = extra.trace[#extra.trace].line
Commit on top of your branch:

====================
diff --git a/src/lua/buffer.lua b/src/lua/buffer.lua
index 5979980ed..43c7e1170 100644
--- a/src/lua/buffer.lua
+++ b/src/lua/buffer.lua
@@ -182,7 +182,7 @@ local ibuf_methods = {
     unused = ibuf_unused;
 }
 
-local function ibuf_tostring(ibuf) -- luacheck: ignore 212
+local function ibuf_tostring()
     return '<ibuf>'
 end
 local ibuf_mt = {
diff --git a/src/lua/env.lua b/src/lua/env.lua
index d851586f0..a31b7098f 100644
--- a/src/lua/env.lua
+++ b/src/lua/env.lua
@@ -28,7 +28,7 @@ os.environ = function()
 end
 
 os.setenv = function(key, value)
-    local rv = nil -- luacheck: ignore 311
+    local rv
     if value ~= nil then
         rv = ffi.C.setenv(key, value, 1)
     else
diff --git a/src/lua/fio.lua b/src/lua/fio.lua
index dd71c6742..362736871 100644
--- a/src/lua/fio.lua
+++ b/src/lua/fio.lua
@@ -306,7 +306,7 @@ fio.abspath = function(path)
         error("Usage: fio.abspath(path)")
     end
     path = path
-    local joined_path = '' -- luacheck: ignore 311
+    local joined_path
     local path_tab = {}
     if string.sub(path, 1, 1) == '/' then
         joined_path = path
@@ -430,6 +430,7 @@ fio.copytree = function(from, to)
     if type(from) ~= 'string' or type(to) ~= 'string' then
         error('Usage: fio.copytree(from, to)')
     end
+    local status, reason -- luacheck: ignore 231
     local st = fio.stat(from)
     if not st then
         return false, string.format("Directory %s does not exist", from)
@@ -443,7 +444,7 @@ fio.copytree = function(from, to)
     end
 
     -- create tree of destination
-    local status, reason = fio.mktree(to) -- luacheck: ignore 231
+    status, reason = fio.mktree(to)
     if reason ~= nil then
         return false, reason
     end
diff --git a/src/lua/msgpackffi.lua b/src/lua/msgpackffi.lua
index 471b72c84..eacf2d1d5 100644
--- a/src/lua/msgpackffi.lua
+++ b/src/lua/msgpackffi.lua
@@ -291,10 +291,6 @@ local function encode(obj)
     return r
 end
 
-local function encode_ibuf(obj, ibuf) -- luacheck: ignore 211
-    encode_r(ibuf, obj, 0)
-end
-
 on_encode(ffi.typeof('uint8_t'), encode_int)
 on_encode(ffi.typeof('uint16_t'), encode_int)
 on_encode(ffi.typeof('uint32_t'), encode_int)
@@ -320,7 +316,6 @@ local decode_r
 
 -- See similar constants in utils.cc
 local DBL_INT_MAX = 1e14 - 1
-local DBL_INT_MIN = -1e14 + 1 -- luacheck: ignore 211
 
 local function decode_u8(data)
     local num = ffi.cast(uint8_ptr_t, data[0])[0]
@@ -465,7 +460,7 @@ end
 local function decode_array(data, size)
     assert (type(size) == "number")
     local arr = {}
-    for i=1,size,1 do -- luacheck: ignore 213
+    for _ = 1, size do
         table.insert(arr, decode_r(data))
     end
     if not msgpack.cfg.decode_save_metatables then
@@ -477,7 +472,7 @@ end
 local function decode_map(data, size)
     assert (type(size) == "number")
     local map = {}
-    for i=1,size,1 do -- luacheck: ignore 213
+    for _ = 1, size do
         local key = decode_r(data);
         local val = decode_r(data);
         map[key] = val
diff --git a/src/lua/socket.lua b/src/lua/socket.lua
index c5691a425..c0aa48bbe 100644
--- a/src/lua/socket.lua
+++ b/src/lua/socket.lua
@@ -1311,10 +1311,9 @@ 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) -- luacheck: ignore 212
+local function lsocket_tcp_settimeout(self, value)
     check_socket(self)
     self.timeout = value
-    -- mode is effectively ignored
     return 1
 end