feat(lint): add nursery rule noTailwindArbitraryValue#10094
feat(lint): add nursery rule noTailwindArbitraryValue#10094THEjacob1000 wants to merge 19 commits into
Conversation
🦋 Changeset detectedLatest commit: 39fea32 The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
Bot
added
A-Project
dyc3
added
the
M-Likely Agent
THEjacob1000
changed the title
THEjacob1000
force-pushed
the
feat/no-arbitrary-value
branch
3 times, most recently
from
86571c0 to
c14c596
Compare
THEjacob1000
force-pushed
the
feat/no-arbitrary-value
branch
from
c14c596 to
1bde577
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new nursery lint rule Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
crates/biome_js_analyze/tests/specs/nursery/noArbitraryValue/invalid_functions.jsx (1)
1-7: Solid coverage of the function/template surface.Strings, multi-arg, object keys, plain tagged template, and static-member tagged template are all here — that's exactly the matrix the rule's matcher cares about. Optional nit: an array-literal arg like
clsx(["w-[400px]"])would round out the call-expression coverage if the rule supports it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_js_analyze/tests/specs/nursery/noArbitraryValue/invalid_functions.jsx` around lines 1 - 7, Add an extra test case that supplies an array-literal argument to the call-expression surface the rule matches (to cover the optional nit mentioned); specifically add a call such as using the existing functions/identifiers clsx, cn or classnames with an array argument (e.g., clsx([...]) or classnames([...])) so the test file that already includes clsx, cn, classnames, tw and tw.div also covers array-literal argument forms for the rule's matcher..changeset/add-no-arbitrary-value.md (1)
1-6: Changeset reads well.
patch+mainis the correct combo for a new nursery rule, and the rule link is in place. Tiny optional polish: you could mention the defaultclass/classNameattribute coverage alongside the "configured utility functions and tagged templates" — that's the headline behaviour users will hit out of the box.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.changeset/add-no-arbitrary-value.md around lines 1 - 6, Update the changeset text for the new nursery rule `noArbitraryValue` in .changeset/add-no-arbitrary-value.md to explicitly mention that, in addition to configured utility functions and tagged templates, the rule covers the default `class`/`className` attributes out of the box; edit the descriptive paragraph to append a short phrase like "and the default class/className attributes" so users immediately see the primary, default behaviour of `noArbitraryValue`.crates/biome_js_analyze/src/lint/nursery/no_arbitrary_value.rs (2)
67-67: Dropfix_kind: FixKind::None— the rule doesn't implementaction().This rule only emits diagnostics, so the field should be omitted entirely (and
FixKindremoved from the import on line 2). SpecifyingFixKind::Nonefalsely advertises a fix path that isn't implemented.♻️ Proposed diff
-use biome_analyze::{Ast, FixKind, Rule, RuleDiagnostic, context::RuleContext, declare_lint_rule}; +use biome_analyze::{Ast, Rule, RuleDiagnostic, context::RuleContext, declare_lint_rule}; @@ pub NoArbitraryValue { version: "next", name: "noArbitraryValue", language: "jsx", recommended: false, - fix_kind: FixKind::None, }Based on learnings: "When defining lint rules (declare_lint_rule!), only specify fix_kind if the rule implements an action(...) function. Rules that only emit diagnostics without a code fix should omit fix_kind."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_js_analyze/src/lint/nursery/no_arbitrary_value.rs` at line 67, Remove the unnecessary fix_kind declaration and import: in the declare_lint_rule! invocation (the rule struct defined in no_arbitrary_value.rs) delete the `fix_kind: FixKind::None,` entry and remove `FixKind` from the imports at the top, since this rule does not implement an `action()`/fix path; leave the rule as diagnostics-only so it no longer advertises a nonexistent fix.
80-87: Minor: round-tripping options intoUseSortedClassesOptionsis awkward.You're cloning both
Box<[Box<str>]>allocations purely to satisfyshould_visit's signature. If you adopt the refactor suggested inno_arbitrary_value.rsoptions (newtype/shared inner), this could just benode.should_visit(&options.0)or similar without the clone. Pairs with the duplication note in the rule_options file — feel free to defer to a follow-up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_js_analyze/src/lint/nursery/no_arbitrary_value.rs` around lines 80 - 87, The current code creates a temporary UseSortedClassesOptions by cloning Box<[Box<str>]> fields to satisfy node.should_visit, which is inefficient; update the options types so you can pass a reference instead of cloning: either change UseSortedClassesOptions/should_visit to accept a reference to the shared inner options or introduce a newtype/shared inner for the existing options (e.g., options.0) and call node.should_visit(&options.0) directly; remove the clones and the intermediate UseSortedClassesOptions construction in the block around UseSortedClassesOptions and node.should_visit to avoid unnecessary allocations.crates/biome_rule_options/src/no_arbitrary_value.rs (1)
1-88: Heads-up: this is a near-verbatim clone ofUseSortedClassesOptions.Per
crates/biome_rule_options/src/use_sorted_classes.rs:1-88, the struct shape,Mergeimpl,ALLOWED_OPTIONS, andDeserializationVisitorare identical apart from doc strings and the type name. Since the rule itself converts back intoUseSortedClassesOptionsto callshould_visit, consider either:
- factoring the visitor/
Merge/ALLOWED_OPTIONSinto a shared helper that both option types reuse, or- having
NoArbitraryValueOptionswrapUseSortedClassesOptions(e.g.#[serde(transparent)]newtype or apub inner: UseSortedClassesOptionsfield) so the two stay in lockstep automatically.Not a blocker for this PR — the duplication just means future tweaks to
attributes/functionsdeserialization must be applied in two places.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_rule_options/src/no_arbitrary_value.rs` around lines 1 - 88, The NoArbitraryValueOptions code duplicates UseSortedClassesOptions behavior; fix by either extracting the shared pieces (ALLOWED_OPTIONS, Merge impl, and DeserializationVisitor) into a common helper/utility that both NoArbitraryValueOptions and UseSortedClassesOptions reuse, or make NoArbitraryValueOptions a thin wrapper around UseSortedClassesOptions (e.g. add a pub inner: UseSortedClassesOptions field or #[serde(transparent)] newtype) and update NoArbitraryValueOptions::merge_with, Deserializable impl, and its DeserializationVisitor to delegate to/use UseSortedClassesOptions’ Merge and visitor logic so attributes/functions deserialization stays in sync.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.changeset/add-no-arbitrary-value.md:
- Around line 1-6: Update the changeset text for the new nursery rule
`noArbitraryValue` in .changeset/add-no-arbitrary-value.md to explicitly mention
that, in addition to configured utility functions and tagged templates, the rule
covers the default `class`/`className` attributes out of the box; edit the
descriptive paragraph to append a short phrase like "and the default
class/className attributes" so users immediately see the primary, default
behaviour of `noArbitraryValue`.
In `@crates/biome_js_analyze/src/lint/nursery/no_arbitrary_value.rs`:
- Line 67: Remove the unnecessary fix_kind declaration and import: in the
declare_lint_rule! invocation (the rule struct defined in no_arbitrary_value.rs)
delete the `fix_kind: FixKind::None,` entry and remove `FixKind` from the
imports at the top, since this rule does not implement an `action()`/fix path;
leave the rule as diagnostics-only so it no longer advertises a nonexistent fix.
- Around line 80-87: The current code creates a temporary
UseSortedClassesOptions by cloning Box<[Box<str>]> fields to satisfy
node.should_visit, which is inefficient; update the options types so you can
pass a reference instead of cloning: either change
UseSortedClassesOptions/should_visit to accept a reference to the shared inner
options or introduce a newtype/shared inner for the existing options (e.g.,
options.0) and call node.should_visit(&options.0) directly; remove the clones
and the intermediate UseSortedClassesOptions construction in the block around
UseSortedClassesOptions and node.should_visit to avoid unnecessary allocations.
In
`@crates/biome_js_analyze/tests/specs/nursery/noArbitraryValue/invalid_functions.jsx`:
- Around line 1-7: Add an extra test case that supplies an array-literal
argument to the call-expression surface the rule matches (to cover the optional
nit mentioned); specifically add a call such as using the existing
functions/identifiers clsx, cn or classnames with an array argument (e.g.,
clsx([...]) or classnames([...])) so the test file that already includes clsx,
cn, classnames, tw and tw.div also covers array-literal argument forms for the
rule's matcher.
In `@crates/biome_rule_options/src/no_arbitrary_value.rs`:
- Around line 1-88: The NoArbitraryValueOptions code duplicates
UseSortedClassesOptions behavior; fix by either extracting the shared pieces
(ALLOWED_OPTIONS, Merge impl, and DeserializationVisitor) into a common
helper/utility that both NoArbitraryValueOptions and UseSortedClassesOptions
reuse, or make NoArbitraryValueOptions a thin wrapper around
UseSortedClassesOptions (e.g. add a pub inner: UseSortedClassesOptions field or
#[serde(transparent)] newtype) and update NoArbitraryValueOptions::merge_with,
Deserializable impl, and its DeserializationVisitor to delegate to/use
UseSortedClassesOptions’ Merge and visitor logic so attributes/functions
deserialization stays in sync.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e3d13417-00e7-4920-a651-efae4c78d54b
⛔ Files ignored due to path filters (10)
Cargo.lockis excluded by!**/*.lockand included by**crates/biome_configuration/src/analyzer/linter/rules.rsis excluded by!**/rules.rsand included by**crates/biome_configuration/src/generated/linter_options_check.rsis excluded by!**/generated/**,!**/generated/**and included by**crates/biome_diagnostics_categories/src/categories.rsis excluded by!**/categories.rsand included by**crates/biome_js_analyze/tests/specs/nursery/noArbitraryValue/invalid.jsx.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/noArbitraryValue/invalid_functions.jsx.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/noArbitraryValue/valid.jsx.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/noArbitraryValue/valid_functions.jsx.snapis excluded by!**/*.snapand included by**packages/@biomejs/backend-jsonrpc/src/workspace.tsis excluded by!**/backend-jsonrpc/src/workspace.tsand included by**packages/@biomejs/biome/configuration_schema.jsonis excluded by!**/configuration_schema.jsonand included by**
📒 Files selected for processing (11)
.changeset/add-no-arbitrary-value.mdcrates/biome_js_analyze/Cargo.tomlcrates/biome_js_analyze/src/lint/nursery/no_arbitrary_value.rscrates/biome_js_analyze/tests/specs/nursery/noArbitraryValue/invalid.jsxcrates/biome_js_analyze/tests/specs/nursery/noArbitraryValue/invalid_functions.jsxcrates/biome_js_analyze/tests/specs/nursery/noArbitraryValue/invalid_functions.options.jsoncrates/biome_js_analyze/tests/specs/nursery/noArbitraryValue/valid.jsxcrates/biome_js_analyze/tests/specs/nursery/noArbitraryValue/valid_functions.jsxcrates/biome_js_analyze/tests/specs/nursery/noArbitraryValue/valid_functions.options.jsoncrates/biome_rule_options/src/lib.rscrates/biome_rule_options/src/no_arbitrary_value.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/biome_js_analyze/src/lint/nursery/no_arbitrary_value.rs`:
- Around line 42-60: Update the rule docs in no_arbitrary_value.rs to include
default values for the documented options ("attributes" and "functions") and
replace the bare JSON block under "## Options" with a proper JSON options block
paired with a code example using the use_options (or expect_diagnostic)
directive; specifically state the defaults (e.g., attributes default to
["class","className"], functions default to the listed defaults), add a
reproducible code snippet that demonstrates an arbitrary value (for example a
JSX line with clsx("w-[400px]")), and reference an accompanying *.options.json
that sets functions: ["clsx"] so the documentation and documentation codegen
include the example and defaults.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 81c93f1f-7f1e-49a1-b01a-9643721da167
⛔ Files ignored due to path filters (1)
crates/biome_js_analyze/tests/specs/nursery/noArbitraryValue/invalid_functions.jsx.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (4)
.changeset/add-no-arbitrary-value.mdcrates/biome_js_analyze/src/lint/nursery/no_arbitrary_value.rscrates/biome_js_analyze/tests/specs/nursery/noArbitraryValue/invalid_functions.jsxcrates/biome_rule_options/src/no_arbitrary_value.rs
✅ Files skipped from review due to trivial changes (1)
- .changeset/add-no-arbitrary-value.md
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/biome_js_analyze/tests/specs/nursery/noArbitraryValue/invalid_functions.jsx
- crates/biome_rule_options/src/no_arbitrary_value.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/biome_js_analyze/src/lint/nursery/no_arbitrary_value.rs (1)
172-174: Avoidunwrap()on the offset conversion.In practice the offset can't exceed
u32::MAX(the parser would have failed long before) so this is mostly cosmetic, but a bareunwrap()here gives no context if it ever did fire. A shortexpectmessage — orTextSize::try_from(offset).unwrap_or(TextSize::from(u32::MAX))— would age better.♻️ Suggested tweak
fn text_size(offset: usize) -> TextSize { - TextSize::from(u32::try_from(offset).unwrap()) + TextSize::from( + u32::try_from(offset).expect("class offset within source file should fit into u32"), + ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_js_analyze/src/lint/nursery/no_arbitrary_value.rs` around lines 172 - 174, Replace the bare unwrap in fn text_size(offset: usize) -> TextSize: instead of u32::try_from(offset).unwrap() use a checked conversion with a clear failure message or a safe fallback; e.g. call u32::try_from(offset).expect("text_size: offset exceeds u32::MAX") and pass that into TextSize::from, or use TextSize::try_from(offset).unwrap_or(TextSize::from(u32::MAX)) so the conversion in text_size() gives contextual diagnostics or a safe clamp rather than panicking with no context.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/biome_js_analyze/src/lint/nursery/no_arbitrary_value.rs`:
- Around line 65-67: The JSX example under the use_options doc block is actually
a violation (clsx("w-[400px]") given functions: ["clsx"]) so update the code
fence for that example to include expect_diagnostic: change the triple-backtick
tag from "jsx,use_options" to "jsx,use_options,expect_diagnostic" so the doctest
expects one diagnostic for the clsx call in no_arbitrary_value.rs's
documentation example.
---
Nitpick comments:
In `@crates/biome_js_analyze/src/lint/nursery/no_arbitrary_value.rs`:
- Around line 172-174: Replace the bare unwrap in fn text_size(offset: usize) ->
TextSize: instead of u32::try_from(offset).unwrap() use a checked conversion
with a clear failure message or a safe fallback; e.g. call
u32::try_from(offset).expect("text_size: offset exceeds u32::MAX") and pass that
into TextSize::from, or use
TextSize::try_from(offset).unwrap_or(TextSize::from(u32::MAX)) so the conversion
in text_size() gives contextual diagnostics or a safe clamp rather than
panicking with no context.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2b2422e4-9403-4c29-89af-03a6f963503a
📒 Files selected for processing (1)
crates/biome_js_analyze/src/lint/nursery/no_arbitrary_value.rs
| /// ```jsx,use_options | ||
| /// <div className={clsx("w-[400px]")} />; | ||
| /// ``` |
There was a problem hiding this comment.
Mark the use_options example as expect_diagnostic.
With functions: ["clsx"] from the JSON options block above, clsx("w-[400px]") is exactly the case the rule is meant to catch — so the documented "valid"-looking example is actually a violation. The doc test will report a diagnostic, but the code block isn't tagged to expect one, leading to a snapshot mismatch and misleading documentation.
📝 Proposed tweak
- /// ```jsx,use_options
+ /// ```jsx,use_options,expect_diagnostic
/// <div className={clsx("w-[400px]")} />;
/// ```As per coding guidelines: "Invalid code examples in rule documentation must use the 'expect_diagnostic' code block property and emit exactly one diagnostic per snippet".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/biome_js_analyze/src/lint/nursery/no_arbitrary_value.rs` around lines
65 - 67, The JSX example under the use_options doc block is actually a violation
(clsx("w-[400px]") given functions: ["clsx"]) so update the code fence for that
example to include expect_diagnostic: change the triple-backtick tag from
"jsx,use_options" to "jsx,use_options,expect_diagnostic" so the doctest expects
one diagnostic for the clsx call in no_arbitrary_value.rs's documentation
example.
Still, the rule is definitely tailored for Tailwind (docs, business logic, tests) so it must be part of the tailwind domain, and it should be called Also, since there's no source rule to check, we would appreciate if you could explain the business logic of the code, so that we know how to review your contribution. Thank you! |
ematipico
removed
the
M-Likely Agent
|
The naming convention I'm using for other tailwind rules is |
|
No strong opinions 👍 |
dyc3
left a comment
dyc3
left a comment
There was a problem hiding this comment.
we should implement this for html as well.
| /// | ||
| pub NoArbitraryValue { | ||
| version: "next", | ||
| name: "noArbitraryValue", |
There was a problem hiding this comment.
| name: "noArbitraryValue", | |
| name: "noTailwindArbitraryValue", |
| pub NoArbitraryValue { | ||
| version: "next", | ||
| name: "noArbitraryValue", | ||
| language: "jsx", | ||
| recommended: false, | ||
| } |
There was a problem hiding this comment.
needs to go in the tailwind domain
| } | ||
|
|
||
| fn text_size(offset: usize) -> TextSize { | ||
| TextSize::from(u32::try_from(offset).unwrap()) |
| markup! { "Avoid arbitrary values in Tailwind CSS classes." }, | ||
| ) | ||
| .note(markup! { | ||
| "Arbitrary values bypass the design system. Use a design token instead." |
There was a problem hiding this comment.
Is "design token" the right wording for this? sounds kinda jargon-y
There was a problem hiding this comment.
I'd say it's both the right wording and kinda jargon-y, but gonna reword to make it more easily understood
THEjacob1000
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
crates/biome_rule_options/src/no_tailwind_arbitrary_value.rs (1)
49-73: Manual JsonSchema may drift fromUseSortedClassesOptions.The hand-rolled schema enumerates
attributesandfunctionsdirectly, so any future additions (or changes to validation) onUseSortedClassesOptionswon't be reflected here unless someone remembers to update both places. Not a blocker — just worth a comment near the inner struct so future changes don't silently desync.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_rule_options/src/no_tailwind_arbitrary_value.rs` around lines 49 - 73, Add a brief developer note next to the NoTailwindArbitraryValueOptions definition (or immediately above the impl schemars::JsonSchema / json_schema function) stating that the JSON schema is hand-written and mirrors the fields of UseSortedClassesOptions, and warning maintainers to update this schema whenever UseSortedClassesOptions changes to avoid drift; reference the symbols NoTailwindArbitraryValueOptions, impl schemars::JsonSchema, and UseSortedClassesOptions in the comment so future editors know the relationship and where to sync changes.crates/biome_html_analyze/src/lint/nursery/no_tailwind_arbitrary_value.rs (1)
139-219: Consider extracting the shared scanning helpers.
class_ranges,text_size,push_arbitrary_value_range,push_modifier_range, and the inner Tailwind-candidate match inarbitrary_rangesare byte-for-byte duplicates of the helpers incrates/biome_js_analyze/src/lint/nursery/no_tailwind_arbitrary_value.rs. Lifting them into a small shared module (e.g. underbiome_tailwind_parseror a newbiome_tailwind_lint_utilshelper) would keep both rules in sync if the Tailwind syntax tree evolves. Not a blocker for this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_html_analyze/src/lint/nursery/no_tailwind_arbitrary_value.rs` around lines 139 - 219, Duplicate Tailwind-scanning helpers exist here; extract them into a shared helper module and call them from this file. Create a new crate/module (e.g. biome_tailwind_lint_utils) that exports the helper functions class_ranges, text_size, push_arbitrary_value_range, push_modifier_range and a single helper that encapsulates the candidate-scanning logic used inside arbitrary_ranges (e.g. scan_tailwind_arbitrary_ranges or collect_arbitrary_ranges_from_parse) which performs the parse.tree().candidates() loop and the match over AnyTwFullCandidate/AnyTwCandidate; then replace the local definitions in this file with imports and call that shared helper from arbitrary_ranges, adjusting signatures to accept TokenText/text offsets and to return or push Vec<TextRange> as needed (refer to arbitrary_ranges, class_ranges, text_size, push_arbitrary_value_range, push_modifier_range, and the AnyTw* candidate types to wire parameters correctly).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/biome_html_analyze/src/lint/nursery/no_tailwind_arbitrary_value.rs`:
- Around line 44-62: Add a documentation example showing how to pass the
`attributes` option and a matching `use_options,expect_diagnostic` code snippet
for this rule (no_tailwind_arbitrary_value) so the options are testable; update
the doc comment in no_tailwind_arbitrary_value.rs to include a JSON options
block (if missing) and immediately below add an
html,use_options,expect_diagnostic fenced example that sets attributes to
["classList"] and includes a sample element (e.g. <div
classList="w-[400px]"></div>) that triggers the diagnostic, ensuring the default
value statement remains present.
- Around line 130-137: The current check in is_html_class_attribute uses
case-sensitive comparisons and will miss ASCII-case variants; update
is_html_class_attribute to use name.eq_ignore_ascii_case("class") and change the
attributes iteration to compare attribute.as_ref().eq_ignore_ascii_case(name).
Also update the similar lookup logic in HtmlAttributeList::find_by_name to
perform ASCII case-insensitive comparisons, and fix the case-sensitive "class"
and "style" checks in no_duplicate_attributes.rs (replace == "class"/== "style"
with eq_ignore_ascii_case) so attribute name matching follows the HTML ASCII
case-insensitive rule.
---
Nitpick comments:
In `@crates/biome_html_analyze/src/lint/nursery/no_tailwind_arbitrary_value.rs`:
- Around line 139-219: Duplicate Tailwind-scanning helpers exist here; extract
them into a shared helper module and call them from this file. Create a new
crate/module (e.g. biome_tailwind_lint_utils) that exports the helper functions
class_ranges, text_size, push_arbitrary_value_range, push_modifier_range and a
single helper that encapsulates the candidate-scanning logic used inside
arbitrary_ranges (e.g. scan_tailwind_arbitrary_ranges or
collect_arbitrary_ranges_from_parse) which performs the
parse.tree().candidates() loop and the match over
AnyTwFullCandidate/AnyTwCandidate; then replace the local definitions in this
file with imports and call that shared helper from arbitrary_ranges, adjusting
signatures to accept TokenText/text offsets and to return or push Vec<TextRange>
as needed (refer to arbitrary_ranges, class_ranges, text_size,
push_arbitrary_value_range, push_modifier_range, and the AnyTw* candidate types
to wire parameters correctly).
In `@crates/biome_rule_options/src/no_tailwind_arbitrary_value.rs`:
- Around line 49-73: Add a brief developer note next to the
NoTailwindArbitraryValueOptions definition (or immediately above the impl
schemars::JsonSchema / json_schema function) stating that the JSON schema is
hand-written and mirrors the fields of UseSortedClassesOptions, and warning
maintainers to update this schema whenever UseSortedClassesOptions changes to
avoid drift; reference the symbols NoTailwindArbitraryValueOptions, impl
schemars::JsonSchema, and UseSortedClassesOptions in the comment so future
editors know the relationship and where to sync changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 81b1aa6a-7ce6-4fd8-b00f-27d9911d48a8
⛔ Files ignored due to path filters (13)
Cargo.lockis excluded by!**/*.lockand included by**crates/biome_configuration/src/analyzer/linter/rules.rsis excluded by!**/rules.rsand included by**crates/biome_configuration/src/generated/domain_selector.rsis excluded by!**/generated/**,!**/generated/**and included by**crates/biome_configuration/src/generated/linter_options_check.rsis excluded by!**/generated/**,!**/generated/**and included by**crates/biome_diagnostics_categories/src/categories.rsis excluded by!**/categories.rsand included by**crates/biome_html_analyze/tests/specs/nursery/noTailwindArbitraryValue/invalid.html.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/nursery/noTailwindArbitraryValue/valid.html.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/noTailwindArbitraryValue/invalid.jsx.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/noTailwindArbitraryValue/invalid_functions.jsx.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/noTailwindArbitraryValue/valid.jsx.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/noTailwindArbitraryValue/valid_functions.jsx.snapis excluded by!**/*.snapand included by**packages/@biomejs/backend-jsonrpc/src/workspace.tsis excluded by!**/backend-jsonrpc/src/workspace.tsand included by**packages/@biomejs/biome/configuration_schema.jsonis excluded by!**/configuration_schema.jsonand included by**
📒 Files selected for processing (15)
.changeset/add-no-tailwind-arbitrary-value.mdcrates/biome_html_analyze/Cargo.tomlcrates/biome_html_analyze/src/lint/nursery/no_tailwind_arbitrary_value.rscrates/biome_html_analyze/tests/specs/nursery/noTailwindArbitraryValue/invalid.htmlcrates/biome_html_analyze/tests/specs/nursery/noTailwindArbitraryValue/invalid.options.jsoncrates/biome_html_analyze/tests/specs/nursery/noTailwindArbitraryValue/valid.htmlcrates/biome_js_analyze/src/lint/nursery/no_tailwind_arbitrary_value.rscrates/biome_js_analyze/tests/specs/nursery/noTailwindArbitraryValue/invalid.jsxcrates/biome_js_analyze/tests/specs/nursery/noTailwindArbitraryValue/invalid_functions.jsxcrates/biome_js_analyze/tests/specs/nursery/noTailwindArbitraryValue/invalid_functions.options.jsoncrates/biome_js_analyze/tests/specs/nursery/noTailwindArbitraryValue/valid.jsxcrates/biome_js_analyze/tests/specs/nursery/noTailwindArbitraryValue/valid_functions.jsxcrates/biome_js_analyze/tests/specs/nursery/noTailwindArbitraryValue/valid_functions.options.jsoncrates/biome_rule_options/src/lib.rscrates/biome_rule_options/src/no_tailwind_arbitrary_value.rs
✅ Files skipped from review due to trivial changes (9)
- crates/biome_html_analyze/tests/specs/nursery/noTailwindArbitraryValue/valid.html
- crates/biome_js_analyze/tests/specs/nursery/noTailwindArbitraryValue/valid_functions.options.json
- crates/biome_rule_options/src/lib.rs
- crates/biome_js_analyze/tests/specs/nursery/noTailwindArbitraryValue/invalid_functions.options.json
- crates/biome_js_analyze/tests/specs/nursery/noTailwindArbitraryValue/valid.jsx
- .changeset/add-no-tailwind-arbitrary-value.md
- crates/biome_js_analyze/tests/specs/nursery/noTailwindArbitraryValue/valid_functions.jsx
- crates/biome_html_analyze/tests/specs/nursery/noTailwindArbitraryValue/invalid.options.json
- crates/biome_html_analyze/tests/specs/nursery/noTailwindArbitraryValue/invalid.html
ah ok then, I saw it in the agents.md and couldn't find it anywhere in the contribution docs so thought I had to add it if I was using AI |
dyc3
left a comment
dyc3
left a comment
There was a problem hiding this comment.
the unrelated changes to other lint rules are technically valid, but they pollute what should be a rather clean PR.
| declare_lint_rule! { | ||
| /// Disallow arbitrary values in Tailwind CSS utility classes. | ||
| /// | ||
| /// Arbitrary values (e.g. `w-[400px]`, `text-[#555]`, `[color:red]`) bypass |
There was a problem hiding this comment.
[color:red] is technically an arbitrary candidate, not a value. But, its likely that users don't really care about that distinction. should we call the rule noTailwindArbitrary? I'd like to know what you think.
There was a problem hiding this comment.
I'd lean toward keeping noTailwindArbitraryValue to match the upstream eslint-plugin-tailwindcss/no-arbitrary-value source and because 'arbitrary value' is the term I believe most users reach for. Happy to clarify in the docs/diagnostic that it also covers arbitrary properties like [color:red]. Open to changing it if you feel strongly though.
There was a problem hiding this comment.
Let's make sure the docs mention that it covers arbitrary properties.
| /// | ||
| /// Controls which attributes and utility functions are checked for arbitrary values. | ||
| #[derive(Default, Clone, Debug, Eq, PartialEq)] | ||
| pub struct NoTailwindArbitraryValueOptions(UseSortedClassesOptions); |
There was a problem hiding this comment.
I'm not sure how I feel about reusing another rule's options like this. It's not something we normally do. cc @ematipico thoughts?
I think long term we'll have dedicated tailwind configuration that can be shared across these rules, but that's not something you necessarily need to worry about for this PR.
|
Also, there's prior art here: https://github.com/francoismassart/eslint-plugin-tailwindcss/blob/master/docs/rules/no-arbitrary-value.md So we should add the appropriate rule sources |
…evert unrelated changes in noTailwindArbitraryValue
- Add EslintTailwindcss variant to RuleSource (francoismassart/eslint-plugin-tailwindcss prior art)
- Add sources field to both HTML and JS noTailwindArbitraryValue rules
- Add contains('[') fast-path in scan_tailwind_arbitrary_ranges to skip parse on class strings with no brackets (fixes ~11% CodSpeed regression on Wikipedia HTML benchmarks)
- Revert unrelated eq_ignore_ascii_case changes to noDuplicateClasses, noDuplicateAttributes, noInlineStyles and their test fixtures
dyc3
left a comment
dyc3
left a comment
There was a problem hiding this comment.
I feel like your agent is trying to compensate too much for bugs in the parser to make the rule work.
also, this still needs an answer: #10094 (comment)
dyc3
left a comment
dyc3
left a comment
There was a problem hiding this comment.
Looking much better! Just a few more things I noticed.
| results.push(TextRange::new( | ||
| content_start + range.start(), | ||
| content_start + range.end(), | ||
| )); |
There was a problem hiding this comment.
| results.push(TextRange::new( | |
| content_start + range.start(), | |
| content_start + range.end(), | |
| )); | |
| results.push(range + content_start); |
| results.push(TextRange::new( | ||
| content_start + range.start(), | ||
| content_start + range.end(), | ||
| )); |
There was a problem hiding this comment.
| results.push(TextRange::new( | |
| content_start + range.start(), | |
| content_start + range.end(), | |
| )); | |
| results.push(range + content_start); |
| /// Collects text ranges of all arbitrary values in the given candidate list. | ||
| /// | ||
| /// `content_start` is the source offset of the first character of the parsed | ||
| /// string, used to translate parse-relative ranges into source ranges. |
There was a problem hiding this comment.
nit: mention this is the business logic of the rule.
| declare_lint_rule! { | ||
| /// Disallow arbitrary values in Tailwind CSS utility classes. | ||
| /// | ||
| /// Arbitrary values (e.g. `w-[400px]`, `text-[#555]`, `[color:red]`) bypass |
There was a problem hiding this comment.
Let's make sure the docs mention that it covers arbitrary properties.
| /// | ||
| /// Controls which attributes and utility functions are checked for arbitrary values. | ||
| #[derive(Default, Clone, Debug, Eq, PartialEq)] | ||
| pub struct NoTailwindArbitraryValueOptions(UseSortedClassesOptions); |
There was a problem hiding this comment.
After thinking about it for a while, the shared options is probably a footgun. Lets duplicate the options for now, even though they are the same.
After thinking about it |
| let sorted_options = UseSortedClassesOptions { | ||
| attributes: options.attributes.clone(), | ||
| functions: options.functions.clone(), | ||
| }; |
There was a problem hiding this comment.
nit: Don't use UseSortedClassesOptions at all
There was a problem hiding this comment.
introduced ClassStringOptions in any_class_string_like.rs to allow the rule to pass its own options directly.
| impl Deserializable for NoTailwindArbitraryValueOptions { | ||
| fn deserialize( | ||
| ctx: &mut impl DeserializationContext, | ||
| value: &impl DeserializableValue, | ||
| name: &str, | ||
| ) -> Option<Self> { | ||
| value.deserialize(ctx, NoTailwindArbitraryValueOptionsVisitor, name) | ||
| } | ||
| } |
| struct NoTailwindArbitraryValueOptionsVisitor; | ||
|
|
||
| impl DeserializationVisitor for NoTailwindArbitraryValueOptionsVisitor { | ||
| type Output = NoTailwindArbitraryValueOptions; | ||
|
|
||
| const EXPECTED_TYPE: DeserializableTypes = DeserializableTypes::MAP; | ||
|
|
||
| fn visit_map( |
There was a problem hiding this comment.
also, why did this snapshot change?
There was a problem hiding this comment.
The snapshot changed because of the text_trimmed() fix. Without it, calling .to_string() on a JsStaticMemberExpression tag includes surrounding whitespace trivia, so tw.div ends up as " tw.div" and misses the string comparison against the configured function names. The fix makes tw.div\...` get detected correctly by useSortedClasses. This new diagnostic is intentional.
There was a problem hiding this comment.
if necessary I'm happy to create an additional patch changeset describing this fix
There was a problem hiding this comment.
ah, ok. seems good to me, but maybe include another changeset for how it affects useSortedClasses
|
that and the changeset i mentioned #10094 (comment) |
|
Please avoid merging main to keep the branch up to date (unless you are resolving merge conflicts). we have to manually approve CI runs because you are a first time contributor. |
@dyc3 my bad I didn't know what the process was to get it merged after initial approval and thought I had to keep the branch up to date |
3 participants
Summary
Adds the nursery lint rule
noTailwindArbitraryValuefor Tailwind CSS class strings in JSX and HTML.The rule reports Tailwind arbitrary values such as
w-[400px],text-[#555], and[color:red]. It splits class strings on whitespace, parses each candidate with Biome's Tailwind parser, and reports arbitrary candidates plus arbitrary functional values/modifiers while allowing arbitrary variants like[&:nth-child(3)]:px-2.Related Issue: #6502
AI Usage
Test Plan
crates/biome_js_analyze/tests/specs/nursery/noTailwindArbitraryValue/crates/biome_html_analyze/tests/specs/nursery/noTailwindArbitraryValue/cargo run -p rules_checkcargo test -p biome_js_analyze no_tailwind_arbitrary_value --test spec_testscargo test -p biome_html_analyze no_tailwind_arbitrary_value --test spec_testsjust lDocs
Rule documentation is inline via rustdoc on the JSX and HTML rule implementations, including invalid/valid examples and options.