feat(lint): add nursery rule useIncludes#9876
Conversation
Port prefer-includes from typescript-eslint. The rule suggests using .includes() over .indexOf() comparisons when checking for the presence of a value in a string or array.
🦋 Changeset detectedLatest commit: 12409fd 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-Linter
WalkthroughThis pull request introduces the 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.
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/use_includes.rs`:
- Line 65: The RuleSource entry currently uses
RuleSource::EslintTypeScript("prefer-includes") without the parity marker;
update the rule definition to use
RuleSource::EslintTypeScript("prefer-includes").same() so it signals identical
behavior to the upstream rule (modify the occurrence of
RuleSource::EslintTypeScript("prefer-includes") in the use_includes rule).
🪄 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: 206da4c5-9ad0-47ec-8ff8-f8a1ca4c2469
📒 Files selected for processing (6)
.changeset/add-use-includes.mdcrates/biome_js_analyze/src/lint/nursery/use_includes.rscrates/biome_js_analyze/tests/specs/nursery/useIncludes/invalid.jscrates/biome_js_analyze/tests/specs/nursery/useIncludes/valid.jscrates/biome_rule_options/src/lib.rscrates/biome_rule_options/src/use_includes.rs
| name: "useIncludes", | ||
| language: "js", | ||
| recommended: false, | ||
| sources: &[RuleSource::EslintTypeScript("prefer-includes")], |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect how existing ESLint / typescript-eslint ports encode source parity.
sed -n '60,67p' crates/biome_js_analyze/src/lint/nursery/use_includes.rs
rg -n -C2 'RuleSource::Eslint(TypeScript)?\(' crates/biome_js_analyze/src/lint --glob '*.rs'Repository: biomejs/biome
Length of output: 1768
🏁 Script executed:
#!/bin/bash
# View the current rule metadata at lines 60-67
echo "=== Current rule metadata (lines 60-67) ==="
sed -n '60,67p' crates/biome_js_analyze/src/lint/nursery/use_includes.rs
# Search for examples of .same() usage with ESLint sources
echo -e "\n=== Examples of .same() usage with ESLint sources ==="
rg -A1 'RuleSource::Eslint.*\.same\(' crates/biome_js_analyze/src/lint --max-count 10
# Search for other Eslint*TypeScript sources to see the pattern
echo -e "\n=== Examples of EslintTypeScript sources ==="
rg -A1 'RuleSource::EslintTypeScript' crates/biome_js_analyze/src/lint --max-count 5Repository: biomejs/biome
Length of output: 50369
Add .same() to indicate matching behaviour with the upstream rule.
The rule currently references RuleSource::EslintTypeScript("prefer-includes") without a parity marker. Based on the consistently applied pattern across the codebase (and confirmed in similar rules like use_find, use_namespace_keyword, and use_regexp_exec), add .same() to signal that this is a faithful port:
Diff
- sources: &[RuleSource::EslintTypeScript("prefer-includes")],
+ sources: &[RuleSource::EslintTypeScript("prefer-includes").same()],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sources: &[RuleSource::EslintTypeScript("prefer-includes")], | |
| sources: &[RuleSource::EslintTypeScript("prefer-includes").same()], |
🤖 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/use_includes.rs` at line 65, The
RuleSource entry currently uses RuleSource::EslintTypeScript("prefer-includes")
without the parity marker; update the rule definition to use
RuleSource::EslintTypeScript("prefer-includes").same() so it signals identical
behavior to the upstream rule (modify the occurrence of
RuleSource::EslintTypeScript("prefer-includes") in the use_includes rule).
|
Good to know that eslint-plugin-unicorn provides a more powerful version: https://github.com/sindresorhus/eslint-plugin-unicorn/blob/HEAD/docs/rules/prefer-includes.md, which I think is more worth following. Also there was another PR in progress #8115, but became a bit stale I think |
ematipico
left a comment
ematipico
left a comment
There was a problem hiding this comment.
Thank you for the contribution!
- The code can be improved; I left some suggestions.
- The diagnostics must follow our guidelines. We're very strict, please refer to the contribution guidelines of the analyzer
- I believe you haven't run any tests, so the testing plan is a bit moot 😅 Please run the tests, inspect the snapshots and commit them
- The current implementation of the rule differs from the source rule. Was that intentional?
| name: "useIncludes", | ||
| language: "js", | ||
| recommended: false, | ||
| sources: &[RuleSource::EslintTypeScript("prefer-includes")], |
There was a problem hiding this comment.
| sources: &[RuleSource::EslintTypeScript("prefer-includes")], | |
| sources: &[RuleSource::EslintTypeScript("prefer-includes").inspired()], |
I chose inspired because this rule you implemented isn't the same as the eslint one. The one from eslint uses type information, while yours is simply using AST, which means it won't yield the same results.
By being aware of this, what's your plan?
| let mut current = node.parent()?; | ||
| loop { | ||
| if let Some(binary) = JsBinaryExpression::cast(current.clone()) { | ||
| return Some(binary); | ||
| } | ||
| if biome_js_syntax::JsParenthesizedExpression::can_cast(current.kind()) { | ||
| current = current.parent()?; | ||
| continue; | ||
| } | ||
| return None; | ||
| } |
There was a problem hiding this comment.
This function can be simplified:
- As a golden rule, never pass untyped nodes e.g.
JsSyntaxNode - Second, there's a method called
ancestors()that does what you need - You can use
find_mapto find the first occurrence you need - For parentheses and such, here's what I suggest you to do
It's a bit simplified, but hopefully you understand what I mean
let expression = AnyJsExpression:cast(node)?; let expression = expression.omit_parenthesis()?; if let Some(binary) = expression.as_js_binary_expression() { return Some(binary) }
|
|
||
| fn expression_matches_target(expression: &AnyJsExpression, target: &JsSyntaxNode) -> bool { | ||
| expression.clone().omit_parentheses().syntax().eq(target) | ||
| } |
There was a problem hiding this comment.
Can you please add docstrings? It's very difficult to understand what kind of comparison you're looking for. Most of the time, this comparison isn't the intended one.
Plus target is untyped, so it's more difficult to understand what the intended behaviour is.
What I suggest:
- add doc strings to explain what we're comparing (business logic)
targetshould be typed, so that you signal the intention
After I understand the intention, we can also properly address the equality operation
| expression.clone().omit_parentheses().syntax().eq(target) | ||
| } | ||
|
|
||
| fn is_number_literal_value(expression: &AnyJsExpression, value: f64) -> bool { |
There was a problem hiding this comment.
| fn is_number_literal_value(expression: &AnyJsExpression, value: f64) -> bool { | |
| fn matches_number_literal_value(expression: &AnyJsExpression, value: f64) -> bool { |
| .is_some_and(|number| number == value) | ||
| } | ||
|
|
||
| fn is_negative_one_literal(expression: &AnyJsExpression) -> bool { |
There was a problem hiding this comment.
Docs string that explains the business logic. There are some magic numbers that require explanation
| let unary = match expression.as_js_unary_expression() { | ||
| Some(unary) => unary, | ||
| None => return false, | ||
| }; |
There was a problem hiding this comment.
| let unary = match expression.as_js_unary_expression() { | |
| Some(unary) => unary, | |
| None => return false, | |
| }; | |
| let Some(unary) = expression.as_js_unary_expression() else { | |
| return false, | |
| }; |
| } | ||
| } | ||
|
|
||
| fn call_method_name(call: &JsCallExpression) -> Option<String> { |
There was a problem hiding this comment.
| fn call_method_name(call: &JsCallExpression) -> Option<String> { | |
| fn call_method_name(call: &JsCallExpression) -> Option<TokenText> { |
| let member = callee.as_js_static_member_expression()?; | ||
| let member_name = member.member().ok()?; | ||
| let name = member_name.as_js_name()?; | ||
| Some(name.value_token().ok()?.text_trimmed().to_string()) |
There was a problem hiding this comment.
| Some(name.value_token().ok()?.text_trimmed().to_string()) | |
| Some(name.value_token().ok()?.text_trimmed()) |
There was a problem hiding this comment.
Where are the snapshots? Did you run the tests? 😅
| markup! { | ||
| "Use "<Emphasis>".includes()"</Emphasis>" rather than "<Emphasis>".indexOf()"</Emphasis>" to check for the presence of a value." | ||
| }, | ||
| ) | ||
| .note(markup! { | ||
| <Emphasis>".includes()"</Emphasis>" is more readable and expressive than comparing "<Emphasis>".indexOf()"</Emphasis>" against "<Emphasis>"-1"</Emphasis>"." | ||
| }), |
There was a problem hiding this comment.
Our contribution guidelines for messages and diagnostics are quite strict and follow a structured framework. Can you please address that? https://biomejs.dev/linter/#rule-pillars
Is that more powerful, though? The TS eslint rule leverages the type system... |
As in wider range, it also covers |
|
@muratclk still interested in this PR? |
4 participants
Summary
Port of
prefer-includesfrom typescript-eslint.This adds a new nursery lint rule
useIncludesthat enforces the use of.includes()over.indexOf()comparisons when checking for the presence of a value in a string or array.Detected patterns
Presence checks (replaced with
x.includes(y)):x.indexOf(y) !== -1x.indexOf(y) != -1x.indexOf(y) >= 0x.indexOf(y) > -1Absence checks (replaced with
!x.includes(y)):x.indexOf(y) === -1x.indexOf(y) == -1x.indexOf(y) < 0Also handles reversed operands and parenthesized expressions.
Closes #7654
Test plan
invalid.jswith all detected patterns including reversed operands and parenthesized expressionsvalid.jswith cases that should not trigger (direct.includes(), positional.indexOf(), different comparisons, different methods)