From 564b9957e95928b80a7036bbf7a1488fc28a020c Mon Sep 17 00:00:00 2001 From: Abid Jappie Date: Fri, 26 Apr 2024 00:47:04 +0900 Subject: [PATCH 01/20] Bootstrapped and added basic logic and failing tests. --- .../biome_configuration/src/linter/rules.rs | 75 +++++++++------ crates/biome_css_analyze/src/lint/nursery.rs | 2 + .../lint/nursery/no_duplicate_selectors.rs | 60 ++++++++++++ crates/biome_css_analyze/src/options.rs | 2 + .../nursery/noDuplicateSelectors/invalid.css | 70 ++++++++++++++ .../noDuplicateSelectors/invalid.css.snap.new | 96 +++++++++++++++++++ .../nursery/noDuplicateSelectors/valid.css | 44 +++++++++ .../noDuplicateSelectors/valid.css.snap.new | 52 ++++++++++ .../src/categories.rs | 1 + 9 files changed, 375 insertions(+), 27 deletions(-) create mode 100644 crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs create mode 100644 crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css create mode 100644 crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css.snap.new create mode 100644 crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/valid.css create mode 100644 crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/valid.css.snap.new diff --git a/crates/biome_configuration/src/linter/rules.rs b/crates/biome_configuration/src/linter/rules.rs index 2de1341aacf..0b633ccefc4 100644 --- a/crates/biome_configuration/src/linter/rules.rs +++ b/crates/biome_configuration/src/linter/rules.rs @@ -2671,6 +2671,9 @@ pub struct Nursery { #[doc = "Disallow two keys with the same name inside a JSON object."] #[serde(skip_serializing_if = "Option::is_none")] pub no_duplicate_json_keys: Option>, + #[doc = ""] + #[serde(skip_serializing_if = "Option::is_none")] + pub no_duplicate_selectors: Option>, #[doc = "Disallow duplicate selectors within keyframe blocks."] #[serde(skip_serializing_if = "Option::is_none")] pub no_duplicate_selectors_keyframe_block: @@ -2737,6 +2740,7 @@ impl Nursery { "noDuplicateElseIf", "noDuplicateFontNames", "noDuplicateJsonKeys", + "noDuplicateSelectors", "noDuplicateSelectorsKeyframeBlock", "noEvolvingAny", "noFlatMapIdentity", @@ -2757,6 +2761,7 @@ impl Nursery { "noDuplicateElseIf", "noDuplicateFontNames", "noDuplicateJsonKeys", + "noDuplicateSelectors", "noDuplicateSelectorsKeyframeBlock", "noEvolvingAny", "noFlatMapIdentity", @@ -2773,7 +2778,8 @@ impl Nursery { RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[9]), RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[10]), RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[11]), - RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[17]), + RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[12]), + RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[18]), ]; const ALL_RULES_AS_FILTERS: &'static [RuleFilter<'static>] = &[ RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[0]), @@ -2797,6 +2803,7 @@ impl Nursery { RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[18]), RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[19]), RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[20]), + RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[21]), ]; #[doc = r" Retrieves the recommended rules"] pub(crate) fn is_recommended_true(&self) -> bool { @@ -2853,71 +2860,76 @@ impl Nursery { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[7])); } } - if let Some(rule) = self.no_duplicate_selectors_keyframe_block.as_ref() { + if let Some(rule) = self.no_duplicate_selectors.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[8])); } } - if let Some(rule) = self.no_evolving_any.as_ref() { + if let Some(rule) = self.no_duplicate_selectors_keyframe_block.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[9])); } } - if let Some(rule) = self.no_flat_map_identity.as_ref() { + if let Some(rule) = self.no_evolving_any.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[10])); } } - if let Some(rule) = self.no_important_in_keyframe.as_ref() { + if let Some(rule) = self.no_flat_map_identity.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[11])); } } - if let Some(rule) = self.no_misplaced_assertion.as_ref() { + if let Some(rule) = self.no_important_in_keyframe.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[12])); } } - if let Some(rule) = self.no_nodejs_modules.as_ref() { + if let Some(rule) = self.no_misplaced_assertion.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[13])); } } - if let Some(rule) = self.no_react_specific_props.as_ref() { + if let Some(rule) = self.no_nodejs_modules.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[14])); } } - if let Some(rule) = self.no_restricted_imports.as_ref() { + if let Some(rule) = self.no_react_specific_props.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[15])); } } - if let Some(rule) = self.no_undeclared_dependencies.as_ref() { + if let Some(rule) = self.no_restricted_imports.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[16])); } } - if let Some(rule) = self.no_unknown_unit.as_ref() { + if let Some(rule) = self.no_undeclared_dependencies.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[17])); } } - if let Some(rule) = self.use_consistent_new_builtin.as_ref() { + if let Some(rule) = self.no_unknown_unit.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[18])); } } - if let Some(rule) = self.use_import_restrictions.as_ref() { + if let Some(rule) = self.use_consistent_new_builtin.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[19])); } } - if let Some(rule) = self.use_sorted_classes.as_ref() { + if let Some(rule) = self.use_import_restrictions.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[20])); } } + if let Some(rule) = self.use_sorted_classes.as_ref() { + if rule.is_enabled() { + index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[21])); + } + } index_set } pub(crate) fn get_disabled_rules(&self) -> IndexSet { @@ -2962,71 +2974,76 @@ impl Nursery { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[7])); } } - if let Some(rule) = self.no_duplicate_selectors_keyframe_block.as_ref() { + if let Some(rule) = self.no_duplicate_selectors.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[8])); } } - if let Some(rule) = self.no_evolving_any.as_ref() { + if let Some(rule) = self.no_duplicate_selectors_keyframe_block.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[9])); } } - if let Some(rule) = self.no_flat_map_identity.as_ref() { + if let Some(rule) = self.no_evolving_any.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[10])); } } - if let Some(rule) = self.no_important_in_keyframe.as_ref() { + if let Some(rule) = self.no_flat_map_identity.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[11])); } } - if let Some(rule) = self.no_misplaced_assertion.as_ref() { + if let Some(rule) = self.no_important_in_keyframe.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[12])); } } - if let Some(rule) = self.no_nodejs_modules.as_ref() { + if let Some(rule) = self.no_misplaced_assertion.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[13])); } } - if let Some(rule) = self.no_react_specific_props.as_ref() { + if let Some(rule) = self.no_nodejs_modules.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[14])); } } - if let Some(rule) = self.no_restricted_imports.as_ref() { + if let Some(rule) = self.no_react_specific_props.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[15])); } } - if let Some(rule) = self.no_undeclared_dependencies.as_ref() { + if let Some(rule) = self.no_restricted_imports.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[16])); } } - if let Some(rule) = self.no_unknown_unit.as_ref() { + if let Some(rule) = self.no_undeclared_dependencies.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[17])); } } - if let Some(rule) = self.use_consistent_new_builtin.as_ref() { + if let Some(rule) = self.no_unknown_unit.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[18])); } } - if let Some(rule) = self.use_import_restrictions.as_ref() { + if let Some(rule) = self.use_consistent_new_builtin.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[19])); } } - if let Some(rule) = self.use_sorted_classes.as_ref() { + if let Some(rule) = self.use_import_restrictions.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[20])); } } + if let Some(rule) = self.use_sorted_classes.as_ref() { + if rule.is_disabled() { + index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[21])); + } + } index_set } #[doc = r" Checks if, given a rule name, matches one of the rules contained in this category"] @@ -3095,6 +3112,10 @@ impl Nursery { .no_duplicate_json_keys .as_ref() .map(|conf| (conf.level(), conf.get_options())), + "noDuplicateSelectors" => self + .no_duplicate_selectors_keyframe_block + .as_ref() + .map(|conf| (conf.level(), conf.get_options())), "noDuplicateSelectorsKeyframeBlock" => self .no_duplicate_selectors_keyframe_block .as_ref() diff --git a/crates/biome_css_analyze/src/lint/nursery.rs b/crates/biome_css_analyze/src/lint/nursery.rs index 6daa2ac3394..cb5c0f05ee7 100644 --- a/crates/biome_css_analyze/src/lint/nursery.rs +++ b/crates/biome_css_analyze/src/lint/nursery.rs @@ -5,6 +5,7 @@ use biome_analyze::declare_group; pub mod no_color_invalid_hex; pub mod no_css_empty_block; 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_unit; @@ -16,6 +17,7 @@ declare_group! { self :: no_color_invalid_hex :: NoColorInvalidHex , self :: no_css_empty_block :: NoCssEmptyBlock , 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_unit :: NoUnknownUnit , diff --git a/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs b/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs new file mode 100644 index 00000000000..dcc36778f3f --- /dev/null +++ b/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs @@ -0,0 +1,60 @@ +use std::collections::HashSet; + +use biome_analyze::{context::RuleContext, declare_rule, Ast, Rule, RuleDiagnostic, RuleSource}; +use biome_console::markup; +use biome_css_syntax::{ AnyCssSelector, CssSelectorList}; +use biome_rowan::AstNode; + +declare_rule! { + /// + /// + pub NoDuplicateSelectors { + version: "next", + name: "noDuplicateSelectors", + recommended: true, + sources: &[RuleSource::Stylelint("no-duplicate-selectors")], + } +} + +impl Rule for NoDuplicateSelectors { + type Query = Ast; + type State = AnyCssSelector; + type Signals = Option; + type Options = (); + + fn run(ctx: &RuleContext) -> Option { + let node = ctx.query(); + let mut selector_list = HashSet::new(); + + for selector in node { + let valid_selector = selector.ok()?; + if !selector_list.insert(valid_selector.text()) { + return Some(valid_selector) + } + } + + + None + } + + fn diagnostic(_: &RuleContext, node: &Self::State) -> Option { + // + // Read our guidelines to write great diagnostics: + // https://docs.rs/biome_analyze/latest/biome_analyze/#what-a-rule-should-say-to-the-user + // + let span = node.range(); + Some( + RuleDiagnostic::new( + rule_category!(), + span, + markup! { + // TODO: Update this + "Unexpected duplicate selector:" {node.text()} + }, + ) + .note(markup! { + "This note will give you more information." + }), + ) + } +} diff --git a/crates/biome_css_analyze/src/options.rs b/crates/biome_css_analyze/src/options.rs index 0be8ac2a7af..1bdea18e4d3 100644 --- a/crates/biome_css_analyze/src/options.rs +++ b/crates/biome_css_analyze/src/options.rs @@ -8,6 +8,8 @@ pub type NoCssEmptyBlock = ::Options; pub type NoDuplicateFontNames = ::Options; +pub type NoDuplicateSelectors = + ::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 NoUnknownUnit = diff --git a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css new file mode 100644 index 00000000000..b4b64e20d0c --- /dev/null +++ b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css @@ -0,0 +1,70 @@ +.foo, +.bar, +.foo { + width: 12; +} + +.foo { + width: 12; +} + + +.bar { + width: 12; +} + +.foo { + width: 12; +} + +.foo .bar { + width: 12; +} + +.bar { + width: 12; +} + +.foo .bar { + width: 12; +} + +@media (min-width: 10px) { + .foo { + width: 12; + } + + .foo { + width: 12; + } +} + +.foo, +.bar { + width: 12; +} + +.bar, +.foo { + width: 12; +} + +a .foo, +b+.bar { + width: 12; +} + +b+.bar, +a .foo { + width: 12; +} + +a b { + width: 12; +} + +a { + & b { + width: 12; + } +} \ No newline at end of file diff --git a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css.snap.new b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css.snap.new new file mode 100644 index 00000000000..36f652ac72b --- /dev/null +++ b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css.snap.new @@ -0,0 +1,96 @@ +--- +source: crates/biome_css_analyze/tests/spec_tests.rs +assertion_line: 83 +expression: invalid.css +--- +# Input +```css +.foo, +.bar, +.foo { + width: 12; +} + +.foo { + width: 12; +} + + +.bar { + width: 12; +} + +.foo { + width: 12; +} + +.foo .bar { + width: 12; +} + +.bar { + width: 12; +} + +.foo .bar { + width: 12; +} + +@media (min-width: 10px) { + .foo { + width: 12; + } + + .foo { + width: 12; + } +} + +.foo, +.bar { + width: 12; +} + +.bar, +.foo { + width: 12; +} + +a .foo, +b+.bar { + width: 12; +} + +b+.bar, +a .foo { + width: 12; +} + +a b { + width: 12; +} + +a { + & b { + width: 12; + } +} +``` + +# Diagnostics +``` +invalid.css:3:1 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Unexpected duplicate selector:.foo + + 1 │ .foo, + 2 │ .bar, + > 3 │ .foo { + │ ^^^^ + 4 │ width: 12; + 5 │ } + + i This note will give you more information. + + +``` diff --git a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/valid.css b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/valid.css new file mode 100644 index 00000000000..43bf259ed8c --- /dev/null +++ b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/valid.css @@ -0,0 +1,44 @@ +.foo { + width: 12; +} + +@media (min-width: 10px) { + .foo { + width: 12; + } +} + +/* +.foo { + .foo { + width: 12; + } +} + +.foo { + width: 12; +} + +.bar { + width: 12; +} + +.foo .bar { + width: 12; +} + +.bar .foo { + width: 12; +} + +a b { + width: 12; +} + +a { + + & b, + & c { + width: 12; + } +} */ \ No newline at end of file diff --git a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/valid.css.snap.new b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/valid.css.snap.new new file mode 100644 index 00000000000..f09e4481787 --- /dev/null +++ b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/valid.css.snap.new @@ -0,0 +1,52 @@ +--- +source: crates/biome_css_analyze/tests/spec_tests.rs +assertion_line: 83 +expression: valid.css +--- +# Input +```css +.foo { + width: 12; +} + +@media (min-width: 10px) { + .foo { + width: 12; + } +} + +/* +.foo { + .foo { + width: 12; + } +} + +.foo { + width: 12; +} + +.bar { + width: 12; +} + +.foo .bar { + width: 12; +} + +.bar .foo { + width: 12; +} + +a b { + width: 12; +} + +a { + + & b, + & c { + width: 12; + } +} */ +``` diff --git a/crates/biome_diagnostics_categories/src/categories.rs b/crates/biome_diagnostics_categories/src/categories.rs index 8633f8b7fc1..64562caa345 100644 --- a/crates/biome_diagnostics_categories/src/categories.rs +++ b/crates/biome_diagnostics_categories/src/categories.rs @@ -118,6 +118,7 @@ define_categories! { "lint/nursery/noDuplicateElseIf": "https://biomejs.dev/linter/rules/no-duplicate-else-if", "lint/nursery/noDuplicateFontNames": "https://biomejs.dev/linter/rules/no-font-family-duplicate-names", "lint/nursery/noDuplicateJsonKeys": "https://biomejs.dev/linter/rules/no-duplicate-json-keys", + "lint/nursery/noDuplicateSelectors": "https://biomejs.dev/linter/rules/no-duplicate-selectors", "lint/nursery/noDuplicateSelectorsKeyframeBlock": "https://biomejs.dev/linter/rules/no-duplicate-selectors-keyframe-block", "lint/nursery/noEvolvingAny": "https://biomejs.dev/linter/rules/no-evolving-any", "lint/nursery/noFlatMapIdentity": "https://biomejs.dev/linter/rules/no-flat-map-identity", From 08c949eec9c293507df20f3e2805e826bd82efac Mon Sep 17 00:00:00 2001 From: Abid Jappie Date: Fri, 26 Apr 2024 01:23:00 +0900 Subject: [PATCH 02/20] Added some helpful comments and save progress. --- .../lint/nursery/no_duplicate_selectors.rs | 4 +- .../nursery/noDuplicateSelectors/invalid.css | 50 ++++---------- .../noDuplicateSelectors/invalid.css.snap.new | 66 +++++++------------ .../nursery/noDuplicateSelectors/valid.css | 22 ++----- .../noDuplicateSelectors/valid.css.snap.new | 22 ++----- 5 files changed, 50 insertions(+), 114 deletions(-) diff --git a/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs b/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs index dcc36778f3f..51d7fac9cf2 100644 --- a/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs +++ b/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs @@ -22,6 +22,8 @@ impl Rule for NoDuplicateSelectors { type Signals = Option; type Options = (); + // TODO: Should allow duplicate in list (option) + // TODO: Traverse and compare with the entire sheet fn run(ctx: &RuleContext) -> Option { let node = ctx.query(); let mut selector_list = HashSet::new(); @@ -32,8 +34,6 @@ impl Rule for NoDuplicateSelectors { return Some(valid_selector) } } - - None } diff --git a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css index b4b64e20d0c..54b445145a7 100644 --- a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css +++ b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css @@ -1,34 +1,31 @@ -.foo, -.bar, -.foo { +/* Duplication of single selectors in a rule's selector list */ +.alpha, +.beta, +.alpha { width: 12; } -.foo { - width: 12; -} - - -.bar { - width: 12; -} - -.foo { +/* Duplication of selector lists within the style sheet */ +/* Duplication is found even if the selectors have different ordering */ +.foo .bar { width: 12; } -.foo .bar { +.bar .foo { width: 12; } -.bar { +a .can, +b+.dle { width: 12; } -.foo .bar { +b+.dle, +a .can { width: 12; } +/* Duplication within @ rule */ @media (min-width: 10px) { .foo { width: 12; @@ -39,26 +36,7 @@ } } -.foo, -.bar { - width: 12; -} - -.bar, -.foo { - width: 12; -} - -a .foo, -b+.bar { - width: 12; -} - -b+.bar, -a .foo { - width: 12; -} - +/* Duplication when the net result of nesting is the same */ a b { width: 12; } diff --git a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css.snap.new b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css.snap.new index 36f652ac72b..71f4e126e74 100644 --- a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css.snap.new +++ b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css.snap.new @@ -5,37 +5,34 @@ expression: invalid.css --- # Input ```css -.foo, -.bar, -.foo { +/* Duplication of single selectors in a rule's selector list */ +.alpha, +.beta, +.alpha { width: 12; } -.foo { - width: 12; -} - - -.bar { - width: 12; -} - -.foo { +/* Duplication of selector lists within the style sheet */ +/* Duplication is found even if the selectors have different ordering */ +.foo .bar { width: 12; } -.foo .bar { +.bar .foo { width: 12; } -.bar { +a .can, +b+.dle { width: 12; } -.foo .bar { +b+.dle, +a .can { width: 12; } +/* Duplication within @ rule */ @media (min-width: 10px) { .foo { width: 12; @@ -46,26 +43,7 @@ expression: invalid.css } } -.foo, -.bar { - width: 12; -} - -.bar, -.foo { - width: 12; -} - -a .foo, -b+.bar { - width: 12; -} - -b+.bar, -a .foo { - width: 12; -} - +/* Duplication when the net result of nesting is the same */ a b { width: 12; } @@ -79,16 +57,16 @@ a { # Diagnostics ``` -invalid.css:3:1 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +invalid.css:4:1 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Unexpected duplicate selector:.foo + ! Unexpected duplicate selector:.alpha - 1 │ .foo, - 2 │ .bar, - > 3 │ .foo { - │ ^^^^ - 4 │ width: 12; - 5 │ } + 2 │ .alpha, + 3 │ .beta, + > 4 │ .alpha { + │ ^^^^^^ + 5 │ width: 12; + 6 │ } i This note will give you more information. diff --git a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/valid.css b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/valid.css index 43bf259ed8c..f258dc7301f 100644 --- a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/valid.css +++ b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/valid.css @@ -1,3 +1,4 @@ +/* Same selector with different parent nodes are allowed */ .foo { width: 12; } @@ -8,29 +9,18 @@ } } -/* -.foo { - .foo { +.bar { + .bar { width: 12; } } -.foo { - width: 12; -} - -.bar { - width: 12; -} - +/* Same selector in a different list */ .foo .bar { width: 12; } -.bar .foo { - width: 12; -} - +/* Net list is different */ a b { width: 12; } @@ -41,4 +31,4 @@ a { & c { width: 12; } -} */ \ No newline at end of file +} \ No newline at end of file diff --git a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/valid.css.snap.new b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/valid.css.snap.new index f09e4481787..a086c6fb111 100644 --- a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/valid.css.snap.new +++ b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/valid.css.snap.new @@ -5,6 +5,7 @@ expression: valid.css --- # Input ```css +/* Same selector with different parent nodes are allowed */ .foo { width: 12; } @@ -15,29 +16,18 @@ expression: valid.css } } -/* -.foo { - .foo { +.bar { + .bar { width: 12; } } -.foo { - width: 12; -} - -.bar { - width: 12; -} - +/* Same selector in a different list */ .foo .bar { width: 12; } -.bar .foo { - width: 12; -} - +/* Net list is different */ a b { width: 12; } @@ -48,5 +38,5 @@ a { & c { width: 12; } -} */ +} ``` From c4674afc15c0ff76cbe35af8e584c514cf61511c Mon Sep 17 00:00:00 2001 From: Abid Jappie Date: Tue, 30 Apr 2024 09:01:18 +0900 Subject: [PATCH 03/20] WIP: it currently returns everything in a single run, we need to split it into multiple runs. --- .../lint/nursery/no_duplicate_selectors.rs | 251 ++++++++++++++++-- 1 file changed, 226 insertions(+), 25 deletions(-) diff --git a/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs b/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs index 51d7fac9cf2..443e33e32d3 100644 --- a/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs +++ b/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs @@ -1,9 +1,13 @@ -use std::collections::HashSet; +use std::borrow::Borrow; +use std::hash::Hash; +use std::{collections::HashSet, io}; +use std::{println, vec}; +use biome_analyze::{AddVisitor, Phases, QueryMatch, Queryable, ServiceBag, Visitor, VisitorContext}; use biome_analyze::{context::RuleContext, declare_rule, Ast, Rule, RuleDiagnostic, RuleSource}; use biome_console::markup; -use biome_css_syntax::{ AnyCssSelector, CssSelectorList}; -use biome_rowan::AstNode; +use biome_css_syntax::{AnyCssDeclarationOrRule, AnyCssRelativeSelector, AnyCssRule, AnyCssSelector, AnyCssSubSelector, CssBogus, CssComplexSelector, CssCompoundSelector, CssCustomIdentifier, CssDeclarationOrRuleBlock, CssIdentifier, CssLanguage, CssNestedQualifiedRule, CssQualifiedRule, CssRelativeSelector, CssRelativeSelectorList, CssRoot, CssRuleList, CssSelectorList, CssSyntaxElement, CssSyntaxNode}; +use biome_rowan::{AstNode, Language, SyntaxNode, TextRange, WalkEvent}; declare_rule! { /// @@ -16,24 +20,219 @@ declare_rule! { } } +#[derive(Default)] + +struct CssRuleSelectorListVisitor { + stack: ( + Vec, + Vec>, + Vec>> + ), // Current group, total group, total for all rules +} + +impl Visitor for CssRuleSelectorListVisitor { + type Language = CssLanguage; + + fn visit( + &mut self, + event: &WalkEvent>, + mut ctx: VisitorContext, + ) { + match event { + WalkEvent::Enter(node) => { + if let Some(node) = CssSelectorList::cast_ref(node) { + // Push to the current group + for selector in node { + match selector { + Ok(result) => { + self.stack.0.push(result); + }, + Err(_) => todo!(), + } + } + } + if let Some(node) = CssRelativeSelector::cast_ref(node) { + let selector = node.selector(); + match selector { + Ok(result) => self.stack.0.push(result), + Err(_) => todo!(), + } + } + // Instead of a NQR we use a DRB because otherwise we will have dangling stack0 + if let Some(_node) = CssDeclarationOrRuleBlock::cast_ref(node) { + // Push the current group to the total group + self.stack.1.push(self.stack.0.clone()); + self.stack.0.clear(); + } + }, + WalkEvent::Leave(node) => { + + // End of rule, clear the stack + if let Some(_node) = CssQualifiedRule::cast_ref(node) { + self.stack.2.push(self.stack.1.clone()); + self.stack.1.clear(); + self.stack.0.clear(); + } + if let Some(_node) = CssNestedQualifiedRule::cast_ref(node) { + self.stack.2.push(self.stack.1.clone()); + // Clear the last group from the rule + self.stack.1.pop(); + } + if let Some(_node) = CssRuleList::cast_ref(node) { + ctx.match_query(CssRuleSelectorList(self.stack.2.clone())); + } + }, + } + } + +} + +pub struct CssRuleSelectorList(Vec>>); + +impl QueryMatch for CssRuleSelectorList { + fn text_range(&self) -> TextRange { + todo!() + } +} + +impl Queryable for CssRuleSelectorList { + type Input = Self; + type Language = CssLanguage; + type Output = Vec>>; + type Services = (); + + fn build_visitor( + analyzer: &mut impl AddVisitor, + _: &::Root, + ) { + // Register our custom visitor to run in the `Syntax` phase + analyzer.add_visitor(Phases::Syntax, CssRuleSelectorListVisitor::default); + } + + // Extract the output object from the input type + fn unwrap_match(services: &ServiceBag, query: &Self::Input) -> Self::Output { + query.0.clone() + } + + +} + impl Rule for NoDuplicateSelectors { - type Query = Ast; - type State = AnyCssSelector; + type Query = CssRuleSelectorList; + type State = Vec; type Signals = Option; type Options = (); - // TODO: Should allow duplicate in list (option) - // TODO: Traverse and compare with the entire sheet fn run(ctx: &RuleContext) -> Option { let node = ctx.query(); - let mut selector_list = HashSet::new(); - for selector in node { - let valid_selector = selector.ok()?; - if !selector_list.insert(valid_selector.text()) { - return Some(valid_selector) + println!("Run result:"); + let mut temp_state: Vec = vec!(); + for rule in node { + let mut paths:Vec = vec!(); + + for group in rule { + let mut group = group.clone(); + group.sort_by_key(|sel| sel.text()); + + if paths.len() == 0 { + for selector in group.clone().into_iter() { + // sort sub selectors + match selector { + AnyCssSelector::CssCompoundSelector(compound_selector) => { + let mut prefix = String::new(); + + if let Some(simple_selector) = compound_selector.simple_selector(){ + prefix = simple_selector.text(); + } + let text_list = compound_selector + .sub_selectors() + .into_iter() + .map(|sub_selector| { sub_selector.text() }); + let mut text_list: Vec = text_list.collect(); + text_list.sort(); + + let final_path = prefix + &text_list.join(""); + if paths.contains(&final_path) { + temp_state.push(compound_selector.into()); + } + paths.push(final_path) + }, + AnyCssSelector::CssComplexSelector(complex_selector) => { + // TODO: this is a brute force approach + // We can do better to join based on whether there is a simple selector or not + let complex_as_string = complex_selector.text(); + let mut complex_as_list: Vec = complex_as_string.split(" ").map(|s| s.to_string()).collect(); + + complex_as_list.sort(); + + let final_path = complex_as_list.join(" "); + if paths.contains(&final_path) { + temp_state.push(complex_selector.into()); + } + paths.push(final_path); + }, + _ => { + paths.push(selector.text()); + } + } + } + } else { + let path_clone = paths.clone(); + paths.clear(); + for path in path_clone { + for selector in group.clone().into_iter() { + if let Some(compound_selector) = CssCompoundSelector::cast_ref(selector.syntax()) { + // Handle sub selector sorting + let mut prefix = "".to_string(); + if let Some(simple_selector) = compound_selector.simple_selector(){ + prefix = simple_selector.text(); + } + let text_list = compound_selector + .sub_selectors() + .into_iter() + .map(|sub_selector| { sub_selector.text() }); + let mut text_list: Vec = text_list.collect(); + text_list.sort(); + // Handle the ampersand + if let Some(_nesting_selector_token) = compound_selector.nesting_selector_token() { + let new_path = path.clone() + &prefix + &text_list.join(""); + let final_path = new_path.replace("&",""); + if paths.contains(&final_path) { + temp_state.push(selector.clone()); + } + paths.push(final_path); + continue; + } + let final_path = path.clone() + " " + &prefix + &text_list.join(""); + if paths.contains(&final_path) { + temp_state.push(selector.clone()); + } + paths.push(final_path); + + continue; + } + // sort sub selectors + + let final_path = path.clone() + " " + &selector.text(); + if paths.contains(&final_path) { + temp_state.push(selector.clone()); + } + paths.push(final_path); + } + } + } + } + + for path in paths { + println!("{path}") } } + + if temp_state.len() != 0 { + return Some(temp_state); + } + None } @@ -42,19 +241,21 @@ impl Rule for NoDuplicateSelectors { // Read our guidelines to write great diagnostics: // https://docs.rs/biome_analyze/latest/biome_analyze/#what-a-rule-should-say-to-the-user // - let span = node.range(); - Some( - RuleDiagnostic::new( - rule_category!(), - span, - markup! { - // TODO: Update this - "Unexpected duplicate selector:" {node.text()} - }, + for n in node.into_iter() { + let n_syn = n.clone().into_syntax(); + return Some( + RuleDiagnostic::new( + rule_category!(), + n_syn.text_range(), + markup! { + "Unexpected duplicate selector" {n_syn.to_string()} + }, + ) + .note(markup! { + "This note will give you more information." + }), ) - .note(markup! { - "This note will give you more information." - }), - ) + } + None } } From 99af5a2b57035b71c85402ce1f12c4e57163c81b Mon Sep 17 00:00:00 2001 From: Abid Jappie Date: Wed, 1 May 2024 00:55:58 +0900 Subject: [PATCH 04/20] Working solution excluding some edge cases, still using a visitor needs to be simplified. --- .../lint/nursery/no_duplicate_selectors.rs | 389 ++++++++++-------- .../noDuplicateSelectors/invalid.css.snap.new | 91 +++- .../noDuplicateSelectors/valid.css.snap.new | 20 + 3 files changed, 312 insertions(+), 188 deletions(-) diff --git a/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs b/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs index 443e33e32d3..5a8ec460bbe 100644 --- a/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs +++ b/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs @@ -1,12 +1,10 @@ -use std::borrow::Borrow; -use std::hash::Hash; -use std::{collections::HashSet, io}; -use std::{println, vec}; +use std::collections::HashSet; +use std::vec; use biome_analyze::{AddVisitor, Phases, QueryMatch, Queryable, ServiceBag, Visitor, VisitorContext}; -use biome_analyze::{context::RuleContext, declare_rule, Ast, Rule, RuleDiagnostic, RuleSource}; +use biome_analyze::{context::RuleContext, declare_rule, Rule, RuleDiagnostic, RuleSource}; use biome_console::markup; -use biome_css_syntax::{AnyCssDeclarationOrRule, AnyCssRelativeSelector, AnyCssRule, AnyCssSelector, AnyCssSubSelector, CssBogus, CssComplexSelector, CssCompoundSelector, CssCustomIdentifier, CssDeclarationOrRuleBlock, CssIdentifier, CssLanguage, CssNestedQualifiedRule, CssQualifiedRule, CssRelativeSelector, CssRelativeSelectorList, CssRoot, CssRuleList, CssSelectorList, CssSyntaxElement, CssSyntaxNode}; +use biome_css_syntax::{AnyCssRule, AnyCssSelector, CssComplexSelector, CssCompoundSelector, CssDeclarationOrRuleBlock, CssLanguage, CssNestedQualifiedRule, CssQualifiedRule, CssRelativeSelector, CssRuleList}; use biome_rowan::{AstNode, Language, SyntaxNode, TextRange, WalkEvent}; declare_rule! { @@ -21,241 +19,266 @@ declare_rule! { } #[derive(Default)] - -struct CssRuleSelectorListVisitor { - stack: ( - Vec, - Vec>, - Vec>> - ), // Current group, total group, total for all rules +struct DeclarationOrRuleBlockListVisitor { + stack: Vec } -impl Visitor for CssRuleSelectorListVisitor { +impl Visitor for DeclarationOrRuleBlockListVisitor { type Language = CssLanguage; fn visit( &mut self, event: &WalkEvent>, - mut ctx: VisitorContext, + mut ctx: VisitorContext ) { match event { WalkEvent::Enter(node) => { - if let Some(node) = CssSelectorList::cast_ref(node) { - // Push to the current group - for selector in node { - match selector { - Ok(result) => { - self.stack.0.push(result); - }, - Err(_) => todo!(), - } - } - } - if let Some(node) = CssRelativeSelector::cast_ref(node) { - let selector = node.selector(); - match selector { - Ok(result) => self.stack.0.push(result), - Err(_) => todo!(), - } + if let Some(node) = CssDeclarationOrRuleBlock::cast_ref(node) { + self.stack.push(node); } - // Instead of a NQR we use a DRB because otherwise we will have dangling stack0 - if let Some(_node) = CssDeclarationOrRuleBlock::cast_ref(node) { - // Push the current group to the total group - self.stack.1.push(self.stack.0.clone()); - self.stack.0.clear(); - } - }, + } WalkEvent::Leave(node) => { - - // End of rule, clear the stack - if let Some(_node) = CssQualifiedRule::cast_ref(node) { - self.stack.2.push(self.stack.1.clone()); - self.stack.1.clear(); - self.stack.0.clear(); - } - if let Some(_node) = CssNestedQualifiedRule::cast_ref(node) { - self.stack.2.push(self.stack.1.clone()); - // Clear the last group from the rule - self.stack.1.pop(); - } if let Some(_node) = CssRuleList::cast_ref(node) { - ctx.match_query(CssRuleSelectorList(self.stack.2.clone())); + ctx.match_query(DeclarationOrRuleBlockList(self.stack.clone())); + self.stack.clear(); } - }, + } } } - } -pub struct CssRuleSelectorList(Vec>>); +pub struct DeclarationOrRuleBlockList(Vec); -impl QueryMatch for CssRuleSelectorList { +impl QueryMatch for DeclarationOrRuleBlockList { fn text_range(&self) -> TextRange { todo!() } } -impl Queryable for CssRuleSelectorList { +impl Queryable for DeclarationOrRuleBlockList { type Input = Self; + type Output = Vec; type Language = CssLanguage; - type Output = Vec>>; type Services = (); fn build_visitor( analyzer: &mut impl AddVisitor, - _: &::Root, + _root: &::Root, ) { - // Register our custom visitor to run in the `Syntax` phase - analyzer.add_visitor(Phases::Syntax, CssRuleSelectorListVisitor::default); + analyzer.add_visitor(Phases::Syntax, DeclarationOrRuleBlockListVisitor::default); } - // Extract the output object from the input type - fn unwrap_match(services: &ServiceBag, query: &Self::Input) -> Self::Output { + fn unwrap_match(_services: &ServiceBag, query: &Self::Input) -> Self::Output { query.0.clone() } - - } impl Rule for NoDuplicateSelectors { - type Query = CssRuleSelectorList; - type State = Vec; - type Signals = Option; + type Query = DeclarationOrRuleBlockList; + type State = SyntaxNode; + type Signals = Vec; type Options = (); - fn run(ctx: &RuleContext) -> Option { - let node = ctx.query(); + fn run(ctx: &RuleContext) -> Vec { + let node_list = ctx.query(); - println!("Run result:"); - let mut temp_state: Vec = vec!(); - for rule in node { - let mut paths:Vec = vec!(); + let mut select_list = HashSet::new(); + let mut output: Vec> = vec!(); - for group in rule { - let mut group = group.clone(); - group.sort_by_key(|sel| sel.text()); + for node in node_list { + if let Some(this_rule) = node.syntax().parent() { + let handled_rule = handle_css_rule(this_rule); + for (selector_node, selector) in handled_rule { + let resolved_selectors = resolve_nested_selectors(selector, node.clone()); + for resolved in resolved_selectors { + if !select_list.insert(resolved) { + output.push(selector_node.clone()); + } + } + } + } + } - if paths.len() == 0 { - for selector in group.clone().into_iter() { - // sort sub selectors - match selector { - AnyCssSelector::CssCompoundSelector(compound_selector) => { - let mut prefix = String::new(); + output + } - if let Some(simple_selector) = compound_selector.simple_selector(){ - prefix = simple_selector.text(); - } - let text_list = compound_selector - .sub_selectors() - .into_iter() - .map(|sub_selector| { sub_selector.text() }); - let mut text_list: Vec = text_list.collect(); - text_list.sort(); - - let final_path = prefix + &text_list.join(""); - if paths.contains(&final_path) { - temp_state.push(compound_selector.into()); - } - paths.push(final_path) - }, - AnyCssSelector::CssComplexSelector(complex_selector) => { - // TODO: this is a brute force approach - // We can do better to join based on whether there is a simple selector or not - let complex_as_string = complex_selector.text(); - let mut complex_as_list: Vec = complex_as_string.split(" ").map(|s| s.to_string()).collect(); - - complex_as_list.sort(); + fn diagnostic(_: &RuleContext, node: &Self::State) -> Option { + // + // Read our guidelines to write great diagnostics: + // https://docs.rs/biome_analyze/latest/biome_analyze/#what-a-rule-should-say-to-the-user + // + Some( + RuleDiagnostic::new( + rule_category!(), + node.text_range(), + markup! { + "Duplicate selector." + }, + ).note(markup! { + "Please fix it." + }), + ) + } +} - let final_path = complex_as_list.join(" "); - if paths.contains(&final_path) { - temp_state.push(complex_selector.into()); - } - paths.push(final_path); - }, - _ => { - paths.push(selector.text()); - } - } - } - } else { - let path_clone = paths.clone(); - paths.clear(); - for path in path_clone { - for selector in group.clone().into_iter() { - if let Some(compound_selector) = CssCompoundSelector::cast_ref(selector.syntax()) { - // Handle sub selector sorting - let mut prefix = "".to_string(); - if let Some(simple_selector) = compound_selector.simple_selector(){ - prefix = simple_selector.text(); - } - let text_list = compound_selector - .sub_selectors() - .into_iter() - .map(|sub_selector| { sub_selector.text() }); - let mut text_list: Vec = text_list.collect(); - text_list.sort(); - // Handle the ampersand - if let Some(_nesting_selector_token) = compound_selector.nesting_selector_token() { - let new_path = path.clone() + &prefix + &text_list.join(""); - let final_path = new_path.replace("&",""); - if paths.contains(&final_path) { - temp_state.push(selector.clone()); - } - paths.push(final_path); - continue; - } - let final_path = path.clone() + " " + &prefix + &text_list.join(""); - if paths.contains(&final_path) { - temp_state.push(selector.clone()); - } - paths.push(final_path); +// TODO: need to handle AtRules etc. +fn resolve_nested_selectors(selector: String, node: CssDeclarationOrRuleBlock) -> Vec { + let mut parent_selectors: Vec = vec!(); - continue; - } - // sort sub selectors + if let Some(this_rule) = node.syntax().parent() { + if let Some(_qualified_rule) = CssQualifiedRule::cast_ref(&this_rule) { + // Highest Level + return vec!(selector); + } + + let parent_node = get_parent_block(this_rule); - let final_path = path.clone() + " " + &selector.text(); - if paths.contains(&final_path) { - temp_state.push(selector.clone()); - } - paths.push(final_path); + if let Some(parent_block) = parent_node.clone() { + if let Some(parent_node_parent) = parent_block.into_syntax().parent() { + if let Some(parent_rule) = CssQualifiedRule::cast_ref(&parent_node_parent) { + for selector in parent_rule.prelude() { + if let Ok(selector) = selector { + parent_selectors.push(handle_css_selector(selector.into())); + } + } + } + if let Some(parent_rule) = CssNestedQualifiedRule::cast_ref(&parent_node_parent) { + for selector in parent_rule.prelude() { + if let Ok(selector) = selector { + parent_selectors.push(handle_css_selector(selector.into())); } } } } - for path in paths { - println!("{path}") - } + let resolved_selectors: Vec = parent_selectors.iter().fold(vec!(), |result: Vec, parent_selector|{ + match &parent_node { + Some(parent_node) => { + if selector.contains("&") { + let resolved_parent_selectors = resolve_nested_selectors(parent_selector.to_string(), parent_node.clone()); + let resolved = resolved_parent_selectors.into_iter().map(|newly_resolved|{ + // TODO: Need to handle the case where an equivalent is the result of an & + // e.g. .a.c { &.b } == .a.b.c but the order will be a.c.b + 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_node.clone()); + return [result, resolved].concat(); + } + } + None => result, + } + }); + return resolved_selectors } + } + vec!(selector) +} - if temp_state.len() != 0 { - return Some(temp_state); +// This does not handle the highest level rules +fn get_parent_block(this_rule: SyntaxNode) -> Option { + if let Some(nested_qualified_rule) = CssNestedQualifiedRule::cast_ref(&this_rule) { + if let Some(rule_grand_parent) = nested_qualified_rule.syntax().grand_parent(){ + if let Some(css_declaration_block) = CssDeclarationOrRuleBlock::cast_ref(&rule_grand_parent) { + return Some(css_declaration_block) + } } + } + None +} - None +fn handle_css_rule(rule: SyntaxNode) -> Vec<(SyntaxNode, String)> { + let mut selector_list = vec!(); + if let Some(any_css_rule) = AnyCssRule::cast_ref(&rule) { + match any_css_rule { + AnyCssRule::CssAtRule(_) => todo!(), + AnyCssRule::CssBogusRule(_) => todo!(), + AnyCssRule::CssNestedQualifiedRule(nested_qualified_rule) => { + for selector in nested_qualified_rule.prelude() { + if let Ok(valid_selector) = selector { + let selector_syntax = valid_selector.into_syntax(); + selector_list.push( + (selector_syntax.clone(), handle_css_selector(selector_syntax)) + ); + } + } + }, + AnyCssRule::CssQualifiedRule(qualified_rule) => { + for selector in qualified_rule.prelude() { + if let Ok(valid_selector) = selector { + let selector_syntax = valid_selector.into_syntax(); + selector_list.push( + (selector_syntax.clone(), handle_css_selector(selector_syntax)) + ); + } + } + }, + } } + selector_list +} - fn diagnostic(_: &RuleContext, node: &Self::State) -> Option { - // - // Read our guidelines to write great diagnostics: - // https://docs.rs/biome_analyze/latest/biome_analyze/#what-a-rule-should-say-to-the-user - // - for n in node.into_iter() { - let n_syn = n.clone().into_syntax(); - return Some( - RuleDiagnostic::new( - rule_category!(), - n_syn.text_range(), - markup! { - "Unexpected duplicate selector" {n_syn.to_string()} - }, - ) - .note(markup! { - "This note will give you more information." - }), - ) +fn handle_css_selector(selector: SyntaxNode) -> String { + if let Some(complex_selector) = CssComplexSelector::cast_ref(&selector) { + let mut resolved = complex_selector.text(); + // This is to handle the special case of an empty combinator + // i.e. .foo .bar == .bar .foo + let mut left_right :Vec = vec!(); + + if let Ok(left) = complex_selector.left() { + left_right.push(handle_css_selector(left.into_syntax())); } - None + if let Ok(right) = complex_selector.right() { + left_right.push(handle_css_selector(right.into_syntax())); + } + if let Ok(combinator) = complex_selector.combinator() { + if combinator.text() == " " { + left_right.sort() + } + resolved = left_right.join(combinator.text()); + } + + return resolved } + if let Some(relative_selector) = CssRelativeSelector::cast_ref(&selector) { + let mut resolved = String::new(); + if let Some(combinator) = relative_selector.combinator() { + resolved.push_str(combinator.text()); + } + if let Ok(selector) = relative_selector.selector(){ + resolved.push_str(&handle_css_selector(selector.into())); + } + return resolved + } + if let Some(compound_selector) = CssCompoundSelector::cast_ref(&selector) { + return format_compound_selector(compound_selector) + } + if let Some(any) = AnyCssSelector::cast_ref(&selector) { + return any.text(); + } + selector.to_string() } + +fn format_compound_selector (selector: CssCompoundSelector) -> String { + let nesting_selector_token = selector.nesting_selector_token(); + let sub_selectors = selector.sub_selectors(); + let simple_selector = selector.simple_selector(); + + let mut resolved = String::new(); + if let Some(token) = nesting_selector_token { + resolved.push_str(&token.text().trim()); + } + if let Some(selector) = simple_selector { + resolved.push_str(&selector.text()); + } + let mut sub_selector_string: Vec = sub_selectors.into_iter().map(|s|{ + return s.text() + }).collect(); + sub_selector_string.sort(); + if sub_selector_string.len() > 0 { + resolved.push_str(&sub_selector_string.join("")); + } + return resolved +} \ No newline at end of file diff --git a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css.snap.new b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css.snap.new index 71f4e126e74..3b2f6d8f216 100644 --- a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css.snap.new +++ b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css.snap.new @@ -57,18 +57,99 @@ a { # Diagnostics ``` -invalid.css:4:1 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +invalid.css:3:7 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Unexpected duplicate selector:.alpha + ! Duplicate selector. + 1 │ /* Duplication of single selectors in a rule's selector list */ 2 │ .alpha, - 3 │ .beta, + > 3 │ .beta, + │ > 4 │ .alpha { - │ ^^^^^^ + │ ^^^^^^^ 5 │ width: 12; 6 │ } - i This note will give you more information. + i Please fix it. + + +``` + +``` +invalid.css:12:2 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Duplicate selector. + + 10 │ .foo .bar { + 11 │ width: 12; + > 12 │ } + │ + > 13 │ + > 14 │ .bar .foo { + │ ^^^^^^^^^^ + 15 │ width: 12; + 16 │ } + + i Please fix it. + + +``` + +``` +invalid.css:21:2 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Duplicate selector. + + 19 │ b+.dle { + 20 │ width: 12; + > 21 │ } + │ + > 22 │ + > 23 │ b+.dle, + │ ^^^^^^ + 24 │ a .can { + 25 │ width: 12; + + i Please fix it. + + +``` + +``` +invalid.css:23:8 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Duplicate selector. + + 21 │ } + 22 │ + > 23 │ b+.dle, + │ + > 24 │ a .can { + │ ^^^^^^^ + 25 │ width: 12; + 26 │ } + + i Please fix it. + + +``` + +``` +invalid.css:32:6 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Duplicate selector. + + 30 │ .foo { + 31 │ width: 12; + > 32 │ } + │ + > 33 │ + > 34 │ .foo { + │ ^^^^^ + 35 │ width: 12; + 36 │ } + + i Please fix it. ``` diff --git a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/valid.css.snap.new b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/valid.css.snap.new index a086c6fb111..f3f757f2f70 100644 --- a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/valid.css.snap.new +++ b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/valid.css.snap.new @@ -40,3 +40,23 @@ a { } } ``` + +# Diagnostics +``` +valid.css:6:27 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Duplicate selector. + + 4 │ } + 5 │ + > 6 │ @media (min-width: 10px) { + │ + > 7 │ .foo { + │ ^^^^^ + 8 │ width: 12; + 9 │ } + + i Please fix it. + + +``` From b6b3d1e965521cf1e8989eb6d273cef3d1b9c4cd Mon Sep 17 00:00:00 2001 From: Abid Jappie Date: Fri, 3 May 2024 01:12:23 +0900 Subject: [PATCH 05/20] WIP: removed the visitor, simplified the logic, added stylelint test cases. TODO: finish updating the logic - some issues with resolving. --- .../lint/nursery/no_duplicate_selectors.rs | 273 +++------- .../nursery/noDuplicateSelectors/invalid.css | 99 ++-- .../noDuplicateSelectors/invalid.css.snap.new | 509 ++++++++++++++---- .../nursery/noDuplicateSelectors/valid.css | 67 ++- .../noDuplicateSelectors/valid.css.snap.new | 271 ++++++++-- 5 files changed, 798 insertions(+), 421 deletions(-) diff --git a/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs b/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs index 5a8ec460bbe..c42d0dc3141 100644 --- a/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs +++ b/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs @@ -1,11 +1,11 @@ use std::collections::HashSet; use std::vec; -use biome_analyze::{AddVisitor, Phases, QueryMatch, Queryable, ServiceBag, Visitor, VisitorContext}; +use biome_analyze::{AddVisitor, Ast, Phases, QueryMatch, Queryable, ServiceBag, Visitor, VisitorContext}; use biome_analyze::{context::RuleContext, declare_rule, Rule, RuleDiagnostic, RuleSource}; use biome_console::markup; -use biome_css_syntax::{AnyCssRule, AnyCssSelector, CssComplexSelector, CssCompoundSelector, CssDeclarationOrRuleBlock, CssLanguage, CssNestedQualifiedRule, CssQualifiedRule, CssRelativeSelector, CssRuleList}; -use biome_rowan::{AstNode, Language, SyntaxNode, TextRange, WalkEvent}; +use biome_css_syntax::{AnyCssRule, AnyCssSelector, CssAtRule, CssComplexSelector, CssCompoundSelector, CssDeclarationBlock, CssDeclarationOrAtRuleBlock, CssDeclarationOrRuleBlock, CssLanguage, CssNestedQualifiedRule, CssQualifiedRule, CssRelativeSelector, CssRuleBlock, CssRuleList, CssSelectorList, CssSyntaxList, CssSyntaxNode}; +use biome_rowan::{AstNode, AstNodeList, Language, SyntaxList, SyntaxNode, SyntaxNodeCast, TextRange, WalkEvent}; declare_rule! { /// @@ -18,87 +18,44 @@ declare_rule! { } } -#[derive(Default)] -struct DeclarationOrRuleBlockListVisitor { - stack: Vec -} - -impl Visitor for DeclarationOrRuleBlockListVisitor { - type Language = CssLanguage; - - fn visit( - &mut self, - event: &WalkEvent>, - mut ctx: VisitorContext - ) { - 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(); - } - } - } - } -} - -pub struct DeclarationOrRuleBlockList(Vec); - -impl QueryMatch for DeclarationOrRuleBlockList { - fn text_range(&self) -> TextRange { - todo!() - } -} - -impl Queryable for DeclarationOrRuleBlockList { - type Input = Self; - type Output = Vec; - type Language = CssLanguage; - type Services = (); - - fn build_visitor( - analyzer: &mut impl AddVisitor, - _root: &::Root, - ) { - analyzer.add_visitor(Phases::Syntax, DeclarationOrRuleBlockListVisitor::default); - } - - fn unwrap_match(_services: &ServiceBag, query: &Self::Input) -> Self::Output { - query.0.clone() - } -} - impl Rule for NoDuplicateSelectors { - type Query = DeclarationOrRuleBlockList; - type State = SyntaxNode; + type Query = Ast; + type State = CssSyntaxNode; type Signals = Vec; type Options = (); fn run(ctx: &RuleContext) -> Vec { - let node_list = ctx.query(); + let node = ctx.query(); + let mut resolved_list = HashSet::new(); + let mut output: Vec = vec!(); + + let selectors = node + .syntax() + .descendants() + .filter_map(|x|x.cast::()); + + for selector in selectors { + let mut this_selctor_list = selector.clone().into_syntax().parent().unwrap(); + + if let Some(parent_selector) = CssComplexSelector::cast_ref(&this_selctor_list) { + // Don't handle the children of complex selectors + this_selctor_list = parent_selector.into_syntax().parent().unwrap() + } else if let Some(parent_selector) = CssRelativeSelector::cast_ref(&this_selctor_list) { + // Don't handle the children of complex relative + this_selctor_list = parent_selector.into_syntax().parent().unwrap(); + } - let mut select_list = HashSet::new(); - let mut output: Vec> = vec!(); + let this_rule = this_selctor_list.parent().unwrap(); + let this_block = this_rule.grand_parent().unwrap(); - for node in node_list { - if let Some(this_rule) = node.syntax().parent() { - let handled_rule = handle_css_rule(this_rule); - for (selector_node, selector) in handled_rule { - let resolved_selectors = resolve_nested_selectors(selector, node.clone()); - for resolved in resolved_selectors { - if !select_list.insert(resolved) { - output.push(selector_node.clone()); - } - } + let resolved = resolve_nested_selectors(selector.clone().text(), this_block); + for r in resolved { + println!("resolved: {:?}", r); + if !resolved_list.insert(r) { + output.push(selector.clone().into_syntax()); } } } - output } @@ -122,32 +79,41 @@ impl Rule for NoDuplicateSelectors { } // TODO: need to handle AtRules etc. -fn resolve_nested_selectors(selector: String, node: CssDeclarationOrRuleBlock) -> Vec { +fn resolve_nested_selectors(selector: String, block: CssSyntaxNode) -> Vec { let mut parent_selectors: Vec = vec!(); - if let Some(this_rule) = node.syntax().parent() { - if let Some(_qualified_rule) = CssQualifiedRule::cast_ref(&this_rule) { - // Highest Level - return vec!(selector); - } - - let parent_node = get_parent_block(this_rule); - - if let Some(parent_block) = parent_node.clone() { - if let Some(parent_node_parent) = parent_block.into_syntax().parent() { - if let Some(parent_rule) = CssQualifiedRule::cast_ref(&parent_node_parent) { - for selector in parent_rule.prelude() { - if let Ok(selector) = selector { - parent_selectors.push(handle_css_selector(selector.into())); - } - } - } - if let Some(parent_rule) = CssNestedQualifiedRule::cast_ref(&parent_node_parent) { - for selector in parent_rule.prelude() { - if let Ok(selector) = selector { - parent_selectors.push(handle_css_selector(selector.into())); - } + let parent_node = get_parent_block(block); + + match &parent_node { + None => { + return vec!(selector) + }, + Some(parent_block) => { + if let Some(parent_node_parent) = parent_block.parent() { + if let Some(parent_rule) = AnyCssRule::cast_ref(&parent_node_parent){ + match parent_rule { + AnyCssRule::CssBogusRule(_) => todo!(), + AnyCssRule::CssAtRule(parent_rule) => { + // Treat the AtRule as a selector + let rule = parent_rule.rule().unwrap(); + parent_selectors.push(rule.text()); + }, + 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()); + } + } + }, } + } } @@ -171,114 +137,19 @@ fn resolve_nested_selectors(selector: String, node: CssDeclarationOrRuleBlock) - None => result, } }); - return resolved_selectors - } - } - vec!(selector) -} - -// This does not handle the highest level rules -fn get_parent_block(this_rule: SyntaxNode) -> Option { - if let Some(nested_qualified_rule) = CssNestedQualifiedRule::cast_ref(&this_rule) { - if let Some(rule_grand_parent) = nested_qualified_rule.syntax().grand_parent(){ - if let Some(css_declaration_block) = CssDeclarationOrRuleBlock::cast_ref(&rule_grand_parent) { - return Some(css_declaration_block) + if resolved_selectors.len() > 0 { + return resolved_selectors } - } - } - None -} - -fn handle_css_rule(rule: SyntaxNode) -> Vec<(SyntaxNode, String)> { - let mut selector_list = vec!(); - if let Some(any_css_rule) = AnyCssRule::cast_ref(&rule) { - match any_css_rule { - AnyCssRule::CssAtRule(_) => todo!(), - AnyCssRule::CssBogusRule(_) => todo!(), - AnyCssRule::CssNestedQualifiedRule(nested_qualified_rule) => { - for selector in nested_qualified_rule.prelude() { - if let Ok(valid_selector) = selector { - let selector_syntax = valid_selector.into_syntax(); - selector_list.push( - (selector_syntax.clone(), handle_css_selector(selector_syntax)) - ); - } - } - }, - AnyCssRule::CssQualifiedRule(qualified_rule) => { - for selector in qualified_rule.prelude() { - if let Ok(valid_selector) = selector { - let selector_syntax = valid_selector.into_syntax(); - selector_list.push( - (selector_syntax.clone(), handle_css_selector(selector_syntax)) - ); - } - } - }, - } + return vec!(selector) + }, } - selector_list } -fn handle_css_selector(selector: SyntaxNode) -> String { - if let Some(complex_selector) = CssComplexSelector::cast_ref(&selector) { - let mut resolved = complex_selector.text(); - // This is to handle the special case of an empty combinator - // i.e. .foo .bar == .bar .foo - let mut left_right :Vec = vec!(); - - if let Ok(left) = complex_selector.left() { - left_right.push(handle_css_selector(left.into_syntax())); - } - if let Ok(right) = complex_selector.right() { - left_right.push(handle_css_selector(right.into_syntax())); - } - if let Ok(combinator) = complex_selector.combinator() { - if combinator.text() == " " { - left_right.sort() - } - resolved = left_right.join(combinator.text()); - } - - return resolved - } - if let Some(relative_selector) = CssRelativeSelector::cast_ref(&selector) { - let mut resolved = String::new(); - if let Some(combinator) = relative_selector.combinator() { - resolved.push_str(combinator.text()); - } - if let Ok(selector) = relative_selector.selector(){ - resolved.push_str(&handle_css_selector(selector.into())); - } - return resolved - } - if let Some(compound_selector) = CssCompoundSelector::cast_ref(&selector) { - return format_compound_selector(compound_selector) - } - if let Some(any) = AnyCssSelector::cast_ref(&selector) { - return any.text(); +// This does not handle the highest level rules +fn get_parent_block(this_block: CssSyntaxNode) -> Option { + if let Some(parent_rule) = this_block.parent() { + return parent_rule.grand_parent(); } - selector.to_string() + return None } -fn format_compound_selector (selector: CssCompoundSelector) -> String { - let nesting_selector_token = selector.nesting_selector_token(); - let sub_selectors = selector.sub_selectors(); - let simple_selector = selector.simple_selector(); - - let mut resolved = String::new(); - if let Some(token) = nesting_selector_token { - resolved.push_str(&token.text().trim()); - } - if let Some(selector) = simple_selector { - resolved.push_str(&selector.text()); - } - let mut sub_selector_string: Vec = sub_selectors.into_iter().map(|s|{ - return s.text() - }).collect(); - sub_selector_string.sort(); - if sub_selector_string.len() > 0 { - resolved.push_str(&sub_selector_string.join("")); - } - return resolved -} \ No newline at end of file diff --git a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css index 54b445145a7..2f9d03731af 100644 --- a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css +++ b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css @@ -1,48 +1,51 @@ -/* Duplication of single selectors in a rule's selector list */ -.alpha, -.beta, -.alpha { - width: 12; -} - -/* Duplication of selector lists within the style sheet */ -/* Duplication is found even if the selectors have different ordering */ -.foo .bar { - width: 12; -} - -.bar .foo { - width: 12; -} - -a .can, -b+.dle { - width: 12; -} - -b+.dle, -a .can { - width: 12; -} - -/* Duplication within @ rule */ -@media (min-width: 10px) { - .foo { - width: 12; - } - - .foo { - width: 12; - } -} - -/* Duplication when the net result of nesting is the same */ -a b { - width: 12; -} - -a { - & b { - width: 12; - } -} \ No newline at end of file +/* duplicate within one rule's selector list */ +a, a {} + +/* duplicate within one rule's selector list. multiline */ +b, +b {} + +/* duplicate within one rule's selector list. multiline */ +c, +d, +d {} + +/* duplicated selectors within one rule's selector list. 3 duplicates */ +.e, +.e, +.e {} + +/* duplicate simple selectors with another rule between */ +f {} g {} f {} + +/* duplicate simple selectors with another rule between */ +h {} +i {} +h {} + +/* duplicate selector lists with different order */ +j, k {} j, k {} + +/* duplicate selectors with multiple components */ +m n {} +m n {} + +/* essentially duplicate selector lists with varied spacing */ +.foo p, q > .bar, +baz {} + #baz, + .foo p,q>.bar {} + +/* duplicate within a media query, in the same rule */ +s {} +@media print { s, s {} } + +/* duplicate within a media query, in different rules */ +t {} +@media print { t {} t {} } + +/* duplicate caused by nesting */ +v w {} v { w {} } + +/* duplicate caused by &-parent selector */ +x { & {} } \ No newline at end of file diff --git a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css.snap.new b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css.snap.new index 3b2f6d8f216..8c80ab4df51 100644 --- a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css.snap.new +++ b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css.snap.new @@ -5,70 +5,229 @@ expression: invalid.css --- # Input ```css -/* Duplication of single selectors in a rule's selector list */ -.alpha, -.beta, -.alpha { - width: 12; -} - -/* Duplication of selector lists within the style sheet */ -/* Duplication is found even if the selectors have different ordering */ -.foo .bar { - width: 12; -} - -.bar .foo { - width: 12; -} - -a .can, -b+.dle { - width: 12; -} - -b+.dle, -a .can { - width: 12; -} - -/* Duplication within @ rule */ -@media (min-width: 10px) { - .foo { - width: 12; - } - - .foo { - width: 12; - } -} - -/* Duplication when the net result of nesting is the same */ -a b { - width: 12; -} - -a { - & b { - width: 12; - } -} +/* duplicate within one rule's selector list */ +a, a {} + +/* duplicate within one rule's selector list. multiline */ +b, +b {} + +/* duplicate within one rule's selector list. multiline */ +c, +d, +d {} + +/* duplicated selectors within one rule's selector list. 3 duplicates */ +.e, +.e, +.e {} + +/* duplicate simple selectors with another rule between */ +f {} g {} f {} + +/* duplicate simple selectors with another rule between */ +h {} +i {} +h {} + +/* duplicate selector lists with different order */ +j, k {} j, k {} + +/* duplicate selectors with multiple components */ +m n {} +m n {} + +/* essentially duplicate selector lists with varied spacing */ +.foo p, q > .bar, +baz {} + #baz, + .foo p,q>.bar {} + +/* duplicate within a media query, in the same rule */ +s {} +@media print { s, s {} } + +/* duplicate within a media query, in different rules */ +t {} +@media print { t {} t {} } + +/* duplicate caused by nesting */ +v w {} v { w {} } + +/* duplicate caused by &-parent selector */ +x { & {} } ``` # Diagnostics ``` -invalid.css:3:7 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +invalid.css:2:4 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Duplicate selector. + + 1 │ /* duplicate within one rule's selector list */ + > 2 │ a, a {} + │ ^^ + 3 │ + 4 │ /* duplicate within one rule's selector list. multiline */ + + i Please fix it. + + +``` + +``` +invalid.css:5:3 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Duplicate selector. + + 4 │ /* duplicate within one rule's selector list. multiline */ + > 5 │ b, + │ + > 6 │ b {} + │ ^^ + 7 │ + 8 │ /* duplicate within one rule's selector list. multiline */ + + i Please fix it. + + +``` + +``` +invalid.css:10:3 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Duplicate selector. + + 8 │ /* duplicate within one rule's selector list. multiline */ + 9 │ c, + > 10 │ d, + │ + > 11 │ d {} + │ ^^ + 12 │ + 13 │ /* duplicated selectors within one rule's selector list. 3 duplicates */ + + i Please fix it. + + +``` + +``` +invalid.css:14:4 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Duplicate selector. + + 13 │ /* duplicated selectors within one rule's selector list. 3 duplicates */ + > 14 │ .e, + │ + > 15 │ .e, + │ ^^ + 16 │ .e {} + 17 │ + + i Please fix it. + + +``` + +``` +invalid.css:15:4 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Duplicate selector. + + 13 │ /* duplicated selectors within one rule's selector list. 3 duplicates */ + 14 │ .e, + > 15 │ .e, + │ + > 16 │ .e {} + │ ^^^ + 17 │ + 18 │ /* duplicate simple selectors with another rule between */ + + i Please fix it. + + +``` + +``` +invalid.css:19:11 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Duplicate selector. + + 18 │ /* duplicate simple selectors with another rule between */ + > 19 │ f {} g {} f {} + │ ^^ + 20 │ + 21 │ /* duplicate simple selectors with another rule between */ + + i Please fix it. + + +``` + +``` +invalid.css:23:5 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Duplicate selector. + + 21 │ /* duplicate simple selectors with another rule between */ + 22 │ h {} + > 23 │ i {} + │ + > 24 │ h {} + │ ^^ + 25 │ + 26 │ /* duplicate selector lists with different order */ + + i Please fix it. + + +``` + +``` +invalid.css:27:9 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Duplicate selector. + + 26 │ /* duplicate selector lists with different order */ + > 27 │ j, k {} j, k {} + │ ^ + 28 │ + 29 │ /* duplicate selectors with multiple components */ + + i Please fix it. + + +``` + +``` +invalid.css:27:12 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Duplicate selector. + + 26 │ /* duplicate selector lists with different order */ + > 27 │ j, k {} j, k {} + │ ^^ + 28 │ + 29 │ /* duplicate selectors with multiple components */ + + i Please fix it. + + +``` + +``` +invalid.css:30:7 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ ! Duplicate selector. - 1 │ /* Duplication of single selectors in a rule's selector list */ - 2 │ .alpha, - > 3 │ .beta, - │ - > 4 │ .alpha { - │ ^^^^^^^ - 5 │ width: 12; - 6 │ } + 29 │ /* duplicate selectors with multiple components */ + > 30 │ m n {} + │ + > 31 │ m n {} + │ ^ + 32 │ + 33 │ /* essentially duplicate selector lists with varied spacing */ i Please fix it. @@ -76,19 +235,17 @@ invalid.css:3:7 lint/nursery/noDuplicateSelectors ━━━━━━━━━━ ``` ``` -invalid.css:12:2 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +invalid.css:30:7 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ ! Duplicate selector. - 10 │ .foo .bar { - 11 │ width: 12; - > 12 │ } - │ - > 13 │ - > 14 │ .bar .foo { - │ ^^^^^^^^^^ - 15 │ width: 12; - 16 │ } + 29 │ /* duplicate selectors with multiple components */ + > 30 │ m n {} + │ + > 31 │ m n {} + │ ^^^^ + 32 │ + 33 │ /* essentially duplicate selector lists with varied spacing */ i Please fix it. @@ -96,19 +253,16 @@ invalid.css:12:2 lint/nursery/noDuplicateSelectors ━━━━━━━━━ ``` ``` -invalid.css:21:2 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +invalid.css:31:3 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ ! Duplicate selector. - 19 │ b+.dle { - 20 │ width: 12; - > 21 │ } - │ - > 22 │ - > 23 │ b+.dle, - │ ^^^^^^ - 24 │ a .can { - 25 │ width: 12; + 29 │ /* duplicate selectors with multiple components */ + 30 │ m n {} + > 31 │ m n {} + │ ^^ + 32 │ + 33 │ /* essentially duplicate selector lists with varied spacing */ i Please fix it. @@ -116,18 +270,187 @@ invalid.css:21:2 lint/nursery/noDuplicateSelectors ━━━━━━━━━ ``` ``` -invalid.css:23:8 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +invalid.css:36:8 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ ! Duplicate selector. - 21 │ } - 22 │ - > 23 │ b+.dle, + 34 │ .foo p, q > .bar, + 35 │ baz {} + > 36 │ #baz, │ - > 24 │ a .can { - │ ^^^^^^^ - 25 │ width: 12; - 26 │ } + > 37 │ .foo p,q>.bar {} + │ ^^^^ + 38 │ + 39 │ /* duplicate within a media query, in the same rule */ + + i Please fix it. + + +``` + +``` +invalid.css:37:12 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Duplicate selector. + + 35 │ baz {} + 36 │ #baz, + > 37 │ .foo p,q>.bar {} + │ ^ + 38 │ + 39 │ /* duplicate within a media query, in the same rule */ + + i Please fix it. + + +``` + +``` +invalid.css:37:14 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Duplicate selector. + + 35 │ baz {} + 36 │ #baz, + > 37 │ .foo p,q>.bar {} + │ ^ + 38 │ + 39 │ /* duplicate within a media query, in the same rule */ + + i Please fix it. + + +``` + +``` +invalid.css:37:16 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Duplicate selector. + + 35 │ baz {} + 36 │ #baz, + > 37 │ .foo p,q>.bar {} + │ ^^^^^ + 38 │ + 39 │ /* duplicate within a media query, in the same rule */ + + i Please fix it. + + +``` + +``` +invalid.css:41:16 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Duplicate selector. + + 39 │ /* duplicate within a media query, in the same rule */ + 40 │ s {} + > 41 │ @media print { s, s {} } + │ ^ + 42 │ + 43 │ /* duplicate within a media query, in different rules */ + + i Please fix it. + + +``` + +``` +invalid.css:41:19 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Duplicate selector. + + 39 │ /* duplicate within a media query, in the same rule */ + 40 │ s {} + > 41 │ @media print { s, s {} } + │ ^^ + 42 │ + 43 │ /* duplicate within a media query, in different rules */ + + i Please fix it. + + +``` + +``` +invalid.css:41:19 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Duplicate selector. + + 39 │ /* duplicate within a media query, in the same rule */ + 40 │ s {} + > 41 │ @media print { s, s {} } + │ ^^ + 42 │ + 43 │ /* duplicate within a media query, in different rules */ + + i Please fix it. + + +``` + +``` +invalid.css:45:16 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Duplicate selector. + + 43 │ /* duplicate within a media query, in different rules */ + 44 │ t {} + > 45 │ @media print { t {} t {} } + │ ^^ + 46 │ + 47 │ /* duplicate caused by nesting */ + + i Please fix it. + + +``` + +``` +invalid.css:45:21 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Duplicate selector. + + 43 │ /* duplicate within a media query, in different rules */ + 44 │ t {} + > 45 │ @media print { t {} t {} } + │ ^^ + 46 │ + 47 │ /* duplicate caused by nesting */ + + i Please fix it. + + +``` + +``` +invalid.css:45:21 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Duplicate selector. + + 43 │ /* duplicate within a media query, in different rules */ + 44 │ t {} + > 45 │ @media print { t {} t {} } + │ ^^ + 46 │ + 47 │ /* duplicate caused by nesting */ + + i Please fix it. + + +``` + +``` +invalid.css:48:8 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Duplicate selector. + + 47 │ /* duplicate caused by nesting */ + > 48 │ v w {} v { w {} } + │ ^^ + 49 │ + 50 │ /* duplicate caused by &-parent selector */ i Please fix it. @@ -135,19 +458,15 @@ invalid.css:23:8 lint/nursery/noDuplicateSelectors ━━━━━━━━━ ``` ``` -invalid.css:32:6 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +invalid.css:48:12 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ ! Duplicate selector. - 30 │ .foo { - 31 │ width: 12; - > 32 │ } - │ - > 33 │ - > 34 │ .foo { - │ ^^^^^ - 35 │ width: 12; - 36 │ } + 47 │ /* duplicate caused by nesting */ + > 48 │ v w {} v { w {} } + │ ^^ + 49 │ + 50 │ /* duplicate caused by &-parent selector */ i Please fix it. diff --git a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/valid.css b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/valid.css index f258dc7301f..e3e18231728 100644 --- a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/valid.css +++ b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/valid.css @@ -1,34 +1,33 @@ -/* Same selector with different parent nodes are allowed */ -.foo { - width: 12; -} - -@media (min-width: 10px) { - .foo { - width: 12; - } -} - -.bar { - .bar { - width: 12; - } -} - -/* Same selector in a different list */ -.foo .bar { - width: 12; -} - -/* Net list is different */ -a b { - width: 12; -} - -a { - - & b, - & c { - width: 12; - } -} \ No newline at end of file +/* no duplicates */ +a {} b {} c {} d, e, f {} + +/* duplicate inside media query */ +g {} +@media print { g {} } + +/* duplicate inside keyframes */ +@keyframes h { 0% {} } +@keyframes h { 0% {} } + +/* duplicates inside nested rules */ +i { i { i {} } } + +/* selectors using parts of other selectors */ +.foo .bar {} +.foo {} +.bar {} +.bar .foo {} + +/* selectors reused in other non-equivalent selector lists */ +j {} j, k {} + +/* nested resolution */ +m n { top: 0; } m { n, p { color: pink; } } + +@mixin abc { &:hover {} } @mixin xyz { &:hover {} } + +/* pass if default setting i.e. disallowInList: false */ +ul, ol {} ul {} + +/* attribute selector */ +[disabled].goo, [disabled] .goo {} diff --git a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/valid.css.snap.new b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/valid.css.snap.new index f3f757f2f70..82dde9afbe5 100644 --- a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/valid.css.snap.new +++ b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/valid.css.snap.new @@ -5,56 +5,241 @@ expression: valid.css --- # Input ```css -/* Same selector with different parent nodes are allowed */ -.foo { - width: 12; -} - -@media (min-width: 10px) { - .foo { - width: 12; - } -} - -.bar { - .bar { - width: 12; - } -} - -/* Same selector in a different list */ -.foo .bar { - width: 12; -} - -/* Net list is different */ -a b { - width: 12; -} - -a { - - & b, - & c { - width: 12; - } -} +/* no duplicates */ +a {} b {} c {} d, e, f {} + +/* duplicate inside media query */ +g {} +@media print { g {} } + +/* duplicate inside keyframes */ +@keyframes h { 0% {} } +@keyframes h { 0% {} } + +/* duplicates inside nested rules */ +i { i { i {} } } + +/* selectors using parts of other selectors */ +.foo .bar {} +.foo {} +.bar {} +.bar .foo {} + +/* selectors reused in other non-equivalent selector lists */ +j {} j, k {} + +/* nested resolution */ +m n { top: 0; } m { n, p { color: pink; } } + +@mixin abc { &:hover {} } @mixin xyz { &:hover {} } + +/* pass if default setting i.e. disallowInList: false */ +ul, ol {} ul {} + +/* attribute selector */ +[disabled].goo, [disabled] .goo {} + ``` # Diagnostics ``` -valid.css:6:27 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +valid.css:6:16 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Duplicate selector. + + 4 │ /* duplicate inside media query */ + 5 │ g {} + > 6 │ @media print { g {} }→ + │ ^^ + 7 │ + 8 │ /* duplicate inside keyframes */ + + i Please fix it. + + +``` + +``` +valid.css:13:5 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Duplicate selector. + + 12 │ /* duplicates inside nested rules */ + > 13 │ i { i { i {} } } + │ ^^ + 14 │ + 15 │ /* selectors using parts of other selectors */ + + i Please fix it. + + +``` + +``` +valid.css:16:13 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Duplicate selector. + + 15 │ /* selectors using parts of other selectors */ + > 16 │ .foo .bar {} + │ + > 17 │ .foo {} + │ ^^^^^ + 18 │ .bar {} + 19 │ .bar .foo {} + + i Please fix it. + + +``` + +``` +valid.css:17:8 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Duplicate selector. + + 15 │ /* selectors using parts of other selectors */ + 16 │ .foo .bar {} + > 17 │ .foo {} + │ + > 18 │ .bar {} + │ ^^^^^ + 19 │ .bar .foo {} + 20 │ + + i Please fix it. + + +``` + +``` +valid.css:18:8 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Duplicate selector. + + 16 │ .foo .bar {} + 17 │ .foo {} + > 18 │ .bar {} + │ + > 19 │ .bar .foo {} + │ ^^^^ + 20 │ + 21 │ /* selectors reused in other non-equivalent selector lists */ + + i Please fix it. + + +``` + +``` +valid.css:19:6 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Duplicate selector. + + 17 │ .foo {} + 18 │ .bar {} + > 19 │ .bar .foo {} + │ ^^^^^ + 20 │ + 21 │ /* selectors reused in other non-equivalent selector lists */ + + i Please fix it. + + +``` + +``` +valid.css:22:6 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Duplicate selector. + + 21 │ /* selectors reused in other non-equivalent selector lists */ + > 22 │ j {} j, k {} + │ ^ + 23 │ + 24 │ /* nested resolution */ + + i Please fix it. + + +``` + +``` +valid.css:25:17 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Duplicate selector. + + 24 │ /* nested resolution */ + > 25 │ m n { top: 0; } m { n, p { color: pink; } }→ + │ ^^ + 26 │ + 27 │ @mixin abc { &:hover {} } @mixin xyz { &:hover {} } + + i Please fix it. + + +``` + +``` +valid.css:25:21 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Duplicate selector. + + 24 │ /* nested resolution */ + > 25 │ m n { top: 0; } m { n, p { color: pink; } }→ + │ ^ + 26 │ + 27 │ @mixin abc { &:hover {} } @mixin xyz { &:hover {} } + + i Please fix it. + + +``` + +``` +valid.css:27:28 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Duplicate selector. + + 25 │ m n { top: 0; } m { n, p { color: pink; } }→ + 26 │ + > 27 │ @mixin abc { &:hover {} } @mixin xyz { &:hover {} } + │ ^^^^^ + 28 │ + 29 │ /* pass if default setting i.e. disallowInList: false */ + + i Please fix it. + + +``` + +``` +valid.css:27:40 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Duplicate selector. + + 25 │ m n { top: 0; } m { n, p { color: pink; } }→ + 26 │ + > 27 │ @mixin abc { &:hover {} } @mixin xyz { &:hover {} } + │ ^^^^^^^^ + 28 │ + 29 │ /* pass if default setting i.e. disallowInList: false */ + + i Please fix it. + + +``` + +``` +valid.css:30:11 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ ! Duplicate selector. - 4 │ } - 5 │ - > 6 │ @media (min-width: 10px) { - │ - > 7 │ .foo { - │ ^^^^^ - 8 │ width: 12; - 9 │ } + 29 │ /* pass if default setting i.e. disallowInList: false */ + > 30 │ ul, ol {} ul {} + │ ^^^ + 31 │ + 32 │ /* attribute selector */ i Please fix it. From 87a6f5e2f7cf6d49b3956abc0ed8efd64511308a Mon Sep 17 00:00:00 2001 From: Abid Jappie Date: Fri, 3 May 2024 23:13:55 +0900 Subject: [PATCH 06/20] WIP: Added option cases, many more test cases are passing. Next: pass all test cases. --- .../lint/nursery/no_duplicate_selectors.rs | 251 +++++++++++++----- .../nursery/noDuplicateSelectors/invalid.css | 4 +- .../noDuplicateSelectors/invalid.css.snap.new | 187 ++----------- .../noDuplicateSelectors/valid.css.snap.new | 205 -------------- 4 files changed, 197 insertions(+), 450 deletions(-) diff --git a/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs b/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs index c42d0dc3141..192a652486e 100644 --- a/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs +++ b/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs @@ -1,11 +1,14 @@ use std::collections::HashSet; use std::vec; -use biome_analyze::{AddVisitor, Ast, Phases, QueryMatch, Queryable, ServiceBag, Visitor, VisitorContext}; +use biome_analyze::{Ast, VisitorContext}; use biome_analyze::{context::RuleContext, declare_rule, Rule, RuleDiagnostic, RuleSource}; use biome_console::markup; -use biome_css_syntax::{AnyCssRule, AnyCssSelector, CssAtRule, CssComplexSelector, CssCompoundSelector, CssDeclarationBlock, CssDeclarationOrAtRuleBlock, CssDeclarationOrRuleBlock, CssLanguage, CssNestedQualifiedRule, CssQualifiedRule, CssRelativeSelector, CssRuleBlock, CssRuleList, CssSelectorList, CssSyntaxList, CssSyntaxNode}; -use biome_rowan::{AstNode, AstNodeList, Language, SyntaxList, SyntaxNode, SyntaxNodeCast, TextRange, WalkEvent}; +use biome_deserialize_macros::Deserializable; +use biome_css_syntax::{kind, AnyCssAtRule, AnyCssRelativeSelector, AnyCssRule, AnyCssSelector, AnyCssSubSelector, CssComplexSelector, CssMediaAtRule, CssRelativeSelector, CssRelativeSelectorList, CssRoot, CssRuleList, CssSelectorList, CssSyntaxKind, CssSyntaxNode}; +use biome_rowan::{AstNode, Language, SyntaxKindSet, SyntaxNodeCast}; + +use serde::{Deserialize, Serialize}; declare_rule! { /// @@ -18,45 +21,134 @@ declare_rule! { } } +#[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, +} + +impl Default for NoDuplicateSelectorsOptions { + fn default() -> Self { + Self { + disallow_in_list: false + } + } +} + impl Rule for NoDuplicateSelectors { type Query = Ast; type State = CssSyntaxNode; type Signals = Vec; - type Options = (); + type Options = NoDuplicateSelectorsOptions; fn run(ctx: &RuleContext) -> Vec { let node = ctx.query(); + let options = ctx.options(); + let mut resolved_list = HashSet::new(); let mut output: Vec = vec!(); - let selectors = node + if (options.disallow_in_list) { + let selectors = node .syntax() .descendants() - .filter_map(|x|x.cast::()); + .filter_map(|x|{ + if let Some(_selector) = x.clone().cast::(){ + return Some(x) + } + if let Some(_relative_selector) = x.clone().cast::(){ + return Some(x) + } + None + }); - for selector in selectors { - let mut this_selctor_list = selector.clone().into_syntax().parent().unwrap(); - - if let Some(parent_selector) = CssComplexSelector::cast_ref(&this_selctor_list) { - // Don't handle the children of complex selectors - this_selctor_list = parent_selector.into_syntax().parent().unwrap() - } else if let Some(parent_selector) = CssRelativeSelector::cast_ref(&this_selctor_list) { - // Don't handle the children of complex relative - this_selctor_list = parent_selector.into_syntax().parent().unwrap(); + for selector in selectors { + let this_list = selector.clone().parent().unwrap(); + + // i.e not actually a list + if let Some(_parent_sel) = CssComplexSelector::cast_ref(&this_list) { + // Don't handle the children of complex selectors + // this_selctor_list = parent_selector.into_syntax().parent().unwrap() + continue; + } else if let Some(_parent_sel) = CssRelativeSelector::cast_ref(&this_list) { + // Don't handle the children of complex relative + // this_selctor_list = parent_selector.into_syntax().parent().unwrap(); + continue; + } + + let this_rule = this_list.parent().unwrap(); + + let mut selector_text = String::new(); + if let Some(selector) = CssRelativeSelector::cast_ref(&selector){ + selector_text = selector.clone().text() + } + if let Some(selector) = AnyCssSelector::cast_ref(&selector){ + selector_text = selector.clone().text() + } + let resolved = resolve_nested_selectors(selector_text, this_rule); + + for r in resolved { + let split: Vec<&str> = r.split_whitespace().collect(); + let normalized = split.join(" ").to_lowercase(); + println!("resolved: {:?}", normalized); + if !resolved_list.insert(normalized) { + output.push(selector.clone()); + } + } } + } else { + let select_lists = node + .syntax() + .descendants() + .filter_map(|x|{ + if let Some(_selector) = x.clone().cast::(){ + return Some(x) + } + if let Some(_relative_selector) = x.clone().cast::(){ + return Some(x) + } + None + }); + + for selector_list in select_lists { + let mut this_list_resolved_list = HashSet::new(); + + let this_rule = selector_list.parent().unwrap(); + let mut selector_list_mapped: Vec = selector_list + .children() + .into_iter() + .filter_map(|child|{ + if let Some(selector) = AnyCssSelector::cast_ref(&child) { + if !this_list_resolved_list.insert(selector.text()) { + output.push(child.clone()); + } + return Some(selector.text()); + } else if let Some(selector) = AnyCssRelativeSelector::cast_ref(&child) { + if !this_list_resolved_list.insert(selector.text()) { + output.push(child.clone()); + } + return Some(selector.text()); + } + None + }) + .collect(); + selector_list_mapped.sort(); + let selector_list_text = selector_list_mapped.join(","); - let this_rule = this_selctor_list.parent().unwrap(); - let this_block = this_rule.grand_parent().unwrap(); + let resolved = resolve_nested_selectors(selector_list_text, this_rule); - let resolved = resolve_nested_selectors(selector.clone().text(), this_block); - for r in resolved { - println!("resolved: {:?}", r); - if !resolved_list.insert(r) { - output.push(selector.clone().into_syntax()); + for r in resolved { + let split: Vec<&str> = r.split_whitespace().collect(); + let normalized = split.join(" ").to_lowercase(); + println!("resolved: {:?}", normalized); + if !resolved_list.insert(normalized) { + output.push(selector_list.clone()); + } } } } - output + output } fn diagnostic(_: &RuleContext, node: &Self::State) -> Option { @@ -78,63 +170,80 @@ impl Rule for NoDuplicateSelectors { } } -// TODO: need to handle AtRules etc. -fn resolve_nested_selectors(selector: String, block: CssSyntaxNode) -> Vec { +fn resolve_nested_selectors(selector: String, this_rule: CssSyntaxNode) -> Vec { let mut parent_selectors: Vec = vec!(); - let parent_node = get_parent_block(block); + let parent_rule = get_parent_rule(this_rule); - match &parent_node { + match &parent_rule { None => { + println!(" parent = none"); return vec!(selector) }, - Some(parent_block) => { - if let Some(parent_node_parent) = parent_block.parent() { - if let Some(parent_rule) = AnyCssRule::cast_ref(&parent_node_parent){ - match parent_rule { - AnyCssRule::CssBogusRule(_) => todo!(), - AnyCssRule::CssAtRule(parent_rule) => { - // Treat the AtRule as a selector - let rule = parent_rule.rule().unwrap(); - parent_selectors.push(rule.text()); - }, - AnyCssRule::CssNestedQualifiedRule(parent_rule) => { - for selector in parent_rule.prelude() { - if let Ok(selector) = selector { - parent_selectors.push(selector.text()); - } + Some(parent_rule) => { + if let Some(parent_rule) = AnyCssAtRule::cast_ref(&parent_rule) { + println!(" parent is any css at rule: {:?}", parent_rule.clone().text()); + // resolve + match parent_rule { + AnyCssAtRule::CssContainerAtRule(rule) => todo!(), + AnyCssAtRule::CssKeyframesAtRule(rule) => todo!(), + AnyCssAtRule::CssLayerAtRule(rule) => todo!(), + AnyCssAtRule::CssMediaAtRule(rule) => { + let mut resolved = "@".to_string(); + resolved.push_str(&rule.media_token().unwrap().text()); + resolved.push_str(&rule.queries().text()); + // Replace the spaces with something that is not valid in CSS + let resolved = resolved.replace(char::is_whitespace, "-"); + parent_selectors.push(resolved); + }, + AnyCssAtRule::CssPageAtRule(rule) => todo!(), + AnyCssAtRule::CssScopeAtRule(rule) => todo!(), + AnyCssAtRule::CssStartingStyleAtRule(rule) => todo!(), + AnyCssAtRule::CssSupportsAtRule(rule) => todo!(), + _ => {} + } + } + if let Some(parent_rule) = AnyCssRule::cast_ref(&parent_rule){ + match parent_rule { + AnyCssRule::CssBogusRule(_) => todo!(), + AnyCssRule::CssAtRule(parent_rule) => { + // Treat the AtRule as a selector + let rule = parent_rule.rule().unwrap(); + println!(" selectors = {:?}", rule.clone().text()); + parent_selectors.push(rule.text()); + }, + AnyCssRule::CssNestedQualifiedRule(parent_rule) => { + println!(" parent = NQR"); + for selector in parent_rule.prelude() { + if let Ok(selector) = selector { + println!(" selectors = {:?}", selector.clone().text()); + 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()); - } + } + }, + AnyCssRule::CssQualifiedRule(parent_rule) => { + println!(" parent = QR"); + for selector in parent_rule.prelude() { + if let Ok(selector) = selector { + println!(" selectors = {:?}", selector.clone().text()); + parent_selectors.push(selector.text()); } - }, - } - + } + }, } } let resolved_selectors: Vec = parent_selectors.iter().fold(vec!(), |result: Vec, parent_selector|{ - match &parent_node { - Some(parent_node) => { - if selector.contains("&") { - let resolved_parent_selectors = resolve_nested_selectors(parent_selector.to_string(), parent_node.clone()); - let resolved = resolved_parent_selectors.into_iter().map(|newly_resolved|{ - // TODO: Need to handle the case where an equivalent is the result of an & - // e.g. .a.c { &.b } == .a.b.c but the order will be a.c.b - 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_node.clone()); - return [result, resolved].concat(); - } - } - None => result, + 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 { @@ -146,9 +255,9 @@ fn resolve_nested_selectors(selector: String, block: CssSyntaxNode) -> Vec Option { - if let Some(parent_rule) = this_block.parent() { - return parent_rule.grand_parent(); +fn get_parent_rule(this_rule: CssSyntaxNode) -> Option { + if let Some(parent_list) = this_rule.parent() { + return parent_list.grand_parent(); } return None } diff --git a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css index 2f9d03731af..106ddc7d02f 100644 --- a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css +++ b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css @@ -24,7 +24,7 @@ i {} h {} /* duplicate selector lists with different order */ -j, k {} j, k {} +j, k {} k, j {} /* duplicate selectors with multiple components */ m n {} @@ -42,7 +42,7 @@ s {} /* duplicate within a media query, in different rules */ t {} -@media print { t {} t {} } +@media screen and (min-width: 900px) { t {} t {} } /* duplicate caused by nesting */ v w {} v { w {} } diff --git a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css.snap.new b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css.snap.new index 8c80ab4df51..eedc80d86fa 100644 --- a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css.snap.new +++ b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css.snap.new @@ -31,7 +31,7 @@ i {} h {} /* duplicate selector lists with different order */ -j, k {} j, k {} +j, k {} k, j {} /* duplicate selectors with multiple components */ m n {} @@ -49,7 +49,7 @@ s {} /* duplicate within a media query, in different rules */ t {} -@media print { t {} t {} } +@media screen and (min-width: 900px) { t {} t {} } /* duplicate caused by nesting */ v w {} v { w {} } @@ -190,48 +190,14 @@ invalid.css:27:9 lint/nursery/noDuplicateSelectors ━━━━━━━━━ ! Duplicate selector. 26 │ /* duplicate selector lists with different order */ - > 27 │ j, k {} j, k {} - │ ^ + > 27 │ j, k {} k, j {} + │ ^^^^^ 28 │ 29 │ /* duplicate selectors with multiple components */ i Please fix it. -``` - -``` -invalid.css:27:12 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - - ! Duplicate selector. - - 26 │ /* duplicate selector lists with different order */ - > 27 │ j, k {} j, k {} - │ ^^ - 28 │ - 29 │ /* duplicate selectors with multiple components */ - - i Please fix it. - - -``` - -``` -invalid.css:30:7 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - - ! Duplicate selector. - - 29 │ /* duplicate selectors with multiple components */ - > 30 │ m n {} - │ - > 31 │ m n {} - │ ^ - 32 │ - 33 │ /* essentially duplicate selector lists with varied spacing */ - - i Please fix it. - - ``` ``` @@ -250,110 +216,6 @@ invalid.css:30:7 lint/nursery/noDuplicateSelectors ━━━━━━━━━ i Please fix it. -``` - -``` -invalid.css:31:3 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - - ! Duplicate selector. - - 29 │ /* duplicate selectors with multiple components */ - 30 │ m n {} - > 31 │ m n {} - │ ^^ - 32 │ - 33 │ /* essentially duplicate selector lists with varied spacing */ - - i Please fix it. - - -``` - -``` -invalid.css:36:8 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - - ! Duplicate selector. - - 34 │ .foo p, q > .bar, - 35 │ baz {} - > 36 │ #baz, - │ - > 37 │ .foo p,q>.bar {} - │ ^^^^ - 38 │ - 39 │ /* duplicate within a media query, in the same rule */ - - i Please fix it. - - -``` - -``` -invalid.css:37:12 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - - ! Duplicate selector. - - 35 │ baz {} - 36 │ #baz, - > 37 │ .foo p,q>.bar {} - │ ^ - 38 │ - 39 │ /* duplicate within a media query, in the same rule */ - - i Please fix it. - - -``` - -``` -invalid.css:37:14 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - - ! Duplicate selector. - - 35 │ baz {} - 36 │ #baz, - > 37 │ .foo p,q>.bar {} - │ ^ - 38 │ - 39 │ /* duplicate within a media query, in the same rule */ - - i Please fix it. - - -``` - -``` -invalid.css:37:16 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - - ! Duplicate selector. - - 35 │ baz {} - 36 │ #baz, - > 37 │ .foo p,q>.bar {} - │ ^^^^^ - 38 │ - 39 │ /* duplicate within a media query, in the same rule */ - - i Please fix it. - - -``` - -``` -invalid.css:41:16 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - - ! Duplicate selector. - - 39 │ /* duplicate within a media query, in the same rule */ - 40 │ s {} - > 41 │ @media print { s, s {} } - │ ^ - 42 │ - 43 │ /* duplicate within a media query, in different rules */ - - i Please fix it. - - ``` ``` @@ -391,14 +253,14 @@ invalid.css:41:19 lint/nursery/noDuplicateSelectors ━━━━━━━━━ ``` ``` -invalid.css:45:16 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +invalid.css:45:45 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ ! Duplicate selector. 43 │ /* duplicate within a media query, in different rules */ 44 │ t {} - > 45 │ @media print { t {} t {} } - │ ^^ + > 45 │ @media screen and (min-width: 900px) { t {} t {} } + │ ^^ 46 │ 47 │ /* duplicate caused by nesting */ @@ -408,14 +270,14 @@ invalid.css:45:16 lint/nursery/noDuplicateSelectors ━━━━━━━━━ ``` ``` -invalid.css:45:21 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +invalid.css:45:45 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ ! Duplicate selector. 43 │ /* duplicate within a media query, in different rules */ 44 │ t {} - > 45 │ @media print { t {} t {} } - │ ^^ + > 45 │ @media screen and (min-width: 900px) { t {} t {} } + │ ^^ 46 │ 47 │ /* duplicate caused by nesting */ @@ -425,30 +287,13 @@ invalid.css:45:21 lint/nursery/noDuplicateSelectors ━━━━━━━━━ ``` ``` -invalid.css:45:21 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - - ! Duplicate selector. - - 43 │ /* duplicate within a media query, in different rules */ - 44 │ t {} - > 45 │ @media print { t {} t {} } - │ ^^ - 46 │ - 47 │ /* duplicate caused by nesting */ - - i Please fix it. - - -``` - -``` -invalid.css:48:8 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +invalid.css:48:12 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ ! Duplicate selector. 47 │ /* duplicate caused by nesting */ > 48 │ v w {} v { w {} } - │ ^^ + │ ^^ 49 │ 50 │ /* duplicate caused by &-parent selector */ @@ -458,15 +303,13 @@ invalid.css:48:8 lint/nursery/noDuplicateSelectors ━━━━━━━━━ ``` ``` -invalid.css:48:12 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +invalid.css:51:5 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ ! Duplicate selector. - 47 │ /* duplicate caused by nesting */ - > 48 │ v w {} v { w {} } - │ ^^ - 49 │ 50 │ /* duplicate caused by &-parent selector */ + > 51 │ x { & {} } + │ ^^ i Please fix it. diff --git a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/valid.css.snap.new b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/valid.css.snap.new index 82dde9afbe5..e794a63750c 100644 --- a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/valid.css.snap.new +++ b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/valid.css.snap.new @@ -40,208 +40,3 @@ ul, ol {} ul {} [disabled].goo, [disabled] .goo {} ``` - -# Diagnostics -``` -valid.css:6:16 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - - ! Duplicate selector. - - 4 │ /* duplicate inside media query */ - 5 │ g {} - > 6 │ @media print { g {} }→ - │ ^^ - 7 │ - 8 │ /* duplicate inside keyframes */ - - i Please fix it. - - -``` - -``` -valid.css:13:5 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - - ! Duplicate selector. - - 12 │ /* duplicates inside nested rules */ - > 13 │ i { i { i {} } } - │ ^^ - 14 │ - 15 │ /* selectors using parts of other selectors */ - - i Please fix it. - - -``` - -``` -valid.css:16:13 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - - ! Duplicate selector. - - 15 │ /* selectors using parts of other selectors */ - > 16 │ .foo .bar {} - │ - > 17 │ .foo {} - │ ^^^^^ - 18 │ .bar {} - 19 │ .bar .foo {} - - i Please fix it. - - -``` - -``` -valid.css:17:8 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - - ! Duplicate selector. - - 15 │ /* selectors using parts of other selectors */ - 16 │ .foo .bar {} - > 17 │ .foo {} - │ - > 18 │ .bar {} - │ ^^^^^ - 19 │ .bar .foo {} - 20 │ - - i Please fix it. - - -``` - -``` -valid.css:18:8 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - - ! Duplicate selector. - - 16 │ .foo .bar {} - 17 │ .foo {} - > 18 │ .bar {} - │ - > 19 │ .bar .foo {} - │ ^^^^ - 20 │ - 21 │ /* selectors reused in other non-equivalent selector lists */ - - i Please fix it. - - -``` - -``` -valid.css:19:6 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - - ! Duplicate selector. - - 17 │ .foo {} - 18 │ .bar {} - > 19 │ .bar .foo {} - │ ^^^^^ - 20 │ - 21 │ /* selectors reused in other non-equivalent selector lists */ - - i Please fix it. - - -``` - -``` -valid.css:22:6 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - - ! Duplicate selector. - - 21 │ /* selectors reused in other non-equivalent selector lists */ - > 22 │ j {} j, k {} - │ ^ - 23 │ - 24 │ /* nested resolution */ - - i Please fix it. - - -``` - -``` -valid.css:25:17 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - - ! Duplicate selector. - - 24 │ /* nested resolution */ - > 25 │ m n { top: 0; } m { n, p { color: pink; } }→ - │ ^^ - 26 │ - 27 │ @mixin abc { &:hover {} } @mixin xyz { &:hover {} } - - i Please fix it. - - -``` - -``` -valid.css:25:21 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - - ! Duplicate selector. - - 24 │ /* nested resolution */ - > 25 │ m n { top: 0; } m { n, p { color: pink; } }→ - │ ^ - 26 │ - 27 │ @mixin abc { &:hover {} } @mixin xyz { &:hover {} } - - i Please fix it. - - -``` - -``` -valid.css:27:28 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - - ! Duplicate selector. - - 25 │ m n { top: 0; } m { n, p { color: pink; } }→ - 26 │ - > 27 │ @mixin abc { &:hover {} } @mixin xyz { &:hover {} } - │ ^^^^^ - 28 │ - 29 │ /* pass if default setting i.e. disallowInList: false */ - - i Please fix it. - - -``` - -``` -valid.css:27:40 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - - ! Duplicate selector. - - 25 │ m n { top: 0; } m { n, p { color: pink; } }→ - 26 │ - > 27 │ @mixin abc { &:hover {} } @mixin xyz { &:hover {} } - │ ^^^^^^^^ - 28 │ - 29 │ /* pass if default setting i.e. disallowInList: false */ - - i Please fix it. - - -``` - -``` -valid.css:30:11 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - - ! Duplicate selector. - - 29 │ /* pass if default setting i.e. disallowInList: false */ - > 30 │ ul, ol {} ul {} - │ ^^^ - 31 │ - 32 │ /* attribute selector */ - - i Please fix it. - - -``` From 39658868c5b909b22472ea75778685c9f8a53c46 Mon Sep 17 00:00:00 2001 From: Abid Jappie Date: Sat, 4 May 2024 00:03:49 +0900 Subject: [PATCH 07/20] WIP: Fixed test case. some cleaning. --- .../lint/nursery/no_duplicate_selectors.rs | 36 ++++++++++++++----- .../nursery/noDuplicateSelectors/invalid.css | 2 +- .../noDuplicateSelectors/invalid.css.snap.new | 22 +++++++++++- .../noDuplicateSelectors/valid.css.snap.new | 1 + 4 files changed, 51 insertions(+), 10 deletions(-) diff --git a/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs b/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs index 192a652486e..f29808a78dd 100644 --- a/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs +++ b/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs @@ -1,12 +1,12 @@ use std::collections::HashSet; use std::vec; -use biome_analyze::{Ast, VisitorContext}; +use biome_analyze::Ast; use biome_analyze::{context::RuleContext, declare_rule, Rule, RuleDiagnostic, RuleSource}; use biome_console::markup; use biome_deserialize_macros::Deserializable; -use biome_css_syntax::{kind, AnyCssAtRule, AnyCssRelativeSelector, AnyCssRule, AnyCssSelector, AnyCssSubSelector, CssComplexSelector, CssMediaAtRule, CssRelativeSelector, CssRelativeSelectorList, CssRoot, CssRuleList, CssSelectorList, CssSyntaxKind, CssSyntaxNode}; -use biome_rowan::{AstNode, Language, SyntaxKindSet, SyntaxNodeCast}; +use biome_css_syntax::{AnyCssAtRule, AnyCssRelativeSelector, AnyCssRule, AnyCssSelector, CssComplexSelector, CssRelativeSelector, CssRelativeSelectorList, CssRuleList, CssSelectorList, CssSyntaxNode}; +use biome_rowan::{AstNode, SyntaxNodeCast}; use serde::{Deserialize, Serialize}; @@ -49,7 +49,7 @@ impl Rule for NoDuplicateSelectors { let mut resolved_list = HashSet::new(); let mut output: Vec = vec!(); - if (options.disallow_in_list) { + if options.disallow_in_list { let selectors = node .syntax() .descendants() @@ -81,10 +81,11 @@ impl Rule for NoDuplicateSelectors { let mut selector_text = String::new(); if let Some(selector) = CssRelativeSelector::cast_ref(&selector){ - selector_text = selector.clone().text() + selector_text = selector.clone().text(); } if let Some(selector) = AnyCssSelector::cast_ref(&selector){ - selector_text = selector.clone().text() + // TODO: test if this needs to be normalized + selector_text = selector.clone().text(); } let resolved = resolve_nested_selectors(selector_text, this_rule); @@ -120,10 +121,11 @@ impl Rule for NoDuplicateSelectors { .into_iter() .filter_map(|child|{ if let Some(selector) = AnyCssSelector::cast_ref(&child) { - if !this_list_resolved_list.insert(selector.text()) { + let selector_text = normalize_complex_selector(selector); + if !this_list_resolved_list.insert(selector_text.clone()) { output.push(child.clone()); } - return Some(selector.text()); + return Some(selector_text); } else if let Some(selector) = AnyCssRelativeSelector::cast_ref(&child) { if !this_list_resolved_list.insert(selector.text()) { output.push(child.clone()); @@ -262,3 +264,21 @@ fn get_parent_rule(this_rule: CssSyntaxNode) -> Option { return None } +fn normalize_complex_selector(selector: AnyCssSelector) -> String { + let mut selector_text = String::new(); + + if let Some(complex_selector) = CssComplexSelector::cast_ref(&selector.clone().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(); +} \ No newline at end of file diff --git a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css index 106ddc7d02f..f2d01f0ab8d 100644 --- a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css +++ b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css @@ -32,7 +32,7 @@ m n {} /* essentially duplicate selector lists with varied spacing */ .foo p, q > .bar, -baz {} +#baz {} #baz, .foo p,q>.bar {} diff --git a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css.snap.new b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css.snap.new index eedc80d86fa..77ec723101d 100644 --- a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css.snap.new +++ b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css.snap.new @@ -39,7 +39,7 @@ m n {} /* essentially duplicate selector lists with varied spacing */ .foo p, q > .bar, -baz {} +#baz {} #baz, .foo p,q>.bar {} @@ -216,6 +216,26 @@ invalid.css:30:7 lint/nursery/noDuplicateSelectors ━━━━━━━━━ i Please fix it. +``` + +``` +invalid.css:35:8 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Duplicate selector. + + 33 │ /* essentially duplicate selector lists with varied spacing */ + 34 │ .foo p, q > .bar, + > 35 │ #baz {} + │ + > 36 │ #baz, + > 37 │ .foo p,q>.bar {} + │ ^^^^^^^^^^^^^^^^^^ + 38 │ + 39 │ /* duplicate within a media query, in the same rule */ + + i Please fix it. + + ``` ``` diff --git a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/valid.css.snap.new b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/valid.css.snap.new index e794a63750c..5ea73dc7dcc 100644 --- a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/valid.css.snap.new +++ b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/valid.css.snap.new @@ -27,6 +27,7 @@ i { i { i {} } } /* selectors reused in other non-equivalent selector lists */ j {} j, k {} +u {} v, u {} /* nested resolution */ m n { top: 0; } m { n, p { color: pink; } } From 3d58e8e2aea2f6dff41b03d459c51862f089dc0d Mon Sep 17 00:00:00 2001 From: Abid Jappie Date: Sun, 5 May 2024 22:05:08 +0900 Subject: [PATCH 08/20] All the current valid and invalid cases are now working as expected. TODO: add tests for Options. --- .../lint/nursery/no_duplicate_selectors.rs | 153 +++++++++++++----- .../noDuplicateSelectors/invalid.css.snap.new | 95 ++++++----- .../nursery/noDuplicateSelectors/valid.css | 1 + 3 files changed, 164 insertions(+), 85 deletions(-) diff --git a/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs b/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs index f29808a78dd..7d58360633c 100644 --- a/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs +++ b/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs @@ -1,17 +1,36 @@ +use std::borrow::Borrow; use std::collections::HashSet; +use std::hash::{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_deserialize_macros::Deserializable; -use biome_css_syntax::{AnyCssAtRule, AnyCssRelativeSelector, AnyCssRule, AnyCssSelector, CssComplexSelector, CssRelativeSelector, CssRelativeSelectorList, CssRuleList, CssSelectorList, CssSyntaxNode}; +use biome_css_syntax::{AnyCssAtRule, AnyCssRelativeSelector, AnyCssRule, AnyCssSelector, CssComplexSelector, CssRelativeSelector, CssRelativeSelectorList, CssRoot, CssRuleList, CssSelectorList, CssSyntaxNode}; use biome_rowan::{AstNode, SyntaxNodeCast}; use serde::{Deserialize, Serialize}; declare_rule! { + /// Disallow duplicate selectors. /// + /// ## Examples + /// + /// ### Invalid + /// + /// ```css,expect_diagnostic + /// .abc, + /// .def, + /// .abc {} + /// ``` + /// + /// ### Valid + /// + /// ``` + /// .foo {} + /// .bar {} + /// ``` /// pub NoDuplicateSelectors { version: "next", @@ -36,9 +55,36 @@ impl Default for NoDuplicateSelectorsOptions { } } +#[derive(Debug, Eq)] +struct ResolvedSelector { + 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(&self, state: &mut H) { + self.selector_text.hash(state); + } +} +impl Borrow for ResolvedSelector { + fn borrow(&self) -> &String { + &self.selector_text + } +} + +pub struct DuplicateSelector { + first: CssSyntaxNode, + duplicate: CssSyntaxNode +} + impl Rule for NoDuplicateSelectors { - type Query = Ast; - type State = CssSyntaxNode; + type Query = Ast; + type State = DuplicateSelector; type Signals = Vec; type Options = NoDuplicateSelectorsOptions; @@ -46,11 +92,12 @@ impl Rule for NoDuplicateSelectors { let node = ctx.query(); let options = ctx.options(); - let mut resolved_list = HashSet::new(); - let mut output: Vec = vec!(); + let mut resolved_list: HashSet = HashSet::new(); + let mut output: Vec = vec!(); if options.disallow_in_list { let selectors = node + .rules() .syntax() .descendants() .filter_map(|x|{ @@ -92,14 +139,23 @@ impl Rule for NoDuplicateSelectors { for r in resolved { let split: Vec<&str> = r.split_whitespace().collect(); let normalized = split.join(" ").to_lowercase(); - println!("resolved: {:?}", normalized); - if !resolved_list.insert(normalized) { - output.push(selector.clone()); + if !resolved_list.insert(ResolvedSelector { + selector_text: normalized.clone(), + selector_node: selector.clone(), + }) { + let first = resolved_list.get(&normalized); + if let Some(first) = first { + output.push(DuplicateSelector { + first: first.selector_node.clone(), + duplicate: selector.clone() + }); + } } } } } else { let select_lists = node + .rules() .syntax() .descendants() .filter_map(|x|{ @@ -113,7 +169,7 @@ impl Rule for NoDuplicateSelectors { }); for selector_list in select_lists { - let mut this_list_resolved_list = HashSet::new(); + let mut this_list_resolved_list: HashSet = HashSet::new(); let this_rule = selector_list.parent().unwrap(); let mut selector_list_mapped: Vec = selector_list @@ -121,14 +177,36 @@ impl Rule for NoDuplicateSelectors { .into_iter() .filter_map(|child|{ if let Some(selector) = AnyCssSelector::cast_ref(&child) { - let selector_text = normalize_complex_selector(selector); - if !this_list_resolved_list.insert(selector_text.clone()) { - output.push(child.clone()); + let selector_text = normalize_complex_selector(selector.clone()); + if !this_list_resolved_list.insert(ResolvedSelector { + selector_text: selector_text.clone(), + selector_node: selector.clone().into(), + }) { + let first = this_list_resolved_list.get(&selector_text); + if let Some(first) = first { + output.push(DuplicateSelector { + first: first.selector_node.clone(), + duplicate: selector.into() + }); + // Return a none, since we already addressed this case + return None + } } return Some(selector_text); } else if let Some(selector) = AnyCssRelativeSelector::cast_ref(&child) { - if !this_list_resolved_list.insert(selector.text()) { - output.push(child.clone()); + if !this_list_resolved_list.insert(ResolvedSelector { + selector_text: selector.clone().text(), + selector_node: selector.clone().into(), + }) { + let first = this_list_resolved_list.get(&selector.text()); + if let Some(first) = first { + output.push(DuplicateSelector { + first: first.selector_node.clone(), + duplicate: selector.into() + }); + // Return a none, since we already addressed this case + return None; + } } return Some(selector.text()); } @@ -143,9 +221,17 @@ impl Rule for NoDuplicateSelectors { for r in resolved { let split: Vec<&str> = r.split_whitespace().collect(); let normalized = split.join(" ").to_lowercase(); - println!("resolved: {:?}", normalized); - if !resolved_list.insert(normalized) { - output.push(selector_list.clone()); + if !resolved_list.insert(ResolvedSelector { + selector_text: normalized.clone(), + selector_node: selector_list.clone().into(), + }) { + let first = resolved_list.get(&normalized); + if let Some(first) = first { + output.push(DuplicateSelector { + first: first.selector_node.clone(), + duplicate: selector_list.clone() + }); + } } } } @@ -158,16 +244,16 @@ impl Rule for NoDuplicateSelectors { // Read our guidelines to write great diagnostics: // https://docs.rs/biome_analyze/latest/biome_analyze/#what-a-rule-should-say-to-the-user // + let first_seen = node.first.to_string(); + let duplicate = node.duplicate.to_string(); Some( RuleDiagnostic::new( rule_category!(), - node.text_range(), + node.duplicate.text_range(), markup! { - "Duplicate selector." - }, - ).note(markup! { - "Please fix it." - }), + "Duplicate selector \""{duplicate}"\", first seen at"{first_seen}"." + } + ) ) } } @@ -179,17 +265,15 @@ fn resolve_nested_selectors(selector: String, this_rule: CssSyntaxNode) -> Vec { - println!(" parent = none"); return vec!(selector) }, Some(parent_rule) => { if let Some(parent_rule) = AnyCssAtRule::cast_ref(&parent_rule) { - println!(" parent is any css at rule: {:?}", parent_rule.clone().text()); // resolve match parent_rule { - AnyCssAtRule::CssContainerAtRule(rule) => todo!(), - AnyCssAtRule::CssKeyframesAtRule(rule) => todo!(), - AnyCssAtRule::CssLayerAtRule(rule) => todo!(), + AnyCssAtRule::CssContainerAtRule(_rule) => todo!(), + AnyCssAtRule::CssKeyframesAtRule(_rule) => todo!(), + AnyCssAtRule::CssLayerAtRule(_rule) => todo!(), AnyCssAtRule::CssMediaAtRule(rule) => { let mut resolved = "@".to_string(); resolved.push_str(&rule.media_token().unwrap().text()); @@ -198,10 +282,10 @@ fn resolve_nested_selectors(selector: String, this_rule: CssSyntaxNode) -> Vec todo!(), - AnyCssAtRule::CssScopeAtRule(rule) => todo!(), - AnyCssAtRule::CssStartingStyleAtRule(rule) => todo!(), - AnyCssAtRule::CssSupportsAtRule(rule) => todo!(), + AnyCssAtRule::CssPageAtRule(_rule) => todo!(), + AnyCssAtRule::CssScopeAtRule(_rule) => todo!(), + AnyCssAtRule::CssStartingStyleAtRule(_rule) => todo!(), + AnyCssAtRule::CssSupportsAtRule(_rule) => todo!(), _ => {} } } @@ -211,23 +295,18 @@ fn resolve_nested_selectors(selector: String, this_rule: CssSyntaxNode) -> Vec { // Treat the AtRule as a selector let rule = parent_rule.rule().unwrap(); - println!(" selectors = {:?}", rule.clone().text()); parent_selectors.push(rule.text()); }, AnyCssRule::CssNestedQualifiedRule(parent_rule) => { - println!(" parent = NQR"); for selector in parent_rule.prelude() { if let Ok(selector) = selector { - println!(" selectors = {:?}", selector.clone().text()); parent_selectors.push(selector.text()); } } }, AnyCssRule::CssQualifiedRule(parent_rule) => { - println!(" parent = QR"); for selector in parent_rule.prelude() { if let Ok(selector) = selector { - println!(" selectors = {:?}", selector.clone().text()); parent_selectors.push(selector.text()); } } diff --git a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css.snap.new b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css.snap.new index 77ec723101d..eeca655e904 100644 --- a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css.snap.new +++ b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css.snap.new @@ -62,7 +62,8 @@ x { & {} } ``` invalid.css:2:4 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selector. + ! Duplicate selector, first seen at/* duplicate within one rule's selector list */ + a 1 │ /* duplicate within one rule's selector list */ > 2 │ a, a {} @@ -78,7 +79,10 @@ invalid.css:2:4 lint/nursery/noDuplicateSelectors ━━━━━━━━━━ ``` invalid.css:5:3 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selector. + ! Duplicate selector, first seen at + + /* duplicate within one rule's selector list. multiline */ + b 4 │ /* duplicate within one rule's selector list. multiline */ > 5 │ b, @@ -96,7 +100,8 @@ invalid.css:5:3 lint/nursery/noDuplicateSelectors ━━━━━━━━━━ ``` invalid.css:10:3 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selector. + ! Duplicate selector, first seen at + d 8 │ /* duplicate within one rule's selector list. multiline */ 9 │ c, @@ -115,7 +120,10 @@ invalid.css:10:3 lint/nursery/noDuplicateSelectors ━━━━━━━━━ ``` invalid.css:14:4 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selector. + ! Duplicate selector, first seen at + + /* duplicated selectors within one rule's selector list. 3 duplicates */ + .e 13 │ /* duplicated selectors within one rule's selector list. 3 duplicates */ > 14 │ .e, @@ -133,7 +141,10 @@ invalid.css:14:4 lint/nursery/noDuplicateSelectors ━━━━━━━━━ ``` invalid.css:15:4 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selector. + ! Duplicate selector, first seen at + + /* duplicated selectors within one rule's selector list. 3 duplicates */ + .e 13 │ /* duplicated selectors within one rule's selector list. 3 duplicates */ 14 │ .e, @@ -152,7 +163,10 @@ invalid.css:15:4 lint/nursery/noDuplicateSelectors ━━━━━━━━━ ``` invalid.css:19:11 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selector. + ! Duplicate selector, first seen at + + /* duplicate simple selectors with another rule between */ + f 18 │ /* duplicate simple selectors with another rule between */ > 19 │ f {} g {} f {} @@ -168,7 +182,10 @@ invalid.css:19:11 lint/nursery/noDuplicateSelectors ━━━━━━━━━ ``` invalid.css:23:5 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selector. + ! Duplicate selector, first seen at + + /* duplicate simple selectors with another rule between */ + h 21 │ /* duplicate simple selectors with another rule between */ 22 │ h {} @@ -187,7 +204,10 @@ invalid.css:23:5 lint/nursery/noDuplicateSelectors ━━━━━━━━━ ``` invalid.css:27:9 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selector. + ! Duplicate selector, first seen at + + /* duplicate selector lists with different order */ + j, k 26 │ /* duplicate selector lists with different order */ > 27 │ j, k {} k, j {} @@ -203,7 +223,10 @@ invalid.css:27:9 lint/nursery/noDuplicateSelectors ━━━━━━━━━ ``` invalid.css:30:7 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selector. + ! Duplicate selector, first seen at + + /* duplicate selectors with multiple components */ + m n 29 │ /* duplicate selectors with multiple components */ > 30 │ m n {} @@ -221,7 +244,11 @@ invalid.css:30:7 lint/nursery/noDuplicateSelectors ━━━━━━━━━ ``` invalid.css:35:8 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selector. + ! Duplicate selector, first seen at + + /* essentially duplicate selector lists with varied spacing */ + .foo p, q > .bar, + #baz 33 │ /* essentially duplicate selector lists with varied spacing */ 34 │ .foo p, q > .bar, @@ -241,7 +268,7 @@ invalid.css:35:8 lint/nursery/noDuplicateSelectors ━━━━━━━━━ ``` invalid.css:41:19 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selector. + ! Duplicate selector, first seen ats 39 │ /* duplicate within a media query, in the same rule */ 40 │ s {} @@ -253,46 +280,12 @@ invalid.css:41:19 lint/nursery/noDuplicateSelectors ━━━━━━━━━ i Please fix it. -``` - -``` -invalid.css:41:19 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - - ! Duplicate selector. - - 39 │ /* duplicate within a media query, in the same rule */ - 40 │ s {} - > 41 │ @media print { s, s {} } - │ ^^ - 42 │ - 43 │ /* duplicate within a media query, in different rules */ - - i Please fix it. - - -``` - -``` -invalid.css:45:45 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - - ! Duplicate selector. - - 43 │ /* duplicate within a media query, in different rules */ - 44 │ t {} - > 45 │ @media screen and (min-width: 900px) { t {} t {} } - │ ^^ - 46 │ - 47 │ /* duplicate caused by nesting */ - - i Please fix it. - - ``` ``` invalid.css:45:45 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selector. + ! Duplicate selector, first seen att 43 │ /* duplicate within a media query, in different rules */ 44 │ t {} @@ -309,7 +302,10 @@ invalid.css:45:45 lint/nursery/noDuplicateSelectors ━━━━━━━━━ ``` invalid.css:48:12 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selector. + ! Duplicate selector, first seen at + + /* duplicate caused by nesting */ + v w 47 │ /* duplicate caused by nesting */ > 48 │ v w {} v { w {} } @@ -325,7 +321,10 @@ invalid.css:48:12 lint/nursery/noDuplicateSelectors ━━━━━━━━━ ``` invalid.css:51:5 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selector. + ! Duplicate selector, first seen at + + /* duplicate caused by &-parent selector */ + x 50 │ /* duplicate caused by &-parent selector */ > 51 │ x { & {} } diff --git a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/valid.css b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/valid.css index e3e18231728..94dbb129e7f 100644 --- a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/valid.css +++ b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/valid.css @@ -20,6 +20,7 @@ i { i { i {} } } /* selectors reused in other non-equivalent selector lists */ j {} j, k {} +u {} v, u {} /* nested resolution */ m n { top: 0; } m { n, p { color: pink; } } From 3ed2b905fe89e4bfbe6ee57f6915da76a97cc451 Mon Sep 17 00:00:00 2001 From: Abid Jappie Date: Mon, 6 May 2024 00:02:39 +0900 Subject: [PATCH 09/20] Added option test cases, updated the diagnostics. WIP: cleanup. --- .../lint/nursery/no_duplicate_selectors.rs | 19 +- .../noDuplicateSelectors/disallowInList.css | 10 + .../disallowInList.css.snap.new | 19 ++ .../disallowInList.options.json | 15 ++ .../noDuplicateSelectors/invalid.css.snap.new | 231 +++++++++++++----- .../@biomejs/biome/configuration_schema.json | 27 ++ 6 files changed, 257 insertions(+), 64 deletions(-) create mode 100644 crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/disallowInList.css create mode 100644 crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/disallowInList.css.snap.new create mode 100644 crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/disallowInList.options.json diff --git a/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs b/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs index 7d58360633c..ab6dd5cf093 100644 --- a/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs +++ b/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs @@ -32,6 +32,20 @@ declare_rule! { /// .bar {} /// ``` /// + /// ## Options + /// + /// If true, disallow duplicate selectors within selector lists. + /// + /// ```json + /// { + /// "noDuplicateSelectors": { + /// "options": { + /// "disallowInList": true + /// } + /// } + /// } + /// ``` + /// pub NoDuplicateSelectors { version: "next", name: "noDuplicateSelectors", @@ -244,16 +258,15 @@ impl Rule for NoDuplicateSelectors { // Read our guidelines to write great diagnostics: // https://docs.rs/biome_analyze/latest/biome_analyze/#what-a-rule-should-say-to-the-user // - let first_seen = node.first.to_string(); let duplicate = node.duplicate.to_string(); Some( RuleDiagnostic::new( rule_category!(), node.duplicate.text_range(), markup! { - "Duplicate selector \""{duplicate}"\", first seen at"{first_seen}"." + "Duplicate selector \""{duplicate}"\"," } - ) + ).detail(node.first.text_range(), "first occurence:") ) } } diff --git a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/disallowInList.css b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/disallowInList.css new file mode 100644 index 00000000000..d7429942ed4 --- /dev/null +++ b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/disallowInList.css @@ -0,0 +1,10 @@ +/* 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 {} diff --git a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/disallowInList.css.snap.new b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/disallowInList.css.snap.new new file mode 100644 index 00000000000..ab7262a5ec7 --- /dev/null +++ b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/disallowInList.css.snap.new @@ -0,0 +1,19 @@ +--- +source: crates/biome_css_analyze/tests/spec_tests.rs +assertion_line: 83 +expression: disallowInList.css +--- +# Input +```css +/* 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 {} + +``` diff --git a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/disallowInList.options.json b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/disallowInList.options.json new file mode 100644 index 00000000000..1c5b1b9db7d --- /dev/null +++ b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/disallowInList.options.json @@ -0,0 +1,15 @@ +{ + "$schema": "../../../../../../packages/@biomejs/biome/configuration_schema.json", + "linter": { + "rules": { + "nursery": { + "noDuplicateSelectors": { + "level": "error", + "options": { + "disallowInList": true + } + } + } + } + } +} diff --git a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css.snap.new b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css.snap.new index eeca655e904..3859541fdb6 100644 --- a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css.snap.new +++ b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css.snap.new @@ -62,8 +62,7 @@ x { & {} } ``` invalid.css:2:4 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selector, first seen at/* duplicate within one rule's selector list */ - a + ! Duplicate selector "a ", 1 │ /* duplicate within one rule's selector list */ > 2 │ a, a {} @@ -71,7 +70,14 @@ invalid.css:2:4 lint/nursery/noDuplicateSelectors ━━━━━━━━━━ 3 │ 4 │ /* duplicate within one rule's selector list. multiline */ - i Please fix it. + i first occurence: + + > 1 │ /* duplicate within one rule's selector list */ + │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + > 2 │ a, a {} + │ ^ + 3 │ + 4 │ /* duplicate within one rule's selector list. multiline */ ``` @@ -79,10 +85,8 @@ invalid.css:2:4 lint/nursery/noDuplicateSelectors ━━━━━━━━━━ ``` invalid.css:5:3 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selector, first seen at - - /* duplicate within one rule's selector list. multiline */ - b + ! Duplicate selector " + b ", 4 │ /* duplicate within one rule's selector list. multiline */ > 5 │ b, @@ -92,7 +96,17 @@ invalid.css:5:3 lint/nursery/noDuplicateSelectors ━━━━━━━━━━ 7 │ 8 │ /* duplicate within one rule's selector list. multiline */ - i Please fix it. + i first occurence: + + 1 │ /* duplicate within one rule's selector list */ + > 2 │ a, a {} + │ + > 3 │ + > 4 │ /* duplicate within one rule's selector list. multiline */ + > 5 │ b, + │ ^ + 6 │ b {} + 7 │ ``` @@ -100,8 +114,8 @@ invalid.css:5:3 lint/nursery/noDuplicateSelectors ━━━━━━━━━━ ``` invalid.css:10:3 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selector, first seen at - d + ! Duplicate selector " + d ", 8 │ /* duplicate within one rule's selector list. multiline */ 9 │ c, @@ -112,7 +126,15 @@ invalid.css:10:3 lint/nursery/noDuplicateSelectors ━━━━━━━━━ 12 │ 13 │ /* duplicated selectors within one rule's selector list. 3 duplicates */ - i Please fix it. + i first occurence: + + 8 │ /* duplicate within one rule's selector list. multiline */ + > 9 │ c, + │ + > 10 │ d, + │ ^ + 11 │ d {} + 12 │ ``` @@ -120,10 +142,8 @@ invalid.css:10:3 lint/nursery/noDuplicateSelectors ━━━━━━━━━ ``` invalid.css:14:4 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selector, first seen at - - /* duplicated selectors within one rule's selector list. 3 duplicates */ - .e + ! Duplicate selector " + .e", 13 │ /* duplicated selectors within one rule's selector list. 3 duplicates */ > 14 │ .e, @@ -133,7 +153,18 @@ invalid.css:14:4 lint/nursery/noDuplicateSelectors ━━━━━━━━━ 16 │ .e {} 17 │ - i Please fix it. + i first occurence: + + 9 │ c, + 10 │ d, + > 11 │ d {} + │ + > 12 │ + > 13 │ /* duplicated selectors within one rule's selector list. 3 duplicates */ + > 14 │ .e, + │ ^^ + 15 │ .e, + 16 │ .e {} ``` @@ -141,10 +172,8 @@ invalid.css:14:4 lint/nursery/noDuplicateSelectors ━━━━━━━━━ ``` invalid.css:15:4 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selector, first seen at - - /* duplicated selectors within one rule's selector list. 3 duplicates */ - .e + ! Duplicate selector " + .e ", 13 │ /* duplicated selectors within one rule's selector list. 3 duplicates */ 14 │ .e, @@ -155,7 +184,18 @@ invalid.css:15:4 lint/nursery/noDuplicateSelectors ━━━━━━━━━ 17 │ 18 │ /* duplicate simple selectors with another rule between */ - i Please fix it. + i first occurence: + + 9 │ c, + 10 │ d, + > 11 │ d {} + │ + > 12 │ + > 13 │ /* duplicated selectors within one rule's selector list. 3 duplicates */ + > 14 │ .e, + │ ^^ + 15 │ .e, + 16 │ .e {} ``` @@ -163,10 +203,7 @@ invalid.css:15:4 lint/nursery/noDuplicateSelectors ━━━━━━━━━ ``` invalid.css:19:11 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selector, first seen at - - /* duplicate simple selectors with another rule between */ - f + ! Duplicate selector "f ", 18 │ /* duplicate simple selectors with another rule between */ > 19 │ f {} g {} f {} @@ -174,7 +211,18 @@ invalid.css:19:11 lint/nursery/noDuplicateSelectors ━━━━━━━━━ 20 │ 21 │ /* duplicate simple selectors with another rule between */ - i Please fix it. + i first occurence: + + 14 │ .e, + 15 │ .e, + > 16 │ .e {} + │ + > 17 │ + > 18 │ /* duplicate simple selectors with another rule between */ + > 19 │ f {} g {} f {} + │ ^^ + 20 │ + 21 │ /* duplicate simple selectors with another rule between */ ``` @@ -182,10 +230,8 @@ invalid.css:19:11 lint/nursery/noDuplicateSelectors ━━━━━━━━━ ``` invalid.css:23:5 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selector, first seen at - - /* duplicate simple selectors with another rule between */ - h + ! Duplicate selector " + h ", 21 │ /* duplicate simple selectors with another rule between */ 22 │ h {} @@ -196,7 +242,17 @@ invalid.css:23:5 lint/nursery/noDuplicateSelectors ━━━━━━━━━ 25 │ 26 │ /* duplicate selector lists with different order */ - i Please fix it. + i first occurence: + + 18 │ /* duplicate simple selectors with another rule between */ + > 19 │ f {} g {} f {} + │ + > 20 │ + > 21 │ /* duplicate simple selectors with another rule between */ + > 22 │ h {} + │ ^^ + 23 │ i {} + 24 │ h {} ``` @@ -204,10 +260,7 @@ invalid.css:23:5 lint/nursery/noDuplicateSelectors ━━━━━━━━━ ``` invalid.css:27:9 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selector, first seen at - - /* duplicate selector lists with different order */ - j, k + ! Duplicate selector "k, j ", 26 │ /* duplicate selector lists with different order */ > 27 │ j, k {} k, j {} @@ -215,7 +268,18 @@ invalid.css:27:9 lint/nursery/noDuplicateSelectors ━━━━━━━━━ 28 │ 29 │ /* duplicate selectors with multiple components */ - i Please fix it. + i first occurence: + + 22 │ h {} + 23 │ i {} + > 24 │ h {} + │ + > 25 │ + > 26 │ /* duplicate selector lists with different order */ + > 27 │ j, k {} k, j {} + │ ^^^^^ + 28 │ + 29 │ /* duplicate selectors with multiple components */ ``` @@ -223,10 +287,8 @@ invalid.css:27:9 lint/nursery/noDuplicateSelectors ━━━━━━━━━ ``` invalid.css:30:7 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selector, first seen at - - /* duplicate selectors with multiple components */ - m n + ! Duplicate selector " + m n ", 29 │ /* duplicate selectors with multiple components */ > 30 │ m n {} @@ -236,7 +298,17 @@ invalid.css:30:7 lint/nursery/noDuplicateSelectors ━━━━━━━━━ 32 │ 33 │ /* essentially duplicate selector lists with varied spacing */ - i Please fix it. + i first occurence: + + 26 │ /* duplicate selector lists with different order */ + > 27 │ j, k {} k, j {} + │ + > 28 │ + > 29 │ /* duplicate selectors with multiple components */ + > 30 │ m n {} + │ ^^^^ + 31 │ m n {} + 32 │ ``` @@ -244,11 +316,9 @@ invalid.css:30:7 lint/nursery/noDuplicateSelectors ━━━━━━━━━ ``` invalid.css:35:8 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selector, first seen at - - /* essentially duplicate selector lists with varied spacing */ - .foo p, q > .bar, - #baz + ! Duplicate selector " + #baz, + .foo p,q>.bar ", 33 │ /* essentially duplicate selector lists with varied spacing */ 34 │ .foo p, q > .bar, @@ -260,7 +330,19 @@ invalid.css:35:8 lint/nursery/noDuplicateSelectors ━━━━━━━━━ 38 │ 39 │ /* duplicate within a media query, in the same rule */ - i Please fix it. + i first occurence: + + 29 │ /* duplicate selectors with multiple components */ + 30 │ m n {} + > 31 │ m n {} + │ + > 32 │ + > 33 │ /* essentially duplicate selector lists with varied spacing */ + > 34 │ .foo p, q > .bar, + > 35 │ #baz {} + │ ^^^^^ + 36 │ #baz, + 37 │ .foo p,q>.bar {} ``` @@ -268,7 +350,7 @@ invalid.css:35:8 lint/nursery/noDuplicateSelectors ━━━━━━━━━ ``` invalid.css:41:19 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selector, first seen ats + ! Duplicate selector "s ", 39 │ /* duplicate within a media query, in the same rule */ 40 │ s {} @@ -277,7 +359,14 @@ invalid.css:41:19 lint/nursery/noDuplicateSelectors ━━━━━━━━━ 42 │ 43 │ /* duplicate within a media query, in different rules */ - i Please fix it. + i first occurence: + + 39 │ /* duplicate within a media query, in the same rule */ + 40 │ s {} + > 41 │ @media print { s, s {} } + │ ^ + 42 │ + 43 │ /* duplicate within a media query, in different rules */ ``` @@ -285,7 +374,7 @@ invalid.css:41:19 lint/nursery/noDuplicateSelectors ━━━━━━━━━ ``` invalid.css:45:45 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selector, first seen att + ! Duplicate selector "t ", 43 │ /* duplicate within a media query, in different rules */ 44 │ t {} @@ -294,7 +383,14 @@ invalid.css:45:45 lint/nursery/noDuplicateSelectors ━━━━━━━━━ 46 │ 47 │ /* duplicate caused by nesting */ - i Please fix it. + i first occurence: + + 43 │ /* duplicate within a media query, in different rules */ + 44 │ t {} + > 45 │ @media screen and (min-width: 900px) { t {} t {} } + │ ^^ + 46 │ + 47 │ /* duplicate caused by nesting */ ``` @@ -302,10 +398,7 @@ invalid.css:45:45 lint/nursery/noDuplicateSelectors ━━━━━━━━━ ``` invalid.css:48:12 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selector, first seen at - - /* duplicate caused by nesting */ - v w + ! Duplicate selector "w ", 47 │ /* duplicate caused by nesting */ > 48 │ v w {} v { w {} } @@ -313,7 +406,18 @@ invalid.css:48:12 lint/nursery/noDuplicateSelectors ━━━━━━━━━ 49 │ 50 │ /* duplicate caused by &-parent selector */ - i Please fix it. + i first occurence: + + 43 │ /* duplicate within a media query, in different rules */ + 44 │ t {} + > 45 │ @media screen and (min-width: 900px) { t {} t {} } + │ + > 46 │ + > 47 │ /* duplicate caused by nesting */ + > 48 │ v w {} v { w {} } + │ ^^^^ + 49 │ + 50 │ /* duplicate caused by &-parent selector */ ``` @@ -321,16 +425,21 @@ invalid.css:48:12 lint/nursery/noDuplicateSelectors ━━━━━━━━━ ``` invalid.css:51:5 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selector, first seen at - - /* duplicate caused by &-parent selector */ - x + ! Duplicate selector "& ", 50 │ /* duplicate caused by &-parent selector */ > 51 │ x { & {} } │ ^^ - i Please fix it. + i first occurence: + + 47 │ /* duplicate caused by nesting */ + > 48 │ v w {} v { w {} } + │ + > 49 │ + > 50 │ /* duplicate caused by &-parent selector */ + > 51 │ x { & {} } + │ ^^ ``` diff --git a/packages/@biomejs/biome/configuration_schema.json b/packages/@biomejs/biome/configuration_schema.json index f6331662d00..ec354a85543 100644 --- a/packages/@biomejs/biome/configuration_schema.json +++ b/packages/@biomejs/biome/configuration_schema.json @@ -1425,6 +1425,18 @@ "properties": { "allowComments": { "type": "boolean" } }, "additionalProperties": false }, + "NoDuplicateSelectorsConfiguration": { + "anyOf": [ + {"$ref": "#/definitions/RulePlainConfiguration"}, + {"$ref": "#/definitions/RuleWithNoDuplicateSelectorsOptions"} + ] + }, + "NoDuplicateSelectorsOptions": { + "type": "object", + "required": ["disallowInList"], + "properties": { "disallowInList": { "type": "boolean" } }, + "additionalProperties": false + }, "Nursery": { "description": "A list of rules that belong to this group", "type": "object", @@ -1489,6 +1501,13 @@ { "type": "null" } ] }, + "noDuplicateSelectors": { + "description": "Disallow duplicate selectors.", + "anyOf": [ + { "$ref": "#/definitions/NoDuplicateSelectorsConfiguration" }, + { "type": "null" } + ] + }, "noDuplicateSelectorsKeyframeBlock": { "description": "Disallow duplicate selectors within keyframe blocks.", "anyOf": [ @@ -1896,6 +1915,14 @@ }, "additionalProperties": false }, + "RuleWithNoDuplicateSelectorsOptions": { + "type": "object", + "required": ["level", "options"], + "properties": { + "level": { "$ref": "#/definitions/RulePlainConfiguration"}, + "options": { "$ref": "#/definitions/NoDuplicateSelectorsOptions"} + } + }, "RuleWithNoOptions": { "type": "object", "required": ["level"], From 65e6ba9c712233791bec6db711d3c6c9964984bf Mon Sep 17 00:00:00 2001 From: Abid Jappie Date: Mon, 6 May 2024 20:08:56 +0900 Subject: [PATCH 10/20] Ran gen-lint. --- .../biome_configuration/src/linter/rules.rs | 4 +- .../lint/nursery/no_duplicate_selectors.rs | 139 +++++++++--------- .../@biomejs/backend-jsonrpc/src/workspace.ts | 15 ++ .../@biomejs/biome/configuration_schema.json | 11 +- 4 files changed, 94 insertions(+), 75 deletions(-) diff --git a/crates/biome_configuration/src/linter/rules.rs b/crates/biome_configuration/src/linter/rules.rs index 0b633ccefc4..fa9a8d9b720 100644 --- a/crates/biome_configuration/src/linter/rules.rs +++ b/crates/biome_configuration/src/linter/rules.rs @@ -2671,7 +2671,7 @@ pub struct Nursery { #[doc = "Disallow two keys with the same name inside a JSON object."] #[serde(skip_serializing_if = "Option::is_none")] pub no_duplicate_json_keys: Option>, - #[doc = ""] + #[doc = "Disallow duplicate selectors."] #[serde(skip_serializing_if = "Option::is_none")] pub no_duplicate_selectors: Option>, #[doc = "Disallow duplicate selectors within keyframe blocks."] @@ -3113,7 +3113,7 @@ impl Nursery { .as_ref() .map(|conf| (conf.level(), conf.get_options())), "noDuplicateSelectors" => self - .no_duplicate_selectors_keyframe_block + .no_duplicate_selectors .as_ref() .map(|conf| (conf.level(), conf.get_options())), "noDuplicateSelectorsKeyframeBlock" => self diff --git a/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs b/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs index ab6dd5cf093..7317a3c6447 100644 --- a/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs +++ b/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs @@ -6,32 +6,36 @@ 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, CssRuleList, CssSelectorList, + CssSyntaxNode, +}; use biome_deserialize_macros::Deserializable; -use biome_css_syntax::{AnyCssAtRule, AnyCssRelativeSelector, AnyCssRule, AnyCssSelector, CssComplexSelector, CssRelativeSelector, CssRelativeSelectorList, CssRoot, CssRuleList, CssSelectorList, CssSyntaxNode}; use biome_rowan::{AstNode, SyntaxNodeCast}; use serde::{Deserialize, Serialize}; declare_rule! { /// Disallow duplicate selectors. - /// + /// /// ## Examples - /// + /// /// ### Invalid - /// + /// /// ```css,expect_diagnostic /// .abc, /// .def, /// .abc {} /// ``` - /// + /// /// ### Valid - /// + /// /// ``` /// .foo {} /// .bar {} /// ``` - /// + /// /// ## Options /// /// If true, disallow duplicate selectors within selector lists. @@ -64,7 +68,7 @@ pub struct NoDuplicateSelectorsOptions { impl Default for NoDuplicateSelectorsOptions { fn default() -> Self { Self { - disallow_in_list: false + disallow_in_list: false, } } } @@ -93,7 +97,7 @@ impl Borrow for ResolvedSelector { pub struct DuplicateSelector { first: CssSyntaxNode, - duplicate: CssSyntaxNode + duplicate: CssSyntaxNode, } impl Rule for NoDuplicateSelectors { @@ -107,23 +111,19 @@ impl Rule for NoDuplicateSelectors { let options = ctx.options(); let mut resolved_list: HashSet = HashSet::new(); - let mut output: Vec = vec!(); + let mut output: Vec = vec![]; if options.disallow_in_list { - let selectors = node - .rules() - .syntax() - .descendants() - .filter_map(|x|{ - if let Some(_selector) = x.clone().cast::(){ - return Some(x) + let selectors = node.rules().syntax().descendants().filter_map(|x| { + if let Some(_selector) = x.clone().cast::() { + return Some(x); } - if let Some(_relative_selector) = x.clone().cast::(){ - return Some(x) + if let Some(_relative_selector) = x.clone().cast::() { + return Some(x); } None }); - + for selector in selectors { let this_list = selector.clone().parent().unwrap(); @@ -141,10 +141,10 @@ impl Rule for NoDuplicateSelectors { let this_rule = this_list.parent().unwrap(); let mut selector_text = String::new(); - if let Some(selector) = CssRelativeSelector::cast_ref(&selector){ + if let Some(selector) = CssRelativeSelector::cast_ref(&selector) { selector_text = selector.clone().text(); } - if let Some(selector) = AnyCssSelector::cast_ref(&selector){ + if let Some(selector) = AnyCssSelector::cast_ref(&selector) { // TODO: test if this needs to be normalized selector_text = selector.clone().text(); } @@ -161,23 +161,19 @@ impl Rule for NoDuplicateSelectors { if let Some(first) = first { output.push(DuplicateSelector { first: first.selector_node.clone(), - duplicate: selector.clone() + duplicate: selector.clone(), }); } } } } } else { - let select_lists = node - .rules() - .syntax() - .descendants() - .filter_map(|x|{ - if let Some(_selector) = x.clone().cast::(){ - return Some(x) + let select_lists = node.rules().syntax().descendants().filter_map(|x| { + if let Some(_selector) = x.clone().cast::() { + return Some(x); } - if let Some(_relative_selector) = x.clone().cast::(){ - return Some(x) + if let Some(_relative_selector) = x.clone().cast::() { + return Some(x); } None }); @@ -189,7 +185,7 @@ impl Rule for NoDuplicateSelectors { let mut selector_list_mapped: Vec = selector_list .children() .into_iter() - .filter_map(|child|{ + .filter_map(|child| { if let Some(selector) = AnyCssSelector::cast_ref(&child) { let selector_text = normalize_complex_selector(selector.clone()); if !this_list_resolved_list.insert(ResolvedSelector { @@ -200,10 +196,10 @@ impl Rule for NoDuplicateSelectors { if let Some(first) = first { output.push(DuplicateSelector { first: first.selector_node.clone(), - duplicate: selector.into() + duplicate: selector.into(), }); // Return a none, since we already addressed this case - return None + return None; } } return Some(selector_text); @@ -216,7 +212,7 @@ impl Rule for NoDuplicateSelectors { if let Some(first) = first { output.push(DuplicateSelector { first: first.selector_node.clone(), - duplicate: selector.into() + duplicate: selector.into(), }); // Return a none, since we already addressed this case return None; @@ -243,14 +239,14 @@ impl Rule for NoDuplicateSelectors { if let Some(first) = first { output.push(DuplicateSelector { first: first.selector_node.clone(), - duplicate: selector_list.clone() + duplicate: selector_list.clone(), }); } } } } } - output + output } fn diagnostic(_: &RuleContext, node: &Self::State) -> Option { @@ -265,21 +261,20 @@ impl Rule for NoDuplicateSelectors { node.duplicate.text_range(), markup! { "Duplicate selector \""{duplicate}"\"," - } - ).detail(node.first.text_range(), "first occurence:") + }, + ) + .detail(node.first.text_range(), "first occurence:"), ) } } fn resolve_nested_selectors(selector: String, this_rule: CssSyntaxNode) -> Vec { - let mut parent_selectors: Vec = vec!(); + let mut parent_selectors: Vec = vec![]; let parent_rule = get_parent_rule(this_rule); match &parent_rule { - None => { - return vec!(selector) - }, + None => return vec![selector], Some(parent_rule) => { if let Some(parent_rule) = AnyCssAtRule::cast_ref(&parent_rule) { // resolve @@ -294,7 +289,7 @@ fn resolve_nested_selectors(selector: String, this_rule: CssSyntaxNode) -> Vec todo!(), AnyCssAtRule::CssScopeAtRule(_rule) => todo!(), AnyCssAtRule::CssStartingStyleAtRule(_rule) => todo!(), @@ -302,49 +297,57 @@ fn resolve_nested_selectors(selector: String, this_rule: CssSyntaxNode) -> Vec {} } } - if let Some(parent_rule) = AnyCssRule::cast_ref(&parent_rule){ + if let Some(parent_rule) = AnyCssRule::cast_ref(&parent_rule) { match parent_rule { AnyCssRule::CssBogusRule(_) => todo!(), AnyCssRule::CssAtRule(parent_rule) => { // Treat the AtRule as a selector let rule = parent_rule.rule().unwrap(); parent_selectors.push(rule.text()); - }, + } 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()); } } - }, + } } } - let resolved_selectors: Vec = parent_selectors.iter().fold(vec!(), |result: Vec, 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(); - } - }); + let resolved_selectors: Vec = + parent_selectors + .iter() + .fold(vec![], |result: Vec, 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 resolved_selectors; } - return vec!(selector) - }, + return vec![selector]; + } } } @@ -353,7 +356,7 @@ fn get_parent_rule(this_rule: CssSyntaxNode) -> Option { if let Some(parent_list) = this_rule.parent() { return parent_list.grand_parent(); } - return None + return None; } fn normalize_complex_selector(selector: AnyCssSelector) -> String { @@ -370,7 +373,7 @@ fn normalize_complex_selector(selector: AnyCssSelector) -> String { if let Ok(right) = complex_selector.right() { selector_text.push_str(&right.text()); } - return selector_text + return selector_text; } return selector.text(); -} \ No newline at end of file +} diff --git a/packages/@biomejs/backend-jsonrpc/src/workspace.ts b/packages/@biomejs/backend-jsonrpc/src/workspace.ts index 891e0728b65..aac1635d723 100644 --- a/packages/@biomejs/backend-jsonrpc/src/workspace.ts +++ b/packages/@biomejs/backend-jsonrpc/src/workspace.ts @@ -940,6 +940,10 @@ export interface Nursery { * Disallow two keys with the same name inside a JSON object. */ noDuplicateJsonKeys?: RuleConfiguration_for_Null; + /** + * Disallow duplicate selectors. + */ + noDuplicateSelectors?: RuleConfiguration_for_NoDuplicateSelectorsOptions; /** * Disallow duplicate selectors within keyframe blocks. */ @@ -1534,6 +1538,9 @@ export type RuleConfiguration_for_DeprecatedHooksOptions = export type RuleConfiguration_for_NoCssEmptyBlockOptions = | RulePlainConfiguration | RuleWithOptions_for_NoCssEmptyBlockOptions; +export type RuleConfiguration_for_NoDuplicateSelectorsOptions = + | RulePlainConfiguration + | RuleWithOptions_for_NoDuplicateSelectorsOptions; export type RuleConfiguration_for_RestrictedImportsOptions = | RulePlainConfiguration | RuleWithOptions_for_RestrictedImportsOptions; @@ -1577,6 +1584,10 @@ export interface RuleWithOptions_for_NoCssEmptyBlockOptions { level: RulePlainConfiguration; options: NoCssEmptyBlockOptions; } +export interface RuleWithOptions_for_NoDuplicateSelectorsOptions { + level: RulePlainConfiguration; + options: NoDuplicateSelectorsOptions; +} export interface RuleWithOptions_for_RestrictedImportsOptions { level: RulePlainConfiguration; options: RestrictedImportsOptions; @@ -1630,6 +1641,9 @@ export interface DeprecatedHooksOptions {} export interface NoCssEmptyBlockOptions { allowComments: boolean; } +export interface NoDuplicateSelectorsOptions { + disallowInList: boolean; +} /** * Options for the rule `noRestrictedImports`. */ @@ -1969,6 +1983,7 @@ export type Category = | "lint/nursery/noDuplicateElseIf" | "lint/nursery/noDuplicateFontNames" | "lint/nursery/noDuplicateJsonKeys" + | "lint/nursery/noDuplicateSelectors" | "lint/nursery/noDuplicateSelectorsKeyframeBlock" | "lint/nursery/noEvolvingAny" | "lint/nursery/noFlatMapIdentity" diff --git a/packages/@biomejs/biome/configuration_schema.json b/packages/@biomejs/biome/configuration_schema.json index ec354a85543..4c8abca8979 100644 --- a/packages/@biomejs/biome/configuration_schema.json +++ b/packages/@biomejs/biome/configuration_schema.json @@ -1427,8 +1427,8 @@ }, "NoDuplicateSelectorsConfiguration": { "anyOf": [ - {"$ref": "#/definitions/RulePlainConfiguration"}, - {"$ref": "#/definitions/RuleWithNoDuplicateSelectorsOptions"} + { "$ref": "#/definitions/RulePlainConfiguration" }, + { "$ref": "#/definitions/RuleWithNoDuplicateSelectorsOptions" } ] }, "NoDuplicateSelectorsOptions": { @@ -1919,9 +1919,10 @@ "type": "object", "required": ["level", "options"], "properties": { - "level": { "$ref": "#/definitions/RulePlainConfiguration"}, - "options": { "$ref": "#/definitions/NoDuplicateSelectorsOptions"} - } + "level": { "$ref": "#/definitions/RulePlainConfiguration" }, + "options": { "$ref": "#/definitions/NoDuplicateSelectorsOptions" } + }, + "additionalProperties": false }, "RuleWithNoOptions": { "type": "object", From 0b51c52a62f3f7fce844dc5bdc49b814d8d3db35 Mon Sep 17 00:00:00 2001 From: Abid Jappie Date: Mon, 6 May 2024 23:07:27 +0900 Subject: [PATCH 11/20] Added more test cases, adjusted the at-rule logic. --- .../lint/nursery/no_duplicate_selectors.rs | 32 ++--- .../noDuplicateSelectors/disallowInList.css | 3 + .../disallowInList.css.snap.new | 120 ++++++++++++++++++ .../nursery/noDuplicateSelectors/valid.css | 8 ++ .../noDuplicateSelectors/valid.css.snap.new | 8 ++ 5 files changed, 148 insertions(+), 23 deletions(-) diff --git a/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs b/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs index 7317a3c6447..9ef705a6049 100644 --- a/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs +++ b/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs @@ -1,6 +1,7 @@ +use core::hash; use std::borrow::Borrow; use std::collections::HashSet; -use std::hash::{Hash, Hasher}; +use std::hash::{DefaultHasher, Hash, Hasher}; use std::vec; use biome_analyze::Ast; @@ -145,8 +146,7 @@ impl Rule for NoDuplicateSelectors { selector_text = selector.clone().text(); } if let Some(selector) = AnyCssSelector::cast_ref(&selector) { - // TODO: test if this needs to be normalized - selector_text = selector.clone().text(); + selector_text = normalize_complex_selector(selector); } let resolved = resolve_nested_selectors(selector_text, this_rule); @@ -277,29 +277,15 @@ fn resolve_nested_selectors(selector: String, this_rule: CssSyntaxNode) -> Vec return vec![selector], Some(parent_rule) => { if let Some(parent_rule) = AnyCssAtRule::cast_ref(&parent_rule) { - // resolve - match parent_rule { - AnyCssAtRule::CssContainerAtRule(_rule) => todo!(), - AnyCssAtRule::CssKeyframesAtRule(_rule) => todo!(), - AnyCssAtRule::CssLayerAtRule(_rule) => todo!(), - AnyCssAtRule::CssMediaAtRule(rule) => { - let mut resolved = "@".to_string(); - resolved.push_str(&rule.media_token().unwrap().text()); - resolved.push_str(&rule.queries().text()); - // Replace the spaces with something that is not valid in CSS - let resolved = resolved.replace(char::is_whitespace, "-"); - parent_selectors.push(resolved); - } - AnyCssAtRule::CssPageAtRule(_rule) => todo!(), - AnyCssAtRule::CssScopeAtRule(_rule) => todo!(), - AnyCssAtRule::CssStartingStyleAtRule(_rule) => todo!(), - AnyCssAtRule::CssSupportsAtRule(_rule) => todo!(), - _ => {} - } + let mut hasher = DefaultHasher::new(); + let _hashed = &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::CssBogusRule(_) => todo!(), + AnyCssRule::CssBogusRule(_) => {} AnyCssRule::CssAtRule(parent_rule) => { // Treat the AtRule as a selector let rule = parent_rule.rule().unwrap(); diff --git a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/disallowInList.css b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/disallowInList.css index d7429942ed4..4135b143a7e 100644 --- a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/disallowInList.css +++ b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/disallowInList.css @@ -8,3 +8,6 @@ button {}; selector, button {} span, div {}; h1, section {}; h1 {} + +/* test regular case for regression */ +v w, x>test {} v { w {} } x > test {} \ No newline at end of file diff --git a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/disallowInList.css.snap.new b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/disallowInList.css.snap.new index ab7262a5ec7..21c73d0cb69 100644 --- a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/disallowInList.css.snap.new +++ b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/disallowInList.css.snap.new @@ -16,4 +16,124 @@ span, div {}; h1, section {}; h1 {} +/* test regular case for regression */ +v w, x>test {} v { w {} } x > test {} +``` + +# Diagnostics +``` +disallowInList.css:2:21 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Duplicate selector "textarea ", + + 1 │ /* duplicate within a grouping selector */ + > 2 │ input, textarea {}; textarea {} + │ ^^^^^^^^^ + 3 │ + 4 │ /* duplicate within a grouping selector, reversed */ + + i first occurence: + + 1 │ /* duplicate within a grouping selector */ + > 2 │ input, textarea {}; textarea {} + │ ^^^^^^^^^ + 3 │ + 4 │ /* duplicate within a grouping selector, reversed */ + + +``` + +``` +disallowInList.css:5:22 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Duplicate selector "button ", + + 4 │ /* duplicate within a grouping selector, reversed */ + > 5 │ button {}; selector, button {} + │ ^^^^^^^ + 6 │ + 7 │ /* duplicate within a grouping selector. multiline */ + + i first occurence: + + 1 │ /* duplicate within a grouping selector */ + > 2 │ input, textarea {}; textarea {} + │ + > 3 │ + > 4 │ /* duplicate within a grouping selector, reversed */ + > 5 │ button {}; selector, button {} + │ ^^^^^^^ + 6 │ + 7 │ /* duplicate within a grouping selector. multiline */ + + +``` + +``` +disallowInList.css:9:17 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Duplicate selector " + h1 ", + + 7 │ /* duplicate within a grouping selector. multiline */ + 8 │ span, div {}; + > 9 │ h1, section {}; + │ + > 10 │ h1 {} + │ ^^^ + 11 │ + 12 │ /* test regular case for regression */ + + i first occurence: + + 7 │ /* duplicate within a grouping selector. multiline */ + > 8 │ span, div {}; + │ + > 9 │ h1, section {}; + │ ^^ + 10 │ h1 {} + 11 │ + + +``` + +``` +disallowInList.css:13:20 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Duplicate selector "w ", + + 12 │ /* test regular case for regression */ + > 13 │ v w, x>test {} v { w {} } x > test {} + │ ^^ + + i first occurence: + + 8 │ span, div {}; + 9 │ h1, section {}; + > 10 │ h1 {} + │ + > 11 │ + > 12 │ /* test regular case for regression */ + > 13 │ v w, x>test {} v { w {} } x > test {} + │ ^^^ + + +``` + +``` +disallowInList.css:13:27 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Duplicate selector "x > test ", + + 12 │ /* test regular case for regression */ + > 13 │ v w, x>test {} v { w {} } x > test {} + │ ^^^^^^^^^ + + i first occurence: + + 12 │ /* test regular case for regression */ + > 13 │ v w, x>test {} v { w {} } x > test {} + │ ^^^^^^^ + + ``` diff --git a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/valid.css b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/valid.css index 94dbb129e7f..d6d1830518e 100644 --- a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/valid.css +++ b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/valid.css @@ -5,6 +5,14 @@ a {} b {} c {} d, e, f {} g {} @media print { g {} } +/* duplicates inside separate media query */ +@media print { + gg.dev {} +} +@media print { + gg.dev {} +} + /* duplicate inside keyframes */ @keyframes h { 0% {} } @keyframes h { 0% {} } diff --git a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/valid.css.snap.new b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/valid.css.snap.new index 5ea73dc7dcc..9adbdb2511b 100644 --- a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/valid.css.snap.new +++ b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/valid.css.snap.new @@ -12,6 +12,14 @@ a {} b {} c {} d, e, f {} g {} @media print { g {} } +/* duplicates inside separate media query */ +@media print { + gg.dev {} +} +@media print { + gg.dev {} +} + /* duplicate inside keyframes */ @keyframes h { 0% {} } @keyframes h { 0% {} } From 40de23d2a39df95b3ff365c057272f3ae88548a4 Mon Sep 17 00:00:00 2001 From: Abid Jappie Date: Tue, 7 May 2024 18:59:13 +0900 Subject: [PATCH 12/20] Code structure improvements and simplifications. --- .../lint/nursery/no_duplicate_selectors.rs | 187 +++++++----------- 1 file changed, 67 insertions(+), 120 deletions(-) diff --git a/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs b/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs index 9ef705a6049..f835e187360 100644 --- a/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs +++ b/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs @@ -115,133 +115,91 @@ impl Rule for NoDuplicateSelectors { let mut output: Vec = vec![]; if options.disallow_in_list { - let selectors = node.rules().syntax().descendants().filter_map(|x| { - if let Some(_selector) = x.clone().cast::() { - return Some(x); - } - if let Some(_relative_selector) = x.clone().cast::() { - return Some(x); - } - None - }); - - for selector in selectors { - let this_list = selector.clone().parent().unwrap(); + let selectors = node.rules().syntax().descendants().filter( + |x| x.clone().cast::().is_some() || x.clone().cast::().is_some() + ); + // 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())).filter(|(_, parent)|{ // i.e not actually a list - if let Some(_parent_sel) = CssComplexSelector::cast_ref(&this_list) { - // Don't handle the children of complex selectors - // this_selctor_list = parent_selector.into_syntax().parent().unwrap() - continue; - } else if let Some(_parent_sel) = CssRelativeSelector::cast_ref(&this_list) { - // Don't handle the children of complex relative - // this_selctor_list = parent_selector.into_syntax().parent().unwrap(); - continue; - } - - let this_rule = this_list.parent().unwrap(); + return !(parent.clone().cast::().is_some() || parent.clone().cast::().is_some()) + }){ + // this_rule unwrap should never fail due to the structure of the AST + let this_rule = selector_list.parent().unwrap(); - let mut selector_text = String::new(); - if let Some(selector) = CssRelativeSelector::cast_ref(&selector) { - selector_text = selector.clone().text(); - } - if let Some(selector) = AnyCssSelector::cast_ref(&selector) { - selector_text = normalize_complex_selector(selector); - } - let resolved = resolve_nested_selectors(selector_text, this_rule); + 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::().unwrap()) + }; - for r in resolved { + 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 !resolved_list.insert(ResolvedSelector { - selector_text: normalized.clone(), - selector_node: selector.clone(), - }) { - let first = resolved_list.get(&normalized); - if let Some(first) = first { - output.push(DuplicateSelector { - first: first.selector_node.clone(), - duplicate: selector.clone(), - }); - } + + 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 select_lists = node.rules().syntax().descendants().filter_map(|x| { - if let Some(_selector) = x.clone().cast::() { - return Some(x); - } - if let Some(_relative_selector) = x.clone().cast::() { - return Some(x); - } - None - }); + let selector_lists = node.rules().syntax().descendants().filter( + |x| x.clone().cast::().is_some() || x.clone().cast::().is_some() + ); - for selector_list in select_lists { + // 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 = HashSet::new(); - let this_rule = selector_list.parent().unwrap(); let mut selector_list_mapped: Vec = selector_list .children() .into_iter() .filter_map(|child| { - if let Some(selector) = AnyCssSelector::cast_ref(&child) { - let selector_text = normalize_complex_selector(selector.clone()); - if !this_list_resolved_list.insert(ResolvedSelector { - selector_text: selector_text.clone(), - selector_node: selector.clone().into(), - }) { - let first = this_list_resolved_list.get(&selector_text); - if let Some(first) = first { - output.push(DuplicateSelector { - first: first.selector_node.clone(), - duplicate: selector.into(), - }); - // Return a none, since we already addressed this case - return None; - } - } - return Some(selector_text); - } else if let Some(selector) = AnyCssRelativeSelector::cast_ref(&child) { - if !this_list_resolved_list.insert(ResolvedSelector { - selector_text: selector.clone().text(), - selector_node: selector.clone().into(), - }) { - let first = this_list_resolved_list.get(&selector.text()); - if let Some(first) = first { - output.push(DuplicateSelector { - first: first.selector_node.clone(), - duplicate: selector.into(), - }); - // Return a none, since we already addressed this case - return None; - } - } - return Some(selector.text()); + let selector_text = if let Some(selector) = AnyCssSelector::cast_ref(&child) { + normalize_complex_selector(selector.clone()) + } else { + child.clone().cast::().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; } - None + + this_list_resolved_list.insert(ResolvedSelector { + selector_text: selector_text.clone(), + selector_node: child, + }); + Some(selector_text) }) .collect(); selector_list_mapped.sort(); - let selector_list_text = selector_list_mapped.join(","); - - let resolved = resolve_nested_selectors(selector_list_text, this_rule); - for r in resolved { + 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 !resolved_list.insert(ResolvedSelector { - selector_text: normalized.clone(), - selector_node: selector_list.clone().into(), - }) { - let first = resolved_list.get(&normalized); - if let Some(first) = first { - output.push(DuplicateSelector { - first: first.selector_node.clone(), - duplicate: selector_list.clone(), - }); - } + if let Some(first) = resolved_list.get(&normalized) { + output.push(DuplicateSelector { + first: first.selector_node.clone(), + duplicate: selector_list.clone(), + }); + } else { + resolved_list.insert(ResolvedSelector { + selector_text: normalized.clone(), + selector_node: selector_list.clone().into(), + }); } } } @@ -270,27 +228,20 @@ impl Rule for NoDuplicateSelectors { fn resolve_nested_selectors(selector: String, this_rule: CssSyntaxNode) -> Vec { let mut parent_selectors: Vec = vec![]; - - let parent_rule = get_parent_rule(this_rule); + 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(); - let _hashed = &parent_rule.range().hash(&mut hasher); + 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::CssBogusRule(_) => {} - AnyCssRule::CssAtRule(parent_rule) => { - // Treat the AtRule as a selector - let rule = parent_rule.rule().unwrap(); - parent_selectors.push(rule.text()); - } AnyCssRule::CssNestedQualifiedRule(parent_rule) => { for selector in parent_rule.prelude() { if let Ok(selector) = selector { @@ -305,6 +256,10 @@ fn resolve_nested_selectors(selector: String, this_rule: CssSyntaxNode) -> Vec { + // Bogus rules are not handled + // AtRule is handled by AnyCssAtRule above + } } } @@ -337,14 +292,6 @@ fn resolve_nested_selectors(selector: String, this_rule: CssSyntaxNode) -> Vec Option { - if let Some(parent_list) = this_rule.parent() { - return parent_list.grand_parent(); - } - return None; -} - fn normalize_complex_selector(selector: AnyCssSelector) -> String { let mut selector_text = String::new(); From 2ed14053664304e2881f753cf89a16988e5d4414 Mon Sep 17 00:00:00 2001 From: Abid Jappie Date: Tue, 7 May 2024 19:00:31 +0900 Subject: [PATCH 13/20] Linting. --- .../lint/nursery/no_duplicate_selectors.rs | 47 ++++++++++++------- 1 file changed, 30 insertions(+), 17 deletions(-) diff --git a/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs b/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs index f835e187360..e2e44af5057 100644 --- a/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs +++ b/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs @@ -1,4 +1,3 @@ -use core::hash; use std::borrow::Borrow; use std::collections::HashSet; use std::hash::{DefaultHasher, Hash, Hasher}; @@ -9,7 +8,7 @@ use biome_analyze::{context::RuleContext, declare_rule, Rule, RuleDiagnostic, Ru use biome_console::markup; use biome_css_syntax::{ AnyCssAtRule, AnyCssRelativeSelector, AnyCssRule, AnyCssSelector, CssComplexSelector, - CssRelativeSelector, CssRelativeSelectorList, CssRoot, CssRuleList, CssSelectorList, + CssRelativeSelector, CssRelativeSelectorList, CssRoot, CssSelectorList, CssSyntaxNode, }; use biome_deserialize_macros::Deserializable; @@ -115,19 +114,25 @@ impl Rule for NoDuplicateSelectors { let mut output: Vec = vec![]; if options.disallow_in_list { - let selectors = node.rules().syntax().descendants().filter( - |x| x.clone().cast::().is_some() || x.clone().cast::().is_some() - ); + let selectors = node.rules().syntax().descendants().filter(|x| { + x.clone().cast::().is_some() + || x.clone().cast::().is_some() + }); // 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())).filter(|(_, parent)|{ - // i.e not actually a list - return !(parent.clone().cast::().is_some() || parent.clone().cast::().is_some()) - }){ + for (selector, selector_list) in selectors + .map(|selector| (selector.clone(), selector.parent().unwrap())) + .filter(|(_, parent)| { + // i.e not actually a list + return !(parent.clone().cast::().is_some() + || parent.clone().cast::().is_some()); + }) + { // this_rule unwrap should never fail due to the structure of the AST let this_rule = selector_list.parent().unwrap(); - let selector_text = if let Some(selector) = CssRelativeSelector::cast_ref(&selector) { + let selector_text = if let Some(selector) = CssRelativeSelector::cast_ref(&selector) + { selector.clone().text() } else { // selector is either AnyCssSelector or AnyCssRelativeSelector @@ -152,28 +157,36 @@ impl Rule for NoDuplicateSelectors { } } } else { - let selector_lists = node.rules().syntax().descendants().filter( - |x| x.clone().cast::().is_some() || x.clone().cast::().is_some() - ); + let selector_lists = node.rules().syntax().descendants().filter(|x| { + x.clone().cast::().is_some() + || x.clone().cast::().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())) { + for (selector_list, rule) in selector_lists + .map(|selector_list| (selector_list.clone(), selector_list.parent().unwrap())) + { let mut this_list_resolved_list: HashSet = HashSet::new(); let mut selector_list_mapped: Vec = selector_list .children() .into_iter() .filter_map(|child| { - let selector_text = if let Some(selector) = AnyCssSelector::cast_ref(&child) { + let selector_text = if let Some(selector) = AnyCssSelector::cast_ref(&child) + { normalize_complex_selector(selector.clone()) } else { - child.clone().cast::().unwrap().text() + child + .clone() + .cast::() + .unwrap() + .text() }; if let Some(first) = this_list_resolved_list.get(&selector_text) { output.push(DuplicateSelector { first: first.selector_node.clone(), - duplicate: child.clone() + duplicate: child.clone(), }); return None; } From 8feeedad7eb3922d9cff3b6bfa3261e4ef7e0e8e Mon Sep 17 00:00:00 2001 From: Abid Jappie Date: Tue, 7 May 2024 19:45:53 +0900 Subject: [PATCH 14/20] Added additional test cases, updated the diagnostic messages. --- .../lint/nursery/no_duplicate_selectors.rs | 12 +- .../noDuplicateSelectors/disallowInList.css | 6 + .../disallowInList.css.snap.new | 142 ++++++++++-------- .../noDuplicateSelectors/invalid.css.snap.new | 98 +++++++----- .../nursery/noDuplicateSelectors/valid.css | 2 +- 5 files changed, 156 insertions(+), 104 deletions(-) diff --git a/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs b/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs index e2e44af5057..40e608ff2ec 100644 --- a/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs +++ b/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs @@ -8,8 +8,7 @@ use biome_analyze::{context::RuleContext, declare_rule, Rule, RuleDiagnostic, Ru use biome_console::markup; use biome_css_syntax::{ AnyCssAtRule, AnyCssRelativeSelector, AnyCssRule, AnyCssSelector, CssComplexSelector, - CssRelativeSelector, CssRelativeSelectorList, CssRoot, CssSelectorList, - CssSyntaxNode, + CssRelativeSelector, CssRelativeSelectorList, CssRoot, CssSelectorList, CssSyntaxNode, }; use biome_deserialize_macros::Deserializable; use biome_rowan::{AstNode, SyntaxNodeCast}; @@ -225,16 +224,19 @@ impl Rule for NoDuplicateSelectors { // 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 = node.duplicate.to_string(); + let duplicate_text = node.duplicate.to_string(); Some( RuleDiagnostic::new( rule_category!(), node.duplicate.text_range(), markup! { - "Duplicate selector \""{duplicate}"\"," + "Duplicate selectors may result in unintentionally overriding rules:"{ duplicate_text } }, ) - .detail(node.first.text_range(), "first occurence:"), + .detail(node.first.text_range(), "Please consider moving the rule's contents to the first occurence:") + .note(markup! { + "Remove duplicate selectors within the rule" + }), ) } } diff --git a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/disallowInList.css b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/disallowInList.css index 4135b143a7e..83019dd5248 100644 --- a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/disallowInList.css +++ b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/disallowInList.css @@ -1,3 +1,9 @@ +/* valid cases: should not error */ +th, td {}; tr {} +*::a {}; a {} +*::c, b {}; c {} +d e, f {}; d {} + /* duplicate within a grouping selector */ input, textarea {}; textarea {} diff --git a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/disallowInList.css.snap.new b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/disallowInList.css.snap.new index 21c73d0cb69..1a71092efd0 100644 --- a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/disallowInList.css.snap.new +++ b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/disallowInList.css.snap.new @@ -5,6 +5,12 @@ expression: disallowInList.css --- # Input ```css +/* valid cases: should not error */ +th, td {}; tr {} +*::a {}; a {} +*::c, b {}; c {} +d e, f {}; d {} + /* duplicate within a grouping selector */ input, textarea {}; textarea {} @@ -22,118 +28,128 @@ v w, x>test {} v { w {} } x > test {} # Diagnostics ``` -disallowInList.css:2:21 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +disallowInList.css:8:21 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selector "textarea ", + ! Duplicate selectors may result in unintentionally overriding rules:textarea + + 7 │ /* duplicate within a grouping selector */ + > 8 │ input, textarea {}; textarea {} + │ ^^^^^^^^^ + 9 │ + 10 │ /* duplicate within a grouping selector, reversed */ - 1 │ /* duplicate within a grouping selector */ - > 2 │ input, textarea {}; textarea {} - │ ^^^^^^^^^ - 3 │ - 4 │ /* duplicate within a grouping selector, reversed */ + i Please consider moving the rule's contents to the first occurence: - i first occurence: + 7 │ /* duplicate within a grouping selector */ + > 8 │ input, textarea {}; textarea {} + │ ^^^^^^^^^ + 9 │ + 10 │ /* duplicate within a grouping selector, reversed */ - 1 │ /* duplicate within a grouping selector */ - > 2 │ input, textarea {}; textarea {} - │ ^^^^^^^^^ - 3 │ - 4 │ /* duplicate within a grouping selector, reversed */ + i Remove duplicate selectors within the rule ``` ``` -disallowInList.css:5:22 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +disallowInList.css:11:22 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selector "button ", + ! Duplicate selectors may result in unintentionally overriding rules:button + + 10 │ /* duplicate within a grouping selector, reversed */ + > 11 │ button {}; selector, button {} + │ ^^^^^^^ + 12 │ + 13 │ /* duplicate within a grouping selector. multiline */ - 4 │ /* duplicate within a grouping selector, reversed */ - > 5 │ button {}; selector, button {} - │ ^^^^^^^ - 6 │ - 7 │ /* duplicate within a grouping selector. multiline */ + i Please consider moving the rule's contents to the first occurence: - i first occurence: + 7 │ /* duplicate within a grouping selector */ + > 8 │ input, textarea {}; textarea {} + │ + > 9 │ + > 10 │ /* duplicate within a grouping selector, reversed */ + > 11 │ button {}; selector, button {} + │ ^^^^^^^ + 12 │ + 13 │ /* duplicate within a grouping selector. multiline */ - 1 │ /* duplicate within a grouping selector */ - > 2 │ input, textarea {}; textarea {} - │ - > 3 │ - > 4 │ /* duplicate within a grouping selector, reversed */ - > 5 │ button {}; selector, button {} - │ ^^^^^^^ - 6 │ - 7 │ /* duplicate within a grouping selector. multiline */ + i Remove duplicate selectors within the rule ``` ``` -disallowInList.css:9:17 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +disallowInList.css:15:17 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selector " - h1 ", + ! Duplicate selectors may result in unintentionally overriding rules: + h1 - 7 │ /* duplicate within a grouping selector. multiline */ - 8 │ span, div {}; - > 9 │ h1, section {}; + 13 │ /* duplicate within a grouping selector. multiline */ + 14 │ span, div {}; + > 15 │ h1, section {}; │ - > 10 │ h1 {} + > 16 │ h1 {} │ ^^^ - 11 │ - 12 │ /* test regular case for regression */ + 17 │ + 18 │ /* test regular case for regression */ - i first occurence: + i Please consider moving the rule's contents to the first occurence: - 7 │ /* duplicate within a grouping selector. multiline */ - > 8 │ span, div {}; + 13 │ /* duplicate within a grouping selector. multiline */ + > 14 │ span, div {}; │ - > 9 │ h1, section {}; + > 15 │ h1, section {}; │ ^^ - 10 │ h1 {} - 11 │ + 16 │ h1 {} + 17 │ + + i Remove duplicate selectors within the rule ``` ``` -disallowInList.css:13:20 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +disallowInList.css:19:20 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selector "w ", + ! Duplicate selectors may result in unintentionally overriding rules:w - 12 │ /* test regular case for regression */ - > 13 │ v w, x>test {} v { w {} } x > test {} + 18 │ /* test regular case for regression */ + > 19 │ v w, x>test {} v { w {} } x > test {} │ ^^ - i first occurence: + i Please consider moving the rule's contents to the first occurence: - 8 │ span, div {}; - 9 │ h1, section {}; - > 10 │ h1 {} + 14 │ span, div {}; + 15 │ h1, section {}; + > 16 │ h1 {} │ - > 11 │ - > 12 │ /* test regular case for regression */ - > 13 │ v w, x>test {} v { w {} } x > test {} + > 17 │ + > 18 │ /* test regular case for regression */ + > 19 │ v w, x>test {} v { w {} } x > test {} │ ^^^ + i Remove duplicate selectors within the rule + ``` ``` -disallowInList.css:13:27 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +disallowInList.css:19:27 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selector "x > test ", + ! Duplicate selectors may result in unintentionally overriding rules:x > test - 12 │ /* test regular case for regression */ - > 13 │ v w, x>test {} v { w {} } x > test {} + 18 │ /* test regular case for regression */ + > 19 │ v w, x>test {} v { w {} } x > test {} │ ^^^^^^^^^ - i first occurence: + i Please consider moving the rule's contents to the first occurence: - 12 │ /* test regular case for regression */ - > 13 │ v w, x>test {} v { w {} } x > test {} + 18 │ /* test regular case for regression */ + > 19 │ v w, x>test {} v { w {} } x > test {} │ ^^^^^^^ + i Remove duplicate selectors within the rule + ``` diff --git a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css.snap.new b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css.snap.new index 3859541fdb6..b73611316ca 100644 --- a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css.snap.new +++ b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css.snap.new @@ -62,7 +62,7 @@ x { & {} } ``` invalid.css:2:4 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selector "a ", + ! Duplicate selectors may result in unintentionally overriding rules:a 1 │ /* duplicate within one rule's selector list */ > 2 │ a, a {} @@ -70,7 +70,7 @@ invalid.css:2:4 lint/nursery/noDuplicateSelectors ━━━━━━━━━━ 3 │ 4 │ /* duplicate within one rule's selector list. multiline */ - i first occurence: + i Please consider moving the rule's contents to the first occurence: > 1 │ /* duplicate within one rule's selector list */ │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -79,14 +79,16 @@ invalid.css:2:4 lint/nursery/noDuplicateSelectors ━━━━━━━━━━ 3 │ 4 │ /* duplicate within one rule's selector list. multiline */ + i Remove duplicate selectors within the rule + ``` ``` invalid.css:5:3 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selector " - b ", + ! Duplicate selectors may result in unintentionally overriding rules: + b 4 │ /* duplicate within one rule's selector list. multiline */ > 5 │ b, @@ -96,7 +98,7 @@ invalid.css:5:3 lint/nursery/noDuplicateSelectors ━━━━━━━━━━ 7 │ 8 │ /* duplicate within one rule's selector list. multiline */ - i first occurence: + i Please consider moving the rule's contents to the first occurence: 1 │ /* duplicate within one rule's selector list */ > 2 │ a, a {} @@ -108,14 +110,16 @@ invalid.css:5:3 lint/nursery/noDuplicateSelectors ━━━━━━━━━━ 6 │ b {} 7 │ + i Remove duplicate selectors within the rule + ``` ``` invalid.css:10:3 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selector " - d ", + ! Duplicate selectors may result in unintentionally overriding rules: + d 8 │ /* duplicate within one rule's selector list. multiline */ 9 │ c, @@ -126,7 +130,7 @@ invalid.css:10:3 lint/nursery/noDuplicateSelectors ━━━━━━━━━ 12 │ 13 │ /* duplicated selectors within one rule's selector list. 3 duplicates */ - i first occurence: + i Please consider moving the rule's contents to the first occurence: 8 │ /* duplicate within one rule's selector list. multiline */ > 9 │ c, @@ -136,14 +140,16 @@ invalid.css:10:3 lint/nursery/noDuplicateSelectors ━━━━━━━━━ 11 │ d {} 12 │ + i Remove duplicate selectors within the rule + ``` ``` invalid.css:14:4 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selector " - .e", + ! Duplicate selectors may result in unintentionally overriding rules: + .e 13 │ /* duplicated selectors within one rule's selector list. 3 duplicates */ > 14 │ .e, @@ -153,7 +159,7 @@ invalid.css:14:4 lint/nursery/noDuplicateSelectors ━━━━━━━━━ 16 │ .e {} 17 │ - i first occurence: + i Please consider moving the rule's contents to the first occurence: 9 │ c, 10 │ d, @@ -166,14 +172,16 @@ invalid.css:14:4 lint/nursery/noDuplicateSelectors ━━━━━━━━━ 15 │ .e, 16 │ .e {} + i Remove duplicate selectors within the rule + ``` ``` invalid.css:15:4 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selector " - .e ", + ! Duplicate selectors may result in unintentionally overriding rules: + .e 13 │ /* duplicated selectors within one rule's selector list. 3 duplicates */ 14 │ .e, @@ -184,7 +192,7 @@ invalid.css:15:4 lint/nursery/noDuplicateSelectors ━━━━━━━━━ 17 │ 18 │ /* duplicate simple selectors with another rule between */ - i first occurence: + i Please consider moving the rule's contents to the first occurence: 9 │ c, 10 │ d, @@ -197,13 +205,15 @@ invalid.css:15:4 lint/nursery/noDuplicateSelectors ━━━━━━━━━ 15 │ .e, 16 │ .e {} + i Remove duplicate selectors within the rule + ``` ``` invalid.css:19:11 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selector "f ", + ! Duplicate selectors may result in unintentionally overriding rules:f 18 │ /* duplicate simple selectors with another rule between */ > 19 │ f {} g {} f {} @@ -211,7 +221,7 @@ invalid.css:19:11 lint/nursery/noDuplicateSelectors ━━━━━━━━━ 20 │ 21 │ /* duplicate simple selectors with another rule between */ - i first occurence: + i Please consider moving the rule's contents to the first occurence: 14 │ .e, 15 │ .e, @@ -224,14 +234,16 @@ invalid.css:19:11 lint/nursery/noDuplicateSelectors ━━━━━━━━━ 20 │ 21 │ /* duplicate simple selectors with another rule between */ + i Remove duplicate selectors within the rule + ``` ``` invalid.css:23:5 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selector " - h ", + ! Duplicate selectors may result in unintentionally overriding rules: + h 21 │ /* duplicate simple selectors with another rule between */ 22 │ h {} @@ -242,7 +254,7 @@ invalid.css:23:5 lint/nursery/noDuplicateSelectors ━━━━━━━━━ 25 │ 26 │ /* duplicate selector lists with different order */ - i first occurence: + i Please consider moving the rule's contents to the first occurence: 18 │ /* duplicate simple selectors with another rule between */ > 19 │ f {} g {} f {} @@ -254,13 +266,15 @@ invalid.css:23:5 lint/nursery/noDuplicateSelectors ━━━━━━━━━ 23 │ i {} 24 │ h {} + i Remove duplicate selectors within the rule + ``` ``` invalid.css:27:9 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selector "k, j ", + ! Duplicate selectors may result in unintentionally overriding rules:k, j 26 │ /* duplicate selector lists with different order */ > 27 │ j, k {} k, j {} @@ -268,7 +282,7 @@ invalid.css:27:9 lint/nursery/noDuplicateSelectors ━━━━━━━━━ 28 │ 29 │ /* duplicate selectors with multiple components */ - i first occurence: + i Please consider moving the rule's contents to the first occurence: 22 │ h {} 23 │ i {} @@ -281,14 +295,16 @@ invalid.css:27:9 lint/nursery/noDuplicateSelectors ━━━━━━━━━ 28 │ 29 │ /* duplicate selectors with multiple components */ + i Remove duplicate selectors within the rule + ``` ``` invalid.css:30:7 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selector " - m n ", + ! Duplicate selectors may result in unintentionally overriding rules: + m n 29 │ /* duplicate selectors with multiple components */ > 30 │ m n {} @@ -298,7 +314,7 @@ invalid.css:30:7 lint/nursery/noDuplicateSelectors ━━━━━━━━━ 32 │ 33 │ /* essentially duplicate selector lists with varied spacing */ - i first occurence: + i Please consider moving the rule's contents to the first occurence: 26 │ /* duplicate selector lists with different order */ > 27 │ j, k {} k, j {} @@ -310,15 +326,17 @@ invalid.css:30:7 lint/nursery/noDuplicateSelectors ━━━━━━━━━ 31 │ m n {} 32 │ + i Remove duplicate selectors within the rule + ``` ``` invalid.css:35:8 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selector " + ! Duplicate selectors may result in unintentionally overriding rules: #baz, - .foo p,q>.bar ", + .foo p,q>.bar 33 │ /* essentially duplicate selector lists with varied spacing */ 34 │ .foo p, q > .bar, @@ -330,7 +348,7 @@ invalid.css:35:8 lint/nursery/noDuplicateSelectors ━━━━━━━━━ 38 │ 39 │ /* duplicate within a media query, in the same rule */ - i first occurence: + i Please consider moving the rule's contents to the first occurence: 29 │ /* duplicate selectors with multiple components */ 30 │ m n {} @@ -344,13 +362,15 @@ invalid.css:35:8 lint/nursery/noDuplicateSelectors ━━━━━━━━━ 36 │ #baz, 37 │ .foo p,q>.bar {} + i Remove duplicate selectors within the rule + ``` ``` invalid.css:41:19 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selector "s ", + ! Duplicate selectors may result in unintentionally overriding rules:s 39 │ /* duplicate within a media query, in the same rule */ 40 │ s {} @@ -359,7 +379,7 @@ invalid.css:41:19 lint/nursery/noDuplicateSelectors ━━━━━━━━━ 42 │ 43 │ /* duplicate within a media query, in different rules */ - i first occurence: + i Please consider moving the rule's contents to the first occurence: 39 │ /* duplicate within a media query, in the same rule */ 40 │ s {} @@ -368,13 +388,15 @@ invalid.css:41:19 lint/nursery/noDuplicateSelectors ━━━━━━━━━ 42 │ 43 │ /* duplicate within a media query, in different rules */ + i Remove duplicate selectors within the rule + ``` ``` invalid.css:45:45 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selector "t ", + ! Duplicate selectors may result in unintentionally overriding rules:t 43 │ /* duplicate within a media query, in different rules */ 44 │ t {} @@ -383,7 +405,7 @@ invalid.css:45:45 lint/nursery/noDuplicateSelectors ━━━━━━━━━ 46 │ 47 │ /* duplicate caused by nesting */ - i first occurence: + i Please consider moving the rule's contents to the first occurence: 43 │ /* duplicate within a media query, in different rules */ 44 │ t {} @@ -392,13 +414,15 @@ invalid.css:45:45 lint/nursery/noDuplicateSelectors ━━━━━━━━━ 46 │ 47 │ /* duplicate caused by nesting */ + i Remove duplicate selectors within the rule + ``` ``` invalid.css:48:12 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selector "w ", + ! Duplicate selectors may result in unintentionally overriding rules:w 47 │ /* duplicate caused by nesting */ > 48 │ v w {} v { w {} } @@ -406,7 +430,7 @@ invalid.css:48:12 lint/nursery/noDuplicateSelectors ━━━━━━━━━ 49 │ 50 │ /* duplicate caused by &-parent selector */ - i first occurence: + i Please consider moving the rule's contents to the first occurence: 43 │ /* duplicate within a media query, in different rules */ 44 │ t {} @@ -419,19 +443,21 @@ invalid.css:48:12 lint/nursery/noDuplicateSelectors ━━━━━━━━━ 49 │ 50 │ /* duplicate caused by &-parent selector */ + i Remove duplicate selectors within the rule + ``` ``` invalid.css:51:5 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selector "& ", + ! Duplicate selectors may result in unintentionally overriding rules:& 50 │ /* duplicate caused by &-parent selector */ > 51 │ x { & {} } │ ^^ - i first occurence: + i Please consider moving the rule's contents to the first occurence: 47 │ /* duplicate caused by nesting */ > 48 │ v w {} v { w {} } @@ -441,5 +467,7 @@ invalid.css:51:5 lint/nursery/noDuplicateSelectors ━━━━━━━━━ > 51 │ x { & {} } │ ^^ + i Remove duplicate selectors within the rule + ``` diff --git a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/valid.css b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/valid.css index d6d1830518e..51e77df7653 100644 --- a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/valid.css +++ b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/valid.css @@ -39,4 +39,4 @@ m n { top: 0; } m { n, p { color: pink; } } ul, ol {} ul {} /* attribute selector */ -[disabled].goo, [disabled] .goo {} +[disabled].goo, [disabled] .goo {} \ No newline at end of file From 648ffdd6921a56e7b683f8a00073895079af85c3 Mon Sep 17 00:00:00 2001 From: Abid Jappie Date: Tue, 7 May 2024 19:48:17 +0900 Subject: [PATCH 15/20] Fixed typo. --- .../tests/specs/nursery/noDuplicateSelectors/invalid.css | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css index f2d01f0ab8d..a7615dcec71 100644 --- a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css +++ b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css @@ -10,7 +10,7 @@ c, d, d {} -/* duplicated selectors within one rule's selector list. 3 duplicates */ +/* duplicated selectors within one rule's selector list. 2 duplicates */ .e, .e, .e {} From e667f3c30314a3c7a26d459a7b989802b1b63b89 Mon Sep 17 00:00:00 2001 From: Abid Jappie Date: Tue, 7 May 2024 20:19:13 +0900 Subject: [PATCH 16/20] Better diagnostic reporting. --- .../lint/nursery/no_duplicate_selectors.rs | 6 +- .../disallowInList.css.snap.new | 48 ++--- .../noDuplicateSelectors/invalid.css.snap.new | 189 ++++++------------ .../noDuplicateSelectors/valid.css.snap.new | 1 - 4 files changed, 87 insertions(+), 157 deletions(-) diff --git a/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs b/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs index 40e608ff2ec..d2dfbf05dd3 100644 --- a/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs +++ b/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs @@ -228,12 +228,12 @@ impl Rule for NoDuplicateSelectors { Some( RuleDiagnostic::new( rule_category!(), - node.duplicate.text_range(), + node.duplicate.text_trimmed_range(), markup! { - "Duplicate selectors may result in unintentionally overriding rules:"{ duplicate_text } + "Duplicate selectors may result in unintentionally overriding rules: "{ duplicate_text } }, ) - .detail(node.first.text_range(), "Please consider moving the rule's contents to the first occurence:") + .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" }), diff --git a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/disallowInList.css.snap.new b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/disallowInList.css.snap.new index 1a71092efd0..c0b1286dae6 100644 --- a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/disallowInList.css.snap.new +++ b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/disallowInList.css.snap.new @@ -30,11 +30,11 @@ v w, x>test {} v { w {} } x > test {} ``` disallowInList.css:8:21 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selectors may result in unintentionally overriding rules:textarea + ! Duplicate selectors may result in unintentionally overriding rules: textarea 7 │ /* duplicate within a grouping selector */ > 8 │ input, textarea {}; textarea {} - │ ^^^^^^^^^ + │ ^^^^^^^^ 9 │ 10 │ /* duplicate within a grouping selector, reversed */ @@ -42,7 +42,7 @@ disallowInList.css:8:21 lint/nursery/noDuplicateSelectors ━━━━━━━ 7 │ /* duplicate within a grouping selector */ > 8 │ input, textarea {}; textarea {} - │ ^^^^^^^^^ + │ ^^^^^^^^ 9 │ 10 │ /* duplicate within a grouping selector, reversed */ @@ -54,23 +54,19 @@ disallowInList.css:8:21 lint/nursery/noDuplicateSelectors ━━━━━━━ ``` disallowInList.css:11:22 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selectors may result in unintentionally overriding rules:button + ! Duplicate selectors may result in unintentionally overriding rules: button 10 │ /* duplicate within a grouping selector, reversed */ > 11 │ button {}; selector, button {} - │ ^^^^^^^ + │ ^^^^^^ 12 │ 13 │ /* duplicate within a grouping selector. multiline */ i Please consider moving the rule's contents to the first occurence: - 7 │ /* duplicate within a grouping selector */ - > 8 │ input, textarea {}; textarea {} - │ - > 9 │ - > 10 │ /* duplicate within a grouping selector, reversed */ + 10 │ /* duplicate within a grouping selector, reversed */ > 11 │ button {}; selector, button {} - │ ^^^^^^^ + │ ^^^^^^ 12 │ 13 │ /* duplicate within a grouping selector. multiline */ @@ -80,25 +76,22 @@ disallowInList.css:11:22 lint/nursery/noDuplicateSelectors ━━━━━━━ ``` ``` -disallowInList.css:15:17 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +disallowInList.css:16:1 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selectors may result in unintentionally overriding rules: + ! Duplicate selectors may result in unintentionally overriding rules: h1 - 13 │ /* duplicate within a grouping selector. multiline */ 14 │ span, div {}; - > 15 │ h1, section {}; - │ + 15 │ h1, section {}; > 16 │ h1 {} - │ ^^^ + │ ^^ 17 │ 18 │ /* test regular case for regression */ i Please consider moving the rule's contents to the first occurence: 13 │ /* duplicate within a grouping selector. multiline */ - > 14 │ span, div {}; - │ + 14 │ span, div {}; > 15 │ h1, section {}; │ ^^ 16 │ h1 {} @@ -112,20 +105,15 @@ disallowInList.css:15:17 lint/nursery/noDuplicateSelectors ━━━━━━━ ``` disallowInList.css:19:20 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selectors may result in unintentionally overriding rules:w + ! Duplicate selectors may result in unintentionally overriding rules: w 18 │ /* test regular case for regression */ > 19 │ v w, x>test {} v { w {} } x > test {} - │ ^^ + │ ^ i Please consider moving the rule's contents to the first occurence: - 14 │ span, div {}; - 15 │ h1, section {}; - > 16 │ h1 {} - │ - > 17 │ - > 18 │ /* test regular case for regression */ + 18 │ /* test regular case for regression */ > 19 │ v w, x>test {} v { w {} } x > test {} │ ^^^ @@ -137,17 +125,17 @@ disallowInList.css:19:20 lint/nursery/noDuplicateSelectors ━━━━━━━ ``` disallowInList.css:19:27 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selectors may result in unintentionally overriding rules:x > test + ! Duplicate selectors may result in unintentionally overriding rules: x > test 18 │ /* test regular case for regression */ > 19 │ v w, x>test {} v { w {} } x > test {} - │ ^^^^^^^^^ + │ ^^^^^^^^ i Please consider moving the rule's contents to the first occurence: 18 │ /* test regular case for regression */ > 19 │ v w, x>test {} v { w {} } x > test {} - │ ^^^^^^^ + │ ^^^^^^ i Remove duplicate selectors within the rule diff --git a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css.snap.new b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css.snap.new index b73611316ca..e1c88265906 100644 --- a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css.snap.new +++ b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css.snap.new @@ -17,7 +17,7 @@ c, d, d {} -/* duplicated selectors within one rule's selector list. 3 duplicates */ +/* duplicated selectors within one rule's selector list. 2 duplicates */ .e, .e, .e {} @@ -62,18 +62,17 @@ x { & {} } ``` invalid.css:2:4 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selectors may result in unintentionally overriding rules:a + ! Duplicate selectors may result in unintentionally overriding rules: a 1 │ /* duplicate within one rule's selector list */ > 2 │ a, a {} - │ ^^ + │ ^ 3 │ 4 │ /* duplicate within one rule's selector list. multiline */ i Please consider moving the rule's contents to the first occurence: - > 1 │ /* duplicate within one rule's selector list */ - │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + 1 │ /* duplicate within one rule's selector list */ > 2 │ a, a {} │ ^ 3 │ @@ -85,26 +84,21 @@ invalid.css:2:4 lint/nursery/noDuplicateSelectors ━━━━━━━━━━ ``` ``` -invalid.css:5:3 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +invalid.css:6:1 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selectors may result in unintentionally overriding rules: + ! Duplicate selectors may result in unintentionally overriding rules: b 4 │ /* duplicate within one rule's selector list. multiline */ - > 5 │ b, - │ + 5 │ b, > 6 │ b {} - │ ^^ + │ ^ 7 │ 8 │ /* duplicate within one rule's selector list. multiline */ i Please consider moving the rule's contents to the first occurence: - 1 │ /* duplicate within one rule's selector list */ - > 2 │ a, a {} - │ - > 3 │ - > 4 │ /* duplicate within one rule's selector list. multiline */ + 4 │ /* duplicate within one rule's selector list. multiline */ > 5 │ b, │ ^ 6 │ b {} @@ -116,25 +110,22 @@ invalid.css:5:3 lint/nursery/noDuplicateSelectors ━━━━━━━━━━ ``` ``` -invalid.css:10:3 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +invalid.css:11:1 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selectors may result in unintentionally overriding rules: + ! Duplicate selectors may result in unintentionally overriding rules: d - 8 │ /* duplicate within one rule's selector list. multiline */ 9 │ c, - > 10 │ d, - │ + 10 │ d, > 11 │ d {} - │ ^^ + │ ^ 12 │ - 13 │ /* duplicated selectors within one rule's selector list. 3 duplicates */ + 13 │ /* duplicated selectors within one rule's selector list. 2 duplicates */ i Please consider moving the rule's contents to the first occurence: 8 │ /* duplicate within one rule's selector list. multiline */ - > 9 │ c, - │ + 9 │ c, > 10 │ d, │ ^ 11 │ d {} @@ -146,14 +137,13 @@ invalid.css:10:3 lint/nursery/noDuplicateSelectors ━━━━━━━━━ ``` ``` -invalid.css:14:4 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +invalid.css:15:1 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selectors may result in unintentionally overriding rules: + ! Duplicate selectors may result in unintentionally overriding rules: .e - 13 │ /* duplicated selectors within one rule's selector list. 3 duplicates */ - > 14 │ .e, - │ + 13 │ /* duplicated selectors within one rule's selector list. 2 duplicates */ + 14 │ .e, > 15 │ .e, │ ^^ 16 │ .e {} @@ -161,12 +151,7 @@ invalid.css:14:4 lint/nursery/noDuplicateSelectors ━━━━━━━━━ i Please consider moving the rule's contents to the first occurence: - 9 │ c, - 10 │ d, - > 11 │ d {} - │ - > 12 │ - > 13 │ /* duplicated selectors within one rule's selector list. 3 duplicates */ + 13 │ /* duplicated selectors within one rule's selector list. 2 duplicates */ > 14 │ .e, │ ^^ 15 │ .e, @@ -178,28 +163,21 @@ invalid.css:14:4 lint/nursery/noDuplicateSelectors ━━━━━━━━━ ``` ``` -invalid.css:15:4 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +invalid.css:16:1 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selectors may result in unintentionally overriding rules: + ! Duplicate selectors may result in unintentionally overriding rules: .e - 13 │ /* duplicated selectors within one rule's selector list. 3 duplicates */ 14 │ .e, - > 15 │ .e, - │ + 15 │ .e, > 16 │ .e {} - │ ^^^ + │ ^^ 17 │ 18 │ /* duplicate simple selectors with another rule between */ i Please consider moving the rule's contents to the first occurence: - 9 │ c, - 10 │ d, - > 11 │ d {} - │ - > 12 │ - > 13 │ /* duplicated selectors within one rule's selector list. 3 duplicates */ + 13 │ /* duplicated selectors within one rule's selector list. 2 duplicates */ > 14 │ .e, │ ^^ 15 │ .e, @@ -213,24 +191,19 @@ invalid.css:15:4 lint/nursery/noDuplicateSelectors ━━━━━━━━━ ``` invalid.css:19:11 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selectors may result in unintentionally overriding rules:f + ! Duplicate selectors may result in unintentionally overriding rules: f 18 │ /* duplicate simple selectors with another rule between */ > 19 │ f {} g {} f {} - │ ^^ + │ ^ 20 │ 21 │ /* duplicate simple selectors with another rule between */ i Please consider moving the rule's contents to the first occurence: - 14 │ .e, - 15 │ .e, - > 16 │ .e {} - │ - > 17 │ - > 18 │ /* duplicate simple selectors with another rule between */ + 18 │ /* duplicate simple selectors with another rule between */ > 19 │ f {} g {} f {} - │ ^^ + │ ^ 20 │ 21 │ /* duplicate simple selectors with another rule between */ @@ -240,29 +213,23 @@ invalid.css:19:11 lint/nursery/noDuplicateSelectors ━━━━━━━━━ ``` ``` -invalid.css:23:5 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +invalid.css:24:1 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selectors may result in unintentionally overriding rules: + ! Duplicate selectors may result in unintentionally overriding rules: h - 21 │ /* duplicate simple selectors with another rule between */ 22 │ h {} - > 23 │ i {} - │ + 23 │ i {} > 24 │ h {} - │ ^^ + │ ^ 25 │ 26 │ /* duplicate selector lists with different order */ i Please consider moving the rule's contents to the first occurence: - 18 │ /* duplicate simple selectors with another rule between */ - > 19 │ f {} g {} f {} - │ - > 20 │ - > 21 │ /* duplicate simple selectors with another rule between */ + 21 │ /* duplicate simple selectors with another rule between */ > 22 │ h {} - │ ^^ + │ ^ 23 │ i {} 24 │ h {} @@ -274,24 +241,19 @@ invalid.css:23:5 lint/nursery/noDuplicateSelectors ━━━━━━━━━ ``` invalid.css:27:9 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selectors may result in unintentionally overriding rules:k, j + ! Duplicate selectors may result in unintentionally overriding rules: k, j 26 │ /* duplicate selector lists with different order */ > 27 │ j, k {} k, j {} - │ ^^^^^ + │ ^^^^ 28 │ 29 │ /* duplicate selectors with multiple components */ i Please consider moving the rule's contents to the first occurence: - 22 │ h {} - 23 │ i {} - > 24 │ h {} - │ - > 25 │ - > 26 │ /* duplicate selector lists with different order */ + 26 │ /* duplicate selector lists with different order */ > 27 │ j, k {} k, j {} - │ ^^^^^ + │ ^^^^ 28 │ 29 │ /* duplicate selectors with multiple components */ @@ -301,28 +263,23 @@ invalid.css:27:9 lint/nursery/noDuplicateSelectors ━━━━━━━━━ ``` ``` -invalid.css:30:7 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +invalid.css:31:1 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selectors may result in unintentionally overriding rules: + ! Duplicate selectors may result in unintentionally overriding rules: m n 29 │ /* duplicate selectors with multiple components */ - > 30 │ m n {} - │ + 30 │ m n {} > 31 │ m n {} - │ ^^^^ + │ ^^^ 32 │ 33 │ /* essentially duplicate selector lists with varied spacing */ i Please consider moving the rule's contents to the first occurence: - 26 │ /* duplicate selector lists with different order */ - > 27 │ j, k {} k, j {} - │ - > 28 │ - > 29 │ /* duplicate selectors with multiple components */ + 29 │ /* duplicate selectors with multiple components */ > 30 │ m n {} - │ ^^^^ + │ ^^^ 31 │ m n {} 32 │ @@ -332,33 +289,28 @@ invalid.css:30:7 lint/nursery/noDuplicateSelectors ━━━━━━━━━ ``` ``` -invalid.css:35:8 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +invalid.css:36:3 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selectors may result in unintentionally overriding rules: + ! Duplicate selectors may result in unintentionally overriding rules: #baz, .foo p,q>.bar - 33 │ /* essentially duplicate selector lists with varied spacing */ 34 │ .foo p, q > .bar, - > 35 │ #baz {} - │ + 35 │ #baz {} > 36 │ #baz, + │ ^^^^^ > 37 │ .foo p,q>.bar {} - │ ^^^^^^^^^^^^^^^^^^ + │ ^^^^^^^^^^^^^^^^^ 38 │ 39 │ /* duplicate within a media query, in the same rule */ i Please consider moving the rule's contents to the first occurence: - 29 │ /* duplicate selectors with multiple components */ - 30 │ m n {} - > 31 │ m n {} - │ - > 32 │ - > 33 │ /* essentially duplicate selector lists with varied spacing */ + 33 │ /* essentially duplicate selector lists with varied spacing */ > 34 │ .foo p, q > .bar, + │ ^^^^^^^^^^^^^^^^^^^ > 35 │ #baz {} - │ ^^^^^ + │ ^^^^ 36 │ #baz, 37 │ .foo p,q>.bar {} @@ -370,12 +322,12 @@ invalid.css:35:8 lint/nursery/noDuplicateSelectors ━━━━━━━━━ ``` invalid.css:41:19 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selectors may result in unintentionally overriding rules:s + ! Duplicate selectors may result in unintentionally overriding rules: s 39 │ /* duplicate within a media query, in the same rule */ 40 │ s {} > 41 │ @media print { s, s {} } - │ ^^ + │ ^ 42 │ 43 │ /* duplicate within a media query, in different rules */ @@ -396,12 +348,12 @@ invalid.css:41:19 lint/nursery/noDuplicateSelectors ━━━━━━━━━ ``` invalid.css:45:45 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selectors may result in unintentionally overriding rules:t + ! Duplicate selectors may result in unintentionally overriding rules: t 43 │ /* duplicate within a media query, in different rules */ 44 │ t {} > 45 │ @media screen and (min-width: 900px) { t {} t {} } - │ ^^ + │ ^ 46 │ 47 │ /* duplicate caused by nesting */ @@ -410,7 +362,7 @@ invalid.css:45:45 lint/nursery/noDuplicateSelectors ━━━━━━━━━ 43 │ /* duplicate within a media query, in different rules */ 44 │ t {} > 45 │ @media screen and (min-width: 900px) { t {} t {} } - │ ^^ + │ ^ 46 │ 47 │ /* duplicate caused by nesting */ @@ -422,24 +374,19 @@ invalid.css:45:45 lint/nursery/noDuplicateSelectors ━━━━━━━━━ ``` invalid.css:48:12 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selectors may result in unintentionally overriding rules:w + ! Duplicate selectors may result in unintentionally overriding rules: w 47 │ /* duplicate caused by nesting */ > 48 │ v w {} v { w {} } - │ ^^ + │ ^ 49 │ 50 │ /* duplicate caused by &-parent selector */ i Please consider moving the rule's contents to the first occurence: - 43 │ /* duplicate within a media query, in different rules */ - 44 │ t {} - > 45 │ @media screen and (min-width: 900px) { t {} t {} } - │ - > 46 │ - > 47 │ /* duplicate caused by nesting */ + 47 │ /* duplicate caused by nesting */ > 48 │ v w {} v { w {} } - │ ^^^^ + │ ^^^ 49 │ 50 │ /* duplicate caused by &-parent selector */ @@ -451,21 +398,17 @@ invalid.css:48:12 lint/nursery/noDuplicateSelectors ━━━━━━━━━ ``` invalid.css:51:5 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selectors may result in unintentionally overriding rules:& + ! Duplicate selectors may result in unintentionally overriding rules: & 50 │ /* duplicate caused by &-parent selector */ > 51 │ x { & {} } - │ ^^ + │ ^ i Please consider moving the rule's contents to the first occurence: - 47 │ /* duplicate caused by nesting */ - > 48 │ v w {} v { w {} } - │ - > 49 │ - > 50 │ /* duplicate caused by &-parent selector */ + 50 │ /* duplicate caused by &-parent selector */ > 51 │ x { & {} } - │ ^^ + │ ^ i Remove duplicate selectors within the rule diff --git a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/valid.css.snap.new b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/valid.css.snap.new index 9adbdb2511b..6b02fb5775b 100644 --- a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/valid.css.snap.new +++ b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/valid.css.snap.new @@ -47,5 +47,4 @@ ul, ol {} ul {} /* attribute selector */ [disabled].goo, [disabled] .goo {} - ``` From 7e2b209d965eac42eea5e2303f205584b19dc9c8 Mon Sep 17 00:00:00 2001 From: Abid Jappie Date: Tue, 7 May 2024 23:36:47 +0900 Subject: [PATCH 17/20] Addressed linter feedback. WIP: addressing review feedback. --- .../lint/nursery/no_duplicate_selectors.rs | 111 ++++++++---------- 1 file changed, 48 insertions(+), 63 deletions(-) diff --git a/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs b/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs index d2dfbf05dd3..57bb0b70f5e 100644 --- a/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs +++ b/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs @@ -11,7 +11,7 @@ use biome_css_syntax::{ CssRelativeSelector, CssRelativeSelectorList, CssRoot, CssSelectorList, CssSyntaxNode, }; use biome_deserialize_macros::Deserializable; -use biome_rowan::{AstNode, SyntaxNodeCast}; +use biome_rowan::{declare_node_union, AstNode, SyntaxNodeCast}; use serde::{Deserialize, Serialize}; @@ -39,7 +39,7 @@ declare_rule! { /// /// If true, disallow duplicate selectors within selector lists. /// - /// ```json + /// ```json5, ignore /// { /// "noDuplicateSelectors": { /// "options": { @@ -57,19 +57,15 @@ declare_rule! { } } -#[derive(Debug, Clone, Deserialize, Deserializable, Eq, PartialEq, Serialize)] +#[derive(Debug, Default, 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, } -impl Default for NoDuplicateSelectorsOptions { - fn default() -> Self { - Self { - disallow_in_list: false, - } - } +declare_node_union! { + pub AnySelectorLike = AnyCssSelector | AnyCssRelativeSelector } #[derive(Debug, Eq)] @@ -113,29 +109,26 @@ impl Rule for NoDuplicateSelectors { let mut output: Vec = vec![]; if options.disallow_in_list { - let selectors = node.rules().syntax().descendants().filter(|x| { - x.clone().cast::().is_some() - || x.clone().cast::().is_some() + let selectors = node.rules().syntax().descendants().filter_map(|x| { + AnySelectorLike::cast_ref(&x) }); - // 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())) - .filter(|(_, parent)| { - // i.e not actually a list - return !(parent.clone().cast::().is_some() - || parent.clone().cast::().is_some()); - }) - { - // this_rule unwrap should never fail due to the structure of the AST - let this_rule = selector_list.parent().unwrap(); + .filter_map(|selector| { + let parent = selector.clone().into_syntax().parent()?; + if parent.clone().cast::().is_some() || parent.clone().cast::().is_some() { + return None; + } + Some((selector, parent)) + }) { + + 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::().unwrap()) + let selector_text = match selector.clone() { + AnySelectorLike::AnyCssSelector(selector) => normalize_complex_selector(selector), + AnySelectorLike::AnyCssRelativeSelector(selector) => selector.text() }; for r in resolve_nested_selectors(selector_text, this_rule) { @@ -145,31 +138,33 @@ impl Rule for NoDuplicateSelectors { if let Some(first) = resolved_list.get(&normalized) { output.push(DuplicateSelector { first: first.selector_node.clone(), - duplicate: selector.clone(), + duplicate: selector.clone().into_syntax(), }); } else { resolved_list.insert(ResolvedSelector { selector_text: normalized.clone(), - selector_node: selector.clone(), + selector_node: selector.clone().into_syntax(), }); } } } } else { - let selector_lists = node.rules().syntax().descendants().filter(|x| { - x.clone().cast::().is_some() - || x.clone().cast::().is_some() - }); + // TODO: Can't use a node union here + let selector_lists = node.rules().syntax().descendants().filter(|x| + x.clone().cast::().is_some() || x.clone().cast::().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())) + .filter_map(|selector_list|{ + let parent = selector_list.clone().parent()?; + Some((selector_list, parent)) + }) { let mut this_list_resolved_list: HashSet = HashSet::new(); let mut selector_list_mapped: Vec = selector_list + .clone() .children() - .into_iter() .filter_map(|child| { let selector_text = if let Some(selector) = AnyCssSelector::cast_ref(&child) { @@ -210,7 +205,7 @@ impl Rule for NoDuplicateSelectors { } else { resolved_list.insert(ResolvedSelector { selector_text: normalized.clone(), - selector_node: selector_list.clone().into(), + selector_node: selector_list.clone(), }); } } @@ -220,11 +215,9 @@ impl Rule for NoDuplicateSelectors { } fn diagnostic(_: &RuleContext, node: &Self::State) -> Option { - // - // Read our guidelines to write great diagnostics: - // https://docs.rs/biome_analyze/latest/biome_analyze/#what-a-rule-should-say-to-the-user - // + // TODO: type this with a union node let duplicate_text = node.duplicate.to_string(); + Some( RuleDiagnostic::new( rule_category!(), @@ -246,30 +239,22 @@ fn resolve_nested_selectors(selector: String, this_rule: CssSyntaxNode) -> Vec return vec![selector], + None => vec![selector], Some(parent_rule) => { - if let Some(parent_rule) = AnyCssAtRule::cast_ref(&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) { + 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()); - } - } + parent_selectors.extend(parent_rule.prelude().into_iter().filter_map(|selector| selector.ok()).map(|selector|selector.text())); } AnyCssRule::CssQualifiedRule(parent_rule) => { - for selector in parent_rule.prelude() { - if let Ok(selector) = selector { - parent_selectors.push(selector.text()); - } - } + parent_selectors.extend(parent_rule.prelude().into_iter().filter_map(|selector| selector.ok()).map(|selector|selector.text())); } _ => { // Bogus rules are not handled @@ -282,27 +267,27 @@ fn resolve_nested_selectors(selector: String, this_rule: CssSyntaxNode) -> Vec, parent_selector| { - if selector.contains("&") { + 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)) + .map(|newly_resolved| selector.replace('&', &newly_resolved)) .collect(); - return [result, resolved].concat(); + [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(); + [result, resolved].concat() } }); - if resolved_selectors.len() > 0 { + if !resolved_selectors.is_empty() { return resolved_selectors; } - return vec![selector]; + vec![selector] } } } @@ -310,7 +295,7 @@ fn resolve_nested_selectors(selector: String, this_rule: CssSyntaxNode) -> Vec String { let mut selector_text = String::new(); - if let Some(complex_selector) = CssComplexSelector::cast_ref(&selector.clone().into_syntax()) { + if let AnyCssSelector::CssComplexSelector(complex_selector) = selector { if let Ok(left) = complex_selector.left() { selector_text.push_str(&left.text()); } @@ -323,5 +308,5 @@ fn normalize_complex_selector(selector: AnyCssSelector) -> String { } return selector_text; } - return selector.text(); + selector.text() } From 4471c4fd5c6d36265998899fd6e312d7113286af Mon Sep 17 00:00:00 2001 From: Abid Jappie Date: Wed, 8 May 2024 14:20:29 +0900 Subject: [PATCH 18/20] WIP: added comments. --- .../lint/nursery/no_duplicate_selectors.rs | 31 +++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-) diff --git a/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs b/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs index 57bb0b70f5e..9499fd3c3cb 100644 --- a/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs +++ b/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs @@ -234,6 +234,21 @@ impl Rule for NoDuplicateSelectors { } } +/// Resolves nested selectors into an equivalent flat selector. +/// If there is no parent rule, return the selector string originally passed. +/// E.g. +/// ```css, ignore +/// a { b { c { /* declaration */ } } } +/// ``` +/// +/// is resolved to: +/// ```css, ignore +/// a b c { +/// /* declaration */ +/// } +/// ``` +/// +/// Returns the resolved selector as a string. fn resolve_nested_selectors(selector: String, this_rule: CssSyntaxNode) -> Vec { let mut parent_selectors: Vec = vec![]; let parent_rule = this_rule.parent().and_then(|parent| parent.grand_parent()); @@ -247,8 +262,7 @@ fn resolve_nested_selectors(selector: String, this_rule: CssSyntaxNode) -> Vec { parent_selectors.extend(parent_rule.prelude().into_iter().filter_map(|selector| selector.ok()).map(|selector|selector.text())); @@ -292,6 +306,19 @@ fn resolve_nested_selectors(selector: String, this_rule: CssSyntaxNode) -> Vec b, c > d { /* declaration */ } +/// ``` +/// +/// is normalized to: +/// ``` css, ignore +/// a>b, c>d { /* declaration */ } +/// ``` +/// +/// Returns the selector as a string. fn normalize_complex_selector(selector: AnyCssSelector) -> String { let mut selector_text = String::new(); From f453e8cb7993efe0da78150bb614a45afe307425 Mon Sep 17 00:00:00 2001 From: Abid Jappie Date: Wed, 8 May 2024 18:59:05 +0900 Subject: [PATCH 19/20] WIP: added comments. --- .../lint/nursery/no_duplicate_selectors.rs | 48 ++++++++++++++++--- 1 file changed, 41 insertions(+), 7 deletions(-) diff --git a/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs b/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs index 9499fd3c3cb..497ec4ba1db 100644 --- a/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs +++ b/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs @@ -16,8 +16,16 @@ use biome_rowan::{declare_node_union, AstNode, SyntaxNodeCast}; use serde::{Deserialize, Serialize}; declare_rule! { - /// Disallow duplicate selectors. - /// + /// Disallow duplicate selectors. This rule checks for two types of duplication: + /// - Duplication of a selector list within a stylesheet, e.g. `a, b {} a, b {}`. Duplicates are found even if the selectors come in different orders or have different spacing, e.g. `a d, b > c {} b>c, a d {}`. + /// - Duplication of a single selector with a rule's selector list, e.g. `a, b, a {}`. (See options below, this is disabled by default) + /// + /// The same selector is allowed to repeat in the following circumstances: + /// - It is used in different selector lists, e.g. `a {} a, b {}`. + /// - The duplicates are in rules with different parent nodes, e.g. inside and outside of a media query. + /// + /// This rule resolves nested selectors. So `a b {} a { & b {} }` counts as a problem, because the resolved selectors end up with a duplicate. + /// /// ## Examples /// /// ### Invalid @@ -25,19 +33,19 @@ declare_rule! { /// ```css,expect_diagnostic /// .abc, /// .def, - /// .abc {} + /// .abc { /* declaration */ } /// ``` /// /// ### Valid /// /// ``` - /// .foo {} - /// .bar {} + /// .foo { /* declaration */ } + /// .bar { /* declaration */ } /// ``` /// /// ## Options /// - /// If true, disallow duplicate selectors within selector lists. + /// If true, disallow duplicate selectors within selector lists. The following settings: /// /// ```json5, ignore /// { @@ -48,6 +56,12 @@ declare_rule! { /// } /// } /// ``` + /// + /// Will result in the following failing: + /// + /// ```css, ignore + /// input, textarea {}; textarea {} + /// ``` /// pub NoDuplicateSelectors { version: "next", @@ -61,6 +75,7 @@ declare_rule! { #[cfg_attr(feature = "schemars", derive(schemars::JsonSchema))] #[serde(rename_all = "camelCase", deny_unknown_fields)] pub struct NoDuplicateSelectorsOptions { + /// If set to `true` this rule will check for duplicate selectors within selector lists. pub disallow_in_list: bool, } @@ -68,6 +83,9 @@ declare_node_union! { pub AnySelectorLike = AnyCssSelector | AnyCssRelativeSelector } +/// Object containing the resolved selector and the relative node for that resolved selector. +/// +/// This struct has a hash function which returns the hash of `selector_node`. #[derive(Debug, Eq)] struct ResolvedSelector { selector_text: String, @@ -248,6 +266,21 @@ impl Rule for NoDuplicateSelectors { /// } /// ``` /// +/// When trying to resolve this_rule of type [AnyCssAtRule], this function will generate a hash to replace the selector name: +/// ```css, ignore +/// @media print { +/// selector { /* declaration */} +/// } +/// ``` +/// +/// is resolved to: +/// ```css, ignore +/// selector { /* declaration */} +/// ``` +/// +/// [AnyCssAtRule] is resolved based on the text range. +/// The same rule will not be resolved to the same hash because these are considered to belong to a separate context. +/// /// Returns the resolved selector as a string. fn resolve_nested_selectors(selector: String, this_rule: CssSyntaxNode) -> Vec { let mut parent_selectors: Vec = vec![]; @@ -306,7 +339,7 @@ fn resolve_nested_selectors(selector: String, this_rule: CssSyntaxNode) -> Vec Vecb, c>d { /* declaration */ } /// ``` /// +/// It will return `selector.text()` if it is unable to cast. /// Returns the selector as a string. fn normalize_complex_selector(selector: AnyCssSelector) -> String { let mut selector_text = String::new(); From 911eacf7496f4b398e497478ef853218dc6af3c2 Mon Sep 17 00:00:00 2001 From: Abid Jappie Date: Wed, 8 May 2024 21:34:06 +0900 Subject: [PATCH 20/20] Handle the text in the diagnostics. --- .../lint/nursery/no_duplicate_selectors.rs | 14 +++++-- .../disallowInList.css.snap.new | 11 +++--- .../noDuplicateSelectors/invalid.css.snap.new | 37 ++++++++----------- 3 files changed, 31 insertions(+), 31 deletions(-) diff --git a/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs b/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs index 497ec4ba1db..cce749ce172 100644 --- a/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs +++ b/crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs @@ -167,7 +167,7 @@ impl Rule for NoDuplicateSelectors { } } } else { - // TODO: Can't use a node union here + // Union node with CssSelectorList and CssRelativeSelectorList does not have overlapping From/Into let selector_lists = node.rules().syntax().descendants().filter(|x| x.clone().cast::().is_some() || x.clone().cast::().is_some() ); @@ -233,8 +233,16 @@ impl Rule for NoDuplicateSelectors { } fn diagnostic(_: &RuleContext, node: &Self::State) -> Option { - // TODO: type this with a union node - let duplicate_text = node.duplicate.to_string(); + let duplicate_text = if let Some(duplicate) = AnySelectorLike::cast_ref(&node.duplicate) { + duplicate.text() + } else if let Some(duplicate) = CssSelectorList::cast_ref(&node.duplicate) { + duplicate.text() + } else if let Some(duplicate) = CssRelativeSelectorList::cast_ref(&node.duplicate) { + duplicate.text() + } + else { + node.duplicate.to_string() + }; Some( RuleDiagnostic::new( diff --git a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/disallowInList.css.snap.new b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/disallowInList.css.snap.new index c0b1286dae6..437cbd8af56 100644 --- a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/disallowInList.css.snap.new +++ b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/disallowInList.css.snap.new @@ -30,7 +30,7 @@ v w, x>test {} v { w {} } x > test {} ``` disallowInList.css:8:21 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selectors may result in unintentionally overriding rules: textarea + ! Duplicate selectors may result in unintentionally overriding rules: textarea 7 │ /* duplicate within a grouping selector */ > 8 │ input, textarea {}; textarea {} @@ -54,7 +54,7 @@ disallowInList.css:8:21 lint/nursery/noDuplicateSelectors ━━━━━━━ ``` disallowInList.css:11:22 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selectors may result in unintentionally overriding rules: button + ! Duplicate selectors may result in unintentionally overriding rules: button 10 │ /* duplicate within a grouping selector, reversed */ > 11 │ button {}; selector, button {} @@ -78,8 +78,7 @@ disallowInList.css:11:22 lint/nursery/noDuplicateSelectors ━━━━━━━ ``` disallowInList.css:16:1 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selectors may result in unintentionally overriding rules: - h1 + ! Duplicate selectors may result in unintentionally overriding rules: h1 14 │ span, div {}; 15 │ h1, section {}; @@ -105,7 +104,7 @@ disallowInList.css:16:1 lint/nursery/noDuplicateSelectors ━━━━━━━ ``` disallowInList.css:19:20 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selectors may result in unintentionally overriding rules: w + ! Duplicate selectors may result in unintentionally overriding rules: w 18 │ /* test regular case for regression */ > 19 │ v w, x>test {} v { w {} } x > test {} @@ -125,7 +124,7 @@ disallowInList.css:19:20 lint/nursery/noDuplicateSelectors ━━━━━━━ ``` disallowInList.css:19:27 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selectors may result in unintentionally overriding rules: x > test + ! Duplicate selectors may result in unintentionally overriding rules: x > test 18 │ /* test regular case for regression */ > 19 │ v w, x>test {} v { w {} } x > test {} diff --git a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css.snap.new b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css.snap.new index e1c88265906..91a45dd9fa0 100644 --- a/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css.snap.new +++ b/crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css.snap.new @@ -62,7 +62,7 @@ x { & {} } ``` invalid.css:2:4 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selectors may result in unintentionally overriding rules: a + ! Duplicate selectors may result in unintentionally overriding rules: a 1 │ /* duplicate within one rule's selector list */ > 2 │ a, a {} @@ -86,8 +86,7 @@ invalid.css:2:4 lint/nursery/noDuplicateSelectors ━━━━━━━━━━ ``` invalid.css:6:1 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selectors may result in unintentionally overriding rules: - b + ! Duplicate selectors may result in unintentionally overriding rules: b 4 │ /* duplicate within one rule's selector list. multiline */ 5 │ b, @@ -112,8 +111,7 @@ invalid.css:6:1 lint/nursery/noDuplicateSelectors ━━━━━━━━━━ ``` invalid.css:11:1 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selectors may result in unintentionally overriding rules: - d + ! Duplicate selectors may result in unintentionally overriding rules: d 9 │ c, 10 │ d, @@ -139,8 +137,7 @@ invalid.css:11:1 lint/nursery/noDuplicateSelectors ━━━━━━━━━ ``` invalid.css:15:1 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selectors may result in unintentionally overriding rules: - .e + ! Duplicate selectors may result in unintentionally overriding rules: .e 13 │ /* duplicated selectors within one rule's selector list. 2 duplicates */ 14 │ .e, @@ -165,8 +162,7 @@ invalid.css:15:1 lint/nursery/noDuplicateSelectors ━━━━━━━━━ ``` invalid.css:16:1 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selectors may result in unintentionally overriding rules: - .e + ! Duplicate selectors may result in unintentionally overriding rules: .e 14 │ .e, 15 │ .e, @@ -191,7 +187,7 @@ invalid.css:16:1 lint/nursery/noDuplicateSelectors ━━━━━━━━━ ``` invalid.css:19:11 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selectors may result in unintentionally overriding rules: f + ! Duplicate selectors may result in unintentionally overriding rules: f 18 │ /* duplicate simple selectors with another rule between */ > 19 │ f {} g {} f {} @@ -215,8 +211,7 @@ invalid.css:19:11 lint/nursery/noDuplicateSelectors ━━━━━━━━━ ``` invalid.css:24:1 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selectors may result in unintentionally overriding rules: - h + ! Duplicate selectors may result in unintentionally overriding rules: h 22 │ h {} 23 │ i {} @@ -241,7 +236,7 @@ invalid.css:24:1 lint/nursery/noDuplicateSelectors ━━━━━━━━━ ``` invalid.css:27:9 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selectors may result in unintentionally overriding rules: k, j + ! Duplicate selectors may result in unintentionally overriding rules: k, j 26 │ /* duplicate selector lists with different order */ > 27 │ j, k {} k, j {} @@ -265,8 +260,7 @@ invalid.css:27:9 lint/nursery/noDuplicateSelectors ━━━━━━━━━ ``` invalid.css:31:1 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selectors may result in unintentionally overriding rules: - m n + ! Duplicate selectors may result in unintentionally overriding rules: m n 29 │ /* duplicate selectors with multiple components */ 30 │ m n {} @@ -291,9 +285,8 @@ invalid.css:31:1 lint/nursery/noDuplicateSelectors ━━━━━━━━━ ``` invalid.css:36:3 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selectors may result in unintentionally overriding rules: - #baz, - .foo p,q>.bar + ! Duplicate selectors may result in unintentionally overriding rules: #baz, + .foo p,q>.bar 34 │ .foo p, q > .bar, 35 │ #baz {} @@ -322,7 +315,7 @@ invalid.css:36:3 lint/nursery/noDuplicateSelectors ━━━━━━━━━ ``` invalid.css:41:19 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selectors may result in unintentionally overriding rules: s + ! Duplicate selectors may result in unintentionally overriding rules: s 39 │ /* duplicate within a media query, in the same rule */ 40 │ s {} @@ -348,7 +341,7 @@ invalid.css:41:19 lint/nursery/noDuplicateSelectors ━━━━━━━━━ ``` invalid.css:45:45 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selectors may result in unintentionally overriding rules: t + ! Duplicate selectors may result in unintentionally overriding rules: t 43 │ /* duplicate within a media query, in different rules */ 44 │ t {} @@ -374,7 +367,7 @@ invalid.css:45:45 lint/nursery/noDuplicateSelectors ━━━━━━━━━ ``` invalid.css:48:12 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selectors may result in unintentionally overriding rules: w + ! Duplicate selectors may result in unintentionally overriding rules: w 47 │ /* duplicate caused by nesting */ > 48 │ v w {} v { w {} } @@ -398,7 +391,7 @@ invalid.css:48:12 lint/nursery/noDuplicateSelectors ━━━━━━━━━ ``` invalid.css:51:5 lint/nursery/noDuplicateSelectors ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! Duplicate selectors may result in unintentionally overriding rules: & + ! Duplicate selectors may result in unintentionally overriding rules: & 50 │ /* duplicate caused by &-parent selector */ > 51 │ x { & {} }