Repository navigation
Conversation
…erSourceOrigin Since the WebKit upgrade in #42319, JavaScriptCore emits no tail call in generator, async function and async generator bodies. A helper of that kind that does `return callerSourceOrigin()` now reports its own file in every case. Before, a generator helper reported the file of its caller, and an async helper returned null after an await. The JSDoc gets the exception and an @example. bun-jsc.test.ts runs the example and checks each kind of helper.
|
Warning Review limit reached
On-demand reviews are free for the next 9 days. After that, they cost $0.25 per reviewed file. Or wait 3 minutes for your next included review. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (3)
Comment |
|
Status: ready for review. Comments only in How it was checked:
|
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because packages/bun-types/ is CODEOWNER-gated, a human sign-off on the JSDoc wording is still worthwhile.
What was reviewed:
- JSDoc
@ exampleextraction test — traced the regex/split against the actualjsc.d.tscontent; the// file:///…/helper.tsoutput lines don't match the^\/\/ (\w+\.ts)$file-header regex, and thelines[i-1]access can't hit index -1 because line 0 is theimportstatement. - Cross-platform path handling —
tempDirrealpathsos.tmpdir()(macOS/private/var), and both sides of thereplaceAllarefile://URLs so Windows backslashes aren't compared. - Harness conventions —
tempDir/bunExe/bunEnv,await usingspawn, concurrent pipe drain, stderr/stdout asserted before exit code, fixture named*-fixture.ts, module-scope import.
Extended reasoning...
Overview
This PR touches three files: a JSDoc-only edit to packages/bun-types/jsc.d.ts (adds one sentence noting that generator/async helpers never tail-call callerSourceOrigin(), plus a two-file @ example), a new seven-variant fixture module test/js/bun/jsc/caller-source-origin-fixture.ts, and two new tests in the existing test/js/bun/jsc/bun-jsc.test.ts. No declaration signatures change and no runtime code is touched — the diff documents an engine-behavior change from a prior WebKit bump and pins it with tests.
Security risks
None. The change is documentation prose, a pure test fixture, and two tests. The subprocess spawn uses bunExe()/bunEnv in a harness-created temp dir with content derived from a checked-in .d.ts file; there is no network access, no untrusted input, and no auth/crypto surface.
Level of scrutiny
Low-to-moderate. The runtime is unchanged, so the review focused on whether the JSDoc claim is accurate (it matches the ECMAScript tail-position rules and the fixture test enforces it against the current engine), whether the @ example-parsing test is robust (traced the slice/regex path against the live file — the lastIndexOf("/**", end)/lastIndexOf("*/", declaration) pair brackets the right comment, the ^[ \t]*\* ? strip handles both * text and bare * lines, and the file-header regex ^\/\/ (\w+\.ts)$ correctly rejects the // file:///… output-comment lines), and whether the tests follow repo conventions (they do: tempDir, await using, Promise.all pipe drain, output asserted before exit code, .toEqual on a combined object, module-scope fixture import, *-fixture.ts naming).
Other factors
.github/CODEOWNERS assigns *.d.ts and /packages/bun-types/ to a named owner, so per the approval guidelines this should get a human look rather than an auto-approve even though the change is small and well-tested. I also checked cross-platform behavior: tempDir builds on fs.realpathSync.native(os.tmpdir()), which resolves the macOS /var → /private/var symlink so Bun.pathToFileURL(String(dir)).href matches what callerSourceOrigin() prints; on Windows both the printed origin and the pathToFileURL result are forward-slash file:/// URLs, so the replaceAll lines up. The PR author reports both new tests pass on a debug build of main and on Windows canary.
Problem
callerSourceOrigin(packages/bun-types/jsc.d.ts:202) says JavaScriptCore performs proper tail calls in all strict mode code. It concludes that a helper that doesreturn callerSourceOrigin()reports the file of its caller.cf1b36ec8703. Since then, generator, async function and async generator bodies emit no tail call forreturn f()([JSC] eval call should recognize tail-position WebKit/WebKit#73407,BytecodeGenerator.cpp:463). A helper of that kind now reports its own file.Fix
@exampleshows a plain helper and an async helper inhelper.ts, called fromindex.ts.test/js/bun/jsc/bun-jsc.test.tsgets two tests. One runs the@exampleand compares stdout with the comments in the example. It fails with the old JSDoc. The other checks seven kinds of helper from a fixture module. Four of them fail on the engine before Upgrade WebKit to cf1b36ec8703 #42319.6a92015fc(Windows x64). All 40 tests of the file pass.Background
callerSourceOrigin()walks the stack from its own frame. It skips builtin functions and returns the source URL of the first other JavaScript frame, ornullif there is none.callerSourceOrigin()the helper is not on the stack, so the first frame is the caller of the helper.Notes
Probe: each helper lives in
helper.mjsand doesreturn callerSourceOrigin().main.mjscalls it. The columns are bun1.4.3-canary.1+4ff919377(WebKitdfd696443b9b) and a debug build of main at 6a92015 (WebKitcf1b36ec8703).awaitalready reported its own file. After anawait, the body ran from the microtask queue and made the tail call with no other JavaScript frame on the stack, so the result wasnull.eval, indirectevalandnew Functionreport the file that created the code.Promise.resolve().then(callerSourceOrigin)givesnull.docs/does not mentioncallerSourceOrigin, and no other doc sentence makes a claim about tail calls.test/integration/bun-types/bun-types.test.tsgives the same result with and without this diff (11 pass, 10 fail). bun-types test: check against the newest @types/node release, not thelatestdist-tag #42230 has the cause (thelatestdist-tag of@types/node) and the fix.Error.stackframes, the await in an async generatorreturn). It does not touchbun:jscor the JSDoc.lazyand JSON5.stringify quotes #42257 addstest/integration/bun-types/jsdoc-examples.test.tsfor examples inbun.d.ts. This PR keeps its test inbun-jsc.test.ts, so the two do not conflict.prettier --checkpasses on the three files.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/jsc/bun-jsc.test.ts