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

[Sergey Bronnikov <sergeyb@tarantool.org>]

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:

 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.

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

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:

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

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

-- 
sergeyb@