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

Code before:
		 * test runners (eg. vitest) fail for empty test suites.
		 */
		this.constructor.describe(ruleName, () => {
			if (test.valid.length > 0) {
				this.constructor.describe("valid", () => {
					const seenTestCases = new Set();
					test.valid.forEach(valid => {
						const item = normalizeTestCase(valid);
						this.constructor[valid.only ? "itOnly" : "it"](
							sanitize(item.name || item.code),
							() => {
								try {
									assertValidTestCase(item, seenTestCases);
									runHook(item, "before");
									testValidTemplate(item);
								} finally {
									runHook(item, "after");
								}
							},
						);
					});
				});
			}

			if (test.invalid.length > 0) {
				this.constructor.describe("invalid", () => {
					const seenTestCases = new Set();
					test.invalid.forEach(invalid => {
						const item = normalizeTestCase(invalid);
						this.constructor[item.only ? "itOnly" : "it"](
							sanitize(item.name || item.code),
							() => {
								try {
									assertInvalidTestCase(
										item,
										seenTestCases,
										ruleName,
									);
									runHook(item, "before");
									testInvalidTemplate(item);
								} finally {
									runHook(item, "after");
								}
							},
						);
					});
				});
			}
		});
	}
}

Review comment (suggestion):
I think it would be safer to keep calling `before()` before checks, because we're calling it as a method so it can modify the test.

Code after:
					result.output,
					item.code,
					"The rule fixed the code. Please add 'output' property.",
				);
			}

			assertASTDidntChange(result.beforeAST, result.afterAST);
		}

		/*
		 * This creates a mocha test suite and pipes all supplied info through
		 * one of the templates above.
		 * The test suites for valid/invalid are created conditionally as
		 * test runners (eg. vitest) fail for empty test suites.
		 */
		this.constructor.describe(ruleName, () => {
			if (test.valid.length > 0) {
				this.constructor.describe("valid", () => {
					const seenTestCases = new Set();
					test.valid.forEach(valid => {
						const item = normalizeTestCase(valid);
						this.constructor[valid.only ? "itOnly" : "it"](
							sanitize(item.name || item.code),
							() => {
								try {
									runHook(item, "before");
									assertValidTestCase(item, seenTestCases);
									testValidTemplate(item);
								} finally {
									runHook(item, "after");
								}
							},
						);
					});
				});
			}

			if (test.invalid.length > 0) {
				this.constructor.describe("invalid", () => {
					const seenTestCases = new Set();
					test.invalid.forEach(invalid => {
						const item = normalizeTestCase(invalid);
						this.constructor[item.only ? "itOnly" : "it"](
							sanitize(item.name || item.code),
							() => {
								try {
									runHook(item, "before");
									assertInvalidTestCase(
										item,
										seenTestCases,
										ruleName,
Repository: eslint/eslint
File: lib/rule-tester/rule-tester.js (JavaScript)

Code before:
									freezeDeeply(context.settings);
									freezeDeeply(context.parserOptions);

									// freezeDeeply(context.languageOptions);

									return rule.create(context);
								},
							}),
						},
					},
				},
				language: defaultConfig[0].language,
			},
			...defaultRuleTesterConfig,
		];

		/**
		 * Runs a hook on the given item when it's assigned to the given property
		 * @param {Object} item Item to run the hook on
		 * @param {string} prop The property having the hook assigned to
		 * @throws {Error} If the property is not a function or that function throws an error
		 * @returns {void}
		 * @private
		 */
		function runHook(item, prop) {
			if (item[prop] !== void 0) {
				assert.strictEqual(
					typeof item[prop],
					"function",
					`Optional test case property '${prop}' must be a function`,
				);
				item[prop]();
			}
		}
		/**
		 * Run the rule for the given item
		 * @param {Object} item Item to run the rule against
		 * @throws {Error} If an invalid schema.
		 * @returns {Object} Eslint run result
		 * @private
		 */
		function runRuleForItem(item) {
			const flatConfigArrayOptions = {
				baseConfig,
			};

			if (item.filename) {
				flatConfigArrayOptions.basePath =
					path.parse(item.filename).root || void 0;
			}


Review comment (suggestion):
```suggestion
			if (hasOwnProperty(item, prop)) {
```

The difference would be when the property is set to `undefined`, or when it exists in the prototype chain.

Code after:
						},

						/*
						 * The rules key on the default plugin is a proxy to lazy-load
						 * just the rules that are needed. So, don't create a new object
						 * here, just use the default one to keep that performance
						 * enhancement.
						 */
						rules: defaultConfig[0].plugins["@"].rules,
						languages: defaultConfig[0].plugins["@"].languages,
					},
					"rule-to-test": {
						rules: {
							[ruleName]: Object.assign({}, rule, {
								// Create a wrapper rule that freezes the `context` properties.
								create(context) {
									freezeDeeply(context.options);
									freezeDeeply(context.settings);
									freezeDeeply(context.parserOptions);

									// freezeDeeply(context.languageOptions);

									return rule.create(context);
								},
							}),
						},
					},
				},
				language: defaultConfig[0].language,
			},
			...defaultRuleTesterConfig,
		];

		/**
		 * Runs a hook on the given item when it's assigned to the given property
		 * @param {Object} item Item to run the hook on
		 * @param {string} prop The property having the hook assigned to
		 * @throws {Error} If the property is not a function or that function throws an error
		 * @returns {void}
		 * @private
		 */
		function runHook(item, prop) {
			if (hasOwnProperty(item, prop)) {
				assert.strictEqual(
					typeof item[prop],
					"function",
					`Optional test case property '${prop}' must be a function`,
				);
				item[prop]();
			}
		}
Repository: eslint/eslint
File: lib/rule-tester/rule-tester.js (JavaScript)

Code before:
			);

			/*
			 * Modify the returned config so that the parser is wrapped to catch
			 * access of the start/end properties. This method is called just
			 * once per code snippet being tested, so each test case gets a clean
			 * parser.
			 */
			configs[ConfigArraySymbol.finalizeConfig] = function (...args) {
				// can't do super here :(
				const proto = Object.getPrototypeOf(this);
				const calculatedConfig = proto[
					ConfigArraySymbol.finalizeConfig
				].apply(this, args);

				// wrap the parser to catch start/end property access
				if (calculatedConfig.language === jslang) {
					calculatedConfig.languageOptions.parser = wrapParser(
						calculatedConfig.languageOptions.parser,
					);
				}

				return calculatedConfig;
			};

			const { code, filename, options = [] } = item;
			let output, beforeAST, afterAST;

			/*
			 * Assumes everything on the item is a config except for the
			 * parameters used by this tester
			 */
			const itemConfig = { ...item };

			for (const parameter of RuleTesterParameters) {
				delete itemConfig[parameter];
			}

			/*
			 * Create the config object from the tester config and this item
			 * specific configurations.
			 */
			configs.push(itemConfig);

			configs.push({
				rules: {
					[ruleId]: [1, ...options],
				},
			});

			let schema;

Review comment (suggestion):
`filename` and `options` were used only if they are test case object's own properties.

Code after:
		 * @throws {Error} If an invalid schema.
		 * @returns {Object} Eslint run result
		 * @private
		 */
		function runRuleForItem(item) {
			const code = item.code;
			const filename = hasOwnProperty(item, "filename")
				? item.filename
				: void 0;
			const options = hasOwnProperty(item, "options") ? item.options : [];
			const flatConfigArrayOptions = {
				baseConfig,
			};

			if (filename) {
				flatConfigArrayOptions.basePath =
					path.parse(filename).root || void 0;
			}

			const configs = new FlatConfigArray(
				testerConfig,
				flatConfigArrayOptions,
			);

			/*
			 * Modify the returned config so that the parser is wrapped to catch
			 * access of the start/end properties. This method is called just
			 * once per code snippet being tested, so each test case gets a clean
			 * parser.
			 */
			configs[ConfigArraySymbol.finalizeConfig] = function (...args) {
				// can't do super here :(
				const proto = Object.getPrototypeOf(this);
				const calculatedConfig = proto[
					ConfigArraySymbol.finalizeConfig
				].apply(this, args);

				// wrap the parser to catch start/end property access
				if (calculatedConfig.language === jslang) {
					calculatedConfig.languageOptions.parser = wrapParser(
						calculatedConfig.languageOptions.parser,
					);
				}

				return calculatedConfig;
			};

			let output, beforeAST, afterAST;

			/*
			 * Assumes everything on the item is a config except for the
Repository: eslint/eslint
File: lib/rules/object-shorthand.js (JavaScript)

Code before:
					if (
						node.value.type === "FunctionExpression" ||
						(node.value.type === "ArrowFunctionExpression" &&
							node.value.body.type === "BlockStatement" &&
							AVOID_EXPLICIT_RETURN_ARROWS &&
							!arrowsWithLexicalIdentifiers.has(node.value))
					) {
						context.report({
							node,
							messageId: "expectedMethodShorthand",
							fix: fixer => makeFunctionShorthand(fixer, node),
						});
					}
				} else if (
					node.value.type === "Identifier" &&
					node.key.name === node.value.name &&
					APPLY_TO_PROPS
				) {
					// Skip if there are JSDoc comments inside the property (e.g., JSDoc type annotations)
					const comments = sourceCode.getCommentsInside(node);
					if (
						comments.some(
							comment =>
								comment.type === "Block" &&
								comment.value.startsWith("*") &&
								comment.value.includes("@type"),
						)
					) {
						return;
					}

					// {x: x} should be written as {x}
					context.report({
						node,
						messageId: "expectedPropertyShorthand",
						fix(fixer) {
							// x: /* */ x
							// x: (/* */ x)
							if (sourceCode.getCommentsInside(node).length > 0) {
								return null;
							}

							return fixer.replaceText(node, node.value.name);
						},
					});
				} else if (
					node.value.type === "Identifier" &&
					node.key.type === "Literal" &&
					node.key.value === node.value.name &&
					APPLY_TO_PROPS
				) {

Review comment (suggestion):
```suggestion
								comment.value.startsWith("* @type {")
```
Certain comments may include unexpected `@type`.

Code after:
					if (
						node.value.type === "FunctionExpression" ||
						(node.value.type === "ArrowFunctionExpression" &&
							node.value.body.type === "BlockStatement" &&
							AVOID_EXPLICIT_RETURN_ARROWS &&
							!arrowsWithLexicalIdentifiers.has(node.value))
					) {
						context.report({
							node,
							messageId: "expectedMethodShorthand",
							fix: fixer => makeFunctionShorthand(fixer, node),
						});
					}
				} else if (
					node.value.type === "Identifier" &&
					node.key.name === node.value.name &&
					APPLY_TO_PROPS
				) {
					// Skip if there are JSDoc comments inside the property (e.g., JSDoc type annotations)
					const comments = sourceCode.getCommentsInside(node);
					if (
						comments.some(
							comment =>
								comment.type === "Block" &&
								JSDOC_COMMENT_REGEX.test(comment.value) &&
								comment.value.includes("@type"),
						)
					) {
						return;
					}

					// {x: x} should be written as {x}
					context.report({
						node,
						messageId: "expectedPropertyShorthand",
						fix(fixer) {
							// x: /* */ x
							// x: (/* */ x)
							if (sourceCode.getCommentsInside(node).length > 0) {
								return null;
							}

							return fixer.replaceText(node, node.value.name);
						},
					});
				} else if (
					node.value.type === "Identifier" &&
					node.key.type === "Literal" &&
					node.key.value === node.value.name &&
					APPLY_TO_PROPS
				) {
Repository: eslint/eslint
File: lib/types/index.d.ts (TypeScript)

Code before:
	SourceRange,
	TextSourceCode,
	TraversalStep,
	RulesConfig,
	GlobalAccess,
	GlobalsConfig,
	LinterOptionsConfig,
	EnvironmentConfig,
	ObjectMetaProperties as CoreObjectMetaProperties,
	Plugin as CorePlugin,
	LintMessage as CoreLintMessage,
	Processor as CoreProcessor,
	ConfigObject,
	LegacyConfigObject,
	SeverityName,
	SeverityLevel,
	Severity as CoreSeverity,
	EcmaVersion as CoreEcmaVersion,
	ConfigOverride as CoreConfigOverride,
	ProcessorFile as CoreProcessorFile,
	JavaScriptParserOptionsConfig,
	RulesMeta,
	RuleTextEditor,
	RuleTextEdit,
} from "@eslint/core";
import { JSONSchema4 } from "json-schema";
import { LegacyESLint } from "./use-at-your-own-risk.js";

export namespace AST {
	type TokenType =
		| "Boolean"
		| "Null"
		| "Identifier"
		| "Keyword"
		| "Punctuator"
		| "JSXIdentifier"
		| "JSXText"
		| "Numeric"
		| "String"
		| "RegularExpression";

	interface Token {
		type: TokenType;
		value: string;
		range: Range;
		loc: SourceLocation;
	}

	interface SourceLocation {
		start: ESTree.Position;
		end: ESTree.Position;

Review comment (suggestion):
This import is no longer used. Seems safe to remove.

```suggestion
```

Code after:
	SourceRange,
	TextSourceCode,
	TraversalStep,
	RulesConfig,
	GlobalAccess,
	GlobalsConfig,
	LinterOptionsConfig,
	EnvironmentConfig,
	ObjectMetaProperties as CoreObjectMetaProperties,
	Plugin as CorePlugin,
	LintMessage as CoreLintMessage,
	Processor as CoreProcessor,
	ConfigObject,
	LegacyConfigObject,
	SeverityName,
	SeverityLevel,
	Severity as CoreSeverity,
	EcmaVersion as CoreEcmaVersion,
	ConfigOverride as CoreConfigOverride,
	ProcessorFile as CoreProcessorFile,
	JavaScriptParserOptionsConfig,
	RulesMeta,
	RuleTextEditor,
	RuleTextEdit,
} from "@eslint/core";
import { LegacyESLint } from "./use-at-your-own-risk.js";

export namespace AST {
	type TokenType =
		| "Boolean"
		| "Null"
		| "Identifier"
		| "Keyword"
		| "Punctuator"
		| "JSXIdentifier"
		| "JSXText"
		| "Numeric"
		| "String"
		| "RegularExpression";

	interface Token {
		type: TokenType;
		value: string;
		range: Range;
		loc: SourceLocation;
	}

	interface SourceLocation {
		start: ESTree.Position;
		end: ESTree.Position;
	}
Repository: eslint/eslint
File: lib/types/index.d.ts (TypeScript)

Code before:
		/**
		 * Specifies whether the rule can return suggestions (defaults to `false` if omitted).
		 * Mandatory for rules that provide suggestions.
		 */
		hasSuggestions?: boolean | undefined;
	}

	interface RuleContext
		extends CoreRuleContext<{
			LangOptions: Linter.LanguageOptions;
			Code: SourceCode;
			RuleOptions: any[];
			Node: JSSyntaxElement;
			MessageIds: string;
		}> {}

	type ReportFixer = (fixer: RuleFixer) => Fix | Iterable<Fix> | null;

	interface ReportDescriptorOptionsBase {
		data?: Record<string, unknown> | undefined;

		fix?: ReportFixer | null | undefined;
	}

	interface SuggestionReportOptions {
		data?: Record<string, unknown> | undefined;

		fix: ReportFixer;
	}

	type SuggestionDescriptorMessage = { desc: string } | { messageId: string };
	type SuggestionReportDescriptor = SuggestionDescriptorMessage &
		SuggestionReportOptions;

	interface ReportDescriptorOptions extends ReportDescriptorOptionsBase {
		suggest?: SuggestionReportDescriptor[] | null | undefined;
	}

	type ReportDescriptor = ReportDescriptorMessage &
		ReportDescriptorLocation &
		ReportDescriptorOptions;
	type ReportDescriptorMessage = { message: string } | { messageId: string };
	type ReportDescriptorLocation =
		| { node: JSSyntaxElement }
		| { loc: AST.SourceLocation | ESTree.Position };

	interface RuleFixer {
		insertTextAfter(
			nodeOrToken: ESTree.Node | AST.Token,
			text: string,
		): Fix;

Review comment (suggestion):
Please also apply the fix from above.
Maybe it would also be a good idea to use a type alias to keep the type for `data` in sync.

Code after:
		/**
		 * Specifies whether the rule can return suggestions (defaults to `false` if omitted).
		 * Mandatory for rules that provide suggestions.
		 */
		hasSuggestions?: boolean | undefined;
	}

	interface RuleContext
		extends CoreRuleContext<{
			LangOptions: Linter.LanguageOptions;
			Code: SourceCode;
			RuleOptions: any[];
			Node: JSSyntaxElement;
			MessageIds: string;
		}> {}

	type ReportFixer = (
		fixer: RuleFixer,
	) => null | Fix | IterableIterator<Fix> | Fix[];

	interface ReportDescriptorOptionsBase {
		data?: { [key: string]: string };

		fix?: null | ReportFixer;
	}

	interface SuggestionReportOptions {
		data?: { [key: string]: string };

		fix: ReportFixer;
	}

	type SuggestionDescriptorMessage = { desc: string } | { messageId: string };
	type SuggestionReportDescriptor = SuggestionDescriptorMessage &
		SuggestionReportOptions;

	interface ReportDescriptorOptions extends ReportDescriptorOptionsBase {
		suggest?: SuggestionReportDescriptor[] | null | undefined;
	}

	type ReportDescriptor = ReportDescriptorMessage &
		ReportDescriptorLocation &
		ReportDescriptorOptions;
	type ReportDescriptorMessage = { message: string } | { messageId: string };
	type ReportDescriptorLocation =
		| { node: ESTree.Node }
		| { loc: AST.SourceLocation | { line: number; column: number } };

	interface RuleFixer {
		insertTextAfter(
			nodeOrToken: ESTree.Node | AST.Token,
Repository: eslint/eslint
File: tools/update-eslint-recommended.js (JavaScript)

Code before:
const recommendedRulesText = JSON.stringify(
	recommendedRule,
	null,
	4,
).replaceAll("\n", "\n    ");

//-----------------------------------------------------------------------------
// Main
//-----------------------------------------------------------------------------

const code = `/*
 * WARNING: This file is autogenerated using the tools/update-eslint-recommended.js
 * script. Do not edit manually.
 */
"use strict";

/*
 * IMPORTANT!
 *
 * We cannot add a "name" property to this object because it's still used in eslintrc
 * which doesn't support the "name" property. If we add a "name" property, it will
 * cause an error.
 */

module.exports = Object.freeze({
	rules: Object.freeze(${recommendedRulesText}),
});
`;

fs.writeFileSync(
	"./packages/js/src/configs/eslint-recommended.js",
	code,
	"utf8",
);


Review comment (suggestion):
```suggestion
    rules: Object.freeze(${recommendedRulesText}),
```

Since the rest of the generated file uses spaces, it would be consistent to have 4 spaces instead of a tab here. 

Code after:
const recommendedRulesText = JSON.stringify(
	recommendedRule,
	null,
	4,
).replaceAll("\n", "\n    ");

//-----------------------------------------------------------------------------
// Main
//-----------------------------------------------------------------------------

const code = `/*
 * WARNING: This file is autogenerated using the tools/update-eslint-recommended.js
 * script. Do not edit manually.
 */
"use strict";

/*
 * IMPORTANT!
 *
 * We cannot add a "name" property to this object because it's still used in eslintrc
 * which doesn't support the "name" property. If we add a "name" property, it will
 * cause an error.
 */

module.exports = Object.freeze({
    rules: Object.freeze(${recommendedRulesText}),
});
`;

fs.writeFileSync(
	"./packages/js/src/configs/eslint-recommended.js",
	code,
	"utf8",
);
Repository: eslint/eslint
File: lib/rules/max-params.js (JavaScript)

Code before:
						type: "integer",
						minimum: 0,
					},
					{
						type: "object",
						properties: {
							maximum: {
								type: "integer",
								minimum: 0,
							},
							max: {
								type: "integer",
								minimum: 0,
							},
							countVoidThis: {
								type: "boolean",
								description:
									"Whether to count a `this` declaration when the type is `void`.",
							},
							countThis: {
								enum: ["never", "except-void", "always"],
								description:
									"Whether to count a `this` declaration.",
							},
						},
						additionalProperties: false,
					},
				],
			},
		],
		messages: {
			exceed: "{{name}} has too many parameters ({{count}}). Maximum allowed is {{max}}.",
		},
	},

	create(context) {
		const sourceCode = context.sourceCode;
		const option = context.options[0];
		let numParams = 3;
		let countThis = "except-void";

		if (typeof option === "object") {
			if (
				Object.hasOwn(option, "maximum") ||
				Object.hasOwn(option, "max")
			) {
				numParams = option.maximum || option.max;
			}

			countThis = option.countThis;
			if (!countThis && Object.hasOwn(option, "countVoidThis")) {

Review comment (suggestion):
```suggestion
						additionalProperties: false,
						not: { required: ["countVoidThis", "countThis"] },
```

We can add this to make these options mutually exclusive.

Code after:
						type: "integer",
						minimum: 0,
					},
					{
						type: "object",
						properties: {
							maximum: {
								type: "integer",
								minimum: 0,
							},
							max: {
								type: "integer",
								minimum: 0,
							},
							countVoidThis: {
								type: "boolean",
								description:
									"Whether to count a `this` declaration when the type is `void`.",
							},
							countThis: {
								enum: ["never", "except-void", "always"],
								description:
									"Whether to count a `this` declaration.",
							},
						},
						additionalProperties: false,
						not: { required: ["countVoidThis", "countThis"] },
					},
				],
			},
		],
		messages: {
			exceed: "{{name}} has too many parameters ({{count}}). Maximum allowed is {{max}}.",
		},
	},

	create(context) {
		const sourceCode = context.sourceCode;
		const option = context.options[0];
		let numParams = 3;
		let countThis = "except-void";

		if (typeof option === "object") {
			if (
				Object.hasOwn(option, "maximum") ||
				Object.hasOwn(option, "max")
			) {
				numParams = option.maximum || option.max;
			}

			countThis = option.countThis;
Repository: eslint/eslint
File: lib/types/rules.d.ts (TypeScript)

Code before:
					max: number;
			  }>,
		]
	>;

	/**
	 * Rule to enforce a maximum number of parameters in function definitions.
	 *
	 * @since 0.0.9
	 * @see https://eslint.org/docs/latest/rules/max-params
	 */
	"max-params": Linter.RuleEntry<
		[
			| number
			| Partial<{
					/**
					 * @deprecated
					 * @default 3
					 */
					maximum: number;
					/**
					 * @default 3
					 */
					max: number;
					/**
					 * @deprecated Replaced with `countThis: 'except-void'
					 * @default false
					 */
					countVoidThis: boolean;
					/**
					 * @default 'except-void'
					 */
					countThis: "always" | "never" | "except-void";
			  }>,
		]
	>;

	/**
	 * Rule to enforce a maximum number of statements allowed in function blocks.
	 *
	 * @since 0.0.9
	 * @see https://eslint.org/docs/latest/rules/max-statements
	 */
	"max-statements": Linter.RuleEntry<
		[
			(
				| number
				| Partial<{
						/**
						 * @deprecated
						 * @default 10

Review comment (suggestion):
```suggestion
					 * @deprecated Replaced with `countThis`
```

Since the equivalent value for `countThis` depends on the value of `countVoidThis`, it might be better to just mention the other option.

Code after:
					max: number;
			  }>,
		]
	>;

	/**
	 * Rule to enforce a maximum number of parameters in function definitions.
	 *
	 * @since 0.0.9
	 * @see https://eslint.org/docs/latest/rules/max-params
	 */
	"max-params": Linter.RuleEntry<
		[
			| number
			| Partial<{
					/**
					 * @deprecated
					 * @default 3
					 */
					maximum: number;
					/**
					 * @default 3
					 */
					max: number;
					/**
					 * @deprecated Replaced with `countThis'
					 * @default false
					 */
					countVoidThis: boolean;
					/**
					 * @default "except-void"
					 */
					countThis: "always" | "never" | "except-void";
			  }>,
		]
	>;

	/**
	 * Rule to enforce a maximum number of statements allowed in function blocks.
	 *
	 * @since 0.0.9
	 * @see https://eslint.org/docs/latest/rules/max-statements
	 */
	"max-statements": Linter.RuleEntry<
		[
			(
				| number
				| Partial<{
						/**
						 * @deprecated
						 * @default 10