From c16ae6038e9d2d922997b8a57c3d0ddf0221f0e0 Mon Sep 17 00:00:00 2001
From: camchenry <1514176+camchenry@users.noreply.github.com>
Date: Tue, 24 Sep 2024 17:51:09 +0000
Subject: [PATCH] perf(linter): `jest/prefer-hooks-in-order`: rewrite rule to
allocate less and iterate fewer times (#6030)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Profiling this in a single-threaded run showed this rule to be relatively slow compared to other rules:
The profile shows that a lot of time is spent doing pushes on `Vec`:
I believe this is because we were essentially duplicating the entirety of `ctx.nodes()`, and then iterating over it again for the actual rule.
I addressed this by:
- We no longer store any nodes (we only store the previous hook span + order), which saves many allocations.
- We only iterate over the nodes once and store the previous hook function in a per-scope hash map. This should be faster since we now only do `n` iterations, instead of `2n` iterations.
Benchmarking on the `microsoft/vscode` repository shows that this is probably faster (by up to ~3%), and probably not any slower:
The snapshot change in this PR is due to slightly different ordering, I think, but it appears to be the same diagnostics overall.
---
.../src/rules/jest/prefer_hooks_in_order.rs | 104 +++++++++---------
.../src/snapshots/prefer_hooks_in_order.snap | 41 ++++---
2 files changed, 75 insertions(+), 70 deletions(-)
diff --git a/crates/oxc_linter/src/rules/jest/prefer_hooks_in_order.rs b/crates/oxc_linter/src/rules/jest/prefer_hooks_in_order.rs
index c5adcdee5748e..fb00367b1f96f 100644
--- a/crates/oxc_linter/src/rules/jest/prefer_hooks_in_order.rs
+++ b/crates/oxc_linter/src/rules/jest/prefer_hooks_in_order.rs
@@ -1,7 +1,7 @@
-use oxc_ast::{ast::CallExpression, AstKind};
+use oxc_ast::AstKind;
use oxc_diagnostics::OxcDiagnostic;
use oxc_macros::declare_oxc_lint;
-use oxc_semantic::{AstNode, ScopeId};
+use oxc_semantic::ScopeId;
use oxc_span::Span;
use rustc_hash::FxHashMap;
@@ -13,10 +13,10 @@ use crate::{
},
};
-fn reorder_hooks(x1: &str, x2: &str, span: Span) -> OxcDiagnostic {
+fn reorder_hooks(hook: (&str, Span), previous_hook: (&str, Span)) -> OxcDiagnostic {
OxcDiagnostic::warn("Prefer having hooks in a consistent order.")
- .with_help(format!("{x1:?} hooks should be before any {x2:?} hooks"))
- .with_label(span)
+ .with_help(format!("{:?} hooks should be before any {:?} hooks", hook.0, previous_hook.0))
+ .with_label(hook.1)
}
#[derive(Debug, Default, Clone)]
@@ -140,60 +140,66 @@ declare_oxc_lint!(
impl Rule for PreferHooksInOrder {
fn run_once(&self, ctx: &LintContext) {
- let mut hook_groups: FxHashMap> = FxHashMap::default();
+ let mut previous_hook_orders: FxHashMap = FxHashMap::default();
for node in ctx.nodes() {
- hook_groups.entry(node.scope_id()).or_default().push(*node);
- }
+ if let AstKind::CallExpression(call_expr) = node.kind() {
+ let possible_jest_node = &PossibleJestNode { node, original: None };
+ let Some(ParsedJestFnCallNew::GeneralJest(jest_fn_call)) =
+ parse_jest_fn_call(call_expr, possible_jest_node, ctx)
+ else {
+ previous_hook_orders.remove(&node.scope_id());
+ continue;
+ };
- for (_, nodes) in hook_groups {
- let mut previous_hook_index = -1;
+ if !matches!(jest_fn_call.kind, JestFnKind::General(JestGeneralFnKind::Hook)) {
+ previous_hook_orders.remove(&node.scope_id());
+ continue;
+ }
- for node in nodes {
- if let AstKind::CallExpression(call_expr) = node.kind() {
- let possible_jest_node = &PossibleJestNode { node: &node, original: None };
- Self::check(&mut previous_hook_index, possible_jest_node, call_expr, ctx);
+ let previous_hook_order = previous_hook_orders.get(&node.scope_id());
+
+ let hook_name = jest_fn_call.name.as_ref();
+ let Some(hook_order) = get_hook_order(hook_name) else {
+ continue;
};
- }
+
+ if let Some((previous_hook_order, previous_hook_span)) = previous_hook_order {
+ if hook_order < *previous_hook_order {
+ let Some(previous_hook_name) = get_hook_name(*previous_hook_order) else {
+ continue;
+ };
+
+ ctx.diagnostic(reorder_hooks(
+ (hook_name, call_expr.span),
+ (previous_hook_name, *previous_hook_span),
+ ));
+ continue;
+ }
+ }
+ previous_hook_orders.insert(node.scope_id(), (hook_order, call_expr.span));
+ };
}
}
}
-impl PreferHooksInOrder {
- fn check<'a>(
- previous_hook_index: &mut i32,
- possible_jest_node: &PossibleJestNode<'a, '_>,
- call_expr: &'a CallExpression<'_>,
- ctx: &LintContext<'a>,
- ) {
- let Some(ParsedJestFnCallNew::GeneralJest(jest_fn_call)) =
- parse_jest_fn_call(call_expr, possible_jest_node, ctx)
- else {
- *previous_hook_index = -1;
- return;
- };
-
- if !matches!(jest_fn_call.kind, JestFnKind::General(JestGeneralFnKind::Hook)) {
- *previous_hook_index = -1;
- return;
- }
-
- let hook_orders = ["beforeAll", "beforeEach", "afterEach", "afterAll"];
- let hook_name = jest_fn_call.name;
- let hook_pos =
- hook_orders.iter().position(|h| h.eq_ignore_ascii_case(&hook_name)).unwrap_or_default();
- let previous_hook_pos = usize::try_from(*previous_hook_index).unwrap_or(0);
-
- if hook_pos < previous_hook_pos {
- let Some(previous_hook_name) = hook_orders.get(previous_hook_pos) else {
- return;
- };
-
- ctx.diagnostic(reorder_hooks(&hook_name, previous_hook_name, call_expr.span));
- return;
- }
+fn get_hook_order(hook_name: &str) -> Option {
+ match hook_name {
+ "beforeAll" => Some(0),
+ "beforeEach" => Some(1),
+ "afterEach" => Some(2),
+ "afterAll" => Some(3),
+ _ => None,
+ }
+}
- *previous_hook_index = i32::try_from(hook_pos).unwrap_or(-1);
+fn get_hook_name(hook_order: u8) -> Option<&'static str> {
+ match hook_order {
+ 0 => Some("beforeAll"),
+ 1 => Some("beforeEach"),
+ 2 => Some("afterEach"),
+ 3 => Some("afterAll"),
+ _ => None,
}
}
diff --git a/crates/oxc_linter/src/snapshots/prefer_hooks_in_order.snap b/crates/oxc_linter/src/snapshots/prefer_hooks_in_order.snap
index 1f1a2c5d547fb..e5f5c54ef537b 100644
--- a/crates/oxc_linter/src/snapshots/prefer_hooks_in_order.snap
+++ b/crates/oxc_linter/src/snapshots/prefer_hooks_in_order.snap
@@ -1,6 +1,5 @@
---
source: crates/oxc_linter/src/tester.rs
-assertion_line: 216
---
⚠ eslint-plugin-vitest(prefer-hooks-in-order): Prefer having hooks in a consistent order.
╭─[prefer_hooks_in_order.tsx:6:21]
@@ -185,16 +184,6 @@ assertion_line: 216
╰────
help: "afterEach" hooks should be before any "afterAll" hooks
- ⚠ eslint-plugin-vitest(prefer-hooks-in-order): Prefer having hooks in a consistent order.
- ╭─[prefer_hooks_in_order.tsx:38:25]
- 37 │
- 38 │ ╭─▶ beforeEach(() => {
- 39 │ │ mockLogger();
- 40 │ ╰─▶ });
- 41 │
- ╰────
- help: "beforeEach" hooks should be before any "afterEach" hooks
-
⚠ eslint-plugin-vitest(prefer-hooks-in-order): Prefer having hooks in a consistent order.
╭─[prefer_hooks_in_order.tsx:7:21]
6 │
@@ -205,6 +194,16 @@ assertion_line: 216
╰────
help: "beforeAll" hooks should be before any "beforeEach" hooks
+ ⚠ eslint-plugin-vitest(prefer-hooks-in-order): Prefer having hooks in a consistent order.
+ ╭─[prefer_hooks_in_order.tsx:38:25]
+ 37 │
+ 38 │ ╭─▶ beforeEach(() => {
+ 39 │ │ mockLogger();
+ 40 │ ╰─▶ });
+ 41 │
+ ╰────
+ help: "beforeEach" hooks should be before any "afterEach" hooks
+
⚠ eslint-plugin-vitest(prefer-hooks-in-order): Prefer having hooks in a consistent order.
╭─[prefer_hooks_in_order.tsx:6:17]
5 │ });
@@ -388,16 +387,6 @@ assertion_line: 216
╰────
help: "afterEach" hooks should be before any "afterAll" hooks
- ⚠ eslint-plugin-vitest(prefer-hooks-in-order): Prefer having hooks in a consistent order.
- ╭─[prefer_hooks_in_order.tsx:38:21]
- 37 │
- 38 │ ╭─▶ beforeEach(() => {
- 39 │ │ mockLogger();
- 40 │ ╰─▶ });
- 41 │
- ╰────
- help: "beforeEach" hooks should be before any "afterEach" hooks
-
⚠ eslint-plugin-vitest(prefer-hooks-in-order): Prefer having hooks in a consistent order.
╭─[prefer_hooks_in_order.tsx:7:17]
6 │
@@ -407,3 +396,13 @@ assertion_line: 216
10 │
╰────
help: "beforeAll" hooks should be before any "beforeEach" hooks
+
+ ⚠ eslint-plugin-vitest(prefer-hooks-in-order): Prefer having hooks in a consistent order.
+ ╭─[prefer_hooks_in_order.tsx:38:21]
+ 37 │
+ 38 │ ╭─▶ beforeEach(() => {
+ 39 │ │ mockLogger();
+ 40 │ ╰─▶ });
+ 41 │
+ ╰────
+ help: "beforeEach" hooks should be before any "afterEach" hooks