feat(linter): implement no-plusplus rule#5570
feat(linter): implement no-plusplus rule#5570IWANABETHATGUY merged 8 commits intooxc-project:mainfrom
no-plusplus rule#5570Conversation
Your org has enabled the Graphite merge queue for merging into mainAdd the label “0-merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix. You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link. |
CodSpeed Performance ReportMerging #5570 will not alter performanceComparing Summary
|
|
Sorry, I misclick the merge button, I want to rebase the branch, since the pr not seems introduce performance regression. |
|
Did some simplify, can you patch this: diff --git a/crates/oxc_linter/src/rules/eslint/no_plusplus.rs b/crates/oxc_linter/src/rules/eslint/no_plusplus.rs
index 89448d11f..7b3f73de5 100644
--- a/crates/oxc_linter/src/rules/eslint/no_plusplus.rs
+++ b/crates/oxc_linter/src/rules/eslint/no_plusplus.rs
@@ -92,7 +92,9 @@ impl Rule for NoPlusplus {
return;
};
- if self.allow_for_loop_afterthoughts && is_for_loop_afterthought(node, ctx) {
+ if self.allow_for_loop_afterthoughts
+ && is_for_loop_afterthought(node, ctx).unwrap_or_default()
+ {
return;
}
@@ -100,52 +102,27 @@ impl Rule for NoPlusplus {
}
}
-/// Whether the given AST node is a ++ or -- inside of a for-loop update.
-fn is_for_statement_update(node: &AstNode, ctx: &LintContext) -> bool {
- let Some(parent) = ctx.nodes().parent_node(node.id()) else {
- return false;
- };
- let AstKind::ForStatement(for_stmt) = parent.kind() else {
- return false;
- };
-
- for_stmt.update.as_ref().is_some_and(|update| is_eq_node_expr(node, update))
-}
-
-/// Checks if the given node is equivalent to the given expression (i.e., they have the same span).
-fn is_eq_node_expr(node: &AstNode, expr: &Expression) -> bool {
- // TODO: This logic should be moved to somewhere more general and shared across rules and expanded
- // to cover all expressions and node types
- let node_span = match node.kind() {
- AstKind::UpdateExpression(expr) => expr.span,
- AstKind::SequenceExpression(expr) => expr.span,
- _ => return false,
- };
- let expr_span = match expr {
- Expression::UpdateExpression(expr) => expr.span,
- Expression::SequenceExpression(expr) => expr.span,
- _ => return false,
- };
- node_span == expr_span
-}
-
/// Determines whether the given node is considered to be a for loop "afterthought" by the logic of this rule.
/// In particular, it returns `true` if the given node is either:
/// - The update node of a `ForStatement`: for (;; i++) {}
/// - An operand of a sequence expression that is the update node: for (;; foo(), i++) {}
/// - An operand of a sequence expression that is child of another sequence expression, etc.,
/// up to the sequence expression that is the update node: for (;; foo(), (bar(), (baz(), i++))) {}
-fn is_for_loop_afterthought(node: &AstNode, ctx: &LintContext) -> bool {
- if let Some(parent) = ctx.nodes().parent_node(node.id()) {
- match parent.kind() {
+fn is_for_loop_afterthought(node: &AstNode, ctx: &LintContext) -> Option<bool> {
+ let mut cur = ctx.nodes().parent_node(node.id())?;
+ // dbg!(cur);
+ loop {
+ match cur.kind() {
AstKind::SequenceExpression(_) | AstKind::ParenthesizedExpression(_) => {
- return is_for_loop_afterthought(parent, ctx)
+ cur = ctx.nodes().parent_node(cur.id())?;
}
- _ => (),
+ _ => break,
}
}
-
- is_for_statement_update(node, ctx)
+ match cur.kind() {
+ AstKind::ForStatement(stmt) => Some(stmt.update.is_some()),
+ _ => Some(false),
+ }
}
#[test] |
|
#5581, it is as I expected, so the performance regressed just because these two case use too much |
|
I'm likely going on leave shortly, so I probably won't be able to take a look at this for a while, so feel free to make edits to this PR and merge/close if needed. |
|
I may have broke codspeed, hold on. |
|
@IWANABETHATGUY I applied your simplifications and even went a bit further by using a while-let loop instead of loop-match. |
|
rest part Looks good to me, |
Merge activity
|
0759a15 to
a9fbec8
Compare
a9fbec8 to
29ab4f8
Compare
## [0.9.5] - 2024-09-12 ### Features - 4b04f65 linter: Implement `no-plusplus` rule (#5570) (Cam McHenry) --------- Co-authored-by: Boshen <[email protected]> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
This PR implements the
no-plusplusrule and is more-or-less a direct port of the ESLint version of the rule.