Skip to content

Commit 4b0b243

Browse files
committed
fix(observability-map): read a branch arm's exit through the liveness fold
selectsADistinctPath decided whether an if or a switch in a catch clause made a real classification decision by asking containsExit, a plain containment walk. Containment is true of an exit that can never run, so catch (e) { if (e instanceof Error) { if (false) { return null; } } return json(x, { status: 500 }); } read as a decision while the same clause without the if read as a swallow: 50 points a route for a behaviour-preserving mechanical edit. Measured over apps/webapp/app/routes, that shape took the tree from 19 to 27 and raised 80 of 412 routes. An earlier wave had already moved catchClauseEvidence's exited flag onto containsLiveExit for the same eleven dead spellings. The branch predicate 120 lines below it kept the containment read, so this is one rule fixed in one place and left in its sibling. The three exit reads now go through containsLiveExit and containsExit is deleted, so there is one exit read in the file. The property that makes one helper safe for two callers reading it for opposite purposes is now written down on containsLiveWhere: it is strictly subtractive against containment, so it only ever un-blinds the exited flag and only ever withholds a branch grant. Conservatism is a property of the helper plus what the caller does with a true, and auditing it at the definition is how this was missed. Adds dead-armed-instanceof-if to the mutation corpus, additive class, and dead-conjunction-instanceof-if under KNOWN_GAPS. The second is the sibling this fix does not close: folding the arm does not fold a dead condition, and if (e instanceof Error && false) reaches the same grant for the same 80 routes. literalTruth treats && and || as always null on purpose, so closing it means widening that fold, a different rule with its own measurement. Recorded and running rather than left to be rediscovered. Requiring the arm to definitelyExits was measured and rejected: it accuses admin.api.v1.orgs.$organizationId.environments.staging.ts, which classifies Prisma's P2002 and rethrows everything else, of taking one way out regardless of what was thrown. Pinned by 'still credits an arm guarded by a condition that does not fold'. The real tree does not move: every route's score and every check's status are byte-identical before and after, global 19 either way.
1 parent 2de22e4 commit 4b0b243

4 files changed

Lines changed: 164 additions & 22 deletions

File tree

internal-packages/observability-map/src/mutationCorpus.test.ts

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,8 +57,19 @@ const ENABLED = process.env.OBS_MAP_MUTATION_CORPUS === "1";
5757
* flag raised before each statement's own branch check, which makes every deciding statement refuse
5858
* itself. Raising it after is byte-identical on the real tree and closes the shape, so the entry is
5959
* defended now and the `if (true)` family needed no condition folding after all.
60+
*
61+
* `dead-conjunction-instanceof-if` is the sibling of `dead-armed-instanceof-if` that the arm-liveness
62+
* fix does not close. `selectsADistinctPath` now folds a dead ARM; a dead CONDITION still reaches
63+
* the grant, and `e instanceof Error && false` is exactly that, a guard that references the caught
64+
* binding and can never be true. No fold in `scan.ts` can see it, because `literalTruth` treats
65+
* `&&` and `||` as always null on purpose so that a live guard can never be read as dead. Widening
66+
* that fold is a different rule from the one this round fixed and needs its own measurement, so the
67+
* shape is recorded and running rather than left for the next person to rediscover.
6068
*/
61-
const KNOWN_GAPS = new Set<string>(["dead-classifying-try-with-call"]);
69+
const KNOWN_GAPS = new Set<string>([
70+
"dead-classifying-try-with-call",
71+
"dead-conjunction-instanceof-if",
72+
]);
6273

6374
type SourceFile = { relativeName: string; source: string };
6475

internal-packages/observability-map/src/mutations.ts

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -843,6 +843,34 @@ export const MUTATIONS: Mutation[] = [
843843
(e) =>
844844
`if (false) { if (${e} instanceof Error) { return new Response(null, { status: 400 }); } } else { 0; }`
845845
),
846+
// The sibling of `empty-instanceof-if`. That entry's arm is empty; this one's arm holds an exit
847+
// that can never run, which is the same no-op written so that a containment read cannot tell the
848+
// difference. `selectsADistinctPath` asked a plain containment question, true of
849+
// `if (false) { return null; }`, so the test read as a real classification decision and turned a
850+
// swallowing catch into a passing one: 80 routes and the tree from 19 to 27 when measured.
851+
// Additive: it plants fake signal. `catchClauseEvidence`'s own `exited` flag had already been
852+
// moved onto `containsLiveExit` for exactly this reason and the branch predicate beside it was
853+
// left behind, which is why the shape is spelled with the corpus's own `if (false)` and not
854+
// something exotic.
855+
prependToEveryCatch(
856+
"dead-armed-instanceof-if",
857+
"preserving",
858+
"splice if (e instanceof Error) { if (false) { return null; } } into every catch",
859+
(e) => `if (${e} instanceof Error) { if (false) { return null; } }`
860+
),
861+
// The sibling the entry above does NOT close, found while closing it and filed here rather than
862+
// fixed. Moving the ARM's exit read onto `containsLiveExit` folds a dead arm; it does not fold a
863+
// dead CONDITION, and a condition that both references the caught binding and is provably false
864+
// reaches the same grant. `literalTruth` cannot see it: `&&` and `||` are documented there as
865+
// always null, deliberately, so `e instanceof Error && false` is an undecidable guard to every
866+
// fold in the file. Closing it means widening that fold, which is a different rule with its own
867+
// measurement, so this runs as a `KNOWN_GAPS` expected failure instead of sitting unrecorded.
868+
prependToEveryCatch(
869+
"dead-conjunction-instanceof-if",
870+
"preserving",
871+
"splice if (e instanceof Error && false) { return null; } into every catch",
872+
(e) => `if (${e} instanceof Error && false) { return null; }`
873+
),
846874
// A finally that leaves itself by `break` cancels the try's completion, so nothing hosted in
847875
// that tryBlock ever escapes the clause: the whole statement is a no-op. The walk's
848876
// catchless-try entry credited it anyway, minting a branch from the hosted classifier on 80
@@ -1087,6 +1115,8 @@ export const ADDITIVE_IDS = [
10871115
"wrap-body-in-rethrow",
10881116
"wrap-body-in-same-arms-throw-ternary",
10891117
"empty-instanceof-if",
1118+
"dead-armed-instanceof-if",
1119+
"dead-conjunction-instanceof-if",
10901120
"dead-classifier-one-arm",
10911121
"dead-throw-in-cancelled-try",
10921122
"dead-deciding-map",

internal-packages/observability-map/src/scan.test.ts

Lines changed: 83 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -759,7 +759,7 @@ describe("scanFile: catch clause evidence", () => {
759759
});
760760

761761
// The mirror of the family above. Each dead spelling earns nothing, and it must also COST
762-
// nothing: `containsExit` was true of the dead statement itself, so prepending one raised the
762+
// nothing: a plain containment read was true of the dead statement itself, so prepending one raised the
763763
// `exited` flag and blinded the walk to the real classification below it, turning a pass into a
764764
// swallow verdict on 78 real routes. `containsLiveExit` folds the literal guard and sees no live
765765
// exit, so the deciding statements keep their credit. The spellings are the CORPUS spellings
@@ -2470,6 +2470,88 @@ describe("a ternary on the error has to send its arms somewhere different", () =
24702470
});
24712471
});
24722472

2473+
// The liveness gap in the branch predicate. `selectsADistinctPath` asked a plain containment
2474+
// question, so an arm holding an exit that can never run read as an arm that takes the error
2475+
// somewhere. `catch (e) { if (e instanceof Error) { if (false) { return null; } } return json(x,
2476+
// { status: 500 }); }` is the same swallow as the clause without the `if`, and it was worth 50
2477+
// points a route. The same eleven dead spellings had already been folded out of
2478+
// `catchClauseEvidence`'s `exited` flag by `containsLiveExit`, and this predicate beside it kept
2479+
// the containment read. `dead-armed-instanceof-if` in the mutation corpus is the tree-scale
2480+
// version: global 19 -> 27 and 80 routes raised, before the fix.
2481+
describe("an arm whose only exit is dead decides nothing", () => {
2482+
const swallow = (mutation: string) => `
2483+
export async function loader() {
2484+
try {
2485+
return await prisma.thing.findMany();
2486+
} catch (error) {
2487+
${mutation}
2488+
return json({ error: "generic" }, { status: 500 });
2489+
}
2490+
}
2491+
`;
2492+
2493+
it("reads the unmutated clause as a swallow, as a baseline", () => {
2494+
const ep = scanFile("x.ts", swallow(""));
2495+
expect(ep!.catches[0]!.branches).toBe(false);
2496+
});
2497+
2498+
// The reported shape, plus three further spellings of the same no-op reaching the same
2499+
// predicate by different routes: a dead loop body, a dead else arm, and a switch clause.
2500+
const DEAD_ARMS: Array<[string, string]> = [
2501+
["an if (false) arm", "if (error instanceof Error) { if (false) { return null; } }"],
2502+
["a while (false) body", "if (error instanceof Error) { while (false) { throw error; } }"],
2503+
[
2504+
"a dead else arm beside an arm that goes nowhere",
2505+
"if (error instanceof Error) { doThing(); } else { for (const k in {}) { return null; } }",
2506+
],
2507+
[
2508+
"a switch clause whose exit is dead",
2509+
"switch (error.code) { case 'x': if (1 === 2) { throw error; } }",
2510+
],
2511+
];
2512+
2513+
for (const [label, mutation] of DEAD_ARMS) {
2514+
it(`does not credit ${label}`, () => {
2515+
const ep = scanFile("x.ts", swallow(mutation));
2516+
expect(ep!.catches[0]!.branches).toBe(false);
2517+
});
2518+
}
2519+
2520+
// The positive controls. The fold is subtractive against containment, so anything it cannot
2521+
// prove dead reads exactly as it did, including a guard whose truth is not decidable from the
2522+
// token alone. Without these the fix could be "always false" and the four cases above would
2523+
// still pass.
2524+
//
2525+
// `an arm guarded by a condition that does not fold` is also the pin on the alternative that was
2526+
// measured and rejected: asking the arm to `definitelyExits` rather than to hold a live exit.
2527+
// That is the "guaranteed" reading, it refuses all four shapes above, and it accuses
2528+
// `admin.api.v1.orgs.$organizationId.environments.staging.ts` on the real tree, taking the global
2529+
// from 19 to 18. That clause recognises Prisma's P2002, re-reads the conflicting row and returns
2530+
// `{ status: "updated" }`, rethrowing everything else: a textbook classification whose arm
2531+
// happens to fall through to the rethrow when the re-read finds nothing. Accusing it of taking
2532+
// one way out regardless of what was thrown is simply false, and a new false accusation is the
2533+
// direction that gets the tool switched off.
2534+
const LIVE_ARMS: Array<[string, string]> = [
2535+
["a plain returning arm", "if (error instanceof Error) { return badRequest(); }"],
2536+
[
2537+
"an arm guarded by a condition that does not fold",
2538+
"if (error instanceof Error) { if (error.code === 'P2002') { return conflict(); } }",
2539+
],
2540+
[
2541+
"a live exit in the else arm only",
2542+
"if (error instanceof Error) { doThing(); } else { return badRequest(); }",
2543+
],
2544+
["a switch clause that returns", "switch (error.code) { case 'P2002': return conflict(); }"],
2545+
];
2546+
2547+
for (const [label, mutation] of LIVE_ARMS) {
2548+
it(`still credits ${label}`, () => {
2549+
const ep = scanFile("x.ts", swallow(mutation));
2550+
expect(ep!.catches[0]!.branches).toBe(true);
2551+
});
2552+
}
2553+
});
2554+
24732555
// C4a. `export const { action, loader } = createActionApiRoute(...)` produced no entry point at
24742556
// all: `scanFile` skipped a non-identifier binding name at the export site, so the route was
24752557
// absent from the denominator rather than parsed, failed or unmeasured. The two-step spelling

internal-packages/observability-map/src/scan.ts

Lines changed: 39 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -650,15 +650,6 @@ function containsEscapingJump(node: ts.Node, jumps: BareJumps = ESCAPES): boolea
650650
return ts.forEachChild(node, (child) => containsEscapingJump(child, jumps)) === true;
651651
}
652652

653-
/** Whether the tree rooted at `node` contains a `return` or a `throw` of its own, not counting one
654-
* inside a nested function. What separates an arm that takes the error somewhere from an arm that
655-
* runs and falls back into the clause's single common exit. */
656-
function containsExit(node: ts.Node): boolean {
657-
if (ts.isFunctionLike(node)) return false;
658-
if (ts.isReturnStatement(node) || ts.isThrowStatement(node)) return true;
659-
return ts.forEachChild(node, containsExit) === true;
660-
}
661-
662653
/**
663654
* Literal truthiness of a guard expression: true, false, or null when not decidable from the
664655
* token alone. Only literal tokens fold; an identifier, call, bigint, `&&`, `||` or a template
@@ -713,13 +704,25 @@ function tryBlockMayThrow(block: ts.Block): boolean {
713704
}
714705

715706
/**
716-
* `containsExit`, minus exits that sit in a provably-untaken branch. `if (false) { throw e; }`
717-
* contains an exit and can never run one; treating it as an exit is what let a dead statement
718-
* blind the walk to the real classification below it, prepending one to a deciding clause turned
719-
* its pass into a swallow verdict on 78 real routes. Folds literal guards only, so an unknown
720-
* condition keeps the containsExit answer, which is the direction that refuses credit rather than
721-
* inventing it. The mirror twins under `dead and deferred code prepended to a deciding catch does
722-
* not blind it` hold the recovered half; the `BRANCH_EXITED` family holds the refusing half.
707+
* Whether the tree rooted at `root` contains a node `hit` accepts that a provably-untaken branch
708+
* does not already rule out. A plain containment walk, minus the hits it can prove never run:
709+
* `if (false) { throw e; }` contains a throw and can never run one.
710+
*
711+
* Folds literal guards only, so wherever `literalTruth` cannot decide, every hit the plain walk
712+
* would have found is still found. That makes this strictly subtractive against containment, which
713+
* is what lets both of its callers read it for opposite purposes:
714+
*
715+
* - `catchClauseEvidence`'s `exited` flag, where a hit BLINDS the walk to whatever follows.
716+
* Containment blinded it on a dead statement, so prepending one to a deciding clause turned its
717+
* pass into a swallow verdict on 78 real routes. Subtracting dead hits only ever un-blinds.
718+
* - `selectsADistinctPath`, where a hit GRANTS a branch. Containment granted one for an arm whose
719+
* only exit was dead, which is `dead-armed-instanceof-if` in the mutation corpus, measured at 80
720+
* routes and the tree from 19 to 27. Subtracting dead hits only ever withholds.
721+
*
722+
* The `exited` half is pinned by the mirror twins under `dead and deferred code prepended to a
723+
* deciding catch does not blind it` (recovered) and the `BRANCH_EXITED` family (refusing). The
724+
* `selectsADistinctPath` half is pinned by `an arm whose only exit is dead decides nothing` and its
725+
* siblings, plus the corpus entry.
723726
*/
724727
function containsLiveWhere(root: ts.Node, hit: (n: ts.Node) => boolean): boolean {
725728
const walk = (node: ts.Node): boolean => {
@@ -809,6 +812,16 @@ function containsLiveReturn(node: ts.Node): boolean {
809812
* An `if`/`else` whose two arms are textually identical does not count, the same comparison
810813
* `selectsAnErrorPath` makes of a ternary's arms.
811814
*
815+
* The exit an arm is credited for has to be a LIVE one, `containsLiveExit` and never a plain
816+
* containment read. `if (e instanceof Error) { if (false) { return null; } }` contains an exit that
817+
* can never run, so under containment it read as a real decision and took a swallowing catch to a
818+
* pass for the price of a mechanical edit: 80 routes and the tree from 19 to 27 when measured. The
819+
* same liveness rule had already been put on `catchClauseEvidence`'s `exited` flag, for the same
820+
* eleven dead spellings, and this predicate beside it kept the containment read. `an arm whose only
821+
* exit is dead decides nothing` and its siblings are the unit pins; `dead-armed-instanceof-if` in
822+
* the mutation corpus is the tree-scale version. Being subtractive against containment, the fold
823+
* can only ever withhold a branch, never invent one, so a live arm reads exactly as it did.
824+
*
812825
* The residual both branch tests share, stated here once for both: two arms that produce the same
813826
* outcome by different spellings still read as a real decision.
814827
* `if (e instanceof Error) { return json(x); } return Response.json(x);` counts and decides
@@ -822,11 +835,17 @@ function selectsADistinctPath(statement: ts.IfStatement | ts.SwitchStatement): b
822835
const otherwise = statement.elseStatement;
823836
if (otherwise !== undefined) {
824837
if (normalizedText(statement.thenStatement) === normalizedText(otherwise)) return false;
825-
return containsExit(statement.thenStatement) || containsExit(otherwise);
838+
return containsLiveExit(statement.thenStatement) || containsLiveExit(otherwise);
826839
}
827-
return containsExit(statement.thenStatement);
840+
return containsLiveExit(statement.thenStatement);
828841
}
829-
return statement.caseBlock.clauses.some((clause) => clause.statements.some(containsExit));
842+
// Per clause statement rather than over the whole switch, so a live exit in any clause counts
843+
// whatever the discriminant is. Reading the switch as one node would hand `containsLiveWhere`'s
844+
// discriminant fold a `switch (e.code)` it cannot decide, which changes nothing, and a
845+
// `switch (1)` it can, which is not this predicate's business: an unreachable CLAUSE is caught
846+
// by the same fold one level down, and the statement is only reached at all when its condition
847+
// references the caught binding.
848+
return statement.caseBlock.clauses.some((clause) => clause.statements.some(containsLiveExit));
830849
}
831850

832851
/**
@@ -893,7 +912,7 @@ function catchClauseEvidence(clause: ts.CatchClause): {
893912
// and all 240 clauses' evidence byte-identical. The tests are the cases in `dead throw written
894913
// after something that already exited`.
895914
//
896-
// Raised off `containsLiveExit`, never `containsExit`. The containment read is true of
915+
// Raised off `containsLiveExit`, never a plain containment read. Containment is true of
897916
// `if (false) { throw e; }` itself, so a provably dead statement raised the flag and blinded the
898917
// walk to the real classification below it: prepending one to a deciding clause turned its pass
899918
// into a swallow verdict on 78 real routes, the same false accusation for all eleven dead

0 commit comments

Comments
 (0)