Add a reportDisables secondary option (#4897)

* Rename UnusedRange to DisableReportRange and .unusedRule to .rule This allows us to re-use this structure for representing errors relating to disabled that aren't specifically related to the disable being unused. * Add a reportDisables secondary option This allows config authors to specify that certain rules may not be disabled by authors. Closes #4875 * Produce different messages for different disable-level failures * Code review
stylelint · Aug 19, 2020 · 858dcd5 · 858dcd5
1 parent 40e60ce
commit 858dcd5
Show file tree

Hide file tree

Showing 26 changed files with 314 additions and 59 deletions.
diff --git a/docs/developer-guide/formatters.md b/docs/developer-guide/formatters.md
@@ -57,7 +57,7 @@ And the second argument (`returnValue`) is an object (type `StylelintStandaloneR
       "ranges": [
         {
           "start": 10,
-          "unusedRule": "indentation"
+          "rule": "indentation"
         }
       ]
     }
@@ -69,7 +69,7 @@ And the second argument (`returnValue`) is an object (type `StylelintStandaloneR
       "ranges": [
         {
           "start": 1,
-          "unusedRule": "color-named"
+          "rule": "color-named"
         }
       ]
     }

diff --git a/docs/user-guide/configure.md b/docs/user-guide/configure.md
@@ -135,6 +135,26 @@ For example:
 
 Reporters may use these severity levels to display violations or exit the process differently.
 
+### `reportDisables`
+
+You can set the `reportDisables` secondary option to report any disable comments for this rule, effectively disallowing authors to opt out of it.
+
+For example:
+
+```json
+{
+  "rules": {
+    "indentation": [
+      2,
+      {
+        "except": ["value"],
+        "reportDisables": true
+      }
+    ]
+  }
+}
+```
+
 ## `defaultSeverity`
 
 You can set the default severity level for all rules that do not have a severity specified in their secondary options. For example, you can set the default severity to `"warning"`:

diff --git a/lib/__tests__/cli.test.js b/lib/__tests__/cli.test.js
@@ -234,14 +234,30 @@ describe('CLI', () => {
 		expect(process.stdout.write).toHaveBeenCalledTimes(2);
 		expect(process.stdout.write).toHaveBeenNthCalledWith(
 			1,
-			expect.stringContaining('unused rule: color-named'),
+			expect.stringContaining('needless disable: color-named'),
 		);
 		expect(process.stdout.write).toHaveBeenNthCalledWith(
 			2,
 			expect.stringContaining('Unexpected empty block'),
 		);
 	});
 
+	it('reports disallowed disables', async () => {
+		await cli([
+			'--config',
+			replaceBackslashes(path.join(fixturesPath, 'config-block-no-empty-report-disables.json')),
+			replaceBackslashes(path.join(fixturesPath, 'empty-block-with-relevant-disable.css')),
+		]);
+
+		expect(process.exitCode).toBe(2);
+
+		expect(process.stdout.write).toHaveBeenCalledTimes(1);
+		expect(process.stdout.write).toHaveBeenNthCalledWith(
+			1,
+			expect.stringContaining('forbidden disable: block-no-empty'),
+		);
+	});
+
 	it('--stdin', async () => {
 		await cli(['--stdin', '--config', `${fixturesPath}/config-no-empty-source.json`]);
 

diff --git a/lib/__tests__/fixtures/config-block-no-empty-report-disables.json b/lib/__tests__/fixtures/config-block-no-empty-report-disables.json
@@ -0,0 +1,5 @@
+{
+	"rules": {
+		"block-no-empty": [true, { "reportDisables": true }]
+	}
+}
diff --git a/lib/__tests__/fixtures/empty-block-with-relevant-disable.css b/lib/__tests__/fixtures/empty-block-with-relevant-disable.css
@@ -0,0 +1,2 @@
+/* stylelint-disable block-no-empty */
+a {}
diff --git a/lib/__tests__/invalidScopeDisables.test.js b/lib/__tests__/invalidScopeDisables.test.js
@@ -40,8 +40,8 @@ it('invalidScopeDisables simple case', () => {
 		expect(report).toHaveLength(1);
 		expect(report[0].ranges).toHaveLength(2);
 		expect(report[0].ranges).toEqual([
-			{ end: 3, start: 1, unusedRule: 'block-no-empty' },
-			{ end: 5, start: 5, unusedRule: 'block-no-empty' },
+			{ end: 3, start: 1, rule: 'block-no-empty', unusedRule: 'block-no-empty' },
+			{ end: 5, start: 5, rule: 'block-no-empty', unusedRule: 'block-no-empty' },
 		]);
 	});
 });
@@ -65,11 +65,11 @@ it('invalidScopeDisables complex case', () => {
 		expect(invalidScopeDisables(linted.results, config)).toEqual([
 			{
 				source: source('disabled-ranges-1.css'),
-				ranges: [{ start: 1, end: 3, unusedRule: 'color-named' }],
+				ranges: [{ start: 1, end: 3, rule: 'color-named', unusedRule: 'color-named' }],
 			},
 			{
 				source: source('disabled-ranges-2.css'),
-				ranges: [{ start: 5, end: 5, unusedRule: 'color-named' }],
+				ranges: [{ start: 5, end: 5, rule: 'color-named', unusedRule: 'color-named' }],
 			},
 		]);
 	});
@@ -91,7 +91,7 @@ it('invalidScopeDisables ignored case', () => {
 		expect(invalidScopeDisables(linted.results, config)).toEqual([
 			{
 				source: source('disabled-ranges-1.css'),
-				ranges: [{ start: 5, end: 7, unusedRule: 'block-no-empty' }],
+				ranges: [{ start: 5, end: 7, rule: 'block-no-empty', unusedRule: 'block-no-empty' }],
 			},
 		]);
 	});
@@ -119,6 +119,7 @@ it('invalidScopeDisables for config file', () => {
 			{
 				end: undefined,
 				start: 4,
+				rule: 'foo',
 				unusedRule: 'foo',
 			},
 		]);

diff --git a/lib/__tests__/needlessDisables.test.js b/lib/__tests__/needlessDisables.test.js
@@ -42,8 +42,8 @@ it('needlessDisables simple case', () => {
 
 		expect(report).toHaveLength(1);
 		expect(report[0].ranges).toEqual([
-			{ start: 7, end: 9, unusedRule: 'all' },
-			{ start: 11, end: 11, unusedRule: 'block-no-empty' },
+			{ start: 7, end: 9, rule: 'all', unusedRule: 'all' },
+			{ start: 11, end: 11, rule: 'block-no-empty', unusedRule: 'block-no-empty' },
 		]);
 	});
 });
@@ -70,17 +70,17 @@ it('needlessDisables complex case', () => {
 			{
 				source: source('disabled-ranges-1.css'),
 				ranges: [
-					{ start: 1, end: 3, unusedRule: 'color-named' },
-					{ start: 5, end: 7, unusedRule: 'block-no-empty' },
-					{ start: 10, end: 10, unusedRule: 'block-no-empty' },
+					{ start: 1, end: 3, rule: 'color-named', unusedRule: 'color-named' },
+					{ start: 5, end: 7, rule: 'block-no-empty', unusedRule: 'block-no-empty' },
+					{ start: 10, end: 10, rule: 'block-no-empty', unusedRule: 'block-no-empty' },
 				],
 			},
 			{
 				source: source('disabled-ranges-2.css'),
 				ranges: [
-					{ start: 5, end: 5, unusedRule: 'color-named' },
-					{ start: 6, end: 6, unusedRule: 'block-no-empty' },
-					{ start: 8, end: 10, unusedRule: 'block-no-empty' },
+					{ start: 5, end: 5, rule: 'color-named', unusedRule: 'color-named' },
+					{ start: 6, end: 6, rule: 'block-no-empty', unusedRule: 'block-no-empty' },
+					{ start: 8, end: 10, rule: 'block-no-empty', unusedRule: 'block-no-empty' },
 				],
 			},
 		]);
@@ -104,9 +104,9 @@ it('needlessDisables ignored case', () => {
 			{
 				source: source('disabled-ranges-1.css'),
 				ranges: [
-					{ start: 1, end: 3, unusedRule: 'color-named' },
-					{ start: 5, end: 7, unusedRule: 'block-no-empty' },
-					{ start: 10, end: 10, unusedRule: 'all' },
+					{ start: 1, end: 3, rule: 'color-named', unusedRule: 'color-named' },
+					{ start: 5, end: 7, rule: 'block-no-empty', unusedRule: 'block-no-empty' },
+					{ start: 10, end: 10, rule: 'all', unusedRule: 'all' },
 				],
 			},
 		]);

diff --git a/lib/__tests__/reportDisables.test.js b/lib/__tests__/reportDisables.test.js
@@ -0,0 +1,105 @@
+'use strict';
+
+const standalone = require('../standalone');
+const stripIndent = require('common-tags').stripIndent;
+
+describe('reportDisables', () => {
+	it('reports a disabled comment', () => {
+		const config = {
+			rules: { 'block-no-empty': [true, { reportDisables: true }] },
+		};
+
+		const css = stripIndent`
+    /* stylelint-disable block-no-empty */
+    a {}
+    /* stylelint-enable block-no-empty */
+    a {
+      b {} /* stylelint-disable-line block-no-empty */
+    }
+    `;
+
+		return standalone({ config, code: css }).then((linted) => {
+			const report = linted.reportedDisables;
+
+			expect(report).toHaveLength(1);
+			expect(report[0].ranges).toEqual([
+				{ start: 1, end: 3, rule: 'block-no-empty', unusedRule: 'block-no-empty' },
+				{ start: 5, end: 5, rule: 'block-no-empty', unusedRule: 'block-no-empty' },
+			]);
+
+			// Although these disables are reported as issues, they're still in effect
+			// so the underlying lint issues are not reported.
+			expect(linted.results[0].warnings).toHaveLength(0);
+		});
+	});
+
+	it('reports an ignored disabled comment', () => {
+		const config = {
+			rules: { 'block-no-empty': [true, { reportDisables: true }] },
+		};
+
+		const css = stripIndent`
+    /* stylelint-disable block-no-empty */
+    a {}
+    /* stylelint-enable block-no-empty */
+    a {
+      b {} /* stylelint-disable-line block-no-empty */
+    }
+    `;
+
+		return standalone({
+			config,
+			code: css,
+			ignoreDisables: true,
+		}).then((linted) => {
+			const report = linted.reportedDisables;
+
+			expect(report).toHaveLength(1);
+			expect(report[0].ranges).toEqual([
+				{ start: 1, end: 3, rule: 'block-no-empty', unusedRule: 'block-no-empty' },
+				{ start: 5, end: 5, rule: 'block-no-empty', unusedRule: 'block-no-empty' },
+			]);
+
+			expect(linted.results[0].warnings).toHaveLength(2);
+		});
+	});
+
+	it("doesn't report disables by default", () => {
+		const config = {
+			rules: { 'block-no-empty': [true] },
+		};
+
+		const css = stripIndent`
+    /* stylelint-disable block-no-empty */
+    a {}
+    /* stylelint-enable block-no-empty */
+    a {
+      b {} /* stylelint-disable-line block-no-empty */
+    }
+    `;
+
+		return standalone({ config, code: css }).then((linted) => {
+			const report = linted.reportedDisables;
+
+			expect(report).toHaveLength(0);
+		});
+	});
+
+	// This should be handled by the global `reportUnscopedDisables` option (#2292).
+	it("doesn't report unscoped disables", () => {
+		const config = {
+			rules: { 'block-no-empty': [true, { reportDisables: true }] },
+		};
+
+		const css = stripIndent`
+    a {} /* stylelint-disable-line */
+    `;
+
+		return standalone({ config, code: css }).then((linted) => {
+			const report = linted.reportedDisables;
+
+			expect(report).toHaveLength(1);
+			expect(report[0].ranges).toHaveLength(0);
+		});
+	});
+});
diff --git a/lib/__tests__/standalone-invalidScopeDisables.test.js b/lib/__tests__/standalone-invalidScopeDisables.test.js
@@ -25,6 +25,7 @@ it('standalone with input css and `reportInvalidScopeDisables`', () => {
 		expect(invalidScopeDisables[0].ranges).toHaveLength(1);
 		expect(invalidScopeDisables[0].ranges[0]).toEqual({
 			start: 1,
+			rule: 'color-named',
 			unusedRule: 'color-named',
 		});
 	});
@@ -45,6 +46,7 @@ it('standalone with input file(s) and `reportInvalidScopeDisables`', () => {
 		);
 		expect(invalidScopeDisables[0].ranges[0]).toEqual({
 			start: 1,
+			rule: 'color-named',
 			unusedRule: 'color-named',
 		});
 	});

diff --git a/lib/__tests__/standalone-needlessDisables.test.js b/lib/__tests__/standalone-needlessDisables.test.js
@@ -27,6 +27,7 @@ it('standalone with input css and `reportNeedlessDisables`', () => {
 		expect(needlessDisables[0].ranges).toHaveLength(1);
 		expect(needlessDisables[0].ranges[0]).toEqual({
 			start: 1,
+			rule: 'color-named',
 			unusedRule: 'color-named',
 		});
 
@@ -130,6 +131,7 @@ it('standalone with input file(s) and `reportNeedlessDisables`', () => {
 		);
 		expect(needlessDisables[0].ranges[0]).toEqual({
 			start: 1,
+			rule: 'color-named',
 			unusedRule: 'color-named',
 		});
 	});

diff --git a/lib/__tests__/stylelintignore-test/stylelintignore.test.js b/lib/__tests__/stylelintignore-test/stylelintignore.test.js
@@ -64,6 +64,7 @@ describe('stylelintignore', () => {
 				expect(data).toEqual({
 					errored: false,
 					output: '[]',
+					reportedDisables: [],
 					results: [],
 				});
 			});
@@ -98,6 +99,7 @@ describe('stylelintignore', () => {
 				expect(data).toEqual({
 					errored: false,
 					output: '[]',
+					reportedDisables: [],
 					results: [],
 				});
 			});
@@ -112,6 +114,7 @@ describe('stylelintignore', () => {
 				expect(data).toEqual({
 					errored: false,
 					output: '[]',
+					reportedDisables: [],
 					results: [],
 				});
 			});

diff --git a/lib/cli.js b/lib/cli.js
@@ -481,16 +481,29 @@ module.exports = (argv) => {
 				.then((linted) => {
 					const reports = [];
 
+					const report = disableOptionsReportStringFormatter(
+						linted.reportedDisables || [],
+						'forbidden disable',
+					);
+
+					if (report) reports.push(report);
+
 					if (reportNeedlessDisables) {
-						const report = disableOptionsReportStringFormatter(linted.needlessDisables || []);
+						const report = disableOptionsReportStringFormatter(
+							linted.needlessDisables || [],
+							'needless disable',
+						);
 
 						if (report) {
 							reports.push(report);
 						}
 					}
 
 					if (reportInvalidScopeDisables) {
-						const report = disableOptionsReportStringFormatter(linted.invalidScopeDisables || []);
+						const report = disableOptionsReportStringFormatter(
+							linted.invalidScopeDisables || [],
+							'disable with invalid scope',
+						);
 
 						if (report) {
 							reports.push(report);