From a30c5c3017042d1bd544f426a6db438ae88f8329 Mon Sep 17 00:00:00 2001 From: Masafumi Koba <473530+ybiquitous@users.noreply.github.com> Date: Sat, 1 Oct 2022 22:13:47 +0900 Subject: [PATCH] Fix `block-no-empty` false positives for `reportNeedlessDisables` --- .changeset/poor-ducks-relate.md | 5 ++ lib/assignDisabledRanges.js | 53 ++++++++++---------- lib/rules/block-no-empty/__tests__/index.js | 42 +++++++++++----- lib/rules/block-no-empty/index.js | 31 ++++++++---- lib/utils/__tests__/stylelintCommand.test.js | 36 +++++++++++++ lib/utils/hasEmptyBlock.js | 6 +-- lib/utils/stylelintCommand.js | 53 ++++++++++++++++++++ 7 files changed, 175 insertions(+), 51 deletions(-) create mode 100644 .changeset/poor-ducks-relate.md create mode 100644 lib/utils/__tests__/stylelintCommand.test.js create mode 100644 lib/utils/stylelintCommand.js diff --git a/.changeset/poor-ducks-relate.md b/.changeset/poor-ducks-relate.md new file mode 100644 index 0000000000..4e07fdff9b --- /dev/null +++ b/.changeset/poor-ducks-relate.md @@ -0,0 +1,5 @@ +--- +"stylelint": patch +--- + +Fixed: `block-no-empty` false positives for `reportNeedlessDisables` diff --git a/lib/assignDisabledRanges.js b/lib/assignDisabledRanges.js index 3bedb8b605..584415b929 100644 --- a/lib/assignDisabledRanges.js +++ b/lib/assignDisabledRanges.js @@ -1,13 +1,16 @@ 'use strict'; const isStandardSyntaxComment = require('./utils/isStandardSyntaxComment'); +const { + DISABLE_COMMAND, + DISABLE_LINE_COMMAND, + DISABLE_NEXT_LINE_COMMAND, + ENABLE_COMMAND, + extractStylelintCommand, + isStylelintCommand, +} = require('./utils/stylelintCommand'); const { assert, assertNumber, assertString } = require('./utils/validateTypes'); -const COMMAND_PREFIX = 'stylelint-'; -const disableCommand = `${COMMAND_PREFIX}disable`; -const enableCommand = `${COMMAND_PREFIX}enable`; -const disableLineCommand = `${COMMAND_PREFIX}disable-line`; -const disableNextLineCommand = `${COMMAND_PREFIX}disable-next-line`; const ALL_RULES = 'all'; /** @typedef {import('postcss').Comment} PostcssComment */ @@ -122,13 +125,6 @@ module.exports = function assignDisabledRanges(root, result) { return result; - /** - * @param {PostcssComment} comment - */ - function isStylelintCommand(comment) { - return comment.text.startsWith(disableCommand) || comment.text.startsWith(enableCommand); - } - /** * @param {PostcssComment} comment */ @@ -137,7 +133,7 @@ module.exports = function assignDisabledRanges(root, result) { const line = comment.source.start.line; const description = getDescription(comment.text); - for (const ruleName of getCommandRules(disableLineCommand, comment.text)) { + for (const ruleName of getCommandRules(DISABLE_LINE_COMMAND, comment.text)) { disableLine(comment, line, ruleName, description); } } @@ -151,7 +147,7 @@ module.exports = function assignDisabledRanges(root, result) { const line = comment.source.end.line; const description = getDescription(comment.text); - for (const ruleName of getCommandRules(disableNextLineCommand, comment.text)) { + for (const ruleName of getCommandRules(DISABLE_NEXT_LINE_COMMAND, comment.text)) { disableLine(comment, line + 1, ruleName, description); } } @@ -197,7 +193,7 @@ module.exports = function assignDisabledRanges(root, result) { function processDisableCommand(comment) { const description = getDescription(comment.text); - for (const ruleToDisable of getCommandRules(disableCommand, comment.text)) { + for (const ruleToDisable of getCommandRules(DISABLE_COMMAND, comment.text)) { const isAllRules = ruleToDisable === ALL_RULES; if (ruleIsDisabled(ruleToDisable)) { @@ -229,7 +225,7 @@ module.exports = function assignDisabledRanges(root, result) { * @param {PostcssComment} comment */ function processEnableCommand(comment) { - for (const ruleToEnable of getCommandRules(enableCommand, comment.text)) { + for (const ruleToEnable of getCommandRules(ENABLE_COMMAND, comment.text)) { // need fallback if endLine will be undefined const endLine = comment.source && comment.source.end && comment.source.end.line; @@ -289,22 +285,25 @@ module.exports = function assignDisabledRanges(root, result) { * @param {PostcssComment} comment */ function checkComment(comment) { - const text = comment.text; - // Ignore comments that are not relevant commands - if (text.indexOf(COMMAND_PREFIX) !== 0) { + if (!isStylelintCommand(comment)) { return; } - if (text.startsWith(disableLineCommand)) { - processDisableLineCommand(comment); - } else if (text.startsWith(disableNextLineCommand)) { - processDisableNextLineCommand(comment); - } else if (text.startsWith(disableCommand)) { - processDisableCommand(comment); - } else if (text.startsWith(enableCommand)) { - processEnableCommand(comment); + switch (extractStylelintCommand(comment)) { + case DISABLE_LINE_COMMAND: + processDisableLineCommand(comment); + break; + case DISABLE_NEXT_LINE_COMMAND: + processDisableNextLineCommand(comment); + break; + case DISABLE_COMMAND: + processDisableCommand(comment); + break; + case ENABLE_COMMAND: + processEnableCommand(comment); + break; } } diff --git a/lib/rules/block-no-empty/__tests__/index.js b/lib/rules/block-no-empty/__tests__/index.js index cc29d74775..31a10a0360 100644 --- a/lib/rules/block-no-empty/__tests__/index.js +++ b/lib/rules/block-no-empty/__tests__/index.js @@ -37,6 +37,15 @@ testRule({ { code: '@import url(x.css)', }, + { + code: 'a { /* stylelint-disable */ }', + }, + { + code: 'a { /* stylelint-disable block-no-empty */ }', + }, + { + code: 'a { /* stylelint-disable-line block-no-empty */ }', + }, ], reject: [ @@ -136,17 +145,20 @@ testRule({ endLine: 1, endColumn: 20, }, + { + code: 'a { /* stylelint-disable foo */ }', + message: messages.rejected, + line: 1, + column: 3, + endLine: 1, + endColumn: 34, + }, ], }); testRule({ ruleName, - config: [ - true, - { - ignore: ['comments'], - }, - ], + config: [true, { ignore: ['comments'] }], accept: [ { @@ -158,6 +170,9 @@ testRule({ { code: '@import "foo.css";', }, + { + code: 'a { /* stylelint-disable */ }', + }, ], reject: [ @@ -201,6 +216,14 @@ testRule({ endLine: 1, endColumn: 31, }, + { + code: 'a { /* stylelint-disable foo */ }', + message: messages.rejected, + line: 1, + column: 3, + endLine: 1, + endColumn: 34, + }, ], }); @@ -249,12 +272,7 @@ testRule({ testRule({ ruleName, - config: [ - true, - { - ignore: ['comments'], - }, - ], + config: [true, { ignore: ['comments'] }], customSyntax: 'postcss-scss', accept: [ diff --git a/lib/rules/block-no-empty/index.js b/lib/rules/block-no-empty/index.js index a74fd05e69..c5eba4c398 100644 --- a/lib/rules/block-no-empty/index.js +++ b/lib/rules/block-no-empty/index.js @@ -2,10 +2,11 @@ const beforeBlockString = require('../../utils/beforeBlockString'); const hasBlock = require('../../utils/hasBlock'); -const hasEmptyBlock = require('../../utils/hasEmptyBlock'); const optionsMatches = require('../../utils/optionsMatches'); const report = require('../../utils/report'); const ruleMessages = require('../../utils/ruleMessages'); +const { isStylelintCommand } = require('../../utils/stylelintCommand'); +const { isComment } = require('../../utils/typeGuards'); const validateOptions = require('../../utils/validateOptions'); const { isBoolean } = require('../../utils/validateTypes'); @@ -48,21 +49,17 @@ const rule = (primary, secondaryOptions) => { root.walkRules(check); root.walkAtRules(check); + /** @typedef {import('postcss').Rule | import('postcss').AtRule} Statement */ + /** - * @param {import('postcss').Rule | import('postcss').AtRule} statement + * @param {Statement} statement */ function check(statement) { - if (!hasEmptyBlock(statement) && !ignoreComments) { - return; - } - if (!hasBlock(statement)) { return; } - const hasCommentsOnly = statement.nodes.every((node) => node.type === 'comment'); - - if (!hasCommentsOnly) { + if (hasNotableChild(statement)) { return; } @@ -81,6 +78,22 @@ const rule = (primary, secondaryOptions) => { ruleName, }); } + + /** + * @param {Statement} statement + * @returns {boolean} + */ + function hasNotableChild(statement) { + return statement.nodes.some((child) => { + if (isComment(child)) { + if (ignoreComments) return false; + + if (isStylelintCommand(child)) return false; + } + + return true; + }); + } }; }; diff --git a/lib/utils/__tests__/stylelintCommand.test.js b/lib/utils/__tests__/stylelintCommand.test.js new file mode 100644 index 0000000000..c3f4db2265 --- /dev/null +++ b/lib/utils/__tests__/stylelintCommand.test.js @@ -0,0 +1,36 @@ +'use strict'; + +const postcss = require('postcss'); + +const { extractStylelintCommand, isStylelintCommand } = require('../stylelintCommand'); + +test('extractStylelintCommand', () => { + expect(extractStylelintCommand(comment('stylelint-disable'))).toBe('stylelint-disable'); + expect(extractStylelintCommand(comment('stylelint-disable '))).toBe('stylelint-disable'); + expect(extractStylelintCommand(comment('stylelint-disable\t'))).toBe('stylelint-disable'); + expect(extractStylelintCommand(comment('stylelint-disable --'))).toBe('stylelint-disable'); + + expect(extractStylelintCommand(comment(''))).toBe(''); + expect(extractStylelintCommand(comment(' '))).toBe(''); + expect(extractStylelintCommand(comment('\t'))).toBe(''); +}); + +test('isStylelintCommand', () => { + expect(isStylelintCommand(comment('stylelint-disable'))).toBe(true); + expect(isStylelintCommand(comment('stylelint-disable-line'))).toBe(true); + expect(isStylelintCommand(comment('stylelint-disable-next-line'))).toBe(true); + expect(isStylelintCommand(comment('stylelint-enable'))).toBe(true); + + expect(isStylelintCommand(comment('stylelint-'))).toBe(false); + expect(isStylelintCommand(comment('stylelint-disable-'))).toBe(false); + expect(isStylelintCommand(comment('stylelint-disable-lineee'))).toBe(false); + expect(isStylelintCommand(comment('stylelint-disable-next'))).toBe(false); + expect(isStylelintCommand(comment('stylelint-enable-'))).toBe(false); + expect(isStylelintCommand(comment(''))).toBe(false); + expect(isStylelintCommand(comment(' '))).toBe(false); + expect(isStylelintCommand(comment('\t'))).toBe(false); +}); + +function comment(text) { + return postcss.comment({ text }); +} diff --git a/lib/utils/hasEmptyBlock.js b/lib/utils/hasEmptyBlock.js index 0bea60bbeb..eb7076967d 100644 --- a/lib/utils/hasEmptyBlock.js +++ b/lib/utils/hasEmptyBlock.js @@ -1,5 +1,7 @@ 'use strict'; +const hasBlock = require('./hasBlock'); + /** * Check if a statement has an empty block. * @@ -7,7 +9,5 @@ * @return {boolean} True if the statement has a block and it is empty */ module.exports = function hasEmptyBlock(statement) { - return ( - statement.nodes !== undefined && statement.nodes.length === 0 // has block - ); // and is empty + return hasBlock(statement) && statement.nodes.length === 0; }; diff --git a/lib/utils/stylelintCommand.js b/lib/utils/stylelintCommand.js new file mode 100644 index 0000000000..3979aee8cf --- /dev/null +++ b/lib/utils/stylelintCommand.js @@ -0,0 +1,53 @@ +'use strict'; + +const { assertString } = require('./validateTypes'); + +const DISABLE_COMMAND = 'stylelint-disable'; +const DISABLE_LINE_COMMAND = 'stylelint-disable-line'; +const DISABLE_NEXT_LINE_COMMAND = 'stylelint-disable-next-line'; +const ENABLE_COMMAND = 'stylelint-enable'; + +const ALL_COMMANDS = new Set([ + DISABLE_COMMAND, + DISABLE_LINE_COMMAND, + DISABLE_NEXT_LINE_COMMAND, + ENABLE_COMMAND, +]); + +/** @typedef {import('postcss').Comment} Comment */ + +/** + * Extract a command from a given comment. + * + * @param {Comment} comment + * @returns {string} + */ +function extractStylelintCommand(comment) { + const [command] = comment.text.split(/\s/, 1); + + assertString(command); + + return command; +} + +/** + * Tests if the given comment is a Stylelint command. + * + * @param {Comment} comment + * @returns {boolean} + */ +function isStylelintCommand(comment) { + const command = extractStylelintCommand(comment); + + return command !== undefined && ALL_COMMANDS.has(command); +} + +module.exports = { + DISABLE_COMMAND, + DISABLE_LINE_COMMAND, + DISABLE_NEXT_LINE_COMMAND, + ENABLE_COMMAND, + + extractStylelintCommand, + isStylelintCommand, +};