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

refactor(css_semantic): improve CSS semantic model structure and API #4908

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ pub struct DescendingSelector {
/// ^^^^^^^
/// }
/// ```
fn find_tail_selector(selector: &AnyCssSelector) -> Option<String> {
fn find_tail_selector_str(selector: &AnyCssSelector) -> Option<String> {
match selector {
AnyCssSelector::CssCompoundSelector(s) => {
let simple = s
Expand All @@ -103,7 +103,7 @@ fn find_tail_selector(selector: &AnyCssSelector) -> Option<String> {
Some(last_selector)
}
AnyCssSelector::CssComplexSelector(s) => {
s.right().as_ref().ok().and_then(find_tail_selector)
s.right().as_ref().ok().and_then(find_tail_selector_str)
}
_ => None,
}
Expand All @@ -119,35 +119,35 @@ fn find_descending_selector(
visited_selectors: &mut FxHashMap<String, (TextRange, Specificity)>,
descending_selectors: &mut Vec<DescendingSelector>,
) {
if visited_rules.contains(&rule.id) {
if !visited_rules.insert(rule.id()) {
return;
} else {
visited_rules.insert(rule.id);
};
}

for selector in &rule.selectors {
let tail_selector = if let Some(s) = find_tail_selector(&selector.original) {
s
} else {
for selector in rule.selectors() {
let Some(casted_selector) = AnyCssSelector::cast(selector.node().clone()) else {
continue;
};
let Some(tail_selector_str) = find_tail_selector_str(&casted_selector) else {
continue;
};

if let Some((last_textrange, last_specificity)) = visited_selectors.get(&tail_selector) {
if last_specificity > &selector.specificity {
if let Some((last_text_range, last_specificity)) = visited_selectors.get(&tail_selector_str)
{
if last_specificity > &selector.specificity() {
descending_selectors.push(DescendingSelector {
high: (*last_textrange, last_specificity.clone()),
low: (selector.range, selector.specificity.clone()),
high: (*last_text_range, *last_specificity),
low: (selector.range(), selector.specificity()),
});
}
} else {
visited_selectors.insert(
tail_selector,
(selector.range, selector.specificity.clone()),
tail_selector_str,
(selector.range(), selector.specificity()),
);
}
}

for child_id in &rule.child_ids {
for child_id in rule.child_ids() {
if let Some(child_rule) = model.get_rule_by_id(*child_id) {
find_descending_selector(
child_rule,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,25 +56,25 @@ impl Rule for NoDuplicateCustomProperties {

let rule = model.get_rule_by_range(node.range())?;

let mut seen: FxHashMap<&str, TextRange> = FxHashMap::default();
let mut seen: FxHashMap<String, TextRange> = FxHashMap::default();

for declaration in rule.declarations.iter() {
for declaration in rule.declarations() {
let prop = &declaration.property;
let prop_name = prop.name.as_str();
let prop_range = prop.range;
let prop_name = prop.text();
let prop_range = prop.range();

let is_custom_property = prop_name.starts_with("--");

if !is_custom_property {
continue;
}

match seen.entry(prop_name) {
match seen.entry(prop_name.clone()) {
Entry::Occupied(entry) => {
return Some((*entry.get(), (prop_range, prop_name.to_string())));
}
Entry::Vacant(_) => {
seen.insert(prop_name, prop_range);
seen.insert(prop_name.clone(), prop_range);
}
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
use std::{borrow::Cow, collections::hash_map::Entry};
use std::collections::hash_map::Entry;

use biome_analyze::{context::RuleContext, declare_lint_rule, Rule, RuleDiagnostic, RuleSource};
use biome_console::markup;
use biome_css_syntax::CssDeclarationOrRuleList;
use biome_rowan::{AstNode, TextRange};
use biome_string_case::StrOnlyExtension;
use rustc_hash::FxHashMap;

use crate::services::semantic::Semantic;
Expand Down Expand Up @@ -55,12 +54,12 @@ impl Rule for NoDuplicateProperties {

let rule = model.get_rule_by_range(node.range())?;

let mut seen: FxHashMap<Cow<'_, str>, TextRange> = FxHashMap::default();
let mut seen: FxHashMap<String, TextRange> = FxHashMap::default();

for declaration in rule.declarations.iter() {
for declaration in rule.declarations() {
let prop = &declaration.property;
let prop_name = prop.name.to_lowercase_cow();
let prop_range = prop.range;
let prop_name = prop.text();
let prop_range = prop.range();

let is_custom_property = prop_name.starts_with("--");

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ declare_lint_rule! {
/// Disallow missing var function for css variables.
///
/// This rule has the following limitations:
/// - It only reports custom properties that are defined and accesible within the same source.
/// - It only reports custom properties that are defined and accessible within the same source.
/// - It does not check properties that can contain author-defined identifiers.
/// - It ignores the following properties:
/// - `animation`
Expand Down Expand Up @@ -164,20 +164,20 @@ impl Rule for NoMissingVarFunction {
let rule = model.get_rule_by_range(node.range())?;

if rule
.declarations
.declarations()
.iter()
.any(|decl| decl.property.name == custom_variable_name)
.any(|decl| decl.property().text() == custom_variable_name)
{
return Some(node.clone());
}

let mut parent_id = rule.parent_id;
let mut parent_id = rule.parent_id();
while let Some(id) = parent_id {
let parent_rule = model.get_rule_by_id(id)?;
if parent_rule
.declarations
.declarations()
.iter()
.any(|decl| decl.property.name == custom_variable_name)
.any(|decl| decl.property().text() == custom_variable_name)
{
return Some(node.clone());
}
Expand Down
2 changes: 1 addition & 1 deletion crates/biome_css_semantic/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ biome_rowan = { workspace = true }
rustc-hash = { workspace = true }

[dev-dependencies]
biome_css_parser = { path = "../biome_css_parser" }
# biome_css_parser = { path = "../biome_css_parser" }

[lints]
workspace = true
66 changes: 16 additions & 50 deletions crates/biome_css_semantic/src/events.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use std::{borrow::Cow, collections::VecDeque};
use std::collections::VecDeque;

use biome_css_syntax::{
AnyCssSelector, CssDeclarationBlock, CssRelativeSelector, CssSyntaxKind::*,
AnyCssSelector, CssDeclarationBlock, CssRelativeSelector, CssSyntaxKind::*, CssSyntaxNode,
};
use biome_rowan::{AstNode, SyntaxNodeCast, SyntaxNodeOptionExt, TextRange};

Expand All @@ -15,18 +15,16 @@ const ROOT_SELECTOR: &str = ":root";

#[derive(Debug)]
pub enum SemanticEvent {
RuleStart(TextRange),
RuleStart(CssSyntaxNode),
RuleEnd,
SelectorDeclaration {
name: String,
range: TextRange,
original: AnyCssSelector,
node: CssSyntaxNode,
specificity: Specificity,
},
PropertyDeclaration {
node: CssSyntaxNode,
property: CssProperty,
value: CssValue,
range: TextRange,
},
/// Indicates the start of a `:root` selector
RootSelectorStart,
Expand All @@ -45,7 +43,6 @@ pub enum SemanticEvent {
#[derive(Default, Debug)]
pub struct SemanticEventExtractor {
stash: VecDeque<SemanticEvent>,
current_rule_stack: Vec<TextRange>,
is_in_root_selector: bool,
}

Expand All @@ -68,9 +65,7 @@ impl SemanticEventExtractor {
|| kind == CSS_MEDIA_AT_RULE
|| kind == CSS_SUPPORTS_AT_RULE =>
{
let range = node.text_range();
self.stash.push_back(SemanticEvent::RuleStart(range));
self.current_rule_stack.push(range);
self.stash.push_back(SemanticEvent::RuleStart(node.clone()));
}
CSS_SELECTOR_LIST => {
if !matches!(
Expand Down Expand Up @@ -103,15 +98,11 @@ impl SemanticEventExtractor {
if let Some(property_name) = node.first_child().and_then(|p| p.first_child()) {
if let Some(value) = property_name.next_sibling() {
self.stash.push_back(SemanticEvent::PropertyDeclaration {
node: node.clone(),
property: CssProperty {
name: property_name.text_trimmed().to_string(),
range: property_name.text_trimmed_range(),
},
value: CssValue {
text: value.text_trimmed().to_string(),
range: value.text_trimmed_range(),
node: property_name,
},
range: node.text_range(),
value: CssValue { node: value },
});
Copy link
Member

Choose a reason for hiding this comment

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

imho: can be change to property_value as it aligns well with property_name?

Suggested change
value: CssValue { node: value },
value: CssValue { node: property_value },

}
}
Expand All @@ -128,12 +119,7 @@ impl SemanticEventExtractor {
match selector {
AnyCssSelector::CssComplexSelector(s) => {
let specificity = evaluate_complex_selector(&s);
self.add_selector_event(
Cow::Borrowed(&s.text()),
s.range(),
AnyCssSelector::CssComplexSelector(s),
specificity,
);
self.add_selector_event(s.into(), specificity);
}

AnyCssSelector::CssCompoundSelector(selector) => {
Expand All @@ -143,12 +129,7 @@ impl SemanticEventExtractor {
self.is_in_root_selector = true;
}
let specificity = evaluate_compound_selector(&selector);
self.add_selector_event(
Cow::Borrowed(&selector_text),
selector.range(),
AnyCssSelector::CssCompoundSelector(selector),
specificity,
)
self.add_selector_event(selector.into(), specificity)
}
_ => {}
}
Expand Down Expand Up @@ -196,10 +177,7 @@ impl SemanticEventExtractor {
if let Ok(prop_name) = prop.name() {
match prop_name.text().as_str() {
"initial-value" => {
initial_value = Some(CssValue {
text: prop.value().text().to_string(),
range: prop.value().range(),
});
initial_value = Some(CssValue { node: prop.into() });
}
"syntax" => {
syntax = Some(prop.value().text().to_string());
Expand All @@ -215,8 +193,7 @@ impl SemanticEventExtractor {

self.stash.push_back(SemanticEvent::AtProperty {
property: CssProperty {
name: property_name.text_trimmed().to_string(),
range: property_name.text_range(),
node: property_name,
},
initial_value,
syntax,
Expand All @@ -225,27 +202,16 @@ impl SemanticEventExtractor {
});
}

fn add_selector_event(
&mut self,
name: Cow<str>,
range: TextRange,
original: AnyCssSelector,
specificity: Specificity,
) {
self.stash.push_back(SemanticEvent::SelectorDeclaration {
name: name.into_owned(),
range,
original,
specificity,
});
fn add_selector_event(&mut self, node: CssSyntaxNode, specificity: Specificity) {
self.stash
.push_back(SemanticEvent::SelectorDeclaration { node, specificity });
}

pub fn leave(&mut self, node: &biome_css_syntax::CssSyntaxNode) {
if matches!(
node.kind(),
CSS_QUALIFIED_RULE | CSS_NESTED_QUALIFIED_RULE | CSS_MEDIA_AT_RULE
) {
self.current_rule_stack.pop();
self.stash.push_back(SemanticEvent::RuleEnd);
if self.is_in_root_selector {
self.stash.push_back(SemanticEvent::RootSelectorEnd);
Expand Down
Loading
Loading