[Tarantool-patches] [PATCH v4 5/10] Fix luacheck warnings in src/box/lua/
Igor Munkin
imun at tarantool.org
Fri Apr 24 01:54:27 MSK 2020
Sergey,
Thanks for the patch! Please consider the comments I left below.
On 21.04.20, sergeyb at tarantool.org wrote:
> From: Sergey Bronnikov <sergeyb at tarantool.org>
>
> Closes #4681
>
> Reviewed-by: Vladislav Shpilevoy <v.shpilevoy at tarantool.org>
> Co-authored-by: Vladislav Shpilevoy <v.shpilevoy at tarantool.org>
> ---
> .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 "<iterator state>"
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)
================================================================================
<snipped>
> 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
<snipped>
> @@ -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
<snipped>
> @@ -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
<snipped>
> @@ -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
<snipped>
> --
> 2.23.0
>
--
Best regards,
IM
More information about the Tarantool-patches
mailing list