lua-users home
lua-l archive

[Date Prev][Date Next][Thread Prev][Thread Next] [Date Index] [Thread Index]


On 12/27/2018 05:24 AM, Domingo Alvarez Duarte wrote:
Doing several conversions from Lua to LJS https://github.com/mingodad/ljs I found a frequent usage of duplicate variable declarations that make the code hard to understand/port and in some cases it causes bugs that pass unnoticed like this one in luajit https://www.freelists.org/post/luajit/Duplicated-variable-declaration-BUG .

These shadowed declarations are caught by luacheck which prints a warning, which I believe can be disabled on a instance-by-instance basis via a comment annotation. That this tool, available for years, is not more widely used is a pity: it typically catches these and many other types of smelly code.

Keep in mind that while shadowing names (either in the same scope or an inner scope) is generally not a good idea, and no doubt the cause of many sleeping bugs in existing codebases, it is currently legal and often deliberately used, so it's not always appropriate to treat it as an error or even a warning. Rightly or wrongly, it's not hard to find lots of code like:
local assert = assert;

So, having not examined your patch, if it does not include the ability to disable the warning, both globally and for particular instances, it is not going to sit well with some programmers.

As an aside, I would note that one frequent cause in my code (and perhaps also in dynasm apparently) is that when the rhs of '=' contains multiple values (e.g. function-call with multiple return values), and one wants to assign to both newly declared local variables and existing locals, upvalues, or _ENV globals, the local declaration must be separated from the assignment statement which looks awkward and less 'elegant':

local foo;
local function x()
  local bar,foo,gum = y();

This is either a mistake or prone to future bugs, but to avoid the shadowed name it becomes:

local foo;
local function x()
  local bar,gum;
  bar,foo,gum = y();

... which doesn't look as nice and seems to decrease the advantages of multiple-value assignment.

The various local-by-default and other previous proposals for syntax-based scoping might ameliorate this.

-- David