What should a GLua linter lint for

[FPtje]

[img]http://i.imgur.com/bFE9HcG.png[/img]

I'm making a GLua [url=https://en.wikipedia.org/wiki/Lint_(software)]linter[/url], but I'm kind of short on ideas on what it should check for.

Currently it checks for:
- Inconsistent use of C-style and Lua style syntax ([I]if true && true and true[/I])
- [url=http://wiki.garrysmod.com/page/Category:Deprecated_Functions]Deprecated functions[/url]
- Scope depth checking (when you have 5 nested scopes you're doing something wrong)
- Goto hating: using goto anywhere but a double nested loop makes you an idiot
- Duplicate keys in a table ({a = 1, a = 2, b = 3})
- self.Weapon in SWEPs/self.Entity in SENTs
- not (a == b), not (a ~= b), not (a > b) etc.
- Inconsistent " vs ' usage
- Variable/parameter/loop var/function name shadowing
- Using "self" in a non-metafunction
- Trailing whitespace
- Lack of whitespace around operators, after a keyword or after a ')'
- Inconsistent use of tabs and spaces for indentation (really basic, only looks for both "\n\t" and "\n " in whitespace)
- Empty conditions/loops

Ideas:
- Dead code checking/live variable analysis (very fucking difficult, I'll leave this for when I have a decent linter)
- Tail recursion, the Lua stack doesn't like that, especially with big things.

[B]What do you think are common errors/bad practices that can be caught with a lexical or Abstract Syntax Tree (AST) based linter? [/B]

(fyi, lexical means it only looks at the script like a list of tokens, when looking at the AST, it looks at the structure of the code)

One limitation: it's very hard to reason about whitespace because that's taken out before analysis starts. I'd have to recode half the thing to get whitespace in the analysis. Comments however are fine.
[B]Update:[/B] The limitation is now somewhat weaker: I can reason about whitespace in on the lexical level, but on scope level is still too hard (e.g. reasoning about code indentation)

[whitestar]

[QUOTE=FPtje;48465934]-snip-[/QUOTE]

Maybe check for not closed stuff eg missing "end", only ( or { and such

[FPtje]

[QUOTE=whitestar;48466055]Maybe check for not closed stuff eg missing "end", only ( or { and such[/QUOTE]

Syntax errors are caught by the parser, but syntax checking is something [url=http://facepunch.com/showthread.php?t=1442142]gluac[/url] already does. I don't want my linter to conflict with it.
Currently the linter doesn't output syntax errors.

[Rubat]

Is this an in-game thing or an external tool?

[Mijyuoon]

[QUOTE=Robotboy655;48466142]Is this an in-game thing or an external tool?[/QUOTE]

Most likely it's based on his GLua parser made in Haskell that he showed before.

[Willox]

"net.WriteEntity( LocalPlayer() )" in the same branch as "net.Start( ... )".

[FPtje]

[QUOTE=Robotboy655;48466142]Is this an in-game thing or an external tool?[/QUOTE]

External, I mean to implement it in sublimeLinter.

[QUOTE=mijyuoon;48466151]Most likely it's based on his GLua parser made in Haskell that he showed before.[/QUOTE]
Yes, very good.

[editline]15th August 2015[/editline]

[QUOTE=Willox;48466160]"net.WriteEntity( LocalPlayer() )" in the same branch as "net.Start( ... )".[/QUOTE]

Good one! That's the kind of bullshit practice that I need. I don't even think it needs to be in a net.Start thing. Seeing that anywhere is pretty much stupid.

[tommy228]

LocalPlayer():ConCommand ?

[Author.]

[QUOTE=tommy228;48466563]LocalPlayer():ConCommand ?[/QUOTE]

[lua]LocalPlayer():ConCommand("connect 127.0.0.1")
> connect 127.0.0.1

RunConsoleCommand("connect", "127.0.0.1")
> connect "127.0.0.1"[/lua]

Only way to connect to a server using Lua as far as I know.

[c👌de_g🎷]

self.Weapon in SWEPs

[tommy228]

Also, checking if a module is loaded before using require(), I'm pretty sure require() already does that
[lua]
if not module then
require("module")
end
[/lua]

[YesBacon]

You could make it check if a non-precached model is being spawned, then prompt you on the model location so you can easily precache it?

[I]I'm not even sure when to precache[/I]

[c👌de_g🎷]

[QUOTE=YesBacon;48468389]You could make it check if a non-precached model is being spawned, then prompt you on the model location so you can easily precache it?

[I]I'm not even sure when to precache[/I][/QUOTE]

This is an out-of-game tool; pretty impossible to check for that

[FPtje]

[QUOTE=tommy228;48466647]Also, checking if a module is loaded before using require(), I'm pretty sure require() already does that
[lua]
if not module then
require("module")
end
[/lua][/QUOTE]

How do I know what the module name is? Should I just throw a warning when I see an if with only one statement (require) and a simple condition?

[editline]16th August 2015[/editline]

[QUOTE=Willox;48466160]"net.WriteEntity( LocalPlayer() )" in the same branch as "net.Start( ... )".[/QUOTE]
[t]http://i.imgur.com/MOP6Vpw.png[/t]
[img]http://i.imgur.com/9ACPlvn.png[/img]

It is done.

[ARitz Cracker]

So are you going to give the programmer alternatives like "Use the 'ply' argument in the net.Recieve function instead" instead of yelling at the noob who doesn't know any better?

Also, (why == true)

[Author.]

[lua]not (a == b)[/lua]

[Lexic]

Obviously you'll need a config file for turning these on/off and customising them (per project .rc style please, so it checks each successive parent folder starting from the file merging every found config), but here's a collection of useful rules blatantly stolen from JSHint.

Inconsistent " vs ' usage. (pick one and stick to it)
Not using the key parameter in a for loop (set it to _)
Writing to a global without using _G (e.g. forgotten local)
Reading from a global without it being visible in the file or declared in a comment (--[[ global foo ]]-- etc)
Inconsistent indentation (Everything in file must be X spaces or a tab)
Trailing whitespace
Using a global function before it's defined
eg
[lua]function two()
one()
end

function one()
print("I work but it's evil!");
end[/lua]
Using common variable names (data, i, k, v whatever - let people customise this in their config file.)
Naming conventions (camelCase, with_underscores)
Empty blocks
Shadowing
eg
[lua]local i;
function foo()
local i;
end[/lua]
or
[lua]function bar(player)
end[/lua]
Unused local variables
Unused local functions
Using self in a function that doesn't define it via : or explicity
Using coroutines (unless they're fixed?)
Using non-ascii characters
Using if chains instead of early return/continue
OR (people have opinions yo)
Using early return/continue instead of ifs
eg
[lua]function blah()
local a = one();
if a then
local b = two();
if b then
three();
end
end
end[/lua]
vs
[lua]function blah()
local a = one();
if not a then
return
end
local b = two();
if not b then
return
end
three();
end[/lua]
and
[lua]for _, item in pairs(tab) do
if item.foo then
-- blah blah blah
end
end[/lua]
vs
[lua]for _, item in pairs(tab) do
if not item.foo then
continue;
end
-- blah blah blah
end[/lua]

I've been wanting a linter for Lua for a while but been far too lazy to do anything about it. Great job!

[Willox]

Coroutines work for anything that doesn't call in to a 3rd-party binary module that uses Garry's ILuaBase.

[FPtje]

[QUOTE=Lexic;48471088]Obviously you'll need a config file for turning these on/off and customising them (per project .rc style please, so it checks each successive parent folder starting from the file merging every found config), but here's a collection of useful rules blatantly stolen from JSHint.

LOADS OF STUFF

I've been wanting a linter for Lua for a while but been far too lazy to do anything about it. Great job![/QUOTE]

Holy shitballs that's a lot of useful ideas.
I'm not sure about rc files, though. I'd rather have any configuration done through command line parameters.

The attribute grammar is going to be huge!

[c👌de_g🎷]

[QUOTE=LegoGuy;48470978]So are you going to give the programmer alternatives like "Use the 'ply' argument in the net.Recieve function instead" instead of yelling at the noob who doesn't know any better?

Also, (why == true)[/QUOTE]

== true is useful for people who mix multiple data types on a single variable -- which you should never do, but without type declaration, is done a lot by people who have never programmed outside of a functional language.

[Lexic]

[QUOTE=FPtje;48471369]Holy shitballs that's a lot of useful ideas.
I'm not sure about rc files, though. I'd rather have any configuration done through command line parameters.

The attribute grammar is going to be huge![/QUOTE]

The RC files are incredibly important for open source projects, especially with the configurable settings.
If I want all strings in my project to use " and someone who prefers 's starts hacking on it, they should bow to my wishes if they want to send a pull request.
Additionally if I'm using Travis or some other CI system on Github, it should obviously use the same config as I do locally, and maintaining two identical configurations is :(

You don't have to go down the magical merging route if you don't want to, simply iterating up from the current file until you find one config file and using that as gold is just as good for most use cases.

[editline]16th August 2015[/editline]

Also - if I've disabled a default linter setting on my project for $reasons, I don't want to have to re-disable it for every computer I want to work on it (at least 3), and anyone else who uses my stuff will not know to disable it.

[ARitz Cracker]

[QUOTE=code_gs;48471694]== true is useful for people who mix multiple data types on a single variable -- which you should never do, but without type declaration, is done a lot by people who have never programmed outside of a functional language.[/QUOTE]

Agreed, but if the variable is never going to be anything other than a Boolean, == true is redundant and looks ugly.

[Ott]

[QUOTE=LegoGuy;48471870]Agreed, but if the variable is never going to be anything other than a Boolean, == true is redundant and looks ugly.[/QUOTE]

I use == false to check for nil.

[Wizard of Ass]

Magic numbers too please.

[editline]16th August 2015[/editline]

Also mixing index types in table declarations eg.

t = {1, 2, 3, dongs = 4}

Usually a sign of bad design.

[editline]16th August 2015[/editline]

Oh and warnings about unnecessary semicolons would be nice, I know they are used in some places to seperate instructions, must in 99% of the cases I've seen them, they were pretty pointless.

[editline]16th August 2015[/editline]

You are probably better of creating a gh repo and let us create tickets tho, you know how feature requests go out of hand in threads.

[FPtje]

[QUOTE=code_gs;48466641]self.Weapon in SWEPs[/QUOTE]
done

[QUOTE=Author.;48471007][lua]not (a == b)[/lua][/QUOTE]

done

[Lexic]

[QUOTE=Wizard of Ass;48472030]Oh and warnings about unnecessary semicolons would be nice, I know they are used in some places to seperate instructions, must in 99% of the cases I've seen them, they were pretty pointless.[/QUOTE]

Also a warning for [i]not[/i] ending each statement with a semicolon.

[FPtje]

[QUOTE=Wizard of Ass;48472030]Magic numbers too please.[/QUOTE]
You can't force people to put things like color numbers in var. This rule is way too strict.

[QUOTE=Wizard of Ass;48472030][editline]16th August 2015[/editline]

Also mixing index types in table declarations eg.

t = {1, 2, 3, dongs = 4}

Usually a sign of bad design.
[/quote]
Nice one.

[QUOTE=Wizard of Ass;48472030]
Oh and warnings about unnecessary semicolons would be nice, I know they are used in some places to seperate instructions, must in 99% of the cases I've seen them, they were pretty pointless.
[/QUOTE]
I already have warnings on double semicolons and commas. Semicolons after statements is a programmer's style choice. I think it's stupid too, but that's my opinion and not a universal truth.

[QUOTE=Wizard of Ass;48472030]
You are probably better of creating a gh repo and let us create tickets tho, you know how feature requests go out of hand in threads.[/QUOTE]
I have no open issues in any of my repositories (as of right now), not even in my DarkRP and FPP repositories. This fact satisfies me greatly. I know with the amount of suggestions I get here I won't be able to close issues as quickly.

Just post them in this thread, it works for me.

[Willox]

[QUOTE=Lexic;48472212]Also a warning for [i]not[/i] ending each statement with a semicolon.[/QUOTE]

Every option should be capitalize-able for the opposite effect!

[Wizard of Ass]

Not sure if that's on the list, add warnings when overriding default libraries/functions local and global.

I've seen many cases where people just declare globals and don't care if they override some native functions.

Next one might be opinionated:

Throw a warning when extending a default library, there almost never is a good reason for this.

[editline]16th August 2015[/editline]

Another probably opinionated:

throw a warning when you write

return nil

most of the time it's absolutely pointless.
throw a warning when declaring a variable as nil eg.

local a = nil

also pointless.

[editline]16th August 2015[/editline]

Also might be worth throwing a warning when using string.dump as it's result cannot be used.

[Willox]

Returning nil and returning nothing are surprisingly far from the same in the eyes of C-defined methods. This is because nil will end up on the stack and nothing won't.

[lua]local function a()
return
end

local function b()
return nil
end

print( a() ) -- no output
print( b() ) -- nil[/lua]

Try passing the results to type(), too: Real Lua will give you an error for no input whilst Garry's Mod outputs "no value".