mpeterv / luacheck

A tool for linting and static analysis of Lua code.
MIT License
1.92k stars 322 forks source link

Distinguish setting global functions from accidental global variables #151

Open FireSiku opened 6 years ago

FireSiku commented 6 years ago

As far as I can tell, these two:

function some_func(str, ...)
    print(str, ...)
end

for i = 1, #table do
    some_var = table[i]
    print(some_var)
end

Will both result in:

(W111) setting non-standard global variable 'some_func' (W111) setting non-standard global variable 'some_var'

While using globals is definitely not something you want to do, if you try to filter this warning, you will filter both cases. Which is a shame because they are vastly different. You might have a reason for setting that function in the global space. Missing a "local" keyword in a variable inside a loop or function is vastly different and should have its own warning.

Something like: (W111) setting non-standard global variable 'some_var' (W114) setting non-standard global function 'some_func'

mpeterv commented 6 years ago

I agree that it's more rare to forget local before a function definition than before a normal assignment, just because of the syntax. However, I think that reasons for setting global functions and other values are very similar, so I'm not sure about splitting the warning. Perhaps it's sufficient to use allow defined top option? It allows global definitions only in the top scope, where often mostly functions are defined.

FireSiku commented 6 years ago

It's not so much of an issue as it is a low priority feature request. It was just that, especially at the time I wrote that, I remember looking at the list of warning and finding some to be very close but still distinct cases. These two cases being tied to the same warning seemed really odd to me.

...I believe i was trying to search for some leaked global variable that was extensively used across a large project that had no prior linting and I couldnt guarantee that some variables werent defined in the top scope and also not supposed to be global. It's niche edge case at best though.

Allow defined top is sufficient in most cases though.