From f1fd8ff8c3644f1f37f0cdacc9c75844c9f49b5d Mon Sep 17 00:00:00 2001 From: Natalie Weizenbaum Date: Thu, 24 Sep 2020 17:45:25 -0700 Subject: [PATCH 1/5] Report disables in the same manner as lints Rather than returning disables that are considered "invalid" for various reasons (such as --report-needless-disables) as a separate list of objects that formatters must handle individually, stylelint now converts them into standard lint warnings. This allows formatters to automatically format them the same as normal warnings without any extra disable-specific code. Closes #4896 --- CHANGELOG.md | 2 + docs/developer-guide/formatters.md | 24 -- docs/user-guide/usage/node-api.md | 8 - docs/user-guide/usage/options.md | 4 +- lib/__tests__/cli.test.js | 21 +- lib/__tests__/descriptionlessDisables.test.js | 39 +-- lib/__tests__/disableRanges.test.js | 264 +++++++++--------- lib/__tests__/invalidScopeDisables.test.js | 90 ++++-- lib/__tests__/needlessDisables.test.js | 149 ++++++++-- lib/__tests__/reportDisables.test.js | 69 +++-- lib/__tests__/standalone-formatter.test.js | 50 ---- .../standalone-invalidScopeDisables.test.js | 53 ---- .../standalone-needlessDisables.test.js | 138 --------- lib/assignDisabledRanges.js | 31 +- lib/descriptionlessDisables.js | 37 +-- lib/invalidScopeDisables.js | 28 +- lib/needlessDisables.js | 93 +++--- lib/prepareReturnValue.js | 22 +- lib/reportDisables.js | 29 +- lib/utils/putIfAbsent.js | 22 ++ scripts/visual.css | 1 + types/stylelint/index.d.ts | 3 +- 22 files changed, 535 insertions(+), 642 deletions(-) delete mode 100644 lib/__tests__/standalone-invalidScopeDisables.test.js delete mode 100644 lib/__tests__/standalone-needlessDisables.test.js create mode 100644 lib/utils/putIfAbsent.js diff --git a/CHANGELOG.md b/CHANGELOG.md index bd027795ff..d23b45d630 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,8 @@ 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. - Fixed: use of full file path without converting it to glob ([#4931](https://github.com/stylelint/stylelint/pull/4931)). ## 13.7.1 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/usage/node-api.md b/docs/user-guide/usage/node-api.md index 635b998b0d..055daa10e4 100644 --- a/docs/user-guide/usage/node-api.md +++ b/docs/user-guide/usage/node-api.md @@ -66,14 +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 }`. -### `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 `stylelint-disable ` comment don't exist within the configuration object. - ## 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 c4df930e30..5d9eae8a1e 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 c 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 don' 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 ## `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 cef906a581..e5bfaf36a8 100644 --- a/lib/__tests__/disableRanges.test.js +++ b/lib/__tests__/disableRanges.test.js @@ -1,6 +1,7 @@ 'use strict'; const assignDisabledRanges = require('../assignDisabledRanges'); +const _ = require('lodash'); const less = require('postcss-less'); const postcss = require('postcss'); const scss = require('postcss-scss'); @@ -13,10 +14,11 @@ 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, + strictStart: true, }, ], @@ -30,10 +32,11 @@ it('disable and re-enable', () => { b {} /* stylelint-enable */ .foo {}`).then((result) => { - expect(result.stylelint.disabledRanges).toEqual({ + expectDisableRanges(result, { all: [ { start: 2, + end: 4, strictStart: true, strictEnd: true, @@ -53,16 +56,18 @@ it('disable and re-enable twice', () => { b {} /* stylelint-enable */ .foo {}`).then((result) => { - expect(result.stylelint.disabledRanges).toEqual({ + expectDisableRanges(result, { all: [ { start: 2, + end: 4, strictStart: true, strictEnd: true, }, { start: 6, + end: 8, strictStart: true, strictEnd: true, @@ -75,11 +80,12 @@ 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': [ { start: 1, + strictStart: true, end: undefined, strictEnd: undefined, @@ -91,11 +97,12 @@ 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': [ { start: 1, + strictStart: true, end: undefined, strictEnd: undefined, @@ -116,7 +123,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 +154,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 +181,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 +249,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 +275,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 +288,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 +320,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 +343,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 +364,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 +407,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 +438,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 +473,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 +514,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 +523,14 @@ it('nested ranges another mix', () => { strictEnd: true, }, ], + bar: [ + { + start: 1, + end: 2, + strictStart: false, + strictEnd: true, + }, + ], foo: [ { start: 1, @@ -548,14 +545,6 @@ it('nested ranges another mix', () => { strictEnd: false, }, ], - bar: [ - { - start: 1, - end: 2, - strictStart: false, - strictEnd: true, - }, - ], }); }); }); @@ -648,7 +637,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 +655,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 +672,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 +692,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 +728,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 +755,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 +782,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': [ { @@ -817,7 +806,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': [ { @@ -838,7 +827,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': [ { @@ -865,7 +854,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': [ { @@ -880,6 +869,25 @@ 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..4102e00b70 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'); @@ -34,14 +33,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(results).toHaveLength(1); - 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(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 +73,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 +113,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 +138,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 +152,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 d57043e059..6c6db33b8d 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, @@ -117,7 +119,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); }); } } @@ -131,18 +133,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', @@ -155,7 +157,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 { @@ -165,7 +167,7 @@ module.exports = function (root, result) { }); } - startDisabledRange(line, ruleName, true, description); + startDisabledRange(comment, line, ruleName, true, description); endDisabledRange(line, ruleName, true); } } @@ -195,10 +197,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); } } }); @@ -239,7 +241,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]); @@ -323,13 +325,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); @@ -357,8 +360,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..f188928cce 100644 --- a/lib/needlessDisables.js +++ b/lib/needlessDisables.js @@ -1,30 +1,25 @@ '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}} */ - const rangeData = _.cloneDeep(result._postcssResult.stylelint.disabledRanges); + /** @type {{[ruleName: string]: Array}} */ + const rangeData = result._postcssResult.stylelint.disabledRanges; if (!rangeData) { return; @@ -32,64 +27,66 @@ module.exports = function (results) { const disabledWarnings = result._postcssResult.stylelint.disabledWarnings || []; + // 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(); + disabledWarnings.forEach((warning) => { 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; - }); + // 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 Object.entries(rangeData)) { + if (rule === 'all') continue; - // 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, - }); - } + _.remove(ranges, (otherRange) => range.comment === otherRange.comment); + } + } - // 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); - } + Object.entries(rangeData).forEach(([rule, ranges]) => { + ranges.forEach((range) => { + 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)) 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; + + 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', + }); }); }); - - unused.ranges = _.sortBy(unused.ranges, ['start', 'end']); - - report.push(unused); }); - - 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; From 56e82862e8b64d422deadcac39a20c5500ae8567 Mon Sep 17 00:00:00 2001 From: Natalie Weizenbaum Date: Fri, 9 Oct 2020 13:05:03 -0700 Subject: [PATCH 2/5] Fix merge conflict test failures --- lib/__tests__/disableRanges.test.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/__tests__/disableRanges.test.js b/lib/__tests__/disableRanges.test.js index 39629dea3b..b72ff8e94b 100644 --- a/lib/__tests__/disableRanges.test.js +++ b/lib/__tests__/disableRanges.test.js @@ -807,7 +807,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': [ { @@ -854,7 +854,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': [ { @@ -877,7 +877,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': [ { @@ -909,7 +909,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': [ { @@ -941,7 +941,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': [ { From f8fd5c160336a9a95b45be9b91536eca791d89e9 Mon Sep 17 00:00:00 2001 From: Natalie Weizenbaum Date: Fri, 9 Oct 2020 13:16:23 -0700 Subject: [PATCH 3/5] Remove extra blank lines --- lib/__tests__/disableRanges.test.js | 6 ------ 1 file changed, 6 deletions(-) diff --git a/lib/__tests__/disableRanges.test.js b/lib/__tests__/disableRanges.test.js index b72ff8e94b..d3c9aea560 100644 --- a/lib/__tests__/disableRanges.test.js +++ b/lib/__tests__/disableRanges.test.js @@ -20,7 +20,6 @@ it('disable without re-enabling', () => { all: [ { start: 1, - strictStart: true, }, ], @@ -38,7 +37,6 @@ it('disable and re-enable', () => { all: [ { start: 2, - end: 4, strictStart: true, strictEnd: true, @@ -62,14 +60,12 @@ it('disable and re-enable twice', () => { all: [ { start: 2, - end: 4, strictStart: true, strictEnd: true, }, { start: 6, - end: 8, strictStart: true, strictEnd: true, @@ -87,7 +83,6 @@ it('disable rule without re-enabling', () => { 'foo-bar': [ { start: 1, - strictStart: true, end: undefined, strictEnd: undefined, @@ -104,7 +99,6 @@ it('disable rule without re-enabling', () => { 'selector-combinator-space-before': [ { start: 1, - strictStart: true, end: undefined, strictEnd: undefined, From b03ef70336b415004a6e5b6749d6211deb5826db Mon Sep 17 00:00:00 2001 From: Natalie Weizenbaum Date: Tue, 13 Oct 2020 14:21:08 -0700 Subject: [PATCH 4/5] Code review --- lib/needlessDisables.js | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/lib/needlessDisables.js b/lib/needlessDisables.js index f188928cce..312304c9c7 100644 --- a/lib/needlessDisables.js +++ b/lib/needlessDisables.js @@ -19,7 +19,7 @@ module.exports = function (results) { } /** @type {{[ruleName: string]: Array}} */ - const rangeData = result._postcssResult.stylelint.disabledRanges; + const rangeData = _.cloneDeep(result._postcssResult.stylelint.disabledRanges); if (!rangeData) { return; @@ -35,7 +35,7 @@ module.exports = function (results) { /** @type {Map>}} */ const usefulDisables = new Map(); - disabledWarnings.forEach((warning) => { + for (const warning of disabledWarnings) { const rule = warning.rule; const ruleRanges = rangeData[rule]; @@ -52,30 +52,31 @@ module.exports = function (results) { putIfAbsent(usefulDisables, range.comment, () => new Set()).add(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. + const rangeEntries = Object.entries(rangeData); for (const range of rangeData.all) { - for (const [rule, ranges] of Object.entries(rangeData)) { + for (const [rule, ranges] of rangeEntries) { if (rule === 'all') continue; _.remove(ranges, (otherRange) => range.comment === otherRange.comment); } } - Object.entries(rangeData).forEach(([rule, ranges]) => { - ranges.forEach((range) => { + 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)) return; + 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) return; + if (!range.comment.source || !range.comment.source.start) continue; result.warnings.push({ text: `Needless disable for "${rule}"`, @@ -84,8 +85,8 @@ module.exports = function (results) { column: range.comment.source.start.column, severity: 'error', }); - }); - }); + } + } }); }; From 0fdef35a3357563c21457b0d0463f1fcb058e512 Mon Sep 17 00:00:00 2001 From: Natalie Weizenbaum Date: Tue, 13 Oct 2020 14:25:33 -0700 Subject: [PATCH 5/5] Fix a lint --- lib/needlessDisables.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/needlessDisables.js b/lib/needlessDisables.js index 312304c9c7..62c7815d15 100644 --- a/lib/needlessDisables.js +++ b/lib/needlessDisables.js @@ -54,9 +54,10 @@ module.exports = function (results) { } } + const rangeEntries = Object.entries(rangeData); + // 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. - const rangeEntries = Object.entries(rangeData); for (const range of rangeData.all) { for (const [rule, ranges] of rangeEntries) { if (rule === 'all') continue;