feat(linter): implement no-plusplus rule

[camchenry]

• part of ☂️ eslint core #479

This PR implements the no-plusplus rule and is more-or-less a direct port of the ESLint version of the rule.

[graphite-app]

Your org has enabled the Graphite merge queue for merging into main

Add 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-hq]

CodSpeed Performance Report

Merging #5570 will not alter performance

_{Comparing camchenry:lint/no-plusplus (29ab4f8) with main (d06bab6)}

Summary

✅ 29 untouched benchmarks

[IWANABETHATGUY]

Sorry, I misclick the merge button, I want to rebase the branch, since the pr not seems introduce performance regression.

[IWANABETHATGUY]

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]

[IWANABETHATGUY]

#5581, it is as I expected, so the performance regressed just because these two case use too much UpdateExpr , maybe we could adjust the RuleCategory( disable it by default) to solve perf issues.

[camchenry]

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.

[Boshen]

I may have broke codspeed, hold on.

[camchenry]

@IWANABETHATGUY I applied your simplifications and even went a bit further by using a while-let loop instead of loop-match.

[IWANABETHATGUY]

rest part Looks good to me,

[graphite-app]

Merge activity

• Sep 11, 10:49 PM EDT: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
• Sep 11, 11:12 PM EDT: IWANABETHATGUY added this pull request to the Graphite merge queue.
• Sep 11, 11:12 PM EDT: The Graphite merge queue couldn't merge this PR because it failed for an unknown reason (Stack merges are not currently supported for forked repositories. Please create a branch in the target repository in order to merge).