perf(linter): dispatch react_perf rules only on JSXAttribute nodes#24084
Closed
anonrig wants to merge 2 commits into
Closed
perf(linter): dispatch react_perf rules only on JSXAttribute nodes#24084anonrig wants to merge 2 commits into
anonrig wants to merge 2 commits into
Conversation
The four react_perf rules got their `Rule` implementation from a blanket `impl<R> Rule for R where R: ReactPerfRule` in `utils/react_perf.rs`. Because the `run` body lived outside the rule files, `oxc_linter_codegen` could not detect the node types these rules match, generating `NODE_TYPES: None` — so all four rules ran on every AST node of every JSX/TSX file. Replace the blanket impl with per-rule `Rule` impls whose `run` starts with the `let AstKind::JSXAttribute(..) = node.kind() else` pattern that codegen detects, delegating to a shared `run_react_perf_rule` helper. The rules are now dispatched only to `JSXAttribute` nodes.
…t.setTimeout references All three diagnostics this rule reports require at least one `jest.setTimeout` member access. Collect those references first and skip collecting possible jest call nodes and the second full pass over all references when there are none, which is the common case for non-test files.
Merging this PR will improve performance by 5.6%
Performance Changes
Tip Curious why this is faster? Comment Comparing 
Footnotes
|
Contributor
|
Thanks for working on this! Looks like camchenry beat you to it! I'll split the jest change as that's still useful |
graphite-app Bot
pushed a commit
that referenced
this pull request
Jul 2, 2026
) ### Summary Extracts the Jest-only performance change from #24084. Skips collecting possible Jest call nodes and the second full reference pass in `jest/no-confusing-set-timeout` when the file has no `jest.setTimeout` references. Every diagnostic emitted by this rule requires at least one `jest.setTimeout` reference, so this keeps behavior unchanged while avoiding work in the common case. ### Review Reviewed #24084 and intentionally left out the separate `react_perf` dispatch changes so this PR contains only the Jest rule update. Co-authored-by: Yagiz Nizipli <[email protected]>
camc314
added a commit
that referenced
this pull request
Jul 3, 2026
) ### Summary Extracts the Jest-only performance change from #24084. Skips collecting possible Jest call nodes and the second full reference pass in `jest/no-confusing-set-timeout` when the file has no `jest.setTimeout` references. Every diagnostic emitted by this rule requires at least one `jest.setTimeout` reference, so this keeps behavior unchanged while avoiding work in the common case. ### Review Reviewed #24084 and intentionally left out the separate `react_perf` dispatch changes so this PR contains only the Jest rule update. Co-authored-by: Yagiz Nizipli <[email protected]>
camc314
added a commit
that referenced
this pull request
Jul 3, 2026
) ### Summary Extracts the Jest-only performance change from #24084. Skips collecting possible Jest call nodes and the second full reference pass in `jest/no-confusing-set-timeout` when the file has no `jest.setTimeout` references. Every diagnostic emitted by this rule requires at least one `jest.setTimeout` reference, so this keeps behavior unchanged while avoiding work in the common case. ### Review Reviewed #24084 and intentionally left out the separate `react_perf` dispatch changes so this PR contains only the Jest rule update. Co-authored-by: Yagiz Nizipli <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
The four
react_perfrules (jsx-no-new-object-as-prop,jsx-no-new-array-as-prop,jsx-no-new-function-as-prop,jsx-no-jsx-as-prop) received theirRuleimplementation from a blanketimpl<R> Rule for R where R: ReactPerfRuleinutils/react_perf.rs. Because therunbody lived outside the rule files,oxc_linter_codegencould not detect the node types they match and generatedNODE_TYPES: None— so all four rules ran on every AST node of every JSX/TSX file, even though they only ever act onAstKind::JSXAttribute.This PR replaces the blanket impl with per-rule
Ruleimpls whoserunstarts with thelet AstKind::JSXAttribute(..) = node.kind() else { return; }pattern that codegen detects, delegating the shared logic to arun_react_perf_rulehelper. The generated runners now carryNODE_TYPES: [JSXAttribute], and withvue/max_propsbeing the only remainingNODE_TYPES: Nonerule (gated to.vuefiles), the "run on any node" bucket is empty for typical configurations.On
kitchen-sink.tsxthis removes ~133k wastedruninvocations per rule per lint (2.66M over 20 iterations; the rules dropped from ~17% of instrumented rule time to ~0.4%).A second commit adds an early exit to
jest/no-confusing-set-timeout: every diagnostic it can report requires at least onejest.setTimeoutreference, so the rule now collects those references first and skips building the possible-jest-node map and a second full pass over all references when there are none (the common case outside test setup files).Benchmarks (local, Apple Silicon)
Retired instruction counts for
Linter::runonly (deterministic, measured viaproc_pid_rusage,ConfigStoreBuilder::all()like the criterion bench):Wall-time (criterion, idle machine, back-to-back runs): kitchen-sink −4.9%, App.tsx −3.0%. CodSpeed should give the authoritative numbers.
Also evaluated and rejected: a chunked rule-major dispatch reorder (running each rule over batches of its matching nodes to make the
RuleEnumdispatch branch predictable) — it measured 3–5% slower due to lost data-cache locality on node data, so it is not included.Testing
oxc_lintertests pass; in debug mode theTesteralso asserts the optimized (type-bucketed) and unoptimized paths produce identical diagnostics for these rules.cargo lintgenregeneratedrule_runner_impls.rs(included); rerunning it is a no-op.oxlintCLI tests pass (exceptlint::suppression::test_fixing_only_ts_go_errors, which also fails onmainin my environment — missing tsgolint binary).Disclosure
This PR was developed with the assistance of an AI coding agent (Grok), per the repo's AI usage policy. I have reviewed, tested, and take responsibility for the changes.