Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat!: disallow multiple configuration comments for same rule #18157

Merged
merged 3 commits into from Mar 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
25 changes: 25 additions & 0 deletions docs/src/use/migrate-to-9.0.0.md
Expand Up @@ -25,6 +25,7 @@ The lists below are ordered roughly by the number of users each change is expect
* [`--output-file` now writes a file to disk even with an empty output](#output-file)
* [Change in behavior when no patterns are passed to CLI](#cli-empty-patterns)
* [`/* eslint */` comments with only severity now retain options from the config file](#eslint-comment-options)
* [Multiple `/* eslint */` comments for the same rule are now disallowed](#multiple-eslint-comments)
* [Stricter `/* exported */` parsing](#exported-parsing)
* [`no-constructor-return` and `no-sequences` rule schemas are stricter](#stricter-rule-schemas)
* [New checks in `no-implicit-coercion` by default](#no-implicit-coercion)
Expand Down Expand Up @@ -190,6 +191,30 @@ Note that this change only affects cases where the same rule is configured in th

**Related issue(s):** [#17381](https://github.com/eslint/eslint/issues/17381)

## <a name="multiple-eslint-comments"></a> Multiple `/* eslint */` comments for the same rule are now disallowed

Prior to ESLint v9.0.0, if the file being linted contained multiple `/* eslint */` configuration comments for the same rule, the last one would be applied, while the others would be silently ignored. For example:

```js
/* eslint semi: ["error", "always"] */
/* eslint semi: ["error", "never"] */

foo() // valid, because the configuration is "never"
```

In ESLint v9.0.0, the first one is applied, while the others are reported as lint errors:

```js
/* eslint semi: ["error", "always"] */
/* eslint semi: ["error", "never"] */ // error: Rule "semi" is already configured by another configuration comment in the preceding code. This configuration is ignored.

foo() // error: Missing semicolon
```

**To address:** Remove duplicate `/* eslint */` comments.

**Related issue(s):** [#18132](https://github.com/eslint/eslint/issues/18132)

## <a name="exported-parsing"></a> Stricter `/* exported */` parsing

Prior to ESLint v9.0.0, the `/* exported */` directive incorrectly allowed the following syntax:
Expand Down
16 changes: 16 additions & 0 deletions lib/linter/linter.js
Expand Up @@ -439,6 +439,14 @@ function getDirectiveComments(sourceCode, ruleMapper, warnInlineConfig, config)
return;
}

if (Object.hasOwn(configuredRules, name)) {
problems.push(createLintingProblem({
message: `Rule "${name}" is already configured by another configuration comment in the preceding code. This configuration is ignored.`,
loc: comment.loc
}));
return;
}

let ruleOptions = Array.isArray(ruleValue) ? ruleValue : [ruleValue];

/*
Expand Down Expand Up @@ -1706,6 +1714,14 @@ class Linter {
return;
}

if (Object.hasOwn(mergedInlineConfig.rules, ruleId)) {
inlineConfigProblems.push(createLintingProblem({
message: `Rule "${ruleId}" is already configured by another configuration comment in the preceding code. This configuration is ignored.`,
loc: node.loc
}));
return;
}

try {

let ruleOptions = Array.isArray(ruleValue) ? ruleValue : [ruleValue];
Expand Down
310 changes: 310 additions & 0 deletions tests/lib/linter/linter.js
Expand Up @@ -1877,6 +1877,156 @@ describe("Linter", () => {
});
});

describe("when evaluating code with multiple configuration comments for same rule", () => {

beforeEach(() => {
linter.defineRule("no-foo", {
meta: {
schema: [{
enum: ["bar", "baz", "qux"]
}]
},
create(context) {
const replacement = context.options[0] ?? "default";

return {
"Identifier[name='foo']"(node) {
context.report(node, `Replace 'foo' with '${replacement}'.`);
}
};
}
});
});

it("should apply the first and report an error for the second when there are two", () => {
const code = "/*eslint no-foo: ['error', 'bar']*/ /*eslint no-foo: ['error', 'baz']*/ foo;";

const messages = linter.verify(code);
const suppressedMessages = linter.getSuppressedMessages();

assert.deepStrictEqual(messages, [
{
ruleId: null,
severity: 2,
message: "Rule \"no-foo\" is already configured by another configuration comment in the preceding code. This configuration is ignored.",
line: 1,
column: 37,
endLine: 1,
endColumn: 72,
nodeType: null
},
{
ruleId: "no-foo",
severity: 2,
message: "Replace 'foo' with 'bar'.",
line: 1,
column: 73,
endLine: 1,
endColumn: 76,
nodeType: "Identifier"
}
]);
assert.strictEqual(suppressedMessages.length, 0);
});

it("should apply the first and report an error for each other when there are more than two", () => {
const code = "/*eslint no-foo: ['error', 'bar']*/ /*eslint no-foo: ['error', 'baz']*/ /*eslint no-foo: ['error', 'qux']*/ foo;";

const messages = linter.verify(code);
const suppressedMessages = linter.getSuppressedMessages();

assert.deepStrictEqual(messages, [
{
ruleId: null,
severity: 2,
message: "Rule \"no-foo\" is already configured by another configuration comment in the preceding code. This configuration is ignored.",
line: 1,
column: 37,
endLine: 1,
endColumn: 72,
nodeType: null
},
{
ruleId: null,
severity: 2,
message: "Rule \"no-foo\" is already configured by another configuration comment in the preceding code. This configuration is ignored.",
line: 1,
column: 73,
endLine: 1,
endColumn: 108,
nodeType: null
},
{
ruleId: "no-foo",
severity: 2,
message: "Replace 'foo' with 'bar'.",
line: 1,
column: 109,
endLine: 1,
endColumn: 112,
nodeType: "Identifier"
}
]);
assert.strictEqual(suppressedMessages.length, 0);
});

it("should apply the first and report an error for the second when both just override severity", () => {
const code = "/*eslint no-foo: 'warn'*/ /*eslint no-foo: 'error'*/ foo;";

const messages = linter.verify(code, { rules: { "no-foo": ["error", "bar"] } });
const suppressedMessages = linter.getSuppressedMessages();

assert.deepStrictEqual(messages, [
{
ruleId: null,
severity: 2,
message: "Rule \"no-foo\" is already configured by another configuration comment in the preceding code. This configuration is ignored.",
line: 1,
column: 27,
endLine: 1,
endColumn: 53,
nodeType: null
},
{
ruleId: "no-foo",
severity: 1,
message: "Replace 'foo' with 'bar'.",
line: 1,
column: 54,
endLine: 1,
endColumn: 57,
nodeType: "Identifier"
}
]);
assert.strictEqual(suppressedMessages.length, 0);
});

it("should apply the second if the first has an invalid configuration", () => {
const code = "/*eslint no-foo: ['error', 'quux']*/ /*eslint no-foo: ['error', 'bar']*/ foo;";

const messages = linter.verify(code);
const suppressedMessages = linter.getSuppressedMessages();

assert.strictEqual(messages.length, 2);
assert.include(messages[0].message, "Configuration for rule \"no-foo\" is invalid");
assert.strictEqual(messages[1].message, "Replace 'foo' with 'bar'.");
assert.strictEqual(suppressedMessages.length, 0);
});

it("should apply configurations for other rules that are in the same comment as the duplicate", () => {
const code = "/*eslint no-foo: ['error', 'bar']*/ /*eslint no-foo: ['error', 'baz'], no-alert: ['error']*/ foo; alert();";

const messages = linter.verify(code);
const suppressedMessages = linter.getSuppressedMessages();

assert.strictEqual(messages.length, 3);
assert.strictEqual(messages[0].message, "Rule \"no-foo\" is already configured by another configuration comment in the preceding code. This configuration is ignored.");
assert.strictEqual(messages[1].message, "Replace 'foo' with 'bar'.");
assert.strictEqual(messages[2].ruleId, "no-alert");
assert.strictEqual(suppressedMessages.length, 0);
});
});

describe("when evaluating code with comments to enable and disable all reporting", () => {
it("should report a violation", () => {

Expand Down Expand Up @@ -11225,6 +11375,166 @@ describe("Linter with FlatConfigArray", () => {
});
});

describe("when evaluating code with multiple configuration comments for same rule", () => {

let baseConfig;

beforeEach(() => {
baseConfig = {
plugins: {
"test-plugin": {
rules: {
"no-foo": {
meta: {
schema: [{
enum: ["bar", "baz", "qux"]
}]
},
create(context) {
const replacement = context.options[0] ?? "default";

return {
"Identifier[name='foo']"(node) {
context.report(node, `Replace 'foo' with '${replacement}'.`);
}
};
}
}
}
}
}
};
});

it("should apply the first and report an error for the second when there are two", () => {
const code = "/*eslint test-plugin/no-foo: ['error', 'bar']*/ /*eslint test-plugin/no-foo: ['error', 'baz']*/ foo;";

const messages = linter.verify(code, baseConfig);
const suppressedMessages = linter.getSuppressedMessages();

assert.deepStrictEqual(messages, [
{
ruleId: null,
severity: 2,
message: "Rule \"test-plugin/no-foo\" is already configured by another configuration comment in the preceding code. This configuration is ignored.",
line: 1,
column: 49,
endLine: 1,
endColumn: 96,
nodeType: null
},
{
ruleId: "test-plugin/no-foo",
severity: 2,
message: "Replace 'foo' with 'bar'.",
line: 1,
column: 97,
endLine: 1,
endColumn: 100,
nodeType: "Identifier"
}
]);
assert.strictEqual(suppressedMessages.length, 0);
});

it("should apply the first and report an error for each other when there are more than two", () => {
const code = "/*eslint test-plugin/no-foo: ['error', 'bar']*/ /*eslint test-plugin/no-foo: ['error', 'baz']*/ /*eslint test-plugin/no-foo: ['error', 'qux']*/ foo;";

const messages = linter.verify(code, baseConfig);
const suppressedMessages = linter.getSuppressedMessages();

assert.deepStrictEqual(messages, [
{
ruleId: null,
severity: 2,
message: "Rule \"test-plugin/no-foo\" is already configured by another configuration comment in the preceding code. This configuration is ignored.",
line: 1,
column: 49,
endLine: 1,
endColumn: 96,
nodeType: null
},
{
ruleId: null,
severity: 2,
message: "Rule \"test-plugin/no-foo\" is already configured by another configuration comment in the preceding code. This configuration is ignored.",
line: 1,
column: 97,
endLine: 1,
endColumn: 144,
nodeType: null
},
{
ruleId: "test-plugin/no-foo",
severity: 2,
message: "Replace 'foo' with 'bar'.",
line: 1,
column: 145,
endLine: 1,
endColumn: 148,
nodeType: "Identifier"
}
]);
assert.strictEqual(suppressedMessages.length, 0);
});

it("should apply the first and report an error for the second when both just override severity", () => {
const code = "/*eslint test-plugin/no-foo: 'warn'*/ /*eslint test-plugin/no-foo: 'error'*/ foo;";

const messages = linter.verify(code, { ...baseConfig, rules: { "test-plugin/no-foo": ["error", "bar"] } });
const suppressedMessages = linter.getSuppressedMessages();

assert.deepStrictEqual(messages, [
{
ruleId: null,
severity: 2,
message: "Rule \"test-plugin/no-foo\" is already configured by another configuration comment in the preceding code. This configuration is ignored.",
line: 1,
column: 39,
endLine: 1,
endColumn: 77,
nodeType: null
},
{
ruleId: "test-plugin/no-foo",
severity: 1,
message: "Replace 'foo' with 'bar'.",
line: 1,
column: 78,
endLine: 1,
endColumn: 81,
nodeType: "Identifier"
}
]);
assert.strictEqual(suppressedMessages.length, 0);
});

it("should apply the second if the first has an invalid configuration", () => {
const code = "/*eslint test-plugin/no-foo: ['error', 'quux']*/ /*eslint test-plugin/no-foo: ['error', 'bar']*/ foo;";

const messages = linter.verify(code, baseConfig);
const suppressedMessages = linter.getSuppressedMessages();

assert.strictEqual(messages.length, 2);
assert.include(messages[0].message, "Inline configuration for rule \"test-plugin/no-foo\" is invalid");
assert.strictEqual(messages[1].message, "Replace 'foo' with 'bar'.");
assert.strictEqual(suppressedMessages.length, 0);
});

it("should apply configurations for other rules that are in the same comment as the duplicate", () => {
const code = "/*eslint test-plugin/no-foo: ['error', 'bar']*/ /*eslint test-plugin/no-foo: ['error', 'baz'], no-alert: ['error']*/ foo; alert();";

const messages = linter.verify(code, baseConfig);
const suppressedMessages = linter.getSuppressedMessages();

assert.strictEqual(messages.length, 3);
assert.strictEqual(messages[0].message, "Rule \"test-plugin/no-foo\" is already configured by another configuration comment in the preceding code. This configuration is ignored.");
assert.strictEqual(messages[1].message, "Replace 'foo' with 'bar'.");
assert.strictEqual(messages[2].ruleId, "no-alert");
assert.strictEqual(suppressedMessages.length, 0);
});
});

describe("when evaluating code with comments to enable and disable all reporting", () => {
it("should report a violation", () => {

Expand Down