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

[Igor Munkin <imun@tarantool.org>]

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