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: validate options when comment with just severity enables rule #18133

Merged
merged 1 commit into from
Feb 23, 2024
Merged

fix: validate options when comment with just severity enables rule #18133

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
215 changes: 105 additions & 110 deletions lib/linter/linter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ const
const { getRuleFromConfig } = require("../config/flat-config-helpers");
const { FlatConfigArray } = require("../config/flat-config-array");
const { RuleValidator } = require("../config/rule-validator");
const { assertIsRuleOptions, assertIsRuleSeverity } = require("../config/flat-config-schema");
const { assertIsRuleSeverity } = require("../config/flat-config-schema");
const { normalizeSeverityToString } = require("../shared/severity");
const debug = require("debug")("eslint:linter");
const MAX_AUTOFIX_PASSES = 10;
Expand Down Expand Up @@ -326,10 +326,11 @@ function createDisableDirectives(options) {
* @param {SourceCode} sourceCode The SourceCode object to get comments from.
* @param {function(string): {create: Function}} ruleMapper A map from rule IDs to defined rules
* @param {string|null} warnInlineConfig If a string then it should warn directive comments as disabled. The string value is the config name what the setting came from.
* @param {ConfigData} config Provided config.
* @returns {{configuredRules: Object, enabledGlobals: {value:string,comment:Token}[], exportedVariables: Object, problems: LintMessage[], disableDirectives: DisableDirective[]}}
* A collection of the directive comments that were found, along with any problems that occurred when parsing
*/
function getDirectiveComments(sourceCode, ruleMapper, warnInlineConfig) {
function getDirectiveComments(sourceCode, ruleMapper, warnInlineConfig, config) {
const configuredRules = {};
const enabledGlobals = Object.create(null);
const exportedVariables = {};
Expand Down Expand Up @@ -438,8 +439,50 @@ function getDirectiveComments(sourceCode, ruleMapper, warnInlineConfig) {
return;
}

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

/*
* If the rule was already configured, 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"]
*/
if (

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

/*
* And the rule was already configured
*/
config.rules && Object.hasOwn(config.rules, name)
) {

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

try {
validator.validateRuleOptions(rule, name, ruleValue);
validator.validateRuleOptions(rule, name, ruleOptions);
} catch (err) {

/*
Expand All @@ -460,7 +503,7 @@ function getDirectiveComments(sourceCode, ruleMapper, warnInlineConfig) {
return;
}

configuredRules[name] = ruleValue;
configuredRules[name] = ruleOptions;
});
} else {
problems.push(parseResult.error);
Expand Down Expand Up @@ -1322,7 +1365,7 @@ class Linter {

const sourceCode = slots.lastSourceCode;
const commentDirectives = options.allowInlineConfig
? getDirectiveComments(sourceCode, ruleId => getRule(slots, ruleId), options.warnInlineConfig)
? getDirectiveComments(sourceCode, ruleId => getRule(slots, ruleId), options.warnInlineConfig, config)
: { configuredRules: {}, enabledGlobals: {}, exportedVariables: {}, problems: [], disableDirectives: [] };

// augment global scope with declared global variables
Expand All @@ -1332,56 +1375,8 @@ 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 @@ -1713,17 +1708,67 @@ class Linter {

try {

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

assertIsRuleOptions(ruleId, ruleValue);
Comment on lines -1716 to -1718
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: I removed this because at this point ruleValue is always an array, so this wasn't actually checking anything.

eslint/lib/config/flat-config-schema.js

Lines 173 to 177 in 8e13a6b

function assertIsRuleOptions(ruleId, value) {
if (typeof value !== "string" && typeof value !== "number" && !Array.isArray(value)) {
throw new InvalidRuleOptionsError(ruleId, value);
}
}

assertIsRuleSeverity(ruleId, ruleOptions[0]);

ruleValidator.validate({
plugins: config.plugins,
rules: {
[ruleId]: ruleOptions
/*
* If the rule was already configured, 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"]
*/

let shouldValidateOptions = true;

if (

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

/*
* And the rule was already configured
*/
config.rules && Object.hasOwn(config.rules, ruleId)
) {

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

// if the rule was enabled, the options have already been validated
if (config.rules[ruleId][0] > 0) {
shouldValidateOptions = false;
}
});
}

if (shouldValidateOptions) {
ruleValidator.validate({
plugins: config.plugins,
rules: {
[ruleId]: ruleOptions
}
});
}

mergedInlineConfig.rules[ruleId] = ruleOptions;
} catch (err) {

Expand Down Expand Up @@ -1763,58 +1808,8 @@ 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