Repository navigation
resolver: with options.paths, ask the package of the calling module for #imports and its own name - #47243
Open
robobun wants to merge 5 commits into
Open
resolver: with options.paths, ask the package of the calling module for #imports and its own name#47243robobun wants to merge 5 commits into
robobun wants to merge 5 commits into
Conversation
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
require.resolve(id, { paths })searches a bareidfrom each entry as if the entry imported it (main,src/resolver/resolver.rs:2197). So the entry's package answers its name and#imports. Node searches onlynode_modules.MODULE_NOT_FOUND, or an entry's copy). In Node it answers both.Fix
check_package_path_in_custom_dirsasks the calling module's package first. Its answer is final.test/js/node/module/module-resolve-filename-paths.test.js(14 fail on main, Node passes its 13),test/js/node/module/,test/js/bun/resolve/.Background
package.jsonabove it.#importsare its aliases. A self-reference requests its name.options.pathsonly starts thenode_modulessearch.check_package_pathandload_node_modules: every bare import pays. Thepathsslot costs them nothing.Downsides
nodejs/node#47681asks for it). A calling package withexportsno longer finds an entry's copy.paths, a resolve executes -9 to +4 instructions. A foundpathscall executes about 1,450 more..textgrows 9,216 bytes.pathscall from an unread directory readspackage.jsononce. A#importthat the caller lacks costs 8 syscalls and leaks 7 KB, as withoutpaths.Notes
How this was found. A differential run against Node v26.3.0 found it. No user reported it. The behaviour came in two steps. #18851 (v1.2.9) gave
require.resolvethepathsoption with each entry as the importing directory. #24325 (v1.3.2) madeModule._resolveFilenamereadoptions.paths. The vendored Node tests do not coverpathswith apackage.jsonon either side. So this PR turns no Node test green. The shared tests of this PR run under Node.What changes.
require.resolve(id, { paths })andModule._resolveFilename(id, parent, isMain, { paths }). Bun'srequire(id, { paths })andimport.meta.require(id, { paths })use the same search, so they load the same file.Choices that this change makes. The sections below give the detail.
pathsarray: the calling module's package answers, then nothing is searched. This is Node's behaviour.Repro (Node v26.3.0 as reference,
bun --no-install):subpkgnode_modules/subpkg/index.jssub/sub-main.jsnode_modules/subpkg/index.jscallerpkgcaller/main.jssub/node_modules/callerpkg/index.jscaller/main.js#xcaller/main.jsMODULE_NOT_FOUNDcaller/main.jsRows against Node v26.3.0. 41 calls, each from a module in a package and from a module in no package: 82 rows, one process per row (Node keeps a self-reference hit in
Module._pathCache). 56 rows passpaths. Release builds of main at 11c3671 and of this change on that commit. Run with the cwd outside a package and again with the cwd in the caller's package. Both runs give these counts.pathsrows that differ from NodeThe 2 rows left are
Module._resolveFilename("#x", null, false, { paths }). Node throws aTypeErrorthere (it readsparent.filenameofnull). Bun throwsMODULE_NOT_FOUND. The 26 rows withoutpathsgive the same answers on main and on this PR. Two more row sets were run on the same builds: 31 rows of parent filename shapes and 98 rows with afile://parent filename. In both, no row that agrees with Node on main differs now. Node v26.12.1 gives the same answers as v26.3.0.Rows outside these sets do move, and reviews found them. In the first four, an entry is the package that the parent is in, and main gave that package's answer through the entry.
{ filename: "./x.js" }, or a module whosefilenamewas set to that). Node reads a package for it. This PR gives it none.Object.create(module), aProxyof it). Also a module whosefilenamewas set toundefinedornull. Node gives each the package of the module. In Bun thefilenamegetter givesundefinedto a receiver that is not the module, and the setter stores the string"undefined", on main too. So this PR gives them no package.pathsit is in the package of the cwd. Withpathsit has none. Node has no such module.blob:ordata:, with or without a/before it. Node gives it no package. This PR gives it the package of the cwd, as it gives a worker that is made from a string. A real file in the root of the filesystem with such a name gets the same.A review ran 11,600 rows with
pathsover 60 parent shapes on an earlier draft. 202 rows agreed with Node on main and differed on that draft. 72 are of the first kind and 27 of the third. 103 came from two defects of that draft. An emptyfilenamehid the id<repl>(64 rows). A real module with a bare relativefilenamegot the package of the cwd (39 rows). Both are fixed. A review of the fix ran 21,528 processes over 77 parent shapes: 120 rows agree with Node on main and differ now. 88 are relative and hand-madeblob:names, and 32 are the second kind.require.call(parent, id, { paths })andModule._resolveFilename(id, parent, false, { paths })gave the same answer in all 2,079 pairs. That review ran beforedata:names were added to theblob:rule.Which package a parent is in.
pathsParentOf(C++) classifies the parent for both callers,requireandModule._resolveFilename.PathsParent::package_scope(Rust) decides the directory.paths.null,{},{ id }, a string parent and aModulewhosefilenamewas never set have none. Node has two exceptions for a parent whosefilenameis not truthy, and both are ported. The id<repl>is in the package of the cwd. The idinternal/preloadis too, but not for a#request.blob:ordata:URL is in the package of the cwd. That is a worker made from a string, from aBlobor from adata:URL. Bun names it with the URL (an ES module:/blob:<id>) and searches from the cwd for it withoutpathstoo. Node names a string worker[worker eval]in the cwd. A hand-made filename that starts like that gets the same.data:URL with a/in it has a name in a directory that does not exist (/data:text/javascript,...). It finds no package, with and withoutpaths.file://URL. For the URL this is Node's behaviour. For a relative filename Node reads<directory>/package.jsonfor each directory that the string names, and nothing above:./x.jsreads the one of the cwd, andx.jsreads none. On Windows Node reads it as a path below the cwd./at the end does not do that in Node's lookup (NormalizePathinsrc/node_modules.cc), and this PR follows that: on Windows CI, Bun gives such a parent no package. Node v26.12 keeps the package of a parent under the name of its directory (getTraversalStartDirectory). So on Windows its answer for that parent depends on what it looked up before, and the test does not ask Node for that row there.package.jsonat or above the directory. One above anode_modulesdirectory does not count, and one in the root of the filesystem does not (TraverseParentin Node'ssrc/node_modules.cc:// We don't need to try "/", in v22.12.0, v24.3.0, v26.3.0 andmain). So a file in the root is in no package. A run in a chroot with apackage.jsonin the root agrees. Node v26.3.0 givesMODULE_NOT_FOUNDfor#xand for the package's name, from/app.jsand from/src/app.js. This PR gives the same withpaths.build.module(), oronLoadin a namespace) has a name in the root, such as/virt. So it has no package withpaths. WithoutpathsBun searches from the cwd for it. Node has no such module.package.jsonthat is not a JSON object is passed over, as in Bun's search withoutpaths. An empty file is a package without a name, as there. Node throwsERR_INVALID_PACKAGE_CONFIGfor both. Apackage.jsonthat is not a regular file is not opened.importsorexportsthat is a number or a boolean is not read. Bun's parser gives anexportsmap with mixed keys ("."beside"default") the same marker. So such a package claims nothing withpaths, and the entries answer, as on main. Node throwsERR_INVALID_PACKAGE_CONFIGthere, and Bun withoutpathsgivesMODULE_NOT_FOUND.<cwd>/<its directory>in an--aotexecutable when that directory exists. An executable readspackage.jsononly when it is built with--compile-autoload-package-json. Then it answers as it does withoutpaths. A default executable reads none. So no package claims a request there, and the answers are main's.--compile-in-place) keeps its source directory, as underbun run. A review found one difference there when the sources were moved away. Withoutpathsthe cwd's package answers through the search after a miss. Withpathsan entry that has the name answers first.paths, a parent object whosefilenameis not a non-empty string has itsidread once, also for a builtin request. Node reads it once or twice, and not for a builtin. Main does not readidinrequire.call(parent, id, { paths }), nor after an emptyfilename. Withoutpathsthe reads are main's.What a claimed request gives. The search is the one of the same request without
paths, from the calling module. So:pathsof the calling module are read before itsexports, as withoutpaths. For every other request the tsconfig above the entry answers, as on main. A rule that the caller's tsconfig answers everypathsrequest would move rows that agree with Node today. It needs a maintainer's yes first.<name>/package.jsonof the calling package resolves althoughexportsdoes not list it, as withoutpaths. Node throwsERR_PACKAGE_PATH_NOT_EXPORTED. Main gives a copy below an entry, orMODULE_NOT_FOUND. The package's name with a?queryresolves as withoutpathstoo. Node does not find it.MODULE_NOT_FOUND. Node throwsERR_PACKAGE_PATH_NOT_EXPORTEDorERR_PACKAGE_IMPORT_NOT_DEFINED, with and withoutpaths.paths: []gives the caller's answer, then nothing is searched. This is Node's behaviour.pathsstarts one. For example animportstarget that is a bare name and is not installed goes to the registry. Under--install=forcethe install runs from the calling module's directory, so itspackage.jsonpicks the version.paths. Node keeps thepackage.jsonthat it read first.paths. Cost has the numbers.Auto-install. A search can end in an install when no directory at or above its start has a
node_modulesdirectory. It also can with--install=fallbackor--install=force. The importer of that install is the directory that the search starts in: an entry, as on main. On main the package of an entry answered#importsand its own name first, so such a request started no install. Two rules keep that:load_from_package_of_custom_dir). That package answers in the search that main runs from the entry. Thenode_modulesdirectories andNODE_PATHare searched before it, as in Node.CustomDirPaths::installs). No install starts at one, and its package does not answer.46 rows give the same file and the same registry requests as main. They cover a plain package, a workspace member, a
file:and alink:dependency, and a package in the global cache. Each runs with one and two entries in both orders. The plain package also runs against a registry that has a package of that name. A review ran 9,195 more rows in five install modes on an earlier draft. Every difference there is one that this description names.--install=forceinstalls in every case, as on main.Question for a maintainer. This PR makes two shipped behaviours go away. Both are Node's behaviour. No Bun issue asks for either.
paths: [dirInPackageX]no longer resolves the name of X or its#importsthrough the entry. That is with anode_modulesdirectory at or above the entry, with--no-install, underbun test, and in a compiled executable that readspackage.json(--compile-autoload-package-json). A default executable reads none, so the entry's package did not answer there on main either. Node throwsMODULE_NOT_FOUNDthere, or finds an installed copy.nodejs/node#47681(open) asks Node to resolve a self-reference throughpaths. Innodejs/node#58487(open) a Node member writes that a difference betweenpaths: [dir]and arequire.resolverun indir"would be a bug".createRequire(dir + path.sep).resolve(id)gives the entry's answer in Node, on main and here.exports, or any#request when it hasimports, is no longer searched from the entries. nx 24.0.1 asks for a copy of itself below an entry atdist/src/daemon/server/nx-console-operations.js:29and:56,shutdown-utils.js:37andcommand-line/configure-ai-agents/configure-ai-agents.js:50. Node gives the running copy there, and so does this PR. Main gives the copy below the entry. nx works around Node's answer indist/src/utils/package-json.jswithcreateRequire('/__nx_detached_resolver__/x.js'), which gives the copy below the entry in Node, on main and here.If Bun should keep the first on purpose, three parts can go: the two gates in
load_node_modules,load_from_package_of_custom_dirand theinstallsflag. The caller's half stays.The directory cache. On main a
pathssearch does not read the calling module's directory. What a later request finds there depends on that: a directory that was read is not read again until a miss drops it. So the package lookup (scope_package_json) reads the directory cache and does not write it.dir_info_cached_maybe_logputs a directory in the cache and then fills it under that lock. A Worker that reads the same directory again does that.package.jsonby path: onestat, and one read when the file is there. The answer is kept in a map on the resolver (custom_dir_scopes) until the directory cache has the directory.package.jsonmakes no read by path (opts.load_package_json). A directory thatBun.build()read in such an executable answers from the cache, as it does for the request withoutpaths.package.jsonpath would be too long for a read by path is asked in the cache only.A review ran 7,979 random sequences against an earlier draft that read the calling directory into the cache and dropped it again. 119 gave another answer than main. On this design the cases that it reported give main's answers, or the calling package's answer that this PR adds. So do 35 more sequences (a package that is created after a first call, then asked for again). The test
the package lookup leaves the directory cache as it washas seven of them. A later review ran 14,000 sequences withoutimportsorexportson the draft before this one: no answer differed from main, and in 220 traced sequences the only added syscalls were thepackage.jsonreads by path.That review found two defects, and both are fixed. The lookup read the cache without the lock, so with a Worker it could see a directory that was not filled yet. And a
package.jsonthat is a FIFO was opened. The testthe package of the calling module answers while a worker reads its directory againcovers the first. On a copy of the release binary with the lock patched out, it fails in 5 of 5 runs. No other test of the file fails there. On this build it passed 10 of 10. A review ran about 14 million requests beside a Worker, and a hang hunt with four Workers andBun.build(): no wrong answer and no hang.Known limits. In each, a request that no package claims reads the calling directory into the cache, gets an old answer, or costs more than on main.
package.jsonthat is created or edited in between is not seen bypathsrequests. Any request withoutpathsfrom that directory reads it again. Node keeps its first read for the life of the process. A draft dropped the answer on each miss: a failed probe then read and parsedpackage.jsonagain on every call.package.jsonlost that claim on disk since. Then the search from the calling module runs, the second check rejects the claim, and the entries answer as on main. But that search read the calling directory into the cache. It takes an edit ofpackage.jsonbetween twopathscalls from a directory that nothing else read.package.jsonthat a listing sees, and a package above claims the request. One case is apackage.jsonthat is a symbolic link to something that is no regular file. The other is a directory of 4,083 to 4,095 bytes. A link to a FIFO then blocks the search, as it blocks the same request withoutpathson main.Cost. Release builds of main at 11c3671 and of this change on that commit, Linux x64. The branch is rebased on a later main.
Instructions executed per call, without
paths, inside$resolveSync,jsFunctionResolveFileNameorBun.resolveSync,_resolve,resolve_and_auto_install,resolve_without_symlinks,check_package_path,check_relative_path,resolve_without_remapping,load_node_modules,dir_info_cached_maybe_logandPackage::parse_name(qemu-x86_64 -one-insn-per-tb -d exec,nochain, exact, (I(2N) - I(N)) / N):pathspathspaths#importhitModule._resolveFilename, bareBun.resolveSync, bareBun.resolveSync, relativeNo function on this path moved out of line. The resolver has the same set of symbols in both builds, plus the new cold functions and the growth code of the map.
Whole process,
perf_event_openinstructions:u, N = 20,000. Each cell is the median of three series, and a series is the median of 5 interleaved runs. The foundpathscall costs 1,430 to 1,484 more in four series. The spread of one cell was up to 493 here, and a review saw up to 1,264 on a loaded machine. So the rows withoutpathsare equal within it. The calling directory had six files: a miss reads it again.paths, 1 entry, foundpaths, 1 entry, found, caller in no packagepaths, 1 entry, found,Module._resolveFilenamepaths, 8 entries, not foundpaths, 1 entry, not foundpaths, 1 entry,nope/not foundpaths, 1 entry,@scope/pkgnot foundpaths: [], not foundpaths, the calling directory does not exist, not foundpaths, the caller's own namepaths, the caller's#xpaths, a#request that the caller'simportsdo not have#request withoutpathspaths, bare, foundpaths, bare, found, caller in no packagepaths, bare, found, tsconfig withpathspaths, self-referencepaths,#importpaths, relativepaths, bare, not foundpaths,Bun.resolveSync, barepathscall (qemu entry counts of the mimalloc entry points): found 2 malloc and 1 free. Not found: 6 malloc, 2 realloc and 5 free. Both are as on main. Claimed by the caller and found: 3 malloc and 2 free.pathscall: 1, as on main. A claimed miss of one segment: 2, as for the same request withoutpaths.pathscall in steady state (qemu-x86_64 -strace): 0, as on main.paths, and it costs what that costs. A claimed miss of one segment reads the calling directory again: 8 file syscalls, and the listing that it replaces is not freed for the life of the process (bust_dir_cachedrops the index entry and not the listing, on main too). 20,000 such misses add 131 to 134 MB of RSS (about 7 KB each). The same misses withoutpathsadd 130 to 137 MB on both builds. Withpathson main they add 6 to 10 MB, because main does not search from the calling directory. A claimed hit can alternate with a miss of one segment from the same directory. That pays half (71 to 76 MB against 8 to 9 MB).pathscall from a directory that the directory cache does not have asks forpackage.jsonby path. That is 1 syscall for each such directory, and 5 when the file is there. An example iscreateRequire()of a file that was never loaded. Main reads nothing there. Per such directory that is about 2,700 more instructions (18,100 to 20,800), or about 9,700 with the parse of apackage.json. The parsed file is kept: 4,000 such directories add 11 to 12 MB of RSS (main: 5 to 6 MB). Without apackage.jsonthey add 6 MB on both. The wall time of 2,000 such calls was 16 to 112 ms against 8 to 20 ms on main, on a shared machine.pathsprobes that the caller's package does not claim: +7 to +11 MB (main: +9 to +13 MB, 5 runs each). The same probes withoutpathsadd 135 to 142 MB on both..text: +9,216 bytes (108,646,665 to 108,655,881). The stripped binary grows by 12,288 bytes. By function (nm -S):importer_package_claimswith the lookup 4,986,check_package_path_in_custom_dirs1,543,pathsParentOf1,081,load_from_package_of_custom_dir345 andPathsParent::package_scope301 (all new), the growth code of the map 740,Bun__resolveSyncWithPaths+535,load_node_modules+252, the drop and the two constructors ofResolver+214,check_package_path+169,jsFunctionResolveFileName+84,$resolveSync+10,resolve_without_symlinks-87.size_of::<Resolver>()grows by 88 bytes (1,272 to 1,360): 32 for the slot (16 to 48) and 56 for the map. The map allocates nothing until apathscall comes from a directory that the cache does not have.Left as they are. This PR does not change these.
paths: ["./sub"],paths: ["sub"]), and the install order of the loop. resolver: anchor a relative require.resolve paths entry at the cwd #41491 owns both. The calling module's package answers first with relative entries too.Module._findPath(request, paths)gives each entry to a full resolve as the parent file.import.meta.resolveandimport.meta.resolveSyncwith{ paths: [p] }takepaths[0]as the parent. This is a Bun-only shape.paths, the search for the caller'spackage.jsongoes above anode_modulesdirectory and reads one in the root of the filesystem. Node stops at both. Withpathsthis PR stops there too.paths, a parent filename that ends in the separator is read as a file in the directory above. Afile://or relative filename gets a package there. Node differs in each.options.conditionsis not read.module.pathsis not searched. A package that is found throughNODE_PATHis resolved without itsexports(resolver: apply package.json exports to packages found via NODE_PATH #36566 is open).onResolvecallback runs while the slot is set. A resolve that it starts is searched with thepathsof the outer call.pathsentry with a NUL byte. Release builds of main and of this PR give the same answer. A debug build of this PR stops at an assertion in the directory lookup of the entry. Main's loop makes that lookup too. No debug build of main was run.Open PRs at the same place.
check_package_path_in_custom_dirs, and rewrites it for relative entries. The two conflict inresolver.rsand in the test file. The PR that lands second rebases: the claim goes before the loop, and theinstallsrule stays in it._resolvewith a replay that checks on disk what the miss read. The two merge without a textual conflict. Two tests here depend on the retry (finds what was created after the calling module's directory was readandthe search after a miss asks the package of the calling module again). The PR that lands second runs them on the merge.package.json.scope_package_jsonis a third. They can share one helper when they land.Also in this PR. A resolve that finds nothing allocates a path buffer for its thread (
specifier_cache_resolver_bufinsrc/jsc/VirtualMachine.rs), and the slot has no destructor. The Worker shutdown now frees it, next to the HPACK buffer that it already frees. LeakSanitizer reported the 4,096 bytes on the ASAN lane, in the child process of the Worker test. Main has the leak. A debug build hides it behind the suppressionleak:Bun__resolveSync, and the release ASAN build has no frame of that name. On a debug build without the call LeakSanitizer counts the leak under that suppression, and with the call it counts none. The VM of a Worker is freed without a drop of its fields, soTranspiler::deinitalso releases the map of this PR (release_custom_dir_scopes). Without that, LeakSanitizer reported 200 bytes for a Worker that resolved withpathsfrom a directory that no search had read.Suites. Run on Linux x64.
cargo check --workspace --target x86_64-pc-windows-msvcpasses. The first CI run passed the test file under Bun on Windows and macOS. Its Node run failed one row on Windows (a filename that ends in/, see above), and its ASAN lane reported the Worker buffer above. The later commits answer both. They also write each comment on one line, and run the memory test on every build with a bound for debug and ASAN builds.node --test.test/js/node/module/177 pass, 1 skip.test/cli/run/run-autoinstall.test.ts300 pass, 1 skip.test/aot/in-place-executables.test.ts35 pass, 6 todo.test/js/bun/plugin/112 pass.test/js/node/worker_threads/191 pass, 1 skip.test/js/web/workers/449 pass, 47 skip. The 48 vendored Node teststest-require-*.jsandtest-module-*.jspass.test/js/bun/resolve/with a 30 s limit for a test: 424 pass, 7 skip, 3 todo. With the default 5 s, two tests can exceed it in this container, on both builds:load the same file 10000 timesandload the same empty JS file 10000 times. Neither passespaths.--compile --aot --compile-in-placegives the answers ofbun runfor a caller in a package. This holds on main and on this PR.nodeExeAtLeastintest/harness.tsnow gives no Node whennodeis Bun under that name. Two of its three other users then skip. The third (node-tls-connect.test.ts) then expects Bun's answer and passes.test/js/bun/module-graph/has tests that time out on a loaded machine, on both builds. That was 3 of 6 runs on main and 1 of 6 on this PR. They pass when run alone.Self-review. Six rounds of review on earlier drafts raised 194 concerns. 177 are addressed here. 17 are not taken:
/.or/... Both are accidents of Node's path walk, and the first differs by platform.onResolvewindow in this PR. It is as on main, and it is listed above.package.jsonthat picks the version of an install, the map alone, the stop at the root of the filesystem (it needs a chroot), and the count of tests in the Node run.run-autoinstall.test.ts. They use the same registry fixture and stay beside the otheroptions.pathsrows.package.jsonwith node:module: implement findPackageJSON #37947 and fix(resolve): match Node 24 package configuration validation #44512. Those PRs are open.package.jsonin a Windows drive root or a UNC share root was not checked. This PR reads neither.\its own entry in the directory cache. resolver: key the directory caches natively on POSIX, so<dir>\and<dir>do not share an entry #47002 owns that key. The package lookup of this PR reads it by its own name.pathsvalue that is not an array, which passes in Node for another reason. node:module: throw ERR_INVALID_ARG_VALUE for a non-array options.paths in Module._resolveFilename #47010 owns that behaviour.The last round also named three things that its review allowed to leave. They are counted as addressed, and this description lists each:
package.jsonthat is a link to something that is no regular file.idbefore a builtin request.[human-review] gate passed · iteration 1 · 13 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 1
evidence per changed file