oxc-project / oxc

⚓ A collection of JavaScript tools written in Rust.
https://oxc.rs
MIT License
12.42k stars 456 forks source link

linter: duplicate work in Jest/Vitest rules #6038

Closed DonIsaac closed 3 weeks ago

DonIsaac commented 1 month ago

This is a big one. I haven't taken the time to write this down until now.

Problem

Most (if not all) eslint-plugin-jest rules use collect_possible_jest_call_node in a run_once block, then pass those nodes to an internal "run" implementation. For example,

// crates/oxc_linter/src/rules/jest/no_hooks.rs
impl Rule for NoHooks {
    fn from_configuration(value: serde_json::Value) -> Self {
        let allow = value
            .get(0)
            .and_then(|config| config.get("allow"))
            .and_then(serde_json::Value::as_array)
            .map(|v| {
                v.iter().filter_map(serde_json::Value::as_str).map(ToString::to_string).collect()
            })
            .unwrap_or_default();

        Self(Box::new(NoHooksConfig { allow }))
    }

    fn run_once(&self, ctx: &LintContext) {
        for possible_jest_node in collect_possible_jest_call_node(ctx) {
            self.run(&possible_jest_node, ctx);
        }
    }
}

This is problematic. Each jest rule repeats the same work, which wastes cycles. To make things worse, collect_possible_jest_call_node allocates nodes it discovers onto the heap.

(Possible) Solution

  1. Update the Rule trait, adding a run_on_jest_node method.
  2. Update Linter::run to use collect_possible_jest_call_node and pass a reference to them to rule.run_on_jest_node
  3. Remove all run_once impls from jest/vitest rules.

Acceptance Criteria

  1. Linter should skip jest node collection if neither the jest or vitest plugins are enabled
  2. Same as above, but non-test files should also be skipped, regardless if jest/vitest plugins are enabled or not

Extra Credit

Bonus points if you can refactor collect_possible_jest_node to return an impl Iterator instead of a Vec.

CC: @camchenry, who has actively been making performance improvements to the linter.

camchenry commented 1 month ago

I attempted to do this, but I had an absolute heck of a time getting the lifetimes to work out properly. I'm not experienced enough with Rust yet to know exactly where the mistake is, so help appreciated: https://github.com/oxc-project/oxc/pull/6049

camchenry commented 3 weeks ago

Closing this as done with https://github.com/oxc-project/oxc/pull/6722, we even got the extra credit 😄