From c341da34d3cd47be45fe57a0668989a78121fb9b Mon Sep 17 00:00:00 2001 From: "Nicholas C. Zakas" Date: Fri, 18 Sep 2026 14:59:31 -0400 Subject: [PATCH 1/2] feat: add no-ls-iteration rule Co-Authored-By: Claude Opus 5 --- README.md | 1 + docs/rules/no-ls-iteration.md | 53 +++++++++++++++++++++++++ src/index.spec.ts | 1 + src/index.ts | 3 ++ src/rules/no-ls-iteration.spec.ts | 53 +++++++++++++++++++++++++ src/rules/no-ls-iteration.ts | 65 +++++++++++++++++++++++++++++++ 6 files changed, 176 insertions(+) create mode 100644 docs/rules/no-ls-iteration.md create mode 100644 src/rules/no-ls-iteration.spec.ts create mode 100644 src/rules/no-ls-iteration.ts diff --git a/README.md b/README.md index 9285b46..0585f71 100644 --- a/README.md +++ b/README.md @@ -77,6 +77,7 @@ Each rule mirrors a well-known ShellCheck check. | ---------------------------------------------------------------------------------- | ---------- | ------------------------------------------------------- | ------- | ----------- | | [`no-backticks`](./docs/rules/no-backticks.md) | SC2006 | Use `$(...)` instead of legacy backticks | ✅ | error | | [`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-useless-echo`](./docs/rules/no-useless-echo.md) | SC2116 | Disallow `$(echo ...)` | | error | diff --git a/docs/rules/no-ls-iteration.md b/docs/rules/no-ls-iteration.md new file mode 100644 index 0000000..87a209e --- /dev/null +++ b/docs/rules/no-ls-iteration.md @@ -0,0 +1,53 @@ +# no-ls-iteration + +Disallow iterating over `ls` output, which breaks on special characters. + +## Background + +A loop such as `for f in $(ls)` splits the output of `ls` on whitespace and then expands globs in each piece. Filenames containing spaces are split into several items, and filenames containing glob characters can expand into other files. Globs such as `for f in *` produce each filename as a single, intact item. + +## Rule Details + +This rule warns about a command substitution in the word list of a `for ... in` loop when the substitution runs `ls`, either directly (`$(ls *.log)`) or as the first command of a pipeline (`$(ls | grep log)`). Both `$(...)` and backtick substitutions are checked. + +This rule only checks `for` loops. Other uses of `ls` output are not reported. + +Examples of **incorrect** code for this rule: + +```bash +# eslint shell/no-ls-iteration: "error" + +for f in $(ls); do echo "$f"; done + +for f in $(ls *.log); do rm "$f"; done + +for f in `ls /tmp`; do echo "$f"; done + +for f in $(ls | grep txt); do echo "$f"; done +``` + +Examples of **correct** code for this rule: + +```bash +# eslint shell/no-ls-iteration: "error" + +for f in *; do echo "$f"; done + +for f in *.log; do rm "$f"; done + +for f in /tmp/*; do echo "$f"; done + +ls -la +``` + +## Options + +This rule has no options. + +## When Not to Use It + +If you control every filename your scripts iterate over and can guarantee they contain no whitespace or glob characters, you can disable this rule. Globs are still simpler and safer. + +## Prior Art + +- [SC2045](https://www.shellcheck.net/wiki/SC2045) diff --git a/src/index.spec.ts b/src/index.spec.ts index 3988838..addd2d6 100644 --- a/src/index.spec.ts +++ b/src/index.spec.ts @@ -45,6 +45,7 @@ describe("plugin", () => { expect(ruleIds.sort()).toEqual([ "no-backticks", "no-expansions-in-single-quotes", + "no-ls-iteration", "no-unquoted-expansions", "no-useless-echo", ]); diff --git a/src/index.ts b/src/index.ts index 76efb67..6bf5cc9 100644 --- a/src/index.ts +++ b/src/index.ts @@ -10,12 +10,14 @@ import { ShellLanguage } from "./languages/shell-language.js"; 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 noUselessEcho from "./rules/no-useless-echo.js"; const rules = { "no-backticks": noBackticks, "no-expansions-in-single-quotes": noExpansionsInSingleQuotes, + "no-ls-iteration": noLsIteration, "no-unquoted-expansions": noUnquotedExpansions, "no-useless-echo": noUselessEcho, }; @@ -41,6 +43,7 @@ const plugin = { rules: { "shell/no-backticks": "error", "shell/no-expansions-in-single-quotes": "warn", + "shell/no-ls-iteration": "error", "shell/no-unquoted-expansions": "error", "shell/no-useless-echo": "error", }, diff --git a/src/rules/no-ls-iteration.spec.ts b/src/rules/no-ls-iteration.spec.ts new file mode 100644 index 0000000..a052221 --- /dev/null +++ b/src/rules/no-ls-iteration.spec.ts @@ -0,0 +1,53 @@ +/** + * @fileoverview Tests for the no-ls-iteration rule. + */ + +import { RuleTester } from "eslint"; +import { ShellLanguage } from "../languages/shell-language.js"; +import rule from "./no-ls-iteration.js"; + +const ruleTester = new RuleTester({ + plugins: { + 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-ls-iteration", rule as never, { + valid: [ + 'for f in *; do echo "$f"; done', + 'for f in *.txt; do echo "$f"; done', + "for f in $(find . -name '*.txt'); do echo \"$f\"; done", + 'for n in 1 2 3; do echo "$n"; done', + // ls outside of a for loop is fine as far as this rule goes + "ls -la", + ], + invalid: [ + { + code: 'for f in $(ls); do echo "$f"; done', + errors: [ + { + messageId: "lsIteration", + line: 1, + column: 10, + endColumn: 15, + }, + ], + }, + { + code: 'for f in $(ls *.txt); do echo "$f"; done', + errors: [{ messageId: "lsIteration" }], + }, + { + code: 'for f in `ls`; do echo "$f"; done', + errors: [{ messageId: "lsIteration" }], + }, + { + code: 'for f in $(ls | grep log); do echo "$f"; done', + errors: [{ messageId: "lsIteration" }], + }, + ], +}); diff --git a/src/rules/no-ls-iteration.ts b/src/rules/no-ls-iteration.ts new file mode 100644 index 0000000..35631af --- /dev/null +++ b/src/rules/no-ls-iteration.ts @@ -0,0 +1,65 @@ +/** + * @fileoverview Rule to disallow iterating over `ls` output. + * Mirrors ShellCheck SC2045/SC2012. + */ + +import { isCommandNamed } from "./utils.js"; +import type { ShellRuleDefinition, StatementNode } from "../types.js"; + +/** + * Determines whether a command substitution body starts with `ls`. + */ +function startsWithLs(body: StatementNode[]): boolean { + const first = body[0]; + + if (!first) { + return false; + } + + if (first.type === "Pipeline") { + const firstCommand = first.commands[0]; + + return firstCommand !== undefined && isCommandNamed(firstCommand, "ls"); + } + + return isCommandNamed(first, "ls"); +} + +const rule: ShellRuleDefinition<{ MessageIds: "lsIteration" }> = { + meta: { + type: "problem", + docs: { + description: + "Disallow iterating over `ls` output, which breaks on special characters", + recommended: true, + url: "https://github.com/eslint/shell/blob/main/docs/rules/no-ls-iteration.md", + }, + schema: [], + messages: { + lsIteration: + "Iterating over ls output is fragile. Use globs (e.g. *) instead. (ShellCheck SC2045)", + }, + }, + + create(context) { + return { + ForStatement(node) { + for (const word of node.words) { + for (const part of word.parts) { + if ( + part.type === "CommandSubstitution" && + startsWithLs(part.body) + ) { + context.report({ + node: part, + messageId: "lsIteration", + }); + } + } + } + }, + }; + }, +}; + +export default rule; From 2a597054fa9a0389a4102b6945d30ecd3179546a Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 29 Sep 2026 20:51:19 +0000 Subject: [PATCH 2/2] fix: preserve ls loop iterations with autofix Co-authored-by: nzakas <38546+nzakas@users.noreply.github.com> --- src/rules/no-ls-iteration.ts | 27 ++++----------------------- src/rules/no-unquoted-expansions.ts | 24 +++++++++++++++--------- src/rules/utils.ts | 19 +++++++++++++++++++ tests/plugin.test.ts | 19 +++++++++++++++++++ 4 files changed, 57 insertions(+), 32 deletions(-) diff --git a/src/rules/no-ls-iteration.ts b/src/rules/no-ls-iteration.ts index 35631af..d17187f 100644 --- a/src/rules/no-ls-iteration.ts +++ b/src/rules/no-ls-iteration.ts @@ -1,29 +1,10 @@ /** * @fileoverview Rule to disallow iterating over `ls` output. - * Mirrors ShellCheck SC2045/SC2012. + * Mirrors ShellCheck SC2045. */ -import { isCommandNamed } from "./utils.js"; -import type { ShellRuleDefinition, StatementNode } from "../types.js"; - -/** - * Determines whether a command substitution body starts with `ls`. - */ -function startsWithLs(body: StatementNode[]): boolean { - const first = body[0]; - - if (!first) { - return false; - } - - if (first.type === "Pipeline") { - const firstCommand = first.commands[0]; - - return firstCommand !== undefined && isCommandNamed(firstCommand, "ls"); - } - - return isCommandNamed(first, "ls"); -} +import { startsWithCommand } from "./utils.js"; +import type { ShellRuleDefinition } from "../types.js"; const rule: ShellRuleDefinition<{ MessageIds: "lsIteration" }> = { meta: { @@ -48,7 +29,7 @@ const rule: ShellRuleDefinition<{ MessageIds: "lsIteration" }> = { for (const part of word.parts) { if ( part.type === "CommandSubstitution" && - startsWithLs(part.body) + startsWithCommand(part.body, "ls") ) { context.report({ node: part, diff --git a/src/rules/no-unquoted-expansions.ts b/src/rules/no-unquoted-expansions.ts index c32b47b..767ff7e 100644 --- a/src/rules/no-unquoted-expansions.ts +++ b/src/rules/no-unquoted-expansions.ts @@ -4,6 +4,7 @@ */ import type { ShellRuleDefinition, WordNode } from "../types.js"; +import { startsWithCommand } from "./utils.js"; /** * Parameters that always expand to values that cannot split, so quoting is @@ -38,7 +39,7 @@ const rule: ShellRuleDefinition<{ create(context) { const { sourceCode } = context; - function checkWord(word: WordNode): void { + function checkWord(word: WordNode, isForWord = false): void { for (const part of word.parts) { if ( part.type !== "ParameterExpansion" && @@ -58,6 +59,10 @@ const rule: ShellRuleDefinition<{ word.parts.length === 1 && word.start === part.start && word.end === part.end; + const isLsIteration = + isForWord && + part.type === "CommandSubstitution" && + startsWithCommand(part.body, "ls"); context.report({ node: part, @@ -69,13 +74,14 @@ const rule: ShellRuleDefinition<{ part.type === "ParameterExpansion" ? { expansion: sourceCode.getText(part) } : {}, - fix: isWholeWord - ? fixer => - fixer.replaceText( - word, - `"${sourceCode.getText(word)}"`, - ) - : undefined, + fix: + isWholeWord && !isLsIteration + ? fixer => + fixer.replaceText( + word, + `"${sourceCode.getText(word)}"`, + ) + : undefined, }); } } @@ -93,7 +99,7 @@ const rule: ShellRuleDefinition<{ ForStatement(node) { for (const word of node.words) { - checkWord(word); + checkWord(word, true); } }, diff --git a/src/rules/utils.ts b/src/rules/utils.ts index 40d3bd0..851e265 100644 --- a/src/rules/utils.ts +++ b/src/rules/utils.ts @@ -90,6 +90,25 @@ export function isCommandNamed( return statement.type === "Command" && getCommandName(statement) === name; } +/** + * Determines whether a command substitution starts with a command of the given + * name. + */ +export function startsWithCommand( + body: StatementNode[], + name: string, +): boolean { + const first = body[0]; + + if (!first) { + return false; + } + + const firstCommand = first.type === "Pipeline" ? first.commands[0] : first; + + return firstCommand !== undefined && isCommandNamed(firstCommand, name); +} + /** * Returns the expansions that make up a word, including those nested inside * double quotes when `includeQuoted` is `true`. Expansions nested inside diff --git a/tests/plugin.test.ts b/tests/plugin.test.ts index d7da37b..fec5280 100644 --- a/tests/plugin.test.ts +++ b/tests/plugin.test.ts @@ -139,4 +139,23 @@ describe("recommended configuration", () => { expect(linter.verify(code, config, "script.sh")[0]?.fatal).toBe(true); expect(linter.verify(code, config, "script.js")).toEqual([]); }); + + it("should preserve ls iterations when autofixing", () => { + const code = 'for f in $(ls); do echo "$f"; done\n'; + const linter = new Linter(); + const result = linter.verifyAndFix( + code, + [shell.configs.recommended] as never, + "script.sh", + ); + + expect(result.output).toBe(code); + expect(result.fixed).toBe(false); + expect(result.messages.map(({ ruleId }) => ruleId)).toContain( + "shell/no-ls-iteration", + ); + expect(result.messages.map(({ ruleId }) => ruleId)).toContain( + "shell/no-unquoted-expansions", + ); + }); });