[Tarantool-patches] [PATCH 0/6] Add static analysis of Lua code in src/ and extra/ with luacheck

[Tarantool-patches] [PATCH 0/6] Add static analysis of Lua code in src/ and extra/ with luacheck

[sergeyb]

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

[Tarantool-patches] [PATCH 1/6] Add initial luacheck config

[sergeyb]

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

Re: [Tarantool-patches] [PATCH 1/6] Add initial luacheck config

[Vladislav Shpilevoy]

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>

Re: [Tarantool-patches] [PATCH 1/6] Add initial luacheck config

[Sergey Bronnikov]

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@

[Tarantool-patches] [PATCH 2/6] build: enable 'make luacheck' target

[sergeyb]

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

[Tarantool-patches] [PATCH 3/6] gitlab-ci: enable static analysis with luacheck

[sergeyb]

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

[Tarantool-patches] [PATCH 4/6] Fix luacheck warnings in extra/dist/

[sergeyb]

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.
+}
+
 
 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
+    },
+}
diff --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

Re: [Tarantool-patches] [PATCH 4/6] Fix luacheck warnings in extra/dist/

[Vladislav Shpilevoy]

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 }.

Re: [Tarantool-patches] [PATCH 4/6] Fix luacheck warnings in extra/dist/

[Sergey Bronnikov]

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.

Re: [Tarantool-patches] [PATCH 4/6] Fix luacheck warnings in extra/dist/

[Vladislav Shpilevoy]

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.

Re: [Tarantool-patches] [PATCH 4/6] Fix luacheck warnings in extra/dist/

[Vladislav Shpilevoy]

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.

Re: [Tarantool-patches] [PATCH 4/6] Fix luacheck warnings in extra/dist/

[Vladislav Shpilevoy]

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.

Re: [Tarantool-patches] [PATCH 4/6] Fix luacheck warnings in extra/dist/

[Sergey Bronnikov]

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.

[Tarantool-patches] [PATCH 5/6] Fix luacheck warnings in src/lua/

[sergeyb]

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

Re: [Tarantool-patches] [PATCH 5/6] Fix luacheck warnings in src/lua/

[Vladislav Shpilevoy]

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)

Re: [Tarantool-patches] [PATCH 5/6] Fix luacheck warnings in src/lua/

[Sergey Bronnikov]

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@

Re: [Tarantool-patches] [PATCH 5/6] Fix luacheck warnings in src/lua/

[Vladislav Shpilevoy]

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.

Re: [Tarantool-patches] [PATCH 5/6] Fix luacheck warnings in src/lua/

[Sergey Bronnikov]

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@

Re: [Tarantool-patches] [PATCH 5/6] Fix luacheck warnings in src/lua/

[Sergey Bronnikov]

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>

Re: [Tarantool-patches] [PATCH 5/6] Fix luacheck warnings in src/lua/

[Igor Munkin]

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

Re: [Tarantool-patches] [PATCH 5/6] Fix luacheck warnings in src/lua/

[Vladislav Shpilevoy]

> 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))

Re: [Tarantool-patches] [PATCH 5/6] Fix luacheck warnings in src/lua/

[Sergey Bronnikov]

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))

Re: [Tarantool-patches] [PATCH 5/6] Fix luacheck warnings in src/lua/

[Vladislav Shpilevoy]

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

Re: [Tarantool-patches] [PATCH 5/6] Fix luacheck warnings in src/lua/

[Sergey Bronnikov]

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

Re: [Tarantool-patches] [PATCH 5/6] Fix luacheck warnings in src/lua/

[Vladislav Shpilevoy]

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

[Tarantool-patches] [PATCH 6/6] Fix luacheck warnings in src/box/lua/

[sergeyb]

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

Re: [Tarantool-patches] [PATCH 6/6] Fix luacheck warnings in src/box/lua/

[Vladislav Shpilevoy]

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)

Re: [Tarantool-patches] [PATCH 6/6] Fix luacheck warnings in src/box/lua/

[Sergey Bronnikov]

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)

Re: [Tarantool-patches] [PATCH 6/6] Fix luacheck warnings in src/box/lua/

[Sergey Bronnikov]

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>

Re: [Tarantool-patches] [PATCH 6/6] Fix luacheck warnings in src/box/lua/

[Vladislav Shpilevoy]

> 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.

Re: [Tarantool-patches] [PATCH 6/6] Fix luacheck warnings in src/box/lua/

[Sergey Bronnikov]

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@

Re: [Tarantool-patches] [PATCH 6/6] Fix luacheck warnings in src/box/lua/

[Vladislav Shpilevoy]

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?

Re: [Tarantool-patches] [PATCH 6/6] Fix luacheck warnings in src/box/lua/

[Sergey Bronnikov]

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/.

Re: [Tarantool-patches] [PATCH 6/6] Fix luacheck warnings in src/box/lua/

[Sergey Bronnikov]

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

Re: [Tarantool-patches] [PATCH 0/6] Add static analysis of Lua code in src/ and extra/ with luacheck

[Sergey Bronnikov]

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/

Re: [Tarantool-patches] [PATCH 0/6] Add static analysis of Lua code in src/ and extra/ with luacheck

[Vladislav Shpilevoy]

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

Re: [Tarantool-patches] [PATCH 0/6] Add static analysis of Lua code in src/ and extra/ with luacheck

[Sergey Bronnikov]

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@

Re: [Tarantool-patches] [PATCH 0/6] Add static analysis of Lua code in src/ and extra/ with luacheck

[Igor Munkin]

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

Re: [Tarantool-patches] [PATCH 0/6] Add static analysis of Lua code in src/ and extra/ with luacheck

[Sergey Bronnikov]

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/

Re: [Tarantool-patches] [PATCH 0/6] Add static analysis of Lua code in src/ and extra/ with luacheck

[Vladislav Shpilevoy]

The patchset LGTM.

Re: [Tarantool-patches] [PATCH 0/6] Add static analysis of Lua code in src/ and extra/ with luacheck

[Kirill Yukhin]

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