From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (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 64D9C4696C3 for ; Fri, 24 Apr 2020 02:01:36 +0300 (MSK) Date: Fri, 24 Apr 2020 01:54:27 +0300 From: Igor Munkin Message-ID: <20200423225427.GC11314@tarantool.org> References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Subject: Re: [Tarantool-patches] [PATCH v4 5/10] Fix luacheck warnings in src/box/lua/ List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: sergeyb@tarantool.org Cc: o.piskunov@tarantool.org, tarantool-patches@dev.tarantool.org, v.shpilevoy@tarantool.org Sergey, Thanks for the patch! Please consider the comments I left below. On 21.04.20, sergeyb@tarantool.org wrote: > From: Sergey Bronnikov > > Closes #4681 > > Reviewed-by: Vladislav Shpilevoy > Co-authored-by: Vladislav Shpilevoy > --- > .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. Furthermore, IMHO, global suppression for (W122)[Setting a read-only field of a global variable] is a bad practice. > 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: ================================================================================ 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: ================================================================================ 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: ================================================================================ 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: ================================================================================ 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: ================================================================================ 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 "" 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) ================================================================================ > 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 > @@ -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 > @@ -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 > @@ -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. > -- Path doesn't require resolve. > if type(path) == 'number' then > idx = path > -- > 2.23.0 > -- Best regards, IM