feat(lint): add useClassNameHelper nursery rule#9064
feat(lint): add useClassNameHelper nursery rule#9064PaulRBerg wants to merge 2 commits intobiomejs:nextfrom
Conversation
- Add useClassNameHelper nursery rule with configurable attributes and helper names - Flag interpolated template literals in class attributes, including nested expressions - Add fixtures/snapshots and changeset linked to discussion biomejs#8652
🦋 Changeset detectedLatest commit: a06ce5c The changes in this PR will be included in the next version bump. This PR includes changesets to release 14 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 |
WalkthroughThis PR introduces a new lint rule Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@crates/biome_js_analyze/src/lint/nursery/use_class_name_helper.rs`:
- Around line 121-133: The helper find_interpolated_template currently searches
the entire subtree (using expression.syntax().descendants()) which can produce
false positives on deeply nested template literals; restrict the search to only
the immediate/semantically relevant positions instead of all descendants — for
example limit traversal depth or only inspect direct value positions such as
call arguments, JSX expression root, ternary branches, or binary/conditional
operands; modify find_interpolated_template (and its use of
JsTemplateExpression::cast and descendants) to walk only those targeted children
of the initial expression (or stop after a fixed depth) so templates inside
unrelated nested constructs (e.g., computed property accesses) are not flagged.
In `@crates/biome_rule_options/src/use_class_name_helper.rs`:
- Around line 20-25: has_attribute currently treats Some([]) as disabling the
rule because as_deref().map_or_else only uses DEFAULT_ATTRIBUTES when attributes
is None; update has_attribute so an empty attributes list falls back to
DEFAULT_ATTRIBUTES (or explicitly document otherwise). Concretely, modify the
logic in has_attribute (and keep using DEFAULT_ATTRIBUTES and contains_name) to
check if attributes.is_empty() and in that case call
DEFAULT_ATTRIBUTES.contains(&name), otherwise call contains_name(attributes,
name).
🧹 Nitpick comments (2)
.changeset/use-class-name-helper-rule.md (1)
5-5: Add an inline example of an invalid case to the changeset.Per project conventions, changeset descriptions for new lint rules should include an example of the code pattern the rule flags. Something like:
`<div className={`px-4 ${isActive ? "a" : "b"}`} />`would help end users quickly understand what the rule catches. Based on learnings: "For new lint rules in changesets, show an example of invalid case in inline code or code block."
crates/biome_js_analyze/src/lint/nursery/use_class_name_helper.rs (1)
109-119:get_attribute_nameonly handles simple JSX names, not namespaced ones.
as_jsx_name()won't match namespaced attributes likearia-classordata-class. This is fine for the current defaults (className,class), but if a user configures a namespaced attribute name it would silently never match. A small doc comment or a note in the Options section would save future confusion.
| fn find_interpolated_template(expression: &AnyJsExpression) -> Option<JsTemplateExpression> { | ||
| if let AnyJsExpression::JsTemplateExpression(template) = expression | ||
| && !template.is_constant() | ||
| { | ||
| return Some(template.clone()); | ||
| } | ||
|
|
||
| expression | ||
| .syntax() | ||
| .descendants() | ||
| .filter_map(JsTemplateExpression::cast) | ||
| .find(|template| !template.is_constant()) | ||
| } |
There was a problem hiding this comment.
descendants() may produce false positives on deeply nested, unrelated templates.
find_interpolated_template searches the entire subtree of the attribute expression. This means a template literal nested inside, say, a computed property access or a callback will also trigger a diagnostic:
<div className={getClass(obj[`key_${i}`])} />
// flagged, but the template is for property access, not class constructionFor a nursery rule this is likely acceptable, but worth noting for promotion. If you'd like to tighten it, you could limit the traversal depth or only walk "direct value" positions (call arguments, ternary branches, etc.).
🤖 Prompt for AI Agents
In `@crates/biome_js_analyze/src/lint/nursery/use_class_name_helper.rs` around
lines 121 - 133, The helper find_interpolated_template currently searches the
entire subtree (using expression.syntax().descendants()) which can produce false
positives on deeply nested template literals; restrict the search to only the
immediate/semantically relevant positions instead of all descendants — for
example limit traversal depth or only inspect direct value positions such as
call arguments, JSX expression root, ternary branches, or binary/conditional
operands; modify find_interpolated_template (and its use of
JsTemplateExpression::cast and descendants) to walk only those targeted children
of the initial expression (or stop after a fixed depth) so templates inside
unrelated nested constructs (e.g., computed property accesses) are not flagged.
| pub fn has_attribute(&self, name: &str) -> bool { | ||
| self.attributes.as_deref().map_or_else( | ||
| || DEFAULT_ATTRIBUTES.contains(&name), | ||
| |attributes| contains_name(attributes, name), | ||
| ) | ||
| } |
There was a problem hiding this comment.
Empty attributes array silently disables the rule.
If a user sets "attributes": [], has_attribute will never match, effectively making the rule a no-op. This differs from the helper_functions path which explicitly falls back to defaults on empty. Consider whether they should behave consistently — either both fall back to defaults on empty, or document that an empty attributes array disables the rule.
🤖 Prompt for AI Agents
In `@crates/biome_rule_options/src/use_class_name_helper.rs` around lines 20 - 25,
has_attribute currently treats Some([]) as disabling the rule because
as_deref().map_or_else only uses DEFAULT_ATTRIBUTES when attributes is None;
update has_attribute so an empty attributes list falls back to
DEFAULT_ATTRIBUTES (or explicitly document otherwise). Concretely, modify the
logic in has_attribute (and keep using DEFAULT_ATTRIBUTES and contains_name) to
check if attributes.is_empty() and in that case call
DEFAULT_ATTRIBUTES.contains(&name), otherwise call contains_name(attributes,
name).
Merging this PR will not alter performance
Comparing Footnotes
|
Summary
useClassNameHelper, to flag interpolated template literals in class attributes.attributes/helperFunctionsoptions, test fixtures and snapshots, and a changeset that references discussion #8652.Test Plan
INSTA_UPDATE=always cargo test -p biome_js_analyze --test spec_tests use_class_name_helperjust gen-rulesjust gen-configurationjust fjust lDocs