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

perf: Use a memoized key lookup for rules #615

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 13 additions & 17 deletions src/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
BrowsersListOpts,
} from "./types";
import { TargetNameMappings } from "./constants";
import { RulesLookup, lookupKey } from "./rules-lookup";

/*
3) Figures out which browsers user is targeting
Expand Down Expand Up @@ -51,13 +52,13 @@ function checkNotInsideIfStatementAndReport(
export function lintCallExpression(
context: Context,
handleFailingRule: HandleFailingRule,
rules: AstMetadataApiWithTargetsResolver[],
rulesByObject: RulesLookup,
sourceCode: SourceCode,
node: ESLintNode
) {
if (!node.callee) return;
const calleeName = node.callee.name;
const failingRule = rules.find((rule) => rule.object === calleeName);
const failingRule = rulesByObject.get(calleeName);
if (failingRule)
checkNotInsideIfStatementAndReport(
context,
Expand All @@ -71,13 +72,13 @@ export function lintCallExpression(
export function lintNewExpression(
context: Context,
handleFailingRule: HandleFailingRule,
rules: Array<AstMetadataApiWithTargetsResolver>,
rulesByObject: RulesLookup,
sourceCode: SourceCode,
node: ESLintNode
) {
if (!node.callee) return;
const calleeName = node.callee.name;
const failingRule = rules.find((rule) => rule.object === calleeName);
const failingRule = rulesByObject.get(calleeName);
if (failingRule)
checkNotInsideIfStatementAndReport(
context,
Expand All @@ -91,14 +92,12 @@ export function lintNewExpression(
export function lintExpressionStatement(
context: Context,
handleFailingRule: HandleFailingRule,
rules: AstMetadataApiWithTargetsResolver[],
rulesByObject: RulesLookup,
sourceCode: SourceCode,
node: ESLintNode
) {
if (!node?.expression?.name) return;
const failingRule = rules.find(
(rule) => rule.object === node?.expression?.name
);
const failingRule = rulesByObject.get(node?.expression?.name);
if (failingRule)
checkNotInsideIfStatementAndReport(
context,
Expand Down Expand Up @@ -135,7 +134,8 @@ function protoChainFromMemberExpression(node: ESLintNode): string[] {
export function lintMemberExpression(
context: Context,
handleFailingRule: HandleFailingRule,
rules: Array<AstMetadataApiWithTargetsResolver>,
rulesByProtoChainId: RulesLookup,
rulesByObjectProperty: RulesLookup,
sourceCode: SourceCode,
node: ESLintNode
) {
Expand All @@ -152,9 +152,7 @@ export function lintMemberExpression(
? rawProtoChain.slice(1)
: rawProtoChain;
const protoChainId = protoChain.join(".");
const failingRule = rules.find(
(rule) => rule.protoChainId === protoChainId
);
const failingRule = rulesByProtoChainId.get(protoChainId);
if (failingRule) {
checkNotInsideIfStatementAndReport(
context,
Expand All @@ -167,11 +165,9 @@ export function lintMemberExpression(
} else {
const objectName = node.object.name;
const propertyName = node.property.name;
const failingRule = rules.find(
(rule) =>
rule.object === objectName &&
(rule.property == null || rule.property === propertyName)
);
const failingRule =
rulesByObjectProperty.get(lookupKey(objectName, null)) ||
rulesByObjectProperty.get(lookupKey(objectName, propertyName));
if (failingRule)
checkNotInsideIfStatementAndReport(
context,
Expand Down
35 changes: 35 additions & 0 deletions src/rules-lookup.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import { AstMetadataApiWithTargetsResolver } from "./types";

// https://stackoverflow.com/q/49752151/25507
type KeysOfType<T, TProp> = keyof T &
{ [P in keyof T]: T[P] extends TProp ? P : never }[keyof T];

export type RulesLookup = Map<
string | undefined,
AstMetadataApiWithTargetsResolver
>;

export function lookupKey(...args: Array<string | null | undefined>) {
return args.map((i) => (i == null ? null : i)).join("\0");
}

export function makeLookup(
rules: AstMetadataApiWithTargetsResolver[],
...keys: Array<
KeysOfType<Required<AstMetadataApiWithTargetsResolver>, string>
>
) {
const lookup = new Map<
string | undefined,
AstMetadataApiWithTargetsResolver
>();
// Iterate in inverse order to favor earlier rules in case of conflict.
for (let i = rules.length - 1; i >= 0; i--) {
const key =
keys.length === 1
? rules[i][keys[0]]
: lookupKey(...keys.map((k) => rules[i][k]));
lookup.set(key, rules[i]);
}
return lookup;
}
64 changes: 48 additions & 16 deletions src/rules/compat.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import {
BrowsersListOpts,
} from "../types";
import { nodes } from "../providers";
import { RulesLookup, makeLookup } from "../rules-lookup";

type ESLint = {
[astNodeTypeName: string]: (node: ESLintNode) => void;
Expand Down Expand Up @@ -112,29 +113,63 @@ type RulesFilteredByTargets = {
ExpressionStatement: AstMetadataApiWithTargetsResolver[];
};

type RuleLookupsSet = {
CallExpressionsByObject: RulesLookup;
NewExpressionsByObject: RulesLookup;
ExpressionStatementsByObject: RulesLookup;
MemberExpressionsByProtoChainId: RulesLookup;
MemberExpressionsByObjectProperty: RulesLookup;
};

/**
* A small optimization that only lints APIs that are not supported by targeted browsers.
* For example, if the user is targeting chrome 50, which supports the fetch API, it is
* wasteful to lint calls to fetch.
*/
const getRulesForTargets = memoize(
(targetsJSON: string, lintAllEsApis: boolean): RulesFilteredByTargets => {
const result = {
CallExpression: [] as AstMetadataApiWithTargetsResolver[],
NewExpression: [] as AstMetadataApiWithTargetsResolver[],
MemberExpression: [] as AstMetadataApiWithTargetsResolver[],
ExpressionStatement: [] as AstMetadataApiWithTargetsResolver[],
(targetsJSON: string, lintAllEsApis: boolean): RuleLookupsSet => {
const rules: RulesFilteredByTargets = {
CallExpression: [],
NewExpression: [],
MemberExpression: [],
ExpressionStatement: [],
};
const targets = JSON.parse(targetsJSON);

nodes
.filter((node) => (lintAllEsApis ? true : node.kind !== "es"))
.forEach((node) => {
if (!node.getUnsupportedTargets(node, targets).length) return;
result[node.astNodeType].push(node);
rules[node.astNodeType].push(node);
});

return result;
const expressionStatementRules = [
...rules.MemberExpression,
...rules.CallExpression,
];
const memberExpressionRules = [
...rules.MemberExpression,
...rules.CallExpression,
...rules.NewExpression,
];

return {
CallExpressionsByObject: makeLookup(rules.CallExpression, "object"),
NewExpressionsByObject: makeLookup(rules.NewExpression, "object"),
ExpressionStatementsByObject: makeLookup(
expressionStatementRules,
"object"
),
MemberExpressionsByProtoChainId: makeLookup(
memberExpressionRules,
"protoChainId"
),
MemberExpressionsByObjectProperty: makeLookup(
memberExpressionRules,
"object",
"property"
),
};
}
);

Expand Down Expand Up @@ -220,32 +255,29 @@ export default {
null,
context,
handleFailingRule,
targetedRules.CallExpression,
targetedRules.CallExpressionsByObject,
sourceCode
),
NewExpression: lintNewExpression.bind(
null,
context,
handleFailingRule,
targetedRules.NewExpression,
targetedRules.NewExpressionsByObject,
sourceCode
),
ExpressionStatement: lintExpressionStatement.bind(
null,
context,
handleFailingRule,
[...targetedRules.MemberExpression, ...targetedRules.CallExpression],
targetedRules.ExpressionStatementsByObject,
sourceCode
),
MemberExpression: lintMemberExpression.bind(
null,
context,
handleFailingRule,
[
...targetedRules.MemberExpression,
...targetedRules.CallExpression,
...targetedRules.NewExpression,
],
targetedRules.MemberExpressionsByProtoChainId,
targetedRules.MemberExpressionsByObjectProperty,
sourceCode
),
// Keep track of all the defined variables. Do not report errors for nodes that are not defined
Expand Down
8 changes: 4 additions & 4 deletions test/e2e.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -542,7 +542,7 @@ ruleTester.run("compat", rule, {
settings: { browsers: ["ie 10"] },
errors: [
{
message: "Promise.resolve() is not supported in IE 10",
message: "Promise is not supported in IE 10",
type: "MemberExpression",
},
],
Expand All @@ -552,7 +552,7 @@ ruleTester.run("compat", rule, {
settings: { browsers: ["ie 10"] },
errors: [
{
message: "Promise.all() is not supported in IE 10",
message: "Promise is not supported in IE 10",
type: "MemberExpression",
},
],
Expand All @@ -562,7 +562,7 @@ ruleTester.run("compat", rule, {
settings: { browsers: ["ie 10"] },
errors: [
{
message: "Promise.race() is not supported in IE 10",
message: "Promise is not supported in IE 10",
type: "MemberExpression",
},
],
Expand All @@ -572,7 +572,7 @@ ruleTester.run("compat", rule, {
settings: { browsers: ["ie 10"] },
errors: [
{
message: "Promise.reject() is not supported in IE 10",
message: "Promise is not supported in IE 10",
type: "MemberExpression",
},
],
Expand Down