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

[Sergey Bronnikov <sergeyb@tarantool.org>]

Igor,

thanks for review, see my comments inline.
All changes are in branch.

On 19:54 Wed 27 May , Igor Munkin wrote:
> Sergey,
> 
> Thanks for the patch! There is a mess with suppressions (branch is OK),
> so I just left only relevant part.
> 
> On 12.05.20, sergeyb@tarantool.org wrote:
> > From: Sergey Bronnikov <sergeyb@tarantool.org>
> > 
> > Closes #4681
> > 
> > Reviewed-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
> > Co-authored-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
> > ---
> >  .luacheckrc | 42 ++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 42 insertions(+)
> > 
> > diff --git a/.luacheckrc b/.luacheckrc
> > index 3cd05a254..e5c4c4509 100644
> > --- a/.luacheckrc
> > +++ b/.luacheckrc
> > @@ -29,6 +29,48 @@ files["extra/dist/tarantoolctl.in"] = {
> >  	globals = {"box", "_TARANTOOL"},
> >  	ignore = {"212/self", "122", "431"}
> >  }
> > +files["**/*.lua"] = {
> 
> Again, spaces vs tabs.

Fixed.

> > +	globals = {"box", "_TARANTOOL", "help", "tutorial"},
> 
> OK, box and _TARANTOOL globals are widely used but AFAICS help and
> tutorial occurs only in src/lua/help.lua and src/box/lua/console.lua, so
> there is no need to suppress it source-wide.

Fixed.

> > +	ignore = {"212/self", "122", "143", "142"}
> 
> I see no warning with [122] and [142] code, so they seem to be excess
> here. Furthermore why do we need to suppress [143] globally if we don't
> use vanilla luacheck?

It is not so, we use vanilla luacheck, look at the changes in
.travis.mk. Anyway removed supressions for W122 and W142.

> > +}
> >  files["src/lua/*.lua"] = {ignore = {"212/self"}}
> 
> If unused self argument is suppressed source-wide, the suppression above
> can be dropped.

Agree, removed.

> >  files["src/lua/init.lua"] = {globals = {"dostring"}}
> >  files["src/lua/swim.lua"] = {ignore = {"431"}}
> > +files["src/box/lua/console.lua"] = {ignore = {"212"}}
> 
> Well, why do we have so much unused args here? Can we fix such warnings?
> Feel free to file a follow-up issue for this.

created issue [2]

> > +files["src/box/lua/load_cfg.lua"] = {ignore = {"542"}}
> 
> The suppression above is excess since the issue is fixed via inline one.

Removed.

> > +files["src/box/lua/net_box.lua"] = {ignore = {"431", "432", "411"}}
> 
> One general comment: please sort suppressed codes, it's just more
> convenient for further maintenance.

Done in a whole .luacheckrc.

> [411] warning can be fixed in a very simple way: just remove local prior
> to create_transport on line 919 (consider the diff at the end). As a
> result [411] suppression can be removed.

Proposed fix brokes tests, so I just supressed warning inline.

> > +files["src/box/lua/schema.lua"] = {globals = {"tonumber64"}, ignore = {"431", "432"}}
>
> Why do we need to explicitly set tonumber64 as global and does not set
> bit? Please choose one: either mention all globals used in sources to
> prepare the sources to be checked against vanilla luacheck or just omit
> such occurences if we're going to use our fork in CI.

Without 'tonumber64' in globals warning W113 triggered. See [1].

> As a result, after applying the following diff everything is OK:
> 
> ================================================================================
> 
> diff --git a/.luacheckrc b/.luacheckrc
> index a141e76ef..716f376b8 100644
> --- a/.luacheckrc
> +++ b/.luacheckrc
> @@ -30,13 +30,11 @@ files["extra/dist/tarantoolctl.in"] = {
>  	ignore = {"212/self", "122", "431"}
>  }
>  files["**/*.lua"] = {
> -	globals = {"box", "_TARANTOOL", "help", "tutorial"},
> -	ignore = {"212/self", "122", "143", "142"}
> +    globals = {"box", "_TARANTOOL"},
> +    ignore = {"212/self"},
>  }
> -files["src/lua/*.lua"] = {ignore = {"212/self"}}
>  files["src/lua/init.lua"] = {globals = {"dostring"}}
>  files["src/lua/swim.lua"] = {ignore = {"431"}}
> -files["src/box/lua/console.lua"] = {ignore = {"212"}}
> -files["src/box/lua/load_cfg.lua"] = {ignore = {"542"}}
> -files["src/box/lua/net_box.lua"] = {ignore = {"431", "432", "411"}}
> -files["src/box/lua/schema.lua"] = {globals = {"tonumber64"}, ignore = {"431", "432"}}
> +files["src/box/lua/console.lua"] = {globals = {"help"}, ignore = {"212"}}
> +files["src/box/lua/net_box.lua"] = {ignore = {"431", "432"}}
> +files["src/box/lua/schema.lua"] = {ignore = {"431", "432"}}
> diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
> index 3f2a9df81..646529173 100644
> --- a/src/box/lua/net_box.lua
> +++ b/src/box/lua/net_box.lua
> @@ -916,7 +916,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,
> +create_transport = function(host, port, user, password, callback,
>                                    connection, greeting)
>      local weak_refs = setmetatable({callback = callback}, {__mode = 'v'})
>      local function weak_callback(...)
> 
> ================================================================================
> 
> I checked the src/box/lua/*.lua sources with our luacheck fork:
> | $ tarantoolctl rocks install luacheck
> | $ $ .rocks/bin/luacheck --codes --config .luacheckrc src/box/lua/*.lua
> | Checking src/box/lua/console.lua                  OK
> | Checking src/box/lua/feedback_daemon.lua          OK
> | Checking src/box/lua/key_def.lua                  OK
> | Checking src/box/lua/load_cfg.lua                 OK
> | Checking src/box/lua/merger.lua                   OK
> | Checking src/box/lua/net_box.lua                  OK
> | Checking src/box/lua/schema.lua                   OK
> | Checking src/box/lua/session.lua                  OK
> | Checking src/box/lua/tuple.lua                    OK
> | Checking src/box/lua/upgrade.lua                  OK
> | Checking src/box/lua/xlog.lua                     OK
> |
> | Total: 0 warnings / 0 errors in 11 files
> 
> <snipped>
> 
> > -- 
> > 2.23.0
> > 
> 
> -- 
> Best regards,
> IM

[1] https://gitlab.com/tarantool/tarantool/-/jobs/571452395
[2] https://github.com/tarantool/tarantool/issues/5032