Fix block-no-empty false positives for reportNeedlessDisables (#6381

)

.changeset/poor-ducks-relate.md

@@ -0,0 +1,5 @@
+---
+"stylelint": patch
+---
+
+Fixed: `block-no-empty` false positives for `reportNeedlessDisables`

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;
 		}
 	}
 

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: [

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;
+			});
+		}
 	};
 };
 

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 });
+}

lib/utils/hasEmptyBlock.js

@@ -1,13 +1,13 @@
 'use strict';
 
+const hasBlock = require('./hasBlock');
+
 /**
  * Check if a statement has an empty block.
  *
  * @param {import('postcss').Rule | import('postcss').AtRule} statement - postcss rule or at-rule node
  * @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;
 };

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,
+};