diff --git a/CHANGELOG.md b/CHANGELOG.md index d72f4a6a29..0234ea34d2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,11 @@ All notable changes to this project are documented in this file. +## Head + +- Added: disable comments that are reported as errors for various reasons are now reported as standard lint errors rather than a separate class of errors that must be handled specially. +- Deprecated: `StylelintStandaloneReturnValue.reportedDisables`, `.descriptionlessDisables`, `.needlessDisables`, and `.invalidScopeDisables`. `.reportedDisables` will always be empty and the other properties will always be undefined, since these errors now show up in `.results` instead. + ## 13.7.2 - Fixed: regression for disable commands and adjacent double-slash comments ([#4950](https://github.com/stylelint/stylelint/pull/4950)). diff --git a/docs/developer-guide/formatters.md b/docs/developer-guide/formatters.md index ad1b2f280d..75099772a9 100644 --- a/docs/developer-guide/formatters.md +++ b/docs/developer-guide/formatters.md @@ -50,30 +50,6 @@ And the second argument (`returnValue`) is an object (type `StylelintStandaloneR ```js { "errored": false, // `true` if there were any warnings with "error" severity - "needlessDisables": [ - // Present if stylelint was configured with `reportNeedlessDisables: true` - { - "source": "path/to/file.css", - "ranges": [ - { - "start": 10, - "rule": "indentation" - } - ] - } - ], - "invalidScopeDisables": [ - // Present if stylelint was configured with `reportInvalidScopeDisables: true` - { - "source": "path/to/file.css", - "ranges": [ - { - "start": 1, - "rule": "color-named" - } - ] - } - ], "maxWarningsExceeded": { // Present if stylelint was configured with a `maxWarnings` count "maxWarnings": 10, diff --git a/docs/user-guide/configure.md b/docs/user-guide/configure.md index b3455dbe08..9c380e808c 100644 --- a/docs/user-guide/configure.md +++ b/docs/user-guide/configure.md @@ -155,7 +155,7 @@ For example: } ``` -The report is included in the [`reportedDisables`](usage/node-api.md#reporteddisables) property to the returned data of Node.js API. +The report is considered to be a lint error. ## `defaultSeverity` diff --git a/docs/user-guide/usage/node-api.md b/docs/user-guide/usage/node-api.md index 902b0e4cae..055daa10e4 100644 --- a/docs/user-guide/usage/node-api.md +++ b/docs/user-guide/usage/node-api.md @@ -66,22 +66,6 @@ An array containing all the stylelint result objects (the objects that formatter An object containing the maximum number of warnings and the amount found, e.g. `{ maxWarnings: 0, foundWarnings: 12 }`. -### `reportedDisables` - -An array of objects, one for each source, with tells you which `stylelint-disable` comments for rules enabled [`reportDisables`](../configure.md#reportdisables). - -### `needlessDisables` - -An array of objects, one for each source, with tells you which `stylelint-disable` comments are not blocking a lint violation. - -### `invalidScopeDisables` - -An array of objects, one for each source, with tells you which rule in a `stylelint-disable ` comment doesn't exist within the configuration object. - -### `descriptionlessDisables` - -An array of objects, one for each source, with tells you which `stylelint-disable` comments without a description. - ## Syntax errors `stylelint.lint()` does not reject the Promise when your CSS contains syntax errors. diff --git a/docs/user-guide/usage/options.md b/docs/user-guide/usage/options.md index 5676c715dc..f4850701a8 100644 --- a/docs/user-guide/usage/options.md +++ b/docs/user-guide/usage/options.md @@ -146,7 +146,7 @@ Produce a report to clean up your codebase, keeping only the `stylelint-disable` If needless disables are found, the: - CLI process exits with code `2` -- Node.js API adds a [`needlessDisables`](node-api.md#needlessdisables) property to the returned data +- Node.js API adds errors to the returned data ## `reportInvalidScopeDisables` @@ -157,7 +157,7 @@ Produce a report of the `stylelint-disable` comments that used for rules that do If invalid scope disables are found, the: - CLI process exits with code `2` -- Node.js API adds a [`invalidScopeDisables`](node-api.md#invalidscopedisables) property to the returned data +- Node.js API adds errors to the returned data ## `reportDescriptionlessDisables` @@ -196,7 +196,7 @@ a {} If descriptionless disables are found, the: - CLI process exits with code `2` -- Node.js API adds a [`descriptionlessDisables`](node-api.md#descriptionlessdisables) property to the returned data +- Node.js API adds errors to the returned data ## `codeFilename` diff --git a/lib/__tests__/cli.test.js b/lib/__tests__/cli.test.js index 9599f39d4e..99d5f2491d 100644 --- a/lib/__tests__/cli.test.js +++ b/lib/__tests__/cli.test.js @@ -232,14 +232,9 @@ describe('CLI', () => { expect(process.exitCode).toBe(2); - expect(process.stdout.write).toHaveBeenCalledTimes(2); - expect(process.stdout.write).toHaveBeenNthCalledWith( - 1, - expect.stringContaining('needless disable: color-named'), - ); - expect(process.stdout.write).toHaveBeenNthCalledWith( - 2, - expect.stringContaining('Unexpected empty block'), + expect(process.stdout.write).toHaveBeenCalledTimes(1); + expect(process.stdout.write).toHaveBeenCalledWith( + expect.stringMatching(/Needless disable for "color-named".*Unexpected empty block/s), ); }); @@ -253,9 +248,8 @@ describe('CLI', () => { expect(process.exitCode).toBe(2); expect(process.stdout.write).toHaveBeenCalledTimes(1); - expect(process.stdout.write).toHaveBeenNthCalledWith( - 1, - expect.stringContaining('forbidden disable: block-no-empty'), + expect(process.stdout.write).toHaveBeenCalledWith( + expect.stringContaining('Rule "block-no-empty" may not be disabled'), ); }); @@ -270,9 +264,8 @@ describe('CLI', () => { expect(process.exitCode).toBe(2); expect(process.stdout.write).toHaveBeenCalledTimes(1); - expect(process.stdout.write).toHaveBeenNthCalledWith( - 1, - expect.stringContaining('descriptionless disable: block-no-empty'), + expect(process.stdout.write).toHaveBeenCalledWith( + expect.stringContaining('Disable for "block-no-empty" is missing a description'), ); }); diff --git a/lib/__tests__/descriptionlessDisables.test.js b/lib/__tests__/descriptionlessDisables.test.js index d1983b83f3..b515bc79d7 100644 --- a/lib/__tests__/descriptionlessDisables.test.js +++ b/lib/__tests__/descriptionlessDisables.test.js @@ -14,7 +14,7 @@ it('descriptionlessDisables', () => { /* stylelint-enable */ a { b {} /* stylelint-disable-line block-no-empty -- Description */ - } + } /* stylelint-disable-next-line block-no-empty * -- * Description */ @@ -35,27 +35,34 @@ it('descriptionlessDisables', () => { code: css, reportDescriptionlessDisables: true, }).then((linted) => { - const report = linted.descriptionlessDisables; + const results = linted.results; + + expect(results).toHaveLength(1); + const warnings = results[0].warnings.filter( + (warning) => warning.rule === '--report-descriptionless-disables', + ); - expect(report).toHaveLength(1); - expect(report[0].ranges).toEqual([ + expect(warnings).toEqual([ { - start: 12, - end: 14, - rule: 'all', - unusedRule: 'all', + line: 12, + column: 1, + rule: '--report-descriptionless-disables', + severity: 'error', + text: 'Disable for "all" is missing a description', }, { - start: 16, - end: 16, - rule: 'block-no-empty', - unusedRule: 'block-no-empty', + line: 16, + column: 8, + rule: '--report-descriptionless-disables', + severity: 'error', + text: 'Disable for "block-no-empty" is missing a description', }, { - start: 19, - end: 19, - rule: 'block-no-empty', - unusedRule: 'block-no-empty', + line: 18, + column: 1, + rule: '--report-descriptionless-disables', + severity: 'error', + text: 'Disable for "block-no-empty" is missing a description', }, ]); }); diff --git a/lib/__tests__/disableRanges.test.js b/lib/__tests__/disableRanges.test.js index 26e6696581..d3c9aea560 100644 --- a/lib/__tests__/disableRanges.test.js +++ b/lib/__tests__/disableRanges.test.js @@ -1,10 +1,13 @@ 'use strict'; +const _ = require('lodash'); const assignDisabledRanges = require('../assignDisabledRanges'); const less = require('postcss-less'); const postcss = require('postcss'); const scss = require('postcss-scss'); +/* eslint jest/expect-expect: ["error", { "assertFunctionNames": ["expect*"] }] */ + it('no disabling', () => { return testDisableRanges('a {}').then((result) => { expect(result.stylelint.disabledRanges).toEqual({ all: [] }); @@ -13,7 +16,7 @@ it('no disabling', () => { it('disable without re-enabling', () => { return testDisableRanges('/* stylelint-disable */\na {}').then((result) => { - expect(result.stylelint.disabledRanges).toEqual({ + expectDisableRanges(result, { all: [ { start: 1, @@ -30,7 +33,7 @@ it('disable and re-enable', () => { b {} /* stylelint-enable */ .foo {}`).then((result) => { - expect(result.stylelint.disabledRanges).toEqual({ + expectDisableRanges(result, { all: [ { start: 2, @@ -53,7 +56,7 @@ it('disable and re-enable twice', () => { b {} /* stylelint-enable */ .foo {}`).then((result) => { - expect(result.stylelint.disabledRanges).toEqual({ + expectDisableRanges(result, { all: [ { start: 2, @@ -75,7 +78,7 @@ it('disable and re-enable twice', () => { it('disable rule without re-enabling', () => { return testDisableRanges('/* stylelint-disable foo-bar */\na {}') .then((result) => { - expect(result.stylelint.disabledRanges).toEqual({ + expectDisableRanges(result, { all: [], 'foo-bar': [ { @@ -91,7 +94,7 @@ it('disable rule without re-enabling', () => { return testDisableRanges( '/* stylelint-disable selector-combinator-space-before */\n' + 'a {}', ).then((result) => { - expect(result.stylelint.disabledRanges).toEqual({ + expectDisableRanges(result, { all: [], 'selector-combinator-space-before': [ { @@ -116,7 +119,7 @@ it('mixed disabling of specific and all rules, enabling of all', () => { b {} /* stylelint-enable */ .foo {}`).then((result) => { - expect(result.stylelint.disabledRanges).toEqual({ + expectDisableRanges(result, { all: [], 'foo-bar': [ { @@ -147,7 +150,7 @@ it('mixed disabling of specific and all rules, enabling of all', () => { it('disable rules with newline in rule list', () => { return testDisableRanges('/* stylelint-disable foo-bar, hoo-hah,\n\tslime */\n' + 'b {}\n').then( (result) => { - expect(result.stylelint.disabledRanges).toEqual({ + expectDisableRanges(result, { all: [], 'foo-bar': [ { @@ -174,66 +177,57 @@ it('disable rules with newline in rule list', () => { it('disable single line all rules', () => { return testDisableRanges('a {} /* stylelint-disable-line */').then((result) => { - expect(result.stylelint.disabledRanges).toEqual( - { - all: [ - { - start: 1, - end: 1, - strictStart: true, - strictEnd: true, - }, - ], - }, - 'disabling all rules', - ); + expectDisableRanges(result, { + all: [ + { + start: 1, + end: 1, + strictStart: true, + strictEnd: true, + }, + ], + }); }); }); it('disable single line one rule', () => { return testDisableRanges('a {} /* stylelint-disable-line block-no-empty */').then((result) => { - expect(result.stylelint.disabledRanges).toEqual( - { + expectDisableRanges(result, { + all: [], + 'block-no-empty': [ + { + start: 1, + end: 1, + strictStart: true, + strictEnd: true, + }, + ], + }); + }); +}); + +it('disable single line multiple rules', () => { + return testDisableRanges('b {}\n\na {} /* stylelint-disable-line block-no-empty, blergh */').then( + (result) => { + expectDisableRanges(result, { all: [], 'block-no-empty': [ { - start: 1, - end: 1, + start: 3, + end: 3, strictStart: true, strictEnd: true, }, ], - }, - 'disabling a single rule', - ); - }); -}); - -it('disable single line multiple rules', () => { - return testDisableRanges('b {}\n\na {} /* stylelint-disable-line block-no-empty, blergh */').then( - (result) => { - expect(result.stylelint.disabledRanges).toEqual( - { - all: [], - 'block-no-empty': [ - { - start: 3, - end: 3, - strictStart: true, - strictEnd: true, - }, - ], - blergh: [ - { - start: 3, - end: 3, - strictStart: true, - strictEnd: true, - }, - ], - }, - 'disabling multiple specific rules', - ); + blergh: [ + { + start: 3, + end: 3, + strictStart: true, + strictEnd: true, + }, + ], + }); }, ); }); @@ -251,9 +245,25 @@ it('disable single line one rule and re-enable all', () => { it('disable next line all rules', () => { return testDisableRanges('/* stylelint-disable-next-line */\na {} ').then((result) => { - expect(result.stylelint.disabledRanges).toEqual( - { - all: [ + expectDisableRanges(result, { + all: [ + { + start: 2, + end: 2, + strictStart: true, + strictEnd: true, + }, + ], + }); + }); +}); + +it('disable next line one rule', () => { + return testDisableRanges('/* stylelint-disable-next-line block-no-empty */\na {}').then( + (result) => { + expectDisableRanges(result, { + all: [], + 'block-no-empty': [ { start: 2, end: 2, @@ -261,29 +271,7 @@ it('disable next line all rules', () => { strictEnd: true, }, ], - }, - 'disabling all rules', - ); - }); -}); - -it('disable next line one rule', () => { - return testDisableRanges('/* stylelint-disable-next-line block-no-empty */\na {}').then( - (result) => { - expect(result.stylelint.disabledRanges).toEqual( - { - all: [], - 'block-no-empty': [ - { - start: 2, - end: 2, - strictStart: true, - strictEnd: true, - }, - ], - }, - 'disabling a single rule', - ); + }); }, ); }); @@ -296,28 +284,25 @@ it('disable next line multiple rules', () => { /* stylelint-disable-next-line block-no-empty, blergh */ a {}`, (result) => { - expect(result.stylelint.disabledRanges).toEqual( - { - all: [], - 'block-no-empty': [ - { - start: 5, - end: 5, - strictStart: true, - strictEnd: true, - }, - ], - blergh: [ - { - start: 5, - end: 5, - strictStart: true, - strictEnd: true, - }, - ], - }, - 'disabling multiple specific rules', - ); + expectDisableRanges(result, { + all: [], + 'block-no-empty': [ + { + start: 5, + end: 5, + strictStart: true, + strictEnd: true, + }, + ], + blergh: [ + { + start: 5, + end: 5, + strictStart: true, + strictEnd: true, + }, + ], + }); }, ); }); @@ -331,7 +316,7 @@ it('SCSS // line-disabling comment', () => { .use(assignDisabledRanges) .process(scssSource, { syntax: scss, from: undefined }) .then((result) => { - expect(result.stylelint.disabledRanges).toEqual({ + expectDisableRanges(result, { all: [], 'declaration-no-important': [ { @@ -354,7 +339,7 @@ it('Less // line-disabling comment', () => { .use(assignDisabledRanges) .process(lessSource, { syntax: less, from: undefined }) .then((result) => { - expect(result.stylelint.disabledRanges).toEqual({ + expectDisableRanges(result, { all: [], 'declaration-no-important': [ { @@ -375,7 +360,7 @@ it('nested ranges all rule-specific', () => { /* stylelint-enable bar */ /* stylelint-enable foo, hop */ /* stylelint-enable baz */`).then((result) => { - expect(result.stylelint.disabledRanges).toEqual({ + expectDisableRanges(result, { all: [], foo: [ { @@ -418,7 +403,7 @@ it('nested ranges all for all rules', () => { /* stylelint-enable bar */ /* stylelint-disable bar */ /* stylelint-enable */`).then((result) => { - expect(result.stylelint.disabledRanges).toEqual({ + expectDisableRanges(result, { all: [ { start: 1, @@ -449,7 +434,7 @@ it('nested ranges disable rules enable all', () => { return testDisableRanges(`/* stylelint-disable foo */ /* stylelint-disable bar, baz */ /* stylelint-enable */`).then((result) => { - expect(result.stylelint.disabledRanges).toEqual({ + expectDisableRanges(result, { all: [], foo: [ { @@ -484,7 +469,7 @@ it('nested ranges mix disabling enabling all rules and specific rules', () => { /* stylelint-enable foo */ /* stylelint-enable */ /* stylelint-disable bar */`).then((result) => { - expect(result.stylelint.disabledRanges).toEqual({ + expectDisableRanges(result, { all: [ { start: 1, @@ -525,7 +510,7 @@ it('nested ranges another mix', () => { /* stylelint-enable foo */ /* stylelint-disable foo */ /* stylelint-enable */`).then((result) => { - expect(result.stylelint.disabledRanges).toEqual({ + expectDisableRanges(result, { all: [ { start: 1, @@ -534,6 +519,14 @@ it('nested ranges another mix', () => { strictEnd: true, }, ], + bar: [ + { + start: 1, + end: 2, + strictStart: false, + strictEnd: true, + }, + ], foo: [ { start: 1, @@ -548,14 +541,6 @@ it('nested ranges another mix', () => { strictEnd: false, }, ], - bar: [ - { - start: 1, - end: 2, - strictStart: false, - strictEnd: true, - }, - ], }); }); }); @@ -648,7 +633,7 @@ it('enable rule without disabling rule', () => { it('disable (with description) without re-enabling', () => { return testDisableRanges('/* stylelint-disable -- Description */\na {}').then((result) => { - expect(result.stylelint.disabledRanges).toEqual({ + expectDisableRanges(result, { all: [ { start: 1, @@ -666,7 +651,7 @@ it('disable and re-enable (with descriptions)', () => { b {} /* stylelint-enable -- Description */ .foo {}`).then((result) => { - expect(result.stylelint.disabledRanges).toEqual({ + expectDisableRanges(result, { all: [ { start: 2, @@ -683,7 +668,7 @@ it('disable and re-enable (with descriptions)', () => { it('disable rule (with description) without re-enabling', () => { return testDisableRanges('/* stylelint-disable foo-bar -- Description */\na {}').then( (result) => { - expect(result.stylelint.disabledRanges).toEqual({ + expectDisableRanges(result, { all: [], 'foo-bar': [ { @@ -703,7 +688,7 @@ it('disable rules (with description) with newline in rule list', () => { return testDisableRanges( '/* stylelint-disable foo-bar, hoo-hah,\n\tslime -- Description */\n' + 'b {}\n', ).then((result) => { - expect(result.stylelint.disabledRanges).toEqual({ + expectDisableRanges(result, { all: [], 'foo-bar': [ { @@ -739,7 +724,7 @@ it('SCSS // line-disabling comment (with description)', () => { .use(assignDisabledRanges) .process(scssSource, { syntax: scss, from: undefined }) .then((result) => { - expect(result.stylelint.disabledRanges).toEqual({ + expectDisableRanges(result, { all: [], 'declaration-no-important': [ { @@ -766,7 +751,7 @@ it('SCSS // disable next-line comment (with multi-line description)', () => { .use(assignDisabledRanges) .process(scssSource, { syntax: scss, from: undefined }) .then((result) => { - expect(result.stylelint.disabledRanges).toEqual({ + expectDisableRanges(result, { all: [], 'declaration-no-important': [ { @@ -793,7 +778,7 @@ it('SCSS // disable comment (with // comment after blank line)', () => { .use(assignDisabledRanges) .process(scssSource, { syntax: scss, from: undefined }) .then((result) => { - expect(result.stylelint.disabledRanges).toEqual({ + expectDisableRanges(result, { all: [], 'declaration-no-important': [ { @@ -816,7 +801,7 @@ it('SCSS // disable comment (with // comment immediately after)', () => { .use(assignDisabledRanges) .process(scssSource, { syntax: scss, from: undefined }) .then((result) => { - expect(result.stylelint.disabledRanges).toEqual({ + expectDisableRanges(result, { all: [], 'declaration-no-important': [ { @@ -840,7 +825,7 @@ it('SCSS /* disable comment (with // comment after blank line)', () => { .use(assignDisabledRanges) .process(scssSource, { syntax: scss, from: undefined }) .then((result) => { - expect(result.stylelint.disabledRanges).toEqual({ + expectDisableRanges(result, { all: [], 'declaration-no-important': [ { @@ -863,7 +848,7 @@ it('SCSS // disable comment (with // comment immediately before)', () => { .use(assignDisabledRanges) .process(scssSource, { syntax: scss, from: undefined }) .then((result) => { - expect(result.stylelint.disabledRanges).toEqual({ + expectDisableRanges(result, { all: [], 'declaration-no-important': [ { @@ -886,7 +871,7 @@ it('SCSS two adjacent // disable comments ', () => { .use(assignDisabledRanges) .process(scssSource, { syntax: scss, from: undefined }) .then((result) => { - expect(result.stylelint.disabledRanges).toEqual({ + expectDisableRanges(result, { all: [], 'declaration-no-important': [ { @@ -918,7 +903,7 @@ it('SCSS two adjacent // disable comments with multi-line descriptions ', () => .use(assignDisabledRanges) .process(scssSource, { syntax: scss, from: undefined }) .then((result) => { - expect(result.stylelint.disabledRanges).toEqual({ + expectDisableRanges(result, { all: [], 'declaration-no-important': [ { @@ -950,7 +935,7 @@ it('SCSS two // disable comments with an unrelated comment between them', () => .use(assignDisabledRanges) .process(scssSource, { syntax: scss, from: undefined }) .then((result) => { - expect(result.stylelint.disabledRanges).toEqual({ + expectDisableRanges(result, { all: [], 'declaration-no-important': [ { @@ -977,7 +962,7 @@ it('Less // line-disabling comment (with description)', () => { .use(assignDisabledRanges) .process(lessSource, { syntax: less, from: undefined }) .then((result) => { - expect(result.stylelint.disabledRanges).toEqual({ + expectDisableRanges(result, { all: [], 'declaration-no-important': [ { @@ -1004,7 +989,7 @@ it('Less // disable next-line comment (with multi-line description)', () => { .use(assignDisabledRanges) .process(lessSource, { syntax: less, from: undefined }) .then((result) => { - expect(result.stylelint.disabledRanges).toEqual({ + expectDisableRanges(result, { all: [], 'declaration-no-important': [ { @@ -1019,6 +1004,29 @@ it('Less // disable next-line comment (with multi-line description)', () => { }); }); +function expectDisableRanges(result, expected) { + const actual = result.stylelint.disabledRanges; + + expect(Object.keys(actual)).toEqual(Object.keys(expected)); + + for (const [name, expectedRanges] of Object.entries(expected)) { + const actualRanges = actual[name]; + + expect(actualRanges).toHaveLength(expectedRanges.length); + + for (let i = 0; i < expectedRanges.length; i++) { + expectDisableRange(actualRanges[i], expectedRanges[i]); + } + } +} + +function expectDisableRange(actual, expected) { + const actualMutable = _.cloneDeep(actual); + + delete actualMutable.comment; + expect(actualMutable).toEqual(expected); +} + function testDisableRanges(source, cb) { return postcss().use(assignDisabledRanges).process(source, { from: undefined }).then(cb); } diff --git a/lib/__tests__/invalidScopeDisables.test.js b/lib/__tests__/invalidScopeDisables.test.js index f3a06bc03a..ffd888b73b 100644 --- a/lib/__tests__/invalidScopeDisables.test.js +++ b/lib/__tests__/invalidScopeDisables.test.js @@ -1,6 +1,5 @@ 'use strict'; -const invalidScopeDisables = require('../invalidScopeDisables'); const path = require('path'); const replaceBackslashes = require('../testUtils/replaceBackslashes'); const standalone = require('../standalone'); @@ -10,10 +9,6 @@ function fixture(name) { return replaceBackslashes(path.join(__dirname, './fixtures/disableOptions', name)); } -function source(name) { - return path.join(__dirname, './fixtures/disableOptions', name); -} - it('invalidScopeDisables simple case', () => { const config = { rules: { @@ -34,14 +29,27 @@ it('invalidScopeDisables simple case', () => { return standalone({ config, code: css, + reportInvalidScopeDisables: true, }).then((linted) => { - const report = invalidScopeDisables(linted.results, config); + const results = linted.results; - expect(report).toHaveLength(1); - expect(report[0].ranges).toHaveLength(2); - expect(report[0].ranges).toEqual([ - { end: 3, start: 1, rule: 'block-no-empty', unusedRule: 'block-no-empty' }, - { end: 5, start: 5, rule: 'block-no-empty', unusedRule: 'block-no-empty' }, + expect(results).toHaveLength(1); + + expect(invalidScopeDisables(results[0].warnings)).toEqual([ + { + line: 1, + column: 1, + rule: '--report-invalid-scope-disables', + text: 'Rule "block-no-empty" isn\'t enabled', + severity: 'error', + }, + { + line: 5, + column: 8, + rule: '--report-invalid-scope-disables', + text: 'Rule "block-no-empty" isn\'t enabled', + severity: 'error', + }, ]); }); }); @@ -61,17 +69,31 @@ it('invalidScopeDisables complex case', () => { // ignore files contain `CssSyntaxError` fixture('disabled-ranges-3.css'), ], + reportInvalidScopeDisables: true, }).then((linted) => { - expect(invalidScopeDisables(linted.results, config)).toEqual([ + const results = linted.results; + + expect(results).toHaveLength(3); + + expect(invalidScopeDisables(results[0].warnings)).toEqual([ { - source: source('disabled-ranges-1.css'), - ranges: [{ start: 1, end: 3, rule: 'color-named', unusedRule: 'color-named' }], + line: 1, + column: 1, + rule: '--report-invalid-scope-disables', + text: 'Rule "color-named" isn\'t enabled', + severity: 'error', }, + ]); + expect(invalidScopeDisables(results[1].warnings)).toEqual([ { - source: source('disabled-ranges-2.css'), - ranges: [{ start: 5, end: 5, rule: 'color-named', unusedRule: 'color-named' }], + line: 5, + column: 6, + rule: '--report-invalid-scope-disables', + text: 'Rule "color-named" isn\'t enabled', + severity: 'error', }, ]); + expect(invalidScopeDisables(results[2].warnings)).toHaveLength(0); }); }); @@ -87,11 +109,19 @@ it('invalidScopeDisables ignored case', () => { files: [fixture('disabled-ranges-1.css'), fixture('ignored-file.css')], ignoreDisables: true, ignorePath: fixture('.stylelintignore'), + reportInvalidScopeDisables: true, }).then((linted) => { - expect(invalidScopeDisables(linted.results, config)).toEqual([ + const results = linted.results; + + expect(results).toHaveLength(1); + + expect(invalidScopeDisables(results[0].warnings)).toEqual([ { - source: source('disabled-ranges-1.css'), - ranges: [{ start: 5, end: 7, rule: 'block-no-empty', unusedRule: 'block-no-empty' }], + line: 5, + column: 1, + rule: '--report-invalid-scope-disables', + text: 'Rule "block-no-empty" isn\'t enabled', + severity: 'error', }, ]); }); @@ -104,8 +134,12 @@ it('invalidScopeDisables without config', () => { }, code: 'a {}', ignoreDisables: true, + reportInvalidScopeDisables: true, }).then((linted) => { - expect(invalidScopeDisables(linted.results)).toEqual([]); + const results = linted.results; + + expect(results).toHaveLength(1); + expect(invalidScopeDisables(results[0].warnings)).toHaveLength(0); }); }); @@ -114,14 +148,22 @@ it('invalidScopeDisables for config file', () => { files: [fixture('file-config/disabled-ranges-1.css')], reportInvalidScopeDisables: true, }).then((linted) => { - expect(linted.invalidScopeDisables).toHaveLength(1); - expect(linted.invalidScopeDisables[0].ranges).toEqual([ + const results = linted.results; + + expect(results).toHaveLength(1); + + expect(invalidScopeDisables(results[0].warnings)).toEqual([ { - end: undefined, - start: 4, - rule: 'foo', - unusedRule: 'foo', + line: 4, + column: 1, + rule: '--report-invalid-scope-disables', + text: 'Rule "foo" isn\'t enabled', + severity: 'error', }, ]); }); }); + +function invalidScopeDisables(warnings) { + return warnings.filter((warning) => warning.rule === '--report-invalid-scope-disables'); +} diff --git a/lib/__tests__/needlessDisables.test.js b/lib/__tests__/needlessDisables.test.js index 2a0b7380e4..dc222bb624 100644 --- a/lib/__tests__/needlessDisables.test.js +++ b/lib/__tests__/needlessDisables.test.js @@ -5,10 +5,6 @@ const replaceBackslashes = require('../testUtils/replaceBackslashes'); const standalone = require('../standalone'); const stripIndent = require('common-tags').stripIndent; -function source(name) { - return path.join(__dirname, './fixtures/disableOptions', name); -} - function fixture(name) { return replaceBackslashes(path.join(__dirname, './fixtures/disableOptions', name)); } @@ -38,12 +34,58 @@ it('needlessDisables simple case', () => { code: css, reportNeedlessDisables: true, }).then((linted) => { - const report = linted.needlessDisables; + const results = linted.results; + + expect(results).toHaveLength(1); + const warnings = results[0].warnings; - expect(report).toHaveLength(1); - expect(report[0].ranges).toEqual([ - { start: 7, end: 9, rule: 'all', unusedRule: 'all' }, - { start: 11, end: 11, rule: 'block-no-empty', unusedRule: 'block-no-empty' }, + expect(warnings).toEqual([ + { + line: 7, + column: 1, + text: 'Needless disable for "all"', + rule: '--report-needless-disables', + severity: 'error', + }, + { + line: 11, + column: 22, + text: 'Needless disable for "block-no-empty"', + rule: '--report-needless-disables', + severity: 'error', + }, + ]); + }); +}); + +it('needlessDisables with multiple rules', () => { + const config = { + rules: { 'block-no-empty': true, 'color-named': true }, + }; + + const css = stripIndent` + /* stylelint-disable-next-line block-no-empty, color-named */ + a {} + `; + + return standalone({ + config, + code: css, + reportNeedlessDisables: true, + }).then((linted) => { + const results = linted.results; + + expect(results).toHaveLength(1); + const warnings = results[0].warnings; + + expect(warnings).toEqual([ + { + line: 1, + column: 1, + text: 'Needless disable for "color-named"', + rule: '--report-needless-disables', + severity: 'error', + }, ]); }); }); @@ -66,24 +108,51 @@ it('needlessDisables complex case', () => { ], reportNeedlessDisables: true, }).then((linted) => { - expect(linted.needlessDisables).toEqual([ + const results = linted.results; + + expect(results).toHaveLength(3); + expect(needlessDisables(results[0].warnings)).toEqual([ + { + line: 1, + column: 1, + text: 'Needless disable for "color-named"', + rule: '--report-needless-disables', + severity: 'error', + }, + { + line: 5, + column: 1, + text: 'Needless disable for "block-no-empty"', + rule: '--report-needless-disables', + severity: 'error', + }, + ]); + + expect(needlessDisables(results[1].warnings)).toEqual([ { - source: source('disabled-ranges-1.css'), - ranges: [ - { 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' }, - ], + line: 6, + column: 19, + text: 'Needless disable for "block-no-empty"', + rule: '--report-needless-disables', + severity: 'error', }, { - source: source('disabled-ranges-2.css'), - ranges: [ - { 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' }, - ], + line: 8, + column: 1, + text: 'Needless disable for "block-no-empty"', + rule: '--report-needless-disables', + severity: 'error', + }, + { + line: 5, + column: 6, + text: 'Needless disable for "color-named"', + rule: '--report-needless-disables', + severity: 'error', }, ]); + + expect(needlessDisables(results[2].warnings)).toHaveLength(0); }); }); @@ -100,15 +169,37 @@ it('needlessDisables ignored case', () => { reportNeedlessDisables: true, ignorePath: fixture('.stylelintignore'), }).then((linted) => { - expect(linted.needlessDisables).toEqual([ + const results = linted.results; + + expect(results).toHaveLength(1); + const warnings = results[0].warnings; + + expect(needlessDisables(warnings)).toEqual([ { - source: source('disabled-ranges-1.css'), - ranges: [ - { 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' }, - ], + line: 10, + column: 19, + text: 'Needless disable for "all"', + rule: '--report-needless-disables', + severity: 'error', + }, + { + line: 1, + column: 1, + text: 'Needless disable for "color-named"', + rule: '--report-needless-disables', + severity: 'error', + }, + { + line: 5, + column: 1, + text: 'Needless disable for "block-no-empty"', + rule: '--report-needless-disables', + severity: 'error', }, ]); }); }); + +function needlessDisables(warnings) { + return warnings.filter((warning) => warning.rule === '--report-needless-disables'); +} diff --git a/lib/__tests__/reportDisables.test.js b/lib/__tests__/reportDisables.test.js index c04d529c19..af682574c6 100644 --- a/lib/__tests__/reportDisables.test.js +++ b/lib/__tests__/reportDisables.test.js @@ -19,17 +19,27 @@ describe('reportDisables', () => { `; 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' }, + const results = linted.results; + + expect(results).toHaveLength(1); + const warnings = results[0].warnings; + + expect(warnings).toEqual([ + { + line: 1, + column: 1, + text: 'Rule "block-no-empty" may not be disabled', + rule: 'reportDisables', + severity: 'error', + }, + { + line: 5, + column: 8, + text: 'Rule "block-no-empty" may not be disabled', + rule: 'reportDisables', + severity: 'error', + }, ]); - - // 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); }); }); @@ -52,15 +62,27 @@ describe('reportDisables', () => { 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' }, + const results = linted.results; + + expect(results).toHaveLength(1); + const warnings = results[0].warnings; + + expect(warnings.filter((warning) => warning.rule === 'reportDisables')).toEqual([ + { + line: 1, + column: 1, + text: 'Rule "block-no-empty" may not be disabled', + rule: 'reportDisables', + severity: 'error', + }, + { + line: 5, + column: 8, + text: 'Rule "block-no-empty" may not be disabled', + rule: 'reportDisables', + severity: 'error', + }, ]); - - expect(linted.results[0].warnings).toHaveLength(2); }); }); @@ -79,9 +101,10 @@ describe('reportDisables', () => { `; return standalone({ config, code: css }).then((linted) => { - const report = linted.reportedDisables; + const results = linted.results; - expect(report).toHaveLength(0); + expect(results).toHaveLength(1); + expect(results[0].warnings).toHaveLength(0); }); }); @@ -96,10 +119,10 @@ describe('reportDisables', () => { `; return standalone({ config, code: css }).then((linted) => { - const report = linted.reportedDisables; + const results = linted.results; - expect(report).toHaveLength(1); - expect(report[0].ranges).toHaveLength(0); + expect(results).toHaveLength(1); + expect(results[0].warnings).toHaveLength(0); }); }); }); diff --git a/lib/__tests__/standalone-formatter.test.js b/lib/__tests__/standalone-formatter.test.js index 31cf06f66e..19ab9dfef2 100644 --- a/lib/__tests__/standalone-formatter.test.js +++ b/lib/__tests__/standalone-formatter.test.js @@ -50,53 +50,3 @@ it('standalone with invalid formatter option', () => { ); }); }); - -it('standalone formatter receives {needlessDisables} as second argument', () => { - const formatter = jest.fn((results, { needlessDisables }) => { - return JSON.stringify({ results, needlessDisables }, null, 2); - }); - - return standalone({ - code: ` - /* stylelint-disable yo */ - a {} - `, - config: configBlockNoEmpty, - reportNeedlessDisables: true, - formatter, - }).then((linted) => { - const { output, results, needlessDisables } = linted; - - expect(needlessDisables).not.toBeUndefined(); - expect(needlessDisables).toHaveLength(1); - - expect(typeof output).toBe('string'); - expect(formatter).toHaveBeenCalledTimes(1); - expect(output).toBe(JSON.stringify({ results, needlessDisables }, null, 2)); - }); -}); - -it('standalone formatter receives {invalidScopeDisables} as second argument', () => { - const formatter = jest.fn((results, { invalidScopeDisables }) => { - return JSON.stringify({ results, invalidScopeDisables }, null, 2); - }); - - return standalone({ - code: ` - /* stylelint-disable indentation */ - a { color: red; } - `, - config: configBlockNoEmpty, - reportInvalidScopeDisables: true, - formatter, - }).then((linted) => { - const { output, results, invalidScopeDisables } = linted; - - expect(invalidScopeDisables).not.toBeUndefined(); - expect(invalidScopeDisables).toHaveLength(1); - - expect(typeof output).toBe('string'); - expect(formatter).toHaveBeenCalledTimes(1); - expect(output).toBe(JSON.stringify({ results, invalidScopeDisables }, null, 2)); - }); -}); diff --git a/lib/__tests__/standalone-invalidScopeDisables.test.js b/lib/__tests__/standalone-invalidScopeDisables.test.js deleted file mode 100644 index 220baff965..0000000000 --- a/lib/__tests__/standalone-invalidScopeDisables.test.js +++ /dev/null @@ -1,53 +0,0 @@ -'use strict'; - -const path = require('path'); -const replaceBackslashes = require('../testUtils/replaceBackslashes'); -const standalone = require('../standalone'); - -const fixturesPath = replaceBackslashes(path.join(__dirname, 'fixtures')); -const config = { - quiet: true, - rules: { - 'block-no-empty': true, - }, -}; - -it('standalone with input css and `reportInvalidScopeDisables`', () => { - return standalone({ - code: '/* stylelint-disable color-named */\na {}', - config, - reportInvalidScopeDisables: true, - }).then((linted) => { - const invalidScopeDisables = linted.invalidScopeDisables; - - expect(typeof invalidScopeDisables).toBe('object'); - expect(invalidScopeDisables).toHaveLength(1); - expect(invalidScopeDisables[0].ranges).toHaveLength(1); - expect(invalidScopeDisables[0].ranges[0]).toEqual({ - start: 1, - rule: 'color-named', - unusedRule: 'color-named', - }); - }); -}); - -it('standalone with input file(s) and `reportInvalidScopeDisables`', () => { - return standalone({ - files: replaceBackslashes(path.join(fixturesPath, 'empty-block-with-disables.css')), - config, - reportInvalidScopeDisables: true, - }).then((linted) => { - const invalidScopeDisables = linted.invalidScopeDisables; - - expect(typeof invalidScopeDisables).toBe('object'); - expect(invalidScopeDisables).toHaveLength(1); - expect(invalidScopeDisables[0].source).toBe( - path.join(fixturesPath, 'empty-block-with-disables.css'), - ); - 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 deleted file mode 100644 index b3559e463a..0000000000 --- a/lib/__tests__/standalone-needlessDisables.test.js +++ /dev/null @@ -1,138 +0,0 @@ -'use strict'; - -const path = require('path'); -const replaceBackslashes = require('../testUtils/replaceBackslashes'); -const standalone = require('../standalone'); - -const fixturesPath = replaceBackslashes(path.join(__dirname, 'fixtures')); - -it('standalone with input css and `reportNeedlessDisables`', () => { - const config = { - quiet: true, - rules: { - 'block-no-empty': true, - 'color-named': 'never', - }, - }; - - return standalone({ - code: '/* stylelint-disable color-named */\na {}', - config, - reportNeedlessDisables: true, - }).then((linted) => { - const needlessDisables = linted.needlessDisables; - - expect(typeof needlessDisables).toBe('object'); - expect(needlessDisables).toHaveLength(1); - expect(needlessDisables[0].ranges).toHaveLength(1); - expect(needlessDisables[0].ranges[0]).toEqual({ - start: 1, - rule: 'color-named', - unusedRule: 'color-named', - }); - - expect(linted.results).toHaveLength(1); - const warnings = linted.results[0].warnings; - - expect(typeof warnings).toBe('object'); - expect(warnings).toHaveLength(1); - expect(warnings[0]).toEqual({ - line: 2, - column: 3, - rule: 'block-no-empty', - severity: 'error', - text: 'Unexpected empty block (block-no-empty)', - }); - }); -}); - -it('standalone with `reportNeedlessDisables` and correctly `stylelint-disable`', () => { - const config = { - quiet: true, - rules: { - 'color-named': 'never', - }, - }; - - return standalone({ - code: '/* stylelint-disable color-named */\na { color: black; }', - config, - reportNeedlessDisables: true, - }).then((linted) => { - const needlessDisables = linted.needlessDisables; - - expect(typeof needlessDisables).toBe('object'); - expect(needlessDisables).toHaveLength(1); - expect(needlessDisables[0].ranges).toHaveLength(0); - - expect(linted.results).toHaveLength(1); - const warnings = linted.results[0].warnings; - - expect(typeof warnings).toBe('object'); - expect(warnings).toHaveLength(0); - }); -}); - -it('standalone with `reportNeedlessDisables` and `ignoreDisables`', () => { - const config = { - quiet: true, - rules: { - 'color-named': 'never', - }, - }; - - return standalone({ - code: '/* stylelint-disable color-named */\na { color: black; }', - config, - reportNeedlessDisables: true, - ignoreDisables: true, - }).then((linted) => { - const needlessDisables = linted.needlessDisables; - - expect(typeof needlessDisables).toBe('object'); - expect(needlessDisables).toHaveLength(1); - expect(needlessDisables[0].ranges).toHaveLength(0); - - expect(linted.results).toHaveLength(1); - const warnings = linted.results[0].warnings; - - expect(typeof warnings).toBe('object'); - expect(warnings).toHaveLength(1); - expect(warnings[0]).toEqual({ - line: 2, - column: 12, - rule: 'color-named', - severity: 'error', - text: 'Unexpected named color "black" (color-named)', - }); - }); -}); - -it('standalone with input file(s) and `reportNeedlessDisables`', () => { - const config = { - quiet: true, - rules: { - 'block-no-empty': true, - 'color-named': 'never', - }, - }; - - return standalone({ - files: replaceBackslashes(path.join(fixturesPath, 'empty-block-with-disables.css')), - config, - reportNeedlessDisables: true, - }).then((linted) => { - const needlessDisables = linted.needlessDisables; - - expect(typeof needlessDisables).toBe('object'); - expect(needlessDisables).toHaveLength(1); - expect(needlessDisables[0].source).toBe( - path.join(fixturesPath, 'empty-block-with-disables.css'), - ); - expect(needlessDisables[0].ranges[0]).toEqual({ - start: 1, - rule: 'color-named', - unusedRule: 'color-named', - }); - }); -}); diff --git a/lib/assignDisabledRanges.js b/lib/assignDisabledRanges.js index 0ba45742b1..5e2f23ef79 100644 --- a/lib/assignDisabledRanges.js +++ b/lib/assignDisabledRanges.js @@ -16,6 +16,7 @@ const ALL_RULES = 'all'; /** @typedef {import('stylelint').DisabledRange} DisabledRange */ /** + * @param {PostcssComment} comment * @param {number} start * @param {boolean} strictStart * @param {string|undefined} description @@ -23,8 +24,9 @@ const ALL_RULES = 'all'; * @param {boolean} [strictEnd] * @returns {DisabledRange} */ -function createDisableRange(start, strictStart, description, end, strictEnd) { +function createDisableRange(comment, start, strictStart, description, end, strictEnd) { return { + comment, start, end: end || undefined, strictStart, @@ -142,7 +144,7 @@ module.exports = function (root, result) { const description = getDescription(comment.text); getCommandRules(disableLineCommand, comment.text).forEach((ruleName) => { - disableLine(line, ruleName, comment, description); + disableLine(comment, line, ruleName, description); }); } } @@ -156,18 +158,18 @@ module.exports = function (root, result) { const description = getDescription(comment.text); getCommandRules(disableNextLineCommand, comment.text).forEach((ruleName) => { - disableLine(line + 1, ruleName, comment, description); + disableLine(comment, line + 1, ruleName, description); }); } } /** + * @param {PostcssComment} comment * @param {number} line * @param {string} ruleName - * @param {PostcssComment} comment * @param {string|undefined} description */ - function disableLine(line, ruleName, comment, description) { + function disableLine(comment, line, ruleName, description) { if (ruleIsDisabled(ALL_RULES)) { throw comment.error('All rules have already been disabled', { plugin: 'stylelint', @@ -180,7 +182,7 @@ module.exports = function (root, result) { const strict = disabledRuleName === ALL_RULES; - startDisabledRange(line, disabledRuleName, strict, description); + startDisabledRange(comment, line, disabledRuleName, strict, description); endDisabledRange(line, disabledRuleName, strict); }); } else { @@ -190,7 +192,7 @@ module.exports = function (root, result) { }); } - startDisabledRange(line, ruleName, true, description); + startDisabledRange(comment, line, ruleName, true, description); endDisabledRange(line, ruleName, true); } } @@ -220,10 +222,10 @@ module.exports = function (root, result) { if (isAllRules) { Object.keys(disabledRanges).forEach((ruleName) => { - startDisabledRange(line, ruleName, ruleName === ALL_RULES, description); + startDisabledRange(comment, line, ruleName, ruleName === ALL_RULES, description); }); } else { - startDisabledRange(line, ruleToDisable, true, description); + startDisabledRange(comment, line, ruleToDisable, true, description); } } }); @@ -264,7 +266,7 @@ module.exports = function (root, result) { // Get a starting point from the where all rules were disabled if (!disabledRanges[ruleToEnable]) { disabledRanges[ruleToEnable] = disabledRanges.all.map(({ start, end, description }) => - createDisableRange(start, false, description, end, false), + createDisableRange(comment, start, false, description, end, false), ); } else { const range = _.last(disabledRanges[ALL_RULES]); @@ -348,13 +350,14 @@ module.exports = function (root, result) { } /** + * @param {PostcssComment} comment * @param {number} line * @param {string} ruleName * @param {boolean} strict * @param {string|undefined} description */ - function startDisabledRange(line, ruleName, strict, description) { - const rangeObj = createDisableRange(line, strict, description); + function startDisabledRange(comment, line, ruleName, strict, description) { + const rangeObj = createDisableRange(comment, line, strict, description); ensureRuleRanges(ruleName); disabledRanges[ruleName].push(rangeObj); @@ -382,8 +385,8 @@ module.exports = function (root, result) { */ function ensureRuleRanges(ruleName) { if (!disabledRanges[ruleName]) { - disabledRanges[ruleName] = disabledRanges.all.map(({ start, end, description }) => - createDisableRange(start, false, description, end, false), + disabledRanges[ruleName] = disabledRanges.all.map(({ comment, start, end, description }) => + createDisableRange(comment, start, false, description, end, false), ); } } diff --git a/lib/descriptionlessDisables.js b/lib/descriptionlessDisables.js index 6bfb634cce..545722d7b1 100644 --- a/lib/descriptionlessDisables.js +++ b/lib/descriptionlessDisables.js @@ -1,17 +1,14 @@ 'use strict'; +/** @typedef {import('postcss/lib/comment')} PostcssComment */ /** @typedef {import('stylelint').RangeType} RangeType */ /** @typedef {import('stylelint').DisableReportRange} DisableReportRange */ /** @typedef {import('stylelint').StylelintDisableOptionsReport} StylelintDisableOptionsReport */ /** * @param {import('stylelint').StylelintResult[]} results - * @returns {StylelintDisableOptionsReport} */ module.exports = function (results) { - /** @type {StylelintDisableOptionsReport} */ - const report = []; - results.forEach((result) => { // File with `CssSyntaxError` have not `_postcssResult` if (!result._postcssResult) { @@ -20,33 +17,29 @@ module.exports = function (results) { const rangeData = result._postcssResult.stylelint.disabledRanges; - /** @type {import('stylelint').StylelintDisableReportEntry} */ - const entry = { source: result.source, ranges: [] }; + /** @type {Set} */ + const alreadyReported = new Set(); Object.keys(rangeData).forEach((rule) => { rangeData[rule].forEach((range) => { if (range.description) return; - // Avoid duplicates from stylelint-disable comments with multiple rules. - const alreadyReported = entry.ranges.find((existing) => { - return existing.start === range.start && existing.end === range.end; - }); + if (alreadyReported.has(range.comment)) return; + + alreadyReported.add(range.comment); - if (alreadyReported) return; + // 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) return; - entry.ranges.push({ - rule, - start: range.start, - end: range.end, - unusedRule: rule, + 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, + severity: 'error', }); }); }); - - if (entry.ranges.length > 0) { - report.push(entry); - } }); - - return report; }; diff --git a/lib/invalidScopeDisables.js b/lib/invalidScopeDisables.js index 1249284cb0..401e2fd42f 100644 --- a/lib/invalidScopeDisables.js +++ b/lib/invalidScopeDisables.js @@ -1,16 +1,11 @@ 'use strict'; /** @typedef {import('stylelint').RangeType} RangeType */ -/** @typedef {import('stylelint').StylelintDisableOptionsReport} StylelintDisableOptionsReport */ /** * @param {import('stylelint').StylelintResult[]} results - * @returns {StylelintDisableOptionsReport} */ module.exports = function (results) { - /** @type {StylelintDisableOptionsReport} */ - const report = []; - results.forEach((result) => { // File with `CssSyntaxError` have not `_postcssResult` if (!result._postcssResult) { @@ -28,8 +23,6 @@ module.exports = function (results) { usedRules.add('all'); - /** @type {import('stylelint').StylelintDisableReportEntry} */ - const sourceReport = { source: result.source, ranges: [] }; const rangeData = result._postcssResult.stylelint.disabledRanges; const disabledRules = Object.keys(rangeData); @@ -43,19 +36,18 @@ module.exports = function (results) { return; } - sourceReport.ranges.push({ - rule, - start: range.start, - end: range.end, - unusedRule: rule, + // 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) return; + + 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, + severity: 'error', }); }); }); - - if (sourceReport.ranges.length > 0) { - report.push(sourceReport); - } }); - - return report; }; diff --git a/lib/needlessDisables.js b/lib/needlessDisables.js index 70eae0f92a..62c7815d15 100644 --- a/lib/needlessDisables.js +++ b/lib/needlessDisables.js @@ -1,29 +1,24 @@ 'use strict'; const _ = require('lodash'); +const putIfAbsent = require('./utils/putIfAbsent'); +/** @typedef {import('postcss/lib/comment')} PostcssComment */ +/** @typedef {import('stylelint').DisabledRange} DisabledRange */ /** @typedef {import('stylelint').RangeType} RangeType */ /** @typedef {import('stylelint').DisableReportRange} DisableReportRange */ -/** @typedef {import('stylelint').StylelintDisableOptionsReport} StylelintDisableOptionsReport */ /** * @param {import('stylelint').StylelintResult[]} results - * @returns {StylelintDisableOptionsReport} */ module.exports = function (results) { - /** @type {StylelintDisableOptionsReport} */ - const report = []; - results.forEach((result) => { // File with `CssSyntaxError` have not `_postcssResult` if (!result._postcssResult) { return; } - /** @type {{ranges: DisableReportRange[], source: string}} */ - const unused = { source: result.source || '', ranges: [] }; - - /** @type {{[ruleName: string]: Array}} */ + /** @type {{[ruleName: string]: Array}} */ const rangeData = _.cloneDeep(result._postcssResult.stylelint.disabledRanges); if (!rangeData) { @@ -32,64 +27,68 @@ module.exports = function (results) { const disabledWarnings = result._postcssResult.stylelint.disabledWarnings || []; - disabledWarnings.forEach((warning) => { - const rule = warning.rule; + // 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) { - // Back to front so we get the *last* range that applies to the warning - for (const range of ruleRanges.reverse()) { + for (const range of ruleRanges) { if (isWarningInRange(warning, range)) { - range.used = true; - - return; + putIfAbsent(usefulDisables, range.comment, () => new Set()).add(rule); } } } - for (const range of rangeData.all.reverse()) { + for (const range of rangeData.all) { if (isWarningInRange(warning, range)) { - range.used = true; - - return; + putIfAbsent(usefulDisables, range.comment, () => new Set()).add(rule); } } - }); + } - Object.keys(rangeData).forEach((rule) => { - rangeData[rule].forEach((range) => { - // Is an equivalent range already marked as unused? - const alreadyMarkedUnused = unused.ranges.find((unusedRange) => { - return unusedRange.start === range.start && unusedRange.end === range.end; - }); + const rangeEntries = Object.entries(rangeData); - // If this range is unused and no equivalent is marked, - // mark this range as unused - if (!range.used && !alreadyMarkedUnused) { - unused.ranges.push({ - rule, - start: range.start, - end: range.end, - unusedRule: rule, - }); - } + // Get rid of the duplicated ranges for each `all` rule. We only care + // if the entire `all` rule is useful as a whole or not. + for (const range of rangeData.all) { + for (const [rule, ranges] of rangeEntries) { + if (rule === 'all') continue; - // If this range is used but an equivalent has been marked as unused, - // remove that equivalent. This can happen because of the duplication - // of ranges in rule-specific range sets and the "all" range set - if (range.used && alreadyMarkedUnused) { - _.remove(unused.ranges, alreadyMarkedUnused); - } - }); - }); - - unused.ranges = _.sortBy(unused.ranges, ['start', 'end']); + _.remove(ranges, (otherRange) => range.comment === otherRange.comment); + } + } - report.push(unused); + for (const [rule, ranges] of rangeEntries) { + for (const range of ranges) { + const useful = usefulDisables.get(range.comment) || new Set(); + + // 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, becuase it covers all of them. + if (rule === 'all' ? useful.size !== 0 : useful.has(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: `Needless disable for "${rule}"`, + rule: '--report-needless-disables', + line: range.comment.source.start.line, + column: range.comment.source.start.column, + severity: 'error', + }); + } + } }); - - return report; }; /** diff --git a/lib/prepareReturnValue.js b/lib/prepareReturnValue.js index 9319433cb2..a533f5dd7f 100644 --- a/lib/prepareReturnValue.js +++ b/lib/prepareReturnValue.js @@ -25,6 +25,14 @@ function prepareReturnValue(stylelintResults, options, formatter) { maxWarnings, } = options; + reportDisables(stylelintResults); + + if (reportNeedlessDisables) needlessDisables(stylelintResults); + + if (reportInvalidScopeDisables) invalidScopeDisables(stylelintResults); + + if (reportDescriptionlessDisables) descriptionlessDisables(stylelintResults); + const errored = stylelintResults.some( (result) => result.errored || result.parseErrors.length > 0, ); @@ -34,21 +42,9 @@ function prepareReturnValue(stylelintResults, options, formatter) { errored, results: [], output: '', - reportedDisables: reportDisables(stylelintResults), + reportedDisables: [], }; - if (reportNeedlessDisables) { - returnValue.needlessDisables = needlessDisables(stylelintResults); - } - - if (reportInvalidScopeDisables) { - returnValue.invalidScopeDisables = invalidScopeDisables(stylelintResults); - } - - if (reportDescriptionlessDisables) { - returnValue.descriptionlessDisables = descriptionlessDisables(stylelintResults); - } - if (maxWarnings !== undefined) { const foundWarnings = stylelintResults.reduce((count, file) => { return count + file.warnings.length; diff --git a/lib/reportDisables.js b/lib/reportDisables.js index a528fe7640..36ac5a109a 100644 --- a/lib/reportDisables.js +++ b/lib/reportDisables.js @@ -4,28 +4,20 @@ const _ = require('lodash'); /** @typedef {import('stylelint').RangeType} RangeType */ /** @typedef {import('stylelint').DisableReportRange} DisabledRange */ -/** @typedef {import('stylelint').StylelintDisableOptionsReport} StylelintDisableOptionsReport */ /** * Returns a report describing which `results` (if any) contain disabled ranges * for rules that disallow disables via `reportDisables: true`. * * @param {import('stylelint').StylelintResult[]} results - * @returns {StylelintDisableOptionsReport} */ module.exports = function (results) { - /** @type {StylelintDisableOptionsReport} */ - const report = []; - results.forEach((result) => { // File with `CssSyntaxError` don't have `_postcssResult`s. if (!result._postcssResult) { return; } - /** @type {{ranges: DisabledRange[], source: string}} */ - const reported = { source: result.source || '', ranges: [] }; - /** @type {{[ruleName: string]: Array}} */ const rangeData = result._postcssResult.stylelint.disabledRanges; @@ -43,21 +35,20 @@ module.exports = function (results) { rangeData[rule].forEach((range) => { if (!reportDisablesForRule(_.get(config, ['rules', rule], []))) return; - reported.ranges.push({ - rule, - start: range.start, - end: range.end, - unusedRule: rule, + // 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) return; + + result.warnings.push({ + text: `Rule "${rule}" may not be disabled`, + rule: 'reportDisables', + line: range.comment.source.start.line, + column: range.comment.source.start.column, + severity: 'error', }); }); }); - - reported.ranges = _.sortBy(reported.ranges, ['start', 'end']); - - report.push(reported); }); - - return report; }; /** diff --git a/lib/utils/putIfAbsent.js b/lib/utils/putIfAbsent.js new file mode 100644 index 0000000000..aeb9eed5fd --- /dev/null +++ b/lib/utils/putIfAbsent.js @@ -0,0 +1,22 @@ +'use strict'; + +/** + * If `map` already has the given `key`, returns its value. Otherwise, calls + * `callback`, adds the result to `map` at `key`, and then returns it. + * + * @template K + * @template V + * @param {Map} map + * @param {K} key + * @param {() => V} callback + * @returns {V} + */ +module.exports = function (map, key, callback) { + if (map.has(key)) return /** @type {V} */ (map.get(key)); + + const value = callback(); + + map.set(key, value); + + return value; +}; diff --git a/scripts/visual.css b/scripts/visual.css index d8e28debf6..3564d9623d 100644 --- a/scripts/visual.css +++ b/scripts/visual.css @@ -1,3 +1,4 @@ +/* stylelint-disable foo */ .baz { color: #4f; } diff --git a/types/stylelint/index.d.ts b/types/stylelint/index.d.ts index 800abaa67e..e24e2f65f1 100644 --- a/types/stylelint/index.d.ts +++ b/types/stylelint/index.d.ts @@ -1,5 +1,5 @@ declare module 'stylelint' { - import { Result, ResultMessage, Root, Syntax, WarningOptions, Warning } from 'postcss'; + import { Comment, Result, ResultMessage, Root, Syntax, WarningOptions, Warning } from 'postcss'; export type StylelintConfigExtends = string | Array; export type StylelintConfigPlugins = string | Array; @@ -31,6 +31,7 @@ declare module 'stylelint' { export type CosmiconfigResult = { config: StylelintConfig; filepath: string }; export type DisabledRange = { + comment: Comment; start: number; strictStart: boolean; end?: number;