Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

linter: duplicate work in Jest/Vitest rules #6038

Open
DonIsaac opened this issue Sep 24, 2024 · 1 comment
Open

linter: duplicate work in Jest/Vitest rules #6038

DonIsaac opened this issue Sep 24, 2024 · 1 comment
Labels
A-linter Area - Linter C-bug Category - Bug C-performance Category - Solution not expected to change functional behavior, only performance

Comments

@DonIsaac
Copy link
Collaborator

DonIsaac commented Sep 24, 2024

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.

@DonIsaac DonIsaac added C-bug Category - Bug A-linter Area - Linter C-performance Category - Solution not expected to change functional behavior, only performance labels Sep 24, 2024
@camchenry
Copy link
Collaborator

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: #6049

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-linter Area - Linter C-bug Category - Bug C-performance Category - Solution not expected to change functional behavior, only performance
Projects
None yet
Development

No branches or pull requests

2 participants