diff --git a/docsrc/warnings.rst b/docsrc/warnings.rst index 5b0a79d5..fe99000e 100644 --- a/docsrc/warnings.rst +++ b/docsrc/warnings.rst @@ -32,7 +32,9 @@ Code Description 312 Value of an argument is unused. 313 Value of a loop variable is unused. 314 Value of a field in a table literal is unused. +315 Unused table field 321 Accessing uninitialized local variable. +325 Accessing unitialized table field 331 Value assigned to a local variable is mutated but never accessed. 341 Mutating uninitialized local variable. 411 Redefining a local variable. diff --git a/luacheck-dev-1.rockspec b/luacheck-dev-1.rockspec index 0389dd14..e6d867b5 100644 --- a/luacheck-dev-1.rockspec +++ b/luacheck-dev-1.rockspec @@ -74,6 +74,7 @@ build = { ["luacheck.stages.detect_empty_blocks"] = "src/luacheck/stages/detect_empty_blocks.lua", ["luacheck.stages.detect_empty_statements"] = "src/luacheck/stages/detect_empty_statements.lua", ["luacheck.stages.detect_globals"] = "src/luacheck/stages/detect_globals.lua", + ["luacheck.stages.check_table_fields"] = "src/luacheck/stages/check_table_fields.lua", ["luacheck.stages.detect_reversed_fornum_loops"] = "src/luacheck/stages/detect_reversed_fornum_loops.lua", ["luacheck.stages.detect_unbalanced_assignments"] = "src/luacheck/stages/detect_unbalanced_assignments.lua", ["luacheck.stages.detect_uninit_accesses"] = "src/luacheck/stages/detect_uninit_accesses.lua", diff --git a/spec/cli_spec.lua b/spec/cli_spec.lua index 4ebb90aa..5ad9e3ed 100644 --- a/spec/cli_spec.lua +++ b/spec/cli_spec.lua @@ -296,32 +296,36 @@ Total: 5 warnings / 0 errors in 1 file end) it("raises critical errors on config without additional operators", function() - assert.equal([[Checking spec/samples/compound_operators.lua 4 errors + assert.equal([[Checking spec/samples/compound_operators.lua 1 warning / 4 errors spec/samples/compound_operators.lua:2:1: assignment uses compound operator += spec/samples/compound_operators.lua:3:1: assignment uses compound operator -= spec/samples/compound_operators.lua:5:2: assignment uses compound operator /= + spec/samples/compound_operators.lua:9:3: value assigned to table field 't'.'a' is unused spec/samples/compound_operators.lua:10:1: assignment uses compound operator *= -Total: 0 warnings / 4 errors in 1 file +Total: 1 warning / 4 errors in 1 file ]], get_output "spec/samples/compound_operators.lua --no-config") end) it("raises critical errors for unfiltered additional operators", function() - assert.equal([[Checking spec/samples/compound_operators.lua 3 errors + assert.equal([[Checking spec/samples/compound_operators.lua 1 warning / 3 errors spec/samples/compound_operators.lua:3:1: assignment uses compound operator -= spec/samples/compound_operators.lua:5:2: assignment uses compound operator /= + spec/samples/compound_operators.lua:9:3: value assigned to table field 't'.'a' is unused spec/samples/compound_operators.lua:10:1: assignment uses compound operator *= -Total: 0 warnings / 3 errors in 1 file +Total: 1 warning / 3 errors in 1 file ]], get_output "spec/samples/compound_operators.lua --no-config --operators +=") end) it("allows to define allowed compound operators", function() - assert.equal([[Checking spec/samples/compound_operators.lua OK + assert.equal([[Checking spec/samples/compound_operators.lua 1 warning -Total: 0 warnings / 0 errors in 1 file + spec/samples/compound_operators.lua:9:3: value assigned to table field 't'.'a' is unused + +Total: 1 warning / 0 errors in 1 file ]], get_output "spec/samples/compound_operators.lua --config=spec/configs/compound_operators_config.luacheckrc") end) @@ -768,18 +772,19 @@ Total: 8 warnings / 3 errors in 2 files it("shows correct ranges for files with utf8", function() assert.equal([[ -Checking spec/samples/utf8.lua 4 warnings +Checking spec/samples/utf8.lua 5 warnings spec/samples/utf8.lua:2:1-4: setting undefined field '분야 명' of global 'math' spec/samples/utf8.lua:2:16-19: accessing undefined field '値' of global 'math' spec/samples/utf8.lua:3:25-25: unused variable 't' spec/samples/utf8.lua:4:5-28: value assigned to field 'päällekkäinen nimi a\u{200B}b' is overwritten on line 5 before use + spec/samples/utf8.lua:5:5-28: value assigned to table field 't'.'päällekkäinen nimi a​b' is unused Checking spec/samples/utf8_error.lua 1 error spec/samples/utf8_error.lua:2:11-11: expected statement near 'о' -Total: 4 warnings / 1 error in 2 files +Total: 5 warnings / 1 error in 2 files ]], get_output "spec/samples/utf8.lua spec/samples/utf8_error.lua --ranges --no-config") end) @@ -1235,7 +1240,7 @@ Codes: true assert.equal(([[ Checking spec/samples/argparse-0.2.0.lua 9 warnings Checking spec/samples/compat.lua 4 warnings -Checking spec/samples/compound_operators.lua 4 errors +Checking spec/samples/compound_operators.lua 1 warning / 4 errors Checking spec/samples/custom_std_inline_options.lua 3 warnings / 1 error Checking spec/samples/global_inline_options.lua 3 warnings Checking spec/samples/globals.lua 2 warnings @@ -1250,10 +1255,10 @@ Checking spec/samples/redefined.lua 7 warnings Checking spec/samples/reversed_fornum.lua 1 warning Checking spec/samples/unused_code.lua 10 warnings Checking spec/samples/unused_secondaries.lua 4 warnings -Checking spec/samples/utf8.lua 4 warnings +Checking spec/samples/utf8.lua 5 warnings Checking spec/samples/utf8_error.lua 1 error -Total: 75 warnings / 9 errors in 21 files +Total: 77 warnings / 9 errors in 21 files ]]):gsub("(spec/samples)/", "%1"..package.config:sub(1, 1)), get_output "spec/samples --config=spec/configs/exclude_files_config.luacheckrc -qq --exclude-files spec/samples/global_fields.lua") end) @@ -1262,7 +1267,7 @@ Total: 75 warnings / 9 errors in 21 files assert.equal([[ Checking argparse-0.2.0.lua 9 warnings Checking compat.lua 4 warnings -Checking compound_operators.lua 4 errors +Checking compound_operators.lua 1 warning / 4 errors Checking custom_std_inline_options.lua 3 warnings / 1 error Checking global_inline_options.lua 3 warnings Checking globals.lua 2 warnings @@ -1277,10 +1282,10 @@ Checking redefined.lua 7 warnings Checking reversed_fornum.lua 1 warning Checking unused_code.lua 10 warnings Checking unused_secondaries.lua 4 warnings -Checking utf8.lua 4 warnings +Checking utf8.lua 5 warnings Checking utf8_error.lua 1 error -Total: 75 warnings / 9 errors in 21 files +Total: 77 warnings / 9 errors in 21 files ]], get_output(". --config=spec/configs/exclude_files_config.luacheckrc -qq --exclude-files global_fields.lua", "spec/samples/")) end) @@ -1288,7 +1293,7 @@ Total: 75 warnings / 9 errors in 21 files assert.equal([[ Checking argparse-0.2.0.lua 9 warnings Checking compat.lua 4 warnings -Checking compound_operators.lua 4 errors +Checking compound_operators.lua 1 warning / 4 errors Checking custom_std_inline_options.lua 3 warnings / 1 error Checking global_inline_options.lua 3 warnings Checking globals.lua 2 warnings @@ -1301,10 +1306,10 @@ Checking redefined.lua 7 warnings Checking reversed_fornum.lua 1 warning Checking unused_code.lua 10 warnings Checking unused_secondaries.lua 4 warnings -Checking utf8.lua 4 warnings +Checking utf8.lua 5 warnings Checking utf8_error.lua 1 error -Total: 67 warnings / 9 errors in 19 files +Total: 69 warnings / 9 errors in 19 files ]], get_output(". --config=spec/configs/exclude_files_config.luacheckrc -qq --exclude-files global_fields.lua --exclude-files " .. quote("./read*"), "spec/samples/")) end) diff --git a/spec/table_field_limitations_spec.lua b/spec/table_field_limitations_spec.lua new file mode 100644 index 00000000..c148e596 --- /dev/null +++ b/spec/table_field_limitations_spec.lua @@ -0,0 +1,184 @@ +local helper = require "spec.helper" + +local function assert_warnings(warnings, src) + assert.same(warnings, helper.get_stage_warnings("check_table_fields", src)) +end + +describe("table field todo tests", function() + it("does nothing for globals", function() + assert_warnings({}, [[ +x = {} +x[1] = 1 +x[2] = x.y +x[1] = 1 + +y[1] = 1 +y[2] = x.y +y[1] = 1 + ]]) + end) + + it("can't parse complicated values out", function() + assert_warnings({}, [[ +local val = nil +local t = {} +t[1] = val +print(t[1]) + ]]) + end) + + it("does nothing for nested tables", function() + assert_warnings({}, [[ +local x = {} +x[1] = {} +x[1][1] = 1 +x[1][1] = x[1][2] +return x + ]]) + end) + + -- Because of possible multiple return + it("assumes tables initialized from functions can have arbitrary keys set", function() + assert_warnings({ + {code = "315", line = 3, column = 3, end_column = 3, name = 'x', field = 'y', set_is_nil = ''}, + }, [[ +local function func() return 1 end +local x = {func()} +x.y = x[2] + ]]) + end) + + it("does nothing for table parameters that aren't declared in scope", function() + assert_warnings({}, [[ +function func(x) + x[1] = x.z + x[1] = 1 +end + ]]) + end) + + it("doesn't handle metatables", function() + assert_warnings({}, [[ +local x = setmetatable({}, {}) +x[1] = 1 +print(x[2]) + ]]) + end) + + it("detects unused and undefined table fields inside control blocks, but not between them", function() + assert_warnings({ + {line = 4, column = 13, name = 'x', end_column = 13, field = 'z', code = '325', }, + {line = 10, column = 13, name = 'x', end_column = 13, field = 'z', code = '325', }, + {line = 16, column = 13, name = 'x', end_column = 13, field = 'z', code = '325', }, + {line = 22, column = 13, name = 'x', end_column = 13, field = 'z', code = '325', }, + {line = 28, column = 13, name = 'x', end_column = 13, field = 'z', code = '325', }, + {line = 34, column = 13, name = 'x', end_column = 13, field = 'z', code = '325', }, + }, [[ +do + local x = {} + x.y = 1 + x[1] = x.z +end + +if true then + local x = {} + x.y = 1 + x[1] = x.z +end + +while true do + local x = {} + x.y = 1 + x[1] = x.z +end + +repeat + local x = {} + x.y = 1 + x[1] = x.z +until false + +for i=1,2 do + local x = {} + x.y = 1 + x[1] = x.z +end + +for _,_ in pairs({}) do + local x = {} + x.y = 1 + x[1] = x.z +end + ]]) + end) + + it("stops checking referenced upvalues if function call is known to not have table as an upvalue", function() + assert_warnings({}, [[ +local x = {} +x[1] = 1 +local function printx() x = 1 end +local function ret2() return 2 end +ret2() +x[1] = 1 + +local y = {} +y[1] = 1 +function y.printx() y = 1 end +function y.ret2() return 2 end +y.ret2() +y[1] = 1 + ]]) + end) + + it("halts checking at the end of control flow blocks with jumps", function() + assert_warnings({}, [[ +local x = {1} +if math.rand(0,1) ~= 1 then + x = {} +end + +x[1] = x[1] + +local y = {1} +if math.random(0,1) == 1 then + y[1] = 2 +else + y = {} +end + +y[1] = y[1] + +local a = {1} +while math.random(0,1) == 1 do + a = {} +end + +a[1] = a[1] + ]]) + end) + + it("stops checking if a function is called", function() + assert_warnings({ + {line = 8, column = 3, name = 'y', end_column = 3, field = 'x', code = '315', set_is_nil = '' }, + {line = 8, column = 9, name = 'y', end_column = 9, field = 'a', code = '325', }, + {line = 14, column = 9, name = 't', end_column = 9, field = 'a', code = '325', }, + }, [[ +local x = {} +x.y = 1 +print("Unrelated text") +x.y = 2 +x[1] = x.z + +local y = {} +y.x = y.a +y.x = 1 +function y:func() return 1 end +y:func() + +local t = {} +t.x = t.a +local var = 'func' +t.x = y[var]() + 1 + ]]) + end) +end) \ No newline at end of file diff --git a/spec/table_fields_spec.lua b/spec/table_fields_spec.lua new file mode 100644 index 00000000..250dc00f --- /dev/null +++ b/spec/table_fields_spec.lua @@ -0,0 +1,438 @@ +local helper = require "spec.helper" + +local function assert_warnings(warnings, src) + assert.same(warnings, helper.get_stage_warnings("check_table_fields", src)) +end + +describe("table field checks", function() + it("detects unused and undefined table fields", function() + assert_warnings({ + {code = "315", line = 2, column = 3, end_column = 3, name = 'x', field = 'y', set_is_nil = ''}, + {code = "315", line = 3, column = 3, end_column = 3, name = 'x', field = 'y', set_is_nil = ''}, + {code = "315", line = 4, column = 3, end_column = 3, name = 'x', field = 1, set_is_nil = ''}, + {code = "325", line = 4, column = 10, end_column = 10, name = 'x', field = 'z'}, + {code = "325", line = 5, column = 9, end_column = 9, name = 'x', field = 'a'}, + {code = "315", line = 6, column = 12, end_column = 15, name = 'x', field = 'func', set_is_nil = ''}, + }, [[ +local x = {} +x.y = 1 +x.y = 2 +x[1] = x.z +x.a = x.a +function x.func() end + ]]) + end) + + it("detects complicated unused and undefined table fields", function() + assert_warnings({ + {line = 4, column = 11, name = 't', end_column = 11, field = 'b', code = '325', }, + {line = 10, column = 3, name = 'a', end_column = 3, field = 1, code = '315', set_is_nil = '' }, + }, [[ +local x = {1} +local t = {} +t.a = 1 +t.x = x[t.b] +x[t.a + 1] = x[t.x] + +local b = {} +b[1] = 1 +local a = {} +a[1] = {} +a[1][1] = 1 +a[2] = {} +a[1][b[1] + 1] = a[2][1] + ]]) + end) + + it("handles upvalue references after definition", function() + assert_warnings({}, [[ +local x = {} +x.y = 1 +function x.func() print(x) end + ]]) + end) + + it("handles upvalue references before definition", function() + assert_warnings({}, [[ +local x +function func() print(x[1]) end +x = {1} + ]]) + end) + + it("handles upvalues references in returned functions", function() + assert_warnings({}, [[ +function inner() + local x = {1} + return function() print(x[1]) end +end + ]]) + end) + + it("handles upvalue mutations", function() + assert_warnings({ + {code = "315", line = 2, column = 3, end_column = 3, name = 'x', field = 'y', set_is_nil = ''}, + {code = "315", line = 3, column = 12, end_column = 15, name = 'x', field = 'func', set_is_nil = ''}, + }, [[ +local x = {} +x.y = 1 +function x.func() x.z = 1 end + ]]) + end) + + it("handles upvalue sets", function() + assert_warnings({ + {code = "315", line = 2, column = 3, end_column = 3, name = 'x', field = 'y', set_is_nil = ''}, + {code = "315", line = 3, column = 12, end_column = 15, name = 'x', field = 'func', set_is_nil = ''}, + }, [[ +local x = {} +x.y = 1 +function x.func() x = 1 end + ]]) + end) + + it("handles complicated upvalue mutations", function() + assert_warnings({}, [[ +local x = {} +x[1] = {} +x[1][1] = function() x[1][1] = 2 end +x[1][1]() +print(x[1][1]) + ]]) + end) + + -- Handled separately, in detect_unused_fields + it("doesn't warn on duplicate keys in initialization", function() + assert_warnings({}, [[ +local x = {key = 1, key = 1} +local y = {1, [1] = 1} +return x,y + ]]) + end) + + it("doesn't warn on overwritten nil-sets in constructor", function() + assert_warnings({}, [[ +local t = {key = nil} +t.key = 1 +return t + ]]) + end) + + it("doesn't warn on unused fields of parameters", function() + assert_warnings({}, [[ +local function func(x) + x = {1} +end + ]]) + end) + + it("handles table assignments", function() + assert_warnings({}, [[ +function new_scope() + local c = {1} + return {key = c} +end + +function new_scope2() + local t = {} + t[1] = 1 + return { [t[1] ] = 1} +end + +local x = {1} +local y = {x} +local b = {key = y} +local a = {1} +a[b or x] = 1 +local d = {[a] = 1} +return {d} or {d} + ]]) + end) + + it("accounts for returned tables", function() + assert_warnings({ + {code = "315", line = 6, column = 3, end_column = 3, name = 't', field = 'x', set_is_nil = ''}, + }, [[ +local x = {} +x[1] = 1 +x.y = 1 +local t = {} +t.y = 1 +t.x = 1 +return x, t.y + ]]) + end) + + it("handles nested indexes correctly", function() + assert_warnings({ + {code = "315", line = 3, column = 3, end_column = 3, name = 'x', field = 'z', set_is_nil = ''}, + }, [[ +local x = {} +x.y = {} +x.z = {} +return x.y.z + ]]) + end) + + it("handles initialized tables", function() + assert_warnings({ + {code = "315", line = 1, column = 12, end_column = 12, name = 'x', field = 1, set_is_nil = ''}, + {code = "315", line = 1, column = 15, end_column = 15, name = 'x', field = 2, set_is_nil = ''}, + {code = "315", line = 1, column = 18, end_column = 18, name = 'x', field = 'a', set_is_nil = ''}, + {code = "315", line = 2, column = 3, end_column = 3, name = 'x', field = 1, set_is_nil = ''}, + {code = "325", line = 2, column = 10, end_column = 10, name = 'x', field = 'z'}, + {code = "315", line = 3, column = 3, end_column = 3, name = 'x', field = 'y', set_is_nil = ''} + }, [[ +local x = {1, 2, a = 3} +x[1] = x.z +x.y = 1 + ]]) + end) + + it("handles tables that are upvalues", function() + assert_warnings({ + {code = "325", line = 5, column = 13, end_column = 13, name = 'x', field = 'a'}, + }, [[ +local x + +function func() + x = {} + x[1] = x.a +end + +local t + +print(function() + t = {t.a} +end) + ]]) + end) + + it("handles table assignments to existing local variables", function() + assert_warnings({ + {code = "315", line = 3, column = 3, end_column = 3, name = 'x', field = 'y', set_is_nil = ''}, + {code = "315", line = 6, column = 3, end_column = 3, name = 'y', field = 'y', set_is_nil = ''}, + {code = "315", line = 8, column = 3, end_column = 3, name = 'y', field = 'y', set_is_nil = ''}, + }, [[ +local x +x = {} +x.y = 1 + +local y = {} +y.y = 1 +y = {} +y.y = 1 + ]]) + end) + + it("handles nil sets correctly", function() + assert_warnings({ + {line = 2, column = 3, name = 'x', end_column = 3, field = 'y', code = '315', set_is_nil = ''}, + {line = 3, column = 3, name = 'x', end_column = 3, field = 'y', code = '315', set_is_nil = 'nil '}, + {line = 5, column = 3, name = 'x', end_column = 3, field = 'z', code = '315', set_is_nil = ''}, + {line = 5, column = 9, name = 'x', end_column = 9, field = 'y', code = '325', }, + {line = 6, column = 3, name = 'x', end_column = 3, field = 'y', code = '315', set_is_nil = ''}, + }, [[ +local x = {} +x.y = 1 +x.y = nil +x.y = nil +x.z = x.y +x.y = 1 + ]]) + end) + + it("handles balanced multiple assignment correctly", function() + assert_warnings({ + {code = "325", line = 2, column = 22, end_column = 22, name = 't', field = 'b'}, + {code = "325", line = 3, column = 20, end_column = 20, name = 't', field = 'z'} + }, [[ +local t = {} +t.x, t.y, t.z = 1, t.b +return t.x, t.y, t.z + ]]) + end) + + it("handles multiple assignment of tables", function() + assert_warnings({ + {code = "315", line = 3, column = 3, end_column = 3, name = 'x', field = 'a', set_is_nil = ''}, + {code = "325", line = 4, column = 10, end_column = 10, name = 'b', field = 'c'}, + }, [[ +local x,y = {}, {} +local a,b = {}, {} +x.a = 1 +return b.c + ]]) + end) + + it("handles imbalanced multiple assignment correctly", function() + assert_warnings({ + {code = "315", line = 2, column = 3, end_column = 3, name = 't', field = 'x', set_is_nil = ''}, + {code = "325", line = 3, column = 10, end_column = 10, name = 't', field = 'y'}, + }, [[ +local t = {} +t.x, t.y = 1 +return t.y + ]]) + end) + + it("tables used as keys create a reference to them", function() + assert_warnings({}, [[ +local t = {} +local y = {1} +t[y or 3] = 1 +return t + ]]) + end) + + it("understands the difference between string and number keys", function() + assert_warnings({ + {code = "315", line = 2, column = 3, end_column = 3, name = 't', field = 1, set_is_nil = ''}, + {code = "315", line = 3, column = 3, end_column = 5, name = 't', field = '2', set_is_nil = ''}, + {code = "325", line = 3, column = 12, end_column = 14, name = 't', field = '1'}, + }, [[ +local t = {} +t[1] = 1 +t["2"] = t["1"] + ]]) + end) + + it("continues checking if the table variable itself is accessed without creating a reference", function() + assert_warnings({ + {code = "315", line = 2, column = 3, end_column = 3, name = 'x', field = 'y', set_is_nil = ''}, + {code = "315", line = 5, column = 3, end_column = 3, name = 'x', field = 'y', set_is_nil = ''} + }, [[ +local x = {} +x.y = 1 +local t = {1} +t[1] = t[x] +x.y = 1 + ]]) + end) + + it("warns on non-atomic key access to an entirely empty table", function() + assert_warnings({ + {code = "325", line = 3, column = 11, end_column = 13, name = 't2', field = '[Non-atomic key]'}, + {code = "325", line = 4, column = 11, end_column = 11, name = 't2', field = 't'}, + }, [[ +local t = {} +local t2 = {} +t[1] = t2[1+1] +t[2] = t2[t] +return t + ]]) + end) + + it("handles aliases correctly", function() + assert_warnings({ + {code = "315", line = 2, column = 3, end_column = 3, name = 'x', field = 1, set_is_nil = ''}, + {code = "325", line = 2, column = 10, end_column = 10, name = 'x', field = 'z'}, + {code = "315", line = 3, column = 3, end_column = 3, name = 'x', field = 'y', set_is_nil = ''}, + {code = "315", line = 6, column = 3, end_column = 3, name = 't', field = 'y', set_is_nil = ''}, + {code = "315", line = 7, column = 3, end_column = 3, name = 't', field = 1, set_is_nil = ''}, + {code = "325", line = 7, column = 10, end_column = 10, name = 't', field = 'z'}, + }, [[ +local x = {} +x[1] = x.z +x.y = 1 +x.x = 1 +local t = x +t.y = 1 +t[1] = t.z +t = t +x = t +return t.x + ]]) + end) + + it("an alias being overwritten doesn't end processing for the other aliases", function() + assert_warnings({ + {code = "315", line = 5, column = 3, end_column = 3, name = 'x', field = 1, set_is_nil = ''}, + }, [[ +local x = {} +local t = x +t[2] = 2 +t = 1 +x[1] = 1 +x[1] = 1 +return x, t + ]]) + end) + + it("any alias being externally referenced blocks unused warnings", function() + assert_warnings({}, [[ +local t +function inner() + local x = {1} + t = x +end + ]]) + end) + + it("handles rhs/lhs order correctly", function() + assert_warnings({ + {code = "325", line = 5, column = 10, end_column = 10, name = 'x', field = 3}, + }, [[ +local x = {} +x[1] = 1 +x[2] = 2 +x[1], x[2] = x[2], x[1] +x[3] = x[3] + +local t = {} +t[1] = 1 +t[t[1] ] = 2 +return x, t + ]]) + end) + + it("assumes that tables initialized from varargs can have arbitary keys set", function() + assert_warnings({ + {code = "315", line = 2, column = 3, end_column = 3, name = 'x', field = 'y', set_is_nil = ''}, + }, [[ +local x = {...} +x.y = x[2] + ]]) + end) + + it("catches unused writes after a non-atomic access", function() + assert_warnings({ + {code = "315", line = 6, column = 3, end_column = 3, name = 'x', field = 'y', set_is_nil = ''}, + {code = "315", line = 10, column = 3, end_column = 3, name = 'a', field = 'y', set_is_nil = ''}, + }, [[ +local var = 1 + +local x = {1} +local t = {} +t[1] = x[var] +x.y = 1 + +local a = {1} +t[2] = a[1 + 1] +a.y = 1 +return t + ]]) + end) + + it("accesses are not forever", function() + assert_warnings({ + {code = "315", line = 3, column = 3, end_column = 3, name = 'x', field = 2, set_is_nil = ''}, + {code = "315", line = 4, column = 3, end_column = 3, name = 'x', field = 1, set_is_nil = ''}, + }, [[ +local x = {} +x[1] = 1 +x[2] = x[1] +x[1] = 1 + ]]) + end) + + it("more complicated function calls", function() + assert_warnings({}, [[ +local t = {} +function t.func(var) print(var) end +local x = {} +x.y = 1 +t.func(x) + ]]) + end) +end) diff --git a/spec/unused_locals_spec.lua b/spec/unused_locals_spec.lua index 383e26d6..30c6e5ab 100644 --- a/spec/unused_locals_spec.lua +++ b/spec/unused_locals_spec.lua @@ -310,7 +310,7 @@ return a end) end) -describe("unused recurisve function detection", function() +describe("unused recursive function detection", function() it("detects unused recursive functions", function() assert_warnings({ {code = "211", name = "f", func = true, recursive = true, line = 1, column = 16, end_column = 16} diff --git a/src/luacheck/profiler.lua b/src/luacheck/profiler.lua index 37104a79..677da2c3 100644 --- a/src/luacheck/profiler.lua +++ b/src/luacheck/profiler.lua @@ -34,7 +34,8 @@ local functions = { {name = "run", module = "stages.detect_unused_fields"}, {name = "run", module = "stages.detect_unused_locals"}, {name = "filter", module = "filter"}, - {name = "normalize", module = "options"} + {name = "normalize", module = "options"}, + {name = "run", module = "stages.check_table_fields"}, } local stats = {} diff --git a/src/luacheck/stages/check_table_fields.lua b/src/luacheck/stages/check_table_fields.lua new file mode 100644 index 00000000..630c3740 --- /dev/null +++ b/src/luacheck/stages/check_table_fields.lua @@ -0,0 +1,491 @@ +local utils = require "luacheck.utils" + +local stage = {} + +stage.warnings = { + ["315"] = { + message_format = "{set_is_nil}value assigned to table field {name!}.{field!} is unused", + fields = {"set_is_nil", "name", "field"} + }, + ["325"] = {message_format = "table field {name!}.{field!} is not defined", fields = {"name", "field"}}, +} + +local function_call_tags = utils.array_to_set({"Call", "Invoke"}) + +local ClosureState = utils.class() + +function ClosureState:__init(chstate) + self.chstate = chstate + + -- Map from table name => table info. See new_local_table for the format. + self.current_tables = {} + + -- externally reference variable means that its fields could potentially all + -- be referenced externally + self.external_references_accessed = {} + + self.max_item = 0 + + -- Luacheck's linearized item format doesn't explicitly have a node for the "end" tag + -- This tracks where end tags would be, so that we can stop tracking + -- This pattern won't be workable if this eventually gets extended to support control flow + self.jump_destinations = {} +end + +-- Start keeping track of a local table +-- Can be from local x = {} OR "local x; x = {}" +function ClosureState:new_local_table(table_name) + self.current_tables[table_name] = { + -- set_keys sets store a mapping from key => {table_name, key_node, value_node} + -- the nodes store the line/column info + set_keys = {}, + -- accessed keys is a mappings from key => key_node + accessed_keys = {}, + -- For a variable key, it's impossible to reliably get the value; any given key could be set or accessed + -- potentially_all_* is set to the node responsible when truthy + potentially_all_set = nil, + potentially_all_accessed = nil, + -- Multiple variable names that point at the same underlying table + -- e.g. local x = {}; local t = x + aliases = {[table_name] = true}, + } +end + +-- Called when a table's field's value is no longer accessible +-- Either the table is gone, or the field has been overwritten +-- Table info can be different from the value of current_tables[table_name] +-- In the case that the original table was removed but an alias is still relevant +function ClosureState:maybe_warn_unused(table_info, key, set_data) + local set_table_name, set_node, assigned_val = set_data.table_name, set_data.key_node, set_data.assigned_node + local access_node = table_info.accessed_keys[key] + local all_access_node = table_info.potentially_all_accessed + -- Warn if there were definitely no accesses for this value + if (not access_node or access_node.line < set_node.line) + and (not all_access_node or all_access_node.line < set_node.line) + then + local original_key = set_node.tag == "Number" and tonumber(set_node[1]) or set_node[1] + self.chstate:warn_range("315", set_node, { + name = set_table_name, + field = original_key, + set_is_nil = assigned_val.tag == "Nil" and "nil " or "" + }) + end +end + +-- Called on accessing a table's field +function ClosureState:maybe_warn_undefined(table_name, key, range) + local table_info = self.current_tables[table_name] + -- Warn if the field is definitely not set + local set_data = table_info.set_keys[key] + local set_node, set_val + if set_data then + set_node, set_val = set_data.key_node, set_data.assigned_node + end + local all_set = table_info.potentially_all_set + if (not set_data and not all_set) + or (set_data and set_val.tag == "Nil" and (not all_set or set_node.line > all_set.line)) + then + self.chstate:warn_range("325", range, { + name = table_name, + field = key + }) + end +end + +-- Called on accessing a table's field with an unknown (variable or otherwise indecipherable) key +-- Can only warn if the table is known to be empty +function ClosureState:maybe_warn_undefined_var_key(table_name, var_key_name, range) + local table_info = self.current_tables[table_name] + -- Are there any non-nil keys at all? + if table_info.potentially_all_set then + return + end + for _, set_data in pairs(table_info.set_keys) do + if set_data.assigned_node.tag ~= "Nil" then + return + end + end + + self.chstate:warn_range("325", range, { + name = table_name, + field = var_key_name + }) +end + +-- Called when setting a new key for a known local table +function ClosureState:set_key(table_name, key_node, assigned_val, in_init) + local table_info = self.current_tables[table_name] + -- Constant key + if key_node.tag == "Number" or key_node.tag == "String" then + -- Don't warn about unused nil initializations + -- Fairly common to declare that a table should end up with fields set + -- by setting them to nil in the constructor + if in_init and assigned_val.tag == "Nil" then + return + end + local key = key_node[1] + if key_node.tag == "Number" then + key = tonumber(key) + end + -- Don't report duplicate keys in the init; other module handles that + if table_info.set_keys[key] and not in_init then + self:maybe_warn_unused(table_info, key, table_info.set_keys[key]) + end + -- Do note: just because a table's key has a value in set_keys doesn't + -- mean that it's not nil! variables, function returns, table indexes, + -- nil itself, and complex boolean conditions can return nil + -- set_keys tracks *specifically* the set itself, not whether the table's + -- field is non-nil + table_info.set_keys[key] = { + table_name = table_name, + key_node = key_node, + assigned_node = assigned_val + } + else + -- variable key + if assigned_val.tag ~= "Nil" then + table_info.potentially_all_set = key_node + end + end +end + +-- Called when indexing into a known local table +function ClosureState:access_key(table_name, key_node) + if key_node.tag == "Number" or key_node.tag == "String" then + local key = key_node[1] + if key_node.tag == "Number" then + key = tonumber(key) + end + self:maybe_warn_undefined(table_name, key, key_node) + self.current_tables[table_name].accessed_keys[key] = key_node + else + -- variable key + local var_key_name = key_node.var and key_node.var.name or "[Non-atomic key]" + self:maybe_warn_undefined_var_key(table_name, var_key_name, key_node) + self.current_tables[table_name].potentially_all_accessed = key_node + end +end + +-- Stop trying to track a table +-- We stop when: +-- * the variable is overwritten entirely +-- * the variable's scope ends +-- * we hit something that leaves us unable to usefully process +function ClosureState:wipe_table_data(table_name) + local info_table = self.current_tables[table_name] + for alias in pairs(info_table.aliases) do + self.current_tables[alias] = nil + end +end + +-- Called when a table variable is no longer accessible +-- i.e. the scope has ended or the variable has been overwritten +function ClosureState:end_table_variable(table_name) + local table_info = self.current_tables[table_name] + table_info.aliases[table_name] = nil + + if next(table_info.aliases) == nil then + for key, set_data in pairs(table_info.set_keys) do + self:maybe_warn_unused(table_info, key, set_data) + end + end + + self.current_tables[table_name] = nil +end + +-- Called on a new scope, including from a function call +-- Unlike end_table_variable, this assumes that any and all existing tables values +-- Can potentially be accessed later on, and so doesn't warn about unused values +function ClosureState:stop_tracking_tables() + for table_name in pairs(self.current_tables) do + self:wipe_table_data(table_name) + end +end + +function ClosureState:on_scope_end_for_var(table_name) + local table_info = self.current_tables[table_name] + local has_external_references = false + for alias in pairs(table_info.aliases) do + if self.external_references_accessed[alias] then + has_external_references = true + end + end + if has_external_references then + self:wipe_table_data(table_name) + else + self:end_table_variable(table_name) + end +end + +function ClosureState:on_scope_end() + for table_name in pairs(self.current_tables) do + self:on_scope_end_for_var(table_name) + end +end + +-- A function call leaves the current scope, and does potentially arbitrary modifications +-- To any externally referencable tables: either upvalues to other functions +-- Or parameters +function ClosureState:check_for_function_calls(node) + if node.tag ~= "Function" then + if function_call_tags[node.tag] then + self:stop_tracking_tables(node) + return true + end + + for _, sub_node in ipairs(node) do + if type(sub_node) == "table" then + if self:check_for_function_calls(sub_node) then + return true + end + end + end + end +end + +-- Records accesses to a specific key in a table +function ClosureState:record_field_accesses(node) + if node.tag ~= "Function" then + if node.tag == "Index" and node[1] then + local sub_node = node[1] + if sub_node.var and self.current_tables[sub_node.var.name] then + self:access_key(sub_node.var.name, node[2]) + end + end + for _, sub_node in ipairs(node) do + if type(sub_node) == "table" then + self:record_field_accesses(sub_node) + end + end + end +end + +-- Records accesses to the table as a whole, i.e. for table x, either t[x] = val or x = t +-- For the former, we stop tracking the table; for the latter, we mark x and t down as aliases if x is a local +-- For existing table t, in "local x = t", x is passed in as the aliased node +function ClosureState:record_table_accesses(node, aliased_node) + -- t[x or y] = val; x = t1 or t2 + if node[1] == "and" or node[1] == "or" then + for _, sub_node in ipairs(node) do + if type(sub_node) == "table" then + self:record_table_accesses(sub_node) + end + end + end + + -- t[{x}] = val; t = {x}; t = {[x] = val}; all keep x alive + if node.tag == "Table" then + for _, sub_node in ipairs(node) do + if sub_node.tag == "Pair" then + local key_node, val_node = sub_node[1], sub_node[2] + self:record_table_accesses(key_node) + self:record_table_accesses(val_node) + elseif sub_node.tag ~= "Nil" then + self:record_table_accesses(sub_node) + end + end + end + + local alias_info = nil + if node.var and self.current_tables[node.var.name] then + -- $lhs = $tracked_table + if aliased_node and aliased_node.var then + alias_info = {aliased_node.var.name, node.var.name} + else + -- assigned to a global; cannot usefully process + self:wipe_table_data(node.var.name) + end + end + + return alias_info +end + +-- Detects accesses to tables and table fields in item +-- For the case local $var = $existing table, returns a table +-- of multiple assignment index => {newly_set_var_name, existing_table_name} +function ClosureState:detect_accesses(sub_nodes, potential_aliases) + local alias_info = {} + for node_index, node in ipairs(sub_nodes) do + self:record_field_accesses(node) + alias_info[node_index] = self:record_table_accesses(node, potential_aliases and potential_aliases[node_index]) + end + return alias_info +end + +function ClosureState:handle_control_flow_item(item) + -- return gets linearized as a return control flow node followed by + -- an eval node of what got returned, followed by a jump + -- We want to defer the scope end processing until the jump so that + -- any accessed in the eval get processed + if not item.node or item.node.tag ~= "Return" then + self:stop_tracking_tables() + end +end + +function ClosureState:handle_local_or_set_item(item) + self:check_for_function_calls(item.node) + + -- Process RHS first, then LHS + -- When creating an alias, i.e. $new_var = $existing_var, need to store that info + -- and record it during LHS processing + local alias_info = {} + if item.rhs then + alias_info = self:detect_accesses(item.rhs, item.lhs) + end + + -- For imbalanced assignment with possible multiple return function + local last_rhs_node = false + for index, lhs_node in ipairs(item.lhs) do + local rhs_node = item.rhs and item.rhs[index] + if not rhs_node then + if last_rhs_node and function_call_tags[last_rhs_node.tag] then + rhs_node = last_rhs_node + else + -- Duck typing seems bad? + rhs_node = { + tag = "Nil" + } + end + else + last_rhs_node = rhs_node + end + + -- Case: $existing_table[key] = value + if lhs_node.tag == "Index" then + local base_node, key_node = lhs_node[1], lhs_node[2] + + -- Case: $var[$existing_table[key]] = value + -- Need to pass in a new array rather than using lhs_node, because that would + -- mark the base *set* as also being an access + self:detect_accesses({key_node}) + + -- Deliberately don't continue down indexes- $table[key1][key2] isn't a new set of key1 + if base_node.tag == "Id" then + -- Might not have a var if it's a global + local lhs_table_name = base_node.var and base_node.var.name + if self.current_tables[lhs_table_name] then + self:set_key(lhs_table_name, key_node, rhs_node, false) + end + end + end + + if alias_info[index] then + local new_var_name, existing_var_name = alias_info[index][1], alias_info[index][2] + self.current_tables[new_var_name] = self.current_tables[existing_var_name] + self.current_tables[new_var_name].aliases[new_var_name] = true + end + + -- Case: $existing_table = new_value + -- Complete overwrite of previous value + if lhs_node.var and self.current_tables[lhs_node.var.name] then + -- $existing_table = $existing_table should do nothing + if not (rhs_node.var + and self.current_tables[rhs_node.var.name] == self.current_tables[lhs_node.var.name]) + then + self:end_table_variable(lhs_node.var.name) + end + end + + -- Case: local $table = {} or local $table; $table = {} + -- New table assignment + if lhs_node.var and rhs_node.tag == "Table" then + local table_var = lhs_node.var + self:new_local_table(table_var.name) + for initialization_index, node in ipairs(rhs_node) do + if node.tag == "Pair" then + local key_node, val_node = node[1], node[2] + self:set_key(table_var.name, key_node, val_node, true) + elseif node.tag == "Dots" or node.tag == "Call" then + -- Vararg can expand to arbitrary size; + -- Function calls can return multiple values + self.current_tables[table_var.name].potentially_all_set = node + break + elseif node.tag ~= "Nil" then + -- Duck typing, meh + local key_node = { + [1] = initialization_index, + tag = "Number", + line = node.line, + offset = node.offset, + end_offset = node.end_offset + } + self:set_key(table_var.name, key_node, node, true) + end + end + end + end +end + +function ClosureState:handle_eval(item) + self:check_for_function_calls(item.node) + self:detect_accesses({item.node}) +end + +function ClosureState:handle_jump(item) + self.jump_destinations[item.to] = true + if item.to > self.max_item then + -- return; see comment under handle_control_flow_item + self:on_scope_end() + else + self:stop_tracking_tables() + end +end + +local item_callbacks = { + Noop = ClosureState.handle_control_flow_item, + Jump = ClosureState.handle_jump, + Cjump = ClosureState.handle_jump, + Eval = ClosureState.handle_eval, + Local = ClosureState.handle_local_or_set_item, + Set = ClosureState.handle_local_or_set_item, + OpSet = ClosureState.handle_local_or_set_item, +} + +-- Steps through the closure one item at a time +-- At each point, tracking for each local table which fields have been set +local function detect_unused_table_fields(closure, check_state) + local closure_state = ClosureState(check_state) + + local args = closure.node[1] + for _, parameter in ipairs(args) do + closure_state.external_references_accessed[parameter.var.name] = true + end + + -- Only need to check set_upvalues because we only track newly set tables + -- Inside the current scope + for var in pairs(closure.set_upvalues) do + closure_state.external_references_accessed[var.name] = true + end + + closure_state.max_item = #closure.items + + for item_index = 1, #closure.items do + if closure_state.jump_destinations[item_index] then + closure_state:stop_tracking_tables() + end + -- Function declaration: function could potentially survive this scope + -- Preserving a reference to its upvalues + local item = closure.items[item_index] + if item.lines then + for _,func_scope in pairs(item.lines) do + for var in pairs(func_scope.accessed_upvalues) do + closure_state.external_references_accessed[var.name] = true + end + end + end + item_callbacks[item.tag](closure_state, item) + end + + -- Handle implicit return + closure_state:on_scope_end() +end + +-- Warns about table fields that are never accessed +-- VERY high false-negative rate, deliberately in order to minimize the false-positive rate +function stage.run(check_state) + for _, closure in ipairs(check_state.lines) do + detect_unused_table_fields(closure, check_state) + end +end + +return stage diff --git a/src/luacheck/stages/init.lua b/src/luacheck/stages/init.lua index d8588785..acb376c9 100644 --- a/src/luacheck/stages/init.lua +++ b/src/luacheck/stages/init.lua @@ -15,6 +15,7 @@ stages.names = { "parse_inline_options", "name_functions", "resolve_locals", + "check_table_fields", "detect_bad_whitespace", "detect_compound_operators", "detect_cyclomatic_complexity",