-
-
Notifications
You must be signed in to change notification settings - Fork 449
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(lint): add rule noUselessLengthCheck
#4060
base: main
Are you sure you want to change the base?
Conversation
CodSpeed Performance ReportMerging #4060 will improve performances by 7.11%Comparing Summary
Benchmarks breakdown
|
}; | ||
// ( hoge ) | ||
if let Some(parenthesized_exp_parent) = JsParenthesizedExpression::cast(parent.clone()) { | ||
return is_logical_exp_child(&AnyJsExpression::from(parenthesized_exp_parent), operator); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is recursive, which we generally try to avoid for performance. You should be able to traverse up the tree by calling node.syntaxI().ancestors()
.
Example:
biome/crates/biome_js_analyze/src/react.rs
Lines 270 to 276 in b1874ed
fn is_react_export(binding: &Binding, lib: ReactLibrary) -> bool { | |
binding | |
.syntax() | |
.ancestors() | |
.find_map(|ancestor| JsImport::cast(ancestor)?.source_text().ok()) | |
.is_some_and(|source| lib.import_names().contains(&source.text())) | |
} |
comparing_length(&left, operator, &right, expect_error) | ||
} | ||
|
||
fn comparing_length( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to leave doc comments explaining what these utility functions do. Doesn't need to be long, just 1-2 sentences explaining the context. It helps a lot when the rule needs to be updated by somebody else later.
|
||
pub type Replacer = (JsLogicalExpression, AnyJsExpression, TextRange); | ||
|
||
fn search_logical_exp( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for this, add a doc comment.
}; | ||
let left = logical_exp.left().ok()?; | ||
let left_replacer = (logical_exp.clone(), logical_exp.right().ok()?, left.range()); | ||
search_logical_exp( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also recursion, though I'm not entirely sure how you would avoid it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is performing a complete search within the range of descendant logical expressions. While using a stack instead of a recursive function comes to mind, I believe it would only complicate things further.
Summary
Implement
noUselessLengthCheck
(unicorn/no-useless-length-check)closes #3941
Test Plan