feat: add no-ls-iteration rule

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       |
 

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)

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",
 		]);

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",
 			},

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" }],
+		},
+	],
+});

src/rules/no-ls-iteration.ts

@@ -0,0 +1,46 @@
+/**
+ * @fileoverview Rule to disallow iterating over `ls` output.
+ * Mirrors ShellCheck SC2045.
+ */
+
+import { startsWithCommand } from "./utils.js";
+import type { ShellRuleDefinition } from "../types.js";
+
+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" &&
+							startsWithCommand(part.body, "ls")
+						) {
+							context.report({
+								node: part,
+								messageId: "lsIteration",
+							});
+						}
+					}
+				}
+			},
+		};
+	},
+};
+
+export default rule;

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);
 				}
 			},
 

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

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",
+		);
+	});
 });