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.

After some discussion in #4849, this PR implements a fix by updating the isStandardSyntaxCombinator rule util to ignore certain combinators that are the first or last combinator in their container. Since combinators should generally be between two different selectors, we can rule out combinators that... aren't. Then, we use this new utility in selectorCombinatorSpaceChecker, which then fixes the false positive in selector-combinator-space-after.

I also added some test to verify our functionality, for isStandardSyntaxCombinator and for the case mentioned in #4584 in selector-combinator-space-after.

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

Closes #4584.

Is there anything in the PR that needs further explanation?

First, a quick question about the > nesting selector: I had to ignore this, because the following SCSS is standard:

a {
  > b {
     ...
  }
}

Not sure if this is the right approach, or if I should instead update the existing tests. Any thoughts on this would be appreciated.

Also, a meta-note: this is a fresh version of the PR #4849, which had some issues after merging master back into the forked repository. Copied over the code to a fresh branch from master.

[jeddy3]

Thank you for continuing to work on this issue. The pull request is almost ready, I think.

First, a quick question about the > nesting selector: I had to ignore this, because the following SCSS is standard:

The isStandard* utilities are for whitelisting standard CSS code. They're used to shield the built-in rules from edge cases introduced in the various CSS Processors like SCSS.

The > in:

a {
  > b {
     ...
  }
}

Is not, I believe, a standard CSS combinator.

or if I should instead update the existing tests.

Yes, I think so.

[mattxwang]

Hey @jeddy3,

Thank you for taking the time to go back-and-forth with me on this PR.

I've removed the special case, and adjusted the test case in a way that I think best implements the feature - but, the caveat is that it actually removes some extra rejection cases. For example, consider the following for selector-combinator-space-after

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

in conjunction with the test config

{
  code: 'a { >/*comment*/a {} }',
  fixed: 'a { > /*comment*/a {} }',
  description: 'scss nesting and comment',	
  message: messages.expectedAfter('>'),	
  line: 1,	
  column: 5,	
},

Previously, we would reject this. However, the new implementation of isStandardSyntaxCombinator treats the > as a non-standard syntax combinator (as you've indicated above), and thus skips this combinator/selector pair entirely, which leads to not rejecting the rule. In my opinion, this seems quite reasonable, since this is the same philosophy behind the problem in the original issue #4584; however, I do want to clarify if I'm taking this approach properly.

Another way I could re-do these specific test cases (and still keep the original test ) is to add the & inherit selector before the >, like so:

{
  code: 'a { & >/*comment*/a {} }',
  fixed: 'a { & > /*comment*/a {} }',
  description: 'scss nesting and comment',	
  message: messages.expectedAfter('>'),	
  line: 1,	
  column: 7,	
},

But this kind of dodges the main problem, which is dropping certain rejection cases, and I don't think it actually tests for scss nesting (since the inside is just like any normal selector).

Sorry for this wall of text, but I want to make sure I get this right! Let me know what you think, and if there's a different option I should take. If not, I think the PR is good to be reviewed!

[jeddy3]

Thank you, LGTM!

In my opinion, this seems quite reasonable, since this is the same philosophy behind the problem in the original issue #4584; however, I do want to clarify if I'm taking this approach properly.

Yes, you are. The built-in rules are geared towards standard CSS. A plugin author can, if they like, create scss/selector-combinator-space-after that deals with non-standard things like the implicit nesting selector. This is already the case for many other SCSS and SCSS-like syntax.