feat: add check for modified constant variables (#116) (#109)

docsrc/cli.rst

@@ -120,6 +120,7 @@ Option                                  Meaning
 ``--enable | -e <patt> [<patt>] ...``   Do not filter out warnings matching patterns.
 ``--only | -o <patt> [<patt>] ...``     Filter out warnings not matching patterns.
 ``--operators <patt> [<patt>] ...``     Allow compound operators matching patterns. (Multiple assignment not supported, as this is specifically for the Playdate SDK.)
+``--const-loop-control-vars``           Flag loop control variables as <const>.
 ``--config <config>``                   Path to custom configuration file (default: ``.luacheckrc``).
 ``--no-config``                         Do not look up custom configuration file.
 ``--default-config <config>``           Default path to custom configuration file, to be used if ``--[no-]config`` is not used and ``.luacheckrc`` is not found.

docsrc/config.rst

@@ -43,6 +43,7 @@ Option                        Type                                     Default v
 ``new_read_globals``          Array of strings or field definition map (Do not overwrite)
 ``not_globals``               Array of strings                         ``{}``
 ``operators``                 Array of strings                         ``{}``
+``const_loop_control_vars``   Boolean                                  ``false``
 ``compat``                    Boolean                                  ``false``
 ``allow_defined``             Boolean                                  ``false``
 ``allow_defined_top``         Boolean                                  ``false``

docsrc/warnings.rst

@@ -44,6 +44,8 @@ Code Description
 431  Shadowing an upvalue.
 432  Shadowing an upvalue argument.
 433  Shadowing an upvalue loop variable.
+441  Constant local variable is modified.
+442  Loop control variable is modified.
 511  Unreachable code.
 512  Loop can be executed at most once.
 521  Unused label.

spec/linearize_spec.lua

@@ -404,3 +404,97 @@ g, a = f()]]))
       end)
    end)
 end)
+
+describe("constant modification detection", function()
+   it("detects modified constants", function()
+      assert.same({
+         {code = "441", line = 4, column = 1, end_column = 17, defined_line = 1, name = 'b'}
+      }, helper.get_stage_warnings("linearize", [[
+local a, b <const>, c = 1, 1, 1
+print(a, b, c)
+
+a, b, c = 2, 2, 2
+print(a, b, c)
+]]))
+   end)
+
+   it("detects a constant overwritten by a function", function()
+      assert.same({
+         {code = "441", line = 4, column = 1, end_column = 16, defined_line = 1, name = 'a'}
+      }, helper.get_stage_warnings("linearize", [[
+local a <const> = 1
+print(a)
+
+function a() end
+a()
+]]))
+   end)
+
+   it("doesn't trigger on a constant redefinition", function()
+      assert.same({
+         {code = "411", line = 4, column = 7, end_column = 7, name = 'a',
+            prev_column = 7, prev_end_column = 7, prev_line = 1}
+      }, helper.get_stage_warnings("linearize", [[
+local a <const> = 1
+print(a)
+
+local a = 1
+print(a)
+]]))
+   end)
+
+   it("doesn't trigger on a loop overriding a constant", function()
+      assert.same({
+         {code = "421", line = 4, column = 5, end_column = 5, name = 'a',
+            prev_column = 7, prev_end_column = 7, prev_line = 1}
+      }, helper.get_stage_warnings("linearize", [[
+local a <const> = 1
+print(a)
+
+for a = 1,10 do
+   print(a)
+end
+]]))
+   end)
+
+   it("handles variable scoping correctly", function()
+      assert.same({
+         {code = "421", line = 5, column = 10, end_column = 10, name = 'a',
+            prev_column = 7, prev_end_column = 7, prev_line = 1},
+         {code = "421", line = 5, column = 13, end_column = 13, name = 'b',
+            prev_column = 18, prev_end_column = 18, prev_line = 1},
+         {code = "441", line = 9, column = 1, end_column = 11, name = 'a',
+            defined_line = 1}
+      }, helper.get_stage_warnings("linearize", [[
+local a <const>, b = 1, 1
+print(a, b)
+
+do
+   local a, b <const> = 2, 2
+   print(a, b)
+end
+
+a, b = 3, 3
+print(a, b)
+]]))
+   end)
+
+   it("handles loop control variables correctly", function()
+      assert.same({
+         {code = "442", line = 2, column = 3, end_column = 11, name = 'i',
+            defined_line = 1},
+         {code = "442", line = 7, column = 3, end_column = 13, name = 'k',
+            defined_line = 6},
+      }, helper.get_stage_warnings("linearize", [[
+for i = 1, 10 do
+  i = i - 1
+  print(i)
+end
+
+for k,_ in pairs({ foo = "bar" }) do
+  k = "k:"..k
+  print(k)
+end
+]]))
+   end)
+end)

spec/parser_spec.lua

@@ -242,7 +242,7 @@ describe("parser", function()
    describe("when parsing for", function()
       it("parses fornum correctly", function()
          assert.same({tag = "Fornum",
-                        {tag = "Id", "i"},
+                        {tag = "Id", const_loop_var = true, "i"},
                         {tag = "Number", "1"},
                         {tag = "Op", "len", {tag = "Id", "t"}},
                         {}
@@ -283,7 +283,7 @@ describe("parser", function()
 
       it("parses fornum with step correctly", function()
          assert.same({tag = "Fornum",
-                        {tag = "Id", "i"},
+                        {tag = "Id", const_loop_var = true, "i"},
                         {tag = "Number", "1"},
                         {tag = "Op", "len", {tag = "Id", "t"}},
                         {tag = "Number", "2"},
@@ -297,14 +297,14 @@ describe("parser", function()
 
       it("parses forin correctly", function()
          assert.same({tag = "Forin", {
-                           {tag = "Id", "i"}
+                           {tag = "Id", const_loop_var = true, "i"}
                         }, {
                            {tag = "Id", "t"}
                         },
                         {}
                      }, get_node("for i in t do end"))
          assert.same({tag = "Forin", {
-                           {tag = "Id", "i"},
+                           {tag = "Id", const_loop_var = true, "i"},
                            {tag = "Id", "j"}
                         }, {
                            {tag = "Id", "t"},
@@ -492,14 +492,14 @@ describe("parser", function()
          )
       end)
 
-      it("accepts (and ignores for now) Lua 5.4 attributes", function()
+      it("accepts Lua 5.4 attributes, ignoring close and parsing const", function()
          assert.same({tag = "Local", {
                            {tag = "Id", "a"}
                         }
                      }, get_node("local a <close>"))
          assert.same({tag = "Local", {
                            {tag = "Id", "a"},
-                           {tag = "Id", "b"}
+                           {tag = "Id", const = true, "b"}
                         }
                      }, get_node("local a <close>, b <const>"))
          assert.same({
@@ -512,7 +512,7 @@ describe("parser", function()
          assert.same({
             tag = "Local", {
                {tag = "Id", "a"},
-               {tag = "Id", "b"}
+               {tag = "Id", const = true, "b"}
          }, {
                {tag = "Id", "c"},
                {tag = "Id", "d"}
@@ -522,6 +522,14 @@ describe("parser", function()
             {line = 1, offset = 16, end_offset = 16, msg = "expected '>' near '='"},
             get_error("local a <close = ")
          )
+         assert.same(
+            {line = 1, offset = 10, end_offset = 13, msg = "Unknown attribute 'cons'"},
+            get_error("local a <cons = ")
+         )
+         assert.same(
+            {line = 1, offset = 18, end_offset = 18, msg = "expected '(' near '<'"},
+            get_error("local function y <const> () end")
+         )
       end)
 
       it("parses local declaration with assignment correctly", function()
@@ -979,7 +987,7 @@ describe("parser", function()
             },
             {tag = "Do",
                {tag = "Fornum",
-                  {tag = "Id", "i"},
+                  {tag = "Id", const_loop_var = true, "i"},
                   {tag = "Number", "1"},
                   {tag = "Number", "2"},
                   {
@@ -990,7 +998,7 @@ describe("parser", function()
                },
                {tag = "Forin",
                   {
-                     {tag = "Id", "k"},
+                     {tag = "Id", const_loop_var = true, "k"},
                      {tag = "Id", "v"}
                   },
                   {

src/luacheck/parser.lua

@@ -719,14 +719,15 @@ statements["for"] = function(state)
 
    local ast_node = {}
    local tag
-   local first_var = parse_id(state)
+   local control_var = parse_id(state)
+   control_var.const_loop_var = true
 
    if state.token == "=" then
       -- Numeric "for" loop.
       tag = "Fornum"
       -- Skip "=".
       skip_token(state)
-      ast_node[1] = first_var
+      ast_node[1] = control_var
       ast_node[2] = parse_expression(state)
       check_and_skip_token(state, ",")
       ast_node[3] = parse_expression(state)
@@ -741,7 +742,7 @@ statements["for"] = function(state)
       -- Generic "for" loop.
       tag = "Forin"
 
-      local iter_vars = {first_var}
+      local iter_vars = {control_var}
       while test_and_skip_token(state, ",") do
          iter_vars[#iter_vars + 1] = parse_id(state)
       end
@@ -821,10 +822,16 @@ statements["local"] = function(state)
       lhs[#lhs + 1] = parse_id(state)
 
       -- Check if a Lua 5.4 attribute is present
+      -- TODO: Warn on different syntax between lua versions?
       if state.token == "<" then
-         -- For now, just consume and ignore it.
          skip_token(state)
-         check_name(state)
+         local attribute = check_name(state)
+         if attribute == "const" then
+            lhs[#lhs].const = true
+         -- Accept but ignore close
+         elseif attribute ~= "close" then
+            parser.syntax_error("Unknown attribute '" .. attribute .. "'", state)
+         end
          skip_token(state)
          check_and_skip_token(state, ">")
       end

src/luacheck/stages/linearize.lua

@@ -20,6 +20,15 @@ stage.warnings = {
    ["431"] = redefined_warning("shadowing upvalue {name!} on line {prev_line}"),
    ["432"] = redefined_warning("shadowing upvalue argument {name!} on line {prev_line}"),
    ["433"] = redefined_warning("shadowing upvalue loop variable {name!} on line {prev_line}"),
+   ["441"] = {
+      message_format = "variable {name!} was defined as const on line {defined_line}",
+      fields = {"name", "defined_line"}
+   },
+   ["442"] = {
+      message_format =
+         "variable {name!}, defined on line {defined_line}, is a loop control variable and shouldn't be modified",
+      fields = {"name", "defined_line"}
+   },
    ["521"] = {message_format = "unused label {label!}", fields = {"label"}}
 }
 
@@ -42,6 +51,14 @@ local function warn_redefined(chstate, var, prev_var, is_same_scope)
    })
 end
 
+local function warn_modified_const_label(chstate, node, var)
+   local code = "44" .. (var.node.const and "1" or "2")
+   chstate:warn_range(code, node, {
+      defined_line = var.node.line,
+      name = var.name
+   })
+end
+
 local function warn_unused_label(chstate, label)
    chstate:warn_range("521", label.range, {
       label = label.name
@@ -518,6 +535,14 @@ function LinState:emit_stmt_Set(node)
 
          if var then
             self:register_upvalue_action(item, var, "set_upvalues")
+
+            if var.node.const then
+               warn_modified_const_label(self.chstate, node, var)
+            end
+
+            if var.node.const_loop_var then
+               warn_modified_const_label(self.chstate, node, var)
+            end
          end
       else
          assert(expr.tag == "Index")

src/luacheck/standards.lua

@@ -128,7 +128,8 @@ local function add_fields(def, fields, overwrite, ignore_array_part, default_rea
       return
    end
 
-   for field_name, field_def in pairs(fields) do
+   for k, v in pairs(fields) do
+      local field_name, field_def = k, v
       if type(field_name) == "string" or not ignore_array_part then
          if type(field_name) ~= "string" then
             field_name = field_def