From 382961fb6eb67557910a1ff7430a7a180f888bcc Mon Sep 17 00:00:00 2001 From: Masafumi Koba <473530+ybiquitous@users.noreply.github.com> Date: Mon, 22 Jan 2024 23:49:40 +0900 Subject: [PATCH] Fix report flags not reporting on subsequent runs when cache is used (#7483) The bug's reason is that the `stylelintError`/`stylelintWarning` properties in a lint result object are not set when `styelint-disable` comment problems are reported. Also, this change refactors the code around `styelint-disable` comments to make it more readable and prevent possible bugs. The new code uses [`Result#warn()`](https://postcss.org/api/#result-warn) in PostCSS for a comment node, like reporting a normal rule problem. --- .changeset/afraid-cobras-heal.md | 5 ++ lib/__tests__/standalone-cache.test.mjs | 22 +++++ lib/descriptionlessDisables.cjs | 74 ++++++++--------- lib/descriptionlessDisables.mjs | 90 +++++++++----------- lib/invalidScopeDisables.cjs | 50 +++++------ lib/invalidScopeDisables.mjs | 50 +++++------ lib/lintSource.cjs | 9 ++ lib/lintSource.mjs | 9 ++ lib/needlessDisables.cjs | 105 +++++++++++------------- lib/needlessDisables.mjs | 105 +++++++++++------------- lib/prepareReturnValue.cjs | 10 --- lib/prepareReturnValue.mjs | 10 --- lib/reportDisables.cjs | 63 ++++++-------- lib/reportDisables.mjs | 63 ++++++-------- lib/utils/reportCommentProblem.cjs | 49 +++++++++++ lib/utils/reportCommentProblem.mjs | 43 ++++++++++ lib/validateDisableSettings.cjs | 27 ++---- lib/validateDisableSettings.mjs | 27 ++---- 18 files changed, 411 insertions(+), 400 deletions(-) create mode 100644 .changeset/afraid-cobras-heal.md create mode 100644 lib/utils/reportCommentProblem.cjs create mode 100644 lib/utils/reportCommentProblem.mjs diff --git a/.changeset/afraid-cobras-heal.md b/.changeset/afraid-cobras-heal.md new file mode 100644 index 0000000000..81dce3c35c --- /dev/null +++ b/.changeset/afraid-cobras-heal.md @@ -0,0 +1,5 @@ +--- +"stylelint": patch +--- + +Fixed: report flags not reporting on subsequent runs when cache is used diff --git a/lib/__tests__/standalone-cache.test.mjs b/lib/__tests__/standalone-cache.test.mjs index 25c8d98f01..540f28b805 100644 --- a/lib/__tests__/standalone-cache.test.mjs +++ b/lib/__tests__/standalone-cache.test.mjs @@ -158,6 +158,28 @@ describe('standalone cache', () => { expect(cache.getKey(newFileDest)).toBeUndefined(); }); + it('files with a special comment problem are not cached', async () => { + const inputFile = path.join(fixturesPath, 'empty-block-with-disables.css'); + + await copyFile(inputFile, newFileDest); + + const { errored, results } = await standalone( + getConfig({ + config: { + rules: {}, + }, + reportNeedlessDisables: true, + }), + ); + + expect(errored).toBe(true); + expect(results).toHaveProperty('[0].warnings[0].rule', '--report-needless-disables'); + + const { cache } = fCache.createFromFile(expectedCacheFilePath); + + expect(cache.getKey(newFileDest)).toBeUndefined(); + }); + it('cache file is removed when cache is disabled', async () => { await standalone(getConfig({ cache: false })); expect(existsSync(expectedCacheFilePath)).toBe(false); diff --git a/lib/descriptionlessDisables.cjs b/lib/descriptionlessDisables.cjs index d5375980e7..3bc399c380 100644 --- a/lib/descriptionlessDisables.cjs +++ b/lib/descriptionlessDisables.cjs @@ -3,61 +3,51 @@ 'use strict'; const optionsMatches = require('./utils/optionsMatches.cjs'); +const reportCommentProblem = require('./utils/reportCommentProblem.cjs'); const validateDisableSettings = require('./validateDisableSettings.cjs'); -/** @typedef {import('postcss').Comment} PostcssComment */ -/** @typedef {import('stylelint').DisableReportRange} DisableReportRange */ -/** @typedef {import('stylelint').DisableOptionsReport} StylelintDisableOptionsReport */ - /** - * @param {import('stylelint').LintResult[]} results + * @param {import('stylelint').PostcssResult} postcssResult + * @returns {void} */ -function descriptionlessDisables(results) { - for (const result of results) { - const settings = validateDisableSettings( - result._postcssResult, - 'reportDescriptionlessDisables', - ); - - if (!settings) continue; +function descriptionlessDisables(postcssResult) { + const [enabled, options] = validateDisableSettings( + postcssResult, + 'reportDescriptionlessDisables', + ); - const [enabled, options, stylelintResult] = settings; + if (!options) return; - /** @type {Set} */ - const alreadyReported = new Set(); + /** @type {Set} */ + const alreadyReported = new Set(); - for (const [rule, ruleRanges] of Object.entries(stylelintResult.disabledRanges)) { - for (const range of ruleRanges) { - if (range.description) continue; + for (const [rule, ruleRanges] of Object.entries(postcssResult.stylelint.disabledRanges)) { + for (const range of ruleRanges) { + if (range.description) continue; - if (alreadyReported.has(range.comment)) continue; + const commentNode = range.comment; - if (enabled === optionsMatches(options, 'except', rule)) { - // An 'all' rule will get copied for each individual rule. If the - // configuration is `[false, {except: ['specific-rule']}]`, we - // don't want to report the copies that match except, so we record - // the comment as already reported. - if (!enabled && rule === 'all') alreadyReported.add(range.comment); + if (alreadyReported.has(commentNode)) continue; - continue; - } + if (enabled === optionsMatches(options, 'except', rule)) { + // An 'all' rule will get copied for each individual rule. If the + // configuration is `[false, {except: ['specific-rule']}]`, we + // don't want to report the copies that match except, so we record + // the comment as already reported. + if (!enabled && rule === 'all') alreadyReported.add(commentNode); - alreadyReported.add(range.comment); + continue; + } - // If the comment doesn't have a location, we can't report a useful error. - // In practice we expect all comments to have locations, though. - if (!range.comment.source || !range.comment.source.start) continue; + alreadyReported.add(commentNode); - result.warnings.push({ - text: `Disable for "${rule}" is missing a description`, - rule: '--report-descriptionless-disables', - line: range.comment.source.start.line, - column: range.comment.source.start.column, - endLine: range.comment.source.end && range.comment.source.end.line, - endColumn: range.comment.source.end && range.comment.source.end.column, - severity: options.severity, - }); - } + reportCommentProblem({ + rule: '--report-descriptionless-disables', + message: `Disable for "${rule}" is missing a description`, + severity: options.severity, + commentNode, + postcssResult, + }); } } } diff --git a/lib/descriptionlessDisables.mjs b/lib/descriptionlessDisables.mjs index c76864a1b2..18548f1217 100644 --- a/lib/descriptionlessDisables.mjs +++ b/lib/descriptionlessDisables.mjs @@ -1,59 +1,49 @@ import optionsMatches from './utils/optionsMatches.mjs'; +import reportCommentProblem from './utils/reportCommentProblem.mjs'; import validateDisableSettings from './validateDisableSettings.mjs'; -/** @typedef {import('postcss').Comment} PostcssComment */ -/** @typedef {import('stylelint').DisableReportRange} DisableReportRange */ -/** @typedef {import('stylelint').DisableOptionsReport} StylelintDisableOptionsReport */ - /** - * @param {import('stylelint').LintResult[]} results + * @param {import('stylelint').PostcssResult} postcssResult + * @returns {void} */ -export default function descriptionlessDisables(results) { - for (const result of results) { - const settings = validateDisableSettings( - result._postcssResult, - 'reportDescriptionlessDisables', - ); - - if (!settings) continue; - - const [enabled, options, stylelintResult] = settings; - - /** @type {Set} */ - const alreadyReported = new Set(); - - for (const [rule, ruleRanges] of Object.entries(stylelintResult.disabledRanges)) { - for (const range of ruleRanges) { - if (range.description) continue; - - if (alreadyReported.has(range.comment)) continue; - - if (enabled === optionsMatches(options, 'except', rule)) { - // An 'all' rule will get copied for each individual rule. If the - // configuration is `[false, {except: ['specific-rule']}]`, we - // don't want to report the copies that match except, so we record - // the comment as already reported. - if (!enabled && rule === 'all') alreadyReported.add(range.comment); - - continue; - } - - alreadyReported.add(range.comment); - - // If the comment doesn't have a location, we can't report a useful error. - // In practice we expect all comments to have locations, though. - if (!range.comment.source || !range.comment.source.start) continue; - - result.warnings.push({ - text: `Disable for "${rule}" is missing a description`, - rule: '--report-descriptionless-disables', - line: range.comment.source.start.line, - column: range.comment.source.start.column, - endLine: range.comment.source.end && range.comment.source.end.line, - endColumn: range.comment.source.end && range.comment.source.end.column, - severity: options.severity, - }); +export default function descriptionlessDisables(postcssResult) { + const [enabled, options] = validateDisableSettings( + postcssResult, + 'reportDescriptionlessDisables', + ); + + if (!options) return; + + /** @type {Set} */ + const alreadyReported = new Set(); + + for (const [rule, ruleRanges] of Object.entries(postcssResult.stylelint.disabledRanges)) { + for (const range of ruleRanges) { + if (range.description) continue; + + const commentNode = range.comment; + + if (alreadyReported.has(commentNode)) continue; + + if (enabled === optionsMatches(options, 'except', rule)) { + // An 'all' rule will get copied for each individual rule. If the + // configuration is `[false, {except: ['specific-rule']}]`, we + // don't want to report the copies that match except, so we record + // the comment as already reported. + if (!enabled && rule === 'all') alreadyReported.add(commentNode); + + continue; } + + alreadyReported.add(commentNode); + + reportCommentProblem({ + rule: '--report-descriptionless-disables', + message: `Disable for "${rule}" is missing a description`, + severity: options.severity, + commentNode, + postcssResult, + }); } } } diff --git a/lib/invalidScopeDisables.cjs b/lib/invalidScopeDisables.cjs index faa3ecd545..9f0bdd6583 100644 --- a/lib/invalidScopeDisables.cjs +++ b/lib/invalidScopeDisables.cjs @@ -3,47 +3,41 @@ 'use strict'; const optionsMatches = require('./utils/optionsMatches.cjs'); +const reportCommentProblem = require('./utils/reportCommentProblem.cjs'); const validateDisableSettings = require('./validateDisableSettings.cjs'); /** - * @param {import('stylelint').LintResult[]} results + * @param {import('stylelint').PostcssResult} postcssResult + * @returns {void} */ -function invalidScopeDisables(results) { - for (const result of results) { - const settings = validateDisableSettings(result._postcssResult, 'reportInvalidScopeDisables'); +function invalidScopeDisables(postcssResult) { + const [enabled, options] = validateDisableSettings(postcssResult, 'reportInvalidScopeDisables'); - if (!settings) continue; + if (!options) return; - const [enabled, options, stylelintResult] = settings; + const configRules = postcssResult.stylelint.config?.rules; - const configRules = (stylelintResult.config || {}).rules || {}; + if (!configRules) return; - const usedRules = new Set(Object.keys(configRules)); + const usedRules = new Set(Object.keys(configRules)); - usedRules.add('all'); + usedRules.add('all'); - for (const [rule, ruleRanges] of Object.entries(stylelintResult.disabledRanges)) { - if (usedRules.has(rule)) continue; + for (const [rule, ruleRanges] of Object.entries(postcssResult.stylelint.disabledRanges)) { + if (usedRules.has(rule)) continue; - if (enabled === optionsMatches(options, 'except', rule)) continue; + if (enabled === optionsMatches(options, 'except', rule)) continue; - for (const range of ruleRanges) { - if (!range.strictStart && !range.strictEnd) continue; + for (const range of ruleRanges) { + if (!range.strictStart && !range.strictEnd) continue; - // If the comment doesn't have a location, we can't report a useful error. - // In practice we expect all comments to have locations, though. - if (!range.comment.source || !range.comment.source.start) continue; - - result.warnings.push({ - text: `Rule "${rule}" isn't enabled`, - rule: '--report-invalid-scope-disables', - line: range.comment.source.start.line, - column: range.comment.source.start.column, - endLine: range.comment.source.end && range.comment.source.end.line, - endColumn: range.comment.source.end && range.comment.source.end.column, - severity: options.severity, - }); - } + reportCommentProblem({ + rule: '--report-invalid-scope-disables', + message: `Rule "${rule}" isn't enabled`, + severity: options.severity, + commentNode: range.comment, + postcssResult, + }); } } } diff --git a/lib/invalidScopeDisables.mjs b/lib/invalidScopeDisables.mjs index 6cb459fd1c..08459927b7 100644 --- a/lib/invalidScopeDisables.mjs +++ b/lib/invalidScopeDisables.mjs @@ -1,45 +1,39 @@ import optionsMatches from './utils/optionsMatches.mjs'; +import reportCommentProblem from './utils/reportCommentProblem.mjs'; import validateDisableSettings from './validateDisableSettings.mjs'; /** - * @param {import('stylelint').LintResult[]} results + * @param {import('stylelint').PostcssResult} postcssResult + * @returns {void} */ -export default function invalidScopeDisables(results) { - for (const result of results) { - const settings = validateDisableSettings(result._postcssResult, 'reportInvalidScopeDisables'); +export default function invalidScopeDisables(postcssResult) { + const [enabled, options] = validateDisableSettings(postcssResult, 'reportInvalidScopeDisables'); - if (!settings) continue; + if (!options) return; - const [enabled, options, stylelintResult] = settings; + const configRules = postcssResult.stylelint.config?.rules; - const configRules = (stylelintResult.config || {}).rules || {}; + if (!configRules) return; - const usedRules = new Set(Object.keys(configRules)); + const usedRules = new Set(Object.keys(configRules)); - usedRules.add('all'); + usedRules.add('all'); - for (const [rule, ruleRanges] of Object.entries(stylelintResult.disabledRanges)) { - if (usedRules.has(rule)) continue; + for (const [rule, ruleRanges] of Object.entries(postcssResult.stylelint.disabledRanges)) { + if (usedRules.has(rule)) continue; - if (enabled === optionsMatches(options, 'except', rule)) continue; + if (enabled === optionsMatches(options, 'except', rule)) continue; - for (const range of ruleRanges) { - if (!range.strictStart && !range.strictEnd) continue; + for (const range of ruleRanges) { + if (!range.strictStart && !range.strictEnd) continue; - // If the comment doesn't have a location, we can't report a useful error. - // In practice we expect all comments to have locations, though. - if (!range.comment.source || !range.comment.source.start) continue; - - result.warnings.push({ - text: `Rule "${rule}" isn't enabled`, - rule: '--report-invalid-scope-disables', - line: range.comment.source.start.line, - column: range.comment.source.start.column, - endLine: range.comment.source.end && range.comment.source.end.line, - endColumn: range.comment.source.end && range.comment.source.end.column, - severity: options.severity, - }); - } + reportCommentProblem({ + rule: '--report-invalid-scope-disables', + message: `Rule "${rule}" isn't enabled`, + severity: options.severity, + commentNode: range.comment, + postcssResult, + }); } } } diff --git a/lib/lintSource.cjs b/lib/lintSource.cjs index 8dd0d345d2..c0cb54e5eb 100644 --- a/lib/lintSource.cjs +++ b/lib/lintSource.cjs @@ -3,11 +3,15 @@ 'use strict'; const node_path = require('node:path'); +const descriptionlessDisables = require('./descriptionlessDisables.cjs'); const getConfigForFile = require('./getConfigForFile.cjs'); const getPostcssResult = require('./getPostcssResult.cjs'); +const invalidScopeDisables = require('./invalidScopeDisables.cjs'); const isPathIgnored = require('./isPathIgnored.cjs'); const isPathNotFoundError = require('./utils/isPathNotFoundError.cjs'); const lintPostcssResult = require('./lintPostcssResult.cjs'); +const needlessDisables = require('./needlessDisables.cjs'); +const reportDisables = require('./reportDisables.cjs'); /** @typedef {import('stylelint').InternalApi} StylelintInternalApi */ /** @typedef {import('stylelint').GetLintSourceOptions} Options */ @@ -110,6 +114,11 @@ async function lintSource(stylelint, options = {}) { await lintPostcssResult(stylelint._options, stylelintPostcssResult, config); + reportDisables(stylelintPostcssResult); + needlessDisables(stylelintPostcssResult); + invalidScopeDisables(stylelintPostcssResult); + descriptionlessDisables(stylelintPostcssResult); + return stylelintPostcssResult; } diff --git a/lib/lintSource.mjs b/lib/lintSource.mjs index ddffbf7879..5472e87510 100644 --- a/lib/lintSource.mjs +++ b/lib/lintSource.mjs @@ -1,10 +1,14 @@ import { isAbsolute } from 'node:path'; +import descriptionlessDisables from './descriptionlessDisables.mjs'; import getConfigForFile from './getConfigForFile.mjs'; import getPostcssResult from './getPostcssResult.mjs'; +import invalidScopeDisables from './invalidScopeDisables.mjs'; import isPathIgnored from './isPathIgnored.mjs'; import isPathNotFoundError from './utils/isPathNotFoundError.mjs'; import lintPostcssResult from './lintPostcssResult.mjs'; +import needlessDisables from './needlessDisables.mjs'; +import reportDisables from './reportDisables.mjs'; /** @typedef {import('stylelint').InternalApi} StylelintInternalApi */ /** @typedef {import('stylelint').GetLintSourceOptions} Options */ @@ -107,6 +111,11 @@ export default async function lintSource(stylelint, options = {}) { await lintPostcssResult(stylelint._options, stylelintPostcssResult, config); + reportDisables(stylelintPostcssResult); + needlessDisables(stylelintPostcssResult); + invalidScopeDisables(stylelintPostcssResult); + descriptionlessDisables(stylelintPostcssResult); + return stylelintPostcssResult; } diff --git a/lib/needlessDisables.cjs b/lib/needlessDisables.cjs index 709fa5c1f2..f97441c30a 100644 --- a/lib/needlessDisables.cjs +++ b/lib/needlessDisables.cjs @@ -4,92 +4,79 @@ const optionsMatches = require('./utils/optionsMatches.cjs'); const putIfAbsent = require('./utils/putIfAbsent.cjs'); +const reportCommentProblem = require('./utils/reportCommentProblem.cjs'); const validateDisableSettings = require('./validateDisableSettings.cjs'); -/** @typedef {import('postcss').Comment} PostcssComment */ -/** @typedef {import('stylelint').DisabledRange} DisabledRange */ -/** @typedef {import('stylelint').DisableReportRange} DisableReportRange */ - /** - * @param {import('stylelint').LintResult[]} results + * @param {import('stylelint').PostcssResult} postcssResult + * @returns {void} */ -function needlessDisables(results) { - for (const result of results) { - const settings = validateDisableSettings(result._postcssResult, 'reportNeedlessDisables'); - - if (!settings) continue; - - const [enabled, options, stylelintResult] = settings; +function needlessDisables(postcssResult) { + const [enabled, options] = validateDisableSettings(postcssResult, 'reportNeedlessDisables'); - const rangeData = stylelintResult.disabledRanges; + if (!options) return; - if (!rangeData) continue; + const rangeData = postcssResult.stylelint.disabledRanges; + const disabledWarnings = postcssResult.stylelint.disabledWarnings || []; - const disabledWarnings = stylelintResult.disabledWarnings || []; + // A map from `stylelint-disable` comments to the set of rules that + // are usefully disabled by each comment. We track this + // comment-by-comment rather than range-by-range because ranges that + // disable *all* rules are duplicated for each rule they apply to in + // practice. + /** @type {Map>}} */ + const usefulDisables = new Map(); - // A map from `stylelint-disable` comments to the set of rules that - // are usefully disabled by each comment. We track this - // comment-by-comment rather than range-by-range because ranges that - // disable *all* rules are duplicated for each rule they apply to in - // practice. - /** @type {Map>}} */ - const usefulDisables = new Map(); - - for (const warning of disabledWarnings) { - const rule = warning.rule; - const ruleRanges = rangeData[rule]; - - if (ruleRanges) { - for (const range of ruleRanges) { - if (isWarningInRange(warning, range)) { - putIfAbsent(usefulDisables, range.comment, () => new Set()).add(rule); - } - } - } + for (const warning of disabledWarnings) { + const rule = warning.rule; + const ruleRanges = rangeData[rule]; - for (const range of rangeData.all || []) { + if (ruleRanges) { + for (const range of ruleRanges) { if (isWarningInRange(warning, range)) { putIfAbsent(usefulDisables, range.comment, () => new Set()).add(rule); } } } - const allRangeComments = new Set((rangeData.all || []).map((range) => range.comment)); + for (const range of rangeData.all || []) { + if (isWarningInRange(warning, range)) { + putIfAbsent(usefulDisables, range.comment, () => new Set()).add(rule); + } + } + } + + const allRangeComments = new Set((rangeData.all || []).map((range) => range.comment)); - for (const [rule, ranges] of Object.entries(rangeData)) { - for (const range of ranges) { - if (rule !== 'all' && allRangeComments.has(range.comment)) continue; + for (const [rule, ranges] of Object.entries(rangeData)) { + for (const range of ranges) { + const commentNode = range.comment; - if (enabled === optionsMatches(options, 'except', rule)) continue; + if (rule !== 'all' && allRangeComments.has(commentNode)) continue; - const useful = usefulDisables.get(range.comment) || new Set(); + if (enabled === optionsMatches(options, 'except', rule)) continue; - // Only emit a warning if this range's comment isn't useful for this rule. - // For the special rule "all", only emit a warning if it's not useful for - // *any* rules, because it covers all of them. - if (rule === 'all' ? useful.size !== 0 : useful.has(rule)) continue; + const useful = usefulDisables.get(commentNode) || new Set(); - // If the comment doesn't have a location, we can't report a useful error. - // In practice we expect all comments to have locations, though. - if (!range.comment.source || !range.comment.source.start) continue; + // Only emit a warning if this range's comment isn't useful for this rule. + // For the special rule "all", only emit a warning if it's not useful for + // *any* rules, because it covers all of them. + if (rule === 'all' ? useful.size !== 0 : useful.has(rule)) continue; - result.warnings.push({ - text: `Needless disable for "${rule}"`, - rule: '--report-needless-disables', - line: range.comment.source.start.line, - column: range.comment.source.start.column, - endLine: range.comment.source.end && range.comment.source.end.line, - endColumn: range.comment.source.end && range.comment.source.end.column, - severity: options.severity, - }); - } + reportCommentProblem({ + rule: '--report-needless-disables', + message: `Needless disable for "${rule}"`, + severity: options.severity, + commentNode, + postcssResult, + }); } } } /** * @param {import('stylelint').DisabledWarning} warning - * @param {DisabledRange} range + * @param {import('stylelint').DisabledRange} range * @return {boolean} */ function isWarningInRange(warning, range) { diff --git a/lib/needlessDisables.mjs b/lib/needlessDisables.mjs index 6c0b64f4db..82c958e779 100644 --- a/lib/needlessDisables.mjs +++ b/lib/needlessDisables.mjs @@ -1,91 +1,78 @@ import optionsMatches from './utils/optionsMatches.mjs'; import putIfAbsent from './utils/putIfAbsent.mjs'; +import reportCommentProblem from './utils/reportCommentProblem.mjs'; import validateDisableSettings from './validateDisableSettings.mjs'; -/** @typedef {import('postcss').Comment} PostcssComment */ -/** @typedef {import('stylelint').DisabledRange} DisabledRange */ -/** @typedef {import('stylelint').DisableReportRange} DisableReportRange */ - /** - * @param {import('stylelint').LintResult[]} results + * @param {import('stylelint').PostcssResult} postcssResult + * @returns {void} */ -export default function needlessDisables(results) { - for (const result of results) { - const settings = validateDisableSettings(result._postcssResult, 'reportNeedlessDisables'); - - if (!settings) continue; - - const [enabled, options, stylelintResult] = settings; +export default function needlessDisables(postcssResult) { + const [enabled, options] = validateDisableSettings(postcssResult, 'reportNeedlessDisables'); - const rangeData = stylelintResult.disabledRanges; + if (!options) return; - if (!rangeData) continue; + const rangeData = postcssResult.stylelint.disabledRanges; + const disabledWarnings = postcssResult.stylelint.disabledWarnings || []; - const disabledWarnings = stylelintResult.disabledWarnings || []; + // A map from `stylelint-disable` comments to the set of rules that + // are usefully disabled by each comment. We track this + // comment-by-comment rather than range-by-range because ranges that + // disable *all* rules are duplicated for each rule they apply to in + // practice. + /** @type {Map>}} */ + const usefulDisables = new Map(); - // A map from `stylelint-disable` comments to the set of rules that - // are usefully disabled by each comment. We track this - // comment-by-comment rather than range-by-range because ranges that - // disable *all* rules are duplicated for each rule they apply to in - // practice. - /** @type {Map>}} */ - const usefulDisables = new Map(); - - for (const warning of disabledWarnings) { - const rule = warning.rule; - const ruleRanges = rangeData[rule]; - - if (ruleRanges) { - for (const range of ruleRanges) { - if (isWarningInRange(warning, range)) { - putIfAbsent(usefulDisables, range.comment, () => new Set()).add(rule); - } - } - } + for (const warning of disabledWarnings) { + const rule = warning.rule; + const ruleRanges = rangeData[rule]; - for (const range of rangeData.all || []) { + if (ruleRanges) { + for (const range of ruleRanges) { if (isWarningInRange(warning, range)) { putIfAbsent(usefulDisables, range.comment, () => new Set()).add(rule); } } } - const allRangeComments = new Set((rangeData.all || []).map((range) => range.comment)); + for (const range of rangeData.all || []) { + if (isWarningInRange(warning, range)) { + putIfAbsent(usefulDisables, range.comment, () => new Set()).add(rule); + } + } + } + + const allRangeComments = new Set((rangeData.all || []).map((range) => range.comment)); - for (const [rule, ranges] of Object.entries(rangeData)) { - for (const range of ranges) { - if (rule !== 'all' && allRangeComments.has(range.comment)) continue; + for (const [rule, ranges] of Object.entries(rangeData)) { + for (const range of ranges) { + const commentNode = range.comment; - if (enabled === optionsMatches(options, 'except', rule)) continue; + if (rule !== 'all' && allRangeComments.has(commentNode)) continue; - const useful = usefulDisables.get(range.comment) || new Set(); + if (enabled === optionsMatches(options, 'except', rule)) continue; - // Only emit a warning if this range's comment isn't useful for this rule. - // For the special rule "all", only emit a warning if it's not useful for - // *any* rules, because it covers all of them. - if (rule === 'all' ? useful.size !== 0 : useful.has(rule)) continue; + const useful = usefulDisables.get(commentNode) || new Set(); - // If the comment doesn't have a location, we can't report a useful error. - // In practice we expect all comments to have locations, though. - if (!range.comment.source || !range.comment.source.start) continue; + // Only emit a warning if this range's comment isn't useful for this rule. + // For the special rule "all", only emit a warning if it's not useful for + // *any* rules, because it covers all of them. + if (rule === 'all' ? useful.size !== 0 : useful.has(rule)) continue; - result.warnings.push({ - text: `Needless disable for "${rule}"`, - rule: '--report-needless-disables', - line: range.comment.source.start.line, - column: range.comment.source.start.column, - endLine: range.comment.source.end && range.comment.source.end.line, - endColumn: range.comment.source.end && range.comment.source.end.column, - severity: options.severity, - }); - } + reportCommentProblem({ + rule: '--report-needless-disables', + message: `Needless disable for "${rule}"`, + severity: options.severity, + commentNode, + postcssResult, + }); } } } /** * @param {import('stylelint').DisabledWarning} warning - * @param {DisabledRange} range + * @param {import('stylelint').DisabledRange} range * @return {boolean} */ function isWarningInRange(warning, range) { diff --git a/lib/prepareReturnValue.cjs b/lib/prepareReturnValue.cjs index bd63a8beb1..1e1cf74107 100644 --- a/lib/prepareReturnValue.cjs +++ b/lib/prepareReturnValue.cjs @@ -2,11 +2,6 @@ // please instead edit the ESM counterpart and rebuild with Rollup (npm run build). 'use strict'; -const descriptionlessDisables = require('./descriptionlessDisables.cjs'); -const invalidScopeDisables = require('./invalidScopeDisables.cjs'); -const needlessDisables = require('./needlessDisables.cjs'); -const reportDisables = require('./reportDisables.cjs'); - /** @typedef {import('stylelint').Formatter} Formatter */ /** @typedef {import('stylelint').LintResult} StylelintResult */ /** @typedef {import('stylelint').LinterOptions["maxWarnings"]} maxWarnings */ @@ -21,11 +16,6 @@ const reportDisables = require('./reportDisables.cjs'); * @returns {LinterResult} */ function prepareReturnValue(stylelintResults, maxWarnings, formatter, cwd) { - reportDisables(stylelintResults); - needlessDisables(stylelintResults); - invalidScopeDisables(stylelintResults); - descriptionlessDisables(stylelintResults); - let errored = false; for (const result of stylelintResults) { diff --git a/lib/prepareReturnValue.mjs b/lib/prepareReturnValue.mjs index fcefa430a2..441bf98a72 100644 --- a/lib/prepareReturnValue.mjs +++ b/lib/prepareReturnValue.mjs @@ -1,8 +1,3 @@ -import descriptionlessDisables from './descriptionlessDisables.mjs'; -import invalidScopeDisables from './invalidScopeDisables.mjs'; -import needlessDisables from './needlessDisables.mjs'; -import reportDisables from './reportDisables.mjs'; - /** @typedef {import('stylelint').Formatter} Formatter */ /** @typedef {import('stylelint').LintResult} StylelintResult */ /** @typedef {import('stylelint').LinterOptions["maxWarnings"]} maxWarnings */ @@ -17,11 +12,6 @@ import reportDisables from './reportDisables.mjs'; * @returns {LinterResult} */ export default function prepareReturnValue(stylelintResults, maxWarnings, formatter, cwd) { - reportDisables(stylelintResults); - needlessDisables(stylelintResults); - invalidScopeDisables(stylelintResults); - descriptionlessDisables(stylelintResults); - let errored = false; for (const result of stylelintResults) { diff --git a/lib/reportDisables.cjs b/lib/reportDisables.cjs index 72c0ff160b..3e6634358c 100644 --- a/lib/reportDisables.cjs +++ b/lib/reportDisables.cjs @@ -2,61 +2,46 @@ // please instead edit the ESM counterpart and rebuild with Rollup (npm run build). 'use strict'; -/** @typedef {import('stylelint').DisableReportRange} DisabledRange */ -/** @typedef {import('stylelint').LintResult} StylelintResult */ -/** @typedef {import('stylelint').ConfigRuleSettings} StylelintConfigRuleSettings */ +const reportCommentProblem = require('./utils/reportCommentProblem.cjs'); /** * Returns a report describing which `results` (if any) contain disabled ranges * for rules that disallow disables via `reportDisables: true`. * - * @param {StylelintResult[]} results + * @param {import('stylelint').PostcssResult} postcssResult + * @returns {void} */ -function reportDisables(results) { - for (const result of results) { - // File with `CssSyntaxError` don't have `_postcssResult`s. - if (!result._postcssResult) { - continue; - } - - const rangeData = result._postcssResult.stylelint.disabledRanges; - - if (!rangeData) continue; +function reportDisables(postcssResult) { + const rangeData = postcssResult.stylelint.disabledRanges; + const configRules = postcssResult.stylelint.config?.rules; - const config = result._postcssResult.stylelint.config; + if (!configRules) return; - if (!config || !config.rules) continue; - - // If no rules actually disallow disables, don't bother looking for ranges - // that correspond to disabled rules. - if (!Object.values(config.rules).some((rule) => reportDisablesForRule(rule))) { - continue; - } + // If no rules actually disallow disables, don't bother looking for ranges + // that correspond to disabled rules. + if (!Object.values(configRules).some((rule) => reportDisablesForRule(rule))) { + return; + } - for (const [rule, ranges] of Object.entries(rangeData)) { - for (const range of ranges) { - if (!reportDisablesForRule(config.rules[rule] || [])) continue; + for (const [rule, ranges] of Object.entries(rangeData)) { + for (const range of ranges) { + if (!configRules[rule]) continue; - // If the comment doesn't have a location, we can't report a useful error. - // In practice we expect all comments to have locations, though. - if (!range.comment.source || !range.comment.source.start) continue; + if (!reportDisablesForRule(configRules[rule])) continue; - result.warnings.push({ - text: `Rule "${rule}" may not be disabled`, - rule: 'reportDisables', - line: range.comment.source.start.line, - column: range.comment.source.start.column, - endLine: range.comment.source.end && range.comment.source.end.line, - endColumn: range.comment.source.end && range.comment.source.end.column, - severity: 'error', - }); - } + reportCommentProblem({ + rule: 'reportDisables', + message: `Rule "${rule}" may not be disabled`, + severity: 'error', + commentNode: range.comment, + postcssResult, + }); } } } /** - * @param {StylelintConfigRuleSettings} options + * @param {import('stylelint').ConfigRuleSettings} options * @return {boolean} */ function reportDisablesForRule(options) { diff --git a/lib/reportDisables.mjs b/lib/reportDisables.mjs index 5a0e886dad..46f3dde691 100644 --- a/lib/reportDisables.mjs +++ b/lib/reportDisables.mjs @@ -1,58 +1,43 @@ -/** @typedef {import('stylelint').DisableReportRange} DisabledRange */ -/** @typedef {import('stylelint').LintResult} StylelintResult */ -/** @typedef {import('stylelint').ConfigRuleSettings} StylelintConfigRuleSettings */ +import reportCommentProblem from './utils/reportCommentProblem.mjs'; /** * Returns a report describing which `results` (if any) contain disabled ranges * for rules that disallow disables via `reportDisables: true`. * - * @param {StylelintResult[]} results + * @param {import('stylelint').PostcssResult} postcssResult + * @returns {void} */ -export default function reportDisables(results) { - for (const result of results) { - // File with `CssSyntaxError` don't have `_postcssResult`s. - if (!result._postcssResult) { - continue; - } - - const rangeData = result._postcssResult.stylelint.disabledRanges; +export default function reportDisables(postcssResult) { + const rangeData = postcssResult.stylelint.disabledRanges; + const configRules = postcssResult.stylelint.config?.rules; - if (!rangeData) continue; + if (!configRules) return; - const config = result._postcssResult.stylelint.config; + // If no rules actually disallow disables, don't bother looking for ranges + // that correspond to disabled rules. + if (!Object.values(configRules).some((rule) => reportDisablesForRule(rule))) { + return; + } - if (!config || !config.rules) continue; + for (const [rule, ranges] of Object.entries(rangeData)) { + for (const range of ranges) { + if (!configRules[rule]) continue; - // If no rules actually disallow disables, don't bother looking for ranges - // that correspond to disabled rules. - if (!Object.values(config.rules).some((rule) => reportDisablesForRule(rule))) { - continue; - } + if (!reportDisablesForRule(configRules[rule])) continue; - for (const [rule, ranges] of Object.entries(rangeData)) { - for (const range of ranges) { - if (!reportDisablesForRule(config.rules[rule] || [])) continue; - - // If the comment doesn't have a location, we can't report a useful error. - // In practice we expect all comments to have locations, though. - if (!range.comment.source || !range.comment.source.start) continue; - - result.warnings.push({ - text: `Rule "${rule}" may not be disabled`, - rule: 'reportDisables', - line: range.comment.source.start.line, - column: range.comment.source.start.column, - endLine: range.comment.source.end && range.comment.source.end.line, - endColumn: range.comment.source.end && range.comment.source.end.column, - severity: 'error', - }); - } + reportCommentProblem({ + rule: 'reportDisables', + message: `Rule "${rule}" may not be disabled`, + severity: 'error', + commentNode: range.comment, + postcssResult, + }); } } } /** - * @param {StylelintConfigRuleSettings} options + * @param {import('stylelint').ConfigRuleSettings} options * @return {boolean} */ function reportDisablesForRule(options) { diff --git a/lib/utils/reportCommentProblem.cjs b/lib/utils/reportCommentProblem.cjs new file mode 100644 index 0000000000..96bb2723ea --- /dev/null +++ b/lib/utils/reportCommentProblem.cjs @@ -0,0 +1,49 @@ +// NOTICE: This file is generated by Rollup. To modify it, +// please instead edit the ESM counterpart and rebuild with Rollup (npm run build). +'use strict'; + +/** + * @param {{ + * rule: string; + * message: string; + * severity: import('stylelint').Severity; + * commentNode: import('postcss').Comment; + * postcssResult: import('stylelint').PostcssResult; + * }} args + * @returns {void} + */ +function reportCommentProblem({ + rule, + message, + severity, + commentNode, + postcssResult, +}) { + const { source } = commentNode; + + // If the comment doesn't have a location, we can't report a useful error. + // In practice we expect all comments to have locations, though. + if (!source?.start) return; + + postcssResult.warn(message, { + rule, + severity, + node: commentNode, + start: source.start, + end: source.end, + }); + + switch (severity) { + case 'error': + postcssResult.stylelint.stylelintError = true; + break; + case 'warning': + postcssResult.stylelint.stylelintWarning = true; + break; + default: + // no-op + break; + } +} + +module.exports = reportCommentProblem; diff --git a/lib/utils/reportCommentProblem.mjs b/lib/utils/reportCommentProblem.mjs new file mode 100644 index 0000000000..fa41956525 --- /dev/null +++ b/lib/utils/reportCommentProblem.mjs @@ -0,0 +1,43 @@ +/** + * @param {{ + * rule: string; + * message: string; + * severity: import('stylelint').Severity; + * commentNode: import('postcss').Comment; + * postcssResult: import('stylelint').PostcssResult; + * }} args + * @returns {void} + */ +export default function reportCommentProblem({ + rule, + message, + severity, + commentNode, + postcssResult, +}) { + const { source } = commentNode; + + // If the comment doesn't have a location, we can't report a useful error. + // In practice we expect all comments to have locations, though. + if (!source?.start) return; + + postcssResult.warn(message, { + rule, + severity, + node: commentNode, + start: source.start, + end: source.end, + }); + + switch (severity) { + case 'error': + postcssResult.stylelint.stylelintError = true; + break; + case 'warning': + postcssResult.stylelint.stylelintWarning = true; + break; + default: + // no-op + break; + } +} diff --git a/lib/validateDisableSettings.cjs b/lib/validateDisableSettings.cjs index 01a39de5bd..954f7cf422 100644 --- a/lib/validateDisableSettings.cjs +++ b/lib/validateDisableSettings.cjs @@ -6,33 +6,25 @@ const validateTypes = require('./utils/validateTypes.cjs'); const validateOptions = require('./utils/validateOptions.cjs'); /** - * @typedef {import('stylelint').PostcssResult} PostcssResult * @typedef {import('stylelint').DisableOptions} DisableOptions - * @typedef {import('stylelint').DisablePropertyName} DisablePropertyName - * @typedef {import('stylelint').StylelintPostcssResult} StylelintPostcssResult */ /** * Validates that the stylelint config for `result` has a valid disable field - * named `field`, and returns the result in normalized form as well as a - * `StylelintPostcssResult` for convenience. + * named `field`, and returns the result in normalized form. * - * Returns `null` if no disables should be reported, and automatically reports - * an invalid configuration. If this returns non-`null`, it guarantees that - * `result._postcssResult` is defined as well. + * Returns `[]` if no disables should be reported, and automatically reports + * an invalid configuration. * - * @param {PostcssResult | undefined} result - * @param {DisablePropertyName} field - * @return {[boolean, Required, StylelintPostcssResult] | null} + * @param {import('stylelint').PostcssResult} result + * @param {import('stylelint').DisablePropertyName} field + * @returns {[boolean, Required] | []} */ function validateDisableSettings(result, field) { - // Files with `CssSyntaxError`s don't have `_postcssResult`s. - if (!result) return null; - const stylelintResult = result.stylelint; // Files with linting errors may not have configs associated with them. - if (!stylelintResult.config) return null; + if (!stylelintResult.config) return []; const rawSettings = stylelintResult.config[field]; @@ -64,11 +56,11 @@ function validateDisableSettings(result, field) { }, ); - if (!validOptions) return null; + if (!validOptions) return []; // If the check is disabled with no exceptions, there's no reason to run // it at all. - if (!enabled && !options.except) return null; + if (!enabled && !options.except) return []; return [ enabled, @@ -76,7 +68,6 @@ function validateDisableSettings(result, field) { except: options.except || [], severity: options.severity || stylelintResult.config.defaultSeverity || 'error', }, - stylelintResult, ]; } diff --git a/lib/validateDisableSettings.mjs b/lib/validateDisableSettings.mjs index 3c33f321fb..a17c2282b9 100644 --- a/lib/validateDisableSettings.mjs +++ b/lib/validateDisableSettings.mjs @@ -2,33 +2,25 @@ import { isRegExp, isString } from './utils/validateTypes.mjs'; import validateOptions from './utils/validateOptions.mjs'; /** - * @typedef {import('stylelint').PostcssResult} PostcssResult * @typedef {import('stylelint').DisableOptions} DisableOptions - * @typedef {import('stylelint').DisablePropertyName} DisablePropertyName - * @typedef {import('stylelint').StylelintPostcssResult} StylelintPostcssResult */ /** * Validates that the stylelint config for `result` has a valid disable field - * named `field`, and returns the result in normalized form as well as a - * `StylelintPostcssResult` for convenience. + * named `field`, and returns the result in normalized form. * - * Returns `null` if no disables should be reported, and automatically reports - * an invalid configuration. If this returns non-`null`, it guarantees that - * `result._postcssResult` is defined as well. + * Returns `[]` if no disables should be reported, and automatically reports + * an invalid configuration. * - * @param {PostcssResult | undefined} result - * @param {DisablePropertyName} field - * @return {[boolean, Required, StylelintPostcssResult] | null} + * @param {import('stylelint').PostcssResult} result + * @param {import('stylelint').DisablePropertyName} field + * @returns {[boolean, Required] | []} */ export default function validateDisableSettings(result, field) { - // Files with `CssSyntaxError`s don't have `_postcssResult`s. - if (!result) return null; - const stylelintResult = result.stylelint; // Files with linting errors may not have configs associated with them. - if (!stylelintResult.config) return null; + if (!stylelintResult.config) return []; const rawSettings = stylelintResult.config[field]; @@ -60,11 +52,11 @@ export default function validateDisableSettings(result, field) { }, ); - if (!validOptions) return null; + if (!validOptions) return []; // If the check is disabled with no exceptions, there's no reason to run // it at all. - if (!enabled && !options.except) return null; + if (!enabled && !options.except) return []; return [ enabled, @@ -72,6 +64,5 @@ export default function validateDisableSettings(result, field) { except: options.except || [], severity: options.severity || stylelintResult.config.defaultSeverity || 'error', }, - stylelintResult, ]; }