Skip to content

Commit

Permalink
fix(linter): fix false-positve and improve the error of noDuplicateCu…
Browse files Browse the repository at this point in the history 
…stomProperties (#4290)
  • Loading branch information
@togami2864
togami2864 authored Oct 14, 2024
1 parent 1d240cc commit 7bab172
Show file tree
Hide file tree
Showing 5 changed files with 122 additions and 42 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,9 @@ our [guidelines for writing a good changelog entry](https://github.com/biomejs/b

Contributed by @fireairforce

- [noDuplicateCustomProperties](https://biomejs.dev/linter/rules/no-duplicate-custom-properties/) now correctly handles custom properties and ignores non-custom properties.
Previously, the rule incorrectly reported duplicates for all properties, including non-custom ones. Contributed by @togami2864

### Parser

#### Bug Fixes
Expand Down
59 changes: 33 additions & 26 deletions crates/biome_css_analyze/src/lint/nursery/no_duplicate_custom_properties.rs
View file
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
use std::collections::hash_map::Entry;

use biome_analyze::{context::RuleContext, declare_lint_rule, Rule, RuleDiagnostic, RuleSource};
use biome_console::markup;
use biome_css_semantic::model::CssProperty;
use biome_css_syntax::CssDeclarationOrRuleList;
use biome_rowan::{AstNode, TextRange};
use rustc_hash::FxHashSet;
use rustc_hash::FxHashMap;

use crate::services::semantic::Semantic;

Expand Down Expand Up @@ -45,7 +46,7 @@ declare_lint_rule! {

impl Rule for NoDuplicateCustomProperties {
type Query = Semantic<CssDeclarationOrRuleList>;
type State = TextRange;
type State = (TextRange, (TextRange, String));
type Signals = Option<Self::State>;
type Options = ();

Expand All @@ -55,42 +56,48 @@ impl Rule for NoDuplicateCustomProperties {

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

let properties = rule
.declarations
.iter()
.map(|d| d.property.clone())
.collect::<Vec<_>>();
let mut seen: FxHashMap<&str, TextRange> = FxHashMap::default();

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

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

if let Some(range) = check_duplicate_custom_properties(properties) {
return Some(range);
if !is_custom_property {
continue;
}

match seen.entry(prop_name) {
Entry::Occupied(entry) => {
return Some((*entry.get(), (prop_range, prop_name.to_string())));
}
Entry::Vacant(_) => {
seen.insert(prop_name, prop_range);
}
}
}

None
}

fn diagnostic(_: &RuleContext<Self>, range: &Self::State) -> Option<RuleDiagnostic> {
fn diagnostic(_: &RuleContext<Self>, state: &Self::State) -> Option<RuleDiagnostic> {
let (first_occurrence_range, (duplicate_range, duplicate_property_name)) = state;
Some(
RuleDiagnostic::new(
rule_category!(),
range,
duplicate_range,
markup! {
"Duplicate custom properties are not allowed."
"Duplicate custom properties can lead to unexpected behavior and may override previous declarations unintentionally."
},
)
.detail(first_occurrence_range, markup! {
<Emphasis>{duplicate_property_name}</Emphasis> " is already defined here."
})
.note(markup! {
"Consider removing the duplicate custom property."
"Remove or rename the duplicate custom property to ensure consistent styling."
}),
)
}
}

fn check_duplicate_custom_properties(properties: Vec<CssProperty>) -> Option<TextRange> {
let mut seen = FxHashSet::default();

for property in properties {
if !seen.insert(property.name) {
return Some(property.range);
}
}

None
}
100 changes: 84 additions & 16 deletions crates/biome_css_analyze/tests/specs/nursery/noDuplicateCustomProperties/invalid.css.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,38 +19,53 @@ a { --custom-property: pink; @media { --custom-property: orange; .foo { --custom
```
invalid.css:1:27 lint/nursery/noDuplicateCustomProperties ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
! Duplicate custom properties are not allowed.
! Duplicate custom properties can lead to unexpected behavior and may override previous declarations unintentionally.
> 1 │ a { --custom-property: 1; --custom-property: 2; }
│ ^^^^^^^^^^^^^^^^^
2 │ a { --custom-property: 1; color: pink; --custom-property: 1; }
3 │ a { --custom-property: 1; --cUstOm-prOpErtY: 1; color: pink; --cUstOm-prOpErtY: 1; }
i Consider removing the duplicate custom property.
i --custom-property is already defined here.
> 1 │ a { --custom-property: 1; --custom-property: 2; }
│ ^^^^^^^^^^^^^^^^^
2 │ a { --custom-property: 1; color: pink; --custom-property: 1; }
3 │ a { --custom-property: 1; --cUstOm-prOpErtY: 1; color: pink; --cUstOm-prOpErtY: 1; }
i Remove or rename the duplicate custom property to ensure consistent styling.
```

```
invalid.css:2:40 lint/nursery/noDuplicateCustomProperties ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
! Duplicate custom properties are not allowed.
! Duplicate custom properties can lead to unexpected behavior and may override previous declarations unintentionally.
1 │ a { --custom-property: 1; --custom-property: 2; }
> 2 │ a { --custom-property: 1; color: pink; --custom-property: 1; }
│ ^^^^^^^^^^^^^^^^^
3 │ a { --custom-property: 1; --cUstOm-prOpErtY: 1; color: pink; --cUstOm-prOpErtY: 1; }
4 │ a { --custom-property: pink; { &:hover { --custom-property: orange; --custom-property: black; } } }
i Consider removing the duplicate custom property.
i --custom-property is already defined here.
1 │ a { --custom-property: 1; --custom-property: 2; }
> 2 │ a { --custom-property: 1; color: pink; --custom-property: 1; }
│ ^^^^^^^^^^^^^^^^^
3 │ a { --custom-property: 1; --cUstOm-prOpErtY: 1; color: pink; --cUstOm-prOpErtY: 1; }
4 │ a { --custom-property: pink; { &:hover { --custom-property: orange; --custom-property: black; } } }
i Remove or rename the duplicate custom property to ensure consistent styling.
```

```
invalid.css:3:62 lint/nursery/noDuplicateCustomProperties ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
! Duplicate custom properties are not allowed.
! Duplicate custom properties can lead to unexpected behavior and may override previous declarations unintentionally.
1 │ a { --custom-property: 1; --custom-property: 2; }
2 │ a { --custom-property: 1; color: pink; --custom-property: 1; }
Expand All @@ -59,15 +74,24 @@ invalid.css:3:62 lint/nursery/noDuplicateCustomProperties ━━━━━━━
4 │ a { --custom-property: pink; { &:hover { --custom-property: orange; --custom-property: black; } } }
5 │ a { --custom-property: pink; @media { --custom-property: orange; --custom-property: black; } }
i Consider removing the duplicate custom property.
i --cUstOm-prOpErtY is already defined here.
1 │ a { --custom-property: 1; --custom-property: 2; }
2 │ a { --custom-property: 1; color: pink; --custom-property: 1; }
> 3 │ a { --custom-property: 1; --cUstOm-prOpErtY: 1; color: pink; --cUstOm-prOpErtY: 1; }
│ ^^^^^^^^^^^^^^^^^
4 │ a { --custom-property: pink; { &:hover { --custom-property: orange; --custom-property: black; } } }
5 │ a { --custom-property: pink; @media { --custom-property: orange; --custom-property: black; } }
i Remove or rename the duplicate custom property to ensure consistent styling.
```

```
invalid.css:4:69 lint/nursery/noDuplicateCustomProperties ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
! Duplicate custom properties are not allowed.
! Duplicate custom properties can lead to unexpected behavior and may override previous declarations unintentionally.
2 │ a { --custom-property: 1; color: pink; --custom-property: 1; }
3 │ a { --custom-property: 1; --cUstOm-prOpErtY: 1; color: pink; --cUstOm-prOpErtY: 1; }
Expand All @@ -76,15 +100,24 @@ invalid.css:4:69 lint/nursery/noDuplicateCustomProperties ━━━━━━━
5 │ a { --custom-property: pink; @media { --custom-property: orange; --custom-property: black; } }
6 │ @media { --custom-property: orange; .foo { --custom-property: black; --custom-property: white; } }
i Consider removing the duplicate custom property.
i --custom-property is already defined here.
2 │ a { --custom-property: 1; color: pink; --custom-property: 1; }
3 │ a { --custom-property: 1; --cUstOm-prOpErtY: 1; color: pink; --cUstOm-prOpErtY: 1; }
> 4 │ a { --custom-property: pink; { &:hover { --custom-property: orange; --custom-property: black; } } }
│ ^^^^^^^^^^^^^^^^^
5 │ a { --custom-property: pink; @media { --custom-property: orange; --custom-property: black; } }
6 │ @media { --custom-property: orange; .foo { --custom-property: black; --custom-property: white; } }
i Remove or rename the duplicate custom property to ensure consistent styling.
```

```
invalid.css:5:66 lint/nursery/noDuplicateCustomProperties ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
! Duplicate custom properties are not allowed.
! Duplicate custom properties can lead to unexpected behavior and may override previous declarations unintentionally.
3 │ a { --custom-property: 1; --cUstOm-prOpErtY: 1; color: pink; --cUstOm-prOpErtY: 1; }
4 │ a { --custom-property: pink; { &:hover { --custom-property: orange; --custom-property: black; } } }
Expand All @@ -93,15 +126,24 @@ invalid.css:5:66 lint/nursery/noDuplicateCustomProperties ━━━━━━━
6 │ @media { --custom-property: orange; .foo { --custom-property: black; --custom-property: white; } }
7 │ a { --custom-property: pink; @media { --custom-property: orange; &::before { --custom-property: black; --custom-property: white; } } }
i Consider removing the duplicate custom property.
i --custom-property is already defined here.
3 │ a { --custom-property: 1; --cUstOm-prOpErtY: 1; color: pink; --cUstOm-prOpErtY: 1; }
4 │ a { --custom-property: pink; { &:hover { --custom-property: orange; --custom-property: black; } } }
> 5 │ a { --custom-property: pink; @media { --custom-property: orange; --custom-property: black; } }
│ ^^^^^^^^^^^^^^^^^
6 │ @media { --custom-property: orange; .foo { --custom-property: black; --custom-property: white; } }
7 │ a { --custom-property: pink; @media { --custom-property: orange; &::before { --custom-property: black; --custom-property: white; } } }
i Remove or rename the duplicate custom property to ensure consistent styling.
```

```
invalid.css:6:70 lint/nursery/noDuplicateCustomProperties ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
! Duplicate custom properties are not allowed.
! Duplicate custom properties can lead to unexpected behavior and may override previous declarations unintentionally.
4 │ a { --custom-property: pink; { &:hover { --custom-property: orange; --custom-property: black; } } }
5 │ a { --custom-property: pink; @media { --custom-property: orange; --custom-property: black; } }
Expand All @@ -110,15 +152,24 @@ invalid.css:6:70 lint/nursery/noDuplicateCustomProperties ━━━━━━━
7 │ a { --custom-property: pink; @media { --custom-property: orange; &::before { --custom-property: black; --custom-property: white; } } }
8 │ a { --custom-property: pink; @media { --custom-property: orange; .foo { --custom-property: black; --custom-property: white; } } }
i Consider removing the duplicate custom property.
i --custom-property is already defined here.
4 │ a { --custom-property: pink; { &:hover { --custom-property: orange; --custom-property: black; } } }
5 │ a { --custom-property: pink; @media { --custom-property: orange; --custom-property: black; } }
> 6 │ @media { --custom-property: orange; .foo { --custom-property: black; --custom-property: white; } }
│ ^^^^^^^^^^^^^^^^^
7 │ a { --custom-property: pink; @media { --custom-property: orange; &::before { --custom-property: black; --custom-property: white; } } }
8 │ a { --custom-property: pink; @media { --custom-property: orange; .foo { --custom-property: black; --custom-property: white; } } }
i Remove or rename the duplicate custom property to ensure consistent styling.
```

```
invalid.css:7:104 lint/nursery/noDuplicateCustomProperties ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
! Duplicate custom properties are not allowed.
! Duplicate custom properties can lead to unexpected behavior and may override previous declarations unintentionally.
5 │ a { --custom-property: pink; @media { --custom-property: orange; --custom-property: black; } }
6 │ @media { --custom-property: orange; .foo { --custom-property: black; --custom-property: white; } }
Expand All @@ -127,23 +178,40 @@ invalid.css:7:104 lint/nursery/noDuplicateCustomProperties ━━━━━━━
8 │ a { --custom-property: pink; @media { --custom-property: orange; .foo { --custom-property: black; --custom-property: white; } } }
9 │
i Consider removing the duplicate custom property.
i --custom-property is already defined here.
5 │ a { --custom-property: pink; @media { --custom-property: orange; --custom-property: black; } }
6 │ @media { --custom-property: orange; .foo { --custom-property: black; --custom-property: white; } }
> 7 │ a { --custom-property: pink; @media { --custom-property: orange; &::before { --custom-property: black; --custom-property: white; } } }
│ ^^^^^^^^^^^^^^^^^
8 │ a { --custom-property: pink; @media { --custom-property: orange; .foo { --custom-property: black; --custom-property: white; } } }
9 │
i Remove or rename the duplicate custom property to ensure consistent styling.
```

```
invalid.css:8:99 lint/nursery/noDuplicateCustomProperties ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
! Duplicate custom properties are not allowed.
! Duplicate custom properties can lead to unexpected behavior and may override previous declarations unintentionally.
6 │ @media { --custom-property: orange; .foo { --custom-property: black; --custom-property: white; } }
7 │ a { --custom-property: pink; @media { --custom-property: orange; &::before { --custom-property: black; --custom-property: white; } } }
> 8 │ a { --custom-property: pink; @media { --custom-property: orange; .foo { --custom-property: black; --custom-property: white; } } }
│ ^^^^^^^^^^^^^^^^^
9 │
i Consider removing the duplicate custom property.
i --custom-property is already defined here.
6 │ @media { --custom-property: orange; .foo { --custom-property: black; --custom-property: white; } }
7 │ a { --custom-property: pink; @media { --custom-property: orange; &::before { --custom-property: black; --custom-property: white; } } }
> 8 │ a { --custom-property: pink; @media { --custom-property: orange; .foo { --custom-property: black; --custom-property: white; } } }
│ ^^^^^^^^^^^^^^^^^
9 │
i Remove or rename the duplicate custom property to ensure consistent styling.
```
1 change: 1 addition & 0 deletions crates/biome_css_analyze/tests/specs/nursery/noDuplicateCustomProperties/valid.css
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,4 @@ a { --custom-property: pink; @media { --custom-property: orange; } }
a { --custom-property: pink; @media { --custom-property: orange; &::before { --custom-property: black; } } }
a { --custom-property: pink; { &:hover { --custom-property: orange; --cUstOm-prOpErtY: black; } } }
a { --cUstOm-prOpErtY: pink; { &:hover { --custom-property: orange; --cUstOm-prOpErtY: black; } } }
a { color: red; color: pink;}
1 change: 1 addition & 0 deletions crates/biome_css_analyze/tests/specs/nursery/noDuplicateCustomProperties/valid.css.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,5 +12,6 @@ a { --custom-property: pink; @media { --custom-property: orange; } }
a { --custom-property: pink; @media { --custom-property: orange; &::before { --custom-property: black; } } }
a { --custom-property: pink; { &:hover { --custom-property: orange; --cUstOm-prOpErtY: black; } } }
a { --cUstOm-prOpErtY: pink; { &:hover { --custom-property: orange; --cUstOm-prOpErtY: black; } } }
a { color: red; color: pink;}
```

0 comments on commit 7bab172

Please sign in to comment.