Add luacheck config #1741

ghost · 2019-06-09T08:20:44Z

In the mumble developer meeting the lua linter luacheck was suggested for the CI pipeline.

As i played around with it, and already have some .luacheckrc snippets for Gluon i want to add the config file and try to fix warnings found by luacheck.

In the check_site.lua files the following issues where fixed

line is too long (137 > 120) -> what is the maximum line length that should be used in Gluon? 120 is the default by luacheck
unused loop variable k, v -> replaced by _
setting non-standard global variable from, to -> add local
line contains trailing whitespace -> removed

ghost · 2019-06-09T08:21:49Z

For check_site.lua one issue is left atm

$ luacheck scripts/check_site.lua 
Checking scripts/check_site.lua                   OK

Total: 0 warnings / 0 errors in 1 file
$ luacheck package/**/check_site.lua
Checking package/gluon-authorized-keys/check_site.lua OK
Checking package/gluon-autoupdater/check_site.lua OK
Checking package/gluon-client-bridge/check_site.lua OK
Checking package/gluon-config-mode-contact-info/check_site.lua OK
Checking package/gluon-config-mode-domain-select/check_site.lua OK
Checking package/gluon-config-mode-geo-location/check_site.lua OK
Checking package/gluon-config-mode-geo-location-osm/check_site.lua OK
Checking package/gluon-config-mode-hostname/check_site.lua OK
Checking package/gluon-core/check_site.lua        OK
Checking package/gluon-ebtables/check_site.lua    OK
Checking package/gluon-ebtables-source-filter/check_site.lua OK
Checking package/gluon-mesh-babel/check_site.lua  OK
Checking package/gluon-mesh-batman-adv/check_site.lua OK
Checking package/gluon-mesh-vpn-core/check_site.lua OK
Checking package/gluon-mesh-vpn-fastd/check_site.lua OK
Checking package/gluon-mesh-vpn-tunneldigger/check_site.lua OK
Checking package/gluon-node-info/check_site.lua   OK
Checking package/gluon-radv-filterd/check_site.lua OK
Checking package/gluon-scheduled-domain-switch/check_site.lua 1 warning

    package/gluon-scheduled-domain-switch/check_site.lua:1:45: (W113) accessing undefined variable check_domain_switch

Checking package/gluon-setup-mode/check_site.lua  OK
Checking package/gluon-web-admin/check_site.lua   OK
Checking package/gluon-web-mesh-vpn-fastd/check_site.lua OK
Checking package/gluon-web-node-role/check_site.lua OK

Total: 1 warning / 0 errors in 23 files

rubo77 · 2019-06-09T09:42:25Z

Already noticed here:
#1684 (comment)

mweinelt · 2019-06-09T11:48:18Z

Good catch, it's the subcheck that should be handed to need_table, but it does not exist, which need_table checks for, before executing it, hence no breakage.

ghost · 2019-06-09T14:10:14Z

Defined several globals per file/path (very simple, there is a more complex way to define read_globals and so on).
Included Lua-Scripts whiteout lua filename extension (maybe there are still some left)
Fixed several minor issues (set local, ..)

mweinelt · 2019-06-09T14:11:57Z

Included Lua-Scripts whiteout lua filename extension (maybe there are still some left)

How about adding lua extensions where possible?

cc @NeoRaider

mweinelt · 2019-06-09T14:14:06Z

line is too long (137 > 120) -> what is the maximum line length that should be used in Gluon? 120 is the default by luacheck

I think 120 characters is mighty fine these days.

...gluon-config-mode-geo-location-osm/luasrc/usr/lib/lua/gluon/config-mode/geo-location-osm.lua

ghost · 2019-06-15T08:23:09Z

As wished on IRC this PR should not mix fixes for warnings and the lua config file. I removed the "fix" part, and might open different PR for that, atm the opinions of the Gluon developers are missing how specific luacheck warnings should be handled (ignored, ..)

The config contains atm global definitions (global functions, variables, and so on), mostly.

neocturne · 2019-06-15T23:24:54Z

How about instead of listing all those globals in the config, we update our modules to define them in an explicit module table? This will be necessary anyways to become compatible with Lua 5.3.

ghost · 2019-06-16T08:20:34Z

@NeoRaider could you point to an example? Checking http://lua-users.org/wiki/ModulesTutorial it could be something like:

local mymodule = {}

function mymodule.foo()
    print("Hello World!")
end

return mymodule

and including the module

local mymodule = require "mymodule"
mymodule.foo()

Or do you have something different in mind?

neocturne · 2019-06-16T08:44:44Z

@NeoRaider could you point to an example? Checking http://lua-users.org/wiki/ModulesTutorial it could be something like:
local mymodule = {}

function mymodule.foo()
    print("Hello World!")
end

return mymodule

Yes, like this.

For including the module, nothing changes
Let's always call the module table M
This change will also get rid of module(...) calls at the top of Lua files (in fact, module() does not exist in newer Lua versions anymore)
We can also remove the annoying definitions for pulling global symbols into the local scope (like local io = io) before module(...), which was only a workaround to avoid using package.seeall

rubo77 · 2019-06-16T09:02:11Z

how do you apply the file .luacheckrc so it gives you those warnings?

I guess, just install and call like this:

apt install luacheck
luacheck .

How do you activate it in github as auto-check then? Do you want to use Travis?

ghost · 2019-06-16T09:23:20Z

What to do about the following issues? (running luacheck without a config file the following warnings show up)

Global variables (1xx)
https://luacheck.readthedocs.io/en/stable/warnings.html#global-variables-1xx
- (W111) setting non-standard global variable xyx
- (W112) mutating non-standard global variable xyz
  - Maybe as suggested by NeoRaider: rewrite Lua code to use local included modules
  - Maybe: change globally defined variables in scripts as local
- (W113) accessing undefined variable xyz
  - these are 'undefined' or better 'non-standard globals', fixing W111 is fixing this
- (W122) setting read-only field ? of global xyz
Unused variables (2xx) and values (3xx)
https://luacheck.readthedocs.io/en/stable/warnings.html#unused-variables-2xx-and-values-3xx
- (W211) unused variable xyz
- (W212) unused argument xyz
- (W213) unused loop variable xyz
  - Maybe: rename of the unused arguments and variables to _
  - Maybe: the arguments could be remove
  - ...
- (W311) value assigned to variable xyz is unused
- (W331) value assigned to variable xyz is mutated but never accessed
  - Maybe: removing the assigned value
  - ...
Shadowing declarations (4xx)
https://luacheck.readthedocs.io/en/stable/warnings.html#shadowing-declarations-4xx
- (W411) variable xyz was previously defined on line ...
- (W412) variable xyz was previously defined as an argument on line ...
- (W421) shadowing definition of variable xyz on line ...
- (W431) shadowing upvalue xyz on line ...
- (W432) shadowing upvalue argument xyz on line ...
  - Maybe: use different names for vars
  - Maybe: only define a var via local once
  - ...
Control flow and data flow issues (5xx)
https://luacheck.readthedocs.io/en/stable/warnings.html#control-flow-and-data-flow-issues-5xx
- (W542) empty if branch
  - Maybe: Remove and adjusting the following conditions
Formatting issues (6xx)
https://luacheck.readthedocs.io/en/stable/warnings.html#formatting-issues-6xx
- (W612) line contains trailing whitespace
  - removing those
- (W631) line is too long (> 120)
  - Maybe: max_line_length = false for ebtables files
  - Maybe: line breaks for the other long lines, but how to do these pretty?
  - ...

rubo77 · 2019-06-16T09:53:24Z

Maybe we should ignore all warnings for now and implement only a check for errors at first.
Later we can add more and more warnings to the output:

ignore = {
	"111", -- setting non-standard global variable xyx
	"112", -- mutating non-standard global variable xyz
	"113", -- accessing undefined variable xyz
	"121", -- setting read-only global variable table
	"122", -- setting read-only field ? of global xyz
	"142", -- setting undefined field optional of global table
	"143", -- accessing undefined field value of global table
	"211", -- unused variable xyz
	"212", -- unused argument xyz
	"213", -- unused loop variable xyz
	"231", -- variable value is never accessed
	"311", -- value assigned to variable xyz is unused
	"321", -- accessing uninitialized variable
	"331", -- value assigned to variable xyz is mutated but never accessed
	"411", -- variable xyz was previously defined
	"412", -- variable xyz was previously defined as an argument
	"421", -- shadowing definition of variable xyz
	"422", -- shadowing definition of argument
	"423", -- shadowing definition of loop variable
	"431", -- shadowing upvalue xyz
	"432", -- shadowing upvalue argument xyz
	"511", -- unreachable code
	"512", -- loop is executed at most once
	"531", -- right side of assignment has more values than left side expects
	"542", -- empty if branch
	"611", -- line contains only whitespace
	"612", -- line contains trailing whitespace
	"614", -- trailing whitespace in a comment
	"621", -- inconsistent indentation (SPACE followed by TAB)
	"631", -- line is too long (> 120)
}

Then we would only get those errors:

    openwrt/feeds/luci/applications/luci-app-ocserv/luasrc/model/cbi/ocserv/users.lua:29:27: (E011) invalid escape sequence '\$'
    openwrt/feeds/luci/applications/luci-app-travelmate/luasrc/model/cbi/travelmate/wifi_edit.lua:32:33: (E011) invalid escape sequence '\+'
    openwrt/feeds/luci/modules/luci-base/luasrc/cbi/datatypes.lua:417:26: (E011) invalid escape sequence '\*'
    openwrt/feeds/luci/modules/luci-base/luasrc/util.lua:208:34: (E011) invalid escape sequence '\-'
    openwrt/package/feeds/luci/luci-app-ocserv/luasrc/model/cbi/ocserv/users.lua:29:27: (E011) invalid escape sequence '\$'
    openwrt/package/feeds/luci/luci-app-travelmate/luasrc/model/cbi/travelmate/wifi_edit.lua:32:33: (E011) invalid escape sequence '\+'
    openwrt/package/feeds/luci/luci-base/luasrc/cbi/datatypes.lua:417:26: (E011) invalid escape sequence '\*'
    openwrt/package/feeds/luci/luci-base/luasrc/util.lua:208:34: (E011) invalid escape sequence '\-'
    packages/luci/applications/luci-app-ocserv/luasrc/model/cbi/ocserv/users.lua:29:27: (E011) invalid escape sequence '\$'
    packages/luci/applications/luci-app-travelmate/luasrc/model/cbi/travelmate/wifi_edit.lua:32:33: (E011) invalid escape sequence '\+'
    packages/luci/modules/luci-base/luasrc/cbi/datatypes.lua:417:26: (E011) invalid escape sequence '\*'
    packages/luci/modules/luci-base/luasrc/util.lua:208:34: (E011) invalid escape sequence '\-'

all no valid errors, because they are a bug in luacheck: mpeterv/luacheck#152

I added this in our Travis check for our site config: Freifunk-Nord/nord-site@a4aeb8a

works fine, thanks ;)

.luacheckrc

neocturne · 2019-06-16T13:35:49Z

@rubo77 Why are you even checking the luci feed? We don't use any packages from that feed, and there is no use in linting code that is not in our repo anyways.

All warnings in the Gluon packages are easy to fix - and some of them hint at real bugs.

neocturne · 2019-06-16T15:16:07Z

@bobcanthelpyou

Included Lua-Scripts whiteout lua filename extension (maybe there are still some left)

How about adding lua extensions where possible?

cc @NeoRaider

Yes, likely a good idea.

neocturne · 2019-06-16T15:37:42Z

@bobcanthelpyou Have you already fixed the issues in package/gluon-web/luasrc/usr/lib/lua/gluon/web/http/protocol.lua? I would really like to go through all of Gluon and fix the remaining luacheck issues, but now I can't see which you already took care of...

ghost · 2019-06-16T18:56:07Z

Added the fixes in a separate pull request #1748

Removed all globals which are not required anymore.

.luacheckrc

neocturne · 2019-06-16T23:03:28Z

I think most of the globals should actually be read_globals.

ghost · 2019-06-17T21:08:09Z

I misused new_read_globals (the docs say: Do not overwrite), switched to read_globals.

Added globals of ./targets and ./scripts

.luacheckrc

ghost · 2019-06-18T06:37:33Z

Squashed and rebased to master

.luacheckrc

rubo77 mentioned this pull request Jun 9, 2019

gluon-check-connection: extracted from gluon-scheduled-domain-switch #1684

Closed

mweinelt reviewed Jun 9, 2019

View reviewed changes

...gluon-config-mode-geo-location-osm/luasrc/usr/lib/lua/gluon/config-mode/geo-location-osm.lua Outdated Show resolved Hide resolved

rotanid requested a review from neocturne June 15, 2019 15:11

rotanid added the 0. type: enhancement The changeset is an enhancement label Jun 15, 2019

neocturne reviewed Jun 16, 2019

View reviewed changes