feat(analyze): add useTailwindShorthandClasses using biome_tailwind_parser#8503
feat(analyze): add useTailwindShorthandClasses using biome_tailwind_parser#8503dyc3 wants to merge 2 commits intodyc3/tailwind-analyzer-plumbingfrom
Conversation
🦋 Changeset detectedLatest commit: 16f6e79 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 |
1d1bda4 to
7e10b19
Compare
Merging this PR will not alter performance
Comparing Footnotes
|
7e10b19 to
a309815
Compare
0cccda6 to
de0c380
Compare
eeedf50 to
5a4523f
Compare
Parser conformance results onjs/262
jsx/babel
markdown/commonmark
symbols/microsoft
ts/babel
ts/microsoft
|
914f050 to
786f200
Compare
4db62da to
52040d7
Compare
WalkthroughAdds end-to-end Tailwind support: a new biome_tailwind_analyze crate with a UseTailwindShorthandClasses lint and tests; a Tailwind parser offset API; workspace/service/document/embedding plumbing to detect and host Tailwind snippets (HTML attributes, JSX/string args, Svelte/Vue directives); settings for Tailwind detection; rule options and suppression handling; and test/spec fixtures and CLI tests exercising automatic fixes. Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/biome_service/src/file_handlers/mod.rs (1)
1088-1133:⚠️ Potential issue | 🟠 MajorWire TailwindLanguage into the shared visitor infrastructure.
The Tailwind handler uses
AnalyzerVisitorBuilder, butRegistryVisitor<TailwindLanguage>implementations are missing fromSyntaxVisitor,LintVisitor, andAssistsVisitor. Additionally,biome_tailwind_analyze::visit_registry()is never called in the three registry-building sections. This means Tailwind rules incrates/biome_tailwind_analyze/src/lint/nursery/are silently unreachable—lint, fix, and code-action passes will all quietly skip Tailwind files.Add the missing visitor implementations (following the pattern of
CssLanguage,JsonLanguage, etc.) and callbiome_tailwind_analyze::visit_registry()in each of the three registry functions (lines ~1958–1962, ~2007–2011, ~2018–2022).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_service/src/file_handlers/mod.rs` around lines 1088 - 1133, The Tailwind analyzer was wired into Features (TailwindFileHandler) but missing registry visitor implementations and registry calls; add RegistryVisitor<TailwindLanguage> impls for SyntaxVisitor, LintVisitor, and AssistsVisitor following the same pattern used for CssLanguage/JsonLanguage (use AnalyzerVisitorBuilder with TailwindLanguage), and ensure you call biome_tailwind_analyze::visit_registry(&mut registry) inside each of the three registry-building functions that build the lint/fix/assists registries (the same locations where other languages call their visit_registry functions). This will register Tailwind rules from biome_tailwind_analyze so lint, fix, and code-action passes see Tailwind files.crates/biome_service/src/file_handlers/html.rs (1)
1377-1382:⚠️ Potential issue | 🟠 MajorDirective string snippets are keyed to a node
update_snippets()never rewrites.This stores
value.range(), but the write-back path only matchesAnyEmbeddedContent,HtmlAttribute, andHtmlTextExpression. Fixes or formatting for@click="...",:prop="...",v-if="...", etc. will therefore disappear. Key these snippets by the owning attribute, or add anHtmlAttributeInitializerClausebranch there.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_service/src/file_handlers/html.rs` around lines 1377 - 1382, The snippet is currently created with EmbeddedSnippet::new using value.range(), but update_snippets() only rewrites keys for AnyEmbeddedContent, HtmlAttribute, and HtmlTextExpression so directive attribute initializers (e.g. `@click`="...", :prop="...", v-if="...") are lost; fix by keying EmbeddedSnippet instances to the owning attribute (the HtmlAttribute node) instead of value.range(), or alternatively add a matching branch for HtmlAttributeInitializerClause in update_snippets() so it recognizes and rewrites snippets created from value.range(); update the creation site (EmbeddedSnippet::new call) and the update_snippets() matcher to use the same unique identifier (owner attribute node id or HtmlAttributeInitializerClause) so directive attribute snippets persist on write-back.
🧹 Nitpick comments (3)
crates/biome_tailwind_analyze/build.rs (1)
37-37: Prefer&Pathover&PathBuffor function parameters.Idiomatic Rust favours
&Pathsince&PathBufderefs to it anyway, and it's more flexible for callers.♻️ Suggested change
+use std::path::Path; + -fn touch_file(path: &PathBuf) -> io::Result<()> { +fn touch_file(path: &Path) -> io::Result<()> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_tailwind_analyze/build.rs` at line 37, The function signature for touch_file currently takes a &PathBuf which is less flexible; change the parameter to accept &Path instead (fn touch_file(path: &Path) -> io::Result<()>) and update any call sites to pass &path or path.as_path() as needed; inside touch_file, update any uses that required PathBuf methods to use Path-compatible methods or clone into a PathBuf only when necessary (refer to the touch_file symbol to locate and adjust the function and its callers).crates/biome_cli/tests/cases/tailwind.rs (1)
137-145: Add the same direct file assertion for the Svelte case.JS and HTML already prove the rewritten file before snapshotting; doing that here as well would make regressions fail faster and save everyone a round of snapshot archaeology.
Possible tweak
assert!(result.is_ok(), "run_cli returned {result:?}"); + assert_file_contents(&fs, svelte_file, r#"<div class={clsx("mx-2")}></div>"#); assert_cli_snapshot(SnapshotPayload::new( module_path!(), "should_apply_tailwind_shorthand_fixes_in_svelte",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_cli/tests/cases/tailwind.rs` around lines 137 - 145, The Svelte test lacks the direct file-content assertion present in the JS/HTML tests; after verifying result.is_ok() in the should_apply_tailwind_shorthand_fixes_in_svelte test, open the rewritten Svelte file from the test fs and assert its contents match the expected rewritten content (the same check used in the JS/HTML cases) before calling assert_cli_snapshot(SnapshotPayload::new(...)); this ensures a fast, explicit failure when the file rewrite is incorrect while preserving the existing snapshot call.crates/biome_tailwind_parser/src/lib.rs (1)
96-107: Skip the extra root clone in these conversions.Both impls call
syntax()and then immediately consumeparsefor diagnostics, so each conversion clones the root for no gain. A quick destructure keeps this path leaner.Possible tweak
impl From<TailwindParse> for AnyParse { fn from(parse: TailwindParse) -> Self { - let root = parse.syntax(); - let diagnostics = parse.into_diagnostics(); + let TailwindParse { root, diagnostics } = parse; NodeParse::new( // SAFETY: the parser should always return a root node root.as_send().unwrap(), diagnostics, ) @@ impl From<TailwindOffsetParse> for AnyParse { fn from(parse: TailwindOffsetParse) -> Self { - let root = parse.syntax(); - let diagnostics = parse.into_diagnostics(); + let TailwindOffsetParse { root, diagnostics } = parse; EmbeddedNodeParse::new( // SAFETY: the parser should always return a root node root.as_embedded_send(), diagnostics, )Also applies to: 162-173
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_tailwind_parser/src/lib.rs` around lines 96 - 107, The conversion currently calls parse.syntax() then consumes parse with parse.into_diagnostics(), causing an unnecessary clone of the root; change the From<TailwindParse> for AnyParse::from implementation to consume TailwindParse and extract both the root node and diagnostics in one move (destructure the TailwindParse value or use an API that returns both parts) and pass the owned root into NodeParse::new (instead of calling syntax() on a borrowed parse), and apply the same destructuring change to the other similar impl at the 162-173 range so neither conversion clones the root.
🤖 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_service/src/file_handlers/html.rs`:
- Around line 1788-1789: The update_snippets branch is using
html_string_literal(snippet.new_code.as_str()) which always emits double-quoted
literals and can break attributes that used single quotes; fix by making the
helper quote-aware or escaping inner quotes before wrapping: inspect the
original attribute quote style from value_token (or the node/attribute context)
and either call the existing html_string_literal_single_quotes() when the
original used single quotes or extend html_string_literal to accept a quote
parameter and pass the detected quote char, then call
mutation.replace_token(value_token, new_token) with the correctly quoted/escaped
new_token (or escape inner quotes in snippet.new_code when you must keep double
quotes). Ensure changes touch html_string_literal (and optionally add a
signature compatible with html_string_literal_single_quotes) and the
update_snippets branch where snippet.new_code is converted so attribute quoting
is preserved.
In `@crates/biome_service/src/file_handlers/tailwind.rs`:
- Around line 167-170: The ParserCapabilities block in tailwind.rs currently
leaves parse and parse_embedded_nodes as None, so downstream code cannot obtain
a TwRoot; replace those Nones with the Tailwind parser functions (e.g. set parse
= Some(parse_tailwind) and parse_embedded_nodes = Some(parse_tailwind_embedded)
or the actual parse_tailwind* helpers you have) where ParserCapabilities is
constructed so the handler can produce TwRoot for the lint/code-action/fix-all
paths and the new tailwind ruledoc source.
In `@crates/biome_service/src/settings.rs`:
- Line 733: The new tailwind field (LanguageSettings<TailwindLanguage>) was
added but OverrideSettings::override_analyzer_rules() still only merges
JS/JSON/CSS/GraphQL/HTML metadata so Tailwind overrides are ignored; update
OverrideSettings::override_analyzer_rules() to include Tailwind in the merge
logic and in any pattern-matching/enable/disable code paths (where
JS/JSON/CSS/GraphQL/HTML are handled) so that rules under languages.tailwind are
applied for matching paths, referencing the tailwind field and the
OverrideSettings::override_analyzer_rules() function to locate where to add the
merge/override handling.
In `@crates/biome_service/src/workspace/server.rs`:
- Around line 755-798: The current loop handling embedded_snippets calls
fix_file_with_embeds and then extends results.actions and uses
document_services, which is wrong because actions' ranges are in snippet-local
coordinates and snippet-specific services should be used; update the loop in
parse_embedded_language_snippets handling so that for each embedded_snippet you
(1) obtain the snippet-local services from the snippet (use
embedded_snippet.parse() / embedded_snippet.file_source_index() to find them)
and pass those services into fix_file_with_embeds instead of the outer
document_services, and (2) when merging results.actions into actions, map/offset
each action's range by embedded_snippet.content_offset() so the reported spans
are shifted into host file coordinates before extending actions (keep creating
UpdateSnippetsNodes with results.code as before).
In
`@crates/biome_tailwind_analyze/src/lint/nursery/use_tailwind_shorthand_classes.rs`:
- Around line 433-450: The current all-or-nothing logic for
effective_replacement_bases drops every replacement base if any one of them
already exists in the class list; change this to produce a filtered list of only
the missing bases: iterate replacement_bases and keep only those rb for which
candidates does NOT contain a TwFunctionalCandidate or TwStaticCandidate whose
base_token().text_trimmed() == rb, then use that filtered collection as
effective_replacement_bases (or set to &[] only if the filtered collection is
empty). Update the construction of effective_replacement_bases and, if needed,
the surrounding types so you can return either the filtered Vec/borrowed slice
or an empty slice while preserving the existing code paths that consume
effective_replacement_bases; reference replacement_bases,
effective_replacement_bases, candidates, AnyTwCandidate, TwFunctionalCandidate
and TwStaticCandidate when locating the change.
- Around line 309-313: The special-case branch that returns
Some(TailwindShorthandViolation { uncompressed_nodes: matched,
replacement_bases: &["truncate"], replace_whole_node: true }) must skip emitting
a replacement when the shorthand "truncate" is already present in the matched
nodes; add the same “shorthand already present” guard used by the generic path
to inspect matched (or uncompressed_nodes) for an existing "truncate" class and
return None (or fall through) instead of returning the violation, so you don't
produce "truncate truncate".
In `@crates/biome_tailwind_analyze/src/suppression_action.rs`:
- Around line 10-12: The default SuppressionAction inserts a comment trivia at
token start which breaks Tailwind syntax; add an override of
apply_top_level_suppression(&self, _node: &JsSyntaxNode, _suppression_text:
&str) -> Option<JsSyntaxToken> in the Tailwind implementation to be a no-op
(return None) similar to the existing apply_inline_suppression() override, and
keep suppression_top_level_comment() unchanged or unused; ensure the method
signature matches the SuppressionAction trait so the default behavior is
suppressed for Tailwind.
---
Outside diff comments:
In `@crates/biome_service/src/file_handlers/html.rs`:
- Around line 1377-1382: The snippet is currently created with
EmbeddedSnippet::new using value.range(), but update_snippets() only rewrites
keys for AnyEmbeddedContent, HtmlAttribute, and HtmlTextExpression so directive
attribute initializers (e.g. `@click`="...", :prop="...", v-if="...") are lost;
fix by keying EmbeddedSnippet instances to the owning attribute (the
HtmlAttribute node) instead of value.range(), or alternatively add a matching
branch for HtmlAttributeInitializerClause in update_snippets() so it recognizes
and rewrites snippets created from value.range(); update the creation site
(EmbeddedSnippet::new call) and the update_snippets() matcher to use the same
unique identifier (owner attribute node id or HtmlAttributeInitializerClause) so
directive attribute snippets persist on write-back.
In `@crates/biome_service/src/file_handlers/mod.rs`:
- Around line 1088-1133: The Tailwind analyzer was wired into Features
(TailwindFileHandler) but missing registry visitor implementations and registry
calls; add RegistryVisitor<TailwindLanguage> impls for SyntaxVisitor,
LintVisitor, and AssistsVisitor following the same pattern used for
CssLanguage/JsonLanguage (use AnalyzerVisitorBuilder with TailwindLanguage), and
ensure you call biome_tailwind_analyze::visit_registry(&mut registry) inside
each of the three registry-building functions that build the lint/fix/assists
registries (the same locations where other languages call their visit_registry
functions). This will register Tailwind rules from biome_tailwind_analyze so
lint, fix, and code-action passes see Tailwind files.
---
Nitpick comments:
In `@crates/biome_cli/tests/cases/tailwind.rs`:
- Around line 137-145: The Svelte test lacks the direct file-content assertion
present in the JS/HTML tests; after verifying result.is_ok() in the
should_apply_tailwind_shorthand_fixes_in_svelte test, open the rewritten Svelte
file from the test fs and assert its contents match the expected rewritten
content (the same check used in the JS/HTML cases) before calling
assert_cli_snapshot(SnapshotPayload::new(...)); this ensures a fast, explicit
failure when the file rewrite is incorrect while preserving the existing
snapshot call.
In `@crates/biome_tailwind_analyze/build.rs`:
- Line 37: The function signature for touch_file currently takes a &PathBuf
which is less flexible; change the parameter to accept &Path instead (fn
touch_file(path: &Path) -> io::Result<()>) and update any call sites to pass
&path or path.as_path() as needed; inside touch_file, update any uses that
required PathBuf methods to use Path-compatible methods or clone into a PathBuf
only when necessary (refer to the touch_file symbol to locate and adjust the
function and its callers).
In `@crates/biome_tailwind_parser/src/lib.rs`:
- Around line 96-107: The conversion currently calls parse.syntax() then
consumes parse with parse.into_diagnostics(), causing an unnecessary clone of
the root; change the From<TailwindParse> for AnyParse::from implementation to
consume TailwindParse and extract both the root node and diagnostics in one move
(destructure the TailwindParse value or use an API that returns both parts) and
pass the owned root into NodeParse::new (instead of calling syntax() on a
borrowed parse), and apply the same destructuring change to the other similar
impl at the 162-173 range so neither conversion clones the root.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9e7c6a67-f179-49fd-806c-693bafa9ecfc
⛔ Files ignored due to path filters (12)
Cargo.lockis excluded by!**/*.lockand included by**crates/biome_cli/src/execute/migrate/eslint_any_rule_to_biome.rsis excluded by!**/migrate/eslint_any_rule_to_biome.rsand included by**crates/biome_cli/tests/snapshots/main_cases_tailwind/should_apply_tailwind_shorthand_fixes_in_html.snapis excluded by!**/*.snapand included by**crates/biome_cli/tests/snapshots/main_cases_tailwind/should_apply_tailwind_shorthand_fixes_in_javascript.snapis excluded by!**/*.snapand included by**crates/biome_cli/tests/snapshots/main_cases_tailwind/should_apply_tailwind_shorthand_fixes_in_svelte.snapis excluded by!**/*.snapand included by**crates/biome_configuration/src/analyzer/linter/rules.rsis excluded by!**/rules.rsand included by**crates/biome_diagnostics_categories/src/categories.rsis excluded by!**/categories.rsand included by**crates/biome_tailwind_analyze/src/lint/nursery.rsis excluded by!**/nursery.rsand included by**crates/biome_tailwind_analyze/tests/specs/nursery/useTailwindShorthandClasses/invalid.json.snapis excluded by!**/*.snapand included by**crates/biome_tailwind_analyze/tests/specs/nursery/useTailwindShorthandClasses/valid.json.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 (35)
Cargo.tomlcrates/biome_cli/tests/cases/mod.rscrates/biome_cli/tests/cases/tailwind.rscrates/biome_configuration_macros/Cargo.tomlcrates/biome_configuration_macros/src/lib.rscrates/biome_configuration_macros/src/visitors.rscrates/biome_js_analyze/Cargo.tomlcrates/biome_rule_options/src/lib.rscrates/biome_rule_options/src/use_tailwind_shorthand_classes.rscrates/biome_ruledoc_utils/src/codeblock.rscrates/biome_service/Cargo.tomlcrates/biome_service/src/file_handlers/html.rscrates/biome_service/src/file_handlers/javascript.rscrates/biome_service/src/file_handlers/mod.rscrates/biome_service/src/file_handlers/tailwind.rscrates/biome_service/src/settings.rscrates/biome_service/src/workspace/document/mod.rscrates/biome_service/src/workspace/server.rscrates/biome_tailwind_analyze/Cargo.tomlcrates/biome_tailwind_analyze/build.rscrates/biome_tailwind_analyze/src/lib.rscrates/biome_tailwind_analyze/src/lint.rscrates/biome_tailwind_analyze/src/lint/nursery/use_tailwind_shorthand_classes.rscrates/biome_tailwind_analyze/src/registry.rscrates/biome_tailwind_analyze/src/suppression_action.rscrates/biome_tailwind_analyze/tests/spec_tests.rscrates/biome_tailwind_analyze/tests/specs/nursery/useTailwindShorthandClasses/invalid.jsoncrates/biome_tailwind_analyze/tests/specs/nursery/useTailwindShorthandClasses/valid.jsoncrates/biome_tailwind_parser/src/lib.rscrates/biome_tailwind_parser/src/parser.rsxtask/codegen/Cargo.tomlxtask/codegen/src/generate_configuration.rsxtask/codegen/src/generate_migrate_eslint.rsxtask/rules_check/Cargo.tomlxtask/rules_check/src/lib.rs
| pub html: LanguageSettings<HtmlLanguage>, | ||
| pub grit: LanguageSettings<GritLanguage>, | ||
| pub markdown: LanguageSettings<MarkdownLanguage>, | ||
| pub tailwind: LanguageSettings<TailwindLanguage>, |
There was a problem hiding this comment.
Tailwind rule overrides still are not wired in.
Adding languages.tailwind is only half the plumbing: OverrideSettings::override_analyzer_rules() later in this file still merges JS/JSON/CSS/GraphQL/HTML metadata only. Override blocks that try to enable or disable Tailwind rules for matching paths will therefore be ignored.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/biome_service/src/settings.rs` at line 733, The new tailwind field
(LanguageSettings<TailwindLanguage>) was added but
OverrideSettings::override_analyzer_rules() still only merges
JS/JSON/CSS/GraphQL/HTML metadata so Tailwind overrides are ignored; update
OverrideSettings::override_analyzer_rules() to include Tailwind in the merge
logic and in any pattern-matching/enable/disable code paths (where
JS/JSON/CSS/GraphQL/HTML are handled) so that rules under languages.tailwind are
applied for matching paths, referencing the tailwind field and the
OverrideSettings::override_analyzer_rules() function to locate where to add the
merge/override handling.
| if let Some(update_snippets) = capabilities.analyzer.update_snippets { | ||
| let exported_bindings = EmbeddedExportedBindings::default(); | ||
| let mut builder = exported_bindings.builder(); | ||
| let mut node_cache = NodeCache::default(); | ||
| let embedded_snippets = self.parse_embedded_language_snippets( | ||
| path, | ||
| &document_file_source, | ||
| &parse, | ||
| &mut node_cache, | ||
| settings, | ||
| &mut builder, | ||
| )?; | ||
|
|
||
| let mut new_snippets = vec![]; | ||
| for embedded_snippet in embedded_snippets { | ||
| let Some(file_source) = self.get_source(embedded_snippet.file_source_index()) | ||
| else { | ||
| continue; | ||
| }; | ||
|
|
||
| let results = self.fix_file_with_embeds( | ||
| embedded_snippet.parse(), | ||
| file_source, | ||
| document_services, | ||
| path, | ||
| fix_file_mode, | ||
| should_format, | ||
| only, | ||
| skip, | ||
| enabled_rules, | ||
| rule_categories, | ||
| suppression_reason.clone(), | ||
| settings, | ||
| plugins.clone(), | ||
| )?; | ||
|
|
||
| actions.extend(results.actions); | ||
| errors += results.errors; | ||
| skipped_suggested_fixes += results.skipped_suggested_fixes; | ||
|
|
||
| new_snippets.push(UpdateSnippetsNodes { | ||
| range: embedded_snippet.element_range(), | ||
| new_code: results.code, | ||
| }); |
There was a problem hiding this comment.
Shift embedded fixes back into host coordinates.
Lines 775-793 recurse into each snippet and then extend(results.actions) verbatim. Those ranges are relative to the snippet parse, so any applied fix inside an embedded class list or <script> block gets reported against the wrong span in the parent file. The same block also keeps passing the outer document_services, which drops any snippet-local services already stored on AnyEmbeddedSnippet.
Please offset the copied actions by embedded_snippet.content_offset() and thread the snippet’s own services through the recursive path instead of reusing the outer ones.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/biome_service/src/workspace/server.rs` around lines 755 - 798, The
current loop handling embedded_snippets calls fix_file_with_embeds and then
extends results.actions and uses document_services, which is wrong because
actions' ranges are in snippet-local coordinates and snippet-specific services
should be used; update the loop in parse_embedded_language_snippets handling so
that for each embedded_snippet you (1) obtain the snippet-local services from
the snippet (use embedded_snippet.parse() / embedded_snippet.file_source_index()
to find them) and pass those services into fix_file_with_embeds instead of the
outer document_services, and (2) when merging results.actions into actions,
map/offset each action's range by embedded_snippet.content_offset() so the
reported spans are shifted into host file coordinates before extending actions
(keep creating UpdateSnippetsNodes with results.code as before).
| Some(TailwindShorthandViolation { | ||
| uncompressed_nodes: matched, | ||
| replacement_bases: &["truncate"], | ||
| replace_whole_node: true, | ||
| }) |
There was a problem hiding this comment.
Don’t add a second truncate.
This special-case always emits replacement_bases: &["truncate"], so truncate overflow-hidden text-ellipsis whitespace-nowrap fixes to truncate truncate. It needs the same “shorthand already present” guard as the generic path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@crates/biome_tailwind_analyze/src/lint/nursery/use_tailwind_shorthand_classes.rs`
around lines 309 - 313, The special-case branch that returns
Some(TailwindShorthandViolation { uncompressed_nodes: matched,
replacement_bases: &["truncate"], replace_whole_node: true }) must skip emitting
a replacement when the shorthand "truncate" is already present in the matched
nodes; add the same “shorthand already present” guard used by the generic path
to inspect matched (or uncompressed_nodes) for an existing "truncate" class and
return None (or fall through) instead of returning the violation, so you don't
produce "truncate truncate".
| // If the shorthand is already present in the class list, just | ||
| // remove the longhands rather than adding a duplicate shorthand. | ||
| let effective_replacement_bases: &[&str] = if replacement_bases.iter().any(|&rb| { | ||
| candidates.iter().any(|c| match c.candidate().ok() { | ||
| Some(AnyTwCandidate::TwFunctionalCandidate(func)) => func | ||
| .base_token() | ||
| .ok() | ||
| .is_some_and(|t| t.text_trimmed() == rb), | ||
| Some(AnyTwCandidate::TwStaticCandidate(st)) => { | ||
| st.base_token().ok().is_some_and(|t| t.text_trimmed() == rb) | ||
| } | ||
| _ => false, | ||
| }) | ||
| }) { | ||
| &[] | ||
| } else { | ||
| replacement_bases | ||
| }; |
There was a problem hiding this comment.
Only drop replacement bases that already exist.
This is all-or-nothing today. With scale-50 scale-x-50 scale-y-50 scale-z-50, the fixer removes the longhands and leaves only scale-50, because the missing scale-3d-* half never gets added. This wants a filtered list of missing replacement bases, not [] for the whole pattern.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@crates/biome_tailwind_analyze/src/lint/nursery/use_tailwind_shorthand_classes.rs`
around lines 433 - 450, The current all-or-nothing logic for
effective_replacement_bases drops every replacement base if any one of them
already exists in the class list; change this to produce a filtered list of only
the missing bases: iterate replacement_bases and keep only those rb for which
candidates does NOT contain a TwFunctionalCandidate or TwStaticCandidate whose
base_token().text_trimmed() == rb, then use that filtered collection as
effective_replacement_bases (or set to &[] only if the filtered collection is
empty). Update the construction of effective_replacement_bases and, if needed,
the surrounding types so you can return either the filtered Vec/borrowed slice
or an empty slice while preserving the existing code paths that consume
effective_replacement_bases; reference replacement_bases,
effective_replacement_bases, candidates, AnyTwCandidate, TwFunctionalCandidate
and TwStaticCandidate when locating the change.
| fn suppression_top_level_comment(&self, suppression_text: &str) -> String { | ||
| // Tailwind class strings have no comment syntax; this should never be called. | ||
| suppression_text.to_string() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd crates/biome_tailwind_analyze && wc -l src/suppression_action.rsRepository: biomejs/biome
Length of output: 85
🏁 Script executed:
cat -n crates/biome_tailwind_analyze/src/suppression_action.rsRepository: biomejs/biome
Length of output: 1373
🏁 Script executed:
# Search for SuppressionAction trait definition
rg "trait SuppressionAction" --type rs -A 10Repository: biomejs/biome
Length of output: 83
🏁 Script executed:
# Search for calls to suppression_top_level_comment
rg "suppression_top_level_comment" --type rs -B 3 -A 3Repository: biomejs/biome
Length of output: 83
🏁 Script executed:
# Search for SuppressionAction trait definition without file type flag
rg "trait SuppressionAction" -A 15Repository: biomejs/biome
Length of output: 1463
🏁 Script executed:
# Search for calls to suppression_top_level_comment in entire codebase
rg "suppression_top_level_comment" -B 2 -A 2Repository: biomejs/biome
Length of output: 4798
🏁 Script executed:
# Look for any tests in the tailwind_analyze crate
find crates/biome_tailwind_analyze -name "*test*" -o -name "*tests.rs"Repository: biomejs/biome
Length of output: 142
🏁 Script executed:
# Find apply_top_level_suppression implementation
rg "apply_top_level_suppression" -B 5 -A 10Repository: biomejs/biome
Length of output: 5534
🏁 Script executed:
# Check the trait definition in detail
cat -n crates/biome_analyze/src/suppression_action.rs | head -100Repository: biomejs/biome
Length of output: 4316
🏁 Script executed:
# Check if Tailwind implements apply_top_level_suppression
cat -n crates/biome_tailwind_analyze/src/suppression_action.rsRepository: biomejs/biome
Length of output: 1373
🏁 Script executed:
# Get the full apply_top_level_suppression implementation in the trait
cat -n crates/biome_analyze/src/suppression_action.rs | sed -n '87,120p'Repository: biomejs/biome
Length of output: 1636
🏁 Script executed:
# Check where the suppression is triggered from
cat -n crates/biome_analyze/src/rule.rs | grep -A 20 "suppression_top_level_comment"Repository: biomejs/biome
Length of output: 965
🏁 Script executed:
# Search for Tailwind rules and when they might trigger suppression
rg "TailwindSuppressionAction" -B 3 -A 3Repository: biomejs/biome
Length of output: 1991
🏁 Script executed:
# Check if there's any guard logic that prevents top-level suppression for Tailwind
rg "Tailwind.*suppress|suppress.*Tailwind" -iRepository: biomejs/biome
Length of output: 590
🏁 Script executed:
# Verify if any other language implements apply_top_level_suppression override
rg "fn apply_top_level_suppression" -B 2 -A 5Repository: biomejs/biome
Length of output: 2152
🏁 Script executed:
# Check if any Tailwind rules actually exist that would trigger suppression
find crates/biome_tailwind_analyze -name "*.rs" -type f | head -20Repository: biomejs/biome
Length of output: 461
🏁 Script executed:
# Look at Tailwind rules registry
cat -n crates/biome_tailwind_analyze/src/registry.rs 2>/dev/null | head -50Repository: biomejs/biome
Length of output: 391
🏁 Script executed:
# Check the lint nursery rules
cat -n crates/biome_tailwind_analyze/src/lint/nursery.rs | head -50Repository: biomejs/biome
Length of output: 578
🏁 Script executed:
# Look at the actual rule implementation
cat -n crates/biome_tailwind_analyze/src/lint/nursery/use_tailwind_shorthand_classes.rs | head -100Repository: biomejs/biome
Length of output: 4235
🏁 Script executed:
# Search for TailwindRuleAction definition
rg "TailwindRuleAction" -B 5 -A 15Repository: biomejs/biome
Length of output: 9705
🏁 Script executed:
# Check if suppressions are even properly integrated for Tailwind rules
rg "suppress.*Tailwind\|Tailwind.*suppress" -i -B 2 -A 2Repository: biomejs/biome
Length of output: 39
🏁 Script executed:
# Look for any tests that exercise suppression on Tailwind rules
find crates/biome_tailwind_analyze/tests -type f -exec grep -l "suppress" {} \;Repository: biomejs/biome
Length of output: 106
🏁 Script executed:
# Look more closely at what the default apply_top_level_suppression does with the suppression_text
cat -n crates/biome_analyze/src/suppression_action.rs | sed -n '87,140p'Repository: biomejs/biome
Length of output: 2449
🏁 Script executed:
# Check if any integration or e2e tests exercise Tailwind suppression
find crates/biome_tailwind_analyze -type f -name "*.rs" -exec grep -l "suppress" {} \;Repository: biomejs/biome
Length of output: 203
🏁 Script executed:
# Check if spec_tests.rs mentions suppression
grep -n "suppress" crates/biome_tailwind_analyze/tests/spec_tests.rsRepository: biomejs/biome
Length of output: 541
🏁 Script executed:
# Look at lib.rs to see if there's any guard for top-level suppression
grep -B 10 -A 10 "apply_top_level_suppression" crates/biome_tailwind_analyze/src/lib.rsRepository: biomejs/biome
Length of output: 39
🏁 Script executed:
# Check the new_trivia_for_top_suppression function to understand what happens
cat -n crates/biome_analyze/src/suppression_action.rs | sed -n '145,180p'Repository: biomejs/biome
Length of output: 1226
Override apply_top_level_suppression() as a no-op for Tailwind.
The default SuppressionAction implementation inserts a comment trivia at the token start. Since Tailwind has no comment syntax, this will corrupt the syntax tree. Add a no-op override matching the pattern already used for apply_inline_suppression().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/biome_tailwind_analyze/src/suppression_action.rs` around lines 10 -
12, The default SuppressionAction inserts a comment trivia at token start which
breaks Tailwind syntax; add an override of apply_top_level_suppression(&self,
_node: &JsSyntaxNode, _suppression_text: &str) -> Option<JsSyntaxToken> in the
Tailwind implementation to be a no-op (return None) similar to the existing
apply_inline_suppression() override, and keep suppression_top_level_comment()
unchanged or unused; ensure the method signature matches the SuppressionAction
trait so the default behavior is suppressed for Tailwind.
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
ematipico
left a comment
There was a problem hiding this comment.
Left some comments, I believe the architecture isn't there yet. The tailwind case is different because there isn't a .tw file, and we're trying to fit it into an architecture that wasn't meant for it, so I believe we should explore something new.
My initial plan was to use the tailwind parser inside the analyzer. We parse the string from a JSX attribute and apply the rule logic. This way, we didn't need to bring the workspace into all of this.
This PR tries to make it an embedded language, which I understand from one point of view, but the architecture feels clunky.
I suggest the following:
- use the tw parser inside the rule (it's freaking fast, I don't think we will see any regression) as I originally envisioned
- Roll back the workspace changes
- Then explore how we can embed TW snippets in a fancy manner. I am actually working on a refactor regarding embeds, and maybe that could help.
- Explore a new testing infrastructure where we can use the workpsace in the analyzer tests, so that we can parse embedded langauges too, so we don't pollute the CLI anymore (this can be done separately)
| type FormatterSettings = (); | ||
| type LinterSettings = TailwindLinterSettings; | ||
| type AssistSettings = (); | ||
| type FormatOptions = TailwindFormatOptions; |
There was a problem hiding this comment.
Why do we need formatting options?
| ) -> Option<ApplySuppression<Self::Language>> { | ||
| // Tailwind class strings have no comment syntax, so inline suppressions | ||
| // are not supported. | ||
| None |
There was a problem hiding this comment.
This is false. We now have a lint rule that is under a different crate/registry. biome_tailwind_analyze provides its own rules, which means that analyser signals are emitted from that crate, hence it needs to provide the information needed for suppression.
| fn parse_linter_suppression_comment( | ||
| text: &str, | ||
| piece_range: TextRange, | ||
| ) -> Vec<Result<AnalyzerSuppression<'_>, SuppressionDiagnostic>> { | ||
| let mut result = Vec::new(); | ||
|
|
||
| for suppression in parse_suppression_comment(text) { |
There was a problem hiding this comment.
The fact that you implemented this function goes in constrast with the the non-implementation of the suppression comments
| expression: valid.json | ||
| --- | ||
| # Input | ||
| ```tw |
There was a problem hiding this comment.
tw doesn't exist. Let's fallback to txt or nothing
| fn fix_file_with_embeds<'a>( | ||
| &self, | ||
| mut parse: AnyParse, | ||
| document_file_source: DocumentFileSource, | ||
| document_services: &'a DocumentServices, | ||
| path: &'a BiomePath, | ||
| fix_file_mode: FixFileMode, | ||
| should_format: bool, | ||
| only: &'a [AnalyzerSelector], | ||
| skip: &'a [AnalyzerSelector], | ||
| enabled_rules: &'a [AnalyzerSelector], | ||
| rule_categories: RuleCategories, | ||
| suppression_reason: Option<String>, | ||
| settings: &'a SettingsWithEditor<'a>, | ||
| plugins: AnalyzerPluginVec, |
There was a problem hiding this comment.
We really should create a struct
| let results = self.fix_file_with_embeds( | ||
| embedded_snippet.parse(), | ||
| file_source, | ||
| document_services, | ||
| path, | ||
| fix_file_mode, | ||
| should_format, | ||
| only, | ||
| skip, | ||
| enabled_rules, | ||
| rule_categories, | ||
| suppression_reason.clone(), | ||
| settings, | ||
| plugins.clone(), | ||
| )?; |
There was a problem hiding this comment.
isnt' this a recursion? can you explain this code to me please?
| @@ -726,6 +725,182 @@ impl WorkspaceServer { | |||
| Ok(embedded_nodes) | |||
| } | |||
|
|
|||
| #[expect(clippy::too_many_arguments)] | |||
| fn fix_file_with_embeds<'a>( | |||
There was a problem hiding this comment.
This function really needs some docstrings. It's very long, and it now does more things than before, which I can't understand much...
| DocumentFileSource::Js(_) => { | ||
| let node = new_root.into_node::<JsLanguage>().unwrap(); | ||
| let current = parse | ||
| .unwrap_as_embedded_syntax_node() | ||
| .into_node::<JsLanguage>(); | ||
| parse.set_new_embedded_root( | ||
| SyntaxNodeWithOffset::new(node, current.base_offset()) | ||
| .as_embedded_send(), | ||
| ); | ||
| } |
There was a problem hiding this comment.
I really don't understand what we're doing here. I think I know, but I would like an explanation. It doesn't feel like the right architecture imho
c619cb1 to
3bb337f
Compare
68cad83 to
16f6e79
Compare
676ceb4 to
67de7ea
Compare
Summary
This ports
enforce-shorthand-classesfromeslint-plugin-better-tailwindcss. I did this mainly to explore what it would look like to start implementing some of these rules.It's marked as inspired because this implementation is a fair bit more intelligent than the source rule, thanks to using the tailwind parser making it really easy to extract the desired info.
TODO:
I've iterated enough that I'm satisfied with the state of the rule now.
Another thing this PR does is set up(moved to #9624) An important part of that is how we define which parts of the code are tailwind (as in which attributes/function params). This will need to become configurable by users, but I chose not to expose the configuration for now since its a breaking change, and should be done in a minor. The goal is to start gathering feedback on the parser from users before we release the next minor.biome_tailwind_analyze, and treating tailwind as an embedded language.This paves the way to implement all the other tailwind rules from
eslint-plugin-better-tailwindcss, and port our useSortedClasses to use the tailwind parser instead.Test Plan
AI wrote a bunch of unit tests, and I wrote some snapshot tests.
Also did some CLI tests for making sure the embedded language part works.
Docs