mirror of
https://github.com/morluto/rea
synced 2026-10-25 22:08:11 -04:00
fix(javascript): evaluate each short-circuit left operand once (#7126)
Logical branch selection evaluated the left operand to decide which branches stay reachable, and value evaluation then evaluated the selected left branch again. In a left-nested `a || b || c ...` chain every level repeated that, so semantic analysis doubled per operand and a 24-operand chain exhausted the per-source deadline. The selection now carries the left operand's value with the branch, and value evaluation reuses it. Provenance still walks the branch nodes. Results are unchanged. Fixes #7115 Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
@@ -1,8 +1,14 @@
|
||||
import * as t from "@babel/types";
|
||||
import { expect, it, vi } from "vitest";
|
||||
|
||||
import { analyzeJavaScriptSemantics } from "./javascriptSemanticAnalysis.js";
|
||||
import {
|
||||
analyzeJavaScriptSemantics,
|
||||
analyzeParsedJavaScriptSemantics,
|
||||
} from "./javascriptSemanticAnalysis.js";
|
||||
import { onlyBinding } from "./javascriptSemanticAnalysis.fixture.js";
|
||||
import type { JavaScriptSemanticIr } from "./javascriptSemanticIr.js";
|
||||
import { traverseJavaScriptAst } from "./javascriptSemanticTraversal.js";
|
||||
import { parseJavaScriptSource } from "./javascriptSourceParser.js";
|
||||
|
||||
/** Count AST ancestry lookups made while analyzing one source. */
|
||||
const ancestryReads = (
|
||||
@@ -145,3 +151,97 @@ it.each([
|
||||
expect(larger).toBeLessThan(8 * smaller);
|
||||
},
|
||||
);
|
||||
|
||||
/** Count reads of short-circuit left operands while analyzing one source. */
|
||||
const logicalOperandReads = (
|
||||
source: string,
|
||||
inspect: (ir: JavaScriptSemanticIr) => void,
|
||||
): number => {
|
||||
const file = parseJavaScriptSource(source);
|
||||
if (file === null) throw new Error("fixture must parse");
|
||||
let reads = 0;
|
||||
traverseJavaScriptAst(file, {
|
||||
enter: (node) => {
|
||||
if (!t.isLogicalExpression(node)) return;
|
||||
const left = node.left;
|
||||
Object.defineProperty(node, "left", {
|
||||
enumerable: true,
|
||||
configurable: true,
|
||||
get: () => {
|
||||
reads++;
|
||||
return left;
|
||||
},
|
||||
});
|
||||
},
|
||||
});
|
||||
reads = 0;
|
||||
inspect(analyzeParsedJavaScriptSemantics(file));
|
||||
return reads;
|
||||
};
|
||||
|
||||
/** A left-nested chain `o0 op1 o1 op2 o2 ...` as written in tokenizers. */
|
||||
const logicalChain = (
|
||||
length: number,
|
||||
operand: (index: number) => string,
|
||||
operator: (index: number) => string,
|
||||
): string =>
|
||||
Array.from({ length: length - 1 }, (unused, index) => index + 1).reduce(
|
||||
(chain, index) => `(${chain}) ${operator(index)} ${operand(index)}`,
|
||||
operand(0),
|
||||
);
|
||||
|
||||
const call = (index: number): string => `f${String(index)}(s)`;
|
||||
|
||||
it.each([
|
||||
["||", call, () => "||"],
|
||||
["&&", (index: number) => `s.k${String(index)}`, () => "&&"],
|
||||
["??", call, () => "??"],
|
||||
[
|
||||
"mixed ||/&&/?? and conditional",
|
||||
(index: number) =>
|
||||
index % 4 === 0
|
||||
? `(s.k ? ${call(index)} : s.k${String(index)})`
|
||||
: call(index),
|
||||
(index: number) => ["||", "&&", "??"][index % 3] ?? "||",
|
||||
],
|
||||
])(
|
||||
"reads each operand of a long %s chain a bounded number of times",
|
||||
(_, operand, operator) => {
|
||||
// Logical branch selection evaluates the left operand. Evaluating it again
|
||||
// as a value branch at every level doubles the work per operand.
|
||||
const reads = (length: number): number =>
|
||||
logicalOperandReads(
|
||||
`function next(s){ var tok=${logicalChain(length, operand, operator)}; return tok; } module.exports=next;`,
|
||||
(ir) => {
|
||||
expect(ir.coverage).toEqual({ status: "complete", omittedCount: 0 });
|
||||
expect(onlyBinding(ir, "tok").value.status).not.toBe("literal");
|
||||
},
|
||||
);
|
||||
|
||||
const smaller = reads(8);
|
||||
const larger = reads(16);
|
||||
expect(smaller).toBeGreaterThan(0);
|
||||
// Doubling per operand would multiply the reads by 256.
|
||||
expect(larger).toBeLessThan(4 * smaller);
|
||||
},
|
||||
);
|
||||
|
||||
it("keeps established logical operands across a long chain", () => {
|
||||
// Known-falsy operands select the right branch at every level; the value and
|
||||
// provenance must still come from the one reachable operand.
|
||||
const ir = analyzeJavaScriptSemantics(
|
||||
`const tok = ${'false || null || 0 || "" || '.repeat(10)}require("electron").app;`,
|
||||
);
|
||||
expect(ir.coverage).toEqual({ status: "complete", omittedCount: 0 });
|
||||
expect(onlyBinding(ir, "tok").provenance).toEqual({
|
||||
status: "module",
|
||||
origins: [
|
||||
{
|
||||
specifier: "electron",
|
||||
importedPath: ["app"],
|
||||
resolutionMode: "require",
|
||||
},
|
||||
],
|
||||
reason: null,
|
||||
});
|
||||
});
|
||||
|
||||
@@ -721,8 +721,10 @@ const evaluateExpressionValue = (
|
||||
if (t.isTemplateLiteral(node)) return evaluateTemplate(node, context);
|
||||
if (t.isConditionalExpression(node) || t.isLogicalExpression(node))
|
||||
return mergeValues(
|
||||
semanticSelectedBranches(node, context).map((branch) =>
|
||||
evaluateExpression(branch, nestedContext(context)),
|
||||
semanticSelectedBranches(node, context).map(
|
||||
(branch) =>
|
||||
branch.value ??
|
||||
evaluateExpression(branch.node, nestedContext(context)),
|
||||
),
|
||||
);
|
||||
if (t.isObjectExpression(node)) return evaluateObject(node, context);
|
||||
@@ -774,27 +776,39 @@ const evaluateTemplate = (
|
||||
return primitiveSet(candidates);
|
||||
};
|
||||
|
||||
/**
|
||||
* A branch that may produce the expression's value. A logical left operand
|
||||
* carries the value its selection already evaluated, so value consumers reuse
|
||||
* it instead of re-evaluating that operand; re-evaluating it at every level of
|
||||
* a left-nested `a || b || c` chain doubles the work per operand.
|
||||
*/
|
||||
interface SemanticSelectedBranch {
|
||||
readonly node: t.Node;
|
||||
readonly value?: JavaScriptSemanticValue;
|
||||
}
|
||||
|
||||
/** Value and provenance selection consume the same established branch facts. */
|
||||
const semanticSelectedBranches = (
|
||||
node: t.ConditionalExpression | t.LogicalExpression,
|
||||
context: EvaluationContext,
|
||||
): readonly t.Node[] => {
|
||||
): readonly SemanticSelectedBranch[] => {
|
||||
if (t.isConditionalExpression(node)) {
|
||||
const test = evaluateExpression(node.test, nestedContext(context));
|
||||
const choice = semanticCondition(test, Boolean);
|
||||
return choice === undefined
|
||||
? [node.consequent, node.alternate]
|
||||
: [choice ? node.consequent : node.alternate];
|
||||
? [{ node: node.consequent }, { node: node.alternate }]
|
||||
: [{ node: choice ? node.consequent : node.alternate }];
|
||||
}
|
||||
const left = evaluateExpression(node.left, nestedContext(context));
|
||||
const value = evaluateExpression(node.left, nestedContext(context));
|
||||
const left = { node: node.left, value };
|
||||
const choice = logicalRightOperand(
|
||||
node.operator,
|
||||
semanticCondition(left, Boolean),
|
||||
semanticCondition(left, (value) => value === null, false),
|
||||
semanticCondition(value, Boolean),
|
||||
semanticCondition(value, (candidate) => candidate === null, false),
|
||||
);
|
||||
return choice === undefined
|
||||
? [node.left, node.right]
|
||||
: [choice ? node.right : node.left];
|
||||
? [left, { node: node.right }]
|
||||
: [choice ? { node: node.right } : left];
|
||||
};
|
||||
|
||||
/** All retained alternatives must agree before discarding a branch. */
|
||||
@@ -1320,7 +1334,7 @@ const provenanceForExpression = (
|
||||
}
|
||||
if (t.isConditionalExpression(node) || t.isLogicalExpression(node)) {
|
||||
const candidates = semanticSelectedBranches(node, context).map((branch) =>
|
||||
provenanceForExpression(branch, nestedContext(context)),
|
||||
provenanceForExpression(branch.node, nestedContext(context)),
|
||||
);
|
||||
const origins = uniqueSemanticOrigins(
|
||||
candidates.flatMap((candidate) => candidate.origins),
|
||||
|
||||
@@ -0,0 +1,52 @@
|
||||
import { writeFile } from "node:fs/promises";
|
||||
import { join } from "node:path";
|
||||
import { expect } from "vitest";
|
||||
import { z } from "zod";
|
||||
|
||||
import { javascriptApplicationAnalysisResultSchema } from "../../../src/domain/javascript/javascriptApplicationAnalysis.js";
|
||||
import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js";
|
||||
import { cliTest } from "../../support/cli/cliFixture.js";
|
||||
|
||||
// Hand-written tokenizers dispatch through long short-circuit chains; the
|
||||
// CodeMirror 5 pug mode's `nextToken` has 38 operands. Work that doubled per
|
||||
// operand could not finish such a chain within any per-source deadline.
|
||||
const operands = 38;
|
||||
|
||||
cliTest(
|
||||
"analyzes a long short-circuit dispatch chain within the source deadline",
|
||||
async ({ cli }) => {
|
||||
const root = await createTestTempDirectory("rea-js-logical-chain-");
|
||||
const chain = Array.from(
|
||||
{ length: operands },
|
||||
(unused, index) => `f${String(index)}(s)`,
|
||||
).join(" || ");
|
||||
await writeFile(
|
||||
join(root, "index.js"),
|
||||
`function next(s) {\n var tok = ${chain};\n return tok;\n}\nmodule.exports = next;\n`,
|
||||
);
|
||||
await writeFile(
|
||||
join(root, "package.json"),
|
||||
'{"name":"chain","version":"1.0.0","main":"index.js"}\n',
|
||||
);
|
||||
const output = await cli.run({
|
||||
arguments: [
|
||||
"analyze-javascript-application",
|
||||
root,
|
||||
"--analysis-timeout-ms",
|
||||
"15000",
|
||||
"--json",
|
||||
],
|
||||
});
|
||||
expect(output.exitCode).toBe(0);
|
||||
const result = z
|
||||
.object({ normalized_result: javascriptApplicationAnalysisResultSchema })
|
||||
.parse(output.json).normalized_result;
|
||||
expect(result.statistics).toMatchObject({
|
||||
parsed_javascript_files: 1,
|
||||
parse_failures: 0,
|
||||
});
|
||||
expect(
|
||||
result.semantic_graph.nodes.filter(({ kind }) => kind === "call-site"),
|
||||
).toHaveLength(operands);
|
||||
},
|
||||
);
|
||||
Reference in New Issue
Block a user