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

Code before:
                }],
                "default-case": [2, {}]
            });

        });


        it("should throw rule validation error for camelcase", async () => {


            /* eslint-disable-next-line prefer-const -- need to allow mutation inside FlatConfigArray */
            let ruleConfig = ["error", {}];

            const configs = new FlatConfigArray([{
                rules: {
                    camelcase: ruleConfig,
                    "default-case": ruleConfig,

                    /* eslint-disable-next-line no-dupe-keys -- needed for testing */
                    camelcase: [
                        "error",
                        {
                            ignoreDestructuring: Date
                        }

                    ]
                }
            }]);

            configs.normalizeSync();

            // exact error may differ based on structuredClone implementation so just test prefix
            assert.throws(() => {
                configs.getConfig("foo.js");
            }, /Key "rules": Key "camelcase":/u);

        });

    });
});


Review comment (refactor):
```suggestion
                    "default-case": ruleConfig,

                    camelcase: [
                        "error",
                        {
                            ignoreDestructuring: Date
                        }

                    ]
```

I believe duplicate keys don't affect this testing, the previous one is just overwritten in the created object. Perhaps the configuration was intended to have two config objects? (though the test might be even better with one to make sure we're always cloning rule configs)

Code after:
                "default-case": [2, {}]
            });

        });


        it("should throw rule validation error for camelcase", async () => {

            const ruleConfig = ["error", {}];

            const configs = new FlatConfigArray([
                {
                    rules: {
                        camelcase: ruleConfig
                    }
                },
                {
                    rules: {
                        "default-case": ruleConfig,


                        camelcase: [
                            "error",
                            {
                                ignoreDestructuring: Date
                            }

                        ]
                    }
                }
            ]);

            configs.normalizeSync();

            // exact error may differ based on structuredClone implementation so just test prefix
            assert.throws(() => {
                configs.getConfig("foo.js");
            }, /Key "rules": Key "camelcase":/u);

        });

    });
});
Repository: eslint/eslint
File: tools/check-rule-examples.js (JavaScript)

Code before:
        fatalErrorCount,
        messages: problems
    };
}

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

// determine which files to check
const filenames = process.argv.slice(2);

(async function() {
    const results = await Promise.all(filenames.map(checkFile));

    if (results.every(result => result.errorCount === 0)) {
        return;
    }

    const formatter = require("../lib/cli-engine/formatters/stylish");
    const output = formatter(results);

    console.error(output);
    process.exitCode = 1;
}());


Review comment (suggestion):
Ideally, the whole validation could be done with ESLint when the work on language plugins is done. For now, I was able to reuse the formatter functionality.

Code after:
        filePath: filename,
        errorCount: problems.length,
        warningCount: 0,
        fatalErrorCount,
        messages: problems
    };
}

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

const patterns = process.argv.slice(2);

(async function() {
    const globAsync = promisify(glob);

    // determine which files to check
    const filenames = (await Promise.all(patterns.map(pattern => globAsync(pattern, { nonull: true })))).flat();
    const results = await Promise.all(filenames.map(checkFile));

    if (results.every(result => result.errorCount === 0)) {
        return;
    }

    const formatter = require("../lib/cli-engine/formatters/stylish");
    const output = formatter(results);

    console.error(output);
    process.exitCode = 1;
}());
Repository: eslint/eslint
File: tests/lib/rules/no-restricted-properties.js (JavaScript)

Code before:
        }, {
            code: "({ bad: bar }) => {};",
            options: [{ property: "bad" }],
            parserOptions: { ecmaVersion: 6 },
            errors: [{
                messageId: "restrictedProperty",
                data: {
                    propertyName: "bad",
                    message: ""
                }
            }]
        }, {
            code: "({ bar: { bad } = {} }) => {};",
            options: [{ property: "bad" }],
            parserOptions: { ecmaVersion: 6 },
            errors: [{
                messageId: "restrictedProperty",
                data: {
                    propertyName: "bad",
                    message: ""
                }
            }]
        }, {
            code: "[{ bad }] = foo;",
            options: [{ property: "bad" }],
            parserOptions: { ecmaVersion: 6 },
            errors: [{
                messageId: "restrictedProperty",
                data: {
                    propertyName: "bad",
                    message: ""
                }
            }]
        }, {
            code: "const [{ bad }] = foo;",
            options: [{ property: "bad" }],
            parserOptions: { ecmaVersion: 6 },
            errors: [{
                messageId: "restrictedProperty",
                data: {
                    propertyName: "bad",
                    message: ""
                }
            }]
        }
    ]
});


Review comment (suggestion):
```suggestion
            languageOptions: { ecmaVersion: 6 },
```

Code after:
        }, {
            code: "({ bad: bar }) => {};",
            options: [{ property: "bad" }],
            languageOptions: { ecmaVersion: 6 },
            errors: [{
                messageId: "restrictedProperty",
                data: {
                    propertyName: "bad",
                    message: ""
                }
            }]
        }, {
            code: "({ bar: { bad } = {} }) => {};",
            options: [{ property: "bad" }],
            languageOptions: { ecmaVersion: 6 },
            errors: [{
                messageId: "restrictedProperty",
                data: {
                    propertyName: "bad",
                    message: ""
                }
            }]
        }, {
            code: "[{ bad }] = foo;",
            options: [{ property: "bad" }],
            languageOptions: { ecmaVersion: 6 },
            errors: [{
                messageId: "restrictedProperty",
                data: {
                    propertyName: "bad",
                    message: ""
                }
            }]
        }, {
            code: "const [{ bad }] = foo;",
            options: [{ property: "bad" }],
            languageOptions: { ecmaVersion: 6 },
            errors: [{
                messageId: "restrictedProperty",
                data: {
                    propertyName: "bad",
                    message: ""
                }
            }]
        }
    ]
});
Repository: eslint/eslint
File: tests/lib/rules/no-restricted-properties.js (JavaScript)

Code before:
        }, {
            code: "({ bar: { bad } = {} }) => {};",
            options: [{ property: "bad" }],
            parserOptions: { ecmaVersion: 6 },
            errors: [{
                messageId: "restrictedProperty",
                data: {
                    propertyName: "bad",
                    message: ""
                }
            }]
        }, {
            code: "[{ bad }] = foo;",
            options: [{ property: "bad" }],
            parserOptions: { ecmaVersion: 6 },
            errors: [{
                messageId: "restrictedProperty",
                data: {
                    propertyName: "bad",
                    message: ""
                }
            }]
        }, {
            code: "const [{ bad }] = foo;",
            options: [{ property: "bad" }],
            parserOptions: { ecmaVersion: 6 },
            errors: [{
                messageId: "restrictedProperty",
                data: {
                    propertyName: "bad",
                    message: ""
                }
            }]
        }
    ]
});


Review comment (suggestion):
```suggestion
            languageOptions: { ecmaVersion: 6 },
```

Code after:
        }, {
            code: "({ bar: { bad } = {} }) => {};",
            options: [{ property: "bad" }],
            languageOptions: { ecmaVersion: 6 },
            errors: [{
                messageId: "restrictedProperty",
                data: {
                    propertyName: "bad",
                    message: ""
                }
            }]
        }, {
            code: "[{ bad }] = foo;",
            options: [{ property: "bad" }],
            languageOptions: { ecmaVersion: 6 },
            errors: [{
                messageId: "restrictedProperty",
                data: {
                    propertyName: "bad",
                    message: ""
                }
            }]
        }, {
            code: "const [{ bad }] = foo;",
            options: [{ property: "bad" }],
            languageOptions: { ecmaVersion: 6 },
            errors: [{
                messageId: "restrictedProperty",
                data: {
                    propertyName: "bad",
                    message: ""
                }
            }]
        }
    ]
});
Repository: eslint/eslint
File: lib/rules/no-implicit-coercion.js (JavaScript)

Code before:
/**
 * @fileoverview A rule to disallow the type conversions with shorter notations.
 * @author Toru Nagashima
 */

"use strict";

const astUtils = require("./utils/ast-utils");

//------------------------------------------------------------------------------
// Helpers
//------------------------------------------------------------------------------

const INDEX_OF_PATTERN = /^(?:i|lastI)ndexOf$/u;
const ALLOWABLE_OPERATORS = ["~", "!!", "+", "--", "-", "*"];

/**
 * Parses and normalizes an option object.
 * @param {Object} options An option object to parse.
 * @returns {Object} The parsed and normalized option object.
 */
function parseOptions(options) {
    return {
        boolean: "boolean" in options ? options.boolean : true,
        number: "number" in options ? options.number : true,
        string: "string" in options ? options.string : true,
        disallowTemplateShorthand: "disallowTemplateShorthand" in options ? options.disallowTemplateShorthand : false,
        allow: options.allow || []
    };
}

/**
 * Checks whether or not a node is a double logical negating.
 * @param {ASTNode} node An UnaryExpression node to check.
 * @returns {boolean} Whether or not the node is a double logical negating.
 */
function isDoubleLogicalNegating(node) {
    return (
        node.operator === "!" &&
        node.argument.type === "UnaryExpression" &&

Review comment (suggestion):
I think this means `-(-expr)` correct? If so, I think this is misleading since this looks like the decrement operator (`--`) which does change the value. I think we need to rethink how this is represented.

Code after:
/**
 * @fileoverview A rule to disallow the type conversions with shorter notations.
 * @author Toru Nagashima
 */

"use strict";

const astUtils = require("./utils/ast-utils");

//------------------------------------------------------------------------------
// Helpers
//------------------------------------------------------------------------------

const INDEX_OF_PATTERN = /^(?:i|lastI)ndexOf$/u;
const ALLOWABLE_OPERATORS = ["~", "!!", "+", "- -", "-", "*"];

/**
 * Parses and normalizes an option object.
 * @param {Object} options An option object to parse.
 * @returns {Object} The parsed and normalized option object.
 */
function parseOptions(options) {
    return {
        boolean: "boolean" in options ? options.boolean : true,
        number: "number" in options ? options.number : true,
        string: "string" in options ? options.string : true,
        disallowTemplateShorthand: "disallowTemplateShorthand" in options ? options.disallowTemplateShorthand : false,
        allow: options.allow || []
    };
}

/**
 * Checks whether or not a node is a double logical negating.
 * @param {ASTNode} node An UnaryExpression node to check.
 * @returns {boolean} Whether or not the node is a double logical negating.
 */
function isDoubleLogicalNegating(node) {
    return (
        node.operator === "!" &&
        node.argument.type === "UnaryExpression" &&
Repository: eslint/eslint
File: lib/rules/no-inner-declarations.js (JavaScript)

Code before:
                messageId: "moveDeclToRoot",
                data: {
                    type: (node.type === "FunctionDeclaration" ? "function" : "variable"),
                    body: getAllowedBodyDescription(node)
                }
            });
        }


        return {

            FunctionDeclaration(node) {
                const isStrictFunction = sourceCode.getScope(node).isStrict;

                if (isStrictFunction && legacy === false) {
                    return;
                }

                if (isStrictFunction && ecmaVersion !== 5) {
                    return;
                }

                check(node);
            },
            VariableDeclaration(node) {
                if (isStrict) {
                    return;
                }

                if (context.options[0] === "both" && node.kind === "var") {
                    check(node);
                }
            }

        };

    }
};


Review comment (suggestion):
We should check strict mode here again, because a variable can be inside of a function that is in strict mode even if the entire script is not.
```suggestion
                if (sourceCode.getScope(node).isStrict) {
```

Code after:
                    body: getAllowedBodyDescription(node)
                }
            });
        }


        return {

            FunctionDeclaration(node) {
                const isInStrictCode = sourceCode.getScope(node).upper.isStrict;

                if (blockScopedFunctions === "allow" && ecmaVersion >= 2015 && isInStrictCode) {
                    return;
                }

                check(node);
            },
            VariableDeclaration(node) {
                if (context.options[0] === "both" && node.kind === "var") {
                    check(node);
                }
            }

        };

    }
};
Repository: eslint/eslint
File: lib/rules/no-inner-declarations.js (JavaScript)

Code before:
                    type: (node.type === "FunctionDeclaration" ? "function" : "variable"),
                    body: getAllowedBodyDescription(node)
                }
            });
        }


        return {

            FunctionDeclaration(node) {
                const isStrictFunction = sourceCode.getScope(node).isStrict;

                if (isStrictFunction && legacy === false) {
                    return;
                }

                if (isStrictFunction && ecmaVersion !== 5) {
                    return;
                }

                check(node);
            },
            VariableDeclaration(node) {
                if (sourceCode.getScope(node).isStrict) {
                    return;
                }

                if (context.options[0] === "both" && node.kind === "var") {
                    check(node);
                }
            }

        };

    }
};


Review comment (refactor):
I don't think we should change anything regarding variable declarations, because they behave the same in all contexts.

Sorry for the lack of precise design details in https://github.com/eslint/eslint/issues/15576. Since we plan to remove this rule from the recommended configuration in v9.0.0-alpha.0 which will be released today, I prepared a separate PR just for that (https://github.com/eslint/eslint/pull/17920), and will focus on this issue next week.

Code after:
                    body: getAllowedBodyDescription(node)
                }
            });
        }


        return {

            FunctionDeclaration(node) {
                const isInStrictCode = sourceCode.getScope(node).upper.isStrict;

                if (blockScopedFunctions === "allow" && ecmaVersion >= 2015 && isInStrictCode) {
                    return;
                }

                check(node);
            },
            VariableDeclaration(node) {
                if (context.options[0] === "both" && node.kind === "var") {
                    check(node);
                }
            }

        };

    }
};
Repository: eslint/eslint
File: lib/rules/no-inner-declarations.js (JavaScript)

Code before:
                parent.type === "BlockStatement" && validBlockStatementParent.has(parent.parent.type)
            ) {
                return;
            }

            if (validParent.has(parent.type)) {
                return;
            }

            context.report({
                node,
                messageId: "moveDeclToRoot",
                data: {
                    type: (node.type === "FunctionDeclaration" ? "function" : "variable"),
                    body: getAllowedBodyDescription(node)
                }
            });
        }


        return {

            FunctionDeclaration(node) {
                const isStrictFunction = sourceCode.getScope(node).isStrict;

                if (isStrictFunction && blockScopedFunctions === "allow" && es6EcmaVersionsList.has(ecmaVersion)) {
                    return;
                }

                check(node);
            },
            VariableDeclaration(node) {
                if (context.options[0] === "both" && node.kind === "var") {
                    check(node);
                }
            }

        };

    }
};


Review comment (refactor):
```suggestion
                if (isStrictFunction && blockScopedFunctions === "allow" && ecmaVersion >= 2015) {
```

We can simplify this.

Code after:
            ) {
                return;
            }

            if (validParent.has(parent.type)) {
                return;
            }

            context.report({
                node,
                messageId: "moveDeclToRoot",
                data: {
                    type: (node.type === "FunctionDeclaration" ? "function" : "variable"),
                    body: getAllowedBodyDescription(node)
                }
            });
        }


        return {

            FunctionDeclaration(node) {
                const isInStrictCode = sourceCode.getScope(node).upper.isStrict;

                if (blockScopedFunctions === "allow" && ecmaVersion >= 2015 && isInStrictCode) {
                    return;
                }

                check(node);
            },
            VariableDeclaration(node) {
                if (context.options[0] === "both" && node.kind === "var") {
                    check(node);
                }
            }

        };

    }
};
Repository: eslint/eslint
File: lib/rules/no-inner-declarations.js (JavaScript)

Code before:

            if (
                parent.type === "BlockStatement" && validBlockStatementParent.has(parent.parent.type)
            ) {
                return;
            }

            if (validParent.has(parent.type)) {
                return;
            }

            context.report({
                node,
                messageId: "moveDeclToRoot",
                data: {
                    type: (node.type === "FunctionDeclaration" ? "function" : "variable"),
                    body: getAllowedBodyDescription(node)
                }
            });
        }


        return {

            FunctionDeclaration(node) {
                const isStrictFunction = sourceCode.getScope(node).isStrict;

                if (isStrictFunction && blockScopedFunctions === "allow" && es6EcmaVersionsList.has(ecmaVersion)) {
                    return;
                }

                check(node);
            },
            VariableDeclaration(node) {
                if (context.options[0] === "both" && node.kind === "var") {
                    check(node);
                }
            }

        };

    }
};


Review comment (performance):
`getScope(node)` is the function's scope, but we should check the upper scope, because even if the function is strict code, when the declaration isn't in strict code the function is still not block-scoped.

```js
{
    function foo() {
        "use strict";
        console.log("foo called");
    }
}

foo(); // logs "foo called"

```

Also, it would be nice to check the scope last, for performance reasons. For example, the check can look like this:

```js
if (
    blockScopedFunctions === "allow" &&
    ecmaVersion >= 2015 &&
    sourceCode.getScope(node).upper.isStrict
) {
    return;
}
```

Code after:
            if (
                parent.type === "BlockStatement" && validBlockStatementParent.has(parent.parent.type)
            ) {
                return;
            }

            if (validParent.has(parent.type)) {
                return;
            }

            context.report({
                node,
                messageId: "moveDeclToRoot",
                data: {
                    type: (node.type === "FunctionDeclaration" ? "function" : "variable"),
                    body: getAllowedBodyDescription(node)
                }
            });
        }


        return {

            FunctionDeclaration(node) {
                const isInStrictCode = sourceCode.getScope(node).upper.isStrict;

                if (blockScopedFunctions === "allow" && ecmaVersion >= 2015 && isInStrictCode) {
                    return;
                }

                check(node);
            },
            VariableDeclaration(node) {
                if (context.options[0] === "both" && node.kind === "var") {
                    check(node);
                }
            }

        };

    }
};