ronantakizawa/codereview-bench huggingface dataset:
Repository: eslint/eslint
File: lib/types/rules.d.ts (TypeScript)

Code before:
	/**
	 * Rule to disallow trailing whitespace at the end of lines.
	 *
	 * @since 0.7.1
	 * @deprecated since 8.53.0.
	 * Formatting rules are being moved out of ESLint core.
	 * Please, use [`no-trailing-spaces`](https://eslint.style/rules/js/no-trailing-spaces) in [`@stylistic/eslint-plugin-js`](https://eslint.style/packages/js).
	 * @see https://eslint.org/docs/latest/rules/no-trailing-spaces
	 */
	"no-trailing-spaces": Linter.RuleEntry<
		[
			Partial<{
				/**
				 * @default false
				 */
				skipBlankLines: boolean;
				/**
				 * @default false
				 */
				ignoreComments: boolean;
			}>,
		]
	>;

	/**
	 * Rule to disallow variable declarations that use `let` or `var` but have no intitial value and are never assigned.
	 *
	 * @see https://eslint.org/docs/latest/rules/no-unassigned-vars
	 */
	"no-unassigned-vars": Linter.RuleEntry<[]>;

	/**
	 * Rule to disallow the use of undeclared variables unless mentioned in \/*global *\/ comments.
	 *
	 * @remarks
	 * Recommended by ESLint, the rule was enabled in `eslint:recommended`.
	 *
	 * @since 0.0.9
	 * @see https://eslint.org/docs/latest/rules/no-undef
	 */
	"no-undef": Linter.RuleEntry<
		[
			Partial<{
				/**
				 * @default false
				 */
				typeof: boolean;
			}>,
		]
	>;


Review comment (suggestion):
```suggestion
	 * Rule to disallow variable declarations that use `let` or `var` but have no initial value and are never assigned.
```

Code after:
	 * @see https://eslint.org/docs/latest/rules/no-ternary
	 */
	"no-ternary": Linter.RuleEntry<[]>;

	/**
	 * Rule to disallow `this`/`super` before calling `super()` in constructors.
	 *
	 * @remarks
	 * Recommended by ESLint, the rule was enabled in `eslint:recommended`.
	 *
	 * @since 0.24.0
	 * @see https://eslint.org/docs/latest/rules/no-this-before-super
	 */
	"no-this-before-super": Linter.RuleEntry<[]>;

	/**
	 * Rule to disallow throwing literals as exceptions.
	 *
	 * @since 0.15.0
	 * @see https://eslint.org/docs/latest/rules/no-throw-literal
	 */
	"no-throw-literal": Linter.RuleEntry<[]>;

	/**
	 * Rule to disallow trailing whitespace at the end of lines.
	 *
	 * @since 0.7.1
	 * @deprecated since 8.53.0.
	 * Formatting rules are being moved out of ESLint core.
	 * Please, use [`no-trailing-spaces`](https://eslint.style/rules/js/no-trailing-spaces) in [`@stylistic/eslint-plugin-js`](https://eslint.style/packages/js).
	 * @see https://eslint.org/docs/latest/rules/no-trailing-spaces
	 */
	"no-trailing-spaces": Linter.RuleEntry<
		[
			Partial<{
				/**
				 * @default false
				 */
				skipBlankLines: boolean;
				/**
				 * @default false
				 */
				ignoreComments: boolean;
			}>,
		]
	>;

	/**
	 * Rule to disallow `let` or `var` variables that are read but never assigned.
	 *
	 * @see https://eslint.org/docs/latest/rules/no-unassigned-vars
Repository: eslint/eslint
File: lib/rules/no-unassigned-vars.js (JavaScript)

Code before:
/**
 * @fileoverview Rule to flag variables that are never assigned
 * @author Jacob Bandes-Storch <https://github.com/jtbandes>
 */
"use strict";

//------------------------------------------------------------------------------
// Rule Definition
//------------------------------------------------------------------------------

/** @type {import('../types').Rule.RuleModule} */
module.exports = {
	meta: {
		type: "problem",

		docs: {
			description:
				"Disallow variable declarations that use `let` or `var` but have no intitial value and are never assigned",
			recommended: false,
			url: "https://eslint.org/docs/latest/rules/no-unassigned-vars",
		},

		schema: [],
		messages: {
			unassigned:
				"'{{name}}' is not initialized and never assigned, so it will always be undefined.",
		},
	},

	create(context) {
		const sourceCode = context.sourceCode;

		return {
			[`VariableDeclaration:matches([kind=let], [kind=var]):not([declare=true]) > VariableDeclarator[id.type=Identifier]:not([init])`](
				node,
			) {
				const [variable] = sourceCode.getDeclaredVariables(node);
				if (!variable) {
					return;
				}
				let hasRead = false;
				for (const reference of variable.references) {
					if (reference.isWrite()) {

Review comment (suggestion):
```suggestion
				"Disallow variable declarations that use `let` or `var` but have no initial value and are never assigned",
```

Code after:
/**
 * @fileoverview Rule to flag variables that are never assigned
 * @author Jacob Bandes-Storch <https://github.com/jtbandes>
 */
"use strict";

//------------------------------------------------------------------------------
// Rule Definition
//------------------------------------------------------------------------------

/** @type {import('../types').Rule.RuleModule} */
module.exports = {
	meta: {
		type: "problem",
		dialects: ["typescript", "javascript"],
		language: "javascript",

		docs: {
			description:
				"Disallow `let` or `var` variables that are read but never assigned",
			recommended: false,
			url: "https://eslint.org/docs/latest/rules/no-unassigned-vars",
		},

		schema: [],
		messages: {
			unassigned:
				"'{{name}}' is always 'undefined' because it's never assigned.",
		},
	},

	create(context) {
		const sourceCode = context.sourceCode;

		return {
			VariableDeclarator(node) {
				/** @type {import('estree').VariableDeclaration} */
				const declaration = node.parent;
				const shouldCheck =
					!node.init &&
					node.id.type === "Identifier" &&
					declaration.kind !== "const" &&
					!declaration.declare;
Repository: eslint/eslint
File: lib/rules/no-unassigned-vars.js (JavaScript)

Code before:
/** @type {import('../types').Rule.RuleModule} */
module.exports = {
	meta: {
		type: "problem",
		dialects: ["typescript", "javascript"],
		language: "javascript",

		docs: {
			description:
				"Disallow variable declarations that use `let` or `var` but have no initial value and are never assigned",
			recommended: false,
			url: "https://eslint.org/docs/latest/rules/no-unassigned-vars",
		},

		schema: [],
		messages: {
			unassigned:
				"'{{name}}' is not initialized and never assigned, so it will always be undefined.",
		},
	},

	create(context) {
		const sourceCode = context.sourceCode;

		return {
			[`VariableDeclaration:matches([kind=let], [kind=var]):not([declare=true]) > VariableDeclarator[id.type=Identifier]:not([init])`](
				node,
			) {
				const [variable] = sourceCode.getDeclaredVariables(node);
				if (!variable) {
					return;
				}
				let hasRead = false;
				for (const reference of variable.references) {
					if (reference.isWrite()) {
						return;
					}
					if (reference.isRead()) {
						hasRead = true;
					}
				}
				if (!hasRead) {
					// Variables that are never read should be flagged by no-unused-vars instead
					return;
				}
				context.report({
					node,
					messageId: "unassigned",
					data: { name: node.id.name },
				});
			},

Review comment (performance):
This is a pretty long selector, which also means it will be fairly slow. Can we change this to just `VariableDeclarator` and then write the conditions in JavaScript? Something like this?

```js
const declaration = sourceCode.getParent(node);
const shouldCheck = !node.init && node.id.type === "Identifier" && declaration.kind !== "const" && !declaration.declare;
```

Code after:
/** @type {import('../types').Rule.RuleModule} */
module.exports = {
	meta: {
		type: "problem",
		dialects: ["typescript", "javascript"],
		language: "javascript",

		docs: {
			description:
				"Disallow `let` or `var` variables that are read but never assigned",
			recommended: false,
			url: "https://eslint.org/docs/latest/rules/no-unassigned-vars",
		},

		schema: [],
		messages: {
			unassigned:
				"'{{name}}' is always 'undefined' because it's never assigned.",
		},
	},

	create(context) {
		const sourceCode = context.sourceCode;

		return {
			VariableDeclarator(node) {
				/** @type {import('estree').VariableDeclaration} */
				const declaration = node.parent;
				const shouldCheck =
					!node.init &&
					node.id.type === "Identifier" &&
					declaration.kind !== "const" &&
					!declaration.declare;
				if (!shouldCheck) {
					return;
				}
				const [variable] = sourceCode.getDeclaredVariables(node);
				if (!variable) {
					return;
				}
				let hasRead = false;
				for (const reference of variable.references) {
					if (reference.isWrite()) {
						return;
					}
					if (reference.isRead()) {
						hasRead = true;
					}
				}
				if (!hasRead) {
					// Variables that are never read should be flagged by no-unused-vars instead
Repository: eslint/eslint
File: lib/rules/no-unassigned-vars.js (JavaScript)

Code before:
/**
 * @fileoverview Rule to flag variables that are never assigned
 * @author Jacob Bandes-Storch <https://github.com/jtbandes>
 */
"use strict";

//------------------------------------------------------------------------------
// Rule Definition
//------------------------------------------------------------------------------

/** @type {import('../types').Rule.RuleModule} */
module.exports = {
	meta: {
		type: "problem",
		dialects: ["typescript", "javascript"],
		language: "javascript",

		docs: {
			description:
				"Disallow variable declarations that use `let` or `var` but have no initial value and are never assigned",
			recommended: false,
			url: "https://eslint.org/docs/latest/rules/no-unassigned-vars",
		},

		schema: [],
		messages: {
			unassigned:
				"'{{name}}' is not initialized and never assigned, so it will always be undefined.",
		},
	},

	create(context) {
		const sourceCode = context.sourceCode;

		return {
			VariableDeclarator(node) {
				/** @type {import('estree').VariableDeclaration} */
				const declaration = node.parent;
				const shouldCheck =
					!node.init &&
					node.id.type === "Identifier" &&
					declaration.kind !== "const" &&
					!declaration.declare;
				if (!shouldCheck) {
					return;

Review comment (suggestion):
`no-unused-vars` will also [report vars](https://eslint.org/play/#eyJ0ZXh0IjoiLyogZXNsaW50IG5vLXVudXNlZC12YXJzOiBcImVycm9yXCIgKi9cblxubGV0IHg7XG4iLCJvcHRpb25zIjp7InJ1bGVzIjp7fSwibGFuZ3VhZ2VPcHRpb25zIjp7InBhcnNlck9wdGlvbnMiOnsiZWNtYUZlYXR1cmVzIjp7fX19fX0=) with this description.

So something like this is more accurate:

```suggestion
			description:
				"Disallow `let` or `var` variables that are read but never assigned.",
```

Code after:
/**
 * @fileoverview Rule to flag variables that are never assigned
 * @author Jacob Bandes-Storch <https://github.com/jtbandes>
 */
"use strict";

//------------------------------------------------------------------------------
// Rule Definition
//------------------------------------------------------------------------------

/** @type {import('../types').Rule.RuleModule} */
module.exports = {
	meta: {
		type: "problem",
		dialects: ["typescript", "javascript"],
		language: "javascript",

		docs: {
			description:
				"Disallow `let` or `var` variables that are read but never assigned",
			recommended: false,
			url: "https://eslint.org/docs/latest/rules/no-unassigned-vars",
		},

		schema: [],
		messages: {
			unassigned:
				"'{{name}}' is always 'undefined' because it's never assigned.",
		},
	},

	create(context) {
		const sourceCode = context.sourceCode;

		return {
			VariableDeclarator(node) {
				/** @type {import('estree').VariableDeclaration} */
				const declaration = node.parent;
				const shouldCheck =
					!node.init &&
					node.id.type === "Identifier" &&
					declaration.kind !== "const" &&
					!declaration.declare;
				if (!shouldCheck) {
					return;
Repository: eslint/eslint
File: tests/lib/rules/no-unused-expressions.js (JavaScript)

Code before:
//------------------------------------------------------------------------------

const ruleTester = new RuleTester();

ruleTester.run("no-unused-expressions", rule, {
	valid: [
		"function f(){}",
		"a = b",
		"new a",
		"{}",
		"f(); g()",
		"i++",
		"a()",
		{ code: "a && a()", options: [{ allowShortCircuit: true }] },
		{ code: "a() || (b = c)", options: [{ allowShortCircuit: true }] },
		{ code: "a ? b() : c()", options: [{ allowTernary: true }] },
		{
			code: "a ? b() || (c = d) : e()",
			options: [{ allowShortCircuit: true, allowTernary: true }],
		},
		"delete foo.bar",
		"void new C",
		{
			code: '"use strict";',
			languageOptions: { ecmaVersion: 3, sourceType: "script" },
		},
		'"directive one"; "directive two"; f();',
		'function foo() {"use strict"; return true; }',
		{
			code: 'var foo = () => {"use strict"; return true; }',
			languageOptions: { ecmaVersion: 6 },
		},
		'function foo() {"directive one"; "directive two"; f(); }',
		'function foo() { var foo = "use strict"; return true; }',
		{
			code: "function* foo(){ yield 0; }",
			languageOptions: { ecmaVersion: 6 },
		},
		{
			code: "async function foo() { await 5; }",
			languageOptions: { ecmaVersion: 8 },
		},
		{
			code: "async function foo() { await foo.bar; }",
			languageOptions: { ecmaVersion: 8 },
		},
		{
			code: "async function foo() { bar && await baz; }",
			options: [{ allowShortCircuit: true }],
			languageOptions: { ecmaVersion: 8 },
		},

Review comment (suggestion):
I think we can keep the old `'"use strict";',` case as well.

Code after:
//------------------------------------------------------------------------------

const ruleTester = new RuleTester();

ruleTester.run("no-unused-expressions", rule, {
	valid: [
		"function f(){}",
		"a = b",
		"new a",
		"{}",
		"f(); g()",
		"i++",
		"a()",
		{ code: "a && a()", options: [{ allowShortCircuit: true }] },
		{ code: "a() || (b = c)", options: [{ allowShortCircuit: true }] },
		{ code: "a ? b() : c()", options: [{ allowTernary: true }] },
		{
			code: "a ? b() || (c = d) : e()",
			options: [{ allowShortCircuit: true, allowTernary: true }],
		},
		"delete foo.bar",
		"void new C",
		'"use strict";',
		{
			code: '"use strict";',
			languageOptions: { ecmaVersion: 3, sourceType: "script" },
		},
		'"directive one"; "directive two"; f();',
		'function foo() {"use strict"; return true; }',
		{
			code: 'var foo = () => {"use strict"; return true; }',
			languageOptions: { ecmaVersion: 6 },
		},
		'function foo() {"directive one"; "directive two"; f(); }',
		'function foo() { var foo = "use strict"; return true; }',
		{
			code: "function* foo(){ yield 0; }",
			languageOptions: { ecmaVersion: 6 },
		},
		{
			code: "async function foo() { await 5; }",
			languageOptions: { ecmaVersion: 8 },
		},
		{
			code: "async function foo() { await foo.bar; }",
			languageOptions: { ecmaVersion: 8 },
		},
		{
			code: "async function foo() { bar && await baz; }",
			options: [{ allowShortCircuit: true }],
			languageOptions: { ecmaVersion: 8 },
Repository: eslint/eslint
File: lib/rules/eqeqeq.js (JavaScript)

Code before:
		 * @returns {void}
		 * @private
		 */
		function report(node, expectedOperator) {
			const operatorToken = sourceCode.getFirstTokenBetween(
				node.left,
				node.right,
				token => token.value === node.operator,
			);

			context.report({
				node,
				loc: operatorToken.loc,
				messageId: "unexpected",
				data: { expectedOperator, actualOperator: node.operator },
				fix(fixer) {
					// If the comparison is a `typeof` comparison or both sides are literals with the same type, then it's safe to fix.
					if (isTypeOfBinary(node) || areLiteralsAndSameType(node)) {
						return fixer.replaceText(
							operatorToken,
							expectedOperator,
						);
					}
					return null;
				},
				suggest: [
					{
						messageId: "unexpectedSuggestion",
						data: {
							expectedOperator,
							actualOperator: node.operator,
						},
						fix: fixer =>
							fixer.replaceText(operatorToken, expectedOperator),
					},
				],
			});
		}

		return {
			BinaryExpression(node) {
				const isNull = isNullCheck(node);

				if (node.operator !== "==" && node.operator !== "!=") {
					if (enforceInverseRuleForNull && isNull) {
						report(node, node.operator.slice(0, -1));
					}
					return;
				}

				if (

Review comment (suggestion):
The same lint message instance should not have both autofix and suggestion(s), so this suggestion should be provided only when the autofix isn't.

Code after:
		 * @returns {void}
		 * @private
		 */
		function report(node, expectedOperator) {
			const operatorToken = sourceCode.getFirstTokenBetween(
				node.left,
				node.right,
				token => token.value === node.operator,
			);

			const commonReportParams = {
				node,
				loc: operatorToken.loc,
				messageId: "unexpected",
				data: { expectedOperator, actualOperator: node.operator },
			};

			if (isTypeOfBinary(node) || areLiteralsAndSameType(node)) {
				context.report({
					...commonReportParams,
					fix(fixer) {
						return fixer.replaceText(
							operatorToken,
							expectedOperator,
						);
					},
				});
			} else {
				context.report({
					...commonReportParams,
					suggest: [
						{
							messageId: "replaceOperator",
							data: {
								expectedOperator,
								actualOperator: node.operator,
							},
							fix: fixer =>
								fixer.replaceText(
									operatorToken,
									expectedOperator,
								),
						},
					],
				});
			}
		}

		return {
			BinaryExpression(node) {
				const isNull = isNullCheck(node);
Repository: eslint/eslint
File: lib/rules/no-unused-expressions.js (JavaScript)

Code before:
			ThisExpression: alwaysTrue,
			UnaryExpression(node) {
				return node.operator !== "void" && node.operator !== "delete";
			},
			// TypeScript-specific node types
			TSAsExpression(node) {
				return Checker.isDisallowed(node.expression);
			},
			TSTypeAssertion(node) {
				return Checker.isDisallowed(node.expression);
			},
			TSNonNullExpression(node) {
				return Checker.isDisallowed(node.expression);
			},
			TSInstantiationExpression(node) {
				return Checker.isDisallowed(node.expression);
			},
		});

		return {
			ExpressionStatement(node) {
				if (
					Checker.isDisallowed(node.expression) &&
					(ignoreDirectives
						? !isDirective(node)
						: !astUtils.isDirective(node))
				) {
					context.report({ node, messageId: "unusedExpression" });
				}
			},
		};
	},
};


Review comment (suggestion):
```suggestion
					!astUtils.isDirective(node) &&
					!(ignoreDirectives && isDirective(node))
```

I think we can always check if the statement was parsed as a directive, as that should be the most reliable check and it isn't costly.

Code after:
			ThisExpression: alwaysTrue,
			UnaryExpression(node) {
				return node.operator !== "void" && node.operator !== "delete";
			},
			// TypeScript-specific node types
			TSAsExpression(node) {
				return Checker.isDisallowed(node.expression);
			},
			TSTypeAssertion(node) {
				return Checker.isDisallowed(node.expression);
			},
			TSNonNullExpression(node) {
				return Checker.isDisallowed(node.expression);
			},
			TSInstantiationExpression(node) {
				return Checker.isDisallowed(node.expression);
			},
		});

		return {
			ExpressionStatement(node) {
				if (
					Checker.isDisallowed(node.expression) &&
					!astUtils.isDirective(node) &&
					!(ignoreDirectives && isDirective(node))
				) {
					context.report({ node, messageId: "unusedExpression" });
				}
			},
		};
	},
};
Repository: eslint/eslint
File: tests/lib/eslint/eslint.js (JavaScript)

Code before:
				}),
			);

			const eslintOptions = {
				overrideConfigFile: true,

				// specifying cache false the cache will be deleted
				cache: false,
				cacheLocation: cacheFilePath,
				overrideConfig: {
					rules: {
						"no-console": 0,
						"no-unused-vars": 2,
					},
				},
				cwd: path.join(fixtureDir, ".."),
			};

			eslint = new ESLint(eslintOptions);

			const file = getFixturePath("cache/src", "test-file.js");

			await eslint.lintFiles([file]);

			assert(
				fsp.unlink.notCalled,
				"Expected attempt to delete the cache was not made.",
			);
		});

		it("should store in the cache a file that has lint messages and a file that doesn't have lint messages", async () => {
			cacheFilePath = getFixturePath(".eslintcache");
			doDelete(cacheFilePath);
			assert(
				!shell.test("-f", cacheFilePath),
				"the cache file already exists and wasn't successfully deleted",
			);

			eslint = new ESLint({
				cwd: path.join(fixtureDir, ".."),
				overrideConfigFile: true,

				// specifying cache true the cache will be created
				cache: true,
				cacheLocation: cacheFilePath,
				overrideConfig: {
					rules: {
						"no-console": 0,
						"no-unused-vars": 2,
					},
				},

Review comment (performance):
I think, for this particular test case, we'd now need the cache file to exist, and write another test for the change made in this PR.

Code after:
				cache: false,
				cacheLocation: cacheFilePath,
				overrideConfig: {
					rules: {
						"no-console": 0,
						"no-unused-vars": 2,
					},
				},
				cwd: path.join(fixtureDir, ".."),
			};

			eslint = new ESLint(eslintOptions);

			const file = getFixturePath("cache/src", "test-file.js");

			await eslint.lintFiles([file]);

			assert(
				spy.notCalled,
				"Expected attempt to delete the cache was not made.",
			);

			spy.restore();
		});

		it("should throw an error if the cache file to be deleted exist on a read-only file system", async () => {
			cacheFilePath = getFixturePath(".eslintcache");
			fs.writeFileSync(cacheFilePath, "");

			// Simulate a read-only file system.
			const unlinkStub = sinon.stub(fsp, "unlink").rejects(
				Object.assign(new Error("read-only file system"), {
					code: "EROFS",
				}),
			);

			const eslintOptions = {
				overrideConfigFile: true,
				cache: false,
				cacheLocation: cacheFilePath,
				overrideConfig: {
					rules: {
						"no-console": 0,
						"no-unused-vars": 2,
					},
				},
				cwd: path.join(fixtureDir, ".."),
			};

			eslint = new ESLint(eslintOptions);