Skip to content

Commit 8c5faf2

Browse files
committed
fix(rule): preserve guarded numeric lookups
1 parent 2093b69 commit 8c5faf2

2 files changed

Lines changed: 135 additions & 1 deletion

File tree

‎packages/oxlint-plugin-react-doctor/src/plugin/rules/js-performance/js-set-map-lookups.regressions.test.ts‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -161,6 +161,38 @@ describe("js-performance/js-set-map-lookups — regressions", () => {
161161
);
162162
});
163163

164+
it("flags a numeric `.indexOf()` query protected by a relational loop guard", () => {
165+
expectFail(`
166+
function f(joinable: number[], from: number, to: number) {
167+
for (let position = from; position <= to; position += 1) {
168+
if (joinable.indexOf(position) === -1) joinable.push(position);
169+
}
170+
}
171+
`);
172+
});
173+
174+
it("does not use a relational loop guard after the query binding is reassigned", () => {
175+
expectPass(`
176+
function f(joinable: number[], from: number, to: number) {
177+
for (let position = from; position <= to; position += 1) {
178+
position = Number.NaN;
179+
if (joinable.indexOf(position) === -1) joinable.push(position);
180+
}
181+
}
182+
`);
183+
});
184+
185+
it("does not use an outer relational loop guard for a shadowed query binding", () => {
186+
expectPass(`
187+
function f(joinable: number[], from: number, to: number) {
188+
for (let position = from; position <= to; position += 1) {
189+
const read = (position: number) => joinable.indexOf(position) === -1;
190+
if (read(Number.NaN)) return position;
191+
}
192+
}
193+
`);
194+
});
195+
164196
it("does not flag `.indexOf(undefined)` because sparse holes become undefined in a Set", () => {
165197
expectPass(
166198
`function f(rows, allowedValues: Array<number | undefined>){ for (const row of rows){ if(allowedValues.indexOf(undefined) !== -1) return row; } }`,

‎packages/oxlint-plugin-react-doctor/src/plugin/rules/js-performance/js-set-map-lookups.ts‎

Lines changed: 103 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,11 +6,14 @@ import type { EsTreeNode } from "../../utils/es-tree-node.js";
66
import type { EsTreeNodeOfType } from "../../utils/es-tree-node-of-type.js";
77
import { findProgramRoot } from "../../utils/find-program-root.js";
88
import { findVariableInitializer } from "../../utils/find-variable-initializer.js";
9+
import { getRangeStart } from "../../utils/get-range-start.js";
910
import { isInlineFunctionExpression } from "../../utils/is-inline-function-expression.js";
11+
import { isFunctionLike } from "../../utils/is-function-like.js";
1012
import { isNodeOfType } from "../../utils/is-node-of-type.js";
1113
import type { RuleContext } from "../../utils/rule-context.js";
1214
import type { RuleVisitors } from "../../utils/rule-visitors.js";
1315
import { stripParenExpression } from "../../utils/strip-paren-expression.js";
16+
import { walkAst } from "../../utils/walk-ast.js";
1417

1518
// HACK: methods that ALWAYS return a string when called on a string
1619
// receiver. Used to recognize `.toLowerCase().includes(x)` chains as
@@ -737,6 +740,105 @@ const isNativeIterationIndex = (identifier: EsTreeNodeOfType<"Identifier">): boo
737740
return isNodeOfType(callbackCall, "CallExpression") && isIterationCallbackCall(callbackCall);
738741
};
739742

743+
const hasSameIdentifierBinding = (
744+
leftIdentifier: EsTreeNodeOfType<"Identifier">,
745+
rightIdentifier: EsTreeNodeOfType<"Identifier">,
746+
): boolean => {
747+
if (leftIdentifier.name !== rightIdentifier.name) return false;
748+
const leftBinding = findVariableInitializer(leftIdentifier, leftIdentifier.name);
749+
const rightBinding = findVariableInitializer(rightIdentifier, rightIdentifier.name);
750+
return Boolean(
751+
leftBinding && rightBinding && leftBinding.bindingIdentifier === rightBinding.bindingIdentifier,
752+
);
753+
};
754+
755+
const NON_NAN_RELATIONAL_OPERATORS: ReadonlySet<string> = new Set(["<", "<=", ">", ">="]);
756+
757+
const testProvesIdentifierIsNotNaN = (
758+
test: EsTreeNode | null | undefined,
759+
identifier: EsTreeNodeOfType<"Identifier">,
760+
): boolean => {
761+
if (!test) return false;
762+
const strippedTest = stripParenExpression(test);
763+
if (isNodeOfType(strippedTest, "LogicalExpression") && strippedTest.operator === "&&") {
764+
return (
765+
testProvesIdentifierIsNotNaN(strippedTest.left, identifier) ||
766+
testProvesIdentifierIsNotNaN(strippedTest.right, identifier)
767+
);
768+
}
769+
if (
770+
!isNodeOfType(strippedTest, "BinaryExpression") ||
771+
!NON_NAN_RELATIONAL_OPERATORS.has(strippedTest.operator)
772+
) {
773+
return false;
774+
}
775+
return (
776+
(isNodeOfType(strippedTest.left, "Identifier") &&
777+
hasSameIdentifierBinding(strippedTest.left, identifier)) ||
778+
(isNodeOfType(strippedTest.right, "Identifier") &&
779+
hasSameIdentifierBinding(strippedTest.right, identifier))
780+
);
781+
};
782+
783+
const writeTargetContainsIdentifierBinding = (
784+
writeTarget: EsTreeNode,
785+
identifier: EsTreeNodeOfType<"Identifier">,
786+
): boolean => {
787+
let containsBinding = false;
788+
walkAst(writeTarget, (child) => {
789+
if (isNodeOfType(child, "Identifier") && hasSameIdentifierBinding(child, identifier)) {
790+
containsBinding = true;
791+
return false;
792+
}
793+
});
794+
return containsBinding;
795+
};
796+
797+
const hasWriteBeforeQuery = (
798+
body: EsTreeNode,
799+
identifier: EsTreeNodeOfType<"Identifier">,
800+
): boolean => {
801+
const queryStart = getRangeStart(identifier);
802+
if (queryStart === null) return true;
803+
let hasEarlierWrite = false;
804+
walkAst(body, (child) => {
805+
if (hasEarlierWrite) return false;
806+
const childStart = getRangeStart(child);
807+
if (childStart !== null && childStart >= queryStart) return false;
808+
if (child !== body && isFunctionLike(child)) return false;
809+
const writeTarget = isNodeOfType(child, "AssignmentExpression")
810+
? child.left
811+
: isNodeOfType(child, "UpdateExpression")
812+
? child.argument
813+
: isNodeOfType(child, "ForInStatement") || isNodeOfType(child, "ForOfStatement")
814+
? child.left
815+
: null;
816+
if (writeTarget && writeTargetContainsIdentifierBinding(writeTarget, identifier)) {
817+
hasEarlierWrite = true;
818+
return false;
819+
}
820+
});
821+
return hasEarlierWrite;
822+
};
823+
824+
const isProtectedByRelationalLoopGuard = (identifier: EsTreeNodeOfType<"Identifier">): boolean => {
825+
let descendant: EsTreeNode = identifier;
826+
let ancestor: EsTreeNode | null | undefined = identifier.parent;
827+
while (ancestor) {
828+
if (isFunctionLike(ancestor)) return false;
829+
if (
830+
(isNodeOfType(ancestor, "ForStatement") || isNodeOfType(ancestor, "WhileStatement")) &&
831+
ancestor.body === descendant &&
832+
testProvesIdentifierIsNotNaN(ancestor.test, identifier)
833+
) {
834+
return !hasWriteBeforeQuery(ancestor.body, identifier);
835+
}
836+
descendant = ancestor;
837+
ancestor = ancestor.parent;
838+
}
839+
return false;
840+
};
841+
740842
const isKnownSafeIndexOfQuery = (query: EsTreeNode | null | undefined): boolean => {
741843
if (!query) return false;
742844
const strippedQuery = stripParenExpression(query);
@@ -745,7 +847,7 @@ const isKnownSafeIndexOfQuery = (query: EsTreeNode | null | undefined): boolean
745847
}
746848
if (!isNodeOfType(strippedQuery, "Identifier")) return false;
747849
if (isNativeIterationIndex(strippedQuery)) return true;
748-
return false;
850+
return isProtectedByRelationalLoopGuard(strippedQuery);
749851
};
750852

751853
const findSameFileTypeAlias = (

0 commit comments

Comments
 (0)