From b7442c19583a569d79d0c7fc61635e34d84c8626 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=90=B4=E4=BD=B3=E6=B5=A9Alben?= <35291372+XiaoBinGan@users.noreply.github.com> Date: Thu, 29 Oct 2026 08:23:53 +0800 Subject: [PATCH] fix(javascript): match resource release methods to the acquired handle (#7773) * fix(javascript): match resource release methods to the acquired handle Every acquisition family admitted every release method, so `h.close()`, `h.destroy()` and `h.end()` each resolved as a complete release of whatever the binding held. Node exposes a different subset per acquisition, and the rejected pairs throw at the call site instead. Measured on Node 24.22.1: | acquisition | value | release members | | --- | --- | --- | | `fs.openSync` | number | none | | `fs.open` (callback) | undefined | none | | `fs.createReadStream` | ReadStream | `close`, `destroy` (`end` is the byte-offset field) | | `fs.createWriteStream` | WriteStream | `close`, `destroy`, `end` | | `fs/promises.open` | FileHandle | `close` | | `net.connect` | Socket | `destroy`, `end` | | `tls.connect` | TLSSocket | `destroy`, `end` | So `fs.openSync(...)` + `fd.close()`, `createReadStream(...)` + `.end()`, `net.connect(...)` + `.close()` and the FileHandle cases emitted a `releases` / `resolved` fact for a call that throws. Carry the specifier with each acquisition candidate, list the release methods each pair actually has, and emit no release when the pair is not one of them. A release stays `complete` only when the binding held exactly one acquisition and that acquisition admitted the method. `tls.createConnection` is not a Node export at all: `'createConnection' in require('node:tls')` is false and the ESM import does not link. Drop it from the table so a `tls.createConnection` call yields no acquisition. * chore: retrigger title and readiness checks after the title was corrected * fix(javascript): cover a mixed-family binding before completing a release greptile asked for the mixed-family case: a binding that can hold either a FileHandle or a socket, then calls close(). The FileHandle is the only linked acquisition and the release stays partial. That case also showed the `candidates.length === 1` guard was dead: an acquisition of another family is not in `linked`, so `holdsOnlyAcquisitions` already returns false for it and the release already stayed partial without the guard. Removing the guard and keeping the test means the behaviour is now pinned by a test rather than by two overlapping conditions. --- .../javascriptBuiltinResourceMethods.ts | 25 +++- ...tSemanticAnalysis.release_families.test.ts | 134 ++++++++++++++++++ .../javascript/javascriptSemanticResources.ts | 45 +++++- 3 files changed, 196 insertions(+), 8 deletions(-) create mode 100644 src/domain/javascript/javascriptSemanticAnalysis.release_families.test.ts diff --git a/src/domain/javascript/javascriptBuiltinResourceMethods.ts b/src/domain/javascript/javascriptBuiltinResourceMethods.ts index 8034b902fe..95f66f250b 100644 --- a/src/domain/javascript/javascriptBuiltinResourceMethods.ts +++ b/src/domain/javascript/javascriptBuiltinResourceMethods.ts @@ -3,9 +3,32 @@ export const BUILTIN_RESOURCE_METHODS = { fs: ["open", "openSync", "createReadStream", "createWriteStream"], "fs/promises": ["open"], net: ["connect", "createConnection"], - tls: ["connect", "createConnection"], + tls: ["connect"], } as const; +/** + * Release methods the value Node returns from each acquisition actually has. + * + * `fs.openSync` returns a numeric descriptor and `fs.open` in its callback form + * returns `undefined`, so neither has a release method at all; the descriptor is + * released through `fs.closeSync`/`fs.close`, which is not a member call. A + * `fs.ReadStream` exposes `end` as its byte-offset field, not a method. A + * `FileHandle` and a `net.Socket`/`TLSSocket` each expose only their own subset, + * so a family-blind release list claims a teardown that throws instead. + */ +export const BUILTIN_RESOURCE_RELEASE_METHODS = { + "fs:open": [], + "fs:openSync": [], + "fs:createReadStream": ["close", "destroy"], + "fs:createWriteStream": ["close", "destroy", "end"], + "fs/promises:open": ["close"], + "net:connect": ["destroy", "end"], + "net:createConnection": ["destroy", "end"], + "tls:connect": ["destroy", "end"], +} as const satisfies Readonly< + Record +>; + export const BUILTIN_RESOURCE_SPECIFIERS = Object.keys( BUILTIN_RESOURCE_METHODS, ); diff --git a/src/domain/javascript/javascriptSemanticAnalysis.release_families.test.ts b/src/domain/javascript/javascriptSemanticAnalysis.release_families.test.ts new file mode 100644 index 0000000000..e7b7aae52f --- /dev/null +++ b/src/domain/javascript/javascriptSemanticAnalysis.release_families.test.ts @@ -0,0 +1,134 @@ +import { expect, it } from "vitest"; + +import { analyzeJavaScriptSemantics } from "./javascriptSemanticAnalysis.js"; + +/** + * Node's own objects expose different teardown members per acquisition, so a + * member call that Node rejects must not be reported as a release. Every row is + * a pair that throws at the call site on Node 24.22.1. + */ +const rejected = [ + [ + "a numeric descriptor from fs.openSync", + `import { openSync } from "node:fs"; + const fd = openSync("input.txt", "r"); + fd.close();`, + ], + [ + "a numeric descriptor released with destroy", + `import { openSync } from "node:fs"; + const fd = openSync("input.txt", "r"); + fd.destroy();`, + ], + [ + "the callback form of fs.open", + `import { open } from "node:fs"; + const pending = open("input.txt", "r", () => {}); + pending.close();`, + ], + [ + "a read stream's byte-offset field", + `import { createReadStream } from "node:fs"; + const stream = createReadStream("input.txt"); + stream.end();`, + ], + [ + "a FileHandle without a release member", + `import { open } from "node:fs/promises"; + const handle = await open("input.txt"); + handle.destroy();`, + ], + [ + "a socket without close", + `import { connect } from "node:net"; + const socket = connect(9); + socket.close();`, + ], + [ + "a TLS socket without close", + `import { connect } from "node:tls"; + const socket = connect(9); + socket.close();`, + ], +]; + +it.each(rejected)("emits no release for %s", (_name, source) => { + expect( + analyzeJavaScriptSemantics(source) + .resourceOperations.filter(({ kind }) => kind === "release") + .map(({ method }) => method), + ).toEqual([]); +}); + +/** The pairs Node does support stay complete, so the filter is pure narrowing. */ +const accepted = [ + [ + "a FileHandle closed", + `import { open } from "node:fs/promises"; + const handle = await open("input.txt"); + handle.close();`, + "close", + ], + [ + "a read stream destroyed", + `import { createReadStream } from "node:fs"; + const stream = createReadStream("input.txt"); + stream.destroy();`, + "destroy", + ], + [ + "a socket ended", + `import { connect } from "node:net"; + const socket = connect(9); + socket.end();`, + "end", + ], +]; + +it.each(accepted)("keeps %s complete", (_name, source, method) => { + expect( + analyzeJavaScriptSemantics(source) + .resourceOperations.filter(({ kind }) => kind === "release") + .map(({ method: released, resolution }) => ({ + method: released, + resolution, + })), + ).toEqual([{ method, resolution: "complete" }]); +}); + +/** + * A binding that can hold more than one acquisition stays partial even when a + * method is admissible for one of them: the release is only certain if the + * binding held exactly one acquisition before the filter ran. + */ +it.each([ + [ + "a FileHandle or a socket, released with close", + `import { open } from "node:fs/promises"; + import { connect } from "node:net"; + let handle; + if (flag) { handle = await open("input.txt"); } + else { handle = connect(9); } + handle.close();`, + [{ method: "close", linked: 1, resolution: "partial" }], + ], + [ + "a read stream or a write stream, released with end", + `import { createReadStream, createWriteStream } from "node:fs"; + let stream; + if (flag) { stream = createReadStream("input.txt"); } + else { stream = createWriteStream("output.txt"); } + stream.end();`, + [{ method: "end", linked: 1, resolution: "partial" }], + ], +])("keeps %s partial", (_name, source, expected) => { + expect( + analyzeJavaScriptSemantics(source) + .resourceOperations.filter(({ kind }) => kind === "release") + .map(({ method, linkedResourceIds, resolution }) => ({ + method, + linked: linkedResourceIds.length, + resolution, + })), + ).toEqual(expected); +}); diff --git a/src/domain/javascript/javascriptSemanticResources.ts b/src/domain/javascript/javascriptSemanticResources.ts index 7ca9420076..ec5e5252f4 100644 --- a/src/domain/javascript/javascriptSemanticResources.ts +++ b/src/domain/javascript/javascriptSemanticResources.ts @@ -3,6 +3,7 @@ import * as t from "@babel/types"; import { BUILTIN_RESOURCE_SPECIFIERS, BUILTIN_RESOURCE_METHODS, + BUILTIN_RESOURCE_RELEASE_METHODS, } from "./javascriptBuiltinResourceMethods.js"; import type { JavaScriptSemanticCallable, @@ -29,8 +30,23 @@ import { range } from "./javascriptStaticAnalysisHelpers.js"; const RELEASE_METHODS = ["close", "destroy", "end"] as const; +/** + * Release methods the value from this acquisition actually has. An unlisted + * specifier/method pair admits none, so a call to it emits no release fact. + */ +const releaseMethodsFor = ( + acquisition: ResourceCandidate, +): readonly ("close" | "destroy" | "end")[] => { + const key = `${acquisition.specifier}:${acquisition.method}`; + const table: Readonly< + Record + > = BUILTIN_RESOURCE_RELEASE_METHODS; + return table[key] ?? []; +}; + interface ResourceCandidate { readonly node: t.CallExpression | t.OptionalCallExpression; + readonly specifier: string; readonly method: JavaScriptSemanticResourceOperation["method"]; readonly ownerCallableId: string | null; readonly resultBindingId: string | null; @@ -62,11 +78,12 @@ const collectAcquisitions = ( const output: ResourceCandidate[] = []; traverseDataEffects(program, context, (node) => { if (!t.isCallExpression(node) && !t.isOptionalCallExpression(node)) return; - const method = acquisitionMethod(node.callee, context.state); - if (method === null) return; + const reference = acquisitionMethod(node.callee, context.state); + if (reference === null) return; output.push({ node, - method, + specifier: reference.specifier, + method: reference.method, ownerCallableId: context.callableStack.at(-1) ?? null, resultBindingId: outerDataEffectBinding(node, context), }); @@ -102,9 +119,15 @@ const collectReleases = ( member.object, member.object.name, ); - const linked = + const candidates = binding === undefined ? [] : (byBinding.get(binding.bindingId) ?? []); - if (binding === undefined || linked.length === 0) return; + if (binding === undefined || candidates.length === 0) return; + const linked = candidates.filter((acquisition) => + releaseMethodsFor(acquisition).includes(method), + ); + // No linked acquisition exposes this release method, so the call is not a + // release of it at all: Node throws at the call site instead. + if (linked.length === 0) return; output.push({ resourceId: `resource:release:${String(node.start ?? -1)}:${String(node.end ?? -1)}`, kind: "release", @@ -169,10 +192,15 @@ const immutableAcquisition = ( resolution: "complete", }); +interface AcquisitionReference { + readonly specifier: string; + readonly method: JavaScriptSemanticResourceOperation["method"]; +} + const acquisitionMethod = ( callee: t.Node, state: JavaScriptSemanticAnalysisState, -): JavaScriptSemanticResourceOperation["method"] | null => { +): AcquisitionReference | null => { for (const specifier of BUILTIN_RESOURCE_SPECIFIERS) { const method = builtinDataEffectMethod(callee, state, [specifier]); if ( @@ -183,7 +211,10 @@ const acquisitionMethod = ( ] as readonly string[] ).includes(method) ) - return method as JavaScriptSemanticResourceOperation["method"]; + return { + specifier, + method: method as JavaScriptSemanticResourceOperation["method"], + }; } return null; };