Fix regression for disable commands and adjacent double-slash comments

[stof]

Clearly describe the bug

This is a follow-up of #4913 which fixed parts of the regression by avoiding to merge double-slash comments separated by empty lines.
There is still a regression when a disabling comment is in a double-shash comment which is not separated from preceding comments with a empty line, as the stylelint-disable is not on the at the beginning of the comment anymore after merging.

Which rule, if any, is the bug related to?

Can be any rule as it is related to disabling comments

What code is needed to reproduce the bug?

See https://github.com/twbs/bootstrap/blob/5c25ea647bdc463df5a322c8248460a78305a1f4/scss/bootstrap-grid.scss#L24-L27 for an example

What stylelint configuration is needed to reproduce the bug?

Which version of stylelint are you using?

e.g. 13.7.1

How are you running stylelint: CLI, PostCSS plugin, Node.js API?

e.g. "CLI with stylelint "**/*.css" --config myconfig.json"

Does the bug relate to non-standard syntax (e.g. SCSS, Less etc.)?

Yes, it's related to SCSS double-slash comments

What did you expect to happen?

rules are properly disabled and re-enabled, as done in Stylelint 13.6

What actually happened (e.g. what warnings or errors did you get)?

Some of these control comments are ignored

[stof]

As comment merging is useful only for descriptions of disable descriptions, I propose the following heuristic:

1. do not merge across empty lines (what Avoid combining disable comments with a blank line #4913 has implemented)
2. do not merge a comment with following one if it does not start with stylelint-. Such comment is not the start of a disable rule and so does not need merging with the next comment line. But this means that the next comment line could still be a comment starting with stylelint-disable. This will perform the merging only when necessary

An alternative to the second rule could be to forbid merging a comment starting with stylelint- into the preceding comment, to keep it as a beginning of comment. The difference is what happens for non-stylelint comments covering multiple lines (in the first case, they are not merged, while they are in the second). AFAICT, this would have no impact on the result for stylelint. It might have an impact on which one is easier to implement, or on which one is easier to upstream to postcss-scss (where the stylelint- line prefix would probably need to be more generic or configurable)

[jeddy3]

@stof Thanks for writing this up.

As comment merging is useful only for descriptions of disable descriptions

We wanted to merge double-slash comment blocks so that the behaviour is consistent with the multiline CSS comments. (It's also how the SCSS parser works, and postcss-scss will likely mimic this behaviour in the future.)

Multiline comments are useful when a user is disabling many rules (as well as now being useful for disable descriptions).

The following has always worked in stylelint:

/*
 stylelint-disable unit-allowed-list, media-feature-name-disallowed-list
 color-no-hex, property-no-unknown
*/

The change was to bring double-slash comment blocks in line with this behaviour:

// stylelint-disable unit-allowed-list, media-feature-name-disallowed-list
// color-no-hex, property-no-unknown

The change would also benefit the new disables description feature:

/*
 stylelint-disable unit-allowed-list, media-feature-name-disallowed-list
 color-no-hex, property-no-unknown
 -- description
*/

// stylelint-disable unit-allowed-list, media-feature-name-disallowed-list
// color-no-hex, property-no-unknown
// -- description

---

In hindsight, it's clear that the merging of double-slash comment blocks was a breaking change.

It changes the behaviour for:

// before
// stylelint-disable color-no-hex
a { color: #fff; }

And:

// stylelint-disable color-no-hex
// after
a { color: #fff; }

Previously both those stylelint disable commands would work but now they don't. Demo.

I'm not sure there's a heuristic we can add to fix the latter example.

What's the normal play when a breaking change is released as a patch? Do we revert the change, release a patch and then prepare for a major release with the change in?

[XhmikosR]

What's the normal play when a breaking change is released as a patch? Do we revert the change, release a patch and then prepare for a major release with the change in?

Usually that's the procedure indeed.

[stof]

my proposal here would fix the regression for the case with text before it (which currently gets silently ignored as not being a stylelint-disable line, which can report extra errors and might break a non-ignored stylelint-enable a few lines later).
It does not change the case of text following it compared to 13.7.1. But this is indeed the expected change to support multiline disable rules. Here, at least, it would report that after is not a valid rule name, which can be fixed either by adding -- to make it a description or by adding an empty line to force splitting the comment.

[stof]

Note that the breaking change was released in a minor, not in a patch. 13.7.1 contains a partial fix for the regression.

[jeddy3]

my proposal here would fix the regression for the case with text before it

Yes, I think your proposal would work for that regression.

Here, at least, it would report that after is not a valid rule name, which can be fixed either by adding -- to make it a description or by adding an empty line to force splitting the comment.

It doesn't report that the rule name is not valid. It silently breaks the disable command, and behaves in the same away as the case with text before.

We could change the behaviour to report it as an invalid name, but that would also be a breaking change and should be a major release.

I can't think of a heuristic that would work around this in a non-breaking way, especially when we consider the report* options like reportInvalidScopeDisables.

Can anyone think of a non-breaking solution to:

// stylelint-disable color-no-hex
// after
a { color: #fff; }

[hudochenkov]

Can anyone think of a non-breaking solution to:

// stylelint-disable color-no-hex
// after
a { color: #fff; }

Following condition should be added:

1. Start new merged comment when reaching `stylelint-disable comment:

   This:

   // before
   // stylelint-disable color-no-hex

   Will be equal:

   /* before */
   /* stylelint-disable color-no-hex */

2. Inline comment starting with stylelint-disable should be merged with next comments only if the former ends with --:

   With current code:

   // stylelint-disable color-no-hex
   // after

   Equals:

   /* stylelint-disable color-no-hex
   after */

   Which is not a valid stylelint-disable comment.

   With new condition following examples would be valid:

   // stylelint-disable color-no-hex --
   // after

   Because it equals to:

   /* stylelint-disable color-no-hex --
   after
   */

It's all according to initial discussion #4886.

---

I noticed that we don't have tests for following cases for -- addition to comments in #4848 and #4895.

/*
stylelint-disable block-opening-brace-space-after
--
after
*/

/*
stylelint-disable block-opening-brace-space-after
-- after
*/

// stylelint-disable block-opening-brace-space-after -- after

// stylelint-disable block-opening-brace-space-after
// -- after

---

The following has always worked in stylelint:

/*
 stylelint-disable unit-allowed-list, media-feature-name-disallowed-list
 color-no-hex, property-no-unknown
*/

I don't think so. Demo.

[hudochenkov]

We wanted to merge double-slash comment blocks so that the behaviour is consistent with the multiline CSS comments. (It's also how the SCSS parser works, and postcss-scss will likely mimic this behaviour in the future.)

It would be unfortunate, because in CSS stylelint-disable should be a separate comment.

We have 3 separate comment:

/* a */
/* stylelint-disable */
/* b */

If we convert them to Sass:

// a
// stylelint-disable
// b

And then back to CSS (virtually, it just easier to understand what grouping of comments would do in postcss-scss):

/* a
stylelint-disable
b */

This is completely different thing, which is not important for Sass compiler, but is important for stylelint, Autoprefixer, Prettier and other tools, which use control comments.

[stof]

2\. Inline comment starting with `stylelint-disable` should be merged with next comments only if the former ends with ` --`:

the rule is more complex than that:

• merging can be triggered either by -- in the first comment (not necessarily at the end) or at the beginning of the second comment
• merging must stop when finding stylelint-disable (actually, when finding stylelint- to account for things like stylelint-enable) at the beginning of the next comment.

This way, merging also works for these cases:

// stylelint-disable block-opening-brace-space-after
// --
// after with
// multiline description

// stylelint-disable block-opening-brace-space-after -- after with
// multiline description

// stylelint-disable block-opening-brace-space-after
// stylelint-enable other-rule

[hudochenkov]

// stylelint-disable block-opening-brace-space-after
// --
// after with

By the way, we have tests for this, but we don't have documentation for this behavior. It's undocumented feature, which we could safely remove, I think.

actually, when finding stylelint- to account for things like stylelint-enable

Good one! I forgot about enable comment :)

[jeddy3]

The following has always worked in stylelint:

/*
 stylelint-disable unit-allowed-list, media-feature-name-disallowed-list
 color-no-hex, property-no-unknown
*/

I don't think so. Demo.

My bad, I missed a comma. Demo of it working.

/*
 stylelint-disable block-opening-brace-space-after,
 color-hex-case
*/
a {color: #FFF; }

---

The intent of the original issue was to add support for multiline descriptions for double slash comments. It did this by merging adjacent comments, which also enabled:

// stylelint-disable block-opening-brace-space-after,
// color-hex-case
a {color: #FFF; }

Demo.

But broke:

// stylelint-disable block-opening-brace-space-after
// after
a {color: #FFF; }

Demo.

---

This is completely different thing, which is not important for Sass compiler, but is important for stylelint, Autoprefixer, Prettier and other tools, which use control comments.

I agree that it's a different behaviour. I also agree with the expectation from the original issue, though:

As a user, though, I'd expect stylelint to treat multiple adjacent inline comments as a single unified comment (which is incidentally how they're exposed in Sass's own AST).

i.e. adjacent double-slash comments are consistently treated as a single unified comment regardless of the content within them.

I think this behaviour is useful for control comments, e.g.:

// stylelint-disable block-opening-brace-space-after,
// color-hex-case, color-no-invalid-hex
// -- hacky code description
a {color: #FF; }

Demo.

Users will need to write:

// a

// stylelint-disable block-opening-brace-space-after,
// color-hex-case, color-no-invalid-hex
// -- hacky code description

// b
a {color: #FF; }

Demo.

I think this is the correct behaviour, and what is currently implemented (if undocumented). However, I understand we probably want to discuss this more to weigh up the pros and cons.

---

I think we have two options:

• revert the change
• add the heuristics described here and here

The outcome of each option is the same:

• the regression will be fixed
• builds will break for anyone relying on the new (undocumented) behaviour

I'm leaning towards the 2nd option as it's the behaviour we described in the changelog: "support for multi-line disable descriptions". If the heuristics prove too tricky to implement, we can fallback on the first option.

Once the patch is released, we can then discuss the merits or not of changing the merging behaviour (in a major release).

Sound good?

@stof @hudochenkov This is quite the gnarly issue to unpick, and I appreciate your time so far unpicking together. Thanks!

[stof]

I agree on trying the 2nd option first, so that most regressions on existing cases are solved while still preserving support for multiline description. And the rules about how to use command comments in scss should be clearly documented (separate them from other comments with an empty line.)

Then, if you want to remove the heuristics in a future major version, I think this would require to trigger some deprecation warning for cases where the heuristics enters into action to prevent merging, so that users can discover these cases (could even be something fixed automatically by adding empty lines).

[hudochenkov]

Second option sound the best for me as well.

@stof could you combine two of your comments (#4937 (comment) and #4937 (comment)) into one set of instructions how it should work. And gather all code examples mentioned here which we need to support.

It would help person, who's going to fix this.

Or you could send pull request with fixed behavior and added tests :)

[jeddy3]

I'll label as ready to implement. If anyone has time to address this, please shout and label the issue as wip.

cc-ing in @jathak as the author of the feature, in case they have time.

[jathak]

I can work on this now. Just to confirm, the desired logic is that merging should only start when a comment starts with stylelint- and should end whenever another comment starting with stylelint- is encountered, right?

Edit: Plus -- must either be somewhere in the first line or at the start of the second. Missed that bit initially

[hudochenkov]

I've wrote comment before you've added more info to your comment, @jathak :)

I think the complete list of rules would be:

• Start merge if comment has stylelint-enable or stylelint-disable at the beginning. I would advice using just stylelint-, because comment could start with stylelint-order (popular stylelint plugin), for example.
• Stop merge:
  • If reached empty line.
  • If reached another stylelint-enable or stylelint-disable comment. Here, probably "start" rule will kick in.
  • If stylelint-enable or stylelint-disable comment doesn't have -- in it, or next comment doesn't have -- at the beginning.

[hudochenkov]

I'll try to release tomorrow.

[jeddy3]

I'll try to release tomorrow.

I'll hopefully have time on Saturday if you don't get around to releasing first.