* [Tarantool-patches] [PATCH 0/6] Add static analysis of Lua code in src/ and extra/ with luacheck
@ 2020-06-04 8:39 sergeyb
2020-06-04 8:39 ` [Tarantool-patches] [PATCH 1/6] Add initial luacheck config sergeyb
` (10 more replies)
0 siblings, 11 replies; 41+ messages in thread
From: sergeyb @ 2020-06-04 8:39 UTC (permalink / raw)
To: tarantool-patches, imun, v.shpilevoy; +Cc: alexander.turenko
From: Sergey Bronnikov <sergeyb@tarantool.org>
Sergey Bronnikov (6):
Add initial luacheck config
build: enable 'make luacheck' target
gitlab-ci: enable static analysis with luacheck
Fix luacheck warnings in extra/dist/
Fix luacheck warnings in src/lua/
Fix luacheck warnings in src/box/lua/
.gitlab-ci.yml | 11 +++++++
.luacheckrc | 55 +++++++++++++++++++++++++++++++++
.travis.mk | 23 +++++++++++++-
CMakeLists.txt | 11 +++++++
extra/dist/tarantoolctl.in | 25 +++++----------
src/box/lua/console.lua | 10 +++---
src/box/lua/feedback_daemon.lua | 2 +-
src/box/lua/key_def.lua | 2 +-
src/box/lua/load_cfg.lua | 11 +++----
src/box/lua/net_box.lua | 52 ++++++++++++-------------------
src/box/lua/schema.lua | 44 +++++++++++++-------------
src/box/lua/tuple.lua | 8 ++---
src/box/lua/upgrade.lua | 19 ++++++------
src/lua/argparse.lua | 3 +-
src/lua/buffer.lua | 4 +--
src/lua/clock.lua | 2 +-
src/lua/crypto.lua | 4 +--
src/lua/csv.lua | 5 ++-
src/lua/digest.lua | 2 +-
src/lua/env.lua | 2 +-
src/lua/fiber.lua | 4 +--
src/lua/fio.lua | 30 +++++++++---------
src/lua/help.lua | 7 ++---
src/lua/httpc.lua | 3 --
src/lua/init.lua | 4 +--
src/lua/msgpackffi.lua | 22 ++++++-------
src/lua/socket.lua | 16 +++++-----
src/lua/string.lua | 1 -
src/lua/swim.lua | 2 +-
src/lua/tap.lua | 4 ---
src/lua/trigger.lua | 3 --
31 files changed, 220 insertions(+), 171 deletions(-)
create mode 100644 .luacheckrc
--
2.23.0
^ permalink raw reply [flat|nested] 41+ messages in thread
* [Tarantool-patches] [PATCH 1/6] Add initial luacheck config
2020-06-04 8:39 [Tarantool-patches] [PATCH 0/6] Add static analysis of Lua code in src/ and extra/ with luacheck sergeyb
@ 2020-06-04 8:39 ` sergeyb
2020-06-06 18:02 ` Vladislav Shpilevoy
2020-06-04 8:39 ` [Tarantool-patches] [PATCH 2/6] build: enable 'make luacheck' target sergeyb
` (9 subsequent siblings)
10 siblings, 1 reply; 41+ messages in thread
From: sergeyb @ 2020-06-04 8:39 UTC (permalink / raw)
To: tarantool-patches, imun, v.shpilevoy; +Cc: alexander.turenko
From: Sergey Bronnikov <sergeyb@tarantool.org>
config includes all files with Lua source code except:
- third_party repositories
- directories with diff-based tests
How-to check:
$ luacheck --codes --config .luacheckrc .
Part of #4681
Reviewed-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Co-authored-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
---
.luacheckrc | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
create mode 100644 .luacheckrc
diff --git a/.luacheckrc b/.luacheckrc
new file mode 100644
index 000000000..e6edf8617
--- /dev/null
+++ b/.luacheckrc
@@ -0,0 +1,16 @@
+std = "luajit"
+
+include_files = {
+ "**/*.lua",
+}
+
+exclude_files = {
+ "build/**/*.lua",
+ "extra/**/*.lua",
+ "src/**/*.lua",
+ "test-run/**/*.lua",
+ "test/**/*.lua",
+ "third_party/**/*.lua",
+ ".rocks/**/*.lua",
+ ".git/**/*.lua",
+}
--
2.23.0
^ permalink raw reply [flat|nested] 41+ messages in thread
* [Tarantool-patches] [PATCH 2/6] build: enable 'make luacheck' target
2020-06-04 8:39 [Tarantool-patches] [PATCH 0/6] Add static analysis of Lua code in src/ and extra/ with luacheck sergeyb
2020-06-04 8:39 ` [Tarantool-patches] [PATCH 1/6] Add initial luacheck config sergeyb
@ 2020-06-04 8:39 ` sergeyb
2020-06-04 8:39 ` [Tarantool-patches] [PATCH 3/6] gitlab-ci: enable static analysis with luacheck sergeyb
` (8 subsequent siblings)
10 siblings, 0 replies; 41+ messages in thread
From: sergeyb @ 2020-06-04 8:39 UTC (permalink / raw)
To: tarantool-patches, imun, v.shpilevoy; +Cc: alexander.turenko
From: Sergey Bronnikov <sergeyb@tarantool.org>
Part of #4681
Reviewed-by: Igor Munkin <imun@tarantool.org>
---
CMakeLists.txt | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/CMakeLists.txt b/CMakeLists.txt
index 1d80b6806..13ff7ed84 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -23,6 +23,7 @@ find_program(CAT cat)
find_program(GIT git)
find_program(LD ld)
find_program(CTAGS ctags)
+find_program(LUACHECK luacheck ENV PATH)
# Define PACKAGE macro in tarantool/config.h
set(PACKAGE "Tarantool" CACHE STRING "Package name.")
@@ -151,6 +152,16 @@ add_custom_target(tags COMMAND ${CTAGS} -R ${tagsExclude} -f tags
WORKING_DIRECTORY ${CMAKE_SOURCE_DIR})
add_custom_target(ctags DEPENDS tags)
+#
+# Enable 'make luacheck' target.
+#
+
+add_custom_target(luacheck)
+add_custom_command(TARGET luacheck
+ COMMAND ${LUACHECK} --codes --config "${PROJECT_SOURCE_DIR}/.luacheckrc" "${PROJECT_SOURCE_DIR}"
+ COMMENT "Perform static analysis of Lua code"
+)
+
#
# Get version
#
--
2.23.0
^ permalink raw reply [flat|nested] 41+ messages in thread
* [Tarantool-patches] [PATCH 3/6] gitlab-ci: enable static analysis with luacheck
2020-06-04 8:39 [Tarantool-patches] [PATCH 0/6] Add static analysis of Lua code in src/ and extra/ with luacheck sergeyb
2020-06-04 8:39 ` [Tarantool-patches] [PATCH 1/6] Add initial luacheck config sergeyb
2020-06-04 8:39 ` [Tarantool-patches] [PATCH 2/6] build: enable 'make luacheck' target sergeyb
@ 2020-06-04 8:39 ` sergeyb
2020-06-04 8:39 ` [Tarantool-patches] [PATCH 4/6] Fix luacheck warnings in extra/dist/ sergeyb
` (7 subsequent siblings)
10 siblings, 0 replies; 41+ messages in thread
From: sergeyb @ 2020-06-04 8:39 UTC (permalink / raw)
To: tarantool-patches, imun, v.shpilevoy; +Cc: alexander.turenko
From: Sergey Bronnikov <sergeyb@tarantool.org>
Part of #4681
---
.gitlab-ci.yml | 11 +++++++++++
.travis.mk | 23 ++++++++++++++++++++++-
2 files changed, 33 insertions(+), 1 deletion(-)
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 7705631dd..041653dae 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -1,4 +1,5 @@
stages:
+ - static_analysis
- test
- perf
- cleanup
@@ -107,6 +108,16 @@ variables:
script:
- ${GITLAB_MAKE} perf_cleanup
+# Static Analysis
+
+luacheck:
+ <<: *docker_test_definition
+ stage: static_analysis
+ tags:
+ - deploy_test
+ script:
+ - ${GITLAB_MAKE} test_debian_docker_luacheck
+
# Tests
release:
diff --git a/.travis.mk b/.travis.mk
index 063537f25..aaceaab34 100644
--- a/.travis.mk
+++ b/.travis.mk
@@ -3,9 +3,11 @@
#
DOCKER_IMAGE?=packpack/packpack:debian-stretch
+DOCKER_IMAGE_TARANTOOL="registry.gitlab.com/tarantool/tarantool/testing/debian-stretch:latest"
TEST_RUN_EXTRA_PARAMS?=
MAX_FILES?=65534
MAX_PROC?=2500
+OOS_SRC_PATH="/source"
all: package
@@ -76,8 +78,10 @@ deps_buster_clang_8: deps_debian
# Release
-build_debian:
+configure_debian:
cmake . -DCMAKE_BUILD_TYPE=RelWithDebInfo -DENABLE_WERROR=ON ${CMAKE_EXTRA_PARAMS}
+
+build_debian: configure_debian
make -j
test_debian_no_deps: build_debian
@@ -146,6 +150,23 @@ test_static_build: deps_debian_static
test_static_docker_build:
docker build --no-cache --network=host --build-arg RUN_TESTS=ON -f Dockerfile.staticbuild .
+# ###################
+# Static Analysis
+# ###################
+
+test_debian_docker_luacheck:
+ docker run -w ${OOS_SRC_PATH} -v ${PWD}:${OOS_SRC_PATH} --privileged \
+ --cap-add=sys_nice --network=host -i ${DOCKER_IMAGE_TARANTOOL} \
+ make -f .travis.mk test_debian_luacheck
+
+test_debian_install_luacheck:
+ apt update -y
+ apt install -y lua5.1 luarocks
+ luarocks install luacheck
+
+test_debian_luacheck: test_debian_install_luacheck configure_debian
+ make luacheck
+
#######
# OSX #
#######
--
2.23.0
^ permalink raw reply [flat|nested] 41+ messages in thread
* [Tarantool-patches] [PATCH 4/6] Fix luacheck warnings in extra/dist/
2020-06-04 8:39 [Tarantool-patches] [PATCH 0/6] Add static analysis of Lua code in src/ and extra/ with luacheck sergeyb
` (2 preceding siblings ...)
2020-06-04 8:39 ` [Tarantool-patches] [PATCH 3/6] gitlab-ci: enable static analysis with luacheck sergeyb
@ 2020-06-04 8:39 ` sergeyb
2020-06-06 18:02 ` Vladislav Shpilevoy
2020-06-19 23:15 ` Vladislav Shpilevoy
2020-06-04 8:39 ` [Tarantool-patches] [PATCH 5/6] Fix luacheck warnings in src/lua/ sergeyb
` (6 subsequent siblings)
10 siblings, 2 replies; 41+ messages in thread
From: sergeyb @ 2020-06-04 8:39 UTC (permalink / raw)
To: tarantool-patches, imun, v.shpilevoy; +Cc: alexander.turenko
From: Sergey Bronnikov <sergeyb@tarantool.org>
Part of #4681
Reviewed-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Reviewed-by: Igor Munkin <imun@tarantool.org>
Co-authored-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Co-authored-by: Igor Munkin <imun@tarantool.org>
---
.luacheckrc | 15 ++++++++++++++-
| 25 +++++++------------------
2 files changed, 21 insertions(+), 19 deletions(-)
diff --git a/.luacheckrc b/.luacheckrc
index e6edf8617..b917eb927 100644
--- a/.luacheckrc
+++ b/.luacheckrc
@@ -1,12 +1,19 @@
std = "luajit"
+globals = {"box", "_TARANTOOL"}
+ignore = {
+ "212/self", -- Unused argument <self>.
+ "411", -- Redefining a local variable.
+ "431", -- Shadowing an upvalue.
+}
+
include_files = {
"**/*.lua",
+ "extra/dist/tarantoolctl.in",
}
exclude_files = {
"build/**/*.lua",
- "extra/**/*.lua",
"src/**/*.lua",
"test-run/**/*.lua",
"test/**/*.lua",
@@ -14,3 +21,9 @@ exclude_files = {
".rocks/**/*.lua",
".git/**/*.lua",
}
+
+files["extra/dist/tarantoolctl.in"] = {
+ ignore = {
+ "122", -- https://github.com/tarantool/tarantool/issues/4929
+ },
+}
--git a/extra/dist/tarantoolctl.in b/extra/dist/tarantoolctl.in
index a98c61b52..90caf58ad 100755
--- a/extra/dist/tarantoolctl.in
+++ b/extra/dist/tarantoolctl.in
@@ -63,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'
udir = udir and fio.stat(udir) and udir or nil
-- or home dir configuration
local homedir = os.getenv('HOME')
@@ -789,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:")
@@ -847,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
@@ -861,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)
@@ -874,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)
@@ -900,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)
@@ -921,22 +919,13 @@ local function play()
remote:close()
end
-local function find_arg(name_arg)
- for i, key in ipairs(arg) do
- if key:find(name_arg) ~= nil then
- return key
- end
- end
- return nil
-end
-
local function rocks()
local cfg = require("luarocks.core.cfg")
cfg.init()
local util = require("luarocks.util")
local command_line = require("luarocks.cmd")
-- Tweak help messages
- util.see_help = function(command, program)
+ util.see_help = function(command, program) -- luacheck: no unused args
-- TODO: print extended help message here
return "See Tarantool documentation for help."
end
@@ -1215,7 +1204,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
--
2.23.0
^ permalink raw reply [flat|nested] 41+ messages in thread
* [Tarantool-patches] [PATCH 5/6] Fix luacheck warnings in src/lua/
2020-06-04 8:39 [Tarantool-patches] [PATCH 0/6] Add static analysis of Lua code in src/ and extra/ with luacheck sergeyb
` (3 preceding siblings ...)
2020-06-04 8:39 ` [Tarantool-patches] [PATCH 4/6] Fix luacheck warnings in extra/dist/ sergeyb
@ 2020-06-04 8:39 ` sergeyb
2020-06-06 18:03 ` Vladislav Shpilevoy
` (2 more replies)
2020-06-04 8:39 ` [Tarantool-patches] [PATCH 6/6] Fix luacheck warnings in src/box/lua/ sergeyb
` (5 subsequent siblings)
10 siblings, 3 replies; 41+ messages in thread
From: sergeyb @ 2020-06-04 8:39 UTC (permalink / raw)
To: tarantool-patches, imun, v.shpilevoy; +Cc: alexander.turenko
From: Sergey Bronnikov <sergeyb@tarantool.org>
Part of #4681
Reviewed-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Reviewed-by: Igor Munkin <imun@tarantool.org>
Co-authored-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Co-authored-by: Igor Munkin <imun@tarantool.org>
---
.luacheckrc | 28 ++++++++++++++++++++++++----
src/lua/argparse.lua | 3 ++-
src/lua/buffer.lua | 4 ++--
src/lua/clock.lua | 2 +-
src/lua/crypto.lua | 4 ++--
src/lua/csv.lua | 5 ++---
src/lua/digest.lua | 2 +-
src/lua/env.lua | 2 +-
src/lua/fiber.lua | 4 ++--
src/lua/fio.lua | 30 ++++++++++++++----------------
src/lua/help.lua | 7 ++-----
src/lua/httpc.lua | 3 ---
src/lua/init.lua | 4 ++--
src/lua/msgpackffi.lua | 22 +++++++++-------------
src/lua/socket.lua | 16 ++++++++--------
src/lua/string.lua | 1 -
src/lua/swim.lua | 2 +-
src/lua/tap.lua | 4 ----
src/lua/trigger.lua | 3 ---
19 files changed, 73 insertions(+), 73 deletions(-)
diff --git a/.luacheckrc b/.luacheckrc
index b917eb927..fb8b9dfb3 100644
--- a/.luacheckrc
+++ b/.luacheckrc
@@ -1,9 +1,14 @@
std = "luajit"
globals = {"box", "_TARANTOOL"}
ignore = {
- "212/self", -- Unused argument <self>.
- "411", -- Redefining a local variable.
- "431", -- Shadowing an upvalue.
+ "143/debug", -- Accessing an undefined field of a global variable <debug>.
+ "143/string", -- Accessing an undefined field of a global variable <string>.
+ "143/table", -- Accessing an undefined field of a global variable <table>.
+ "212/self", -- Unused argument <self>.
+ "411", -- Redefining a local variable.
+ "421", -- Shadowing a local variable.
+ "431", -- Shadowing an upvalue.
+ "432", -- Shadowing an upvalue argument.
}
@@ -14,7 +19,7 @@ include_files = {
exclude_files = {
"build/**/*.lua",
- "src/**/*.lua",
+ "src/box/**/*.lua",
"test-run/**/*.lua",
"test/**/*.lua",
"third_party/**/*.lua",
@@ -27,3 +32,18 @@ files["extra/dist/tarantoolctl.in"] = {
"122", -- https://github.com/tarantool/tarantool/issues/4929
},
}
+files["src/lua/help.lua"] = {
+ globals = {"help", "tutorial"}, -- globals defined for interactive mode.
+}
+files["src/lua/init.lua"] = {
+ globals = {"dostring"}, -- miscellaneous global function definition.
+ ignore = {
+ "122/os", -- set tarantool specific behaviour for os.exit.
+ "142/package", -- add custom functions into Lua package namespace.
+ },
+}
+files["src/lua/swim.lua"] = {
+ ignore = {
+ "212/m", -- respect swim module code style.
+ },
+}
diff --git a/src/lua/argparse.lua b/src/lua/argparse.lua
index faa0ae130..6c8f10fc1 100644
--- a/src/lua/argparse.lua
+++ b/src/lua/argparse.lua
@@ -91,7 +91,8 @@ local function convert_parameter(name, convert_from, convert_to)
end
local function parameters_parse(t_in, options)
- local t_out, t_in = {}, t_in or {}
+ local t_out = {}
+ t_in = t_in or {}
-- Prepare a lookup table for options. An option name -> a
-- type name to convert a parameter into or true (which means
diff --git a/src/lua/buffer.lua b/src/lua/buffer.lua
index 9aac82b39..00846bb20 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(self)
return '<ibuf>'
end
local ibuf_mt = {
@@ -193,7 +193,7 @@ local ibuf_mt = {
ffi.metatype(ibuf_t, ibuf_mt);
-local function ibuf_new(arg, arg2)
+local function ibuf_new(arg)
local buf = ffi.new(ibuf_t)
local slabc = builtin.tarantool_lua_slab_cache()
builtin.ibuf_create(buf, slabc, READAHEAD)
diff --git a/src/lua/clock.lua b/src/lua/clock.lua
index 60c78663c..fee43ccde 100644
--- a/src/lua/clock.lua
+++ b/src/lua/clock.lua
@@ -33,7 +33,7 @@ clock.bench = function(fun, ...)
overhead = clock.proc() - overhead
local start_time = clock.proc()
local res = {0, fun(...)}
- res[1] = clock.proc() - start_time - overhead, res
+ res[1] = clock.proc() - start_time - overhead
return res
end
diff --git a/src/lua/crypto.lua b/src/lua/crypto.lua
index bf3064c70..f31708e29 100644
--- a/src/lua/crypto.lua
+++ b/src/lua/crypto.lua
@@ -75,7 +75,7 @@ local function openssl_err_str()
end
local digests = {}
-for class, name in pairs({
+for class, _ in pairs({
md2 = 'MD2', md4 = 'MD4', md5 = 'MD5',
sha1 = 'SHA1', sha224 = 'SHA224',
sha256 = 'SHA256', sha384 = 'SHA384', sha512 = 'SHA512',
@@ -428,7 +428,7 @@ local public_methods = {
}
local module_mt = {
- __serialize = function(s)
+ __serialize = function(self)
return public_methods
end,
__index = public_methods
diff --git a/src/lua/csv.lua b/src/lua/csv.lua
index 7dff2f213..de6726bb7 100644
--- a/src/lua/csv.lua
+++ b/src/lua/csv.lua
@@ -188,10 +188,10 @@ module.dump = function(t, opts, writable)
if type(writable) == 'nil' then
result_table = {}
end
- for k, line in pairs(t) do
+ for _, line in pairs(t) do
local first = true
local output_tuple = {}
- for k2, field in pairs(line) do
+ for _, field in pairs(line) do
local strf = tostring(field)
local buf_new_size = (strf:len() + 1) * 2
if buf_new_size > bufsz then
@@ -214,7 +214,6 @@ module.dump = function(t, opts, writable)
else
writable:write(table.concat(output_tuple))
end
- output_tuple = {}
end
ffi.C.csv_destroy(csv)
csv.realloc(buf, 0)
diff --git a/src/lua/digest.lua b/src/lua/digest.lua
index 6ed91cfa2..7f1aea8d0 100644
--- a/src/lua/digest.lua
+++ b/src/lua/digest.lua
@@ -246,7 +246,7 @@ local m = {
end
}
-for digest, name in pairs(digest_shortcuts) do
+for digest, _ in pairs(digest_shortcuts) do
m[digest] = function (str)
return crypto.digest[digest](str)
end
diff --git a/src/lua/env.lua b/src/lua/env.lua
index dd1616a84..a31b7098f 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
if value ~= nil then
rv = ffi.C.setenv(key, value, 1)
else
diff --git a/src/lua/fiber.lua b/src/lua/fiber.lua
index 89c17f63d..b14117714 100644
--- a/src/lua/fiber.lua
+++ b/src/lua/fiber.lua
@@ -39,8 +39,8 @@ local stall = fiber.stall
fiber.stall = nil
local worker_next_task = nil
-local worker_last_task = nil
-local worker_fiber = nil
+local worker_last_task
+local worker_fiber
--
-- Worker is a singleton fiber for not urgent delayed execution of
diff --git a/src/lua/fio.lua b/src/lua/fio.lua
index 83fddaa0a..b31311b7b 100644
--- a/src/lua/fio.lua
+++ b/src/lua/fio.lua
@@ -329,7 +329,7 @@ fio.abspath = function(path)
error("Usage: fio.abspath(path)")
end
path = path
- local joined_path = ''
+ local joined_path
local path_tab = {}
if string.sub(path, 1, 1) == '/' then
joined_path = path
@@ -366,7 +366,7 @@ fio.listdir = function(path)
return t
end
local names = string.split(str, "\n")
- for i, name in ipairs(names) do
+ for _, name in ipairs(names) do
table.insert(t, name)
end
return t
@@ -378,15 +378,15 @@ fio.mktree = function(path, mode)
end
path = fio.abspath(path)
- local path = string.gsub(path, '^/', '')
+ path = string.gsub(path, '^/', '')
local dirs = string.split(path, "/")
local current_dir = "/"
- for i, dir in ipairs(dirs) do
+ for _, dir in ipairs(dirs) do
current_dir = fio.pathjoin(current_dir, dir)
local stat = fio.stat(current_dir)
if stat == nil then
- local st, err = fio.mkdir(current_dir, mode)
+ local _, err = fio.mkdir(current_dir, mode)
-- fio.stat() and fio.mkdir() above are separate calls
-- and a file system may be changed between them. So
-- if the error here is due to an existing directory,
@@ -407,27 +407,26 @@ fio.rmtree = function(path)
if type(path) ~= 'string' then
error("Usage: fio.rmtree(path)")
end
- local status, err
path = fio.abspath(path)
local ls, err = fio.listdir(path)
if err ~= nil then
return nil, err
end
- for i, f in ipairs(ls) do
+ for _, f in ipairs(ls) do
local tmppath = fio.pathjoin(path, f)
local st = fio.lstat(tmppath)
if st then
if st:is_dir() then
- st, err = fio.rmtree(tmppath)
+ _, err = fio.rmtree(tmppath)
else
- st, err = fio.unlink(tmppath)
+ _, err = fio.unlink(tmppath)
end
if err ~= nil then
return nil, err
end
end
end
- status, err = fio.rmdir(path)
+ local _, err = fio.rmdir(path)
if err ~= nil then
return false, string.format("failed to remove %s: %s", path, err)
end
@@ -453,7 +452,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)
@@ -467,22 +465,22 @@ fio.copytree = function(from, to)
end
-- create tree of destination
- status, reason = fio.mktree(to)
+ local _, reason = fio.mktree(to)
if reason ~= nil then
return false, reason
end
- for i, f in ipairs(ls) do
+ for _, f in ipairs(ls) do
local ffrom = fio.pathjoin(from, f)
local fto = fio.pathjoin(to, f)
local st = fio.lstat(ffrom)
if st and st:is_dir() then
- status, reason = fio.copytree(ffrom, fto)
+ _, reason = fio.copytree(ffrom, fto)
if reason ~= nil then
return false, reason
end
end
if st:is_reg() then
- status, reason = fio.copyfile(ffrom, fto)
+ _, reason = fio.copyfile(ffrom, fto)
if reason ~= nil then
return false, reason
end
@@ -492,7 +490,7 @@ fio.copytree = function(from, to)
if reason ~= nil then
return false, reason
end
- status, reason = fio.symlink(link_to, fto)
+ _, reason = fio.symlink(link_to, fto)
if reason ~= nil then
return false, "can't create symlink in place of existing file "..fto
end
diff --git a/src/lua/help.lua b/src/lua/help.lua
index 54ff1b5d0..4f024832d 100644
--- a/src/lua/help.lua
+++ b/src/lua/help.lua
@@ -11,10 +11,7 @@ help = { doc.help }
tutorial = {}
tutorial[1] = help[1]
-local help_function_data = {};
-local help_object_data = {}
-
-local function help_call(table, param)
+local function help_call()
return help
end
@@ -22,7 +19,7 @@ setmetatable(help, { __call = help_call })
local screen_id = 1;
-local function tutorial_call(table, action)
+local function tutorial_call(self, action)
if action == 'start' then
screen_id = 1;
elseif action == 'next' or action == 'more' then
diff --git a/src/lua/httpc.lua b/src/lua/httpc.lua
index 6381c6a1a..9336dcee0 100644
--- a/src/lua/httpc.lua
+++ b/src/lua/httpc.lua
@@ -29,8 +29,6 @@
-- SUCH DAMAGE.
--
-local fiber = require('fiber')
-
local driver = package.loaded.http.client
package.loaded.http = nil
@@ -112,7 +110,6 @@ local special_characters = {
[']'] = true,
['<'] = true,
['>'] = true,
- ['>'] = true,
['@'] = true,
[','] = true,
[';'] = true,
diff --git a/src/lua/init.lua b/src/lua/init.lua
index ff3e74c3c..aea7a7491 100644
--- a/src/lua/init.lua
+++ b/src/lua/init.lua
@@ -144,9 +144,9 @@ local function gen_search_func(path_fn, templates, need_traverse)
local searchpaths = {}
- for _, path in ipairs(paths) do
+ for _, p in ipairs(paths) do
for _, template in pairs(templates) do
- table.insert(searchpaths, fio.pathjoin(path, template))
+ table.insert(searchpaths, fio.pathjoin(p, template))
end
end
diff --git a/src/lua/msgpackffi.lua b/src/lua/msgpackffi.lua
index f01ffaef0..cb7ad5b88 100644
--- a/src/lua/msgpackffi.lua
+++ b/src/lua/msgpackffi.lua
@@ -302,10 +302,6 @@ local function encode(obj)
return r
end
-local function encode_ibuf(obj, ibuf)
- encode_r(ibuf, obj, 0)
-end
-
on_encode(ffi.typeof('uint8_t'), encode_int)
on_encode(ffi.typeof('uint16_t'), encode_int)
on_encode(ffi.typeof('uint32_t'), encode_int)
@@ -332,7 +328,6 @@ local decode_r
-- See similar constants in utils.cc
local DBL_INT_MAX = 1e14 - 1
-local DBL_INT_MIN = -1e14 + 1
local function decode_u8(data)
local num = ffi.cast(uint8_ptr_t, data[0])[0]
@@ -477,8 +472,7 @@ end
local function decode_array(data, size)
assert (type(size) == "number")
local arr = {}
- local i
- for i=1,size,1 do
+ for _ = 1, size do
table.insert(arr, decode_r(data))
end
if not msgpack.cfg.decode_save_metatables then
@@ -490,8 +484,7 @@ end
local function decode_map(data, size)
assert (type(size) == "number")
local map = {}
- local i
- for i=1,size,1 do
+ for _ = 1, size do
local key = decode_r(data);
local val = decode_r(data);
map[key] = val
@@ -504,11 +497,15 @@ end
local ext_decoder = {
-- MP_UNKNOWN_EXTENSION
- [0] = function(data, len) error("unsupported extension type") end,
+ [0] = function(data, len) error("unsupported extension type") end, -- luacheck: no unused args
-- MP_DECIMAL
[1] = function(data, len) local num = ffi.new("decimal_t") builtin.decimal_unpack(data, len, num) return num end,
-- MP_UUID
- [2] = function(data, len) local uuid = ffi.new("struct tt_uuid") builtin.uuid_unpack(data, len, uuid) return uuid end,
+ [2] = function(data, len)
+ local uuid = ffi.new("struct tt_uuid")
+ builtin.uuid_unpack(data, len, uuid)
+ return uuid
+ end,
}
local function decode_ext(data)
@@ -516,7 +513,6 @@ local function decode_ext(data)
-- mp_decode_extl and mp_decode_decimal
-- need type code
data[0] = data[0] - 1
- local old_data = data[0]
local len = builtin.mp_decode_extl(data, t)
local fun = ext_decoder[t[0]]
if type(fun) == 'function' then
@@ -603,7 +599,7 @@ local function check_offset(offset, len)
if offset == nil then
return 1
end
- local offset = ffi.cast('ptrdiff_t', offset)
+ offset = ffi.cast('ptrdiff_t', offset)
if offset < 1 or offset > len then
error(string.format("offset = %d is out of bounds [1..%d]",
tonumber(offset), len))
diff --git a/src/lua/socket.lua b/src/lua/socket.lua
index bbdef01e3..2a2c7a32c 100644
--- a/src/lua/socket.lua
+++ b/src/lua/socket.lua
@@ -177,7 +177,7 @@ local function get_iflags(table, flags)
if type(flags) ~= 'table' then
flags = { flags }
end
- for i, f in pairs(flags) do
+ for _, f in pairs(flags) do
if table[f] == nil then
return nil
end
@@ -665,7 +665,7 @@ local function check_delimiter(self, limit, eols)
end
local shortest
- for i, eol in ipairs(eols) do
+ for _, eol in ipairs(eols) do
local data = ffi.C.memmem(rbuf.rpos, rbuf:size(), eol, #eol)
if data ~= nil then
local len = ffi.cast('char *', data) - rbuf.rpos + #eol
@@ -1044,7 +1044,7 @@ local function tcp_connect(host, port, timeout)
boxerrno(0)
return s
end
- local timeout = timeout or TIMEOUT_INFINITY
+ timeout = timeout or TIMEOUT_INFINITY
local stop = fiber.clock() + timeout
local dns = getaddrinfo(host, port, timeout, { type = 'SOCK_STREAM',
protocol = 'tcp' })
@@ -1052,7 +1052,7 @@ local function tcp_connect(host, port, timeout)
boxerrno(boxerrno.EINVAL)
return nil
end
- for i, remote in pairs(dns) do
+ for _, remote in pairs(dns) do
timeout = stop - fiber.clock()
if timeout <= 0 then
boxerrno(boxerrno.ETIMEDOUT)
@@ -1319,7 +1319,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: no unused args
check_socket(self)
self.timeout = value
-- mode is effectively ignored
@@ -1475,7 +1475,7 @@ local function lsocket_tcp_receive(self, pattern, prefix)
local result = { prefix }
local deadline = fiber.clock() + (self.timeout or TIMEOUT_INFINITY)
repeat
- local data = read(self, LIMIT_INFINITY, timeout, check_infinity)
+ data = read(self, LIMIT_INFINITY, timeout, check_infinity)
if data == nil then
if not errno_is_transient[self._errno] then
return nil, socket_error(self)
@@ -1543,7 +1543,7 @@ lsocket_tcp_mt.__index.send = lsocket_tcp_send;
-- TCP Constructor and Shortcuts
--
-local function lsocket_tcp()
+local function lsocket_tcp(self)
local s = socket_new('AF_INET', 'SOCK_STREAM', 'tcp')
if not s then
return nil, socket_error(self)
@@ -1567,7 +1567,7 @@ local function lsocket_bind(host, port, backlog)
if host == nil or port == nil then
error("Usage: luasocket.bind(host, port [, backlog])")
end
- local function prepare(s) return backlog end
+ local function prepare(s) return backlog end -- luacheck: no unused args
local s = tcp_server_bind(host, port, prepare)
if not s then
return nil, boxerrno.strerror()
diff --git a/src/lua/string.lua b/src/lua/string.lua
index 6e12c59ae..d3a846645 100644
--- a/src/lua/string.lua
+++ b/src/lua/string.lua
@@ -233,7 +233,6 @@ local function string_startswith(inp, head, _start, _end)
return false
end
_start = _start - 1
- _end = _start + head_len - 1
return memcmp(c_char_ptr(inp) + _start, c_char_ptr(head), head_len) == 0
end
diff --git a/src/lua/swim.lua b/src/lua/swim.lua
index 0859915c9..cdf9d7df0 100644
--- a/src/lua/swim.lua
+++ b/src/lua/swim.lua
@@ -371,7 +371,7 @@ end
--
local function swim_member_payload_str(m)
local ptr = swim_check_member(m, 'member:payload_str()')
- local cdata, size = swim_member_payload_raw(ptr)
+ local _, size = swim_member_payload_raw(ptr)
if size > 0 then
return ffi.string(swim_member_payload_raw(ptr))
end
diff --git a/src/lua/tap.lua b/src/lua/tap.lua
index 94b080d5a..346724d84 100644
--- a/src/lua/tap.lua
+++ b/src/lua/tap.lua
@@ -53,7 +53,6 @@ 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")
extra.trace = traceback()
extra.filename = extra.trace[#extra.trace].filename
extra.line = extra.trace[#extra.trace].line
@@ -76,9 +75,6 @@ local function skip(test, message, extra)
ok(test, true, message.." # skip", extra)
end
-
-local nan = 0/0
-
local function cmpdeeply(got, expected, extra)
if type(expected) == "number" or type(got) == "number" then
extra.got = got
diff --git a/src/lua/trigger.lua b/src/lua/trigger.lua
index 76a582c47..1330ecdd4 100644
--- a/src/lua/trigger.lua
+++ b/src/lua/trigger.lua
@@ -1,7 +1,4 @@
local fun = require('fun')
-local log = require('log')
-
-local table_clear = require('table.clear')
--
-- Checks that argument is a callable, i.e. a function or a table
--
2.23.0
^ permalink raw reply [flat|nested] 41+ messages in thread
* [Tarantool-patches] [PATCH 6/6] Fix luacheck warnings in src/box/lua/
2020-06-04 8:39 [Tarantool-patches] [PATCH 0/6] Add static analysis of Lua code in src/ and extra/ with luacheck sergeyb
` (4 preceding siblings ...)
2020-06-04 8:39 ` [Tarantool-patches] [PATCH 5/6] Fix luacheck warnings in src/lua/ sergeyb
@ 2020-06-04 8:39 ` sergeyb
2020-06-06 18:03 ` Vladislav Shpilevoy
` (2 more replies)
2020-06-04 8:43 ` [Tarantool-patches] [PATCH 0/6] Add static analysis of Lua code in src/ and extra/ with luacheck Sergey Bronnikov
` (4 subsequent siblings)
10 siblings, 3 replies; 41+ messages in thread
From: sergeyb @ 2020-06-04 8:39 UTC (permalink / raw)
To: tarantool-patches, imun, v.shpilevoy; +Cc: alexander.turenko
From: Sergey Bronnikov <sergeyb@tarantool.org>
Part of #4681
Reviewed-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Co-authored-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
---
.luacheckrc | 10 +++++--
src/box/lua/console.lua | 10 +++----
src/box/lua/feedback_daemon.lua | 2 +-
src/box/lua/key_def.lua | 2 +-
src/box/lua/load_cfg.lua | 11 ++++---
src/box/lua/net_box.lua | 52 +++++++++++++--------------------
src/box/lua/schema.lua | 44 +++++++++++++---------------
src/box/lua/tuple.lua | 8 ++---
src/box/lua/upgrade.lua | 19 ++++++------
9 files changed, 73 insertions(+), 85 deletions(-)
diff --git a/.luacheckrc b/.luacheckrc
index fb8b9dfb3..9fd017629 100644
--- a/.luacheckrc
+++ b/.luacheckrc
@@ -1,11 +1,12 @@
std = "luajit"
-globals = {"box", "_TARANTOOL"}
+globals = {"box", "_TARANTOOL", "tonumber64"}
ignore = {
"143/debug", -- Accessing an undefined field of a global variable <debug>.
"143/string", -- Accessing an undefined field of a global variable <string>.
"143/table", -- Accessing an undefined field of a global variable <table>.
"212/self", -- Unused argument <self>.
"411", -- Redefining a local variable.
+ "412", -- Redefining an argument.
"421", -- Shadowing a local variable.
"431", -- Shadowing an upvalue.
"432", -- Shadowing an upvalue argument.
@@ -19,7 +20,7 @@ include_files = {
exclude_files = {
"build/**/*.lua",
- "src/box/**/*.lua",
+ "src/box/lua/serpent.lua", -- third-party source code
"test-run/**/*.lua",
"test/**/*.lua",
"third_party/**/*.lua",
@@ -47,3 +48,8 @@ files["src/lua/swim.lua"] = {
"212/m", -- respect swim module code style.
},
}
+files["src/box/lua/console.lua"] = {
+ ignore = {
+ "212", -- https://github.com/tarantool/tarantool/issues/5032
+ }
+}
diff --git a/src/box/lua/console.lua b/src/box/lua/console.lua
index 6ea27a393..5743d2481 100644
--- a/src/box/lua/console.lua
+++ b/src/box/lua/console.lua
@@ -17,7 +17,6 @@ ffi.cdef[[
console_set_output_format(enum output_format output_format);
]]
-local serpent = require('serpent')
local internal = require('console')
local session_internal = require('box.internal.session')
local fiber = require('fiber')
@@ -28,6 +27,7 @@ local urilib = require('uri')
local yaml = require('yaml')
local net_box = require('net.box')
local box_internal = require('box.internal')
+local help = require('help').help
local PUSH_TAG_HANDLE = '!push!'
@@ -313,7 +313,7 @@ local function set_param(storage, func, param, value)
end
local function help_wrapper(storage)
- return format(true, help()) -- defined in help.lua
+ return format(true, help())
end
local function quit(storage)
@@ -455,7 +455,7 @@ local text_connection_mt = {
-- Make sure it is exactly "\set output" command.
if operators[items[1]] == set_param and
param_handlers[items[2]] == set_output then
- local err, fmt, opts = parse_output(items[3])
+ local err, fmt = parse_output(items[3])
if not err then
self.fmt = fmt
self.eos = output_eos[fmt]
@@ -479,7 +479,7 @@ local text_connection_mt = {
break
end
if fmt == "yaml" then
- local handle, prefix = yaml.decode(rc, {tag_only = true})
+ local handle = yaml.decode(rc, {tag_only = true})
if not handle then
-- Can not fail - tags are encoded with no
-- user participation and are correct always.
@@ -859,7 +859,7 @@ local function listen(uri)
host = u.host
port = u.service or 3313
end
- local s, addr = socket.tcp_server(host, port, { handler = client_handler,
+ local s = socket.tcp_server(host, port, { handler = client_handler,
name = 'console'})
if not s then
error(string.format('failed to create server %s:%s: %s',
diff --git a/src/box/lua/feedback_daemon.lua b/src/box/lua/feedback_daemon.lua
index 95130d696..db74f558b 100644
--- a/src/box/lua/feedback_daemon.lua
+++ b/src/box/lua/feedback_daemon.lua
@@ -61,7 +61,7 @@ local function guard_loop(self)
self.fiber = fiber.create(feedback_loop, self)
log.verbose("%s restarted", PREFIX)
end
- local st, err = pcall(fiber.sleep, self.interval)
+ local st = pcall(fiber.sleep, self.interval)
if not st then
-- fiber was cancelled
break
diff --git a/src/box/lua/key_def.lua b/src/box/lua/key_def.lua
index 586005709..97905eebf 100644
--- a/src/box/lua/key_def.lua
+++ b/src/box/lua/key_def.lua
@@ -15,5 +15,5 @@ ffi.metatype(key_def_t, {
__index = function(self, key)
return methods[key]
end,
- __tostring = function(key_def) return "<struct key_def &>" end,
+ __tostring = function(self) return "<struct key_def &>" end,
})
diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
index 5d818addf..dd77f66bc 100644
--- a/src/box/lua/load_cfg.lua
+++ b/src/box/lua/load_cfg.lua
@@ -272,8 +272,8 @@ local dynamic_cfg = {
sql_cache_size = private.cfg_set_sql_cache_size,
}
-ifdef_feedback = nil
-ifdef_feedback_set_params = nil
+ifdef_feedback = nil -- luacheck: ignore
+ifdef_feedback_set_params = nil -- luacheck: ignore
--
-- For some options it is important in which order they are set.
@@ -431,7 +431,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
@@ -447,7 +447,6 @@ local function prepare_cfg(cfg, default_cfg, template_cfg, modify_cfg, prefix)
else
local good_types = string.gsub(template_cfg[k], ' ', '');
if (string.find(',' .. good_types .. ',', ',' .. type(v) .. ',') == nil) then
- good_types = string.gsub(good_types, ',', ', ');
box.error(box.error.CFG, readable_name, "should be one of types "..
template_cfg[k])
end
@@ -544,7 +543,7 @@ for k, v in pairs(box) do
end
setmetatable(box, {
- __index = function(table, index)
+ __index = function(table, index) -- luacheck: no unused args
error(debug.traceback("Please call box.cfg{} first"))
error("Please call box.cfg{} first")
end
@@ -568,7 +567,7 @@ local function load_cfg(cfg)
box_configured = nil
box.cfg = setmetatable(cfg,
{
- __newindex = function(table, index)
+ __newindex = function(table, index) -- luacheck: no unused args
error('Attempt to modify a read-only table')
end,
__call = locked(reload_cfg),
diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
index 9560bfdd4..f71744014 100644
--- a/src/box/lua/net_box.lua
+++ b/src/box/lua/net_box.lua
@@ -39,10 +39,6 @@ local IPROTO_STATUS_KEY = 0x00
local IPROTO_ERRNO_MASK = 0x7FFF
local IPROTO_SYNC_KEY = 0x01
local IPROTO_SCHEMA_VERSION_KEY = 0x05
-local IPROTO_METADATA_KEY = 0x32
-local IPROTO_SQL_INFO_KEY = 0x42
-local SQL_INFO_ROW_COUNT_KEY = 0
-local IPROTO_FIELD_NAME_KEY = 0
local IPROTO_DATA_KEY = 0x30
local IPROTO_ERROR_24 = 0x31
local IPROTO_ERROR = 0x52
@@ -68,18 +64,18 @@ error_unref(struct error *e);
-- 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
@@ -122,7 +118,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
@@ -187,7 +183,7 @@ local function next_id(id) return band(id + 1, 0x7FFFFFFF) end
-- @retval two non-nils A connected socket and a decoded greeting.
--
local function establish_connection(host, port, timeout)
- local timeout = timeout or DEFAULT_CONNECT_TIMEOUT
+ timeout = timeout or DEFAULT_CONNECT_TIMEOUT
local begin = fiber.clock()
local s = socket.tcp_connect(host, port, timeout)
if not s then
@@ -212,7 +208,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
@@ -253,7 +249,7 @@ local function on_push_sync_default(...) end
--
-- The following events are delivered, with arguments:
--
--- 'state_changed', state, errno, error
+-- 'state_changed', state, error
-- 'handshake', greeting -> nil (accept) / errno, error (reject)
-- 'will_fetch_schema' -> true (approve) / false (skip fetch)
-- 'did_fetch_schema', schema_version, spaces, indices
@@ -449,7 +445,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)
state_cond:broadcast()
if state == 'error' or state == 'error_reconnect' or
state == 'closed' then
@@ -598,7 +594,7 @@ local function create_transport(host, port, user, password, callback,
-- Handle errors
requests[id] = nil
request.id = nil
- local map_len, key, skip
+ local map_len, key
map_len, body_rpos = decode_map_header(body_rpos, body_len)
-- Reserve for 2 keys and 2 array indexes. Because no
-- any guarantees how Lua will decide to save the
@@ -610,7 +606,7 @@ local function create_transport(host, port, user, password, callback,
if rdec then
body[key], body_rpos = rdec(body_rpos)
else
- skip, body_rpos = decode(body_rpos)
+ _, body_rpos = decode(body_rpos)
end
end
assert(body_end == body_rpos, "invalid xrow length")
@@ -689,7 +685,7 @@ local function create_transport(host, port, user, password, callback,
local function send_and_recv_iproto(timeout)
local data_len = recv_buf.wpos - recv_buf.rpos
- local required = 0
+ local required
if data_len < 5 then
required = 5
else
@@ -771,7 +767,6 @@ local function create_transport(host, port, user, password, callback,
end
console_sm = function(rid)
- local delim = '\n...\n'
local err, response = send_and_recv_console()
if err then
return error_sm(err, response)
@@ -795,13 +790,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_24])
end
set_state('fetch_schema')
@@ -852,8 +846,7 @@ local function create_transport(host, port, user, password, callback,
peer_has_vcollation = false
goto continue
end
- local body
- body, body_end = decode(body_rpos)
+ local body = decode(body_rpos)
return error_sm(E_NO_CONNECTION, body[IPROTO_ERROR_24])
end
if schema_version == nil then
@@ -862,8 +855,7 @@ local function create_transport(host, port, user, password, callback,
-- schema changed while fetching schema; restart loader
return iproto_schema_sm()
end
- local body
- body, body_end = decode(body_rpos)
+ local body = decode(body_rpos)
response[id] = body[IPROTO_DATA_KEY]
end
::continue::
@@ -880,14 +872,11 @@ local function create_transport(host, port, user, password, callback,
local err, hdr, body_rpos, body_end = send_and_recv_iproto()
if err then return error_sm(err, hdr) end
dispatch_response_iproto(hdr, body_rpos, body_end)
- local status = hdr[IPROTO_STATUS_KEY]
local response_schema_version = hdr[IPROTO_SCHEMA_VERSION_KEY]
if response_schema_version > 0 and
response_schema_version ~= schema_version then
-- schema_version has been changed - start to load a new version.
-- Sic: self.schema_version will be updated only after reload.
- local body
- body, body_end = decode(body_rpos)
set_state('fetch_schema')
return iproto_schema_sm(schema_version)
end
@@ -926,7 +915,7 @@ end
-- Now it is necessary to have a strong ref to callback somewhere or
-- it is GC-ed prematurely. We wrap stop() method, stashing the
-- ref in an upvalue (stop() performance doesn't matter much.)
-local create_transport = function(host, port, user, password, callback,
+local create_transport = function(host, port, user, password, callback, -- luacheck: ignore
connection, greeting)
local weak_refs = setmetatable({callback = callback}, {__mode = 'v'})
local function weak_callback(...)
@@ -1004,7 +993,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
@@ -1275,7 +1264,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")
@@ -1640,7 +1629,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 {}
@@ -1651,14 +1640,13 @@ this_module.self = {
rollback()
return box.error() -- re-throw
end
- local result
if obj ~= nil then
return handle_eval_result(pcall(proc, obj, unpack(args)))
else
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 {}
diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
index cdfbb65f7..4fe0ff8da 100644
--- a/src/box/lua/schema.lua
+++ b/src/box/lua/schema.lua
@@ -2,7 +2,6 @@
--
local ffi = require('ffi')
local msgpack = require('msgpack')
-local msgpackffi = require('msgpackffi')
local fun = require('fun')
local log = require('log')
local buffer = require('buffer')
@@ -212,7 +211,7 @@ local function revoke_object_privs(object_type, object_id)
local _vpriv = box.space[box.schema.VPRIV_ID]
local _priv = box.space[box.schema.PRIV_ID]
local privs = _vpriv.index.object:select{object_type, object_id}
- for k, tuple in pairs(privs) do
+ for _, tuple in pairs(privs) do
local uid = tuple.grantee
_priv:delete{uid, object_type, object_id}
end
@@ -262,7 +261,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
@@ -276,7 +275,6 @@ local function check_param_table(table, template)
local haystack = ',' .. good_types .. ','
local needle = ',' .. param_type(v) .. ','
if (string.find(haystack, needle) == nil) then
- good_types = string.gsub(good_types, ',', ', ')
box.error(box.error.ILLEGAL_PARAMS,
"options parameter '" .. k ..
"' should be one of types: " .. template[k])
@@ -584,9 +582,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
-- Path doesn't require resolve.
if type(path) == 'number' then
idx = path
@@ -846,7 +844,7 @@ local function space_sequence_alter_prepare(format, parts, options,
if id == nil then
box.error(box.error.NO_SUCH_SEQUENCE, new_sequence.id)
end
- local tuple = _space_sequence.index.sequence:select(id)[1]
+ tuple = _space_sequence.index.sequence:select(id)[1]
if tuple ~= nil and tuple.is_generated then
box.error(box.error.ALTER_SPACE, space_name,
"can not attach generated sequence")
@@ -1146,7 +1144,7 @@ box.schema.index.alter = function(space_id, index_id, options)
local can_update_field = {id = true, name = true, type = true }
local can_update = true
local cant_update_fields = ''
- for k,v in pairs(options) do
+ for k, _ in pairs(options) do
if not can_update_field[k] then
can_update = false
cant_update_fields = cant_update_fields .. ' ' .. k
@@ -1190,7 +1188,7 @@ box.schema.index.alter = function(space_id, index_id, options)
if options.type == nil then
options.type = tuple.type
end
- for k, t in pairs(index_options) do
+ for k, _ in pairs(index_options) do
if options[k] ~= nil then
index_opts[k] = options[k]
end
@@ -1233,12 +1231,12 @@ end
local iterator_t = ffi.typeof('struct iterator')
ffi.metatype(iterator_t, {
- __tostring = function(iterator)
+ __tostring = function(self)
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.
@@ -1272,7 +1270,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
@@ -1555,7 +1553,7 @@ end
base_index_mt.select_luac = function(index, key, opts)
check_index_arg(index, 'select')
- local key = keify(key)
+ key = keify(key)
local iterator, offset, limit = check_select_opts(opts, #key == 0)
return internal.select(index.space_id, index.id, iterator,
offset, limit, key)
@@ -1775,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)
@@ -2486,24 +2484,24 @@ 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}
- for k, tuple in pairs(spaces) do
+ for _, tuple in pairs(spaces) do
box.space[tuple.id]:drop()
end
local funcs = box.space[box.schema.VFUNC_ID].index.owner:select{uid}
- for k, tuple in pairs(funcs) do
+ for _, tuple in pairs(funcs) do
box.schema.func.drop(tuple.id)
end
-- if this is a role, revoke this role from whoever it was granted to
local grants = _vpriv.index.object:select{'role', uid}
- for k, tuple in pairs(grants) do
+ for _, tuple in pairs(grants) do
revoke(tuple.grantee, tuple.grantee, uid)
end
local sequences = box.space[box.schema.VSEQUENCE_ID].index.owner:select{uid}
- for k, tuple in pairs(sequences) do
+ for _, tuple in pairs(sequences) do
box.schema.sequence.drop(tuple.id)
end
-- xxx: hack, we have to revoke session and usage privileges
@@ -2515,7 +2513,7 @@ local function drop(uid, opts)
end
local privs = _vpriv.index.primary:select{uid}
- for k, tuple in pairs(privs) do
+ for _, tuple in pairs(privs) do
-- we need an additional box.session.su() here, because of
-- unnecessary check for privilege PRIV_REVOKE in priv_def_check()
box.session.su("admin", revoke, uid, uid, tuple.privilege,
@@ -2565,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)
@@ -2681,7 +2679,7 @@ box.once = function(key, func, ...)
box.error(box.error.ILLEGAL_PARAMS, "Usage: box.once(key, func, ...)")
end
- local key = "once"..key
+ key = "once"..key
if box.space._schema:get{key} ~= nil then
return
end
diff --git a/src/box/lua/tuple.lua b/src/box/lua/tuple.lua
index f97aa1579..87f53258b 100644
--- a/src/box/lua/tuple.lua
+++ b/src/box/lua/tuple.lua
@@ -1,7 +1,6 @@
-- tuple.lua (internal file)
local ffi = require('ffi')
-local yaml = require('yaml')
local msgpackffi = require('msgpackffi')
local fun = require('fun')
local buffer = require('buffer')
@@ -81,7 +80,6 @@ local tuple_encode = function(obj)
encode_r(tmpbuf, obj, 1)
elseif type(obj) == "table" then
encode_array(tmpbuf, #obj)
- local i
for i = 1, #obj, 1 do
encode_r(tmpbuf, obj[i], 1)
end
@@ -232,7 +230,7 @@ local function tuple_update(tuple, expr)
error("Usage: tuple:update({ { op, field, arg}+ })")
end
local pexpr, pexpr_end = tuple_encode(expr)
- local tuple = builtin.box_tuple_update(tuple, pexpr, pexpr_end)
+ tuple = builtin.box_tuple_update(tuple, pexpr, pexpr_end)
if tuple == nil then
return box.error()
end
@@ -245,7 +243,7 @@ local function tuple_upsert(tuple, expr)
error("Usage: tuple:upsert({ { op, field, arg}+ })")
end
local pexpr, pexpr_end = tuple_encode(expr)
- local tuple = builtin.box_tuple_upsert(tuple, pexpr, pexpr_end)
+ tuple = builtin.box_tuple_upsert(tuple, pexpr, pexpr_end)
if tuple == nil then
return box.error()
end
@@ -339,7 +337,7 @@ ffi.metatype(tuple_t, {
ffi.metatype(tuple_iterator_t, {
__call = tuple_iterator_next;
- __tostring = function(it) return "<tuple iterator>" end;
+ __tostring = function(self) return "<tuple iterator>" end;
})
-- Free methods, which are not needed anymore.
diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
index 075cc236e..208b53f15 100644
--- a/src/box/lua/upgrade.lua
+++ b/src/box/lua/upgrade.lua
@@ -52,10 +52,9 @@ end
local function truncate(space)
local pk = space.index[0]
while pk:len() > 0 do
- local state, t
- for state, t in pk:pairs() do
+ for _, t in pk:pairs() do
local key = {}
- for _k2, parts in ipairs(pk.parts) do
+ for _, parts in ipairs(pk.parts) do
table.insert(key, t[parts.fieldno])
end
space:delete(key)
@@ -173,7 +172,7 @@ local function initial_1_7_5()
format[5] = {name='field_count', type='unsigned'}
format[6] = {name='flags', type='map'}
format[7] = {name='format', type='array'}
- local def = _space:insert{_space.id, ADMIN, '_space', 'memtx', 0, MAP, format}
+ _space:insert{_space.id, ADMIN, '_space', 'memtx', 0, MAP, format}
-- space name is unique
log.info("create index primary on _space")
_index:insert{_space.id, 0, 'primary', 'tree', { unique = true }, {{0, 'unsigned'}}}
@@ -194,7 +193,7 @@ local function initial_1_7_5()
format[4] = {name = 'type', type = 'string'}
format[5] = {name = 'opts', type = 'map'}
format[6] = {name = 'parts', type = 'array'}
- def = _space:insert{_index.id, ADMIN, '_index', 'memtx', 0, MAP, format}
+ _space:insert{_index.id, ADMIN, '_index', 'memtx', 0, MAP, format}
-- index name is unique within a space
log.info("create index primary on _index")
_index:insert{_index.id, 0, 'primary', 'tree', {unique = true}, {{0, 'unsigned'}, {1, 'unsigned'}}}
@@ -211,7 +210,7 @@ local function initial_1_7_5()
format[2] = {name='owner', type='unsigned'}
format[3] = {name='name', type='string'}
format[4] = {name='setuid', type='unsigned'}
- def = _space:insert{_func.id, ADMIN, '_func', 'memtx', 0, MAP, format}
+ _space:insert{_func.id, ADMIN, '_func', 'memtx', 0, MAP, format}
-- function name and id are unique
log.info("create index _func:primary")
_index:insert{_func.id, 0, 'primary', 'tree', {unique = true}, {{0, 'unsigned'}}}
@@ -231,7 +230,7 @@ local function initial_1_7_5()
format[3] = {name='name', type='string'}
format[4] = {name='type', type='string'}
format[5] = {name='auth', type='map'}
- def = _space:insert{_user.id, ADMIN, '_user', 'memtx', 0, MAP, format}
+ _space:insert{_user.id, ADMIN, '_user', 'memtx', 0, MAP, format}
-- user name and id are unique
log.info("create index _func:primary")
_index:insert{_user.id, 0, 'primary', 'tree', {unique = true}, {{0, 'unsigned'}}}
@@ -251,7 +250,7 @@ local function initial_1_7_5()
format[3] = {name='object_type', type='string'}
format[4] = {name='object_id', type='unsigned'}
format[5] = {name='privilege', type='unsigned'}
- def = _space:insert{_priv.id, ADMIN, '_priv', 'memtx', 0, MAP, format}
+ _space:insert{_priv.id, ADMIN, '_priv', 'memtx', 0, MAP, format}
-- user id, object type and object id are unique
log.info("create index primary on _priv")
_index:insert{_priv.id, 0, 'primary', 'tree', {unique = true}, {{1, 'unsigned'}, {2, 'string'}, {3, 'unsigned'}}}
@@ -580,7 +579,7 @@ local function upgrade_to_2_1_0()
-- Now, abscent field means NULL, so we can safely set second
-- field in format, marking it nullable.
log.info("Add nullable value field to space _schema")
- local format = {}
+ format = {}
format[1] = {type='string', name='key'}
format[2] = {type='any', name='value', is_nullable=true}
box.space._schema:format(format)
@@ -734,7 +733,7 @@ local function upgrade_collation_to_2_1_3()
local id = 4
for _, collation in ipairs(coll_lst) do
- for i, strength in ipairs(coll_strengths) do
+ for _, strength in ipairs(coll_strengths) do
local coll_name = 'unicode_' .. collation.name .. "_" .. strength.s
log.info("creating collation %s", coll_name)
box.space._collation:replace{id, coll_name, ADMIN, "ICU", collation.loc_str, strength.opt }
--
2.23.0
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Tarantool-patches] [PATCH 0/6] Add static analysis of Lua code in src/ and extra/ with luacheck
2020-06-04 8:39 [Tarantool-patches] [PATCH 0/6] Add static analysis of Lua code in src/ and extra/ with luacheck sergeyb
` (5 preceding siblings ...)
2020-06-04 8:39 ` [Tarantool-patches] [PATCH 6/6] Fix luacheck warnings in src/box/lua/ sergeyb
@ 2020-06-04 8:43 ` Sergey Bronnikov
2020-06-19 23:15 ` Vladislav Shpilevoy
2020-06-04 9:04 ` Igor Munkin
` (3 subsequent siblings)
10 siblings, 1 reply; 41+ messages in thread
From: Sergey Bronnikov @ 2020-06-04 8:43 UTC (permalink / raw)
To: tarantool-patches, imun, v.shpilevoy; +Cc: alexander.turenko
GH issue: https://github.com/tarantool/tarantool/issues/4681
GitHub branch: https://github.com/tarantool/tarantool/tree/ligurio/gh-4681-fix-luacheck-warnings-src
GitLab CI: https://gitlab.com/tarantool/tarantool/-/pipelines/152745880
On 11:39 Thu 04 Jun , sergeyb@tarantool.org wrote:
> From: Sergey Bronnikov <sergeyb@tarantool.org>
>
> Sergey Bronnikov (6):
> Add initial luacheck config
> build: enable 'make luacheck' target
> gitlab-ci: enable static analysis with luacheck
> Fix luacheck warnings in extra/dist/
> Fix luacheck warnings in src/lua/
> Fix luacheck warnings in src/box/lua/
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Tarantool-patches] [PATCH 0/6] Add static analysis of Lua code in src/ and extra/ with luacheck
2020-06-04 8:39 [Tarantool-patches] [PATCH 0/6] Add static analysis of Lua code in src/ and extra/ with luacheck sergeyb
` (6 preceding siblings ...)
2020-06-04 8:43 ` [Tarantool-patches] [PATCH 0/6] Add static analysis of Lua code in src/ and extra/ with luacheck Sergey Bronnikov
@ 2020-06-04 9:04 ` Igor Munkin
2020-06-04 11:17 ` Sergey Bronnikov
` (2 subsequent siblings)
10 siblings, 0 replies; 41+ messages in thread
From: Igor Munkin @ 2020-06-04 9:04 UTC (permalink / raw)
To: sergeyb; +Cc: tarantool-patches, v.shpilevoy, alexander.turenko
Sergey,
Thanks for the series! Please provide a ChangeLog entry.
Since I was involved in preparing this series, my review is not
sufficient anymore.
Vlad, Sasha, could you please take look on the patchset? I guess
everything we discussed[1] is fixed now.
On 04.06.20, sergeyb@tarantool.org wrote:
> From: Sergey Bronnikov <sergeyb@tarantool.org>
>
> Sergey Bronnikov (6):
> Add initial luacheck config
> build: enable 'make luacheck' target
> gitlab-ci: enable static analysis with luacheck
> Fix luacheck warnings in extra/dist/
> Fix luacheck warnings in src/lua/
> Fix luacheck warnings in src/box/lua/
>
> .gitlab-ci.yml | 11 +++++++
> .luacheckrc | 55 +++++++++++++++++++++++++++++++++
> .travis.mk | 23 +++++++++++++-
> CMakeLists.txt | 11 +++++++
> extra/dist/tarantoolctl.in | 25 +++++----------
> src/box/lua/console.lua | 10 +++---
> src/box/lua/feedback_daemon.lua | 2 +-
> src/box/lua/key_def.lua | 2 +-
> src/box/lua/load_cfg.lua | 11 +++----
> src/box/lua/net_box.lua | 52 ++++++++++++-------------------
> src/box/lua/schema.lua | 44 +++++++++++++-------------
> src/box/lua/tuple.lua | 8 ++---
> src/box/lua/upgrade.lua | 19 ++++++------
> src/lua/argparse.lua | 3 +-
> src/lua/buffer.lua | 4 +--
> src/lua/clock.lua | 2 +-
> src/lua/crypto.lua | 4 +--
> src/lua/csv.lua | 5 ++-
> src/lua/digest.lua | 2 +-
> src/lua/env.lua | 2 +-
> src/lua/fiber.lua | 4 +--
> src/lua/fio.lua | 30 +++++++++---------
> src/lua/help.lua | 7 ++---
> src/lua/httpc.lua | 3 --
> src/lua/init.lua | 4 +--
> src/lua/msgpackffi.lua | 22 ++++++-------
> src/lua/socket.lua | 16 +++++-----
> src/lua/string.lua | 1 -
> src/lua/swim.lua | 2 +-
> src/lua/tap.lua | 4 ---
> src/lua/trigger.lua | 3 --
> 31 files changed, 220 insertions(+), 171 deletions(-)
> create mode 100644 .luacheckrc
>
> --
> 2.23.0
[1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-June/017370.html
--
Best regards,
IM
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Tarantool-patches] [PATCH 0/6] Add static analysis of Lua code in src/ and extra/ with luacheck
2020-06-04 8:39 [Tarantool-patches] [PATCH 0/6] Add static analysis of Lua code in src/ and extra/ with luacheck sergeyb
` (7 preceding siblings ...)
2020-06-04 9:04 ` Igor Munkin
@ 2020-06-04 11:17 ` Sergey Bronnikov
2020-07-14 22:49 ` Vladislav Shpilevoy
2020-07-15 9:51 ` Kirill Yukhin
10 siblings, 0 replies; 41+ messages in thread
From: Sergey Bronnikov @ 2020-06-04 11:17 UTC (permalink / raw)
To: tarantool-patches, imun, v.shpilevoy; +Cc: alexander.turenko
Changelog:
- enabled luacheck in continuous integration
- fixed warnings (two of them were real bugs!) found by luacheck in a
source code
On 11:39 Thu 04 Jun , sergeyb@tarantool.org wrote:
> From: Sergey Bronnikov <sergeyb@tarantool.org>
>
> Sergey Bronnikov (6):
> Add initial luacheck config
> build: enable 'make luacheck' target
> gitlab-ci: enable static analysis with luacheck
> Fix luacheck warnings in extra/dist/
> Fix luacheck warnings in src/lua/
> Fix luacheck warnings in src/box/lua/
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Tarantool-patches] [PATCH 1/6] Add initial luacheck config
2020-06-04 8:39 ` [Tarantool-patches] [PATCH 1/6] Add initial luacheck config sergeyb
@ 2020-06-06 18:02 ` Vladislav Shpilevoy
2020-06-09 16:16 ` Sergey Bronnikov
0 siblings, 1 reply; 41+ messages in thread
From: Vladislav Shpilevoy @ 2020-06-06 18:02 UTC (permalink / raw)
To: sergeyb, tarantool-patches, imun; +Cc: alexander.turenko
Hi! Thanks for the patch!
On 04/06/2020 10:39, sergeyb@tarantool.org wrote:
> From: Sergey Bronnikov <sergeyb@tarantool.org>
>
> config includes all files with Lua source code except:
Lets start sentences from capital letters.
> - third_party repositories
> - directories with diff-based tests
Seems like src/* is excluded. So nothing is checked, and the
commit message is wrong.
I see, that you turn on the checks in next commits, but then the
commit message should be changed.
> How-to check:
>
> $ luacheck --codes --config .luacheckrc .
>
> Part of #4681
>
> Reviewed-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
> Co-authored-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Tarantool-patches] [PATCH 4/6] Fix luacheck warnings in extra/dist/
2020-06-04 8:39 ` [Tarantool-patches] [PATCH 4/6] Fix luacheck warnings in extra/dist/ sergeyb
@ 2020-06-06 18:02 ` Vladislav Shpilevoy
2020-06-09 16:17 ` Sergey Bronnikov
2020-06-19 23:15 ` Vladislav Shpilevoy
1 sibling, 1 reply; 41+ messages in thread
From: Vladislav Shpilevoy @ 2020-06-06 18:02 UTC (permalink / raw)
To: sergeyb, tarantool-patches, imun; +Cc: alexander.turenko
Thanks for the patch!
On 04/06/2020 10:39, sergeyb@tarantool.org wrote:
> From: Sergey Bronnikov <sergeyb@tarantool.org>
>
> Part of #4681
>
> Reviewed-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
> Reviewed-by: Igor Munkin <imun@tarantool.org>
>
> Co-authored-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
> Co-authored-by: Igor Munkin <imun@tarantool.org>
> ---
> .luacheckrc | 15 ++++++++++++++-
> extra/dist/tarantoolctl.in | 25 +++++++------------------
> 2 files changed, 21 insertions(+), 19 deletions(-)
>
> diff --git a/.luacheckrc b/.luacheckrc
> index e6edf8617..b917eb927 100644
> --- a/.luacheckrc
> +++ b/.luacheckrc
> @@ -1,12 +1,19 @@
> std = "luajit"
> +globals = {"box", "_TARANTOOL"}
> +ignore = {
> + "212/self", -- Unused argument <self>.
> + "411", -- Redefining a local variable.
> + "431", -- Shadowing an upvalue.
> +}
> +
Unnecessary empty line after }.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Tarantool-patches] [PATCH 5/6] Fix luacheck warnings in src/lua/
2020-06-04 8:39 ` [Tarantool-patches] [PATCH 5/6] Fix luacheck warnings in src/lua/ sergeyb
@ 2020-06-06 18:03 ` Vladislav Shpilevoy
2020-06-11 12:00 ` Sergey Bronnikov
2020-06-11 17:22 ` Igor Munkin
2020-07-05 16:28 ` Vladislav Shpilevoy
2020-07-13 22:59 ` Vladislav Shpilevoy
2 siblings, 2 replies; 41+ messages in thread
From: Vladislav Shpilevoy @ 2020-06-06 18:03 UTC (permalink / raw)
To: sergeyb, tarantool-patches, imun; +Cc: alexander.turenko
Thanks for the patch!
See 11 comments below.
On 04/06/2020 10:39, sergeyb@tarantool.org wrote:
> From: Sergey Bronnikov <sergeyb@tarantool.org>
>
> Part of #4681
>
> Reviewed-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
> Reviewed-by: Igor Munkin <imun@tarantool.org>
>
> Co-authored-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
> Co-authored-by: Igor Munkin <imun@tarantool.org>
> ---
> .luacheckrc | 28 ++++++++++++++++++++++++----
> src/lua/argparse.lua | 3 ++-
> src/lua/buffer.lua | 4 ++--
> src/lua/clock.lua | 2 +-
> src/lua/crypto.lua | 4 ++--
> src/lua/csv.lua | 5 ++---
> src/lua/digest.lua | 2 +-
> src/lua/env.lua | 2 +-
> src/lua/fiber.lua | 4 ++--
> src/lua/fio.lua | 30 ++++++++++++++----------------
> src/lua/help.lua | 7 ++-----
> src/lua/httpc.lua | 3 ---
> src/lua/init.lua | 4 ++--
> src/lua/msgpackffi.lua | 22 +++++++++-------------
> src/lua/socket.lua | 16 ++++++++--------
> src/lua/string.lua | 1 -
> src/lua/swim.lua | 2 +-
> src/lua/tap.lua | 4 ----
> src/lua/trigger.lua | 3 ---
> 19 files changed, 73 insertions(+), 73 deletions(-)
>
> diff --git a/.luacheckrc b/.luacheckrc
> index b917eb927..fb8b9dfb3 100644
> --- a/.luacheckrc
> +++ b/.luacheckrc
> @@ -1,9 +1,14 @@
> std = "luajit"
> globals = {"box", "_TARANTOOL"}
> ignore = {
> - "212/self", -- Unused argument <self>.
> - "411", -- Redefining a local variable.
> - "431", -- Shadowing an upvalue.
> + "143/debug", -- Accessing an undefined field of a global variable <debug>.
1. Why are global variables ignored sometimes using '143/<name>', sometimes
via adding to the global 'globals', sometimes via the per-file 'globals'?
'debug', 'string', and 'table' are globals, available in all the sources,
just like 'box'.
> + "143/string", -- Accessing an undefined field of a global variable <string>.
> + "143/table", -- Accessing an undefined field of a global variable <table>.
> + "212/self", -- Unused argument <self>.
2. This is one of the main reasons, why non-standardized alignment applied
to everything is evil. Diff split into patches does not make much sense, when a
next patch anyway should rewrite 100% of the previous one just to make alignment
correct. I would propose to make the alignment correct right away, from the first
patch. Or remove it. Or make it less strict, so not all lines are required to be
equaly aligned. Whatever helps not to change already fine lines.
> + "411", -- Redefining a local variable.
> + "421", -- Shadowing a local variable.
> + "431", -- Shadowing an upvalue.
> + "432", -- Shadowing an upvalue argument.
> }
> @@ -27,3 +32,18 @@ files["extra/dist/tarantoolctl.in"] = {
> "122", -- https://github.com/tarantool/tarantool/issues/4929
> },
> }
> +files["src/lua/help.lua"] = {
> + globals = {"help", "tutorial"}, -- globals defined for interactive mode.
3. Lets use single style for all comments: start sentences from
capital letters. Instead of switching between lowcase and normal
writing from time to time.
> +}> +files["src/lua/init.lua"] = {
> + globals = {"dostring"}, -- miscellaneous global function definition.
> + ignore = {
> + "122/os", -- set tarantool specific behaviour for os.exit.
> + "142/package", -- add custom functions into Lua package namespace.
4. os and package changes can be done using rawset() function. It does not
produce a warning, and behaves in the above cases the same. However this
should be consulted with other reviewers.
Another option - add 'os' and 'package' to the list of globals. It also fixes
the warnings.
> + },
> +}
> +files["src/lua/swim.lua"] = {
> + ignore = {
> + "212/m", -- respect swim module code style.
5. Once again I wonder why do we ignore unused arguments sometimes globaly in
'ignore = {...}', sometimes per-file like here, sometimes using inlined luacheck
comments.
In this particular case there is even a fourth option - rename 'm' to 'self' in
the single problematic line. Like you did for many similar places.
> + },
> +}
> diff --git a/src/lua/argparse.lua b/src/lua/argparse.lua
> index faa0ae130..6c8f10fc1 100644
> --- a/src/lua/argparse.lua
> +++ b/src/lua/argparse.lua
> @@ -91,7 +91,8 @@ local function convert_parameter(name, convert_from, convert_to)
> end
>
> local function parameters_parse(t_in, options)
> - local t_out, t_in = {}, t_in or {}
> + local t_out = {}
> + t_in = t_in or {}
6. Unnecessary diff. At least when I check on top of the branch.
> diff --git a/src/lua/init.lua b/src/lua/init.lua
> index ff3e74c3c..aea7a7491 100644
> --- a/src/lua/init.lua
> +++ b/src/lua/init.lua
> @@ -144,9 +144,9 @@ local function gen_search_func(path_fn, templates, need_traverse)
>
> local searchpaths = {}
>
> - for _, path in ipairs(paths) do
> + for _, p in ipairs(paths) do
> for _, template in pairs(templates) do
> - table.insert(searchpaths, fio.pathjoin(path, template))
> + table.insert(searchpaths, fio.pathjoin(p, template))
> end
7. Ditto.
> end
>
> @@ -603,7 +599,7 @@ local function check_offset(offset, len)
> if offset == nil then
> return 1
> end
> - local offset = ffi.cast('ptrdiff_t', offset)
> + offset = ffi.cast('ptrdiff_t', offset)
8. Ditto.
> if offset < 1 or offset > len then
> error(string.format("offset = %d is out of bounds [1..%d]",
> tonumber(offset), len))
> diff --git a/src/lua/socket.lua b/src/lua/socket.lua
> index bbdef01e3..2a2c7a32c 100644
> --- a/src/lua/socket.lua
> +++ b/src/lua/socket.lua
> @@ -1044,7 +1044,7 @@ local function tcp_connect(host, port, timeout)
> boxerrno(0)
> return s
> end
> - local timeout = timeout or TIMEOUT_INFINITY
> + timeout = timeout or TIMEOUT_INFINITY
9. Ditto.
> local stop = fiber.clock() + timeout
> local dns = getaddrinfo(host, port, timeout, { type = 'SOCK_STREAM',
> protocol = 'tcp' })
> @@ -1319,7 +1319,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: no unused args
10. Mode is really ignored, and this is not some kind of built-in
function with 'fixed' signature. And it is not used 'virtually',
when there is a variable keeping pointer at one function among
many having the same signature. So what is a purpose of keeping it?
> check_socket(self)
> self.timeout = value
> -- mode is effectively ignored
> @@ -1475,7 +1475,7 @@ local function lsocket_tcp_receive(self, pattern, prefix)
> local result = { prefix }
> local deadline = fiber.clock() + (self.timeout or TIMEOUT_INFINITY)
> repeat
> - local data = read(self, LIMIT_INFINITY, timeout, check_infinity)
> + data = read(self, LIMIT_INFINITY, timeout, check_infinity)
11. Unnecessary diff.
> if data == nil then
> if not errno_is_transient[self._errno] then
> return nil, socket_error(self)
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Tarantool-patches] [PATCH 6/6] Fix luacheck warnings in src/box/lua/
2020-06-04 8:39 ` [Tarantool-patches] [PATCH 6/6] Fix luacheck warnings in src/box/lua/ sergeyb
@ 2020-06-06 18:03 ` Vladislav Shpilevoy
2020-06-11 12:01 ` Sergey Bronnikov
2020-06-19 23:15 ` Vladislav Shpilevoy
2020-07-13 22:59 ` Vladislav Shpilevoy
2 siblings, 1 reply; 41+ messages in thread
From: Vladislav Shpilevoy @ 2020-06-06 18:03 UTC (permalink / raw)
To: sergeyb, tarantool-patches, imun; +Cc: alexander.turenko
Thanks for the patch!
See 7 comments below.
On 04/06/2020 10:39, sergeyb@tarantool.org wrote:
> From: Sergey Bronnikov <sergeyb@tarantool.org>
>
> Part of #4681
>
> Reviewed-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
> Co-authored-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
> ---
> .luacheckrc | 10 +++++--
> src/box/lua/console.lua | 10 +++----
> src/box/lua/feedback_daemon.lua | 2 +-
> src/box/lua/key_def.lua | 2 +-
> src/box/lua/load_cfg.lua | 11 ++++---
> src/box/lua/net_box.lua | 52 +++++++++++++--------------------
> src/box/lua/schema.lua | 44 +++++++++++++---------------
> src/box/lua/tuple.lua | 8 ++---
> src/box/lua/upgrade.lua | 19 ++++++------
> 9 files changed, 73 insertions(+), 85 deletions(-)
>
> diff --git a/.luacheckrc b/.luacheckrc
> index fb8b9dfb3..9fd017629 100644
> --- a/.luacheckrc
> +++ b/.luacheckrc
> @@ -19,7 +20,7 @@ include_files = {
>
> exclude_files = {
> "build/**/*.lua",
> - "src/box/**/*.lua",
> + "src/box/lua/serpent.lua", -- third-party source code
1. I see the comments are getting less and less strict in every new patch.
Please, be consistent. Use a capital letter to start a sentence, and end it
with dot.
> "test-run/**/*.lua",
> "test/**/*.lua",
> "third_party/**/*.lua",
> diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
> index 9560bfdd4..f71744014 100644
> --- a/src/box/lua/net_box.lua
> +++ b/src/box/lua/net_box.lua
> @@ -187,7 +183,7 @@ local function next_id(id) return band(id + 1, 0x7FFFFFFF) end
> -- @retval two non-nils A connected socket and a decoded greeting.
> --
> local function establish_connection(host, port, timeout)
> - local timeout = timeout or DEFAULT_CONNECT_TIMEOUT
> + timeout = timeout or DEFAULT_CONNECT_TIMEOUT
2. Unnecessary diff. I reverted this line and nothing changed.
> local begin = fiber.clock()
> local s = socket.tcp_connect(host, port, timeout)
> if not s then
> diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
> index cdfbb65f7..4fe0ff8da 100644
> --- a/src/box/lua/schema.lua
> +++ b/src/box/lua/schema.lua
> @@ -846,7 +844,7 @@ local function space_sequence_alter_prepare(format, parts, options,
> if id == nil then
> box.error(box.error.NO_SUCH_SEQUENCE, new_sequence.id)
> end
> - local tuple = _space_sequence.index.sequence:select(id)[1]
> + tuple = _space_sequence.index.sequence:select(id)[1]
3. Ditto.
> if tuple ~= nil and tuple.is_generated then
> box.error(box.error.ALTER_SPACE, space_name,
> "can not attach generated sequence")
> @@ -2681,7 +2679,7 @@ box.once = function(key, func, ...)
> box.error(box.error.ILLEGAL_PARAMS, "Usage: box.once(key, func, ...)")
> end
>
> - local key = "once"..key
> + key = "once"..key
4. Ditto.
> if box.space._schema:get{key} ~= nil then
> return
> end
> diff --git a/src/box/lua/tuple.lua b/src/box/lua/tuple.lua
> index f97aa1579..87f53258b 100644
> --- a/src/box/lua/tuple.lua
> +++ b/src/box/lua/tuple.lua
> @@ -232,7 +230,7 @@ local function tuple_update(tuple, expr)
> error("Usage: tuple:update({ { op, field, arg}+ })")
> end
> local pexpr, pexpr_end = tuple_encode(expr)
> - local tuple = builtin.box_tuple_update(tuple, pexpr, pexpr_end)
> + tuple = builtin.box_tuple_update(tuple, pexpr, pexpr_end)
5. Ditto.
> if tuple == nil then
> return box.error()
> end
> @@ -245,7 +243,7 @@ local function tuple_upsert(tuple, expr)
> error("Usage: tuple:upsert({ { op, field, arg}+ })")
> end
> local pexpr, pexpr_end = tuple_encode(expr)
> - local tuple = builtin.box_tuple_upsert(tuple, pexpr, pexpr_end)
> + tuple = builtin.box_tuple_upsert(tuple, pexpr, pexpr_end)
6. Ditto.
> if tuple == nil then
> return box.error()
> end
> diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
> index 075cc236e..208b53f15 100644
> --- a/src/box/lua/upgrade.lua
> +++ b/src/box/lua/upgrade.lua
> @@ -580,7 +579,7 @@ local function upgrade_to_2_1_0()
> -- Now, abscent field means NULL, so we can safely set second
> -- field in format, marking it nullable.
> log.info("Add nullable value field to space _schema")
> - local format = {}
> + format = {}
7. Ditto.
> format[1] = {type='string', name='key'}
> format[2] = {type='any', name='value', is_nullable=true}
> box.space._schema:format(format)
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Tarantool-patches] [PATCH 1/6] Add initial luacheck config
2020-06-06 18:02 ` Vladislav Shpilevoy
@ 2020-06-09 16:16 ` Sergey Bronnikov
0 siblings, 0 replies; 41+ messages in thread
From: Sergey Bronnikov @ 2020-06-09 16:16 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: alexander.turenko, tarantool-patches
Hi,
thanks for review!
On 20:02 Sat 06 Jun , Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
>
> On 04/06/2020 10:39, sergeyb@tarantool.org wrote:
> > From: Sergey Bronnikov <sergeyb@tarantool.org>
> >
> > config includes all files with Lua source code except:
>
> Lets start sentences from capital letters.
>
> > - third_party repositories
> > - directories with diff-based tests
>
> Seems like src/* is excluded. So nothing is checked, and the
> commit message is wrong.
Agree, removed section "how-to" in a commit message.
Next commit brings target 'luacheck' and it's commit message
self-explain how to execute static analysis.
> I see, that you turn on the checks in next commits, but then the
> commit message should be changed.
>
> > How-to check:
> >
> > $ luacheck --codes --config .luacheckrc .
> >
> > Part of #4681
> >
> > Reviewed-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
> > Co-authored-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
--
sergeyb@
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Tarantool-patches] [PATCH 4/6] Fix luacheck warnings in extra/dist/
2020-06-06 18:02 ` Vladislav Shpilevoy
@ 2020-06-09 16:17 ` Sergey Bronnikov
0 siblings, 0 replies; 41+ messages in thread
From: Sergey Bronnikov @ 2020-06-09 16:17 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: alexander.turenko, tarantool-patches
Hi,
thanks for review!
On 20:02 Sat 06 Jun , Vladislav Shpilevoy wrote:
<snipped>
> > diff --git a/.luacheckrc b/.luacheckrc
> > index e6edf8617..b917eb927 100644
> > --- a/.luacheckrc
> > +++ b/.luacheckrc
> > @@ -1,12 +1,19 @@
> > std = "luajit"
> > +globals = {"box", "_TARANTOOL"}
> > +ignore = {
> > + "212/self", -- Unused argument <self>.
> > + "411", -- Redefining a local variable.
> > + "431", -- Shadowing an upvalue.
> > +}
> > +
>
> Unnecessary empty line after }.
Fixed in a branch.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Tarantool-patches] [PATCH 5/6] Fix luacheck warnings in src/lua/
2020-06-06 18:03 ` Vladislav Shpilevoy
@ 2020-06-11 12:00 ` Sergey Bronnikov
2020-06-11 19:52 ` Vladislav Shpilevoy
2020-06-18 9:35 ` Sergey Bronnikov
2020-06-11 17:22 ` Igor Munkin
1 sibling, 2 replies; 41+ messages in thread
From: Sergey Bronnikov @ 2020-06-11 12:00 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: alexander.turenko, tarantool-patches
Hi, Vlad
thanks for review!
On 20:03 Sat 06 Jun , Vladislav Shpilevoy wrote:
> Thanks for the patch!
>
> See 11 comments below.
>
> On 04/06/2020 10:39, sergeyb@tarantool.org wrote:
> > From: Sergey Bronnikov <sergeyb@tarantool.org>
> >
> > Part of #4681
> >
> > Reviewed-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
> > Reviewed-by: Igor Munkin <imun@tarantool.org>
> >
> > Co-authored-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
> > Co-authored-by: Igor Munkin <imun@tarantool.org>
> > ---
> > .luacheckrc | 28 ++++++++++++++++++++++++----
> > src/lua/argparse.lua | 3 ++-
> > src/lua/buffer.lua | 4 ++--
> > src/lua/clock.lua | 2 +-
> > src/lua/crypto.lua | 4 ++--
> > src/lua/csv.lua | 5 ++---
> > src/lua/digest.lua | 2 +-
> > src/lua/env.lua | 2 +-
> > src/lua/fiber.lua | 4 ++--
> > src/lua/fio.lua | 30 ++++++++++++++----------------
> > src/lua/help.lua | 7 ++-----
> > src/lua/httpc.lua | 3 ---
> > src/lua/init.lua | 4 ++--
> > src/lua/msgpackffi.lua | 22 +++++++++-------------
> > src/lua/socket.lua | 16 ++++++++--------
> > src/lua/string.lua | 1 -
> > src/lua/swim.lua | 2 +-
> > src/lua/tap.lua | 4 ----
> > src/lua/trigger.lua | 3 ---
> > 19 files changed, 73 insertions(+), 73 deletions(-)
> >
> > diff --git a/.luacheckrc b/.luacheckrc
> > index b917eb927..fb8b9dfb3 100644
> > --- a/.luacheckrc
> > +++ b/.luacheckrc
> > @@ -1,9 +1,14 @@
> > std = "luajit"
> > globals = {"box", "_TARANTOOL"}
> > ignore = {
> > - "212/self", -- Unused argument <self>.
> > - "411", -- Redefining a local variable.
> > - "431", -- Shadowing an upvalue.
> > + "143/debug", -- Accessing an undefined field of a global variable <debug>.
>
> 1. Why are global variables ignored sometimes using '143/<name>', sometimes
> via adding to the global 'globals', sometimes via the per-file 'globals'?
> 'debug', 'string', and 'table' are globals, available in all the sources,
> just like 'box'.
the main idea was to supress rarely used variables inline and often used
variables globally in .luacheckrc. Suggested by Igor in at least [1]
and I'm agree with his approach.
1. https://lists.tarantool.org/pipermail/tarantool-patches/2020-May/017108.html
>
> > + "143/string", -- Accessing an undefined field of a global variable <string>.
> > + "143/table", -- Accessing an undefined field of a global variable <table>.
> > + "212/self", -- Unused argument <self>.
>
> 2. This is one of the main reasons, why non-standardized alignment applied
> to everything is evil. Diff split into patches does not make much sense, when a
> next patch anyway should rewrite 100% of the previous one just to make alignment
> correct. I would propose to make the alignment correct right away, from the first
> patch. Or remove it. Or make it less strict, so not all lines are required to be
> equaly aligned. Whatever helps not to change already fine lines.
Moved comments before code.
> > + "411", -- Redefining a local variable.
> > + "421", -- Shadowing a local variable.
> > + "431", -- Shadowing an upvalue.
> > + "432", -- Shadowing an upvalue argument.
> > }
> > @@ -27,3 +32,18 @@ files["extra/dist/tarantoolctl.in"] = {
> > "122", -- https://github.com/tarantool/tarantool/issues/4929
> > },
> > }
> > +files["src/lua/help.lua"] = {
> > + globals = {"help", "tutorial"}, -- globals defined for interactive mode.
>
> 3. Lets use single style for all comments: start sentences from
> capital letters. Instead of switching between lowcase and normal
> writing from time to time.
Fixed.
>
> > +}> +files["src/lua/init.lua"] = {
> > + globals = {"dostring"}, -- miscellaneous global function definition.
> > + ignore = {
> > + "122/os", -- set tarantool specific behaviour for os.exit.
> > + "142/package", -- add custom functions into Lua package namespace.
>
> 4. os and package changes can be done using rawset() function. It does not
> produce a warning, and behaves in the above cases the same. However this
> should be consulted with other reviewers.
>
> Another option - add 'os' and 'package' to the list of globals. It also fixes
> the warnings.
Fixed using rawset().
>
> > + },
> > +}
> > +files["src/lua/swim.lua"] = {
> > + ignore = {
> > + "212/m", -- respect swim module code style.
>
> 5. Once again I wonder why do we ignore unused arguments sometimes globaly in
> 'ignore = {...}', sometimes per-file like here, sometimes using inlined luacheck
> comments.
Answered in comment 1.
> In this particular case there is even a fourth option - rename 'm' to 'self' in
> the single problematic line. Like you did for many similar places.
replaced 'm' to 'self'.
> > + },
> > +}
> > diff --git a/src/lua/argparse.lua b/src/lua/argparse.lua
> > index faa0ae130..6c8f10fc1 100644
> > --- a/src/lua/argparse.lua
> > +++ b/src/lua/argparse.lua
> > @@ -91,7 +91,8 @@ local function convert_parameter(name, convert_from, convert_to)
> > end
> >
> > local function parameters_parse(t_in, options)
> > - local t_out, t_in = {}, t_in or {}
> > + local t_out = {}
> > + t_in = t_in or {}
>
> 6. Unnecessary diff. At least when I check on top of the branch.
change is required, see [1]
1. https://gitlab.com/tarantool/tarantool/-/jobs/589857153
>
> > diff --git a/src/lua/init.lua b/src/lua/init.lua
> > index ff3e74c3c..aea7a7491 100644
> > --- a/src/lua/init.lua
> > +++ b/src/lua/init.lua
> > @@ -144,9 +144,9 @@ local function gen_search_func(path_fn, templates, need_traverse)
> >
> > local searchpaths = {}
> >
> > - for _, path in ipairs(paths) do
> > + for _, p in ipairs(paths) do
> > for _, template in pairs(templates) do
> > - table.insert(searchpaths, fio.pathjoin(path, template))
> > + table.insert(searchpaths, fio.pathjoin(p, template))
> > end
>
> 7. Ditto.
change reverted
>
> > end
> >
> > @@ -603,7 +599,7 @@ local function check_offset(offset, len)
> > if offset == nil then
> > return 1
> > end
> > - local offset = ffi.cast('ptrdiff_t', offset)
> > + offset = ffi.cast('ptrdiff_t', offset)
>
> 8. Ditto.
change is required, see [1]
1. https://gitlab.com/tarantool/tarantool/-/jobs/589857153
>
> > if offset < 1 or offset > len then
> > error(string.format("offset = %d is out of bounds [1..%d]",
> > tonumber(offset), len))
> > diff --git a/src/lua/socket.lua b/src/lua/socket.lua
> > index bbdef01e3..2a2c7a32c 100644
> > --- a/src/lua/socket.lua
> > +++ b/src/lua/socket.lua
> > @@ -1044,7 +1044,7 @@ local function tcp_connect(host, port, timeout)
> > boxerrno(0)
> > return s
> > end
> > - local timeout = timeout or TIMEOUT_INFINITY
> > + timeout = timeout or TIMEOUT_INFINITY
>
> 9. Ditto.
change is required, see [1]
src/lua/socket.lua:344:11: variable timeout was previously defined as an argument on line 340
1. https://gitlab.com/tarantool/tarantool/-/jobs/589857153
> > local stop = fiber.clock() + timeout
> > local dns = getaddrinfo(host, port, timeout, { type = 'SOCK_STREAM',
> > protocol = 'tcp' })
> > @@ -1319,7 +1319,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: no unused args
>
> 10. Mode is really ignored, and this is not some kind of built-in
> function with 'fixed' signature. And it is not used 'virtually',
> when there is a variable keeping pointer at one function among
> many having the same signature. So what is a purpose of keeping it?
removed mode arg
> > check_socket(self)
> > self.timeout = value
> > -- mode is effectively ignored
> > @@ -1475,7 +1475,7 @@ local function lsocket_tcp_receive(self, pattern, prefix)
> > local result = { prefix }
> > local deadline = fiber.clock() + (self.timeout or TIMEOUT_INFINITY)
> > repeat
> > - local data = read(self, LIMIT_INFINITY, timeout, check_infinity)
> > + data = read(self, LIMIT_INFINITY, timeout, check_infinity)
>
> 11. Unnecessary diff.
change reverted
> > if data == nil then
> > if not errno_is_transient[self._errno] then
> > return nil, socket_error(self)
--
sergeyb@
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Tarantool-patches] [PATCH 6/6] Fix luacheck warnings in src/box/lua/
2020-06-06 18:03 ` Vladislav Shpilevoy
@ 2020-06-11 12:01 ` Sergey Bronnikov
2020-06-18 9:37 ` Sergey Bronnikov
0 siblings, 1 reply; 41+ messages in thread
From: Sergey Bronnikov @ 2020-06-11 12:01 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: alexander.turenko, tarantool-patches
Hi,
thanks for review!
On 20:03 Sat 06 Jun , Vladislav Shpilevoy wrote:
> Thanks for the patch!
>
> See 7 comments below.
>
> On 04/06/2020 10:39, sergeyb@tarantool.org wrote:
> > From: Sergey Bronnikov <sergeyb@tarantool.org>
> >
> > Part of #4681
> >
> > Reviewed-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
> > Co-authored-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
> > ---
> > .luacheckrc | 10 +++++--
> > src/box/lua/console.lua | 10 +++----
> > src/box/lua/feedback_daemon.lua | 2 +-
> > src/box/lua/key_def.lua | 2 +-
> > src/box/lua/load_cfg.lua | 11 ++++---
> > src/box/lua/net_box.lua | 52 +++++++++++++--------------------
> > src/box/lua/schema.lua | 44 +++++++++++++---------------
> > src/box/lua/tuple.lua | 8 ++---
> > src/box/lua/upgrade.lua | 19 ++++++------
> > 9 files changed, 73 insertions(+), 85 deletions(-)
> >
> > diff --git a/.luacheckrc b/.luacheckrc
> > index fb8b9dfb3..9fd017629 100644
> > --- a/.luacheckrc
> > +++ b/.luacheckrc
> > @@ -19,7 +20,7 @@ include_files = {
> >
> > exclude_files = {
> > "build/**/*.lua",
> > - "src/box/**/*.lua",
> > + "src/box/lua/serpent.lua", -- third-party source code
>
> 1. I see the comments are getting less and less strict in every new patch.
> Please, be consistent. Use a capital letter to start a sentence, and end it
> with dot.
Fixed.
> > "test-run/**/*.lua",
> > "test/**/*.lua",
> > "third_party/**/*.lua",
> > diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
> > index 9560bfdd4..f71744014 100644
> > --- a/src/box/lua/net_box.lua
> > +++ b/src/box/lua/net_box.lua
> > @@ -187,7 +183,7 @@ local function next_id(id) return band(id + 1, 0x7FFFFFFF) end
> > -- @retval two non-nils A connected socket and a decoded greeting.
> > --
> > local function establish_connection(host, port, timeout)
> > - local timeout = timeout or DEFAULT_CONNECT_TIMEOUT
> > + timeout = timeout or DEFAULT_CONNECT_TIMEOUT
>
> 2. Unnecessary diff. I reverted this line and nothing changed.
change is required, see [1]
src/box/lua/net_box.lua:186:11: variable timeout was previously defined as an argument on line 185
1. https://gitlab.com/tarantool/tarantool/-/jobs/589857153
> > local begin = fiber.clock()
> > local s = socket.tcp_connect(host, port, timeout)
> > if not s then
> > diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
> > index cdfbb65f7..4fe0ff8da 100644
> > --- a/src/box/lua/schema.lua
> > +++ b/src/box/lua/schema.lua
> > @@ -846,7 +844,7 @@ local function space_sequence_alter_prepare(format, parts, options,
> > if id == nil then
> > box.error(box.error.NO_SUCH_SEQUENCE, new_sequence.id)
> > end
> > - local tuple = _space_sequence.index.sequence:select(id)[1]
> > + tuple = _space_sequence.index.sequence:select(id)[1]
>
> 3. Ditto.
change is reverted
> > if tuple ~= nil and tuple.is_generated then
> > box.error(box.error.ALTER_SPACE, space_name,
> > "can not attach generated sequence")
> > @@ -2681,7 +2679,7 @@ box.once = function(key, func, ...)
> > box.error(box.error.ILLEGAL_PARAMS, "Usage: box.once(key, func, ...)")
> > end
> >
> > - local key = "once"..key
> > + key = "once"..key
>
> 4. Ditto.
change is reverted
> > if box.space._schema:get{key} ~= nil then
> > return
> > end
> > diff --git a/src/box/lua/tuple.lua b/src/box/lua/tuple.lua
> > index f97aa1579..87f53258b 100644
> > --- a/src/box/lua/tuple.lua
> > +++ b/src/box/lua/tuple.lua
> > @@ -232,7 +230,7 @@ local function tuple_update(tuple, expr)
> > error("Usage: tuple:update({ { op, field, arg}+ })")
> > end
> > local pexpr, pexpr_end = tuple_encode(expr)
> > - local tuple = builtin.box_tuple_update(tuple, pexpr, pexpr_end)
> > + tuple = builtin.box_tuple_update(tuple, pexpr, pexpr_end)
>
> 5. Ditto.
change is required, see [1]
1. https://gitlab.com/tarantool/tarantool/-/jobs/589857153
>
> > if tuple == nil then
> > return box.error()
> > end
> > @@ -245,7 +243,7 @@ local function tuple_upsert(tuple, expr)
> > error("Usage: tuple:upsert({ { op, field, arg}+ })")
> > end
> > local pexpr, pexpr_end = tuple_encode(expr)
> > - local tuple = builtin.box_tuple_upsert(tuple, pexpr, pexpr_end)
> > + tuple = builtin.box_tuple_upsert(tuple, pexpr, pexpr_end)
>
> 6. Ditto.
change is required, see [1]
1. https://gitlab.com/tarantool/tarantool/-/jobs/589857153
> > if tuple == nil then
> > return box.error()
> > end
> > diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
> > index 075cc236e..208b53f15 100644
> > --- a/src/box/lua/upgrade.lua
> > +++ b/src/box/lua/upgrade.lua
> > @@ -580,7 +579,7 @@ local function upgrade_to_2_1_0()
> > -- Now, abscent field means NULL, so we can safely set second
> > -- field in format, marking it nullable.
> > log.info("Add nullable value field to space _schema")
> > - local format = {}
> > + format = {}
>
> 7. Ditto.
change is reverted
> > format[1] = {type='string', name='key'}
> > format[2] = {type='any', name='value', is_nullable=true}
> > box.space._schema:format(format)
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Tarantool-patches] [PATCH 5/6] Fix luacheck warnings in src/lua/
2020-06-06 18:03 ` Vladislav Shpilevoy
2020-06-11 12:00 ` Sergey Bronnikov
@ 2020-06-11 17:22 ` Igor Munkin
1 sibling, 0 replies; 41+ messages in thread
From: Igor Munkin @ 2020-06-11 17:22 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches, alexander.turenko
Vlad,
Thanks for your review!
On 06.06.20, Vladislav Shpilevoy wrote:
> Thanks for the patch!
>
<snipped>
> > diff --git a/.luacheckrc b/.luacheckrc
> > index b917eb927..fb8b9dfb3 100644
> > --- a/.luacheckrc
> > +++ b/.luacheckrc
> > @@ -1,9 +1,14 @@
> > std = "luajit"
> > globals = {"box", "_TARANTOOL"}
> > ignore = {
> > - "212/self", -- Unused argument <self>.
> > - "411", -- Redefining a local variable.
> > - "431", -- Shadowing an upvalue.
> > + "143/debug", -- Accessing an undefined field of a global variable <debug>.
>
> 1. Why are global variables ignored sometimes using '143/<name>', sometimes
> via adding to the global 'globals', sometimes via the per-file 'globals'?
> 'debug', 'string', and 'table' are globals, available in all the sources,
> just like 'box'.
Historically, someone desided to spoil default Lua namespaces with
user-defined functions and auxiliary extensions. Thereby luacheck
reports about non-standart key lookups in these namespaces. There is
nothing bad in using these namespaces, so only this type of warnings is
suppressed for them.
On the other hand, such globals ad <box>, <_TARANTOOL>, and <tonumber64>
are Tarantool specific ones and it was decided to suppress them globally
since their usage is OK as well as <string>, <_VERSION> and <tonumber>
names provided by Lua.
>
<snipped>
>
> > +}> +files["src/lua/init.lua"] = {
> > + globals = {"dostring"}, -- miscellaneous global function definition.
> > + ignore = {
> > + "122/os", -- set tarantool specific behaviour for os.exit.
> > + "142/package", -- add custom functions into Lua package namespace.
>
> 4. os and package changes can be done using rawset() function. It does not
> produce a warning, and behaves in the above cases the same. However this
> should be consulted with other reviewers.
>
> Another option - add 'os' and 'package' to the list of globals. It also fixes
> the warnings.
The issue is similar to the '143/string' suppression: several default
Lua namespaces need to be (re)defined and it's a single "violation"
reported by luacheck. So we can either fix it the way you proposed above
(but it look less readable for me), or just suppress it for this file.
Suppressing it globally relaxes the already not strict checks.
>
<snipped>
>
> > + },
> > +}
> > +files["src/lua/swim.lua"] = {
> > + ignore = {
> > + "212/m", -- respect swim module code style.
>
> 5. Once again I wonder why do we ignore unused arguments sometimes globaly in
> 'ignore = {...}', sometimes per-file like here, sometimes using inlined luacheck
> comments.
>
> In this particular case there is even a fourth option - rename 'm' to 'self' in
> the single problematic line. Like you did for many similar places.
I just wanted to save the code style for the whole swim source file and
totally OK with such renaming if it doesn't bothers you.
>
<snipped>
> > @@ -1319,7 +1319,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: no unused args
>
> 10. Mode is really ignored, and this is not some kind of built-in
> function with 'fixed' signature. And it is not used 'virtually',
> when there is a variable keeping pointer at one function among
> many having the same signature. So what is a purpose of keeping it?
This *is* a function with the fixed signature[1], since this method
emulates LuaSocket API. This is the reason why the mode argument is
left.
>
<snipped>
[1]: http://w3.impa.br/~diego/software/luasocket/tcp.html#settimeout
--
Best regards,
IM
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Tarantool-patches] [PATCH 5/6] Fix luacheck warnings in src/lua/
2020-06-11 12:00 ` Sergey Bronnikov
@ 2020-06-11 19:52 ` Vladislav Shpilevoy
2020-06-18 9:32 ` Sergey Bronnikov
2020-06-18 9:35 ` Sergey Bronnikov
1 sibling, 1 reply; 41+ messages in thread
From: Vladislav Shpilevoy @ 2020-06-11 19:52 UTC (permalink / raw)
To: Sergey Bronnikov; +Cc: alexander.turenko, tarantool-patches
Thanks for the fixes!
>>> + },
>>> +}
>>> diff --git a/src/lua/argparse.lua b/src/lua/argparse.lua
>>> index faa0ae130..6c8f10fc1 100644
>>> --- a/src/lua/argparse.lua
>>> +++ b/src/lua/argparse.lua
>>> @@ -91,7 +91,8 @@ local function convert_parameter(name, convert_from, convert_to)
>>> end
>>>
>>> local function parameters_parse(t_in, options)
>>> - local t_out, t_in = {}, t_in or {}
>>> + local t_out = {}
>>> + t_in = t_in or {}
>>
>> 6. Unnecessary diff. At least when I check on top of the branch.
>
> change is required, see [1]
>
> 1. https://gitlab.com/tarantool/tarantool/-/jobs/589857153
It reports error 412. It is suppressed globally in .luacheckrc. How
is it possible? I tried it locally, and the warning is not reported.
In the job I see that the .luacheck file is outdated - it does not
contain 412 suppression:
https://gitlab.com/tarantool/tarantool/-/blob/b435c7d040c7ce160d67afde270c336f1b91fddb/.luacheckrc
While on the branch in github the warning is suppressed.
So the diff is unnecessary, after all. Unless the github branch is
outdated.
https://github.com/tarantool/tarantool/blob/ligurio/gh-4681-fix-luacheck-warnings-src/.luacheckrc#L15
The same for all the other places, where you said the change is required.
In the other commits too.
>>> local stop = fiber.clock() + timeout
>>> local dns = getaddrinfo(host, port, timeout, { type = 'SOCK_STREAM',
>>> protocol = 'tcp' })
>>> @@ -1319,7 +1319,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: no unused args
>>
>> 10. Mode is really ignored, and this is not some kind of built-in
>> function with 'fixed' signature. And it is not used 'virtually',
>> when there is a variable keeping pointer at one function among
>> many having the same signature. So what is a purpose of keeping it?
>
> removed mode arg
Yeah, looks like it is needed, after all. Igor says, it conforms
to http://w3.impa.br/~diego/software/luasocket/tcp.html#settimeout
or whatever. Personally, I would remove it anyway. Because in our
sockets and in our code globally this argument is never needed,
and does not conform to anything.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Tarantool-patches] [PATCH 5/6] Fix luacheck warnings in src/lua/
2020-06-11 19:52 ` Vladislav Shpilevoy
@ 2020-06-18 9:32 ` Sergey Bronnikov
0 siblings, 0 replies; 41+ messages in thread
From: Sergey Bronnikov @ 2020-06-18 9:32 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: alexander.turenko, tarantool-patches
Hi,
On 21:52 Thu 11 Jun , Vladislav Shpilevoy wrote:
> Thanks for the fixes!
>
> >>> + },
> >>> +}
> >>> diff --git a/src/lua/argparse.lua b/src/lua/argparse.lua
> >>> index faa0ae130..6c8f10fc1 100644
> >>> --- a/src/lua/argparse.lua
> >>> +++ b/src/lua/argparse.lua
> >>> @@ -91,7 +91,8 @@ local function convert_parameter(name, convert_from, convert_to)
> >>> end
> >>>
> >>> local function parameters_parse(t_in, options)
> >>> - local t_out, t_in = {}, t_in or {}
> >>> + local t_out = {}
> >>> + t_in = t_in or {}
> >>
> >> 6. Unnecessary diff. At least when I check on top of the branch.
> >
> > change is required, see [1]
> >
> > 1. https://gitlab.com/tarantool/tarantool/-/jobs/589857153
>
> It reports error 412. It is suppressed globally in .luacheckrc. How
> is it possible? I tried it locally, and the warning is not reported.
> In the job I see that the .luacheck file is outdated - it does not
> contain 412 suppression:
> https://gitlab.com/tarantool/tarantool/-/blob/b435c7d040c7ce160d67afde270c336f1b91fddb/.luacheckrc
>
> While on the branch in github the warning is suppressed.
>
> So the diff is unnecessary, after all. Unless the github branch is
> outdated.
> https://github.com/tarantool/tarantool/blob/ligurio/gh-4681-fix-luacheck-warnings-src/.luacheckrc#L15
>
> The same for all the other places, where you said the change is required.
> In the other commits too.
I'm so sorry, I was wrong and all your suggested diffs have been applied.
> >>> local stop = fiber.clock() + timeout
> >>> local dns = getaddrinfo(host, port, timeout, { type = 'SOCK_STREAM',
> >>> protocol = 'tcp' })
> >>> @@ -1319,7 +1319,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: no unused args
> >>
> >> 10. Mode is really ignored, and this is not some kind of built-in
> >> function with 'fixed' signature. And it is not used 'virtually',
> >> when there is a variable keeping pointer at one function among
> >> many having the same signature. So what is a purpose of keeping it?
> >
> > removed mode arg
>
> Yeah, looks like it is needed, after all. Igor says, it conforms
> to http://w3.impa.br/~diego/software/luasocket/tcp.html#settimeout
> or whatever. Personally, I would remove it anyway. Because in our
> sockets and in our code globally this argument is never needed,
> and does not conform to anything.
Well, kept it as is (with removed arg)
--
sergeyb@
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Tarantool-patches] [PATCH 5/6] Fix luacheck warnings in src/lua/
2020-06-11 12:00 ` Sergey Bronnikov
2020-06-11 19:52 ` Vladislav Shpilevoy
@ 2020-06-18 9:35 ` Sergey Bronnikov
1 sibling, 0 replies; 41+ messages in thread
From: Sergey Bronnikov @ 2020-06-18 9:35 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: alexander.turenko, tarantool-patches
Hi,
On 15:00 Thu 11 Jun , Sergey Bronnikov wrote:
<snipped>
> > > diff --git a/src/lua/argparse.lua b/src/lua/argparse.lua
> > > index faa0ae130..6c8f10fc1 100644
> > > --- a/src/lua/argparse.lua
> > > +++ b/src/lua/argparse.lua
> > > @@ -91,7 +91,8 @@ local function convert_parameter(name, convert_from, convert_to)
> > > end
> > >
> > > local function parameters_parse(t_in, options)
> > > - local t_out, t_in = {}, t_in or {}
> > > + local t_out = {}
> > > + t_in = t_in or {}
> >
> > 6. Unnecessary diff. At least when I check on top of the branch.
>
> change is required, see [1]
>
> 1. https://gitlab.com/tarantool/tarantool/-/jobs/589857153
I was wrong here, change has been reverted.
> > > diff --git a/src/lua/init.lua b/src/lua/init.lua
> > > index ff3e74c3c..aea7a7491 100644
> > > --- a/src/lua/init.lua
> > > +++ b/src/lua/init.lua
> > > @@ -144,9 +144,9 @@ local function gen_search_func(path_fn, templates, need_traverse)
> > >
> > > local searchpaths = {}
> > >
> > > - for _, path in ipairs(paths) do
> > > + for _, p in ipairs(paths) do
> > > for _, template in pairs(templates) do
> > > - table.insert(searchpaths, fio.pathjoin(path, template))
> > > + table.insert(searchpaths, fio.pathjoin(p, template))
> > > end
> >
> > 7. Ditto.
>
> change reverted
> >
> > > end
> > >
> > > @@ -603,7 +599,7 @@ local function check_offset(offset, len)
> > > if offset == nil then
> > > return 1
> > > end
> > > - local offset = ffi.cast('ptrdiff_t', offset)
> > > + offset = ffi.cast('ptrdiff_t', offset)
> >
> > 8. Ditto.
>
> change is required, see [1]
>
> 1. https://gitlab.com/tarantool/tarantool/-/jobs/589857153
I was wrong here, change has been reverted.
> > > if offset < 1 or offset > len then
> > > error(string.format("offset = %d is out of bounds [1..%d]",
> > > tonumber(offset), len))
> > > diff --git a/src/lua/socket.lua b/src/lua/socket.lua
> > > index bbdef01e3..2a2c7a32c 100644
> > > --- a/src/lua/socket.lua
> > > +++ b/src/lua/socket.lua
> > > @@ -1044,7 +1044,7 @@ local function tcp_connect(host, port, timeout)
> > > boxerrno(0)
> > > return s
> > > end
> > > - local timeout = timeout or TIMEOUT_INFINITY
> > > + timeout = timeout or TIMEOUT_INFINITY
> >
> > 9. Ditto.
>
> change is required, see [1]
>
> src/lua/socket.lua:344:11: variable timeout was previously defined as an argument on line 340
>
> 1. https://gitlab.com/tarantool/tarantool/-/jobs/589857153
I was wrong here, change has been reverted.
<snipped>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Tarantool-patches] [PATCH 6/6] Fix luacheck warnings in src/box/lua/
2020-06-11 12:01 ` Sergey Bronnikov
@ 2020-06-18 9:37 ` Sergey Bronnikov
0 siblings, 0 replies; 41+ messages in thread
From: Sergey Bronnikov @ 2020-06-18 9:37 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: alexander.turenko, tarantool-patches
Hi,
On 15:01 Thu 11 Jun , Sergey Bronnikov wrote:
<snipped>
> > > diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
> > > index 9560bfdd4..f71744014 100644
> > > --- a/src/box/lua/net_box.lua
> > > +++ b/src/box/lua/net_box.lua
> > > @@ -187,7 +183,7 @@ local function next_id(id) return band(id + 1, 0x7FFFFFFF) end
> > > -- @retval two non-nils A connected socket and a decoded greeting.
> > > --
> > > local function establish_connection(host, port, timeout)
> > > - local timeout = timeout or DEFAULT_CONNECT_TIMEOUT
> > > + timeout = timeout or DEFAULT_CONNECT_TIMEOUT
> >
> > 2. Unnecessary diff. I reverted this line and nothing changed.
>
> change is required, see [1]
>
> src/box/lua/net_box.lua:186:11: variable timeout was previously defined as an argument on line 185
>
> 1. https://gitlab.com/tarantool/tarantool/-/jobs/589857153
I was wrong here, change has been reverted.
<snipped>
> > > diff --git a/src/box/lua/tuple.lua b/src/box/lua/tuple.lua
> > > index f97aa1579..87f53258b 100644
> > > --- a/src/box/lua/tuple.lua
> > > +++ b/src/box/lua/tuple.lua
> > > @@ -232,7 +230,7 @@ local function tuple_update(tuple, expr)
> > > error("Usage: tuple:update({ { op, field, arg}+ })")
> > > end
> > > local pexpr, pexpr_end = tuple_encode(expr)
> > > - local tuple = builtin.box_tuple_update(tuple, pexpr, pexpr_end)
> > > + tuple = builtin.box_tuple_update(tuple, pexpr, pexpr_end)
> >
> > 5. Ditto.
>
> change is required, see [1]
>
> 1. https://gitlab.com/tarantool/tarantool/-/jobs/589857153
I was wrong here, change has been reverted.
> > > if tuple == nil then
> > > return box.error()
> > > end
> > > @@ -245,7 +243,7 @@ local function tuple_upsert(tuple, expr)
> > > error("Usage: tuple:upsert({ { op, field, arg}+ })")
> > > end
> > > local pexpr, pexpr_end = tuple_encode(expr)
> > > - local tuple = builtin.box_tuple_upsert(tuple, pexpr, pexpr_end)
> > > + tuple = builtin.box_tuple_upsert(tuple, pexpr, pexpr_end)
> >
> > 6. Ditto.
>
> change is required, see [1]
>
> 1. https://gitlab.com/tarantool/tarantool/-/jobs/589857153
I was wrong here, change has been reverted.
<snipped>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Tarantool-patches] [PATCH 0/6] Add static analysis of Lua code in src/ and extra/ with luacheck
2020-06-04 8:43 ` [Tarantool-patches] [PATCH 0/6] Add static analysis of Lua code in src/ and extra/ with luacheck Sergey Bronnikov
@ 2020-06-19 23:15 ` Vladislav Shpilevoy
2020-07-13 9:53 ` Sergey Bronnikov
0 siblings, 1 reply; 41+ messages in thread
From: Vladislav Shpilevoy @ 2020-06-19 23:15 UTC (permalink / raw)
To: Sergey Bronnikov, tarantool-patches, imun; +Cc: alexander.turenko
Hi! Thanks for the fixes!
The check fails in gitlab:
https://gitlab.com/tarantool/tarantool/-/jobs/600867104
Also it fails locally:
Checking src/lua/log.lua 2 warnings
src/lua/log.lua:321:33: (W212) unused argument update_box_cfg
src/lua/log.lua:441:25: (W212) unused argument oldcfg
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Tarantool-patches] [PATCH 4/6] Fix luacheck warnings in extra/dist/
2020-06-04 8:39 ` [Tarantool-patches] [PATCH 4/6] Fix luacheck warnings in extra/dist/ sergeyb
2020-06-06 18:02 ` Vladislav Shpilevoy
@ 2020-06-19 23:15 ` Vladislav Shpilevoy
2020-07-05 16:28 ` Vladislav Shpilevoy
2020-07-13 10:01 ` Sergey Bronnikov
1 sibling, 2 replies; 41+ messages in thread
From: Vladislav Shpilevoy @ 2020-06-19 23:15 UTC (permalink / raw)
To: sergeyb, tarantool-patches, imun; +Cc: alexander.turenko
Thanks for the fixes!
See 6 comments below.
I see on the branch there is now a mess with commits.
Here is how this commit looks on the branch.
====================
> Fix luacheck warnings in extra/dist/
>
> Part of #4681
>
> Reviewed-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
> Reviewed-by: Igor Munkin <imun@tarantool.org>
>
> Co-authored-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
> Co-authored-by: Igor Munkin <imun@tarantool.org>
>
> diff --git a/.luacheckrc b/.luacheckrc
> index e6edf8617..90d6f7570 100644
> --- a/.luacheckrc
> +++ b/.luacheckrc
> @@ -1,16 +1,51 @@
> std = "luajit"
> +globals = {"box", "_TARANTOOL"}
> +ignore = {
> + -- Unused argument <self>.
> + "212/self",
> + -- Redefining a local variable.
> + "411",
> + -- Shadowing an upvalue.
> + "431",
> +}
>
> include_files = {
> "**/*.lua",
> + "extra/dist/tarantoolctl.in",
> }
>
> exclude_files = {
> "build/**/*.lua",
> - "extra/**/*.lua",
> - "src/**/*.lua",
> + "src/box/**/*.lua",
1. Why did you enable src/lua check, but didn't fix src/lua warnings
in this commit? Instead, there is a next commit which actually fixes
the warnings. What makes this commit not atomic, related not only to
extra/dist/. While you said in the title
"Fix luacheck warnings in extra/dist/".
This patch didn't touch src/lua before. Why did you change it?
> "test-run/**/*.lua",
> "test/**/*.lua",
> "third_party/**/*.lua",
> ".rocks/**/*.lua",
> ".git/**/*.lua",
> }
> +
> +files["extra/dist/tarantoolctl.in"] = {
> + ignore = {
> + -- https://github.com/tarantool/tarantool/issues/4929
> + "122",
> + },
> +}
> +files["src/lua/help.lua"] = {
> + -- Globals defined for interactive mode.
> + globals = {"help", "tutorial"},
> +}
> +files["src/lua/init.lua"] = {
> + -- Miscellaneous global function definition.
> + globals = {"dostring"},
> + ignore = {
> + -- Set tarantool specific behaviour for os.exit.
> + "122/os",
> + -- Add custom functions into Lua package namespace.
> + "142/package",
> + },
> +}
> +files["src/lua/swim.lua"] = {
> + ignore = {
> + "212/m", -- Respect swim module code style.
2. I thought we already dropped this exception and replaced m with self.
Why is it back?
> + },
> +}
Now look at the next commit:
> Fix luacheck warnings in src/lua/
>
> Part of #4681
>
> Reviewed-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
> Reviewed-by: Igor Munkin <imun@tarantool.org>
>
> Co-authored-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
> Co-authored-by: Igor Munkin <imun@tarantool.org>
>
> diff --git a/.luacheckrc b/.luacheckrc
> index 90d6f7570..ef3d30857 100644
> --- a/.luacheckrc
> +++ b/.luacheckrc
> @@ -1,12 +1,22 @@
> std = "luajit"
> -globals = {"box", "_TARANTOOL"}
> +globals = {"box", "_TARANTOOL", "tonumber64"}
> ignore = {
> + -- Accessing an undefined field of a global variable <debug>.
> + "143/debug",
> + -- Accessing an undefined field of a global variable <string>.
> + "143/string",
> + -- Accessing an undefined field of a global variable <table>.
> + "143/table",
> -- Unused argument <self>.
> "212/self",
> -- Redefining a local variable.
> "411",
> + -- Shadowing a local variable.
> + "421",
> -- Shadowing an upvalue.
> "431",
> + -- Shadowing an upvalue argument.
> + "432",
> }
>
> include_files = {
> @@ -16,7 +26,8 @@ include_files = {
>
> exclude_files = {
> "build/**/*.lua",
> - "src/box/**/*.lua",
> + -- Third-party source code.
> + "src/box/lua/serpent.lua",
3. The commit says it fixes src/lua checks, but in fact
- src/lua checks were already enabled. Why were they enabled without
having the warnings fixed?
- this commit enables src/box/lua checks. It does not seem
related to src/lua things.
> "test-run/**/*.lua",
> "test/**/*.lua",
> "third_party/**/*.lua",
> @@ -37,15 +48,10 @@ files["src/lua/help.lua"] = {
> files["src/lua/init.lua"] = {
> -- Miscellaneous global function definition.
> globals = {"dostring"},
> - ignore = {
> - -- Set tarantool specific behaviour for os.exit.
> - "122/os",
> - -- Add custom functions into Lua package namespace.
> - "142/package",
> - },
> }
> -files["src/lua/swim.lua"] = {
> +files["src/box/lua/console.lua"] = {
> ignore = {
> - "212/m", -- Respect swim module code style.
> - },
> + -- https://github.com/tarantool/tarantool/issues/5032
> + "212",
> + }
> }
4. This hunk literally drops the code introduced in the previous
commit. Why did it change so radically? In the last review there
were just unnecessary diff hunks. And now the commits seem to be
completely screwed and mixed.
> diff --git a/src/lua/socket.lua b/src/lua/socket.lua
> index bbdef01e3..a0e8faf38 100644
> --- a/src/lua/socket.lua
> +++ b/src/lua/socket.lua
> @@ -341,7 +341,7 @@ local function do_wait(self, what, timeout)
> local fd = check_socket(self)
>
> self._errno = nil
> - timeout = timeout or TIMEOUT_INFINITY
> + local timeout = timeout or TIMEOUT_INFINITY
5. You said you reverted it, but it is still here.
>
> repeat
> local started = fiber.clock()
> @@ -1044,7 +1044,7 @@ local function tcp_connect(host, port, timeout)
> boxerrno(0)
> return s
> end
> - local timeout = timeout or TIMEOUT_INFINITY
> + timeout = timeout or TIMEOUT_INFINITY
6. Ditto.
I want to kindly ask you to spend more time on self-reviews
before sending a patch. Otherwise it consumes lots of time
when I need to point out the same things again and again.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Tarantool-patches] [PATCH 6/6] Fix luacheck warnings in src/box/lua/
2020-06-04 8:39 ` [Tarantool-patches] [PATCH 6/6] Fix luacheck warnings in src/box/lua/ sergeyb
2020-06-06 18:03 ` Vladislav Shpilevoy
@ 2020-06-19 23:15 ` Vladislav Shpilevoy
2020-07-01 14:02 ` Sergey Bronnikov
2020-07-13 22:59 ` Vladislav Shpilevoy
2 siblings, 1 reply; 41+ messages in thread
From: Vladislav Shpilevoy @ 2020-06-19 23:15 UTC (permalink / raw)
To: sergeyb, tarantool-patches, imun; +Cc: alexander.turenko
> diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
> index e6844b45f..c104dd9c7 100644
> --- a/src/box/lua/schema.lua
> +++ b/src/box/lua/schema.lua
> @@ -1555,7 +1553,7 @@ end
>
> base_index_mt.select_luac = function(index, key, opts)
> check_index_arg(index, 'select')
> - local key = keify(key)
> + key = keify(key)
Unnecessary diff.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Tarantool-patches] [PATCH 6/6] Fix luacheck warnings in src/box/lua/
2020-06-19 23:15 ` Vladislav Shpilevoy
@ 2020-07-01 14:02 ` Sergey Bronnikov
0 siblings, 0 replies; 41+ messages in thread
From: Sergey Bronnikov @ 2020-07-01 14:02 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: alexander.turenko, tarantool-patches
On 01:15 Sat 20 Jun , Vladislav Shpilevoy wrote:
> > diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
> > index e6844b45f..c104dd9c7 100644
> > --- a/src/box/lua/schema.lua
> > +++ b/src/box/lua/schema.lua
> > @@ -1555,7 +1553,7 @@ end
> >
> > base_index_mt.select_luac = function(index, key, opts)
> > check_index_arg(index, 'select')
> > - local key = keify(key)
> > + key = keify(key)
>
> Unnecessary diff.
>
Reverted in a branch
--
sergeyb@
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Tarantool-patches] [PATCH 4/6] Fix luacheck warnings in extra/dist/
2020-06-19 23:15 ` Vladislav Shpilevoy
@ 2020-07-05 16:28 ` Vladislav Shpilevoy
2020-07-12 15:37 ` Vladislav Shpilevoy
2020-07-13 10:01 ` Sergey Bronnikov
1 sibling, 1 reply; 41+ messages in thread
From: Vladislav Shpilevoy @ 2020-07-05 16:28 UTC (permalink / raw)
To: sergeyb, tarantool-patches, imun; +Cc: alexander.turenko
I see you made a small fix in one of the commits about
unnecessary diff. However there is still a mess with the
commits. So can't give LGTM yet.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Tarantool-patches] [PATCH 5/6] Fix luacheck warnings in src/lua/
2020-06-04 8:39 ` [Tarantool-patches] [PATCH 5/6] Fix luacheck warnings in src/lua/ sergeyb
2020-06-06 18:03 ` Vladislav Shpilevoy
@ 2020-07-05 16:28 ` Vladislav Shpilevoy
2020-07-10 16:55 ` Sergey Bronnikov
2020-07-13 22:59 ` Vladislav Shpilevoy
2 siblings, 1 reply; 41+ messages in thread
From: Vladislav Shpilevoy @ 2020-07-05 16:28 UTC (permalink / raw)
To: sergeyb, tarantool-patches, imun; +Cc: alexander.turenko
> diff --git a/src/lua/msgpackffi.lua b/src/lua/msgpackffi.lua
> index f01ffaef0..cb7ad5b88 100644
> --- a/src/lua/msgpackffi.lua
> +++ b/src/lua/msgpackffi.lua
> @@ -603,7 +599,7 @@ local function check_offset(offset, len)
> if offset == nil then
> return 1
> end
> - local offset = ffi.cast('ptrdiff_t', offset)
> + offset = ffi.cast('ptrdiff_t', offset)
Unnecessary diff.
> if offset < 1 or offset > len then
> error(string.format("offset = %d is out of bounds [1..%d]",
> tonumber(offset), len))
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Tarantool-patches] [PATCH 5/6] Fix luacheck warnings in src/lua/
2020-07-05 16:28 ` Vladislav Shpilevoy
@ 2020-07-10 16:55 ` Sergey Bronnikov
2020-07-12 15:37 ` Vladislav Shpilevoy
0 siblings, 1 reply; 41+ messages in thread
From: Sergey Bronnikov @ 2020-07-10 16:55 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: alexander.turenko, tarantool-patches
On 18:28 Sun 05 Jul , Vladislav Shpilevoy wrote:
> > diff --git a/src/lua/msgpackffi.lua b/src/lua/msgpackffi.lua
> > index f01ffaef0..cb7ad5b88 100644
> > --- a/src/lua/msgpackffi.lua
> > +++ b/src/lua/msgpackffi.lua
> > @@ -603,7 +599,7 @@ local function check_offset(offset, len)
> > if offset == nil then
> > return 1
> > end
> > - local offset = ffi.cast('ptrdiff_t', offset)
> > + offset = ffi.cast('ptrdiff_t', offset)
>
> Unnecessary diff.
Reverted in a branch.
> > if offset < 1 or offset > len then
> > error(string.format("offset = %d is out of bounds [1..%d]",
> > tonumber(offset), len))
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Tarantool-patches] [PATCH 4/6] Fix luacheck warnings in extra/dist/
2020-07-05 16:28 ` Vladislav Shpilevoy
@ 2020-07-12 15:37 ` Vladislav Shpilevoy
0 siblings, 0 replies; 41+ messages in thread
From: Vladislav Shpilevoy @ 2020-07-12 15:37 UTC (permalink / raw)
To: sergeyb, tarantool-patches, imun; +Cc: alexander.turenko
When we talked last time, you said you probably fixed it already
on the branch, and I could forget to make git reset --hard origin/,
but I just checked - nothing is fixed on the branch.
On 05.07.2020 18:28, Vladislav Shpilevoy wrote:
> I see you made a small fix in one of the commits about
> unnecessary diff. However there is still a mess with the
> commits. So can't give LGTM yet.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Tarantool-patches] [PATCH 5/6] Fix luacheck warnings in src/lua/
2020-07-10 16:55 ` Sergey Bronnikov
@ 2020-07-12 15:37 ` Vladislav Shpilevoy
2020-07-13 9:51 ` Sergey Bronnikov
0 siblings, 1 reply; 41+ messages in thread
From: Vladislav Shpilevoy @ 2020-07-12 15:37 UTC (permalink / raw)
To: Sergey Bronnikov; +Cc: alexander.turenko, tarantool-patches
On 10.07.2020 18:55, Sergey Bronnikov wrote:
> On 18:28 Sun 05 Jul , Vladislav Shpilevoy wrote:
>>> diff --git a/src/lua/msgpackffi.lua b/src/lua/msgpackffi.lua
>>> index f01ffaef0..cb7ad5b88 100644
>>> --- a/src/lua/msgpackffi.lua
>>> +++ b/src/lua/msgpackffi.lua
>>> @@ -603,7 +599,7 @@ local function check_offset(offset, len)
>>> if offset == nil then
>>> return 1
>>> end
>>> - local offset = ffi.cast('ptrdiff_t', offset)
>>> + offset = ffi.cast('ptrdiff_t', offset)
>>
>> Unnecessary diff.
>
> Reverted in a branch.
>
>>> if offset < 1 or offset > len then
>>> error(string.format("offset = %d is out of bounds [1..%d]",
>>> tonumber(offset), len))
Looks like it is not. I still see it on the branch.
https://github.com/tarantool/tarantool/blob/ligurio/gh-4681-fix-luacheck-warnings-src/src/lua/msgpackffi.lua#L602
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Tarantool-patches] [PATCH 5/6] Fix luacheck warnings in src/lua/
2020-07-12 15:37 ` Vladislav Shpilevoy
@ 2020-07-13 9:51 ` Sergey Bronnikov
0 siblings, 0 replies; 41+ messages in thread
From: Sergey Bronnikov @ 2020-07-13 9:51 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: alexander.turenko, tarantool-patches
On 17:37 Sun 12 Jul , Vladislav Shpilevoy wrote:
> On 10.07.2020 18:55, Sergey Bronnikov wrote:
> > On 18:28 Sun 05 Jul , Vladislav Shpilevoy wrote:
> >>> diff --git a/src/lua/msgpackffi.lua b/src/lua/msgpackffi.lua
> >>> index f01ffaef0..cb7ad5b88 100644
> >>> --- a/src/lua/msgpackffi.lua
> >>> +++ b/src/lua/msgpackffi.lua
> >>> @@ -603,7 +599,7 @@ local function check_offset(offset, len)
> >>> if offset == nil then
> >>> return 1
> >>> end
> >>> - local offset = ffi.cast('ptrdiff_t', offset)
> >>> + offset = ffi.cast('ptrdiff_t', offset)
> >>
> >> Unnecessary diff.
> >
> > Reverted in a branch.
> >
> >>> if offset < 1 or offset > len then
> >>> error(string.format("offset = %d is out of bounds [1..%d]",
> >>> tonumber(offset), len))
>
> Looks like it is not. I still see it on the branch.
> https://github.com/tarantool/tarantool/blob/ligurio/gh-4681-fix-luacheck-warnings-src/src/lua/msgpackffi.lua#L602
It was not, because branch was not pushed to remote.
Now fix is in a remote branch:
local function check_offset(offset, len)
if offset == nil then
return 1
end
local offset = ffi.cast('ptrdiff_t', offset)
if offset < 1 or offset > len then
error(string.format("offset = %d is out of bounds [1..%d]",
tonumber(offset), len))
end
return offset
end
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Tarantool-patches] [PATCH 0/6] Add static analysis of Lua code in src/ and extra/ with luacheck
2020-06-19 23:15 ` Vladislav Shpilevoy
@ 2020-07-13 9:53 ` Sergey Bronnikov
0 siblings, 0 replies; 41+ messages in thread
From: Sergey Bronnikov @ 2020-07-13 9:53 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: alexander.turenko, tarantool-patches
On 01:15 Sat 20 Jun , Vladislav Shpilevoy wrote:
> Hi! Thanks for the fixes!
>
> The check fails in gitlab:
> https://gitlab.com/tarantool/tarantool/-/jobs/600867104
This time there were problems with cleanup on CI,
AFAIK they fixed now.
CI status for updated branch - https://gitlab.com/tarantool/tarantool/-/pipelines/165870175
> Also it fails locally:
>
> Checking src/lua/log.lua 2 warnings
>
> src/lua/log.lua:321:33: (W212) unused argument update_box_cfg
> src/lua/log.lua:441:25: (W212) unused argument oldcfg
--
sergeyb@
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Tarantool-patches] [PATCH 4/6] Fix luacheck warnings in extra/dist/
2020-06-19 23:15 ` Vladislav Shpilevoy
2020-07-05 16:28 ` Vladislav Shpilevoy
@ 2020-07-13 10:01 ` Sergey Bronnikov
1 sibling, 0 replies; 41+ messages in thread
From: Sergey Bronnikov @ 2020-07-13 10:01 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: alexander.turenko, tarantool-patches
Thanks for review!
On 01:15 Sat 20 Jun , Vladislav Shpilevoy wrote:
> Thanks for the fixes!
>
> See 6 comments below.
>
> I see on the branch there is now a mess with commits.
> Here is how this commit looks on the branch.
>
> ====================
> > Fix luacheck warnings in extra/dist/
> >
> > Part of #4681
> >
> > Reviewed-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
> > Reviewed-by: Igor Munkin <imun@tarantool.org>
> >
> > Co-authored-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
> > Co-authored-by: Igor Munkin <imun@tarantool.org>
> >
> > diff --git a/.luacheckrc b/.luacheckrc
> > index e6edf8617..90d6f7570 100644
> > --- a/.luacheckrc
> > +++ b/.luacheckrc
> > @@ -1,16 +1,51 @@
> > std = "luajit"
> > +globals = {"box", "_TARANTOOL"}
> > +ignore = {
> > + -- Unused argument <self>.
> > + "212/self",
> > + -- Redefining a local variable.
> > + "411",
> > + -- Shadowing an upvalue.
> > + "431",
> > +}
> >
> > include_files = {
> > "**/*.lua",
> > + "extra/dist/tarantoolctl.in",
> > }
> >
> > exclude_files = {
> > "build/**/*.lua",
> > - "extra/**/*.lua",
> > - "src/**/*.lua",
> > + "src/box/**/*.lua",
>
> 1. Why did you enable src/lua check, but didn't fix src/lua warnings
> in this commit? Instead, there is a next commit which actually fixes
> the warnings. What makes this commit not atomic, related not only to
> extra/dist/. While you said in the title
>
> "Fix luacheck warnings in extra/dist/".
>
> This patch didn't touch src/lua before. Why did you change it?
It is a mess due to inaccurate rebase. Fixed it in a branch.
> > "test-run/**/*.lua",
> > "test/**/*.lua",
> > "third_party/**/*.lua",
> > ".rocks/**/*.lua",
> > ".git/**/*.lua",
> > }
> > +
> > +files["extra/dist/tarantoolctl.in"] = {
> > + ignore = {
> > + -- https://github.com/tarantool/tarantool/issues/4929
> > + "122",
> > + },
> > +}
> > +files["src/lua/help.lua"] = {
> > + -- Globals defined for interactive mode.
> > + globals = {"help", "tutorial"},
> > +}
> > +files["src/lua/init.lua"] = {
> > + -- Miscellaneous global function definition.
> > + globals = {"dostring"},
> > + ignore = {
> > + -- Set tarantool specific behaviour for os.exit.
> > + "122/os",
> > + -- Add custom functions into Lua package namespace.
> > + "142/package",
> > + },
> > +}
> > +files["src/lua/swim.lua"] = {
> > + ignore = {
> > + "212/m", -- Respect swim module code style.
>
> 2. I thought we already dropped this exception and replaced m with self.
> Why is it back?
I see only one reason - due to inaccurate rebase.
> > + },
> > +}
>
> Now look at the next commit:
>
> > Fix luacheck warnings in src/lua/
> >
> > Part of #4681
> >
> > Reviewed-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
> > Reviewed-by: Igor Munkin <imun@tarantool.org>
> >
> > Co-authored-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
> > Co-authored-by: Igor Munkin <imun@tarantool.org>
> >
> > diff --git a/.luacheckrc b/.luacheckrc
> > index 90d6f7570..ef3d30857 100644
> > --- a/.luacheckrc
> > +++ b/.luacheckrc
> > @@ -1,12 +1,22 @@
> > std = "luajit"
> > -globals = {"box", "_TARANTOOL"}
> > +globals = {"box", "_TARANTOOL", "tonumber64"}
> > ignore = {
> > + -- Accessing an undefined field of a global variable <debug>.
> > + "143/debug",
> > + -- Accessing an undefined field of a global variable <string>.
> > + "143/string",
> > + -- Accessing an undefined field of a global variable <table>.
> > + "143/table",
> > -- Unused argument <self>.
> > "212/self",
> > -- Redefining a local variable.
> > "411",
> > + -- Shadowing a local variable.
> > + "421",
> > -- Shadowing an upvalue.
> > "431",
> > + -- Shadowing an upvalue argument.
> > + "432",
> > }
> >
> > include_files = {
> > @@ -16,7 +26,8 @@ include_files = {
> >
> > exclude_files = {
> > "build/**/*.lua",
> > - "src/box/**/*.lua",
> > + -- Third-party source code.
> > + "src/box/lua/serpent.lua",
>
> 3. The commit says it fixes src/lua checks, but in fact
>
> - src/lua checks were already enabled. Why were they enabled without
> having the warnings fixed?
>
> - this commit enables src/box/lua checks. It does not seem
> related to src/lua things.
>
> > "test-run/**/*.lua",
> > "test/**/*.lua",
> > "third_party/**/*.lua",
> > @@ -37,15 +48,10 @@ files["src/lua/help.lua"] = {
> > files["src/lua/init.lua"] = {
> > -- Miscellaneous global function definition.
> > globals = {"dostring"},
> > - ignore = {
> > - -- Set tarantool specific behaviour for os.exit.
> > - "122/os",
> > - -- Add custom functions into Lua package namespace.
> > - "142/package",
> > - },
> > }
> > -files["src/lua/swim.lua"] = {
> > +files["src/box/lua/console.lua"] = {
> > ignore = {
> > - "212/m", -- Respect swim module code style.
> > - },
> > + -- https://github.com/tarantool/tarantool/issues/5032
> > + "212",
> > + }
> > }
>
> 4. This hunk literally drops the code introduced in the previous
> commit. Why did it change so radically? In the last review there
> were just unnecessary diff hunks. And now the commits seem to be
> completely screwed and mixed.
I have looked on patches again and made them atomic.
Each patch introduce only changes required by this patch.
> > diff --git a/src/lua/socket.lua b/src/lua/socket.lua
> > index bbdef01e3..a0e8faf38 100644
> > --- a/src/lua/socket.lua
> > +++ b/src/lua/socket.lua
> > @@ -341,7 +341,7 @@ local function do_wait(self, what, timeout)
> > local fd = check_socket(self)
> >
> > self._errno = nil
> > - timeout = timeout or TIMEOUT_INFINITY
> > + local timeout = timeout or TIMEOUT_INFINITY
>
> 5. You said you reverted it, but it is still here.
reverted in a branch and pushed to remote:
local function do_wait(self, what, timeout)
local fd = check_socket(self)
self._errno = nil
timeout = timeout or TIMEOUT_INFINITY
repeat
> >
> > repeat
> > local started = fiber.clock()
> > @@ -1044,7 +1044,7 @@ local function tcp_connect(host, port, timeout)
> > boxerrno(0)
> > return s
> > end
> > - local timeout = timeout or TIMEOUT_INFINITY
> > + timeout = timeout or TIMEOUT_INFINITY
>
> 6. Ditto.
return s
end
local timeout = timeout or TIMEOUT_INFINITY
local stop = fiber.clock() + timeout
local dns = getaddrinfo(host, port, timeout, { type = 'SOCK_STREAM',
protocol = 'tcp' })
if dns == nil or #dns == 0 then
boxerrno(boxerrno.EINVAL)
return nil
> I want to kindly ask you to spend more time on self-reviews
> before sending a patch. Otherwise it consumes lots of time
> when I need to point out the same things again and again.
This time I spend more time on self-review before sending updated patches. I
have applied changes suggested by you in a comments, self-reviewed changes in
each patch, run "luacheck ." after applying each patch.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Tarantool-patches] [PATCH 5/6] Fix luacheck warnings in src/lua/
2020-06-04 8:39 ` [Tarantool-patches] [PATCH 5/6] Fix luacheck warnings in src/lua/ sergeyb
2020-06-06 18:03 ` Vladislav Shpilevoy
2020-07-05 16:28 ` Vladislav Shpilevoy
@ 2020-07-13 22:59 ` Vladislav Shpilevoy
2 siblings, 0 replies; 41+ messages in thread
From: Vladislav Shpilevoy @ 2020-07-13 22:59 UTC (permalink / raw)
To: sergeyb, tarantool-patches, imun; +Cc: alexander.turenko
Hi! Thanks for the fixes!
I've force pushed the following changes:
====================
diff --git a/src/lua/fio.lua b/src/lua/fio.lua
index b31311b7b..954c44cdf 100644
--- a/src/lua/fio.lua
+++ b/src/lua/fio.lua
@@ -378,7 +378,7 @@ fio.mktree = function(path, mode)
end
path = fio.abspath(path)
- path = string.gsub(path, '^/', '')
+ local path = string.gsub(path, '^/', '')
====================
As I pointed out already, all kinds of redefinition warnings
were disabled. No need to touch them.
====================
local dirs = string.split(path, "/")
local current_dir = "/"
diff --git a/src/lua/init.lua b/src/lua/init.lua
index 280ecc03f..9e3c813c3 100644
--- a/src/lua/init.lua
+++ b/src/lua/init.lua
@@ -80,7 +80,7 @@ dostring = function(s, ...)
end
local fiber = require("fiber")
-local exit = function(code)
+local function exit(code)
====================
Easier to grep, and just looks more natural.
====================
code = (type(code) == 'number') and code or 0
ffi.C.tarantool_exit(code)
-- Make sure we yield even if the code after
diff --git a/src/lua/log.lua b/src/lua/log.lua
index bf0e4283c..cfd83f592 100644
--- a/src/lua/log.lua
+++ b/src/lua/log.lua
@@ -459,7 +459,7 @@ local function reload_cfg(cfg)
end
-- Load or reload configuration via log.cfg({}) call.
-local function load_cfg(oldcf, cfg) -- luacheck: no unused args
+local function load_cfg(self, cfg)
====================
The function is a __call method of the configuration table.
So the first argument is always self. And it is legal to
rename it. Also this is preferable since we force 'self' as
a special name in our luacheck config.
====================
cfg = cfg or {}
-- log option might be zero length string, which
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Tarantool-patches] [PATCH 6/6] Fix luacheck warnings in src/box/lua/
2020-06-04 8:39 ` [Tarantool-patches] [PATCH 6/6] Fix luacheck warnings in src/box/lua/ sergeyb
2020-06-06 18:03 ` Vladislav Shpilevoy
2020-06-19 23:15 ` Vladislav Shpilevoy
@ 2020-07-13 22:59 ` Vladislav Shpilevoy
2020-07-14 10:54 ` Sergey Bronnikov
2 siblings, 1 reply; 41+ messages in thread
From: Vladislav Shpilevoy @ 2020-07-13 22:59 UTC (permalink / raw)
To: sergeyb, tarantool-patches, imun; +Cc: alexander.turenko
Thanks for the fixes!
I force pushed the following change:
====================
diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
index 7256bd0a3..1d3c474d5 100644
--- a/src/box/lua/net_box.lua
+++ b/src/box/lua/net_box.lua
@@ -915,7 +915,7 @@ end
-- Now it is necessary to have a strong ref to callback somewhere or
-- it is GC-ed prematurely. We wrap stop() method, stashing the
-- ref in an upvalue (stop() performance doesn't matter much.)
-local create_transport = function(host, port, user, password, callback, -- luacheck: ignore
+local create_transport = function(host, port, user, password, callback,
connection, greeting)
local weak_refs = setmetatable({callback = callback}, {__mode = 'v'})
local function weak_callback(...)
====================
But what I don't understand is why this commit does not close 4681?
What is left?
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Tarantool-patches] [PATCH 6/6] Fix luacheck warnings in src/box/lua/
2020-07-13 22:59 ` Vladislav Shpilevoy
@ 2020-07-14 10:54 ` Sergey Bronnikov
2020-07-14 11:24 ` Sergey Bronnikov
0 siblings, 1 reply; 41+ messages in thread
From: Sergey Bronnikov @ 2020-07-14 10:54 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: alexander.turenko, tarantool-patches
On 00:59 Tue 14 Jul , Vladislav Shpilevoy wrote:
> Thanks for the fixes!
>
<snipped>
> But what I don't understand is why this commit does not close 4681?
> What is left?
Earlier we decided to split patch series for {extra,src} and test/.
So there is another patch series for Lua source code in test/.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Tarantool-patches] [PATCH 6/6] Fix luacheck warnings in src/box/lua/
2020-07-14 10:54 ` Sergey Bronnikov
@ 2020-07-14 11:24 ` Sergey Bronnikov
0 siblings, 0 replies; 41+ messages in thread
From: Sergey Bronnikov @ 2020-07-14 11:24 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches, alexander.turenko
On 13:54 Tue 14 Jul , Sergey Bronnikov wrote:
> On 00:59 Tue 14 Jul , Vladislav Shpilevoy wrote:
> > Thanks for the fixes!
> >
> <snipped>
> > But what I don't understand is why this commit does not close 4681?
> > What is left?
>
> Earlier we decided to split patch series for {extra,src} and test/.
> So there is another patch series for Lua source code in test/.
See patch series for test/ in [1]. As soon as we will commit patch
series for {extra,src} I'll resent updated patch series for test/.
1. https://lists.tarantool.org/pipermail/tarantool-patches/2020-May/017237.html
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Tarantool-patches] [PATCH 0/6] Add static analysis of Lua code in src/ and extra/ with luacheck
2020-06-04 8:39 [Tarantool-patches] [PATCH 0/6] Add static analysis of Lua code in src/ and extra/ with luacheck sergeyb
` (8 preceding siblings ...)
2020-06-04 11:17 ` Sergey Bronnikov
@ 2020-07-14 22:49 ` Vladislav Shpilevoy
2020-07-15 9:51 ` Kirill Yukhin
10 siblings, 0 replies; 41+ messages in thread
From: Vladislav Shpilevoy @ 2020-07-14 22:49 UTC (permalink / raw)
To: sergeyb, tarantool-patches, imun; +Cc: alexander.turenko
The patchset LGTM.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Tarantool-patches] [PATCH 0/6] Add static analysis of Lua code in src/ and extra/ with luacheck
2020-06-04 8:39 [Tarantool-patches] [PATCH 0/6] Add static analysis of Lua code in src/ and extra/ with luacheck sergeyb
` (9 preceding siblings ...)
2020-07-14 22:49 ` Vladislav Shpilevoy
@ 2020-07-15 9:51 ` Kirill Yukhin
10 siblings, 0 replies; 41+ messages in thread
From: Kirill Yukhin @ 2020-07-15 9:51 UTC (permalink / raw)
To: sergeyb; +Cc: alexander.turenko, tarantool-patches, v.shpilevoy
Hello,
On 04 июн 11:39, sergeyb@tarantool.org wrote:
> From: Sergey Bronnikov <sergeyb@tarantool.org>
>
> Sergey Bronnikov (6):
> Add initial luacheck config
> build: enable 'make luacheck' target
> gitlab-ci: enable static analysis with luacheck
> Fix luacheck warnings in extra/dist/
> Fix luacheck warnings in src/lua/
> Fix luacheck warnings in src/box/lua/
I've checked the patchset into master.
--
Regards, Kirill Yukhin
^ permalink raw reply [flat|nested] 41+ messages in thread
end of thread, other threads:[~2020-07-15 9:51 UTC | newest]
Thread overview: 41+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-04 8:39 [Tarantool-patches] [PATCH 0/6] Add static analysis of Lua code in src/ and extra/ with luacheck sergeyb
2020-06-04 8:39 ` [Tarantool-patches] [PATCH 1/6] Add initial luacheck config sergeyb
2020-06-06 18:02 ` Vladislav Shpilevoy
2020-06-09 16:16 ` Sergey Bronnikov
2020-06-04 8:39 ` [Tarantool-patches] [PATCH 2/6] build: enable 'make luacheck' target sergeyb
2020-06-04 8:39 ` [Tarantool-patches] [PATCH 3/6] gitlab-ci: enable static analysis with luacheck sergeyb
2020-06-04 8:39 ` [Tarantool-patches] [PATCH 4/6] Fix luacheck warnings in extra/dist/ sergeyb
2020-06-06 18:02 ` Vladislav Shpilevoy
2020-06-09 16:17 ` Sergey Bronnikov
2020-06-19 23:15 ` Vladislav Shpilevoy
2020-07-05 16:28 ` Vladislav Shpilevoy
2020-07-12 15:37 ` Vladislav Shpilevoy
2020-07-13 10:01 ` Sergey Bronnikov
2020-06-04 8:39 ` [Tarantool-patches] [PATCH 5/6] Fix luacheck warnings in src/lua/ sergeyb
2020-06-06 18:03 ` Vladislav Shpilevoy
2020-06-11 12:00 ` Sergey Bronnikov
2020-06-11 19:52 ` Vladislav Shpilevoy
2020-06-18 9:32 ` Sergey Bronnikov
2020-06-18 9:35 ` Sergey Bronnikov
2020-06-11 17:22 ` Igor Munkin
2020-07-05 16:28 ` Vladislav Shpilevoy
2020-07-10 16:55 ` Sergey Bronnikov
2020-07-12 15:37 ` Vladislav Shpilevoy
2020-07-13 9:51 ` Sergey Bronnikov
2020-07-13 22:59 ` Vladislav Shpilevoy
2020-06-04 8:39 ` [Tarantool-patches] [PATCH 6/6] Fix luacheck warnings in src/box/lua/ sergeyb
2020-06-06 18:03 ` Vladislav Shpilevoy
2020-06-11 12:01 ` Sergey Bronnikov
2020-06-18 9:37 ` Sergey Bronnikov
2020-06-19 23:15 ` Vladislav Shpilevoy
2020-07-01 14:02 ` Sergey Bronnikov
2020-07-13 22:59 ` Vladislav Shpilevoy
2020-07-14 10:54 ` Sergey Bronnikov
2020-07-14 11:24 ` Sergey Bronnikov
2020-06-04 8:43 ` [Tarantool-patches] [PATCH 0/6] Add static analysis of Lua code in src/ and extra/ with luacheck Sergey Bronnikov
2020-06-19 23:15 ` Vladislav Shpilevoy
2020-07-13 9:53 ` Sergey Bronnikov
2020-06-04 9:04 ` Igor Munkin
2020-06-04 11:17 ` Sergey Bronnikov
2020-07-14 22:49 ` Vladislav Shpilevoy
2020-07-15 9:51 ` Kirill Yukhin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox