Make --report-needless-disables CLI flag respect stylelint-disable commands

[saiichihashimoto]

Clearly describe the bug

When using --report-needless-disables, it ignores all of my stylelint-disables.

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

all

What CSS is needed to reproduce the bug?

/* stylelint-disable selector-max-type */
/* stylelint-disable selector-max-universal */

*,
::after,
::before {
	box-sizing: border-box;
	padding: 0;
	margin: 0;
}

a {
	color: inherit;
	text-decoration: inherit;
}

What stylelint configuration is needed to reproduce the bug?

e.g.

{
  "rules": {
    "selector-max-type": 0,
    "selector-max-universal": 0
  }
}

Which version of stylelint are you using?

Commit d3a6cb47fc2d5c59e61f6fac618c6121b316b97c (it comes with some --report-needless-disables fix that I need since the last release)

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

stylelint --report-needless-disables

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

The problem applies to both

What did you expect to happen?

For it to disable the rules

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

They were not disabled, receive these errors:

src/index.css
  6:1  ✖  Expected "*" to have no more than 0 universal selectors        selector-max-universal
 14:1  ✖  Don't style html elements. It leads to css specificity issues  selector-max-type

Without the flag, these errors do not appear.

I messed up reporting #4190, it happens even without --fix.

[hudochenkov]

Thank you for clarification!

[vankop]

report-needless-disables runs withignoreDisables = true by design. See https://github.com/stylelint/stylelint/blob/master/lib/lintSource.js#L180

[vankop]

Right now it is required (ignoreDisables = true) to clarify which rules are needless

[saiichihashimoto]

So, just to clarify, there's not a way to run stylelint normally (ie respecting disables) while also reporting disables that are needless?

[alexander-akait]

It is expected, because we will want other CLI arg for this #2291 and #2292, for me multiple flags look like as overloading in configuration, but maybe i am wrong

[hipstersmoothie]

So, just to clarify, there's not a way to run stylelint normally (ie respecting disables) while also reporting disables that are needless?

This is how eslint works. The behavior should probably be matched. I expect it to work like above

[maxmilton]

v11.0.0 was released with the --report-needless-disables CLI flag running both linting and reporting needless disables, which is really great. So now, wouldn't it make sense to respect stylelint-disable directives? Otherwise there's little value in report-needless-disables in real world projects (unless you run stylelint twice).

[hipstersmoothie]

Came back to report the same thing, thought v11 would have the desired behavior but it still ignores my disabled-comments. Right now I have to run stylelint twice like @maxmilton suggested

[jeddy3]

So now, wouldn't it make sense to respect stylelint-disable directives? Otherwise there's little value in report-needless-disables in real world projects (unless you run stylelint twice).

This makes sense.

Labeling as "help wanted" and "enhancement" (rather than future-next milestone) as the change will result in fewer warnings.

[thibaudcolas]

I think I would be keen to implement this, but looking around the code I’m not quite sure where to start – can anyone offer some guidance? This seems like quite a big change if the current implementation relies on the commands being disregarded in order to produce the report.

[jeddy3]

This seems like quite a big change if the current implementation relies on the commands being disregarded in order to produce the report.

Yes, I think it'll require some architectural changes.

The author of the majority of the engine code is no longer active. However, I can provide a potted history of our design decisions.

In the beginning stylelint was solely a PostCSS plugin. We used PostCSS's warn() to attach warnings to nodes within the AST for PostCSS reporters. We then created a report util, that wraps warn(), to support severities and then disable commands. Next we turned stylelint into a standalone tool with a CLI and Node.js API. Around this time, we created our own formatters. We then added the report* options: reportNeedlessDisables and reportInvalidScopeDisables. Lastly, came autofix which I believe is tightly woven into this issue.

Hopefully, this gives you a sense of the parts involved. The set of features grew organically, and only now are we in a position to see the complete picture.

I believe we need to reassess how we report violations. One idea could be changing report so that it doesn't directly attach warnings to nodes within the AST. Violations can be stored in a bespoke data structure instead. Disable comments logic, report* features and autofix can work against this structure. We can then attach the remaining violations to nodes using PostCSS warn() for:

• stylelint formatters
• PostCSS reporters

You're welcome to create a proof of concept pull request to see if this approach is viable. It's just an idea, though. Investigating how ESLint does it is another option. Having someone champion the engine code would be fantastic, and it's an opportunity to make it your own.

[thibaudcolas]

Thank you @jeddy3, since this requires quite in-depth changes your input is invaluable. I’ve had a brief look at how ESLint did it and was able to make some sense of it (I think!). It’s hard to judge which is the best way forward for this in stylelint though. So it does sound like a proof of concept PR is a good plan.

I can’t commit to doing this just because of how much time this is going to take, but will try to at least have another look at this from an architectural standpoint and report back.

[jeddy3]

Thank you @jeddy3, since this requires quite in-depth changes your input is invaluable

No worries.

It’s hard to judge which is the best way forward for this in stylelint though

Yes, I'm not sure how much the underlying architectures differ.

I can’t commit to doing this just because of how much time this is going to take

That's understandable. It's a sizable piece of work. I believe everyone who contributes to stylelint does so in their spare time, so we rarely work on these time-consuming issues. Hopefully, a corporation will back someone to work on them in the future.