-
-
Notifications
You must be signed in to change notification settings - Fork 717
perf(linter): detect node types from let..else statements
#13690
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
perf(linter): detect node types from let..else statements
#13690
Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
d0b309f to
ab01e3a
Compare
ef16e98 to
99c3a07
Compare
CodSpeed Instrumentation Performance ReportMerging #13690 will improve performances by 45.05%Comparing Summary
Benchmarks breakdown
Footnotes |
99c3a07 to
dc4e535
Compare
ab01e3a to
53f5218
Compare
dc4e535 to
048edf2
Compare
048edf2 to
c12a102
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enhances the linter codegen performance by adding detection for let..else statement patterns when analyzing lint rules. The purpose is to extract node types from let AstKind::Something(...) = node.kind() else { return }; patterns in addition to the existing if..else pattern detection, enabling more precise rule runner trait implementations.
Key changes:
- Adds a new
LetElseDetectorto complement the existingIfElseKindDetector - Updates the main detection logic to try both detectors sequentially
- Modifies some existing lint rules to use the optimized
let..elsepattern
Reviewed Changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tasks/linter_codegen/src/main.rs | Integrates LetElseDetector and updates detection logic to try both detectors |
| tasks/linter_codegen/src/let_else_detector.rs | Implements new detector for let..else AST node type patterns |
| crates/oxc_linter/src/rules/unicorn/consistent_date_clone.rs | Converts from reference pattern to direct pattern matching |
| crates/oxc_linter/src/rules/react/no_unknown_property.rs | Converts from reference pattern to direct pattern matching |
| crates/oxc_linter/src/rule.rs | Updates test cases to verify correct node type detection |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
@camchenry Amazing work!! does this make our codspeed benchmarks not very representative of the real world any more, since we aren't actally runnint the linter on them? |
Merge activity
|
- part of #12223 Adds basic detection for syntax like `let AstKind::Something(...) = node.kind() else { return };` in lint rules for generating rule runner trait impls. ## Correctness Spot checking it on my laptop shows same number of diagnostics for `kibana` and `vscode`: <img width="640" height="180" alt="Screenshot 2025-09-11 at 5 35 47 PM" src="https://github.com/user-attachments/assets/43eb90a7-c3a9-46ed-8b73-e077a1d8d7f3" /> <img width="632" height="179" alt="Screenshot 2025-09-11 at 5 36 08 PM" src="https://github.com/user-attachments/assets/93652148-e27a-4d8c-9121-307624f2ee27" /> ## Benchmarks ~15% faster on `oven-sh/bun`: <img width="880" height="369" alt="Screenshot 2025-09-11 at 5 26 49 PM" src="https://github.com/user-attachments/assets/69a4c806-003e-4ff0-a679-ab9c6616dfc4" /> ~9% faster on `microsoft/vscode`: <img width="833" height="331" alt="Screenshot 2025-09-11 at 5 27 50 PM" src="https://github.com/user-attachments/assets/a6a42f96-ecab-448e-919c-550e6b4bdc26" /> ~11% faster on `posthog/posthog`: <img width="814" height="323" alt="Screenshot 2025-09-11 at 5 28 23 PM" src="https://github.com/user-attachments/assets/64daac0d-5bff-40df-aee7-ee843ba84351" /> ~8% faster on `elastic/kibana`: <img width="797" height="324" alt="Screenshot 2025-09-11 at 5 31 00 PM" src="https://github.com/user-attachments/assets/3ca57a2b-0e69-4460-903f-baa36de69123" />
c12a102 to
90c8286
Compare
|
@camc314 They were never truly representative of the real-world, since they run with all rules and plugins enabled which is pretty much never true in reality. It should make the CodSpeed benchmarks more sensitive to smaller changes in the linter though, since we're not measuring as many no-op rules anymore. |
## [1.16.0] - 2025-09-16 ### 🚀 Features - 97c8d06 linter: Add `preserve-caught-error` rule (#13748) (孔辉) - 8c19b18 linter/exhaustive-deps: Implement fixer for dep in global scope (#13783) (camc314) - 06bce8f linter/exhaustive-deps: Implement fixer for missing dep (#13782) (camc314) - a8675f4 linter: Add eslint/class-methods-use-this rule (#12977) (Peter Cardenas) - db33196 parser: Adds typescript rule for empty argument list (#13730) (Karan Kiri) - 2751193 linter: Add `eslint/no-useless-computed-key` rule (#13428) (yefan) - 9a205d1 regex-parser: Parse simple `TemplateLiterals` (#13265) (Sysix) ### 🐛 Bug Fixes - a2c91cd linter: Drop `rules` to allow mutable access to `ctx_host` in `run_external_rules` (#13832) (camc314) - 3af1e5d linter/no-unsafe-declaration-merging: Always mark first span as primary (#13830) (camc314) - 1c43c7c linter: Keep message when merging composite fixes (#13827) (camc314) - 26af302 linter/exhaustive-deps: Check stable value is on lhs of assignment expr (#13815) (camc314) - 4bc12d0 linter/exhaustive-deps: Remove impossible comparison with parent kind (#13814) (camc314) - 12baf5e linter/exhaustive-deps: Respect primary span when identifying disable directive location (#13781) (camc314) - fa7400a linter/no-undef: False positive with `arguments` in functions (#13763) (camc314) - 50e6e3c editor: Restrict servers paths for `oxc.path.server` (#13740) (Sysix) - b45077d editor: Strip leading slash for bin path on windows (#13738) (Sysix) - 8fa6227 editor: Don't allow `oxc.path.server` for untrusted workspaces (#13734) (Sysix) - 56da114 linter/react/jsx-handler-names: Do not detect the function name within the inline-function's body block (#13456) (Takuji Shimokawa) - b2bc5b4 linter/react-perf/jsx-no-new-object-as-prop: Skip as/satisfies exprs (#13718) (camc314) - ab51394 raw_transfer: Disable layout assertions on some 32-bit platforms (#13716) (overlookmotel) - 09428f6 linter/plugins: Remove outdated comment (#13691) (overlookmotel) - a294721 linter/plugins: Exit early if JS plugins enabled on unsupported platforms (#13689) (overlookmotel) - 68a2280 linter/plugins: More graceful exit for `--experimental-js-plugins` CLI option (#13688) (overlookmotel) ### 🚜 Refactor - 395d40d linter: Derive inmpls for `PartialEq`, `Eq` over manual ones (#13828) (camc314) - 8e4cd8f linter/func-names: Use `run_once` over looping over all nodes (#13798) (camc314) - 7f4e2fe eslint/func-names: Clean up implementation and improve documentation (#13601) (Antoine Zanardi) - 137896a language_server: Split options for linting and formatting (#13627) (Sysix) - 7346099 linter: Move `oxlint` application code into separate module (#13745) (overlookmotel) - 6dd4107 linter: Remove `#[cfg(test)]` attributes from `tester` module (#13714) (overlookmotel) - c40c6ef linter/plugins: Directory for JS plugins-related code (#13701) (overlookmotel) - a0022c1 linter/plugins: Improve error messages for JS plugins (#13699) (overlookmotel) - 1fd993f napi/oxlint: Rename `napi/oxlint2` to `napi/oxlint` (#13682) (overlookmotel) ### ⚡ Performance - 90c8286 linter: Detect node types from `let..else` statements (#13690) (camchenry) - 08c05df semantic: Make CFG construction a compile-time feature (#13678) (Boshen) ### 🎨 Styling - 99a7638 linter: Add comments + re-organise imports (#13715) (overlookmotel) ### 🧪 Testing - 18a1145 linter: Add debug assertions for skipping rules (#13724) (camc314) - cb080de linter/no-unused-vars: Add test for non ASCII chars in JSX components (#13820) (camc314) - b6eba27 linter/no-undef: Add more test cases for `arguments` (#13764) (camc314) - fb2d087 linter: Set CWD for tests (#13722) (overlookmotel) Co-authored-by: camc314 <18101008+camc314@users.noreply.github.com>

Adds basic detection for syntax like
let AstKind::Something(...) = node.kind() else { return };in lint rules for generating rule runner trait impls.Correctness
Spot checking it on my laptop shows same number of diagnostics for
kibanaandvscode:Benchmarks
~15% faster on
oven-sh/bun:~9% faster on
microsoft/vscode:~11% faster on
posthog/posthog:~8% faster on
elastic/kibana: