Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add a rule to detect useless computed property key #111

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
22 changes: 22 additions & 0 deletions docsrc/warnings.rst
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ Code Description
614 Trailing whitespace in a comment.
621 Inconsistent indentation (``SPACE`` followed by ``TAB``).
631 Line is too long.
701 Useless computed key
==== =============================================================================

Global variables (1xx)
Expand Down Expand Up @@ -294,3 +295,24 @@ Additionally, separate limits can be set for three different type of lines:

These types of lines are limited using CLI options named ``--[no-]max-string-line-length``, ``--[no-]max-comment-line-length``,
and ``--[no-]max-code-line-length``, with similar config and inline options.

Style issues (6xx)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

6xx as a header seems to be in conflict with the 7xx code described in the section

-----------------------

Useless computed key (701)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we do move ahead with this (I have some reservations about it being on by default) then we'd need to call it "unnecessary", not "useless".

^^^^^^^^^^^^^^^^^^^^^^^^^^

It’s unnecessary to use computed properties with literals such as:

.. code-block:: lua
:linenos:

local foo = {
["a"] = 0, -- bad
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a "computed property", there is no computation going on here. It is just a quoted value that can be quite handy when Lua can't parse it as a keyword. This is commonly used in generated code when the code generator can't know if the key is going to be an acceptable literal keyword or not.

At the very least calling these "computed" doesn't seem right. And I agree using them is sometimes unnecessary, but I don't think linting them as "bad" is going to be correct all the time.

}
foo["a"] -- bad

local foo = {
a = 0, -- good
}
foo.a -- good
53 changes: 53 additions & 0 deletions spec/no_useless_computed_key_spec.lua
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
local helper = require "spec.helper"

local function assert_warnings(warnings, src)
assert.same(warnings, helper.get_stage_warnings("detect_useless_computed_key", src))
end

describe("useless computed property key detection", function()
it("does not detect anything wrong key is a keyword", function()
assert_warnings({}, [[
aTable["and"] = 0
]])
end)

it("does not detect anything wrong key is not a string", function()
assert_warnings({}, [[
aTable[{}] = 0
]])
end)

it("does not detect anything wrong key start with a number", function()
assert_warnings({}, [[
aTable["1key"] = 0
]])
end)


it("detects useless computed key in table creation", function()
assert_warnings({
{code = "701", line = 2, column = 5, end_column = 11, name = "aKey1"},
}, [[
local aTable = {
["aKey1"] = 0
}
]])
end)

it("detects useless computed key when affecting a value", function()
assert_warnings({
{code = "701", line = 1, column = 8, end_column = 14, name = "aKey2"},
}, [[
aTable["aKey2"] = 0
]])
end)

it("detects useless computed key when accessing a value", function()
assert_warnings({
{code = "701", line = 1, column = 14, end_column = 20, name = "aKey3"},
}, [[
print(aTable["aKey3"])
]])
end)

end)
15 changes: 15 additions & 0 deletions spec/samples/useless_computed_key.lua
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
-- warn, could be written simply with "aKey = 0"
local aTable = {
["aKey"] = 0
}

-- warn, could be written simply with "aTable.aKey = 1"
aTable["aKey"] = 1

-- no warn, "and" is a keyword
aTable["and"] = 0

-- no warn, "1key" is not a valid name for key
aTable["1key"] = 0

print(aTable)
55 changes: 55 additions & 0 deletions src/luacheck/stages/detect_useless_computed_key.lua
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
local utils = require "luacheck.utils"

local stage = {}

local function useless_computed_key_message_format()
return "It's unnecessary to use computed properties with literals such as {name!}"
end

stage.warnings = {
["701"] = {message_format = useless_computed_key_message_format,
fields = {"name"}}
}

local function warn_useless_computed_key(chstate, node, symbol)
chstate:warn_range("701", node, {
name = symbol,
})
end

local keywords = utils.array_to_set({
"and", "break", "do", "else", "elseif", "end", "false", "for", "function", "goto", "if", "in",
"local", "nil", "not", "or", "repeat", "return", "then", "true", "until", "while"})

local function check_computed_key(chstate, key_node)
if key_node.tag == "String" then
local symbol = key_node[1]
if (key_node.end_offset - key_node.offset + 1) > #symbol then
if string.gmatch(symbol, "[%a_][%a%w_]*$")() == symbol and not keywords[symbol] then
warn_useless_computed_key(chstate, key_node, symbol)
end
end
end
end

local function check_nodes(chstate, nodes)
for _, node in ipairs(nodes) do
if type(node) == "table" then
if node.tag == "Pair" then
local key_node = node[1]
check_computed_key(chstate, key_node)
elseif node.tag == "Index" then
local key_node = node[2]
check_computed_key(chstate, key_node)
end

check_nodes(chstate, node)
end
end
end

function stage.run(chstate)
check_nodes(chstate, chstate.ast)
end

return stage
3 changes: 2 additions & 1 deletion src/luacheck/stages/init.lua
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,8 @@ stages.names = {
"detect_uninit_accesses",
"detect_unreachable_code",
"detect_unused_fields",
"detect_unused_locals"
"detect_unused_locals",
"detect_useless_computed_key"
}

stages.modules = {}
Expand Down