From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp55.i.mail.ru (smtp55.i.mail.ru [217.69.128.35]) (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 8FE45469710 for ; Thu, 7 May 2020 13:33:16 +0300 (MSK) Date: Thu, 7 May 2020 13:32:55 +0300 From: Sergey Bronnikov Message-ID: <20200507103255.GA34300@pony.bronevichok.ru> References: <20200423225427.GC11314@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200423225427.GC11314@tarantool.org> 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: Igor Munkin Cc: o.piskunov@tarantool.org, tarantool-patches@dev.tarantool.org, v.shpilevoy@tarantool.org Igor, thanks for review! I have corrected patches according to your comments. On 01:54 Fri 24 Apr , Igor Munkin wrote: > 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. I'll move these globals to a patch specific to src/lua/. > Furthermore, IMHO, global suppression for (W122)[Setting a read-only > field of a global variable] is a bad practice. requested by Vladislav in [1] (comment 5): "This error is actually ridiculous in such a language as Lua. Can we ignore it globally? The same for error 122." [1] https://lists.tarantool.org/pipermail/tarantool-patches/2020-April/015404.html > > 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: I thought we don't want to use inline supressions at all. Well, applied patch below on a branch. > ================================================================================ > > 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: Applied, thanks! > ================================================================================ > > 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: Applied, thanks! > ================================================================================ > > 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: Applied, thanks! > ================================================================================ > > 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: Applied. > ================================================================================ > > 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. Done > > -- Path doesn't require resolve. > > if type(path) == 'number' then > > idx = path >