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

feat(biome_css_analyze): noDuplicateSelectors #2660

Draft
wants to merge 22 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
564b995
Bootstrapped and added basic logic and failing tests.
abidjappie Apr 25, 2024
08c949e
Added some helpful comments and save progress.
abidjappie Apr 25, 2024
c4674af
WIP: it currently returns everything in a single run, we need to spli…
abidjappie Apr 30, 2024
99af5a2
Working solution excluding some edge cases, still using a visitor nee…
abidjappie Apr 30, 2024
b6b3d1e
WIP: removed the visitor, simplified the logic, added stylelint test …
abidjappie May 2, 2024
87a6f5e
WIP: Added option cases, many more test cases are passing. Next: pass…
abidjappie May 3, 2024
3965886
WIP: Fixed test case. some cleaning.
abidjappie May 3, 2024
3d58e8e
All the current valid and invalid cases are now working as expected. …
abidjappie May 5, 2024
3ed2b90
Added option test cases, updated the diagnostics. WIP: cleanup.
abidjappie May 5, 2024
65e6ba9
Ran gen-lint.
abidjappie May 6, 2024
0b51c52
Added more test cases, adjusted the at-rule logic.
abidjappie May 6, 2024
40de23d
Code structure improvements and simplifications.
abidjappie May 7, 2024
2ed1405
Linting.
abidjappie May 7, 2024
8feeeda
Added additional test cases, updated the diagnostic messages.
abidjappie May 7, 2024
648ffdd
Fixed typo.
abidjappie May 7, 2024
6ed13f2
Fixed merge conflicts, gen.
abidjappie May 7, 2024
e667f3c
Better diagnostic reporting.
abidjappie May 7, 2024
7e2b209
Addressed linter feedback. WIP: addressing review feedback.
abidjappie May 7, 2024
4471c4f
WIP: added comments.
abidjappie May 8, 2024
f453e8c
WIP: added comments.
abidjappie May 8, 2024
911eacf
Handle the text in the diagnostics.
abidjappie May 8, 2024
04556f6
Fixed merge conflicts, gen.
abidjappie May 9, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
113 changes: 67 additions & 46 deletions crates/biome_configuration/src/linter/rules.rs

Large diffs are not rendered by default.

2 changes: 2 additions & 0 deletions crates/biome_css_analyze/src/lint/nursery.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ pub mod no_color_invalid_hex;
pub mod no_css_empty_block;
pub mod no_duplicate_at_import_rules;
pub mod no_duplicate_font_names;
pub mod no_duplicate_selectors;
pub mod no_duplicate_selectors_keyframe_block;
pub mod no_important_in_keyframe;
pub mod no_unknown_function;
Expand All @@ -22,6 +23,7 @@ declare_group! {
self :: no_css_empty_block :: NoCssEmptyBlock ,
self :: no_duplicate_at_import_rules :: NoDuplicateAtImportRules ,
self :: no_duplicate_font_names :: NoDuplicateFontNames ,
self :: no_duplicate_selectors :: NoDuplicateSelectors ,
self :: no_duplicate_selectors_keyframe_block :: NoDuplicateSelectorsKeyframeBlock ,
self :: no_important_in_keyframe :: NoImportantInKeyframe ,
self :: no_unknown_function :: NoUnknownFunction ,
Expand Down
327 changes: 327 additions & 0 deletions crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,327 @@
use std::borrow::Borrow;
use std::collections::HashSet;
use std::hash::{DefaultHasher, Hash, Hasher};
use std::vec;

use biome_analyze::Ast;
use biome_analyze::{context::RuleContext, declare_rule, Rule, RuleDiagnostic, RuleSource};
use biome_console::markup;
use biome_css_syntax::{
AnyCssAtRule, AnyCssRelativeSelector, AnyCssRule, AnyCssSelector, CssComplexSelector,
CssRelativeSelector, CssRelativeSelectorList, CssRoot, CssSelectorList, CssSyntaxNode,
};
use biome_deserialize_macros::Deserializable;
use biome_rowan::{AstNode, SyntaxNodeCast};

use serde::{Deserialize, Serialize};

declare_rule! {
/// Disallow duplicate selectors.
///
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/// ## Examples
///
/// ### Invalid
///
/// ```css,expect_diagnostic
/// .abc,
/// .def,
/// .abc {}
/// ```
///
/// ### Valid
///
/// ```
/// .foo {}
/// .bar {}
/// ```
///
/// ## Options
///
/// If true, disallow duplicate selectors within selector lists.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also provide an example? When going so, add css,ignore, so the code gen won't try to test it.

///
/// ```json
/// {
/// "noDuplicateSelectors": {
/// "options": {
/// "disallowInList": true
/// }
/// }
/// }
/// ```
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

///
pub NoDuplicateSelectors {
version: "next",
name: "noDuplicateSelectors",
recommended: true,
sources: &[RuleSource::Stylelint("no-duplicate-selectors")],
}
}

#[derive(Debug, Clone, Deserialize, Deserializable, Eq, PartialEq, Serialize)]
#[cfg_attr(feature = "schemars", derive(schemars::JsonSchema))]
#[serde(rename_all = "camelCase", deny_unknown_fields)]
pub struct NoDuplicateSelectorsOptions {
pub disallow_in_list: bool,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

impl Default for NoDuplicateSelectorsOptions {
fn default() -> Self {
Self {
disallow_in_list: false,
}
}
}

#[derive(Debug, Eq)]
struct ResolvedSelector {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

selector_text: String,
selector_node: CssSyntaxNode,
}

impl PartialEq for ResolvedSelector {
fn eq(&self, other: &ResolvedSelector) -> bool {
self.selector_text == other.selector_text
}
}
impl Hash for ResolvedSelector {
fn hash<H: Hasher>(&self, state: &mut H) {
self.selector_text.hash(state);
}
}
impl Borrow<String> for ResolvedSelector {
fn borrow(&self) -> &String {
&self.selector_text
}
}

pub struct DuplicateSelector {
first: CssSyntaxNode,
duplicate: CssSyntaxNode,
}

impl Rule for NoDuplicateSelectors {
type Query = Ast<CssRoot>;
type State = DuplicateSelector;
type Signals = Vec<Self::State>;
type Options = NoDuplicateSelectorsOptions;

fn run(ctx: &RuleContext<Self>) -> Vec<Self::State> {
let node = ctx.query();
let options = ctx.options();

let mut resolved_list: HashSet<ResolvedSelector> = HashSet::new();
let mut output: Vec<DuplicateSelector> = vec![];

if options.disallow_in_list {
let selectors = node.rules().syntax().descendants().filter(|x| {
x.clone().cast::<AnyCssSelector>().is_some()
|| x.clone().cast::<AnyCssRelativeSelector>().is_some()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • use filter_map instead of filter, so you can return an Option
  • if you use filter_map you can use the following inside the closure
     AnyCssSelector:cast_ref(c).or(|c| AnyCssRelativeSelector::cast_ref(c))

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's way easier if you create a new node that is a union of AnyCssSelector and AnyCssRelativeSelector so you won't need to do any complex operation, such as unwrapping:

declare_node_union! {
	pub AnySelectorLike = AnyCssSelector | AnyCssRelativeSelector
}

The macro will implement all AstNode methods to AnySelectorLike.

});
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// selector_list unwrap should never fail due to the structure of the AST
for (selector, selector_list) in selectors
.map(|selector| (selector.clone(), selector.parent().unwrap()))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't use unwrap, it's very unsafe. Use filter_map instead of map and filter. In filter_map you can return an Option and use ? instead of unwrap

.filter(|(_, parent)| {
// i.e not actually a list
return !(parent.clone().cast::<CssComplexSelector>().is_some()
|| parent.clone().cast::<CssRelativeSelector>().is_some());
})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: It's more readable if you save this into a variable. And you can use filter_map

{
// this_rule unwrap should never fail due to the structure of the AST
let this_rule = selector_list.parent().unwrap();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, this is never guaranteed, because another rule could create an invalid CST.

I suggest the following:

let Some(this_rule) = selector_list.parent() else {
	continue;
}


let selector_text = if let Some(selector) = CssRelativeSelector::cast_ref(&selector)
{
selector.clone().text()
} else {
// selector is either AnyCssSelector or AnyCssRelativeSelector
normalize_complex_selector(selector.clone().cast::<AnyCssSelector>().unwrap())
};

for r in resolve_nested_selectors(selector_text, this_rule) {
let split: Vec<&str> = r.split_whitespace().collect();
let normalized = split.join(" ").to_lowercase();

if let Some(first) = resolved_list.get(&normalized) {
output.push(DuplicateSelector {
first: first.selector_node.clone(),
duplicate: selector.clone(),
});
} else {
resolved_list.insert(ResolvedSelector {
selector_text: normalized.clone(),
selector_node: selector.clone(),
});
}
}
}
} else {
let selector_lists = node.rules().syntax().descendants().filter(|x| {
x.clone().cast::<CssSelectorList>().is_some()
|| x.clone().cast::<CssRelativeSelectorList>().is_some()
});

// this_rule unwrap should never fail due to the structure of the AST
for (selector_list, rule) in selector_lists
.map(|selector_list| (selector_list.clone(), selector_list.parent().unwrap()))
{
let mut this_list_resolved_list: HashSet<ResolvedSelector> = HashSet::new();

let mut selector_list_mapped: Vec<String> = selector_list
.children()
.into_iter()
.filter_map(|child| {
let selector_text = if let Some(selector) = AnyCssSelector::cast_ref(&child)
{
normalize_complex_selector(selector.clone())
} else {
child
.clone()
.cast::<AnyCssRelativeSelector>()
.unwrap()
.text()
};

if let Some(first) = this_list_resolved_list.get(&selector_text) {
output.push(DuplicateSelector {
first: first.selector_node.clone(),
duplicate: child.clone(),
});
return None;
}

this_list_resolved_list.insert(ResolvedSelector {
selector_text: selector_text.clone(),
selector_node: child,
});
Some(selector_text)
})
.collect();
selector_list_mapped.sort();

for r in resolve_nested_selectors(selector_list_mapped.join(","), rule) {
let split: Vec<&str> = r.split_whitespace().collect();
let normalized = split.join(" ").to_lowercase();
if let Some(first) = resolved_list.get(&normalized) {
output.push(DuplicateSelector {
first: first.selector_node.clone(),
duplicate: selector_list.clone(),
});
Comment on lines +222 to +225
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor
refactor

see l186-189, l146-149

} else {
resolved_list.insert(ResolvedSelector {
selector_text: normalized.clone(),
selector_node: selector_list.clone().into(),
});
}
}
}
}
output
}

fn diagnostic(_: &RuleContext<Self>, node: &Self::State) -> Option<RuleDiagnostic> {
//
// Read our guidelines to write great diagnostics:
// https://docs.rs/biome_analyze/latest/biome_analyze/#what-a-rule-should-say-to-the-user
//
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
//

let duplicate_text = node.duplicate.to_string();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Some(
RuleDiagnostic::new(
rule_category!(),
node.duplicate.text_trimmed_range(),
markup! {
"Duplicate selectors may result in unintentionally overriding rules: "<Emphasis>{ duplicate_text }</Emphasis>
},
)
.detail(node.first.text_trimmed_range(), "Please consider moving the rule's contents to the first occurence:")
.note(markup! {
"Remove duplicate selectors within the rule"
}),
)
}
}

fn resolve_nested_selectors(selector: String, this_rule: CssSyntaxNode) -> Vec<String> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have some documentation that explains this function? It's quite long, so I suppose the logic needs some explanation

let mut parent_selectors: Vec<String> = vec![];
let parent_rule = this_rule.parent().and_then(|parent| parent.grand_parent());

match &parent_rule {
None => return vec![selector],
Some(parent_rule) => {
if let Some(parent_rule) = AnyCssAtRule::cast_ref(&parent_rule) {
let mut hasher = DefaultHasher::new();
parent_rule.range().hash(&mut hasher);
// Each @rule is unique scope
// Use a hash to create the comparable scope
parent_selectors.push(hasher.finish().to_string());
}
if let Some(parent_rule) = AnyCssRule::cast_ref(&parent_rule) {
match parent_rule {
AnyCssRule::CssNestedQualifiedRule(parent_rule) => {
for selector in parent_rule.prelude() {
if let Ok(selector) = selector {
parent_selectors.push(selector.text());
}
}
}
AnyCssRule::CssQualifiedRule(parent_rule) => {
for selector in parent_rule.prelude() {
if let Ok(selector) = selector {
parent_selectors.push(selector.text());
}
}
}
_ => {
// Bogus rules are not handled
// AtRule is handled by AnyCssAtRule above
}
}
}

let resolved_selectors: Vec<String> =
parent_selectors
.iter()
.fold(vec![], |result: Vec<String>, parent_selector| {
if selector.contains("&") {
let resolved_parent_selectors = resolve_nested_selectors(
parent_selector.to_string(),
parent_rule.clone(),
);
let resolved = resolved_parent_selectors
.into_iter()
.map(|newly_resolved| return selector.replace("&", &newly_resolved))
.collect();
return [result, resolved].concat();
} else {
let combined_selectors = parent_selector.to_owned() + " " + &selector;
let resolved =
resolve_nested_selectors(combined_selectors, parent_rule.clone());
return [result, resolved].concat();
}
});
if resolved_selectors.len() > 0 {
return resolved_selectors;
}
return vec![selector];
}
}
}

fn normalize_complex_selector(selector: AnyCssSelector) -> String {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

let mut selector_text = String::new();

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if let Ok(left) = complex_selector.left() {
selector_text.push_str(&left.text());
}
if let Ok(combinator) = complex_selector.combinator() {
let combinator = combinator.text_trimmed();
selector_text.push_str(combinator);
}
if let Ok(right) = complex_selector.right() {
selector_text.push_str(&right.text());
}
return selector_text;
}
return selector.text();
}
2 changes: 2 additions & 0 deletions crates/biome_css_analyze/src/options.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ pub type NoCssEmptyBlock =
pub type NoDuplicateAtImportRules = < lint :: nursery :: no_duplicate_at_import_rules :: NoDuplicateAtImportRules as biome_analyze :: Rule > :: Options ;
pub type NoDuplicateFontNames =
<lint::nursery::no_duplicate_font_names::NoDuplicateFontNames as biome_analyze::Rule>::Options;
pub type NoDuplicateSelectors =
<lint::nursery::no_duplicate_selectors::NoDuplicateSelectors as biome_analyze::Rule>::Options;
pub type NoDuplicateSelectorsKeyframeBlock = < lint :: nursery :: no_duplicate_selectors_keyframe_block :: NoDuplicateSelectorsKeyframeBlock as biome_analyze :: Rule > :: Options ;
pub type NoImportantInKeyframe = < lint :: nursery :: no_important_in_keyframe :: NoImportantInKeyframe as biome_analyze :: Rule > :: Options ;
pub type NoUnknownFunction =
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
/* valid cases: should not error */
Copy link

@Mouvedia Mouvedia May 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit

You could add a skipped/passing :is() test case for future improvements to the rule.
i.e. is:(a), a {} could be detected one day

th, td {}; tr {}
*::a {}; a {}
*::c, b {}; c {}
d e, f {}; d {}

/* duplicate within a grouping selector */
input, textarea {}; textarea {}

/* duplicate within a grouping selector, reversed */
button {}; selector, button {}

/* duplicate within a grouping selector. multiline */
span, div {};
h1, section {};
h1 {}

/* test regular case for regression */
v w, x>test {} v { w {} } x > test {}