From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id EAE374696C3 for ; Wed, 15 Apr 2020 18:21:47 +0300 (MSK) Date: Wed, 15 Apr 2020 18:14:41 +0300 From: Igor Munkin Message-ID: <20200415151441.GC8314@tarantool.org> References: <10d6c48a3f320e81b8bd8b05de385f20cbfead9c.1586341316.git.sergeyb@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <10d6c48a3f320e81b8bd8b05de385f20cbfead9c.1586341316.git.sergeyb@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v2 4/6] Fix luacheck warnings in extra/dist/tarantoolctl.in List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Sergey Bronnikov Cc: o.piskunov@tarantool.org, tarantool-patches@dev.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 > > 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 > @@ -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 > }) > -- > 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