Repository navigation
Conversation
… that member
With `--define AWS='{"ACCESS_KEY":"abc","SECRET_KEY":"def"}'`, Bun prints
`AWS.ACCESS_KEY` as `{ ACCESS_KEY: "abc", SECRET_KEY: "def" }.ACCESS_KEY`.
The visit of `AWS` puts a copy of the whole value in its place before the
member read sees it.
The visit of a member target that is an object or array define now leaves
the value to the member read. The read copies the one own member that it
names: `AWS.ACCESS_KEY`, `AWS["ACCESS_KEY"]`, `CFG.a.b`, `LIST[0]`,
`LIST.length`. The key is a name, or a string or a number that is written
as a literal. A member that is an object or an array goes to the member
read around it in the same way.
Every other use gets a copy of the whole value, as before: a key that the
value does not own, a computed key, `__proto__`, an optional chain, a
store, a `delete`, a call and a destructuring pattern. A chain that ends
in such a use takes no member, and the define under it puts the whole
value there. So does a define in a visit for a substitution.
A replaced read is what a define of that path is. `bun build` resolves
`require(CFG.path)`, and a file whose only `require` stands behind such a
read is an ES module under `bun run --define`.
`--compile --aot` decides the same reads in `fold_define_member`, from the
same keys. `fold_member` now reads a key through a type assertion, as the
transpiler does. The text of a function shows the member. A type inside a
part of the text that has a value in its place goes with that part. A type
error at such a read, in its object or at its key is still reported, and a
read in an arm that never runs stays unchecked.
|
Status
CI
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline findings, I also checked a few adjacent paths and found them consistent with the parser rule: process.env["NODE_ENV"] as a bracket read of an object define in src/sema/fold/define_members.rs (the is_user_defined_behind_index branch drops the user-defined flag so the build-added path define wins, as bun run does), an intermediate link of a chain like CFG.a.b (the entry recorded at line 139 is only consumed by the enclosing member read, which gives up on it at line 144), and the Function.prototype.toString text of a bracket member read in src/aot/lower/context/text_replaced_by_bundling.rs (the Index arm mirrors the Dot arm's bounds). Given the size of the parser and sema changes, a human look at the rest is still worthwhile.
Extended reasoning...
The change rewrites how the JS parser substitutes an object or array --define under a member read (src/js_parser/p.rs, visit_expr.rs, lib.rs) and mirrors that rule in the sema folder (src/sema/fold/define_members.rs and its callers) plus AOT lowering, with the runtime transpiler cache version bumped; it touches no auth, crypto or injection surface. Two confirmed findings are posted inline (bundler now resolves require(CFG.path) at build time, and an --aot module-format decision that can disagree with bun run when process.env is object-defined), and the diff is large (+1744/-36 across parser, sema and tests), so it is not approvable; this note only records the extra paths examined and ruled out.
|
Answers to the two inline findings of the review. I ran each command on a release build of the base (#47655) and of this branch.
This is the change that the first bullet of Downsides states. A replaced read is what a define of that path is.
I did not add an exception for a read that is the argument of A The PR notes list this as number 16 of the faults that a replaced read reaches. The values differ on the base, not only the kind of the file. Measured with console.log(process.env.NODE_ENV);
if (process.env.NODE_ENV !== "staging") require("node:path");
To make the two agree where the values differ, |
… compiled once `read_member_of_define` is compiled once for each parser type. The part that reads the key of the member uses no parameter of those types. It is now `defines::own_member_of_read`.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Each comment of two or more lines that the change added under `src/` is now one line.
There was a problem hiding this comment.
Code review completed
Nothing new to post: everything this review found is already covered by existing comments on this pull request or didn't merit a separate one.
Still open from earlier reviews (2):
- 🔴
src/js_parser/p.rs:7272—Users of bun build with an object define now get a build failure or an unintentionally bundled module when a member of… - 🔴
src/sema/fold/define_members.rs:181—Users who object-define process.env under --compile --aot can get a module format or dead-code decision that disagrees…
If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.
Fixes #21312
Stacked on #47655.
Problem
bun build --define AWS='{"ACCESS_KEY":"abc","SECRET_KEY":"def"}'printsconsole.log({ ACCESS_KEY: "abc", SECRET_KEY: "def" }.ACCESS_KEY);forconsole.log(AWS.ACCESS_KEY);.AWSwith the whole value before it visits.ACCESS_KEY(value_for_define,src/js_parser/p.rs:7136).Fix
AWS.ACCESS_KEY,AWS["ACCESS_KEY"],CFG.a.b,LIST[0].?., a store, a call.fold_define_member(src/sema/fold) folds the same reads for--compile --aot.transpiler.test.js,esbuild/extra.test.ts,bake/dev/hot.test.ts, twotest/aotfiles. Self-reviewed: 124 concerns raised, 99 addressed (Notes).Background
bun run, so--aotfolds the same reads.LIST[0]. Considered a fold in minified syntax alone (js_parser: fold a direct read of a multi-property object literal in minified builds #47604):bun runstill builds the object.Downsides
--define CFG.path=...:bun buildresolvesrequire(CFG.path), and a file without its onlyrequireis an ES module.require.resolveof a joined string), js_parser: roll back a removed statement's tracked uses along with its use counts #45345 (a minified bundle can fail).typescript.js: 965.7 to 964.2 million instructions). Text: +10,496 bytes.Notes
Why stacked. A string that a read takes out of the value is a string literal to the rules for one. On main those rules read 8-bit text that is not ASCII by byte, and fold
"abc".lengthunder a store. #47655 puts both conditions on the rules. With it,CFG.s[1]for{"s":"aéb"}is"é",(CFG.s + "x").lengthis 4, andCFG.s.length = 0stays a store. Tests for these rows are in this PR.What each use prints.
AWSis{"ACCESS_KEY":"abc","SECRET_KEY":"def"},CFGis{"a":{"b":{"c":7}},"list":["z",{"x":2}]}.AWS.ACCESS_KEY,AWS["ACCESS_KEY"],AWS[`ACCESS_KEY`]"abc"CFG.a.b.c,CFG.a.b,CFG.list[1].x,CFG.list.length7,{ c: 7 },2,2AWS,AWS.REGION,AWS?.ACCESS_KEY,AWS[key],AWS["ACCESS" + "_KEY"]AWS.ACCESS_KEY = 1,delete AWS.ACCESS_KEY,AWS.ACCESS_KEY(),const { ACCESS_KEY } = AWSCFG.a.missing,CFG.a.b(),CFG.list.map(f),CFG.list[0] = 1,delete CFG.a.bAWS["ACCESS" + "_KEY"],AWS[KEY]with a constant andLIST[1 - 1]the define puts the whole value there in every mode, andfold_define_memberreads the same literal. A rule of minified syntax can still fold that copy: withLIST=[10,[20],30],LIST[1 - 1]prints10there and underbun run, as on the base.LIST["0"]andOBJ[0]keep the whole value. So do__proto__and a string in brackets that is not ASCII (CFG["é"]): the parser stores such a string as UTF-16, and treats nox["é"]asx.éon main.CFG.éis replaced.keeps_define_whole), unless the target is the identifier of the define itself.let p = process; return p.env.FOO + 1;). A define keeps its whole value in that visit, as on the base. A key that the first visit computed is no literal of the source there.--define CFG.a=1besideCFG), anddropapplies as before.Results that change. Each is what
--define CFG.key=valuegives on main.bun run --define. WithSETTINGS={"debug":false,"level":3},if (SETTINGS.debug) require("node:path");leftrequirein use, and the file was CommonJS. Now it is an ES module. A one-key object already folds underbun runon main. The rows are ina-file-whose-only-require-a-define-removes-is-a-module.test.ts.require(CFG.path),require.resolve(CFG.path)andimport(CFG.path)are resolved bybun build. A file that is not there fails the build withCould not resolve.externaland a read through a variable (const cfg = CFG; require(cfg.path)) leave the specifier for run time.extra/DefineObjectMemberAs*pin the three cases. In an--aotexecutable such arequireloads the file. On the base it fails at run time.enum E { A = CFG.a, B }withCFG={"a":1}:E.Bis2, notundefined. A string member makes a string member of the enum:enum E { A = CFG.s }has noE["abc"].import.meta.hot.accept(CFG.deps, cb)is a static dependency list. On main it throws when the page loads.hot.test.tspins it.feature()argument can be a member read. Not pinned in this PR.bun runquotes the member ("abc".x), not the object.bun builddrops an unusedclass F extends CFG.max {}, as it dropsclass F extends 10 {}andclass F extends { a: 1 } {}. TheTypeErrorof that statement is gone.Faults of main that a replaced read now reaches. None is specific to an object define. A define of the same path, or the same literal in the source, has each of them on main. The first 8 show under
bun runorbun build. The other 8 are differences betweenbun runand an--aotexecutable.require.resolve(CFG.dir + "/dep.js")resolves./lib(Make EString::string() join folded ropes, fixing require.resolve() and other readers of the first segment #38944).--minify-syntax --target=bun,const h = require("./esm.js"); if (h && CFG.debug) log(); const call = name => h[name]();fails at run time (js_parser: roll back a removed statement's tracked uses along with its use counts #45345).switch (x) { case 0: const Y = CFG.n; case 1: return Y; }with--no-bundle --minify-syntax: the declaration is dropped and the read in the later case stays. Main does this forconst Y = 1.5.withblock, where a property of the object has to come first. Main does this forconst Y = 1.5.bun test --coverage):CFG.s;with the value"use strict".(1).#xas1.#x.CFG.n.#xwith a number member reaches it.import.meta[CFG.k]with the value"main"isimport.meta.main, which--format=iifeprints with a__requirethat is not there.--minify, the printer puts no space between a name that ends in a character that is not ASCII andinorinstanceof:obj[CFG.u] in ywith the value"é"printso.éin e. Main prints the same forx.é in y.toString()of an arrow function that ends in a define whose value is longer than the text of its read is cut to the length of that text, and a quote in an error message repeats the end of such a value. Main does this for() => SHORT. A member that is longer than its read is cut in the same way.typeof CFG.n,void CFG.n,CFG.s === "str"and!CFG.oprint astypeof 10,void 10,"str" === "str"and!{ k: 1 }. Main printstypeof 3fortypeof LEVEL.bun runapplies to a literal. ArequirebehindNAME.length,NAME[0],NAME.charCodeAt(0),({ k: DEBUG }).k, a constantconst X = typeof ZEROor an enum member (enum E { A = 1 },E.A !== 1), and the statements-FLAG, module;and[...LIST, module];, leave an ES module underbun runand CommonJS in the executable on main. The same reads of a member now differ in the same way (SETTINGS.name.length,enum E { A = CFG.one }).enum E { A = S.name, B = A }hasE["n"]in the executable and not underbun run, asA = NAMEhas on main. AOT: model the member reads of literals thatbun runfolds #47646 models these rules for literals in the source.if (CFG.a && CFG.list) {} else require("node:path");with an object and an array member is CommonJS underbun runand an ES module in the executable. Main has this forif (OBJ && ARR)with two defines.const X = CFG.off; if (X) module.exports = 1;behind animportstatement is an error underbun runand runs in the executable, asconst X = falsedoes on main.feature(CFG.flag):bun runandbun build --compiletake it. The--aotbuild stops withTS100101: feature() argument must be a string literal, as it does forfeature(NAME)on main.return,new.targetordelete xin an arm that a member rules out:bun runrunsif (CFG.off) return;, and the--aotbuild stops withTS1108. Main has this forif (OFF) return;.bun runhasx["name"]asx.name, and a build without--minify-syntaxhas not. With--define app.debug=false,app["debug"]isfalseunderbun runand aReferenceErrorin the executable on main. With aprocess.envobject,process["env"].DEBUGis now an ES module underbun runand CommonJS in the executable.bun buildaddsprocess.env.NODE_ENV,process.env.BUN_ENVandprocess.browserby itself, and with--compilealsoprocess.platform,process.archandprocess.versions.bun.bun runadds none. With--define process.env='{"NODE_ENV":"production"}'the executable printsdevelopmentandbun runprintsproductionon main. Where the two values are the same, the two now agree on the kind of the file. Where they differ, the kind differs too.One more edge is from this PR. A read that takes no member under a chain of two or more links visits the chain a second time. With a define that is named
arguments,class A { x = arguments.a.missing }then reportsCannot access "arguments" heretwice. A reserved word of strict mode is reported once.Measurements. Release builds for linux x64 of the base (#47655,
bf41088d8af) and of this branch. Each row was run on the base and on the branch in turn. User-mode instructions of the whole command on all threads (perf_event_open, least of 3 runs), peak RSS, bytes of output.bun build --no-bundletypescript.js(9.1 MB), no definef(a.id, b.name);), no definevar a = 1;), no defineAWS["KEY_n"]AWS?.KEY_n(not folded)DB.users.uN.name, 12,000 leavesbun --define AWS=<4,000 keys> -e 1(least of 20 runs)cmpb $0xff,<slot>; jne <cold>behind the visit of its target. The three rows without a define count fewer instructions than the base. That is what the other code ofvisit_expr_in_outcosts in this build, not a gain of the change.visit_expr_in_outissub $0x1f8,%rspon both builds. The function is 12,238 B on the base and 12,416 B here, twice (P<false>andP<true>).read_member_of_defineis cold and out of line, and takes the read and itsExprInalone. The rule for the key isdefines::own_member_of_read, which is compiled once for all parser types.find_symbolhas the same code on both builds.leave_define_to_member_readis a call there. A test ofis_property_access_targetin front of that call was built and measured. The other code ofvisit_expr_in_outthen came out differently: 38 more instructions per use in that row,typescript.js966.1 million, 800,000 member reads 2,646.0 million. So the call stays.size bun-profile: text 140,314,786 to 140,325,282 B, data and bss the same. The stripped binary grows by 8,192 B.size_of::<P>()4,432 to 4,440 B.ExprInstays 5 B.Define::insertis not in the diff.Sizes that a project can have, with the last two releases (1 run each for the releases). This table passes the define through
bunfig.toml.--define AWS.ACCESS_KEY='"abc"'printsconsole.log("abc");on 1.4.3, on 1.4.4 and on main. What one define per key cannot give:LIST[0], one flag for a whole JSON value, andAWS["ACCESS_KEY"]without--minify-syntax.A read finds its member by a scan of the keys from the last one. So n reads of n keys cost about 4 times as much for each doubling (2.8 to 4.3 in the table). Memory does not grow that way.
DB.users.uN.name) 2,000 / 4,000 / 8,000 / 16,000bun build --compile --aotof n reads of an n-key define: 443,654,106,776 instructions and 6,315.1 MB on the base at 4,000 (1 run). Here: 1,460,894,358 / 99.2 MB at 2,000, 2,715,920,682 / 103.5 MB at 4,000, 5,672,722,798 / 135.2 MB at 8,000.Run time: 2,000,000 calls of
function f() { return CFG.key12; }with a 23-key define andBUN_JSC_useDFGJIT=0: 2,068,305,085 instructions on the base, 173,623,957 here.CFG[key]is 2,112 million on the base and 2,110 million here. With all tiers on the counts do not rank the builds: four repeats of the least of 3 runs give 70 to 78 million on the base and 31 to 64 million here, and the 3 runs of one repeat span up to 255 million.Uses that still copy the whole value.
AWS,AWS?.KEY,AWS[key], a computed key, a key that the value does not own, a call, a store,const { KEY } = AWS, and a chain that ends in one of these (CFG.list.map(f)). Each costs what it costs on main: theAWS?.KEY_nrow.AOT.
fold_memberdecides a member of a define value after the define table, so a define of the path itself still comes first.fold_membernow reads a key through a type assertion (CFG["a" as const],LIST[0 as number]), ahead of the define table. The transpiler has no node for an assertion. This also holds for a define of a path.bun runreads the member out of a user's object where the build has a define of its own for the path (process.env.NODE_ENV).note_member_that_bun_run_readsrecords that where the two values are the same.bun runhasboth["on"]asboth.on. Where a user's define of that path exists, the model does not take the member of the object for whatbun runhas.toString()) shows the member:text_replaced_by_bundling.rshandles a decideda[0]and leaves out the value of the object of a decided member.(AWS as any).KEY,AWS!.KEY,CFG["a" as const]):find_type_ranges_beside(src/sema/erasure/type_ranges.rs) no longer removes it a second time. On main the value is then dropped and the cut stays:(process as any).env.NODE_ENVwith a define of that path prints(process;. Now it prints"production".Decisions::is_part_of_define_member_readmarks the object and the key of a read thatfold_define_memberdecided, and the checker does not take them for dead code. A read in an arm that never runs stays unchecked, as before. A define of a path drops such an error on main. This PR leaves that as it is.test/aot/minifying-first-saves-no-code/known-today.tsloses 4 entries that pass now (extra/DefineObject1,DefineObject2,DefineArray,DefineArray2).Related PRs.
CFG.t.lengthas a read of the literal. After this PR that row prints3.bun runfolds #47646: editsfold_memberandknown_to_bun_run.rstoo. It leaves member reads of define values out of its model. The PR that lands second updates the rows of the other.?.and string indexes. The?.rows of this PR pin what main prints, the whole value. bundler: match defines through optional chains, string indexes, and this, and parse define values as member chains #40857 has to change them if it lands second.eval(String(f)), a Worker) has no such variable there.Self-review. Three rounds: 42 concerns on the first draft, 42 on the reworked diff, 40 on the diff after the second rework. 99 are addressed:
{}.k ??= vand a rule for[a, b][n]then folded)visit_expr_in_out--aot: function text behind a type assertion at the place where types and replaced text meet, type errors at a read with an arm that never runs left unchecked, a string member behind a type assertion, a define of the build beside a user's object, a key in brackets beside a define of its pathimport.meta, a bound on memory, the dev server, specifiers, a key that is not ASCII and that the value owns, a number as the key of an object, arrows that end in a read behind a type20 are addressed in part. Most of them are faults of main that this text names and that this PR does not repair: numbers 8 to 16 above. The others: macro arguments and
feature()are not pinned, macro objects keep their own rule, the second report ofargumentsunder a chain of two links stays, andtest/aot/parallel/bundler/define-object-member.tsguards values and passes on the base.5 are not taken:
bun run. That is another change tobun run. This PR follows the rule that a define of a path follows.--aotfunction text for a value longer than its read in this PR. The fault is on main for every define.gives what the engine gives for each generated read) that takes 2 to 5 s in a debug build. It is not in this diff.is_property_access_targetin front of the call ofleave_define_to_member_read. It was built and measured, and the numbers are under Measurements.Suites run. Debug build with ASAN: the 5 changed test files. Release build of the branch: all of
test/bundler(8,063 pass, the 2bytecode cache portabilityfailures are on the base), all oftest/aot/parallel(7,169 of 7,185 pass, 12 skip, and the 4 that fail also fail on the base:packages/subpaths/exports-closed0,exports-closed6,module-loaders/raw-behind-a-path-asks-for-the-text-of-the-file,errors/call-sites-default-constructor-local-heat-shuffled.cjs),test/internal/source-lints.bun run rust:testforbun_sema,bun_sema_parser,bun_aot,bun_bundler. Each of the 5 changed test files fails on a release build of the base. A differential check outside the tree: 4,652 expressions over 6 define values in 7 modes againstJSON.parseof the value. The rows that differ are the same 10 on the base and on this branch.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/bake/dev/hot.test.ts, test/aot/the-text-of-a-function-lacks-what-a-bundle-lacks.test.ts, test/aot/a-file-whose-only-require-a-define-removes-is-a-module.test.ts