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.

Sign up for GitHub

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

Jump to bottom

fix!: configuration comments with just severity should retain options #17945

Merged
merged 2 commits into from Jan 3, 2024
Merged

fix!: configuration comments with just severity should retain options #17945

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3fa70a8
fix!: configuration comments with just severity should retain options
mdjermanovic Jan 2, 2024
00d6b6f
update wording in migration guide
mdjermanovic Jan 2, 2024
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
41 changes: 41 additions & 0 deletions docs/src/use/migrate-to-9.0.0.md
View file
Open in desktop
Expand Up @@ -23,6 +23,7 @@ The lists below are ordered roughly by the number of users each change is expect
* [`eslint:recommended` has been updated](#eslint-recommended)
* [`--quiet` no longer runs rules set to `"warn"`](#quiet-warn)
* [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)
* [`no-constructor-return` and `no-sequences` rule schemas are stricter](#stricter-rule-schemas)
* [New checks in `no-implicit-coercion` by default](#no-implicit-coercion)
* [Case-sensitive flags in `no-invalid-regexp`](#no-invalid-regexp)
Expand Down Expand Up @@ -135,6 +136,46 @@ Prior to ESLint v9.0.0, running the ESLint CLI without any file or directory pat

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

## <a name="eslint-comment-options"></a> `/* eslint */` comments with only severity now retain options from the config file

Prior to ESLint v9.0.0, configuration comments such as `/* eslint curly: "warn" */` or `/* eslint curly: ["warn"] */` would completely override any configuration specified for the rule in the config file, and thus enforce the default options of the rule.

In ESLint v9.0.0, the behavior of configuration comments is aligned with how rule configurations in config files are merged, meaning that a configuration comment with only severity now retains options specified in the config file and just overrides the severity.

For example, if you have the following config file:

```js
// eslint.config.js

export default [{
rules: {
curly: ["error", "multi"]
}
}];
```

and the following configuration comment:

```js
// my-file.js

/* eslint curly: "warn" */
```

the resulting configuration for the `curly` rule when linting `my-file.js` will be `curly: ["warn", "multi"]`.

Note that this change only affects cases where the same rule is configured in the config file with options and using a configuration comment without options. In all other cases (e.g. the rule is only configured using a configuration comment), the behavior remains the same as prior to ESLint v9.0.0.

**To address:** We expect that in most cases no change is necessary, as rules configured using configuration comments are typically not already configured in the config file. However, if you need a configuration comment to completely override configuration from the config file and enforce the default options, you'll need to specify at least one option:

```js
// my-file.js

/* eslint curly: ["warn", "all"] */
```

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

## <a name="stricter-rule-schemas"></a> `no-constructor-return` and `no-sequences` rule schemas are stricter

In previous versions of ESLint, `no-constructor-return` and `no-sequences` rules were mistakenly accepting invalid options.
Expand Down
102 changes: 101 additions & 1 deletion lib/linter/linter.js
View file
Open in desktop
Expand Up @@ -1331,7 +1331,56 @@ class Linter {
{ exportedVariables: commentDirectives.exportedVariables, enabledGlobals: commentDirectives.enabledGlobals }
);

/*
* Now we determine the final configurations for rules.
* First, let all inline rule configurations override those from the config.
* Then, check for a special case: if a rule is configured in both places,
* inline rule configuration that only has severity should retain options from
* the config and just override the severity.
*
* Example:
*
* {
* rules: {
* curly: ["error", "multi"]
* }
* }
*
* /* eslint curly: ["warn"] * /
*
* Results in:
*
* curly: ["warn", "multi"]
*/
const configuredRules = Object.assign({}, config.rules, commentDirectives.configuredRules);

if (config.rules) {
for (const [ruleId, ruleInlineConfig] of Object.entries(commentDirectives.configuredRules)) {
if (

/*
* If inline config for the rule has only severity
*/
(!Array.isArray(ruleInlineConfig) || ruleInlineConfig.length === 1) &&

/*
* And provided config for the rule has options
*/
Object.hasOwn(config.rules, ruleId) &&
(Array.isArray(config.rules[ruleId]) && config.rules[ruleId].length > 1)
) {

/*
* Then use severity from the inline config and options from the provided config
*/
configuredRules[ruleId] = [
Array.isArray(ruleInlineConfig) ? ruleInlineConfig[0] : ruleInlineConfig, // severity from the inline config
...config.rules[ruleId].slice(1) // options from the provided config
];
}
}
}

let lintingProblems;

try {
Expand Down Expand Up @@ -1674,7 +1723,7 @@ class Linter {
[ruleId]: ruleOptions
}
});
mergedInlineConfig.rules[ruleId] = ruleValue;
mergedInlineConfig.rules[ruleId] = ruleOptions;
} catch (err) {

/*
Expand Down Expand Up @@ -1713,7 +1762,58 @@ class Linter {
)
: { problems: [], disableDirectives: [] };

/*
* Now we determine the final configurations for rules.
* First, let all inline rule configurations override those from the config.
* Then, check for a special case: if a rule is configured in both places,
* inline rule configuration that only has severity should retain options from
* the config and just override the severity.
*
* Example:
*
* {
* rules: {
* curly: ["error", "multi"]
* }
* }
*
* /* eslint curly: ["warn"] * /
*
* Results in:
*
* curly: ["warn", "multi"]
*
* At this point, all rule configurations are normalized to arrays.
*/
const configuredRules = Object.assign({}, config.rules, mergedInlineConfig.rules);

if (config.rules) {
for (const [ruleId, ruleInlineConfig] of Object.entries(mergedInlineConfig.rules)) {
if (

/*
* If inline config for the rule has only severity
*/
ruleInlineConfig.length === 1 &&

/*
* And provided config for the rule has options
*/
Object.hasOwn(config.rules, ruleId) &&
config.rules[ruleId].length > 1
) {

/*
* Then use severity from the inline config and options from the provided config
*/
configuredRules[ruleId] = [
ruleInlineConfig[0], // severity from the inline config
...config.rules[ruleId].slice(1) // options from the provided config
];
}
}
}

let lintingProblems;

sourceCode.finalize();
Expand Down
172 changes: 172 additions & 0 deletions tests/lib/linter/linter.js
View file
Open in desktop
Expand Up @@ -1455,6 +1455,90 @@ describe("Linter", () => {
assert.strictEqual(messages.length, 1);
assert.strictEqual(suppressedMessages.length, 0);
});

describe("when the rule was already configured", () => {

beforeEach(() => {
linter.defineRule("my-rule", {
meta: {
schema: [{
type: "string"
}]
},
create(context) {
const message = context.options[0] ?? "option not provided";

return {
Program(node) {
context.report({ node, message });
}
};
}
});
});

[
"off",
"warn",
"error",
["off"],
["warn"],
["error"],
["off", "bar"],
["warn", "bar"],
["error", "bar"]
].forEach(ruleConfig => {
const config = {
rules: {
"my-rule": ruleConfig
}
};

it(`severity from the /*eslint*/ comment and options from the config should apply when the comment has only severity (original config: ${JSON.stringify(ruleConfig)})`, () => {
const code = "/*eslint my-rule: 'warn' */";
const messages = linter.verify(code, config);
const suppressedMessages = linter.getSuppressedMessages();

const expectedMessage = Array.isArray(ruleConfig) && ruleConfig.length > 1
? ruleConfig[1]
: "option not provided";

assert.strictEqual(messages.length, 1);
assert.strictEqual(messages[0].ruleId, "my-rule");
assert.strictEqual(messages[0].severity, 1);
assert.strictEqual(messages[0].message, expectedMessage);
assert.strictEqual(suppressedMessages.length, 0);
});

it(`severity from the /*eslint*/ comment and options from the config should apply when the comment has array with only severity (original config: ${JSON.stringify(ruleConfig)})`, () => {
const code = "/*eslint my-rule: ['warn'] */";
const messages = linter.verify(code, config);
const suppressedMessages = linter.getSuppressedMessages();

const expectedMessage = Array.isArray(ruleConfig) && ruleConfig.length > 1
? ruleConfig[1]
: "option not provided";

assert.strictEqual(messages.length, 1);
assert.strictEqual(messages[0].ruleId, "my-rule");
assert.strictEqual(messages[0].severity, 1);
assert.strictEqual(messages[0].message, expectedMessage);
assert.strictEqual(suppressedMessages.length, 0);
});

it(`severity and options from the /*eslint*/ comment should apply when the comment includes options (original config: ${JSON.stringify(ruleConfig)})`, () => {
const code = "/*eslint my-rule: ['warn', 'foo'] */";
const messages = linter.verify(code, config);
const suppressedMessages = linter.getSuppressedMessages();

assert.strictEqual(messages.length, 1);
assert.strictEqual(messages[0].ruleId, "my-rule");
assert.strictEqual(messages[0].severity, 1);
assert.strictEqual(messages[0].message, "foo");
assert.strictEqual(suppressedMessages.length, 0);
});
});
});
});

describe("when evaluating code with invalid comments to enable rules", () => {
Expand Down Expand Up @@ -10625,6 +10709,94 @@ describe("Linter with FlatConfigArray", () => {
assert.strictEqual(messages.length, 1);
assert.strictEqual(suppressedMessages.length, 0);
});

describe("when the rule was already configured", () => {
const plugin = {
rules: {
"my-rule": {
meta: {
schema: [{
type: "string"
}]
},
create(context) {
const message = context.options[0] ?? "option not provided";

return {
Program(node) {
context.report({ node, message });
}
};
}
}
}
};

[
"off",
"warn",
"error",
["off"],
["warn"],
["error"],
["off", "bar"],
["warn", "bar"],
["error", "bar"]
].forEach(ruleConfig => {
const config = {
plugins: {
test: plugin
},
rules: {
"test/my-rule": ruleConfig
}
};

it(`severity from the /*eslint*/ comment and options from the config should apply when the comment has only severity (original config: ${JSON.stringify(ruleConfig)})`, () => {
const code = "/*eslint test/my-rule: 'warn' */";
const messages = linter.verify(code, config);
const suppressedMessages = linter.getSuppressedMessages();

const expectedMessage = Array.isArray(ruleConfig) && ruleConfig.length > 1
? ruleConfig[1]
: "option not provided";

assert.strictEqual(messages.length, 1);
assert.strictEqual(messages[0].ruleId, "test/my-rule");
assert.strictEqual(messages[0].severity, 1);
assert.strictEqual(messages[0].message, expectedMessage);
assert.strictEqual(suppressedMessages.length, 0);
});

it(`severity from the /*eslint*/ comment and options from the config should apply when the comment has array with only severity (original config: ${JSON.stringify(ruleConfig)})`, () => {
const code = "/*eslint test/my-rule: ['warn'] */";
const messages = linter.verify(code, config);
const suppressedMessages = linter.getSuppressedMessages();

const expectedMessage = Array.isArray(ruleConfig) && ruleConfig.length > 1
? ruleConfig[1]
: "option not provided";

assert.strictEqual(messages.length, 1);
assert.strictEqual(messages[0].ruleId, "test/my-rule");
assert.strictEqual(messages[0].severity, 1);
assert.strictEqual(messages[0].message, expectedMessage);
assert.strictEqual(suppressedMessages.length, 0);
});

it(`severity and options from the /*eslint*/ comment should apply when the comment includes options (original config: ${JSON.stringify(ruleConfig)})`, () => {
const code = "/*eslint test/my-rule: ['warn', 'foo'] */";
const messages = linter.verify(code, config);
const suppressedMessages = linter.getSuppressedMessages();

assert.strictEqual(messages.length, 1);
assert.strictEqual(messages[0].ruleId, "test/my-rule");
assert.strictEqual(messages[0].severity, 1);
assert.strictEqual(messages[0].message, "foo");
assert.strictEqual(suppressedMessages.length, 0);
});
});
});
});

describe("when evaluating code with invalid comments to enable rules", () => {
Expand Down