ronantakizawa/codereview-bench huggingface dataset:
Repository: eslint/eslint
File: lib/eslint/eslint.js (JavaScript)

Code before:
		debug(`Using file patterns: ${normalizedPatterns}`);

		const {
			allowInlineConfig,
			cache,
			cwd,
			fix,
			fixTypes,
			ruleFilter,
			stats,
			globInputPaths,
			errorOnUnmatchedPattern,
			warnIgnored,
		} = eslintOptions;
		const startTime = Date.now();
		const fixTypesSet = fixTypes ? new Set(fixTypes) : null;

		// Delete cache file; should this be done here?
		if (!cache && cacheFilePath) {
			debug(`Deleting cache file at ${cacheFilePath}`);

			try {
				if (existsSync(cacheFilePath)) {
					await fs.unlink(cacheFilePath);
				}
			} catch (error) {
				const errorCode = error && error.code;

				// Ignore errors when no such file exists or file system is read only (and cache file does not exist)
				if (
					errorCode !== "ENOENT" &&
					!(errorCode === "EROFS" && !existsSync(cacheFilePath))
				) {
					throw error;
				}
			}
		}

		const filePaths = await findFiles({
			patterns: normalizedPatterns,
			cwd,
			globInputPaths,
			configLoader: this.#configLoader,
			errorOnUnmatchedPattern,
		});
		const controller = new AbortController();
		const retryCodes = new Set(["ENFILE", "EMFILE"]);
		const retrier = new Retrier(error => retryCodes.has(error.code), {
			concurrency: 100,
		});


Review comment (performance):
I'm not sure if the try-catch is still necessary after this change. It looks like the situations the catch block was intended to address—when the cache file was missing on a writable file system or when it was missing on a read-only file system—are now both handled by the `existsSync` check.

Code after:
		debug(`Using file patterns: ${normalizedPatterns}`);

		const {
			allowInlineConfig,
			cache,
			cwd,
			fix,
			fixTypes,
			ruleFilter,
			stats,
			globInputPaths,
			errorOnUnmatchedPattern,
			warnIgnored,
		} = eslintOptions;
		const startTime = Date.now();
		const fixTypesSet = fixTypes ? new Set(fixTypes) : null;

		// Delete cache file; should this be done here?
		if (!cache && cacheFilePath) {
			debug(`Deleting cache file at ${cacheFilePath}`);

			try {
				if (existsSync(cacheFilePath)) {
					await fs.unlink(cacheFilePath);
				}
			} catch (error) {
				if (existsSync(cacheFilePath)) {
					throw error;
				}
			}
		}

		const filePaths = await findFiles({
			patterns: normalizedPatterns,
			cwd,
			globInputPaths,
			configLoader: this.#configLoader,
			errorOnUnmatchedPattern,
		});
		const controller = new AbortController();
		const retryCodes = new Set(["ENFILE", "EMFILE"]);
		const retrier = new Retrier(error => retryCodes.has(error.code), {
			concurrency: 100,
		});

		debug(
			`${filePaths.length} files found in: ${Date.now() - startTime}ms`,
		);

		/*
		 * Because we need to process multiple files, including reading from disk,
Repository: eslint/eslint
File: tests/lib/mcp/mcp-server.js (JavaScript)

Code before:
	type: "object",
};

//-----------------------------------------------------------------------------
// Tests
//-----------------------------------------------------------------------------

describe("MCP Server", () => {
	let client, clientTransport, serverTransport;

	beforeEach(async () => {
		client = new Client({
			name: "test client",
			version: "1.0",
		});

		[clientTransport, serverTransport] =
			InMemoryTransport.createLinkedPair();

		// Note: must connect server first or else client hangs
		await mcpServer.connect(serverTransport);
		await client.connect(clientTransport);

		sinon
			.stub(process, "emitWarning")
			.withArgs(sinon.match.any, "ESLintIgnoreWarning");
	});

	afterEach(() => {
		sinon.restore();
	});

	describe("Tools", () => {
		it("should list tools", async () => {
			const { tools } = await client.listTools();

			assert.strictEqual(tools.length, 1);
			assert.strictEqual(tools[0].name, "lint-files");
			assert.deepStrictEqual(tools[0].inputSchema, filePathsJsonSchema);
		});

		describe("lint-files", () => {
			it("should return zero lint messages for a valid file", async () => {
				const { content: rawResults } = await client.callTool({
					name: "lint-files",
					arguments: {
						filePaths: ["tests/fixtures/passing.js"],
					},
				});

				const expectedFilePath = path.join(

Review comment (suggestion):
```suggestion
			.withArgs(sinon.match.any, "ESLintIgnoreWarning")
			.returns();
		process.emitWarning.callThrough();
```


To allow other potentially useful warnings to appear in the output.

Code after:
	type: "object",
};

//-----------------------------------------------------------------------------
// Tests
//-----------------------------------------------------------------------------

describe("MCP Server", () => {
	let client, clientTransport, serverTransport;

	beforeEach(async () => {
		client = new Client({
			name: "test client",
			version: "1.0",
		});

		[clientTransport, serverTransport] =
			InMemoryTransport.createLinkedPair();

		// Note: must connect server first or else client hangs
		await mcpServer.connect(serverTransport);
		await client.connect(clientTransport);

		sinon
			.stub(process, "emitWarning")
			.callThrough()
			.withArgs(sinon.match.any, "ESLintIgnoreWarning")
			.returns();
	});

	afterEach(() => {
		sinon.restore();
	});

	describe("Tools", () => {
		it("should list tools", async () => {
			const { tools } = await client.listTools();

			assert.strictEqual(tools.length, 1);
			assert.strictEqual(tools[0].name, "lint-files");
			assert.deepStrictEqual(tools[0].inputSchema, filePathsJsonSchema);
		});

		describe("lint-files", () => {
			it("should return zero lint messages for a valid file", async () => {
				const { content: rawResults } = await client.callTool({
					name: "lint-files",
					arguments: {
						filePaths: ["tests/fixtures/passing.js"],
					},
				});
Repository: eslint/eslint
File: tests/lib/rules/no-var.js (JavaScript)

Code before:
const ruleTester = new RuleTester({
	languageOptions: { ecmaVersion: 6, sourceType: "script" },
});

ruleTester.run("no-var", rule, {
	valid: [
		"const JOE = 'schmoe';",
		"let moo = 'car';",
		{
			code: "const JOE = 'schmoe';",
			languageOptions: {
				parserOptions: { ecmaFeatures: { globalReturn: true } },
			},
		},
		{
			code: "let moo = 'car';",
			languageOptions: {
				parserOptions: { ecmaFeatures: { globalReturn: true } },
			},
		},
		{
			code: "declare global { var bar: 'car' }",
			languageOptions: {
				parser: require("@typescript-eslint/parser"),
			},
		},
	],

	invalid: [
		{
			code: "var foo = bar;",
			output: "let foo = bar;",
			languageOptions: {
				parserOptions: { ecmaFeatures: { globalReturn: true } },
			},
			errors: [
				{
					messageId: "unexpectedVar",
					type: "VariableDeclaration",
				},
			],
		},
		{
			code: "var foo = bar, toast = most;",
			output: "let foo = bar, toast = most;",
			languageOptions: {
				parserOptions: { ecmaFeatures: { globalReturn: true } },
			},
			errors: [
				{
					messageId: "unexpectedVar",

Review comment (suggestion):
What we have been following up untill now for TS cases is creating a separate `ruleTesterTypescript` instance. You can refer this PR https://github.com/eslint/eslint/pull/19560/

https://github.com/eslint/eslint/blob/58a171e8f0dcc1e599ac22bf8c386abacdbee424/tests/lib/rules/no-loss-of-precision.js#L322-L327

Code after:
const ruleTester = new RuleTester({
	languageOptions: { ecmaVersion: 6, sourceType: "script" },
});

ruleTester.run("no-var", rule, {
	valid: [
		"const JOE = 'schmoe';",
		"let moo = 'car';",
		{
			code: "const JOE = 'schmoe';",
			languageOptions: {
				parserOptions: { ecmaFeatures: { globalReturn: true } },
			},
		},
		{
			code: "let moo = 'car';",
			languageOptions: {
				parserOptions: { ecmaFeatures: { globalReturn: true } },
			},
		},
	],

	invalid: [
		{
			code: "var foo = bar;",
			output: "let foo = bar;",
			languageOptions: {
				parserOptions: { ecmaFeatures: { globalReturn: true } },
			},
			errors: [
				{
					messageId: "unexpectedVar",
					type: "VariableDeclaration",
				},
			],
		},
		{
			code: "var foo = bar, toast = most;",
			output: "let foo = bar, toast = most;",
			languageOptions: {
				parserOptions: { ecmaFeatures: { globalReturn: true } },
			},
			errors: [
				{
					messageId: "unexpectedVar",
					type: "VariableDeclaration",
				},
			],
		},
		{
			code: "var foo = bar; let toast = most;",
Repository: eslint/eslint
File: lib/rules/no-unassigned-vars.js (JavaScript)

Code before:
			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 ancestors = context.sourceCode.getAncestors(node);
				const insideDeclareModule = ancestors.some(
					ancestorNode =>
						ancestorNode.type === "TSModuleDeclaration" &&
						ancestorNode.declare === true,
				);
				const shouldSkip =
					node.init ||
					node.id.type !== "Identifier" ||
					declaration.kind === "const" ||
					declaration.declare ||
					insideDeclareModule;
				if (shouldSkip) {
					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
					return;

Review comment (performance):
Can we, instead of always getting all ancestors for each variable declarator, add a listener for this and set this flag in it? I think that would be more efficient.

```js
"TSModuleDeclaration[declare=true]"() {
	insideDeclareModule = true;
},
"TSModuleDeclaration[declare=true]:exit"() {
	insideDeclareModule = false;
},
```

Code after:
			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;
		let insideDeclareModule = false;

		return {
			"TSModuleDeclaration[declare=true]"() {
				insideDeclareModule = true;
			},
			"TSModuleDeclaration[declare=true]:exit"() {
				insideDeclareModule = false;
			},
			VariableDeclarator(node) {
				/** @type {import('estree').VariableDeclaration} */
				const declaration = node.parent;
				const shouldSkip =
					node.init ||
					node.id.type !== "Identifier" ||
					declaration.kind === "const" ||
					declaration.declare ||
					insideDeclareModule;
				if (shouldSkip) {
					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/types/index.d.ts (TypeScript)

Code before:
			options?:
				| {
						filter?: ((token: AST.Token) => boolean) | undefined;
						includeComments?: false | undefined;
						count?: number | undefined;
				  }
				| ((token: AST.Token) => boolean)
				| number,
		): AST.Token[];
		(
			left: ESTree.Node | AST.Token | ESTree.Comment,
			right: ESTree.Node | AST.Token | ESTree.Comment,
			options: {
				filter?:
					| ((token: AST.Token | ESTree.Comment) => boolean)
					| undefined;
				includeComments: boolean;
				count?: number | undefined;
			},
		): Array<AST.Token | ESTree.Comment>;
	}
}

// #endregion

type RuleContextNode = {
	type: string;
	loc?: ESTree.SourceLocation | null | undefined;
};

export namespace Rule {
	interface RuleModule
		extends RuleDefinition<{
			LangOptions: Linter.LanguageOptions;
			Code: SourceCode;
			RuleOptions: any[];
			Visitor: NodeListener;
			Node: RuleContextNode;
			MessageIds: string;
			ExtRuleDocs: {};
		}> {
		create(context: RuleContext): NodeListener;
	}

	type NodeTypes = ESTree.Node["type"];
	interface NodeListener extends RuleVisitor {
		ArrayExpression?:
			| ((node: ESTree.ArrayExpression & NodeParentExtension) => void)
			| undefined;
		"ArrayExpression:exit"?:
			| ((node: ESTree.ArrayExpression & NodeParentExtension) => void)

Review comment (suggestion):
I might call this `JSSyntaxElement` instead. That makes it clear that this type represents some kind of syntax rather than some kind of node. (Technically, tokens and comments are not nodes because they aren't part of the tree.)

Code after:
			options?:
				| {
						filter?: ((token: AST.Token) => boolean) | undefined;
						includeComments?: false | undefined;
						count?: number | undefined;
				  }
				| ((token: AST.Token) => boolean)
				| number,
		): AST.Token[];
		(
			left: ESTree.Node | AST.Token | ESTree.Comment,
			right: ESTree.Node | AST.Token | ESTree.Comment,
			options: {
				filter?:
					| ((token: AST.Token | ESTree.Comment) => boolean)
					| undefined;
				includeComments: boolean;
				count?: number | undefined;
			},
		): Array<AST.Token | ESTree.Comment>;
	}
}

// #endregion

export type JSSyntaxElement = {
	type: string;
	loc?: ESTree.SourceLocation | null | undefined;
};

export namespace Rule {
	interface RuleModule
		extends RuleDefinition<{
			LangOptions: Linter.LanguageOptions;
			Code: SourceCode;
			RuleOptions: any[];
			Visitor: NodeListener;
			Node: JSSyntaxElement;
			MessageIds: string;
			ExtRuleDocs: {};
		}> {
		create(context: RuleContext): NodeListener;
	}

	type NodeTypes = ESTree.Node["type"];
	interface NodeListener extends RuleVisitor {
		ArrayExpression?:
			| ((node: ESTree.ArrayExpression & NodeParentExtension) => void)
			| undefined;
		"ArrayExpression:exit"?:
			| ((node: ESTree.ArrayExpression & NodeParentExtension) => void)
Repository: eslint/eslint
File: lib/types/index.d.ts (TypeScript)

Code before:
						includeComments?: false | undefined;
						count?: number | undefined;
				  }
				| ((token: AST.Token) => boolean)
				| number,
		): AST.Token[];
		(
			left: ESTree.Node | AST.Token | ESTree.Comment,
			right: ESTree.Node | AST.Token | ESTree.Comment,
			options: {
				filter?:
					| ((token: AST.Token | ESTree.Comment) => boolean)
					| undefined;
				includeComments: boolean;
				count?: number | undefined;
			},
		): Array<AST.Token | ESTree.Comment>;
	}
}

// #endregion

type JSSyntaxElement = {
	type: string;
	loc?: ESTree.SourceLocation | null | undefined;
};

export namespace Rule {
	interface RuleModule
		extends RuleDefinition<{
			LangOptions: Linter.LanguageOptions;
			Code: SourceCode;
			RuleOptions: any[];
			Visitor: NodeListener;
			Node: JSSyntaxElement;
			MessageIds: string;
			ExtRuleDocs: {};
		}> {
		create(context: RuleContext): NodeListener;
	}

	type NodeTypes = ESTree.Node["type"];
	interface NodeListener extends RuleVisitor {
		ArrayExpression?:
			| ((node: ESTree.ArrayExpression & NodeParentExtension) => void)
			| undefined;
		"ArrayExpression:exit"?:
			| ((node: ESTree.ArrayExpression & NodeParentExtension) => void)
			| undefined;
		ArrayPattern?:
			| ((node: ESTree.ArrayPattern & NodeParentExtension) => void)

Review comment (suggestion):
```suggestion
export type JSSyntaxElement = {
	type: string;
	loc?: ESTree.SourceLocation | null | undefined;
};
```

I think it would be useful to export this type like we do with `JSRuleDefinition`. We could add a test to check that the type can be imported.


Code after:
						includeComments?: false | undefined;
						count?: number | undefined;
				  }
				| ((token: AST.Token) => boolean)
				| number,
		): AST.Token[];
		(
			left: ESTree.Node | AST.Token | ESTree.Comment,
			right: ESTree.Node | AST.Token | ESTree.Comment,
			options: {
				filter?:
					| ((token: AST.Token | ESTree.Comment) => boolean)
					| undefined;
				includeComments: boolean;
				count?: number | undefined;
			},
		): Array<AST.Token | ESTree.Comment>;
	}
}

// #endregion

export type JSSyntaxElement = {
	type: string;
	loc?: ESTree.SourceLocation | null | undefined;
};

export namespace Rule {
	interface RuleModule
		extends RuleDefinition<{
			LangOptions: Linter.LanguageOptions;
			Code: SourceCode;
			RuleOptions: any[];
			Visitor: NodeListener;
			Node: JSSyntaxElement;
			MessageIds: string;
			ExtRuleDocs: {};
		}> {
		create(context: RuleContext): NodeListener;
	}

	type NodeTypes = ESTree.Node["type"];
	interface NodeListener extends RuleVisitor {
		ArrayExpression?:
			| ((node: ESTree.ArrayExpression & NodeParentExtension) => void)
			| undefined;
		"ArrayExpression:exit"?:
			| ((node: ESTree.ArrayExpression & NodeParentExtension) => void)
			| undefined;
		ArrayPattern?:
			| ((node: ESTree.ArrayPattern & NodeParentExtension) => void)
Repository: eslint/eslint
File: lib/rules/no-restricted-globals.js (JavaScript)

Code before:
		}

		/**
		 * Check if the given name is a restricted global name.
		 * @param {string} name name of a variable
		 * @returns {boolean} whether the variable is a restricted global or not
		 * @private
		 */
		function isRestricted(name) {
			return Object.hasOwn(restrictedGlobalMessages, name);
		}

		/**
		 * Check if the given reference occurs within a TypeScript type context.
		 * @param {Reference} reference The variable reference to check.
		 * @returns {boolean} Whether the reference is in a type context.
		 * @private
		 */
		function isInTypeContext(reference) {
			const parent = reference.identifier.parent;

			return (
				parent.type === "TSTypeReference" ||
				parent.type === "TSInterfaceHeritage" ||
				parent.type === "TSClassImplements"
			);
		}

		return {
			Program(node) {
				const scope = sourceCode.getScope(node);

				// Report variables declared elsewhere (ex: variables defined as "global" by eslint)
				scope.variables.forEach(variable => {
					if (!variable.defs.length && isRestricted(variable.name)) {
						variable.references.forEach(reference => {
							if (!isInTypeContext(reference)) {
								reportReference(reference);
							}
						});
					}
				});

				// Report variables not declared at all
				scope.through.forEach(reference => {
					if (
						isRestricted(reference.identifier.name) &&
						!isInTypeContext(reference)
					) {
						reportReference(reference);
					}

Review comment (refactor):
We can simplify this by creating a set containing these strings and then just doing `typeNodes.has(parent.type)` instead of doing multiple comparisons.

Code after:
			const name = reference.identifier.name,
				customMessage = restrictedGlobalMessages[name],
				messageId = customMessage ? "customMessage" : "defaultMessage";

			context.report({
				node: reference.identifier,
				messageId,
				data: {
					name,
					customMessage,
				},
			});
		}

		/**
		 * Check if the given name is a restricted global name.
		 * @param {string} name name of a variable
		 * @returns {boolean} whether the variable is a restricted global or not
		 * @private
		 */
		function isRestricted(name) {
			return Object.hasOwn(restrictedGlobalMessages, name);
		}

		/**
		 * Check if the given reference occurs within a TypeScript type context.
		 * @param {Reference} reference The variable reference to check.
		 * @returns {boolean} Whether the reference is in a type context.
		 * @private
		 */
		function isInTypeContext(reference) {
			const parent = reference.identifier.parent;

			return TYPE_NODES.has(parent.type);
		}

		return {
			Program(node) {
				const scope = sourceCode.getScope(node);

				// Report variables declared elsewhere (ex: variables defined as "global" by eslint)
				scope.variables.forEach(variable => {
					if (!variable.defs.length && isRestricted(variable.name)) {
						variable.references.forEach(reference => {
							if (!isInTypeContext(reference)) {
								reportReference(reference);
							}
						});
					}
				});
Repository: eslint/eslint
File: lib/rules/no-unused-vars.js (JavaScript)

Code before:
								config.varsIgnorePattern &&
								config.varsIgnorePattern.test(def.name.name)
							) {
								if (
									config.reportUsedIgnorePattern &&
									isUsedVariable(variable)
								) {
									context.report({
										node: def.name,
										messageId: "usedIgnoredVar",
										data: getUsedIgnoredMessageData(
											variable,
											"variable",
										),
									});
								}

								continue;
							}
						}
					}

					if (
						!isUsedVariable(variable) &&
						!isExported(variable) &&
						!usesExplicitResourceManagement(variable) &&
						!hasRestSpreadSibling(variable)
					) {
						unusedVars.push(variable);
					}
				}
			}

			for (i = 0, l = childScopes.length; i < l; ++i) {
				collectUnusedVariables(childScopes[i], unusedVars);
			}

			return unusedVars;
		}

		/**
		 * fixes unused variables
		 * @param {Object} fixer fixer object
		 * @param {Object} unusedVar unused variable to fix
		 * @returns {Object} fixer object
		 */
		function handleFixes(fixer, unusedVar) {
			const id = unusedVar.identifiers[0];
			const parent = id.parent;
			const parentType = parent.type;
			const tokenBefore = sourceCode.getTokenBefore(id);

Review comment (bug):
While this variable is used while disposing, thus technically used, I think it's an edge case and is more likely that the "real" use is missing. Perhaps this should be behind an option?

Code after:
							) {
								continue;
							}
						} else {
							// skip ignored variables
							if (
								config.varsIgnorePattern &&
								config.varsIgnorePattern.test(def.name.name)
							) {
								if (
									config.reportUsedIgnorePattern &&
									isUsedVariable(variable)
								) {
									context.report({
										node: def.name,
										messageId: "usedIgnoredVar",
										data: getUsedIgnoredMessageData(
											variable,
											"variable",
										),
									});
								}

								continue;
							}
						}
					}

					if (
						!isUsedVariable(variable) &&
						!isExported(variable) &&
						!(
							config.ignoreUsingDeclarations &&
							usesExplicitResourceManagement(variable)
						) &&
						!hasRestSpreadSibling(variable)
					) {
						unusedVars.push(variable);
					}
				}
			}

			for (i = 0, l = childScopes.length; i < l; ++i) {
				collectUnusedVariables(childScopes[i], unusedVars);
			}

			return unusedVars;
		}

		/**
		 * fixes unused variables