Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(biome_css_analyzer): noUnknownUnit #2535

Merged
merged 26 commits into from
Apr 23, 2024
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
1683c6a
chore: add noUnknownUnit rule
neoki07 Apr 19, 2024
b6a5279
test: add no-unknown-unit tests
neoki07 Apr 19, 2024
7eebe23
feat: implement no-unknown-unit rule
neoki07 Apr 20, 2024
4b420a9
test: fix css hack test
neoki07 Apr 20, 2024
5b437d8
test: add snapshots
neoki07 Apr 20, 2024
bfee475
fix: corresponding to edge cases
neoki07 Apr 20, 2024
306e4a4
test: update test cases
neoki07 Apr 20, 2024
9cb08e4
fix: break loop when allowing x
neoki07 Apr 20, 2024
f8af9e9
fix: remove dbg macro
neoki07 Apr 20, 2024
ba1aef2
refactor: change rule state name
neoki07 Apr 20, 2024
0953ba7
chore: fix conflicts
neoki07 Apr 21, 2024
1adcd00
fix: add noUnknownUnit to recommended rules
neoki07 Apr 21, 2024
4990cb9
refactor: sort expressions in match statement
neoki07 Apr 21, 2024
c52e807
fix: set recommended to true
neoki07 Apr 21, 2024
652ca86
fix: use ends_with instead of strip_vendor_prefix
neoki07 Apr 21, 2024
a47ad86
refactor: add documentation on CSS hack
neoki07 Apr 21, 2024
073f1b7
refactor: remove ambiguous document
neoki07 Apr 21, 2024
18cb89b
fix: update diagnostic
neoki07 Apr 21, 2024
1e89e19
fix: update document texts of rule
neoki07 Apr 21, 2024
a801b5d
test: update invalid.css.snap and remove snap.new file
neoki07 Apr 21, 2024
ac189f9
fix: remove processing for css hack
neoki07 Apr 21, 2024
c21d091
refactor: add comment as to why `x` is checked
neoki07 Apr 21, 2024
4ed73c5
refactor: fix comment as to why `x` is checked
neoki07 Apr 21, 2024
8685fdf
chore: update auto generated files
neoki07 Apr 21, 2024
da1338a
Merge remote-tracking branch 'origin/main' into feat/unit-no-unknown
ematipico Apr 22, 2024
1f077ac
chore: run codegen
ematipico Apr 22, 2024
File filter

Filter by extension

Filter by extension


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

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

27 changes: 23 additions & 4 deletions crates/biome_configuration/src/linter/rules.rs

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

1 change: 1 addition & 0 deletions crates/biome_css_analyze/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ biome_css_syntax = { workspace = true }
biome_diagnostics = { workspace = true }
biome_rowan = { workspace = true }
lazy_static = { workspace = true }
regex = { workspace = true }

[dev-dependencies]
biome_css_parser = { path = "../biome_css_parser" }
Expand Down
2 changes: 2 additions & 0 deletions crates/biome_css_analyze/src/lint/nursery.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,15 @@ use biome_analyze::declare_group;

pub mod no_color_invalid_hex;
pub mod no_duplicate_font_names;
pub mod no_unknown_unit;

declare_group! {
pub Nursery {
name : "nursery" ,
rules : [
self :: no_color_invalid_hex :: NoColorInvalidHex ,
self :: no_duplicate_font_names :: NoDuplicateFontNames ,
self :: no_unknown_unit :: NoUnknownUnit ,
]
}
}
191 changes: 191 additions & 0 deletions crates/biome_css_analyze/src/lint/nursery/no_unknown_unit.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,191 @@
use biome_analyze::{context::RuleContext, declare_rule, Ast, Rule, RuleDiagnostic, RuleSource};
use biome_console::markup;
use biome_css_syntax::{
AnyCssDimension, CssFunction, CssGenericProperty, CssQueryFeaturePlain, CssSyntaxKind,
};
use biome_rowan::{SyntaxNodeCast, TextRange};

use crate::utils::strip_vendor_prefix;

const RESOLUTION_MEDIA_FEATURE_NAMES: [&str; 3] =
["resolution", "min-resolution", "max-resolution"];

fn is_css_hack_unit(value: &str) -> bool {
Copy link
Member

Choose a reason for hiding this comment

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

Can you please document what this hack is for and why it exists?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added a description of the function and the reason for using this function where it's called.

value == "\\0"
}

declare_rule! {
/// Disallow unknown units.
///
/// This rule considers units defined in the CSS Specifications, up to and including Editor's Drafts, to be known.
ematipico marked this conversation as resolved.
Show resolved Hide resolved
///
///
/// ## Examples
///
/// ### Invalid
///
/// ```css,expect_diagnostic
/// a {
/// width: 10pixels;
/// }
/// ```
///
/// ```css,expect_diagnostic
/// a {
/// width: calc(10px + 10pixels);
/// }
/// ```
///
/// ### Valid
///
/// ```css
/// a {
/// width: 10px;
/// }
/// ```
///
/// ```css
/// a {
/// width: 10Px;
/// }
/// ```
///
/// ```css
/// a {
/// width: 10pX;
/// }
/// ```
///
/// ```css
/// a {
/// width: calc(10px + 10px);
/// }
/// ```
///
pub NoUnknownUnit {
version: "next",
name: "noUnknownUnit",
recommended: false,
sources: &[RuleSource::Stylelint("unit-no-unknown")],
}
}

pub struct RuleState {
value: String,
span: TextRange,
}

impl Rule for NoUnknownUnit {
type Query = Ast<AnyCssDimension>;
type State = RuleState;
type Signals = Option<Self::State>;
type Options = ();

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

// dbg!(ctx.root());

match node {
AnyCssDimension::CssUnknownDimension(dimension) => {
let unit_token = dimension.unit_token().ok()?;
let unit = unit_token.text_trimmed().to_string();

if is_css_hack_unit(&unit) {
return None;
}

Some(RuleState {
value: unit,
span: unit_token.text_trimmed_range(),
})
}
AnyCssDimension::CssRegularDimension(dimension) => {
let unit_token = dimension.unit_token().ok()?;
let unit = unit_token.text_trimmed().to_string();

if unit == "x" {
ematipico marked this conversation as resolved.
Show resolved Hide resolved
let mut allow_x = false;

for ancestor in dimension.unit_token().ok()?.ancestors() {
match ancestor.kind() {
CssSyntaxKind::CSS_FUNCTION => {
let function_name = ancestor
.cast::<CssFunction>()?
.name()
.ok()?
.value_token()
.ok()?
.text_trimmed()
.to_lowercase();

if strip_vendor_prefix(function_name.as_str()) == "image-set" {
allow_x = true;
break;
}
}
CssSyntaxKind::CSS_QUERY_FEATURE_PLAIN => {
let feature_name = ancestor
.cast::<CssQueryFeaturePlain>()?
.name()
.ok()?
.value_token()
.ok()?
.text_trimmed()
.to_lowercase();

if RESOLUTION_MEDIA_FEATURE_NAMES.contains(&feature_name.as_str()) {
allow_x = true;
break;
}
}
CssSyntaxKind::CSS_GENERIC_PROPERTY => {
let property_name = ancestor
.cast::<CssGenericProperty>()?
.name()
.ok()?
.as_css_identifier()?
.value_token()
.ok()?
.text_trimmed()
.to_lowercase();

if property_name == "image-resolution" {
allow_x = true;
break;
}
}
_ => {}
}
}

if !allow_x {
return Some(RuleState {
value: unit,
span: unit_token.text_trimmed_range(),
});
}
}

None
}
_ => None,
}
}

fn diagnostic(_: &RuleContext<Self>, state: &Self::State) -> Option<RuleDiagnostic> {
let span = state.span;
Some(
RuleDiagnostic::new(
rule_category!(),
span,
markup! {
"Unexpected unknown unit: "<Emphasis>{ state.value }</Emphasis>
},
)
.note(markup! {
"Fix to a known unit."
ematipico marked this conversation as resolved.
Show resolved Hide resolved
}),
)
}
}
2 changes: 2 additions & 0 deletions crates/biome_css_analyze/src/options.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,5 @@ pub type NoColorInvalidHex =
<lint::nursery::no_color_invalid_hex::NoColorInvalidHex as biome_analyze::Rule>::Options;
pub type NoDuplicateFontNames =
<lint::nursery::no_duplicate_font_names::NoDuplicateFontNames as biome_analyze::Rule>::Options;
pub type NoUnknownUnit =
<lint::nursery::no_unknown_unit::NoUnknownUnit as biome_analyze::Rule>::Options;
6 changes: 6 additions & 0 deletions crates/biome_css_analyze/src/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use crate::keywords::{
};
use biome_css_syntax::{AnyCssGenericComponentValue, AnyCssValue, CssGenericComponentValueList};
use biome_rowan::{AstNode, SyntaxNodeCast};
use regex::Regex;

pub fn is_font_family_keyword(value: &str) -> bool {
BASIC_KEYWORDS.contains(&value) || FONT_FAMILY_KEYWORDS.contains(&value)
Expand Down Expand Up @@ -92,3 +93,8 @@ pub fn find_font_family(value: CssGenericComponentValueList) -> Vec<AnyCssValue>
}
font_families
}

pub fn strip_vendor_prefix(property: &str) -> String {
let re = Regex::new(r"^-\w+-").unwrap();
re.replace(property, "").to_string()
}
ematipico marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have brought stylelint tests (link). However, root { --margin: 10px + 10pix; } was not added because the syntax seems incorrect.

Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
a { font-size: 13pp; }
a { margin: 13xpx; }
a { font-size: .5remm; }
a { font-size: 0.5remm; }
a { color: rgb(255pix, 0, 51); }
a { color: hsl(255pix, 0, 51); }
a { color: rgba(255pix, 0, 51, 1); }
a { color: hsla(255pix, 0, 51, 1); }
a { margin: calc(13pix + 10px); }
a { margin: calc(10pix*2); }
a { margin: calc(2*10pix); }
a { -webkit-transition-delay: 10pix; }
a { margin: -webkit-calc(13pix + 10px); }
a { margin: some-function(13pix + 10px); }
root { --margin: 10pix; }
@media (min-width: 13pix) {}
@media (min-width: 10px)\n and (max-width: 20PIX) {}
@media (width < 10.01REMS) {}
a { width: 1e4pz; }
a { flex: 0 9r9 auto; }
a { width: 400x; }
@media (resolution: 2x) and (min-width: 200x) {}
@media ( resolution: /* comment */ 2x ) and (min-width: 200x) {}
a { background: image-set('img1x.png' 1x, 'img2x.png' 2x) left 20x / 15% 60% repeat-x; }
a { background: /* comment */ image-set('img1x.png' 1x, 'img2x.png' 2x) left 20x / 15% 60% repeat-x; }
a { background-image: image-set('img1x.png' 1pix, 'img2x.png' 2x); }
@font-face { color: U+0100-024F; }
a { unicode-range: U+0100-024F; }
Loading