Skip to content

Commit

Permalink
Fix report flags not reporting on subsequent runs when cache is used (#…
Browse files Browse the repository at this point in the history
…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.
  • Loading branch information
ybiquitous committed Jan 22, 2024
1 parent f02d168 commit 382961f
Show file tree
Hide file tree
Showing 18 changed files with 411 additions and 400 deletions.
5 changes: 5 additions & 0 deletions .changeset/afraid-cobras-heal.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"stylelint": patch
---

Fixed: report flags not reporting on subsequent runs when cache is used
22 changes: 22 additions & 0 deletions lib/__tests__/standalone-cache.test.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
74 changes: 32 additions & 42 deletions lib/descriptionlessDisables.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -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<PostcssComment>} */
const alreadyReported = new Set();
/** @type {Set<import('postcss').Comment>} */
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,
});
}
}
}
Expand Down
90 changes: 40 additions & 50 deletions lib/descriptionlessDisables.mjs
Original file line number Diff line number Diff line change
@@ -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<PostcssComment>} */
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<import('postcss').Comment>} */
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,
});
}
}
}
50 changes: 22 additions & 28 deletions lib/invalidScopeDisables.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
});
}
}
}
Expand Down
50 changes: 22 additions & 28 deletions lib/invalidScopeDisables.mjs
Original file line number Diff line number Diff line change
@@ -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,
});
}
}
}
9 changes: 9 additions & 0 deletions lib/lintSource.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Expand Down Expand Up @@ -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;
}

Expand Down

0 comments on commit 382961f

Please sign in to comment.