Fix reported ranges of warnings in selectors

pamelalozano16 commented 1 month ago

What minimal example or steps are needed to reproduce the bug?

What minimal configuration is needed to reproduce the bug?

{
  "extends": [
    "stylelint-config-standard"
  ]
}

How did you run Stylelint?

Demo

Which Stylelint-related dependencies are you using?

{
  "stylelint": "16.8.0",
  "stylelint-config-standard": "latest"
}

What did you expect to happen?

In older versions of Stylelint, using a stylelint-disable-next-line comment before a CSS selector list would disable linting for the entire selector block, even if it spanned multiple lines:

/* stylelint-disable no-invalid-double-slash-comments */
.a, // Comment 1
.b, // Comment 2
{
  display: none;
}

Stylelint demo with v16.2.0

What actually happened?

In the newer version of Stylelint, this behavior has changed. Now, stylelint-disable-next-line only disables linting for the literal next line, not the entire block:

/* stylelint-disable-next-line no-invalid-double-slash-comments */
.a, // Comment 1
.b, // Comment 2 -> Error: Unexpected double-slash CSS comment (no-invalid-double-slash-comments)
{
  display: none; 
}

Demo

Additionally, placing stylelint-disable-next-line within the middle of a selector list doesn't disable the lint for the next line:

/* stylelint-disable-next-line no-invalid-double-slash-comments */
.a, // Comment 1
/* stylelint-disable-next-line no-invalid-double-slash-comments */
.b, // Comment 2 -> Error: Unexpected double-slash CSS comment (no-invalid-double-slash-comments)
{
  display: none; 
}

Demo

Root cause

See: https://github.com/stylelint/stylelint/issues/7904#issuecomment-2275024780

Do you have a proposal to fix the bug?

Update all affected rules so that they report the correct and most relevant positions for warnings.

[x] https://github.com/stylelint/stylelint/pull/7932
[ ] https://github.com/stylelint/stylelint/pull/7992
[ ] no-descending-specificity
[x] https://github.com/stylelint/stylelint/pull/7938
[ ] no-invalid-double-slash-comments
[ ] no-irregular-whitespace
[ ] selector-anb-no-unmatchable
[ ] selector-attribute-name-disallowed-list
[ ] selector-attribute-operator-allowed-list
[ ] selector-attribute-operator-disallowed-list
[ ] selector-attribute-quotes
[ ] selector-class-pattern
[ ] selector-combinator-allowed-list
[ ] selector-combinator-disallowed-list
[ ] selector-disallowed-list
[ ] selector-id-pattern
[ ] selector-max-attribute
[ ] selector-max-class
[ ] selector-max-combinators
[ ] selector-max-compound-selectors
[ ] selector-max-id
[ ] selector-max-pseudo-class
[ ] selector-max-specificity
[x] https://github.com/stylelint/stylelint/pull/7916
[ ] selector-max-universal
[ ] selector-nested-pattern
[x] https://github.com/stylelint/stylelint/pull/7937
[ ] selector-no-vendor-prefix
[ ] selector-not-notation
[ ] selector-pseudo-class-allowed-list
[ ] selector-pseudo-class-disallowed-list
[ ] selector-pseudo-class-no-unknown
[ ] selector-pseudo-element-allowed-list
[ ] selector-pseudo-element-colon-notation
[ ] selector-pseudo-element-disallowed-list
[ ] selector-pseudo-element-no-unknown
[ ] selector-type-case
[ ] selector-type-no-unknown

Mouvedia commented 1 month ago

stylelint-disable-next-line only disables linting for the literal next line, not the entire block

For me, that makes perfect sense. Was the "block" exception covered by a test? i.e. having it disabling lines after the next line would either be a bug or an undocumented feature that you were relying on

Mouvedia commented 1 month ago

@pamelalozano16 your first example uses stylelint-disable not stylelint-disable-next-line.

Here's a demo on the latest version of Stylelint that had that odd behaviour. i.e. 16.6.0

Mouvedia commented 1 month ago

Additionally, placing stylelint-disable-next-line within the middle of a selector list doesn't disable the lint for the next line

This is something that we discussed before:

It could be unexpected hence we will have to add a note/warning in the documentation even if we are not supporting less/sass comments officially anymore.

And we forgot to update the documentation in #7839. i.e. try replacing // with /* … */, it should work fine e.g. demo (of course that example makes no sense now)

The commit that inadvertently fixed the bug was 8f59ebd96.

pamelalozano16 commented 1 month ago

I agree that it should only disable the subsequent line of the comment. Based on that premise, shouldn't these comments disable the lint for both comment 1 and 2?

/* stylelint-disable-next-line no-invalid-double-slash-comments */
.a, // Comment 1
/* stylelint-disable-next-line no-invalid-double-slash-comments */
.b, // Comment 2 -> Error: Unexpected double-slash CSS comment (no-invalid-double-slash-comments)
{
  display: none; 
}

Even though // Comment 2 is a double-slash comment, the /* stylelint-disable-next-line no-invalid-double-slash-comments */ comment above, should be enough to suppress the error, right? Demo

Mouvedia commented 1 month ago

Even though // Comment 2 is a double-slash comment, the /* stylelint-disable-next-line no-invalid-double-slash-comments */ comment above, should be enough to suppress the error, right?

This is a limitation that is known when using //; which is missing from the documentation. i.e. only valid CSS comments are taken into account by configuration comments within selectors There is no regression here; it's a bug fix followed by a feature addition which lacked proper documentation. Still there's some hope for you if you feel strongly about it:

Let's revisit it if people would strongly request //.

BTW //, in CSS, comments out the next construct, so having a // comment within a selector doesn't even make sense for the intended purpose of //. i.e. if you didn't want to use /* … */ and ignore it, the comment would have to look like // Comment 3 {} We might have to add a note about this edge case on the no-invalid-double-slash-comment page though.

romainmenke commented 1 month ago

🤔 I think something else is going on here.

Consider this example:

https://stylelint.io/demo/#N4Igxg9gJgpiBcIA6A7AdAQwDSoPQCoACAMwgkP11TQCMtDddCBhCAWzZhQBdCAmVMFSFCUAJYBnAA4AbDAE94hFBBQwA3KgC+qEFhDExMmADkMnBCBgAPc7JhowEiXvCrDAc0tCUIpFetuLigJfyUAbWERQn8JbnljGTEeAFpIFE8UuIwUKAwAJyh-KIBdbVd0zwAxCHy2DG5LACsJVVdYKRdEHz8QOISYJJ4wmJA5ILj-HF9R-sTk7jT3MQ8s7hy8wpH-cZhJ5BQtEC0gA

.a,
/* foo */
.b, // Comment 2
{
  display: none;
}

Here the error for // Comment 2 is shown on the line with /* foo */.

I think that no-invalid-double-slash-comments is using selector values that do not contain regular CSS comments.

So this rule is seeing :

.a,
.b, // Comment 2
{
  display: none;
}

Notice how .b, // Comment 2 is now on the second line, and not the third. Any warning for .b, // Comment 2 is reported as if it were coming from the second line, which is incorrect.

Such a shift would also break the configuration comments as they compare the position of the comment with the position of the report.

And configuration comments themselves are comments which are apparerntly stripped and causing the issue in the first place.

romainmenke commented 1 month ago

Still there's some hope for you if you feel strongly about it: https://github.com/stylelint/stylelint/issues/3111#issuecomment-2233173700

That linked comment is unrelated :) That comment is about writing configuration commands with // comments, not about how // comments are affected by rules or other configuration commands.

ybiquitous commented 1 month ago

@romainmenke Just to make sure, you think this issue is a bug of no-invalid-double-slash-comments rather than a documentation issue, right?

romainmenke commented 1 month ago

Yes, fairly certain that this is a bug in this rule.

https://github.com/stylelint/stylelint/blob/d29b948cb25946d7644248db2cdea02c44d935bb/lib/rules/no-invalid-double-slash-comments/index.mjs#L37

This seems to work more as I would expect:

- const selectors = ruleNode.selector.split(',');
+ const selectors = (ruleNode.raws?.selector?.raw ?? ruleNode.selector).split(',');

But more testing and analysis might be needed :)

romainmenke commented 1 month ago

(We should definitely also update docs for the configuration commands)

ybiquitous commented 1 month ago

Okay, let's address both in this issue. I'm okay with separating pull requests. 👍🏼

pamelalozano16 commented 1 month ago

What I mean is, if a stylelint-disable-next-line comment is placed between selectors in a selector list, it doesn't work as expected. This happens with several rules, not only no-invalid-double-slash-comments. In this example, I would expect selector-max-type to be disabled for & h2 as well. But instead, it produces an error:

/* stylelint-disable-next-line selector-max-type */
div .c, 
/* stylelint-disable-next-line selector-max-type */
& h2 { /* Error -> Expected " stylelint-disable-next-line selector-max-type & span" to have no more than 0 type selectors */
  color: black;
}

Same problem here:

/* stylelint-disable-next-line selector-max-type */
div 
.c, /* stylelint-disable-next-line selector-max-type */
& h2 { /* Error -> Expected " stylelint-disable-next-line selector-max-type & span" to have no more than 0 type selectors */
  color: black;
}

However, if we put the disable comment inline with the first selector, it works:

/* stylelint-disable-next-line selector-max-type */
div .c, /* stylelint-disable-next-line selector-max-type */
& h2 {
  color: black;
}

See Demo

Due to constraints on maximum line length, placing disable comments on separate lines can be useful.

romainmenke commented 1 month ago

It is definitely possible that multiple rules suffer from the same or very similar issues.

It would be good to do a wide review of rules and make sure that they work as expected in various cases.

It wasn't immediately clear from the original report that multiple rules were affected :)

Mouvedia commented 1 month ago

It was considered off-topic back then. I guess each affected rule should have its own issue and if we have more than 2 we will create a umbrella issue to track them all.

romainmenke commented 1 month ago

I still don't think that this issue is related to that @Mouvedia :)

I don't think that adding support for configuration commands in values and selectors changed anything. I also don't see how it could. What did changed however are those rules themselves.

Stylelint 16.2: https://stylelint.io/demo/#N4Igxg9gJgpiBcID0AqABAZwC4E8A2MeAlgHZYC0URGAhgEYHkkwAeFxzmhMYWEATuQC2NFuVwAHGGhRIAOiSoA3NADowAGjQLUmXAQ4UqtBjCat2paRm68Bw0eJxSZ8kgDI0ACwBMaYApoaJB4AvBoDDRgANYA3AoAviAaIABmRAQAcjRCcIisORIE6hgYyeAQJOkA5gggASRBciAWMIoYzeEA2oFB2iDY+ISkFJBVRNXk2DSKNPxQzb0Auhq9zfwArgQdCP592o19zTYEdoIiYpIwnWgADL0JieVjNQBiAiJYdQBWGJXlsAkZUQDSaA30wzIN2aAEYAGyqHyqe7JNbgoaGcgvCZTLAzKBzBa7Zp4GhYGDYRYkJIJIA

Stylelint 16.3: https://stylelint.io/demo/#N4Igxg9gJgpiBcID0AqABAZwC4E8A2MeAlgHZYC0URGAhgEYHkkwAeFxzmhMYWEATuQC2NFuVwAHGGhRIAOiSoA3NADowAGjQLUmXAQ4UqtBjCat2paRm68Bw0eJxSZ8kgDI0ACwBMaYApoaJB4AvBoDDRgANYA3AoAviAaIABmRAQAcjRCcIisORIE6hgYyeAQJOkA5gggASRBciAWMIoYzeEA2oFB2iDY+ISkFJBVRNXk2DSKNPxQzb0Auhq9zfwArgQdCP592o19zTYEdoIiYpIwnWgADL0JieVjNQBiAiJYdQBWGJXlsAkZUQDSaA30wzIN2aAEYAGyqADMqnuyTW4KGhnILwmUywMygcwWu2aeBoWBg2EWJCSCSAA

And in 16.3 we changed this rule: https://github.com/stylelint/stylelint/blob/main/CHANGELOG.md#1630

Fixed: selector-max-type end positions (#7518) (@romainmenke).

We have been fixing the positions of error reports in many rules and the now different positions require updated configuration commands.

Simultaneously we have also added support for configuration commands inside values and selectors. Making it possible (in theory) to resolve all issues by moving or adding more comments with configuration commands.

It is possible that either the rules have more bugs or the newly added support for configuration commands in values and selectors might have bugs.

And those (still unknown) issues apparently make it impossible to add fine grained configuration commands.

I think we should focus on:

making sure that the newly improved support for configuration commands is actually correct and doesn't have any off by one errors or other issues
doing a wide review of rules to make sure that error reports have the correct positions even when there are any comments

Mouvedia commented 1 month ago

Those cases will have to be found and potentially tests will have to be added. i.e. tests weren't added because we thought it would never change (bad regression coverage) e.g. maybe some rules are stripping comments

and

Currently a rule that strip the comments is removing user comments; it could maybe be argued that they ought to be kept, but it's nothing major. Once these stylelint commands won't be considered bogus, the removal of those comments will have an impact that I don't consider out of scope.

seems related to me. Anyway, Ill try to review the rules in the upcoming weeks. It would be great if all the fixes were to be released in the same version but it's not a must.

romainmenke commented 1 month ago

I am sorry @Mouvedia I can't see how this newly mentioned issue (that can be easily shown to be the direct result of the changes in https://github.com/stylelint/stylelint/pull/7518) is instead caused by something that was only changed months later.

(But as I said in that thread, please open an issue for the concerns you raised there. Then we have a thread to discuss and investigate those concerns specifically. They won't get addressed otherwise.)

I think that assigning the incorrect cause will prevent us from solving these issues swiftly :)

A simplified example:

https://stylelint.io/demo/#N4Igxg9gJgpiBcIB0YA0AdAdgegFQAIBnAFwE8AbGcgS02IFoprCBDAI0vsxgA8GbuRKjDDEIAJ3oBbFj3pkADjHy5sWABYAmfMCz58kchPj4OLMAGsA3FgC+IVCABm1SgDkWUuIl6eFlFEJCB3AITBcAcwQQXUx9dBBeYhhMKEIEkwBtPX18BJIKKloGSHDqCPoSFlSWcSgEnIBdDDi8kHEAV0p0hB1cvNb4kEJhUQlpWXlSJQz8AAYc2zsQ0siAMQkZYmiAK0IwkNgFYMRYoYLKAWJZhPIWZJIElvOyS+L6VfLK4mqoWvrerd7jBHiBlrYgA

.c, 
/* stylelint-disable-next-line selector-max-type */
h2 {
  color: black;
}

The reported range for the error is actually the entire selector, starting from after the comma.


/* stylelint-disable-next-line selector-max-type */
h2

While the configuration command is for the next line.

Because the report start position is before the actual configuration command it doesn't work to use next line.

https://stylelint.io/demo/#N4Igxg9gJgpiBcIB0YA0AdAdgegFQAIBnAFwE8AbGcgS02IFoprCBDAI0vpsxiKpjDEIAJ3oBbFgA96ZAA69c2LAAsATPmBZ8+SORHx8HFmADWAbiwBfEKhAAzapQByLMXEQxJr2ZRSFCNuAQmA4A5gggmpja6CCexDCYUISxBgDaWtr4sSQUVLQMkCHUofQkLEkswlCxmQC6GNHZIMIArpQpCBpZ2U0xIIT8giLiUjKk8qn4AAyZllaBRWEAYiISxBEAVoTBgbCyAYhR-bmU3MRTseQsCSSxjSdkZwX0SyVlxBVQVTVdVzcwO4gBaWIA

This however works fine:

.c,
/* stylelint-disable-line selector-max-type */
h2 {
  color: black;
}

This isn't ideal because it isn't intuitive that invisible characters count as part of the selector.

Instead the selector start index should be that of the first meaningful character, in this case the h out of h2.

I think we need to use a more refined approach to calculate the start and end indices for reports that cover an entire selector.

Mouvedia commented 1 month ago

I think that assigning the incorrect cause will prevent us from solving these issues swiftly :)

👍

I was just pointing out that no-invalid-double-slash-comments was stripping comments as I was warning about. i.e. my demo in the other issue demonstrate that selector-not-notation is probably also affected

As you have correctly said earlier there are probably several causes.

tl;dr: we will need a bunch of regression tests for a lot of rules to be added either before (skipped) or while fixing the affected rules

romainmenke commented 1 month ago

@Mouvedia that is not correct :) This issue is about the behavior you can see for calc()

Unexpected function "calc"

See how it is reported on the wrong line when any comment is present.

This behavior existed before we added support for configuration commands and didn't change because we added such support.

While the concerns you raised are for the mutation of :not() where the configuration command is not preserved.

I will file a separate issue for those so that we have a place to discuss them.

Edit: https://github.com/stylelint/stylelint/issues/7908

romainmenke commented 1 month ago

I think there are two types of issues reported here. That doesn't mean that there aren't more kinds still hiding :)

Both issues look the same to users, and have the same outcome, configuration commands that don't work as expected. But each requires a specific fix. Some rules might even be affected by both issues.

Type 1:

rules use values from the AST that do not contain any comments. (rule.selector over rule.raws.selector.raw))
the calculated indices for warnings are incorrect because they do not take preceding comments into account
configuration commands (disable-next-line) don't work as expected because the ranges do not match

.a,
/* foo */
.b, // Comment 2
{
  display: none;
}

Fix:

We can fix these issues by consistently using the right helpers in rules (e.g. getRuleSelector)

Type 2:

rules use report ranges that are too large, larger than what is intuitive for users
configuration commands (disable-next-line) don't work as expected because the report range starts too early.

.c, /* -> report starts here */
/* stylelint-disable-next-line selector-max-type */
h2 {
  color: black;
}

Fix:

We can adjust the reported ranges so that preceding whitespace and comments are skipped. Reports could start at the first meaningful character.

A helper like this for selectors might be a good start:

/**
 * Get the source indices of a selector, excluding whitespace and comments.
 *
 * @param {import('postcss-selector-parser').Selector} node
 *
 * @returns {{index: number, endIndex: number}}
 */
export default function getSelectorMeaningfulSourceIndices(node) {
    const first = node.nodes.find((x) => !selectorParser.isComment(x));
    const last = node.nodes.findLast((x) => !selectorParser.isComment(x));

    if (!first || !last) {
        const index = node.first?.sourceIndex ?? node.sourceIndex;
        const endIndex = index + node.toString().trim().length;

        return {
            index,
            endIndex
        }
    }

    return {
        index: first.sourceIndex,
        endIndex: last.sourceIndex + last.toString().trim().length
    }
}

ybiquitous commented 1 month ago

@Mouvedia @romainmenke Thanks for addressing this issue.

https://github.com/stylelint/stylelint/issues/7904#issuecomment-2275024780 sounds good to me.

You can directly open a PR for bug fixes or open a new issue if you need to. 👍🏼

romainmenke commented 1 month ago

Hehe, comment handling in the postcss ecosystem is a constant source of hurdles and challenges. Apparently postcss-selector-parser also stores some, but not all comments in raw fields, just like postcss does.

So we can't simply slice out all leading and trailing whitespace and comments to extract the "meaningful" selector.

Mouvedia commented 4 weeks ago

@romainmenke can we close this issue? We can always refer to it in upcoming PRs or create new an umbrella issue or several issues for the other rules impacted. e.g. function-disallowed-list

romainmenke commented 4 weeks ago

Better to make this the umbrella issue and close it when it has been fully resolved as @pamelalozano16 stated that they were referring to the entirety of the problem and only bringing up specific rules as examples.

see: https://github.com/stylelint/stylelint/issues/7904#issuecomment-2274105337

Edit:

I've removed the documentation aspect from the title and summary because although it was related it wasn't part of the core issue.

I've added a todo list with rules that call walkRules in the issue summary and are likely affected by this issue.

I've also setup an example PR that resolves this issue for a single rule: https://github.com/stylelint/stylelint/pull/7932

Please consider contributing if you have time.

stylelint / stylelint