diff --git a/README.md b/README.md index f4faf13..d72a007 100644 --- a/README.md +++ b/README.md @@ -91,12 +91,21 @@ Each rule mirrors a well-known ShellCheck check. | [`no-expansions-in-single-quotes`](./docs/rules/no-expansions-in-single-quotes.md) | SC2016 | Expressions don't expand in single quotes | | warn | | [`no-ls-iteration`](./docs/rules/no-ls-iteration.md) | SC2045 | Don't iterate over `ls` output; use globs | | error | | [`no-unquoted-expansions`](./docs/rules/no-unquoted-expansions.md) | SC2086/46 | Quote expansions subject to word splitting and globbing | ✅ | error | +| [`no-unused-vars`](./docs/rules/no-unused-vars.md) | SC2034 | Disallow variables that are assigned but never used | | error | | [`no-useless-cat`](./docs/rules/no-useless-cat.md) | SC2002 | Don't pipe from a single-file `cat` | | error | | [`no-useless-echo`](./docs/rules/no-useless-echo.md) | SC2116 | Disallow `$(echo ...)` | | error | | [`no-variables-in-printf-format`](./docs/rules/no-variables-in-printf-format.md) | SC2059 | Don't put variables in the `printf` format string | | error | | [`require-cd-guard`](./docs/rules/require-cd-guard.md) | SC2164 | Handle `cd` failure with `\|\| exit` (has suggestions) | | error | | [`require-read-r`](./docs/rules/require-read-r.md) | SC2162 | Use `read -r` so backslashes aren't mangled | ✅ | error | +### Rule options + +`shell/no-unused-vars` accepts an object option: + +```js +"shell/no-unused-vars": ["error", { allowed: ["MY_GLOBAL"] }] +``` + ## Configuration comments Standard ESLint configuration comments work inside shell scripts: diff --git a/docs/rules/no-unused-vars.md b/docs/rules/no-unused-vars.md new file mode 100644 index 0000000..5bda02e --- /dev/null +++ b/docs/rules/no-unused-vars.md @@ -0,0 +1,94 @@ +# no-unused-vars + +Disallow variables that are assigned but never used. + +## Background + +A variable that is assigned but never read is usually a sign of a bug: a misspelled name (`colour=red` followed by `echo "$color"`), leftover code from a refactor, or a value that was meant to be used later. Removing unused variables also makes scripts easier to follow. + +## Rule Details + +This rule warns about variables that are assigned somewhere in the file but never read anywhere in the file. + +A variable counts as assigned by: + +- an assignment, including `declare`, `local`, `typeset`, and array element assignments such as `arr[1]=x`; +- the variable of a `for` loop; +- the variable names passed to `read`, `mapfile`, `readarray`, or `getopts`; +- an arithmetic assignment such as `(( total = 1 + 2 ))` or `(( count++ ))`. + +A variable counts as used when: + +- it is expanded with `$name` or `${...}`; +- it appears by name in an arithmetic context, as in `$((count + 1))`, an array subscript such as `arr[i]=x` or `arr=([i]=x)`, a slice such as `${str:start:len}`, or an arithmetic comparison such as `[[ count -gt 0 ]]`; +- it is passed to `unset`, `export`, `readonly`, or `declare -p`; +- it is exported, with `export` or `declare -x`; +- it is assigned as an environment prefix to a command, as in `LC_ALL=C sort`. + +The rule never warns about: + +- names that start with `_`; +- special variables that the shell or common tools read, such as `IFS`, `PATH`, `PS1`, `OPTIND`, `REPLY`, and the `LC_*` variables; +- names listed in the `allowed` option. + +If a file uses `eval`, `source` (or `.`), or a nameref (`declare -n`), this rule doesn't report anything in that file, because variables may be read in ways that static analysis can't see. + +The rule analyzes the whole file at once and doesn't model function scope. For example, an unused `local x` inside a function isn't reported if some other `x` is read elsewhere in the file. Indirect references such as `${!ref}` count as a use of `ref` only, not of the variable it points to. + +Examples of **incorrect** code for this rule: + +```bash +# eslint shell/no-unused-vars: "error" + +count=0 + +read -r first second + +for i in 1 2 3; do echo "hello"; done + +(( total = price * 2 )) +``` + +Examples of **correct** code for this rule: + +```bash +# eslint shell/no-unused-vars: "error" + +name="world" +echo "Hello, $name" + +read -r first _rest +echo "$first" + +for i in 1 2 3; do echo "$i"; done + +export API_URL=https://example.com + +LC_ALL=C sort data.txt + +IFS=, + +_ignored=1 +``` + +## Options + +This rule has one object option: + +- `allowed` (default: `[]`): an array of variable names to never report. This is useful for variables that are read by other scripts that source this one. + +Examples of **correct** code for this rule with `{ "allowed": ["VERSION"] }`: + +```bash +# eslint shell/no-unused-vars: ["error", { "allowed": ["VERSION"] }] + +VERSION=1.2.3 +``` + +## When Not to Use It + +Scripts that are meant to be sourced by other scripts often define variables for the caller to use. The rule can't see those uses, so it reports the variables as unused. For such files, use the `allowed` option, prefix the names with `_`, or disable this rule. + +## Prior Art + +- [SC2034](https://www.shellcheck.net/wiki/SC2034) diff --git a/src/index.spec.ts b/src/index.spec.ts index 50c4f1b..d3f67c2 100644 --- a/src/index.spec.ts +++ b/src/index.spec.ts @@ -39,6 +39,7 @@ describe("plugin", () => { "no-expansions-in-single-quotes", "no-ls-iteration", "no-unquoted-expansions", + "no-unused-vars", "no-useless-cat", "no-useless-echo", "no-variables-in-printf-format", diff --git a/src/index.ts b/src/index.ts index c50dae2..1976df3 100644 --- a/src/index.ts +++ b/src/index.ts @@ -8,6 +8,7 @@ import noBackticks from "./rules/no-backticks.js"; import noExpansionsInSingleQuotes from "./rules/no-expansions-in-single-quotes.js"; import noLsIteration from "./rules/no-ls-iteration.js"; import noUnquotedExpansions from "./rules/no-unquoted-expansions.js"; +import noUnusedVars from "./rules/no-unused-vars.js"; import noUselessCat from "./rules/no-useless-cat.js"; import noUselessEcho from "./rules/no-useless-echo.js"; import noVariablesInPrintfFormat from "./rules/no-variables-in-printf-format.js"; @@ -19,6 +20,7 @@ const rules = { "no-expansions-in-single-quotes": noExpansionsInSingleQuotes, "no-ls-iteration": noLsIteration, "no-unquoted-expansions": noUnquotedExpansions, + "no-unused-vars": noUnusedVars, "no-useless-cat": noUselessCat, "no-useless-echo": noUselessEcho, "no-variables-in-printf-format": noVariablesInPrintfFormat, @@ -49,6 +51,7 @@ const plugin = { "shell/no-expansions-in-single-quotes": "warn", "shell/no-ls-iteration": "error", "shell/no-unquoted-expansions": "error", + "shell/no-unused-vars": "error", "shell/no-useless-cat": "error", "shell/no-useless-echo": "error", "shell/no-variables-in-printf-format": "error", diff --git a/src/rules/no-unused-vars.spec.ts b/src/rules/no-unused-vars.spec.ts new file mode 100644 index 0000000..a9083bc --- /dev/null +++ b/src/rules/no-unused-vars.spec.ts @@ -0,0 +1,163 @@ +/** + * @fileoverview Tests for the no-unused-vars rule. + */ + +import { RuleTester } from "eslint"; +import { ShellLanguage } from "../languages/shell-language.js"; +import rule from "./no-unused-vars.js"; + +const ruleTester = new RuleTester({ + plugins: { + shell: { + meta: { namespace: "shell" }, + languages: { bash: new ShellLanguage() }, + }, + // eslint-disable-next-line @typescript-eslint/no-explicit-any -- plugin shape is validated by ESLint at runtime. + } as any, + language: "shell/bash", +}); + +ruleTester.run("no-unused-vars", rule as never, { + valid: [ + // Used in an expansion + 'x=1\necho "$x"', + // Used unquoted + "x=1\necho $x", + // Used in arithmetic without $ + "x=1\necho $((x + 1))", + // Used in a (( )) command + "x=1\n(( x > 0 )) && echo big", + // Written by read and used + 'read -r line\necho "$line"', + // Exported variables are used externally + "export TOKEN=abc", + "declare -x TOKEN=abc", + // Environment prefix assignments are passed to the command + "LC_ALL=C sort file", + // Special shell variables configure behavior + 'IFS=","', + "PS1='$ '", + // Underscore prefix opts out + "_unused=1", + // The allowed option opts out + { + code: "custom=1", + options: [{ allowed: ["custom"] }], + }, + // For loop variable used in body + 'for f in a b; do echo "$f"; done', + // eval makes analysis unreliable, so nothing is reported + 'x=1\neval "echo \\$x"', + // getopts variable used + 'while getopts "ab:" opt; do echo "$opt"; done', + // ${x:=default} counts as a use + "x=1\n: ${x:=2}", + // unset counts as a use + "x=1\nunset x", + // Array literal subscripts are arithmetic contexts + 'i=0\narr=([i]=value)\necho "${arr[@]}"', + // Slice offsets and lengths are arithmetic contexts + 'start=1\nlen=2\ns=abcdef\necho "${s:start:len}"', + // Subscripts inside arithmetic are read + "arr=(1 2)\ni=0\necho $(( arr[i] ))", + // Arithmetic comparisons in [[ ]] read bare names + "x=1\n[[ x -gt 0 ]] && echo big", + "x=1\ny=2\n[[ ! ( x -eq y ) ]] && echo diff", + // readonly and declare -p reference existing variables + "x=1\nreadonly x", + "x=1\ndeclare -p x", + // Any LC_* variable configures the locale + "LC_TIME=C", + // read -a assigns the named array + 'read -r -a parts\necho "${parts[@]}"', + // mapfile -C takes a callback, not a variable name + 'mapfile -t -C handler lines\necho "${lines[@]}"', + ], + invalid: [ + { + code: "unused=1", + errors: [ + { + messageId: "unusedVariable", + data: { name: "unused" }, + line: 1, + column: 1, + endColumn: 7, + }, + ], + }, + { + code: 'used=1\nunused=2\necho "$used"', + errors: [ + { + messageId: "unusedVariable", + data: { name: "unused" }, + line: 2, + }, + ], + }, + { + code: "read -r line", + errors: [ + { + messageId: "unusedVariable", + data: { name: "line" }, + }, + ], + }, + { + code: "for f in a b; do echo static; done", + errors: [ + { + messageId: "unusedVariable", + data: { name: "f" }, + }, + ], + }, + { + code: "declare count=0", + errors: [ + { + messageId: "unusedVariable", + data: { name: "count" }, + }, + ], + }, + { + code: "read -r -a parts", + errors: [ + { + messageId: "unusedVariable", + data: { name: "parts" }, + }, + ], + }, + { + code: "x=1\nexport -n x", + errors: [ + { + messageId: "unusedVariable", + data: { name: "x" }, + }, + ], + }, + { + code: "x=1\n[[ x == 1 ]] && echo one", + errors: [ + { + messageId: "unusedVariable", + data: { name: "x" }, + }, + ], + }, + { + code: "(( total = 1 + 2 ))", + errors: [ + { + messageId: "unusedVariable", + data: { name: "total" }, + }, + ], + }, + ], +}); diff --git a/src/rules/no-unused-vars.ts b/src/rules/no-unused-vars.ts new file mode 100644 index 0000000..775b8c7 --- /dev/null +++ b/src/rules/no-unused-vars.ts @@ -0,0 +1,461 @@ +/** + * @fileoverview Rule to flag variables that are assigned but never used. + * Mirrors ShellCheck SC2034. + */ + +import { getCommandName, getStaticText } from "./utils.js"; +import type { + ArithmeticExpressionNode, + ShellRuleDefinition, + IdentifierNode, + TestExpressionNode, + WordNode, +} from "../types.js"; + +/** Variables that are meaningful to the shell or to common external tools. */ +const SPECIAL_VARIABLES = new Set([ + "BASH_ENV", + "CDPATH", + "EDITOR", + "ENV", + "GLOBIGNORE", + "HISTCONTROL", + "HISTFILE", + "HISTFILESIZE", + "HISTSIZE", + "HOME", + "IFS", + "LANG", + "LD_LIBRARY_PATH", + "OPTARG", + "OPTERR", + "OPTIND", + "PAGER", + "PATH", + "PROMPT_COMMAND", + "PS1", + "PS2", + "PS3", + "PS4", + "REPLY", + "TERM", + "TMOUT", + "TZ", + "VISUAL", +]); + +const IDENTIFIER = /^[A-Za-z_][A-Za-z0-9_]*/u; + +/** `read` flags that consume the following word as their argument. */ +const READ_FLAGS_WITH_ARGUMENT = /^-[a-zA-Z]*[dinNptu]$/u; + +/** `mapfile`/`readarray` flags that consume the following word. */ +const MAPFILE_FLAGS_WITH_ARGUMENT = /^-[a-zA-Z]*[cCdnOsu]$/u; + +/** Test operators whose operands are evaluated arithmetically. */ +const ARITHMETIC_TEST_OPERATORS = new Set([ + "-eq", + "-ne", + "-lt", + "-le", + "-gt", + "-ge", +]); + +/** Arithmetic operators that assign to their left operand. */ +const ARITHMETIC_ASSIGNMENT = /^(?:=|\+=|-=|\*=|\/=|%=|<<=|>>=|&=|\^=|\|=)$/u; + +const rule: ShellRuleDefinition<{ + MessageIds: "unusedVariable"; + RuleOptions: [{ allowed?: string[] }?]; +}> = { + meta: { + type: "problem", + languages: ["shell/bash", "shell/posix", "shell/mksh"], + docs: { + description: "Disallow variables that are assigned but never used", + recommended: true, + dialects: ["Bash", "POSIX sh", "mksh"], + url: "https://github.com/eslint/shell/blob/main/docs/rules/no-unused-vars.md", + }, + schema: [ + { + type: "object", + properties: { + allowed: { + type: "array", + items: { type: "string" }, + }, + }, + additionalProperties: false, + }, + ], + defaultOptions: [{}], + messages: { + unusedVariable: + "Variable '{{name}}' is assigned but never used. (ShellCheck SC2034)", + }, + }, + + create(context) { + const [{ allowed = [] } = {}] = context.options; + const allowedNames = new Set(allowed); + const writes = new Map(); + const reads = new Set(); + + // Set when the file uses constructs that can reference variables + // dynamically (eval, source), making usage analysis unreliable. + let unsafe = false; + + function addWrite(name: string, node: IdentifierNode): void { + if (!writes.has(name)) { + writes.set(name, node); + } + } + + function addReadableIdentifier(word: WordNode): void { + const text = getStaticText(word); + const match = text === null ? null : IDENTIFIER.exec(text); + + if (match) { + reads.add(match[0]); + } + } + + /** + * Records reads/writes for bare identifiers inside arithmetic + * expressions, where `x` references the variable without `$`. + */ + function scanArithmetic( + expression: ArithmeticExpressionNode, + isWrite: boolean, + ): void { + switch (expression.type) { + case "Word": { + const text = getStaticText(expression); + const match = text === null ? null : IDENTIFIER.exec(text); + + if (match && match[0] === text) { + if (isWrite) { + const identifier: IdentifierNode = { + type: "Identifier", + start: expression.start, + end: expression.end, + name: text as string, + }; + + addWrite(text as string, identifier); + } else { + reads.add(match[0]); + } + } + break; + } + + case "BinaryArithmetic": + if (ARITHMETIC_ASSIGNMENT.test(expression.operator)) { + scanArithmetic(expression.left, true); + + // Compound assignments also read the variable. + if (expression.operator !== "=") { + scanArithmetic(expression.left, false); + } + } else { + scanArithmetic(expression.left, false); + } + + scanArithmetic(expression.right, false); + break; + + case "UnaryArithmetic": + if ( + expression.operator === "++" || + expression.operator === "--" + ) { + scanArithmetic(expression.argument, true); + } + + scanArithmetic(expression.argument, false); + break; + + case "ParenthesizedArithmetic": + scanArithmetic(expression.expression, false); + break; + + // no default + } + } + + /** + * Records reads for bare identifiers used as operands of arithmetic + * comparisons in `[[ ... ]]`, such as `[[ x -gt 0 ]]`. + */ + function scanTest(expression: TestExpressionNode): void { + switch (expression.type) { + case "BinaryTest": + if (ARITHMETIC_TEST_OPERATORS.has(expression.operator)) { + for (const operand of [ + expression.left, + expression.right, + ]) { + if (operand.type === "Word") { + scanArithmetic(operand, false); + } + } + } else { + scanTest(expression.left); + scanTest(expression.right); + } + break; + + case "UnaryTest": + scanTest(expression.argument); + break; + + case "ParenthesizedTest": + scanTest(expression.expression); + break; + + // no default + } + } + + return { + VariableAssignment(node) { + if (node.name) { + const parent = context.sourceCode.getParent(node); + + // Environment prefixes such as `FOO=1 cmd` are passed to + // the command and therefore used. + if (parent?.type === "Command" && parent.name !== null) { + reads.add(node.name.name); + } + + addWrite(node.name.name, node.name); + } + + if (node.index) { + scanArithmetic(node.index, false); + } + }, + + ArrayElement(node) { + if (node.index) { + scanArithmetic(node.index, false); + } + }, + + ParameterExpansion(node) { + reads.add(node.name); + + for (const expression of [ + node.index, + node.sliceOffset, + node.sliceLength, + ]) { + if (expression) { + scanArithmetic(expression, false); + } + } + }, + + ForStatement(node) { + if (node.variable) { + addWrite(node.variable.name, node.variable); + } + }, + + TestCommand(node) { + scanTest(node.expression); + }, + + ArithmeticCommand(node) { + if (node.expression) { + scanArithmetic(node.expression, false); + } + }, + + ArithmeticExpansion(node) { + if (node.expression) { + scanArithmetic(node.expression, false); + } + }, + + ArithmeticForStatement(node) { + for (const expression of [node.init, node.test, node.update]) { + if (expression) { + scanArithmetic(expression, false); + } + } + }, + + LetCommand(node) { + for (const expression of node.expressions) { + scanArithmetic(expression, false); + } + }, + + DeclarationCommand(node) { + let exported = node.kind === "export"; + let nameref = node.kind === "nameref"; + let printed = false; + + for (const argument of node.arguments) { + if (argument.type === "Word") { + const text = getStaticText(argument); + + if (text !== null && text.startsWith("-")) { + if (text.includes("x")) { + exported = true; + } + + // `export -n` removes the export attribute + // rather than creating a nameref. + if (text.includes("n")) { + if (node.kind === "export") { + exported = false; + } else { + nameref = true; + } + } + + if (text.includes("p")) { + printed = true; + } + } + } + } + + if (nameref) { + unsafe = true; + } + + for (const argument of node.arguments) { + if ( + argument.type !== "VariableAssignment" || + !argument.name + ) { + continue; + } + + // Exported variables are visible to child processes, so + // consider them used. Naming an existing variable in + // `readonly x` or `declare -p x` also references it. + if ( + exported || + (argument.value === null && + argument.array === null && + (node.kind === "readonly" || printed)) + ) { + reads.add(argument.name.name); + } + } + }, + + Command(node) { + const name = getCommandName(node); + + switch (name) { + case "eval": + case "source": + case ".": + unsafe = true; + break; + + case "unset": + case "export": + case "readonly": + for (const argument of node.arguments) { + addReadableIdentifier(argument); + } + break; + + case "getopts": { + const variable = node.arguments[1]; + + if (variable) { + const text = getStaticText(variable); + + if (text !== null && IDENTIFIER.test(text)) { + addWrite(text, { + type: "Identifier", + start: variable.start, + end: variable.end, + name: text, + }); + } + } + break; + } + + case "read": + case "mapfile": + case "readarray": { + let skipNext = false; + + for (const argument of node.arguments) { + if (skipNext) { + skipNext = false; + continue; + } + + const text = getStaticText(argument); + + if (text === null) { + continue; + } + + if (text.startsWith("-")) { + skipNext = + name === "read" + ? READ_FLAGS_WITH_ARGUMENT.test(text) + : MAPFILE_FLAGS_WITH_ARGUMENT.test( + text, + ); + continue; + } + + if (IDENTIFIER.test(text)) { + addWrite(text, { + type: "Identifier", + start: argument.start, + end: argument.end, + name: text, + }); + } + } + break; + } + + // no default + } + }, + + "Program:exit"() { + if (unsafe) { + return; + } + + for (const [name, identifier] of writes) { + if ( + reads.has(name) || + SPECIAL_VARIABLES.has(name) || + name.startsWith("LC_") || + allowedNames.has(name) || + name.startsWith("_") + ) { + continue; + } + + context.report({ + node: identifier, + messageId: "unusedVariable", + data: { name }, + }); + } + }, + }; + }, +}; + +export default rule;