[Tarantool-patches] [PATCH] Fix luacheck warnings in src/lua/*.lua
Sergey Bronnikov
sergeyb at tarantool.org
Tue Apr 7 18:35:25 MSK 2020
Vlad, thanks a lot for your review. I have updated patchset according to
your comments and will send updated patchset to a list soon. Also see my
comments below.
On 00:39 Sat 04 Apr , Vladislav Shpilevoy wrote:
> 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'
Done.
> 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.
Done.
> > 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'.
Done.
> > 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.
Sure, both warnings W122 and W112 now ignored globally.
> > })
> > 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.
Done.
> > 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.
Moved all supressions from source code to a luacheck config.
> > 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?
Global variables specified per file in a luacheck config.
> > +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.
Done by applying your patch.
> >
> > 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?
Perhaps it was a first solution and then decided to use '_' :)
> > 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.
Done by applying your patch.
> > 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?
Sure, renamed uuid_ back and supressed warning in a luacheck config.
> 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.
Agree with you, some categories of warnings are annoying. But some
warnings complains about inaccurate coding and fixing them make code
clear. See my summary about using luacheck in updated patchset.
> 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 have fixed a number of warnings like 'unused index variables in a
loop' in updated patchset and did my best to keep code understandable.
Anyway please look on updated version.
> 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.
Did it and added .luacheckrc.
> > 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?
AFAIR it has been fixed by your patch.
> > extra.trace = traceback()
> > extra.filename = extra.trace[#extra.trace].filename
> > extra.line = extra.trace[#extra.trace].line
> Commit on top of your branch:
>
<snipped>
Sergey
More information about the Tarantool-patches
mailing list