Compiler warning/error for duplicate variable declaration

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

Compiler warning/error for duplicate variable declaration

Domingo Alvarez Duarte
Hello !

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 .

In LJS I created a compiler warning/error that catch those problems at
compile time see patches that can be applied to Lua in
http://lua-users.org/wiki/LuaPowerPatches .

I hope more people would be aware of this problem and that it could be
eliminated by using a compiler help like the one proposed.

If you have any code in Lua that you care I invite you to compile a
patched version of lua and try run your code with it to see if you have
this problem in your code.

Any comment/suggestion is welcome !

Cheers !



Reply | Threaded
Open this post in threaded view
|

Re: Compiler warning/error for duplicate variable declaration

David Favro
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