[Date Prev][Date Next][Thread Prev][Thread Next]
[Date Index]
[Thread Index]
- Subject: Re: Compiler warning/error for duplicate variable declaration
- From: David Favro <lua@...>
- Date: Thu, 27 Dec 2018 15:11:29 -0500
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