feat(biome_css_analyze): noDuplicateSelectors

[abidjappie]

Summary

This PR aims to implement no-duplicate-selectors from the recommended Stylelint rules. See here:
https://github.com/stylelint/stylelint/tree/main/lib/rules/no-duplicate-selectors

Approach

This rule is run on CssRoot and the analysis is performed on either:

• For disallow_in_list = true: CssComplexSelector and CssRelativeSelector
• For disallow_in_list = false: CssSelectorList and CssRelativeSelectorList, i.e it treats the entire list as a selector instead of comparing individual selectors in the list.

The motivation behind using CssRoot instead of some union of selectors is that we need to compare all the selectors within the root against each other, so having the rule trigger once per root allows us to maintain a single working state.

Nested selectors are resolved to an equivalent flat selector in text form. The resolved selectors are then checked for duplicates using a HashSet.

/* Given the following rule, and resolved_list = [] : */
div.some {
  /* resolved_list = ["div.some"] */
  div.other { /* declaration */ }
  /* resolved_list = ["div.some", "div.some div.other"] */
  .foo.bar { /* declaration */ }
  /* resolved_list = ["div.some", "div.some div.other", "div.some .foo.bar"] */
  .div.other {/* declaration */}
  /* duplicate found, "div.some div.other" already exists in resolved_list */
}

Resolution

The resolution method is inspired by postcss-resolve-nested-selector which is used by the original stylelint no-duplicate-selectors. To resolve the selectors: recursively find the parent rule and and resolve the selectors in the prelude.

/* When disallow_in_list = true */
a, b { c, d {  e { /* declaration */ } } }
/* Selector "e" resolves to */
a c e {}
a d e {}
b c e {}
b d e {}
/* Selector "c" resolves to */
a c {}
b c {}

/* When disallow_in_list = false */
a, b { c, d {  e { /* declaration */ } } }
/* Selector list "e" resolves to */
a,b c,d e {}
/* Selector list "c,d" resolves to */
a,b c,d {}

There is special handling for CssAtRule - since children of the at rule cannot be compared to other at-rules (it has a unique context) we use a hash to make them unique. i.e. only children within the same at-rule are comparable.

@media print { s {} t {} }
@media print { x {} y {} }
/* Resolves to, using an example hash */
12307447392357712145 s {}
12307447392357712145 t {}
9798198868576407827 x {}
9798198868576407827 y {}

Caveats

• This PR does not implement support for SCSS and LESS

resolves #2514

Test Plan

Passes tests from Stylelint: https://github.com/stylelint/stylelint/blob/main/lib/rules/no-duplicate-selectors/__tests__/index.mjs

[Mouvedia]

Maybe add

.bar,
.bar,
.bar {
    width: 12;
}

Ensure it reports either for the first and the second or the second and the third.
Also it either reports per selector list or separately.
i.e. should it report

  ! Duplicate selectors.

  > 1 │ .bar,
        ^^^^
  > 2 │ .bar,
        ^^^^
  > 3 │ .bar, {
    4 │     width: 12;
    5 │ }

?

[abidjappie]

@Mouvedia thank you for the suggestions. I will update the test cases and pay attention to this case.

Probably, to be consistent with stylelint it should only report on the first selector in this case.
In the case of two separate rules:

.bar {}
.bar {}

it will report on the second one!

[Mouvedia]

@abidjappie see stylelint/stylelint#7583
i.e. it's planned

[Mouvedia]

I don't know why you requested a review from a rust noob but here you go.

[Mouvedia]

question
refactor

This looks similar to l159-162; could it become a function?
i.e. <T>

disclaimer: rust is not my main language but you asked for a review

[Mouvedia]

minor
refactor

see l186-189, l146-149

[abidjappie]

I don't know why you requested a review from a rust noob but here you go.

Thank you for the feedback! Since you were providing helpful feedback before, I wanted to do a final check that your feedback was adequately addressed. Anything that can help improve the experience provided by this rule is good I guess 👍

[codspeed-hq]

CodSpeed Performance Report

Merging #2660 will not alter performance

_{Comparing abidjappie:feat/stylelint-no-duplicate-selectors (911eacf) with main (f943fad)}

Summary

✅ 82 untouched benchmarks

[ematipico]

You can use ::can_cast if you need a boolean

[ematipico]

Don't unwrap(), it's very unsafe. Inside a rule, you can usually do this: .ok()? instead. We convert Result to Option and then use the ? operator.

[ematipico]

Can we have more documentation here? Maybe a longer explanation of what it does?

[ematipico]

Can we document the option using doc comments (///)? They will appear in the configuration schema

[ematipico]

Suggested change

	/// ```json
	/// {
	/// "noDuplicateSelectors": {
	/// "options": {
	/// "disallowInList": true
	/// }
	/// }
	/// }
	/// ```
	/// ```json5, ignore
	/// {
	/// // ...
	/// "noDuplicateSelectors": {
	/// "options": {
	/// "disallowInList": true
	/// }
	/// }
	/// // ...
	/// }
	/// ```

[ematipico]

Can we document the function? "Normalize" has a very broad meaning and changes based the context.

[ematipico]

Suggested change

	if let Some(complex_selector) = CssComplexSelector::cast_ref(&selector.clone().into_syntax()) {
	if let AnyCssSelector::CssComplexSelector(complex_selector) = selector {

No need cloning and into_syntax

[ematipico]

Let's document this type and how it will be used. Considering that we use a specific Hash, it's worth the explanation

[ematipico]

Suggested change

	//
	// Read our guidelines to write great diagnostics:
	// https://docs.rs/biome_analyze/latest/biome_analyze/#what-a-rule-should-say-to-the-user
	//

[ematipico]

Don't use the to_string of a node because it will write trivia too. Instead, use a typed node or a union of nodes, and implement a text() that returns the exact name you require printing.

[abidjappie]

@ematipico thank you for the feedback! will address ASAP.
Running through the lint issues first 🙇

[ematipico]

@abidjappie As for now, I just did a dumb review of your code. However, I noticed that the logic of the code is quite lengthy, so we would appreciate it if you explained how you tackled the problem and the logic of your code in the description of the PR.

Without this information, it will be very difficult to get a sensible review, and it will take a long time to merge this.

[abidjappie]

@ematipico thank you for the feedback so far.

I added some documentation and comments, as well as addressing the feedback from before as best as I could.

I would like to make this as easy as possible to review, so if the documentation is lacking or if there is any additional content I should prepare please let me know! 🙇

[ematipico]

Thank you @abidjappie for the explanation.

Here are some thoughts about how to approach the problem

Technical

• I quickly checked postcss-resolve-nested-selector, and it uses a recursion. In fact, the provided solution uses recursion, too. I strongly advise against using recursion in tree-like data structures because they put a lot of memory pressure. In Rust, we can use loops and iterators.
• I would split the implementation into multiple PRs. In this rule, it seems you tried to cover many cases, however, it's very easy to get lost and fall in some subtle issues. For example, in the following example, we emit two diagnostics, while we can be more clever and emit only when, where we point the different repetitions:

   .e, .e, .e {}

  The last .e should point to the error (or the first one?), and then we keep track of all the repetitions (text ranges) and eventually add multiple code frames to point the rest of .e
  My advice would be to tackle simple selectors first and then work towards complex selectors and nested selectors.
• I know you wanted to use CssRoot to minimise the access to multiple selectors, however I think a better solution is to use a custom visitor to better track selectors you encounter and to save or dispose of the information we need during the visiting of the AST. We other rules that use custom visitors, you can check how to use them.
• Don't use unwrap, ever, unless we are 100% sure it's safe to use. Inside lint rules we rarely use unwrap because we have better tools to handle Result/Option. When using unwrap, it's custom in Rust to add // SAFETY: <explanation why it's safe> comment.
• There's a lot of String allocation. Any chance we can reduce them, or use another type for doing checks? Also, there are weird cases that aren't explain in the code:

   let split: Vec<&str> = r.split_whitespace().collect();
   let normalized = split.join(" ").to_lowercase();

  We allocate another string from the one emitted by r

Docs

• The documentation of the resolution of nested selectors needs some work. For example, it's great that you provided an example at the beginning, but what's resolved doesn't match Vec<String>, so I guess the function returns ["a", "b", "c"]?

[dyc3]

Make sure to cargo insta accept these snapshots

[abidjappie]

@ematipico thank you for the explanations! It's super insightful. 🙇

I think my next steps will be to set this PR back to draft or close it (I would like to use it a reference though) and prepare some more manageable smaller PR's.

My previous attempt using the visitor was doing something like this:

        match event {
            WalkEvent::Enter(node) => {
                if let Some(node) = CssDeclarationOrRuleBlock::cast_ref(node) {
                    self.stack.push(node);
                }
            }
            WalkEvent::Leave(node) => {
                if let Some(_node) = CssRuleList::cast_ref(node) {
                    ctx.match_query(DeclarationOrRuleBlockList(self.stack.clone()));
                    self.stack.clear();
                }
            }
        }

But still resolving it using recursion, I think I need to wrap my head around solving this from a parent -> child rather than from child -> parent 🤔

[Mouvedia]

I think you should take a look at stylelint/stylelint#7869.

[abidjappie]

Thanks for the feedback!