perf: Use Map instead of AstMetadataApiWithTargetsResolver[] for faster matching#679
Conversation
Use Maps instead of arrays
3d3032d to
ca74d10
Compare
|
So updated the branch. Updated benchmarks: https://gist.github.com/igeligel/548048b85386d1f3760ba80e5be0f628 |
|
Fixed conflicts by merging the New benchmark results: https://gist.github.com/igeligel/fabb1ddfdeefcf66c777788eb875845f cc @amilajack |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Hey @amilajack , |
|
@codex review |
There was a problem hiding this comment.
Pull request overview
This PR refactors rule selection and lookup in the compat ESLint rule to speed up matching by replacing per-node linear searches over arrays with Map-based lookups.
Changes:
- Refactor
getRulesForTargetsto build and return per-AST-node-typeMapstructures for faster rule retrieval. - Update helper lint functions (
lintCallExpression,lintNewExpression, etc.) to consumeMapinputs instead of arrays and use.get()lookups. - Rewire
compatrule wiring to pass the new maps into the lint helpers.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/rules/compat.ts | Builds per-node-type rule maps in getRulesForTargets and updates rule visitor bindings to pass maps. |
| src/helpers.ts | Changes lint helper APIs to accept rule maps and perform Map lookups during AST traversal. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const callExpression = new Map<string, AstMetadataApiWithTargetsResolver>(); | ||
| for (const rule of byType.CallExpression) { | ||
| const key = rule.object.toLowerCase(); | ||
| if (!callExpression.has(key)) callExpression.set(key, rule); | ||
| } |
| const protoChainKey = rule.protoChainId.toLowerCase(); | ||
| if (!memberExpression.has(protoChainKey)) | ||
| memberExpression.set(protoChainKey, rule); | ||
| const key = (rule.property ? `${rule.object}.${rule.property}` : rule.object).toLowerCase(); |
| if (!node.callee) return; | ||
| const calleeName = node.callee.name; | ||
| const failingRule = rules.find((rule) => rule.object === calleeName); | ||
| if (!calleeName) return; | ||
| const failingRule = rulesMap.get(calleeName.toLowerCase()); | ||
| if (failingRule) |
| const failingRule = rules.find( | ||
| (rule) => rule.protoChainId === protoChainId | ||
| ); | ||
| const failingRule = rulesMap.get(protoChainId.toLowerCase()); |
| const getRulesForTargets = memoize( | ||
| (targetsJSON: string, lintAllEsApis: boolean): RulesFilteredByTargets => { | ||
| const result = { | ||
| (targetsJSON: string, lintAllEsApis: boolean): RuleMapsForTargets => { |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 16e3367912
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const calleeName = node.callee.name; | ||
| const failingRule = rules.find((rule) => rule.object === calleeName); | ||
| if (!calleeName) return; | ||
| const failingRule = rulesMap.get(calleeName.toLowerCase()); |
There was a problem hiding this comment.
Restore case-sensitive matching for call/new/expression
Lowercasing identifiers during lookup changes CallExpression/NewExpression/ExpressionStatement matching from exact to case-insensitive, which introduces false positives for non-equivalent symbols (for example new url() now matches the URL rule). Previous logic only allowed case-insensitive matching in the browser-global member-expression path; this broad lookup makes unrelated identifiers fail compat checks purely because they share lowercase forms.
Useful? React with 👍 / 👎.
| rulesMap.get(`${objectName}.${propertyName}`.toLowerCase()) ?? | ||
| rulesMap.get(objectName.toLowerCase()); |
There was a problem hiding this comment.
Preserve property-case checks in member lookup
The member-expression key is lowercased as ${objectName}.${propertyName}, so property matching becomes case-insensitive even for non-browser globals. Because the subsequent guard only validates failingRule.object case, Promise.Resolve will match Promise.resolve and be reported as unsupported. This regresses correctness by flagging APIs that are not actually referenced.
Useful? React with 👍 / 👎.
|
Thanks for the review! Will adjust based on the comments later. |
|
feel free to only focus on the codex comments |
Feedback
|
@amilajack - Adjusted and I have added some tests as well: https://github.com/amilajack/eslint-plugin-compat/pull/679/changes#diff-3b214fee0799850287be9de60a3b777c8d9c2d8128458b0e465bbe197dbe794e |
…faster matching (#679) * perf: compat Use Maps instead of arrays * refactor: ci Feedback
## [7.0.2](v7.0.1...v7.0.2) (2026-04-29) ### Performance Improvements * Use `Map` instead of `AstMetadataApiWithTargetsResolver[]` for faster matching ([#679](#679)) ([fecdcc6](fecdcc6))
This refactors the
getRulesForTargetsfunction to returnMap<string, AstMetadataApiWithTargetsResolver>for each expression matching.Before it returned:
Now it returns:
This also requires changes now in
helpers.tsxand makes a lot ofrules.find(calls much faster. O(1) instead of O(n).I benchmarked the changes as well (Thanks for providing the benchmarks):
Before: https://gist.github.com/igeligel/8c2dd38d1da1363f3a57c3820b208688#file-1-before-txt
After: https://gist.github.com/igeligel/8c2dd38d1da1363f3a57c3820b208688#file-2-after-txt
There are some more improvements as well that I will follow up in other Pull Requests.