Process initial Lua linter results & tweak its config

Dutchman101 commented 2 years ago

Lua linter from PR #354 (merged) will mostly be used to hint PR authors about potential mistakes & check off warnings before proceeding. But we also have some interesting results from the initial scan of our entire repository.

Tasks:

Identify the most common false positive warning types (due to MTA's Lua stack) so we can add more ignore rules, even after the last one we added: 7c135ae - the below warning log file will be helpful for this
Triage the warning log to identify and fix issues with official resources in mtasa-resources Warning log: https://pastebin.mtasa.com/999998690 or see attached .txt file: LuaLinter_results.txt

To work from: Repository tree (version: 7c135ae) to keep this log relevant for a while

Any help with either points will be appreciated. Especially skimming through warnings on our existing scripts and checking if they identified real problems, is time-consuming. If you have any candidates to be added to the ignore list, please comment them.

For a list of warning ID's and types see luacheck/warnings.rst

Disinterpreter commented 2 years ago

In addertum to D101. All linters and static analysis software not ideal.

So we recommend listen to them, but don't trust if the situation allows.

If you see the luacheck wrong in a specific situation, please use: https://luacheck.readthedocs.io/en/stable/inline.html?highlight=ignore#inline-options

Dutchman101 commented 2 years ago

So we recommend listen to them, but don't trust if the situation allows

yes, that's why i said: skimming through warnings on our existing scripts and checking if they identified real problems, all static analyzer output requires manual checking as most of them are false positive/not relevant

stoneage-mta commented 2 years ago

I've been checking for some time and so far i havent found any real problem... and IMHO we could add id 213 - (unused loop variable), and maybe all shadowing warnings (421, 422, 423, 431, 432, 433) to the ignore list.

stoneage-mta commented 2 years ago

It might also be worth adding 212 (unused argument) to the ignore list??? because in my opinion, in some cases like the one below, [example1] is much more newbie-friendly than [example2], and in the end it won't make any difference...

[example1]:

function bruh(weapon, ammo, ammoInClip, hitX, hitY, hitZ, hitElement)
    -- do something with hitElement

[example2]:

function bruh(_, _, _, _, _, _, hitElement)
    -- do something with hitElement

What do you think?

Disinterpreter commented 2 years ago

It might also be worth adding 212 (unused argument) to the ignore list??? because in my opinion, in some cases like the one below, [example1] is much more newbie-friendly than [example2], and in the end it won't make any difference...

[example1]:
function bruh(weapon, ammo, ammoInClip, hitX, hitY, hitZ, hitElement)
    -- do something with hitElement
[example2]:
function bruh(_, _, _, _, _, _, hitElement)
    -- do something with hitElement
What do you think?

For example, golang compiller deny the same things like unused variable/argument, so I think it is good. But about newbie, I think you are right. IMHO: _ is good.

There aren't right opinion I suppose.

ds1-e commented 2 years ago

It shorten lines, which in my opinion is better than having bunch of unused stuff, causing some lines to be hella longer.

Dutchman101 commented 2 years ago

It shorten lines, which in my opinion is better than having bunch of unused stuff, causing some lines to be hella longer.

Putting all arguments in the function is a very common practise in MTA (and already widely adopted in mtasa-resources).. not to speak about its value to beginning scripters. Because MTA differs from typical Lua in this aspect, it's related to our stack and those warnings should be ignored. I mean, i feel like conforming to this very common practise in MTA scripts takes priority over shortening lines.

Dutchman101 commented 2 years ago

There is some good feedback over at https://github.com/multitheftauto/mtasa-resources/pull/368#issuecomment-960816322, to be looked into after this stage (preferably with a new issue & project goal)

ArranTuna commented 2 years ago

Rule 213 (unused loop variable) needs adding because "for i, plr in ipairs(getElementsByType("player)) do" will warn on this unused loop variable on "i" which is just stupid. There are 1000 matches of this.

Rule 211 (unused local variable) with348 matches is just annoying too for example label1 is unused in this:

local label1 = guiCreateLabel ( xpos,       ypos, 50, 16,   "AC #" .. info.id,          false, tab )
local label2 = guiCreateLabel ( xpos+70,    ypos, 50, 16,   bEnabled and "ON" or "OFF", false, tab )
local label3 = guiCreateLabel ( xpos+110,   ypos, 450, 16,  "( "..info.desc.." )",      false, tab )

I don't see how anyone benefits from changing it to this when it looks less organised:

guiCreateLabel ( xpos,      ypos, 50, 16,   "AC #" .. info.id,          false, tab )
local label2 = guiCreateLabel ( xpos+70,    ypos, 50, 16,   bEnabled and "ON" or "OFF", false, tab )
local label3 = guiCreateLabel ( xpos+110,   ypos, 450, 16,  "( "..info.desc.." )",      false, tab )

TheNormalnij commented 2 years ago

Im used to organize large function calls like this

guiCreateLabel(
    xpos, ypos,
    50, 16,
    "AC #" .. info.id,
    false, tab
)

local smtStateLabel = guiCreateLabel(
    xpos + 70, ypos,
    50, 16,
    bEnabled and "ON" or "OFF",
    false, tab
)

local desctiptionLabel = guiCreateLabel(
    xpos + 110, ypos,
    450, 16,
    "( "..info.desc.." )",
    false, tab
)

It looks more readable and refactor-friendly. Variable names should have sense also. Rule 213 haven't sense for me.


for i = 1, 3 do -- << looks better
    -- smt
end

for _ = 1, 3 do
    -- smt
end

ArranTuna commented 2 years ago

611 | A line consists of nothing but whitespace.

This rule should be disabled too because if there is a blank line inside a function and that line is indented like it should be, this rule is triggered and I can't see any logic as to why it shouldn't be properly indented.

212 | Unused argument.

Everyone above agreed to disable this 1.

@Dutchman101 So if 221, 212, 213 and 611 are added to ignore it will clear 90% of the warnings and the ones left are much more likely to be stuff worth correcting.

TheNormalnij commented 2 years ago

611 | A line consists of nothing but whitespace.

This rule should be disabled too because if there is a blank line inside a function and that line is indented like it should be, this rule is triggered and I can't see any logic as to why it shouldn't be properly indented.

Modern IDE's remove "garbage" spaces and tabs when you go to new line. I see no problem with it.

212 | Unused argument.

Everyone above agreed to disable this 1.

I think this rule works well only with internal resource functions and looks bad with addEventHandler. This rule helps you when you refactor something, but i prefer to see all event handler arguments. And MTA resources are all about event handling

Dutchman101 commented 2 years ago

@Dutchman101 So if 221, 212, 213 and 611 are added to ignore it will clear 90% of the warnings and the ones left are much more likely to be stuff worth correcting

Done, see 6575451

yes, the results are more useful now.. https://github.com/multitheftauto/mtasa-resources/runs/4829699683

ArranTuna commented 2 years ago

Found 1 more that I think needs suppressing:

[admin]/admin/client/colorpicker/colorpicker.lua:185:3: loop is executed at most once

This is the code and it's not like can check if pickerTable[1] if it's not an iterated table.

function areThereAnyPickers() for _ in pairs(pickerTable) do return true end return false end

Fernando-A-Rocha commented 2 months ago

afaik can be closed because the lua linter has been working perfectly fine with MTA resources

multitheftauto / mtasa-resources

Process initial Lua linter results & tweak its config #365