Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix false negatives for where, is, nth-child and nth-last-child in selector-max-* (except selector-max-type) #4842

Merged
merged 4 commits into from Jul 23, 2020

Conversation

@malsf21
Copy link
Member

@malsf21 malsf21 commented Jun 24, 2020

Hey,

This is a PR to address a concern brought up in #4836. As suggested, I

  1. Pulled is, not, matches and has out of this set and into a new logicalCombinationsPseudoClasses set.
  2. Added where to this set.
  3. Pulled nth-child and nth-last-child out of this set and into a new aNPlusBOfSNotationPseudoClasses set (note the extra OfS).
  4. Renamed isLogicalCombination to isContextFunctionalPseudoClass, and replace the switch with requiring the two new sets and checking for a match.

In addition, I also

  1. Updated the set union pseudoClasses to contain our new keyword sets
  2. Changed the tests for isContextFunctionalPseudoClass from the ones in isLogicalCombination
  3. Updated isStandardSyntaxTypeSelector to also check for aNPlusBOfSNotationPseudoClasses, as it previously relied on nth-child and nth-last-child being in aNPlusBNotationPseudoClasses
  4. Added a check for !isStandardSyntaxTypeSelector in selector-max-type

In general, it seems like everything works as intended; however, I noticed that we now fail the tests for selector-max-type, and in particular those involving nth-child and nth-last-child - I think it might be incorrectly recognizing n or tokens involving n as a type selector, but I'm not entirely sure. It also might be that we would want to preserve the original behaviour of isLogicalCombination in this case. I've spent a few days wiggling things around, but I think my unfamiliarity with the implementation of selector-max-type is hindering me. Would love to get feedback on this!

Until then, I'm marking this PR as a draft.

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

Closes #4836.

Is there anything in the PR that needs further explanation?

Don't think so!

malsf21 added 2 commits Jun 23, 2020
…alPseudoClass

note: possible logic error in selector-max-type, will investigate further
@malsf21 malsf21 marked this pull request as draft Jun 24, 2020
@jeddy3
Copy link
Member

@jeddy3 jeddy3 commented Jun 25, 2020

@malsf21 Thanks for starting this. It's looking great so far.

I noticed that we now fail the tests for selector-max-type, and in particular those involving nth-child and nth-last-child - I think it might be incorrectly recognizing n or tokens involving n as a type selector, but I'm not entirely sure.

That's correct. The rule now checks within nth-child and nth-last-child because of their inclusion in isContextFunctionalPseudoClass, but the selector parser is incorrectly classifying n etc as type selectors.

I believe the selector-max-type rule should make use of the isStandardSyntaxTypeSelector util:

// add to selector-max-type/index.js line 86
if (childNode.type === 'tag' && !isStandardSyntaxTypeSelector(childNode)) {
  return total;
}

The util has some logic within it to workaround this limitation of the selector parser. However in its current form, the util will blanket ignore all type selectors that are child nodes of nth-child and nth-last-child pseudo-classes:

        if (parentValue) {
		const normalisedParentName = parentValue.toLowerCase().replace(/:+/, '');
		if (
			parentType === 'pseudo' &&
			(keywordSets.aNPlusBNotationPseudoClasses.has(normalisedParentName) ||
				keywordSets.aNPlusBOfSNotationPseudoClasses.has(normalisedParentName) ||
				keywordSets.linguisticPseudoClasses.has(normalisedParentName) ||
				keywordSets.shadowTreePseudoElements.has(normalisedParentName))
		) {
			return false;
		}
	}

For example, it will have a false negative for the li in .foo:nth-child(-n+3 of li.important).

The heuristics need to be more nuanced and only ignore <an+b> notation parts within these pseudo-classes, but not the selector list. It should ignore:

  • even
  • odd
  • n
  • -n
  • n-3
  • 3n
  • and so on

It should also ignore of.

But it shouldn't ignore, for example, the li type selector in .foo:nth-child(-n+3 of li.important).

We can ignore the likes of odd, even, of and n easily, but we'll likely need a regex for other parts of the notation, e.g. 3n, n-3 and so on. Offhand, I can't think of what that might look like but you may be able to rustle up something.

@malsf21
Copy link
Member Author

@malsf21 malsf21 commented Jun 25, 2020

Gotcha, thank you for the in-depth reply! Hm, looking for those elements might be a bit tricky, I'll try to think of a workaround. Regex is one option, though I'm thinking about tokenizing (?) the thing inside the pseudo-class and splitting it up into a set of possible types:

  • operators like + or -
  • the atoms of, even, odd
  • the atom n and the numerical atoms [0-9]
  • everything else is implicitly interpreted as a CSS node?

That being said, I wonder if we can easily do this by stripping out elements one at a time (e.g. remove all operators, then numerical atoms, etc.). Not sure how that will work with an edge case like .odd-column or #row-3.

I'll toy around and see what I can conjure up. Worst-case scenario, I could revert the change for this current rule, and then create another issue to tackle this problem later on.

@jeddy3 jeddy3 changed the title Fix false negatives for where, is, nth-child and nth-last-child in selector-max-* Fix false negatives for where, is, nth-child and nth-last-child in selector-max-* (except selector-max-type) Jun 26, 2020
@jeddy3
Copy link
Member

@jeddy3 jeddy3 commented Jun 26, 2020

though I'm thinking about tokenizing

That would be amazing. If you've time to delve into tokenising it may be better to fix this issue upstream in the parser itself. (I've only had time to operate one-level above the parsers, i.e. helping to build stylelint consuming the various parsers rather than working on the parsers themselves, so it's not an area I know.)

Worst-case scenario, I could revert the change for this current rule, and then create another issue to tackle this problem later on.

Yes, I agree we push this to another issue. I suggest we make one more change to this pull request; make use of the isStandardSyntaxTypeSelector util in the selector-max-type rule.

// add to selector-max-type/index.js line 86
if (childNode.type === 'tag' && !isStandardSyntaxTypeSelector(childNode)) {
  return total;
}

The tests should pass then and all the selector-max-* will have consistent code.

The pull request will fix the false negatives for where, is, nth-child and nth-last-child in all the selector-max-* rules except for selector-max-type. In a follow-up issue, we can address the selector-max-type false negatives by either improving the heuristics in isStandardSyntaxTypeSelector util or by improving the upstream parser.

@malsf21
Copy link
Member Author

@malsf21 malsf21 commented Jun 27, 2020

Great, that sounds good. I've implemented the addition to selector-max-id, and marked this PR as ready for review (it passes all the tests).

I've got to say, I have very little experience with tokenization or writing parsers (I'm only vaguely familiar from a PL class I took in university), and as you've noticed I've been trying to get familiar with stylelint by taking on low-hanging fruit. I'll take a look upstream and see what I can do!

@malsf21 malsf21 marked this pull request as ready for review Jun 27, 2020
Copy link
Member

@jeddy3 jeddy3 left a comment

@malsf21 Looking good. I've a few nitpicks (as suggestions), then I think we're ready to merge.

lib/rules/selector-max-type/index.js Outdated Show resolved Hide resolved
lib/utils/isContextFunctionalPseudoClass.js Outdated Show resolved Hide resolved
lib/rules/selector-max-pseudo-class/index.js Outdated Show resolved Hide resolved
lib/rules/selector-max-id/index.js Outdated Show resolved Hide resolved
lib/rules/selector-max-compound-selectors/index.js Outdated Show resolved Hide resolved
lib/rules/selector-max-class/index.js Outdated Show resolved Hide resolved
lib/rules/selector-max-attribute/index.js Outdated Show resolved Hide resolved
@jeddy3
Copy link
Member

@jeddy3 jeddy3 commented Jun 27, 2020

I've got to say, I have very little experience with tokenization or writing parsers (I'm only vaguely familiar from a PL class I took in university)

If you've not seen it already, the The Super Tiny Compiler is an excellent learning resource.

as you've noticed I've been trying to get familiar with stylelint by taking on low-hanging fruit

Yes, it's great to see! Thank you for your contributions. There are plenty of interesting issues to work on directly in stylelint if that parser fix doesn't end up appealing to you.

minor documentation fixes!

Co-authored-by: Richard Hallows <jeddy3@users.noreply.github.com>
@malsf21
Copy link
Member Author

@malsf21 malsf21 commented Jun 28, 2020

Implemented the changes!

@jeddy3
jeddy3 approved these changes Jun 28, 2020
Copy link
Member

@jeddy3 jeddy3 left a comment

Excellent. LGTM, thanks!

@jeddy3 jeddy3 mentioned this pull request Jul 6, 2020
6 of 6 tasks complete
Copy link
Member

@m-allanson m-allanson left a comment

Thank you @malsf21, this LGTM!

Your clear write-up was particularly helpful for review purposes 👍

@m-allanson m-allanson merged commit cba8dcf into stylelint:master Jul 23, 2020
8 checks passed
8 checks passed
Lint on Node.js 12 and ubuntu-latest
Details
Test on Node.js 10 and ubuntu-latest
Details
Test on Node.js 10 and windows-latest
Details
Test on Node.js 10 and macos-latest
Details
Test on Node.js 12 and windows-latest
Details
Test on Node.js 12 and macos-latest
Details
Coverage on Node.js 12 and ubuntu-latest
Details
coverage/coveralls Coverage increased (+0.002%) to 96.62%
Details
@m-allanson
Copy link
Member

@m-allanson m-allanson commented Jul 23, 2020

Updated changelog:

  • Fixed: false negatives for where, is, nth-child and nth-last-child in selector-max-* rules (except selector-max-type) (#4842).
m-allanson added a commit that referenced this pull request Jul 23, 2020
* master:
  Update CHANGELOG.md
  Fix false negatives for where, is, nth-child and nth-last-child in selector-max-* (except selector-max-type) (#4842)
  Bump @types/lodash from 4.14.155 to 4.14.157 (#4869)
  Remove `postcss-reporter` package (#4858)
  Bump lodash from 4.17.15 to 4.17.19 (#4864)
  Replace 3rd-party type definitions (#4857)
@malsf21 malsf21 deleted the malsf21:fix/false-negatives-for-some-pseudo-classes branch Jul 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.