Re: [Tarantool-patches] [PATCH] luacheck: change global vars to local in sql-tap

[Artem <artemreyt@tarantool.org>]

Sergey,

see my 5 answers below. Branch is updated.

10.11.2020 17:09, Sergey Bronnikov пишет:
> Artem,
>
> see my 5 comments below.
>
> --- a/.luacheckrc
> +++ b/.luacheckrc
> @@ -22,25 +22,60 @@ ignore = {
>   }
>
> 1. I don't get it why we should replace blacklist of files to whitelist.
> And there is unneeded change in config:
1. I explained the reason in commit message:

"It was bad idea to add deep directories to `exclude_files`,

because after that there is no way to enable files or child dirs

of this directories for processing by luacheck.

In this case, it was impossible to enable `test/sql-tap/` directory

because whole `test/` was in `exclude_files` and consequently ignored."

>   files["src/lua/help.lua"] = {
>       -- Globals defined for interactive mode.
>       globals = {"help", "tutorial"},
>   }
> +
>   files["src/lua/init.lua"] = {
>       -- Miscellaneous global function definition.
>       globals = {"dostring"},
>
>
> --- a/test/sql-tap/alias.test.lua
> +++ b/test/sql-tap/alias.test.lua
> @@ -1,5 +1,5 @@
>   #!/usr/bin/env tarantool
> -test = require("sqltester")
> +local test = require("sqltester")
>   test:plan(9)
>   
>   --!./tcltestrunner.lua
> @@ -24,6 +24,7 @@ test:plan(9)
>   -- A procedure to return a sequence of increasing integers.
>   --
>   
> +-- luacheck: globals counter
>   counter = 0
>
> 2. We decided to keep supression lines on the same line.
2. See 3rd comment of my previous message (it doesn't work with `globals`).
> --- a/test/sql-tap/analyzeF.test.lua
> +++ b/test/sql-tap/analyzeF.test.lua
> @@ -1,8 +1,8 @@
> 		 -where_clauses_x = {"x = 4 AND y = 19", "x = '4' AND y = '19'",
> 		 +local where_clauses_x = {"x = 4 AND y = 19", "x = '4' AND y = '19'",
> 		         x = substr('145', 2, 1) AND y = substr('5195', 2, 2)"}
> 				
> 				  -where_clauses_y = {"x = 19 AND y = 4", "x = '19' AND y = '4'",
> 				  +local where_clauses_y = {"x = 19 AND y = 4", "x = '19' AND y = '4'",
>
> 3. trailing spaces added
3. Removed
>
> 4. in a previous review I asked you to reduce some changes.
> For example in a file test/sql-tap/analyze5.test.lua
> you can easily reduce change to 2-lines change:
4. Fixed in other cases too.
> -            x = z
> -            w = z
> -            t = (z + 0.5)
> +            local x = z
> +            local w = z
> +            local t = (z + 0.5)
> +            local u
>               if z == 0 then
>
> to reduced patch:
>
> + local x, w, t
>    x = z
>    w = z
>    t = (z + 0.5)
> + local u
>    if z == 0 then
>
> In the same file:
>
>           function()
> -            w2 = v[1]:gsub('y', '+y'):gsub('z', '+z')
> -            a1 = test:execsql("SELECT id FROM t1 NOT INDEXED WHERE "..w2.." ORDER BY +id")
> -            a2 = test:execsql("SELECT id FROM t1 WHERE "..v[1].." ORDER BY +id")
> +            local w2 = v[1]:gsub('y', '+y'):gsub('z', '+z')
> +            local a1 = test:execsql("SELECT id FROM t1 NOT INDEXED WHERE "..w2.." ORDER BY +id")
> +            local a2 = test:execsql("SELECT id FROM t1 WHERE "..v[1].." ORDER BY +id")
> +            local res
>               if (test.is_deeply_regex(a1, a2))
>
> Why have you ignore comment?
>
> 5. What are the reasons of these changes?
5. It was my mistake, fixed.
> b/test/sql-tap/e_expr.test.lua
>
> -            local A = val[2]
> -            local B = val[3]
> -            local C = val[4]
> -            local testname = string.format("e_expr-1.%s.%s.%s", opname[op1], opname[op2], tn)
> +            local A, B, C, testname
> +            A = val[2]
> +            B = val[3]
> +            C = val[4]
> +            testname = string.format("e_expr-1.%s.%s.%s", opname[op1], opname[op2], tn)
>
>
> On 17:38 Mon 09 Nov , Artem wrote:
>> Thanks for your comments!
>>
>> Pushed fixes on https://github.com/tarantool/tarantool/tree/artemreyt/gh-5173-fix-luacheck-warnings-sql-tap
>>
>> Commented your notes below.
>>
>> 06.11.2020 02:46, Alexander Turenko writes:
>>> Thanks for the effort!
>> 1. I fixed those warnings, which were produced be `luacheck .`, now it's OK.
>>> Just `luacheck .` shows the following warnings, for example:
>>>
>>> test/sql-tap/minmax2.test.lua:64:9: setting non-standard global variable sql_search_count
>>> test/sql-tap/subquery.test.lua:696:9: setting non-standard global variable callcnt
>>>
>>> It seems, `make luacheck` target does not work.
>>>
>>>    | $ make VERBOSE=1 luacheck
>>>    | <...>
>>>    | /usr/bin/luacheck --codes --config                \
>>>    |     /home/alex/p/tarantool-meta/r/t-3/.luacheckrc \
>>>    |     /home/alex/p/tarantool-meta/r/t-3
>>>    | <...>
>>>
>>> If I run luacheck this way, it does not give any warnings. This does not
>>> look as the problem from your patch, but it is something we should fix
>>> prior it.
>> 2. Changed whitelist to blacklist and referred to gh-5464 here.
>>>> +files["test/sql-tap/**/*.lua"] = {
>>>> +    only = {
>>>> +        -- Setting an undefined global variable.
>>>> +        "111"
>>>> +    }
>>>> +}
>>>>
>>>> Can we mention further issues here (gh-5181 and gh-5464)? It would make
>>>> clear that this clause is necessary only as the temporary solution until
>>>> we'll actually fix more / all warnings. I'll be glad if you'll add a
>>>> list of warnings and its amount into the gh-5464 issue: it'll help with
>>>> futher estimation.
>>>>
>>>> Can we use a blacklist instead of the whitelist? This way we'll also
>>>> catch all standard warnings except the listed ones. It seems, there are
>>>> not so many warnings to blacklist:
>>>>
>>>>    | $ luacheck --codes test/sql-tap/*.lua test/sql-tap/*/*.lua | \
>>>>    |     grep -o '(W[0-9]\+)' | sort -u | cat -n
>>>>    |      1	(W111)
>>>>    |      2	(W113)
>>>>    |      3	(W211)
>>>>    |      4	(W212)
>>>>    |      5	(W213)
>>>>    |      6	(W231)
>>>>    |      7	(W311)
>>>>    |      8	(W511)
>>>>    |      9	(W512)
>>>>    |     10	(W542)
>>>>    |     11	(W611)
>>>>    |     12	(W612)
>>>>    |     13	(W613)
>>>>    |     14	(W614)
>>>>    |     15	(W621)
>>>>    |     16	(W631)
>>>>
>>>> I would ask you to comment each of them (like you do it for the
>>>> whitelist) and leave an issue numbers, where it is expected to be fixed.
>>>>
>>>> Some warnings that were not appear before, appear now:
>>>>
>>>>    | # Remove the suppression
>>>>    | $ git diff | cat
>>>>    | diff --git a/.luacheckrc b/.luacheckrc
>>>>    | index 6fbd1a5b3..e9a906f08 100644
>>>>    | --- a/.luacheckrc
>>>>    | +++ b/.luacheckrc
>>>>    | @@ -33,13 +33,6 @@ exclude_files = {
>>>>    |      "src/box/lua/serpent.lua",
>>>>    |  }
>>>>    |
>>>>    | -files["test/sql-tap/**/*.lua"] = {
>>>>    | -    only = {
>>>>    | -        -- Setting an undefined global variable.
>>>>    | -        "111"
>>>>    | -    }
>>>>    | -}
>>>>    | -
>>>>    |  files["src/lua/help.lua"] = {
>>>>    |      -- Globals defined for interactive mode.
>>>>    |      globals = {"help", "tutorial"},
>>>>    |
>>>>    | # Revert the changes in the test/ directory.
>>>>    | $ git show HEAD test >p && patch -R -p1 <p && rm p
>>>>    |
>>>>    | # Collect warnings.
>>>>    | $ luacheck --no-color --codes test/sql-tap/*.lua test/sql-tap/*/*.lua | \
>>>>    |     grep -o '(W[0-9]\+).*$' | sort | uniq -c | sort -rnk1,1 >before.txt
>>>>    |
>>>>    | # Apply back the changes in the test/ directory.
>>>>    | $ git checkout -- test
>>>>    |
>>>>    | # Collect the warnings again.
>>>>    | $ luacheck --no-color --codes test/sql-tap/*.lua test/sql-tap/*/*.lua | \
>>>>    |     grep -o '(W[0-9]\+).*$' | sort | uniq -c | sort -rnk1,1 >after.txt
>>>>    |
>>>>    | # Look at the changed counters.
>>>>    | $ diff -u before.txt after.txt | grep '^+'
>>>>    | +++ after.txt	2020-11-06 01:40:41.940542423 +0300
>>>>    | +     58 (W211) unused variable 'testprefix'
>>>>    | +     33 (W631) line is too long (132 > 120)
>>>>    | +     20 (W631) line is too long (126 > 120)
>>>>    | +      9 (W113) accessing undefined variable 'sql_search_count'
>>>>    | +      9 (W111) setting non-standard global variable 'sql_search_count'
>>>>    | +      8 (W113) accessing undefined variable 'sql'
>>>>    | +      7 (W211) unused variable 'i'
>>>>    | +      6 (W111) setting non-standard global variable 'counter'
>>>>    | +      4 (W211) unused variable 'json'
>>>>    | +      4 (W113) accessing undefined variable 'i'
>>>>    | +      3 (W211) unused variable 'yaml'
>>>>    | +      3 (W211) unused variable 's'
>>>>    | +      2 (W211) unused variable 'rc'
>>>>    | +      2 (W211) unused variable 'msg'
>>>>    | +      2 (W113) accessing undefined variable 'STMT'
>>>>    | +      2 (W113) accessing undefined variable 'res'
>>>>    | +      2 (W111) setting non-standard global variable 'callcnt'
>>>>    | +      1 (W231) variable 'V' is never accessed
>>>>    | +      1 (W231) variable 'c' is never accessed
>>>>    | +      1 (W231) variable 'b' is never accessed
>>>>    | +      1 (W231) variable 'a' is never accessed
>>>>    | +      1 (W211) unused variable 'view_v1'
>>>>    | +      1 (W211) unused variable 'sql_search_count'
>>>>    | +      1 (W211) unused variable 'prev'
>>>>    | +      1 (W211) unused variable 'NULL'
>>>>    | +      1 (W211) unused variable 'nm'
>>>>    | +      1 (W211) unused variable 'limit'
>>>>    | +      1 (W211) unused variable 'i0'
>>>>    | +      1 (W211) unused variable 'fio'
>>>>    | +      1 (W211) unused variable 'DB'
>>>>    | +      1 (W113) accessing undefined variable 'y'
>>>>    | +      1 (W113) accessing undefined variable 'stmt'
>>>>    | +      1 (W113) accessing undefined variable 'r'
>>>>    | +      1 (W113) accessing undefined variable 'DB'
>>>>    |
>>>>    | # Look more thoroughly.
>>>>    | $ diff -u before.txt after.txt | grep '^[-+]'
>>>>
>>>> Some of those new warnings looks suspectful, e.g. 'testprefix' should be
>>>> removed in several places instead of making it local. We should verify
>>>> all those cases. I would formulate the criteria as follows: a fix of a
>>>> warning should not produce any new warnings (even such as 'too long
>>>> line').
>>>>
>>>> test/sql-tap/e_expr.test.lua:
>>>>
>>>> -            local A = val[2]
>>>> -            local B = val[3]
>>>> -            local C = val[4]
>>>> -            local testname = string.format("e_expr-1.%s.%s.%s", opname[op1], opname[op2], tn)
>>>> +            local A, B, C, testname
>>>> +            A = val[2]
>>>> +            B = val[3]
>>>> +            C = val[4]
>>>> +            testname = string.format("e_expr-1.%s.%s.%s", opname[op1], opname[op2], tn)
>>> It seems, you changed those lines unintentionally.
>>>
>>> test/sql-tap/gh2250-trigger-chain-limit.test.lua:
>>>
>>>> -    function check(sql)
>>>> -        msg = ''
>>>> +local function check(sql)
>>>> +        local msg = ''
>>> The indentation of the 'local function' clause is broken.
>>>
>>> test/sql-tap/index7.test.lua:
>>>
>>>>    -- Capture the output of a pragma in a TEMP table.
>>>>    --
>>>> -local function capture_pragma(db, tabname, sql)
>>>> +local function capture_pragma(db, tabname, sql) -- luacheck: ignore once
>>>>        once = 1
>>>>    end
>>> The function is referred only from the commented code. There is no any
>>> reference to 'once' too. In fact, it is a kind of inaccurate converting
>>> from the original sqlite test ([1]). I would just remove it. If we'll
>>> implement partial indices and will enable related SQL tests, we'll
>>> anyway need to look into the original code.
>>>
>>> [1]: https://sqlite.org/src/file?name=test/index7.test
>>>
>>> test/sql-tap/trigger9.test.lua:
>> 3. Unfortunately, `globals` inline option doesn't work with variables one
>> the same line,
>>
>> so i was forced to use two lines in such places.
>>
>>>>    #!/usr/bin/env tarantool
>>>> +-- luacheck: globals test
>>>>    test = require("sqltester")
>>>>    test:plan(28)
>>> I all other places you add the luacheck directive on the same line as a
>>> corresponding Lua statement. Let's do this consistently.
>>>
>>> I'm okay with doing it in two lines if it becomes too long when written
>>> in one line (not this case).