Skip to content

Comments

feat(linter): implement no-plusplus rule#5570

Merged
IWANABETHATGUY merged 8 commits intooxc-project:mainfrom
camchenry:lint/no-plusplus
Sep 12, 2024
Merged

feat(linter): implement no-plusplus rule#5570
IWANABETHATGUY merged 8 commits intooxc-project:mainfrom
camchenry:lint/no-plusplus

Conversation

@camchenry
Copy link
Member

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

@graphite-app
Copy link
Contributor

graphite-app bot commented Sep 7, 2024

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.

@github-actions github-actions bot added the A-linter Area - Linter label Sep 7, 2024
@codspeed-hq
Copy link

codspeed-hq bot commented Sep 7, 2024

CodSpeed Performance Report

Merging #5570 will not alter performance

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

Summary

✅ 29 untouched benchmarks

@IWANABETHATGUY
Copy link
Contributor

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

@IWANABETHATGUY
Copy link
Contributor

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
Copy link
Contributor

#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
Copy link
Member Author

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
Copy link
Member

Boshen commented Sep 7, 2024

I may have broke codspeed, hold on.

@camchenry
Copy link
Member Author

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

@IWANABETHATGUY
Copy link
Contributor

rest part Looks good to me,

@IWANABETHATGUY IWANABETHATGUY added the 0-merge Merge with Graphite Merge Queue label Sep 12, 2024
@graphite-app
Copy link
Contributor

graphite-app bot commented Sep 12, 2024

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).

@graphite-app graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Sep 12, 2024
@IWANABETHATGUY IWANABETHATGUY merged commit 4b04f65 into oxc-project:main Sep 12, 2024
@oxc-bot oxc-bot mentioned this pull request Sep 12, 2024
@oxc-bot oxc-bot mentioned this pull request Sep 12, 2024
Boshen added a commit that referenced this pull request Sep 12, 2024
## [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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-linter Area - Linter

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants