fix(linter): avoid infinite loop when traverse ancestors in jest/no_conditional_expect (#3330)

closes: #3325

crates/oxc_linter/src/rules/jest/no_conditional_expect.rs

@@ -1,6 +1,5 @@
 use oxc_diagnostics::OxcDiagnostic;
-
+use rustc_hash::FxHashSet;
-use std::collections::HashMap;
 
 use oxc_ast::AstKind;
 use oxc_macros::declare_oxc_lint;
@@ -58,24 +57,19 @@ declare_oxc_lint!(
     correctness
 );
 
+// To flag we encountered a conditional block/catch block when traversing the parents.
+#[derive(Debug, Clone, Copy)]
+struct InConditional(bool);
+
 impl Rule for NoConditionalExpect {
     fn run_once(&self, ctx: &LintContext) {
-        let possible_jest_nodes = collect_possible_jest_call_node(ctx);
-        let id_nodes_mapping = possible_jest_nodes.iter().fold(HashMap::new(), |mut acc, cur| {
-            acc.entry(cur.node.id()).or_insert(cur);
-            acc
-        });
         for node in &collect_possible_jest_call_node(ctx) {
-            run(node, &id_nodes_mapping, ctx);
+            run(node, ctx);
         }
     }
 }
 
-fn run<'a>(
+fn run<'a>(possible_jest_node: &PossibleJestNode<'a, '_>, ctx: &LintContext<'a>) {
-    possible_jest_node: &PossibleJestNode<'a, '_>,
-    id_nodes_mapping: &HashMap<AstNodeId, &PossibleJestNode<'a, '_>>,
-    ctx: &LintContext<'a>,
-) {
     let node = possible_jest_node.node;
     if let AstKind::CallExpression(call_expr) = node.kind() {
         let Some(jest_fn_call) = parse_expect_jest_fn_call(call_expr, possible_jest_node, ctx)
@@ -83,8 +77,12 @@ fn run<'a>(
             return;
         };
 
-        let has_condition_or_catch = check_parents(node, id_nodes_mapping, ctx, false);
+        // Record visited nodes for avoid infinite loop.
-        if has_condition_or_catch {
+        let mut visited = FxHashSet::default();
+
+        // When first visiting the node, we assume it's not in a conditional block.
+        let has_condition_or_catch = check_parents(node, &mut visited, InConditional(false), ctx);
+        if matches!(has_condition_or_catch, InConditional(true)) {
             ctx.diagnostic(no_conditional_expect_diagnostic(jest_fn_call.head.span));
         }
     }
@@ -92,22 +90,26 @@ fn run<'a>(
 
 fn check_parents<'a>(
     node: &AstNode<'a>,
-    id_nodes_mapping: &HashMap<AstNodeId, &PossibleJestNode<'a, '_>>,
+    visited: &mut FxHashSet<AstNodeId>,
+    in_conditional: InConditional,
     ctx: &LintContext<'a>,
-    in_conditional: bool,
+) -> InConditional {
-) -> bool {
+    // if the node is already visited, we should return `false` to avoid infinite loop.
+    if !visited.insert(node.id()) {
+        return InConditional(false);
+    }
+
     let Some(parent_node) = ctx.nodes().parent_node(node.id()) else {
-        return false;
+        return InConditional(false);
     };
 
     match parent_node.kind() {
         AstKind::CallExpression(call_expr) => {
-            let Some(parent) = id_nodes_mapping.get(&parent_node.id()) else {
+            let jest_node = PossibleJestNode { node: parent_node, original: None };
-                return check_parents(parent_node, id_nodes_mapping, ctx, in_conditional);
+
-            };
             if is_type_of_jest_fn_call(
                 call_expr,
-                parent,
+                &jest_node,
                 ctx,
                 &[JestFnKind::General(JestGeneralFnKind::Test)],
             ) {
@@ -116,7 +118,7 @@ fn check_parents<'a>(
 
             if let Some(member_expr) = call_expr.callee.as_member_expression() {
                 if member_expr.static_property_name() == Some("catch") {
-                    return check_parents(parent_node, id_nodes_mapping, ctx, true);
+                    return check_parents(parent_node, visited, InConditional(true), ctx);
                 }
             }
         }
@@ -126,29 +128,38 @@ fn check_parents<'a>(
         | AstKind::ConditionalExpression(_)
         | AstKind::AwaitExpression(_)
         | AstKind::LogicalExpression(_) => {
-            return check_parents(parent_node, id_nodes_mapping, ctx, true)
+            return check_parents(parent_node, visited, InConditional(true), ctx)
         }
         AstKind::Function(function) => {
             let Some(ident) = &function.id else {
-                return false;
+                return InConditional(false);
             };
             let symbol_table = ctx.semantic().symbols();
             let Some(symbol_id) = ident.symbol_id.get() else {
-                return false;
+                return InConditional(false);
             };
 
-            return symbol_table.get_resolved_references(symbol_id).any(|reference| {
+            // Consider cases like:
+            // ```javascript
+            // function foo() {
+            //   foo()
+            // }
+            // ````
+            // To avoid infinite loop, we need to check if the function is already visited when
+            // call `check_parents`.
+            let boolean = symbol_table.get_resolved_references(symbol_id).any(|reference| {
                 let Some(parent) = ctx.nodes().parent_node(reference.node_id()) else {
                     return false;
                 };
-                check_parents(parent, id_nodes_mapping, ctx, in_conditional)
+                matches!(check_parents(parent, visited, in_conditional, ctx), InConditional(true))
             });
+            return InConditional(boolean);
         }
-        AstKind::Program(_) => return false,
+        AstKind::Program(_) => return InConditional(false),
         _ => {}
     }
 
-    check_parents(parent_node, id_nodes_mapping, ctx, in_conditional)
+    check_parents(parent_node, visited, in_conditional, ctx)
 }
 
 #[test]
@@ -405,6 +416,22 @@ fn test() {
             ",
             None,
         ),
+        (
+            "function verifyVNodeTree(vnode) {
+                    if (vnode._nextDom) {
+                        expect.fail('vnode should not have _nextDom:' + vnode._nextDom);
+                    }
+
+                    if (vnode._children) {
+                        for (let child of vnode._children) {
+                            if (child) {
+                                verifyVNodeTree(child);
+                            }
+                        }
+                    }
+                }",
+            None,
+        ),
     ];
 
     let fail = vec![
@@ -838,6 +865,28 @@ fn test() {
             ",
             None,
         ),
+        (
+            "
+            it('works', async () => {
+                verifyVNodeTree(vnode);
+
+                function verifyVNodeTree(vnode) {
+                    if (vnode._nextDom) {
+                        expect.fail('vnode should not have _nextDom:' + vnode._nextDom);
+                    }
+
+                    if (vnode._children) {
+                        for (let child of vnode._children) {
+                            if (child) {
+                                    verifyVNodeTree(child);
+                            }
+                        }
+                    }
+                }
+            });
+            ",
+            None,
+        ),
     ];
 
     Tester::new(NoConditionalExpect::NAME, pass, fail).with_jest_plugin(true).test_and_snapshot();

crates/oxc_linter/src/snapshots/no_conditional_expect.snap

@@ -406,3 +406,12 @@ expression: no_conditional_expect
  4 │                 });
    ╰────
   help: Avoid calling `expect` conditionally`
+
+  ⚠ eslint-plugin-jest(no-conditional-expect): Unexpected conditional expect
+   ╭─[no_conditional_expect.tsx:7:25]
+ 6 │                     if (vnode._nextDom) {
+ 7 │                         expect.fail('vnode should not have _nextDom:' + vnode._nextDom);
+   ·                         ──────
+ 8 │                     }
+   ╰────
+  help: Avoid calling `expect` conditionally`