mpeterv / luacheck

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

Mutating a value in a table is not counted as "access" #156

Open mwchase opened 6 years ago

mwchase commented 6 years ago

Small reproduction:

local my_tables = {}
my_tables[1] = {}

my_tables[1][1] = 'Hello'

This produces the lint message: 1:7 warning luacheck: W241 variable 'my_tables' is mutated but never accessed

Rewriting to the mostly-identical

local my_tables = {}
my_tables[1] = {}

local intermediate = my_tables[1]

intermediate[1] = 'Hello'

eliminates the warning.

I would expect the my_tables[1] to count as an access whenever it's used as an expression, as in the first snippet.

mpeterv commented 6 years ago

The idea of the warning is that when a table is created, then something is added to it, but it's not used in any way, something is wrong (the whole table can be removed, perhaps except evaluation of some assigned values if they have side-effects). Luacheck considers t[k1][k2] = v a mutation of t, as it doesn't export or use its values, unlike an access. Accesses can still be useless in the same sense, like in the second snippet, but Luacheck doesn't do clever enough analysis to catch that.

mwchase commented 6 years ago

I'd argue that t[k1][k2] = v does use a value of t. It uses the value stored at k1. Get rid of the mutation that populates k1, and the assignment fails.

(The context I encountered this in wasn't "useless", but I didn't want to explain what I was doing, so I cut down the reproduction as much as possible. The original point was to provide access to several local tables from a loop body that had associated closures.)

mpeterv commented 6 years ago

What I mean is that the whole table is useless if the warning is issued, even though some assignments to fields allow further assignments to "deeper" fields, the table as a whole can be removed as it's not accessed externally.

If the warning is sometimes issued even though the table is accessed externally, please provide an example, that would be a bug.

mwchase commented 6 years ago

I was using the table to provide limited access to arbitrarily many upvalues (accessed by a proportional number of closures). So: a for loop creates an unknown number of scopes at runtime. Each scope has its own locals, including some tables. Each scope defines functions that operate on that scope's locals. When the scope is over, the functions are packaged up to be passed around, and the local tables are packaged up separately. They are stored in an external table that is indexed with the loop variable: action_at_a_distance_table[loop_variable] = {<LOCALS>}. The code uses action_at_a_distance_table to mutate the locals, through assignments to action_at_a_distance_table[loop_variable].local[key], which changes the local[key] visible to the associated closures. This mutation is done in a closure defined outside the for loop. As such, action_at_a_distance_table doesn't have to be returned from the topmost function, because it's an upvalue of the closure that is returned. This is even though the use the closure makes of it is purely on the left-hand side of assignments.

I'll maybe post the full code later; it has unrelated issues I want to fix first.

mpeterv commented 6 years ago

I see, so as I understand it, this is an example of the issue:

local function gen_accessors(t)
   local wrappers = {} -- Luacheck warning: variable wrappers is mutated but never accessed.
   local getters = {}

   for key, value in pairs(t) do
      local wrapper = {value}
      getters[key] = function() return wrapper[1] end
      wrappers[key] = wrapper
   end

   return getters, function(key, value) wrappers[key][1] = value end
end

local getters, setter = gen_accessors({foo = 1, bar = 2})
print(getters["foo"]()) -- Prints 1.
print(getters["bar"]()) -- Prints 2.
setter("foo", 3)
print(getters["foo"]()) -- Prints 3.

So in general, after t[k] = (some externally accessible value), mutations like t[k][k2] should cancel the warning. Thanks for reporting this!