Fix false positives for trailing combinator in selector-combinator-space-after

[mattxwang]

Hey there!

This is a PR that tries to fix the false positive brought up in #4584, namely that a trailing combinator (such as the general sibling combinator, ~) is not properly handled by selector-combinator-space-after.

This PR implements the suggested fix, by adding a check to isStandardSyntaxSelector for trailing combinators. It then uses isStandardSyntaxSelector in selectorCombinatorSpaceChecker, a util used by both selector-combinator-space-after and selector-combinator-space-before. Finally, it adds the test suggested in the original issue.

Which issue, if any, is this issue related to?

Closes #4584.

Is there anything in the PR that needs further explanation?

I wrote a (what I think is poor) implementation of the trailing combinator check:

for (let combinator of punctuationSets.nonSpaceCombinators) {
	if (selector.includes(`${combinator},`)) {
		return false;
	}
}

This runs in O(n) in the number of nonSpaceCombinators, so there aren't any performance benefits from using a set (and I can't imagine running .includes() n times is that speedy either).

However, I wasn't able to think of another way to do this. Is there something I'm missing? I may have not understood the "search for a trailing combinator" portion of the issue.

[jeddy3]

@malsf21 Thanks for getting started on this.

I've dug a little deeper and I realise now that my original suggestion of adding a clause to isStandardSyntaxSelector isn't right. The isStandardSyntaxSelector does high-level heuristics on the whole selector string. I think we, therefore, need a new isStandardSyntaxCombinator util that will accept a combinator node.

I believe all combinators need to, by their very nature, come between two selectors to be of standard syntax.

For example:

a + b {}
a { & + b {} }

In the new util, we should return false if the combinator node is the first or last node in the container (using the selector parser API), and return true otherwise.

In the following selector list, the combinator is the last node of the first selector container:

a ~, b {}

It is therefore not a standard syntax combinator.

Same is true for:

a, ~ b {}

Where the combinator is the first node of the second selector container.

Unfortunately, I don't think is an AST Explorer for PostCSS Selector Parser that I can link to to show you this. However, you can view the AST locally using:

var parser = require("postcss-selector-parser")
var ast = parser().astSync('a ~, b')
console.log(ast);

Thanks again for tackling these issues. This is another one where we're getting into the nitty-gritty of the PostCSS parsers and ASTs!

[jeddy3]

Let's add a !isStandardSyntaxCombinator(node) conditional here instead.

[mattxwang]

Gotcha, I think that makes sense. A bit swamped with work this week but I'll see if I can make the revision! I'll make a new util isStandardSyntaxCombinator, write some tests, and then implement it in selector-combinator-space-after.

Just to be clear, I should then revert my change to isStandardSyntaxSelector?

[jeddy3]

Just to be clear, I should then revert my change to isStandardSyntaxSelector?

That's correct.

A bit swamped with work this week but I'll see if I can make the revision!

No worries. Only do it if you've time. I'm likely to be swamped with work too going forward, but I'll try to make time for reviewing your pull requests as I enjoy it.

[mattxwang]

No worries. Only do it if you've time.

Yup, will do! Aiming for a target of this weekend!

[mattxwang]

I think we, therefore, need a new isStandardSyntaxCombinator util that will accept a combinator node.

Just to clarify @jeddy3, there's already a util called isStandardSyntaxCombinator, which does something else. Should I modify this one, create a new one, do some refactoring, etc? Confused on what you mean by "new util".

[jeddy3]

Just to clarify @jeddy3, there's already a util called isStandardSyntaxCombinator, which does something else. Should I modify this one, create a new one, do some refactoring, etc? Confused on what you mean by "new util".

My bad, I didn't notice that the util already existed. We'll want to modify this one to include the new conditionals rather than create a new utility.

[mattxwang]

Implemented the new utility, though I do have a quick question. If I implement the check to just returning false on the first and last nodes, I get an error for this type of test case, involving the SCSS/SASS use case of the nested >:

'a { 
  >/*comment*/a {} 
}

In this case, the > combinator is the first combinator in its container, so the new implementation of the rule would say that it's non-standard, and skip the block. But, the test case in this scenario doesn't like that, and wants a space in between > and a (ignoring the comment).

In what I've pushed to the branch, I chose to ignore cases of >, but I'm not sure if this is the right way to proceed. In this case, should isStandardSyntaxCombinator return true or false for this nested >?

if (
  node.parent !== undefined &&
  node.parent !== null &&
  node.value !== '>' // should this be here?
) {
  let parent = node.parent;
  if (node === parent.first) {
    return false;
  }
  if (node === parent.last) {
    return false;
  }
}

[m-allanson]

@malsf21 it looks like something has gone wonky with the PR, it's showing 131 changed files. Most of them are changes that have already been merged.

I'm guessing it's caused by the way GitHub doesn't track changes on the target branch after the PR is opened. There are some suggested fixes here: https://stackoverflow.com/questions/16306012/github-pull-request-showing-commits-that-are-already-in-target-branch

[mattxwang]

@malsf21 it looks like something has gone wonky with the PR, it's showing 131 changed files. Most of them are changes that have already been merged.

I'm guessing it's caused by the way GitHub doesn't track changes on the target branch after the PR is opened. There are some suggested fixes here: https://stackoverflow.com/questions/16306012/github-pull-request-showing-commits-that-are-already-in-target-branch

Hm, I tried a few but I'm not getting too much success. I may close this PR and re-open it with new changes. I think it's because I cloned into my fork's master to update the files, when in reality I probably should've taken a different approach. Give me a moment to test things out!

[m-allanson]

I may close this PR and re-open it with new changes.

That's often the quickest way to fix these sorts of things 😅.

I think it's because I cloned into my fork's master

Feel free to clone stylelint/stylelint and create branches straight from this repo instead of your fork. Though I sometimes prefer working from a forked repo too.

Give me a moment to test things out!

Absolutely, no rush.

[mattxwang]

Closed in favour of #4878, which is the same solution but with a cleaner git history.