Repository navigation
js_parser: make --drop turn <dropped>.method.bind(...) into () => {}, not undefined
#45566
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1869,6 +1869,98 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O | |
| p.should_fold_typescript_constant_expressions = | ||
| prev_should_fold_typescript_constant_expressions; | ||
| } | ||
| /// `console.log.bind(console)` makes a function to call later: `() => {}`, not `undefined`. | ||
| #[cold] | ||
| #[inline(never)] | ||
| fn replace_bind_of_dropped_function(p: &mut Self, e: &mut Expr) -> bool { | ||
| let Data::ECall(mut call) = e.data else { | ||
| return false; | ||
| }; | ||
| let Data::EDot(bind) = call.target.data else { | ||
| return false; | ||
| }; | ||
| debug_assert!(bind.name == b"bind"); | ||
| // Not a bind of a dropped function: `Mousetrap.bind(k, f)`, or a callee on the drop list. | ||
| if !matches!(bind.target.data, Data::EDot(..)) | ||
| || Self::is_drop_key(p, call.target) | ||
| || !Self::is_dropped_name(p, bind.target) | ||
| { | ||
| return false; | ||
| } | ||
| // The arguments are removed but still visited: the visit enters every scope in parse order. | ||
| let old_is_control_flow_dead = p.is_control_flow_dead; | ||
| p.is_control_flow_dead = true; | ||
| for arg in call.args.slice_mut() { | ||
| p.visit_expr(arg); | ||
| } | ||
| p.is_control_flow_dead = old_is_control_flow_dead; | ||
| *e = p.new_expr(E::Arrow::NOOP_RETURN_UNDEFINED, e.loc); | ||
| true | ||
| } | ||
| /// Whether `callee` spells a drop key. Unlike `is_dot_define_match`, `a?.b` counts as `a.b`. | ||
| fn is_drop_key(p: &Self, callee: Expr) -> bool { | ||
| let Data::EDot(last) = callee.data else { | ||
| return false; | ||
| }; | ||
| let Some(keys) = p.define.dots_for(last.name.slice()) else { | ||
| return false; | ||
| }; | ||
| keys.as_slice().iter().any(|key| { | ||
| let Some((root, members)) = key.parts.split_first() else { | ||
| return false; | ||
| }; | ||
| let mut expr = callee; | ||
| for member in members.iter().rev() { | ||
| match expr.data { | ||
| Data::EDot(dot) if strings::eql(member, &dot.name) => expr = dot.target, | ||
| _ => return false, | ||
| } | ||
| } | ||
| let Data::EIdentifier(id) = expr.data else { | ||
| return false; | ||
| }; | ||
| key.data.method_call_must_be_replaced_with_undefined() | ||
| && strings::eql(p.load_name_from_ref(id.ref_), root) | ||
| }) | ||
|
Comment on lines
+1899
to
+1924
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 (optional) Users who list both Why this was flaggedInput Verification: Trigger: |
||
| } | ||
| /// Reads the drop list, not the flag alone: `import.meta.hot` raises it with no drop list. | ||
| fn is_dropped_name(p: &mut Self, mut expr: Expr) -> bool { | ||
| loop { | ||
| match expr.data { | ||
| Data::EDot(dot) => { | ||
| if Self::is_dropped_dot(p, expr) { | ||
| return true; | ||
| } | ||
| expr = dot.target; | ||
| } | ||
| Data::EIdentifier(id) => { | ||
| let name = p.load_name_from_ref(id.ref_); | ||
| return p.symbols[id.ref_.inner_index() as usize].kind | ||
| == js_ast::symbol::Kind::Unbound | ||
| && !id.must_keep_due_to_with_stmt() | ||
| && p.define.for_identifier(name).is_some_and(|define| { | ||
| define.method_call_must_be_replaced_with_undefined() | ||
| }); | ||
| } | ||
| _ => return false, | ||
| } | ||
| } | ||
| } | ||
| fn is_dropped_dot(p: &mut Self, expr: Expr) -> bool { | ||
| let Data::EDot(dot) = expr.data else { | ||
| return false; | ||
| }; | ||
| let defines = p.define; | ||
| let Some(candidates) = defines.dots_for(dot.name.slice()) else { | ||
| return false; | ||
| }; | ||
| for define in candidates.as_slice() { | ||
| if p.is_dot_define_match(expr, &define.parts) { | ||
| return define.data.method_call_must_be_replaced_with_undefined(); | ||
| } | ||
| } | ||
| false | ||
| } | ||
| fn e_call(p: &mut Self, e: &mut Expr, in_: ExprIn) { | ||
| let expr = *e; | ||
| let mut e_ = expr.data.e_call().expect("infallible: variant checked"); | ||
|
|
@@ -2117,6 +2209,14 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O | |
| if method_call_should_be_replaced_with_undefined { | ||
| p.method_call_must_be_replaced_with_undefined = false; | ||
| match &e_.target.data { | ||
| Data::EDot(dot) | ||
| if dot.name == b"bind" && Self::replace_bind_of_dropped_function(p, e) => | ||
| { | ||
| p.options.ignore_dce_annotations = old_ce; | ||
| p.should_fold_typescript_constant_expressions = | ||
| old_should_fold_typescript_constant_expressions; | ||
| return; | ||
| } | ||
| // If we're removing this call, don't count any arguments as symbol uses | ||
| Data::EIndex(..) | Data::EDot(..) | Data::EIdentifier(..) => { | ||
| p.is_control_flow_dead = true; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -114,4 +114,192 @@ describe("bundler", () => { | |
| drop: ["ASSERT"], | ||
| backend: "api", | ||
| }); | ||
|
|
||
| // `console.log.bind(console)` does not call console.log. It makes a function that the | ||
| // program calls later, so it must stay a function. | ||
| for (const [suffix, options] of [ | ||
| ["", { backend: "api" }], | ||
| ["Cli", { backend: "cli" }], | ||
| ["NoBundle", { bundling: false }], | ||
| ["MinifySyntax", { minifySyntax: true }], | ||
| ] as const) { | ||
| itBundled(`drop/BindStaysCallable${suffix}`, { | ||
| files: { | ||
| "/a.js": ` | ||
| const out = text => process.stdout.write(text + "\\n"); | ||
| globalThis.my = { logger() { out("FAIL"); } }; | ||
| const log = console.log.bind(console); | ||
| log("FAIL"); | ||
| const logger = { info: console.info.bind(console), warn: console.warn.bind(console) }; | ||
| logger.info("FAIL"); | ||
| logger.warn("FAIL"); | ||
| class Logger { | ||
| log = console.log.bind(console); | ||
| } | ||
| new Logger().log("FAIL"); | ||
| function withDefault(log = console.log.bind(console)) { | ||
| log("FAIL"); | ||
| } | ||
| withDefault(); | ||
| console.log.bind(console)("FAIL"); | ||
| console.log.bind(console).bind(null)("FAIL"); | ||
| console.log?.bind(console)("FAIL"); | ||
| console?.log.bind(console)("FAIL"); | ||
| console.log.bind?.(console)("FAIL"); | ||
| ["FAIL"].forEach(console.log.bind(console)); | ||
| Promise.reject(new Error("FAIL")).catch(console.error.bind(console)); | ||
| my.logger.bind(null)("FAIL"); | ||
|
Comment on lines
+147
to
+151
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 nit (optional): a maintainer gets a Why this was flaggedThe row Verification: The |
||
| Bun.inspect.table.bind(null)("FAIL"); | ||
| // With minifySyntax, and at run time, these single-use locals are inlined and visited again. | ||
| function viaAlias() { | ||
| const m = my; | ||
| return typeof m.logger.bind(null) === "function" ? "callable" : "broken"; | ||
| } | ||
| function viaLocal() { | ||
| const log = console.log.bind(console); | ||
| return log("FAIL") || "called"; | ||
| } | ||
| out(viaAlias()); | ||
| out(viaLocal()); | ||
| out(console.log.bind(console) ? "truthy" : "falsy"); | ||
| out(typeof console.log.bind(console)); | ||
| out(typeof console["log"].bind(console)); | ||
| out(typeof console.log["bind"](console)); | ||
| out("ALIVE"); | ||
| `, | ||
| }, | ||
| drop: ["console", "my.logger", "Bun.inspect"], | ||
| run: { stdout: "callable\ncalled\ntruthy\nfunction\nfunction\nfunction\nALIVE" }, | ||
| ...options, | ||
| }); | ||
| } | ||
| itBundled("drop/BindBecomesEmptyFunction", { | ||
| files: { | ||
| "/a.js": `export const log = console.log.bind(console);`, | ||
| }, | ||
| drop: ["console"], | ||
| bundling: false, | ||
| onAfterBundle(api) { | ||
| api.expectFile("out.js").toContain("log = () => {}"); | ||
| }, | ||
| }); | ||
| // A dropped call loses its arguments, and so does `.bind`: `ASSERT` does not exist at run | ||
| // time, `label.cjs` must not be bundled, and the scopes in the arguments are still visited. | ||
| itBundled("drop/BindArgumentsAreRemoved", { | ||
| files: { | ||
| "/a.js": ` | ||
| const out = value => process.stdout.write(value + "\\n"); | ||
| const fail = () => out("FAIL"); | ||
| const v = 2; | ||
| const check = ASSERT.ok.bind(ASSERT, fail()); | ||
| check(false); | ||
| console.log.bind(console, require("./label.cjs"))("FAIL"); | ||
| console.log.bind(console, v => v, function f(b) { return b; }, class K { m() {} }); | ||
| class After { value() { return v; } } | ||
| const after = () => new After().value(); | ||
| out(after()); | ||
| out("ALIVE"); | ||
| `, | ||
| "/label.cjs": ` | ||
| process.stdout.write("LABEL\\n"); | ||
| module.exports = "[app]"; | ||
| `, | ||
| }, | ||
| drop: ["console", "ASSERT"], | ||
| run: { stdout: "2\nALIVE" }, | ||
| onAfterBundle(api) { | ||
| api.expectFile("out.js").not.toContain("LABEL"); | ||
| }, | ||
| }); | ||
| // These call the dropped function, or a method of the dropped name, so they stay `undefined`. | ||
| // `app.keys.bind` and `lib.hot.keys.bind` are on the drop list, and so are `app` and `lib.hot`. | ||
| itBundled("drop/BindKeepsPlainCallsDropped", { | ||
| files: { | ||
| "/a.js": ` | ||
| const out = value => process.stdout.write(value + "\\n"); | ||
| const fail = () => out("FAIL"); | ||
| out(console.log.call(console, fail())); | ||
| out(console.log.apply(console, [fail()])); | ||
| out(console.log.bind.call(console.log, fail())); | ||
| out(console.bind(fail())); | ||
| out(Mousetrap.bind("ctrl+s", fail())); | ||
| out(hotkeys.bind("ctrl+s", fail())); | ||
| out(app.keys.bind("ctrl+s", fail())); | ||
| out(app.keys.bind?.("ctrl+s", fail())); | ||
| out(app.keys?.bind("ctrl+s", fail())); | ||
| out(app?.keys.bind("ctrl+s", fail())); | ||
| out(app?.keys?.bind?.("ctrl+s", fail())); | ||
| out(lib.hot.keys.bind("ctrl+s", fail())); | ||
| `, | ||
| }, | ||
| drop: ["console", "Mousetrap", "hotkeys.bind", "app", "app.keys.bind", "lib.hot", "lib.hot.keys.bind"], | ||
| run: { stdout: Array(12).fill("undefined").join("\n") }, | ||
| }); | ||
| // One row for each spelling of a callee: "kept" when `callee(arg())` is not dropped, else | ||
| // `typeof callee.bind(null)`. The third column is the value with `minifySyntax`, where | ||
| // `a["b"]` is `a.b`. | ||
| const bindOfDroppedCallee: [callee: string, expected: string, minified?: string][] = [ | ||
| ["debug.log", "function"], | ||
| ["debug?.log", "function"], | ||
| ["(debug.log)", "function"], | ||
| ["debug.a.b", "function"], | ||
| ['debug["log"]', "kept", "function"], | ||
| ["debug[key]", "kept"], | ||
| ["(0, debug.log)", "kept"], | ||
| ["globalThis.debug.log", "kept"], | ||
| ["debug.log.call", "function"], | ||
| ["debug.log.bind", "function"], | ||
| ['debug.log["bind"]', "kept", "function"], | ||
| ["ASSERT.ok", "function"], | ||
| ["my.logger", "function"], | ||
| ["my?.logger", "kept"], | ||
| ['my["logger"]', "kept", "function"], | ||
| ["my.logger.info", "function"], | ||
| ['my["logger"].info', "kept", "function"], | ||
| ['lib["log"].info', "function"], | ||
| // `ASSERT.bind(null)` looks the same as `Mousetrap.bind(key, fn)`: a call of a dropped method. | ||
| ["ASSERT", "undefined"], | ||
| // `debug.a.bind` is on the drop list. | ||
| ["debug.a", "undefined"], | ||
| // Not a `drop` name: the HMR API without HMR. | ||
| ["import.meta.hot.accept", "undefined"], | ||
| // The define renames `renamed` after the drop key matched it. | ||
| ["renamed.log", "undefined"], | ||
| ]; | ||
| for (const minifySyntax of [false, true]) { | ||
| itBundled(`drop/BindOfEachDroppedCallee${minifySyntax ? "MinifySyntax" : ""}`, { | ||
| files: { | ||
| "/a.js": ` | ||
| const fn = () => {}; | ||
| const namespace = { log: fn, info: fn, ok: fn, a: Object.assign(() => {}, { b: fn }) }; | ||
| Object.assign(globalThis, { | ||
| key: "log", | ||
| debug: namespace, | ||
| other: namespace, | ||
| ASSERT: Object.assign(() => {}, namespace), | ||
| my: { logger: Object.assign(() => {}, namespace) }, | ||
| lib: { log: namespace }, | ||
| }); | ||
| let calls = 0; | ||
| const arg = () => calls++; | ||
| ${bindOfDroppedCallee | ||
| .map( | ||
| ([callee]) => ` | ||
| calls = 0; | ||
| ${callee}(arg()); | ||
| process.stdout.write(${JSON.stringify(callee)} + ": " + (calls ? "kept" : typeof ${callee}.bind(null)) + "\\n");`, | ||
| ) | ||
| .join("")} | ||
| `, | ||
| }, | ||
| drop: ["debug", "debug.a.bind", "ASSERT", "my.logger", "lib.log.info", "renamed.log"], | ||
| define: { renamed: "other" }, | ||
| minifySyntax, | ||
| run: { | ||
| stdout: bindOfDroppedCallee | ||
| .map(([callee, expected, minified]) => `${callee}: ${(minifySyntax && minified) || expected}`) | ||
| .join("\n"), | ||
| }, | ||
| }); | ||
| } | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -454,6 +454,20 @@ describe("transpiler cache", () => { | |
| expect(run(temp_dir, ["a.js"])).toBe("logged\nwritten"); | ||
| expect(newCacheCount()).toBe(0); | ||
| }); | ||
|
|
||
| test("--drop keeps a bound console method callable", () => { | ||
| writeFileSync( | ||
| join(temp_dir, "a.js"), | ||
| `const log = console.log.bind(console);\nlog("logged");\nprocess.stdout.write(typeof log + "\\n");` + | ||
| "\n//" + | ||
| filler, | ||
| ); | ||
|
|
||
| expect(run(temp_dir, ["a.js"])).toBe("logged\nfunction"); | ||
| // Transpiled on the first run, served from the cache on the second. | ||
| expect(run(temp_dir, ["--drop=console", "a.js"])).toBe("function"); | ||
| expect(run(temp_dir, ["--drop=console", "a.js"])).toBe("function"); | ||
|
Comment on lines
+467
to
+469
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 nit (optional): the new cache test cannot tell a cache hit from a re-transpile, so the EXPECTED_VERSION bump it accompanies is not exercised. The comment at test/cli/run/transpiler-cache.test.ts:467 says the second run is served from the cache, but no Why this was flaggedThe test at test/cli/run/transpiler-cache.test.ts:458-470 runs the file three times and only compares stdout. Nothing in it reads the cache directory, so the test passes identically whether the second Verification: nit. Triggering condition: whenever the new test runs, it exercises only stdout, so it passes with the on-disk cache engaged, bypassed, or disabled. At test/cli/run/transpiler-cache.test.ts:458-470 the three |
||
| }); | ||
| }); | ||
|
|
||
| // Serving the entry point from the cache must not change how the modules it | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 (optional) Users who drop a namespace whose API has a method literally named
bindnow get a bare() => {}where the base gaveundefined, so guards likeif (h) h.unbind()enter and throw. src/js_parser/visit/visit_expr.rs:1884 only rejects a directX.bindreceiver;app.keys.bind(k, fn)with--drop=apppasses becauseapp.keysis an EDot andis_dropped_namematchesapp. Fix: rewrite to an arrow only when the receiver is a dropped callable method shape the user asked for (for example require the exacta.b.bindnot be a plain method of a dropped namespace, or default toundefinedwhen the bind call carries more than one argument), and apply the same rule at every sibling receiver shape.Why this was flagged
Trigger:
--drop=app(or--drop=$,--drop=Mousetrap-style namespace entries) and a callapp.keys.bind("ctrl+s", fn)reached through e_call at src/js_parser/visit/visit_expr.rs:2212. The guard at src/js_parser/visit/visit_expr.rs:1884 only requiresbind.targetto be an EDot;is_drop_keyat :1901 is false unless the user also listedapp.keys.bind;is_dropped_nameat :1930 returns true as soon as the rootappis a drop entry. The helper then emits() => {}at :1897. On the base the same call reached :2245 and becameundefined. Code that stored the result and guarded it,const h = app.keys.bind(k, fn); if (h) h.unbind(), skipped the branch on the base and now callsunbindon a bare arrow, throwingTypeError. The PR text names this as a downside but sizes it as rare; jQuery-style$.fn.bindand hotkey libraries makebinda common method name under a dropped namespace.Verification: Trigger:
--drop=<ns>where<ns>has a nested object whose own method is literally namedbindand the call's return value is used. At src/js_parser/visit/visit_expr.rs:1884bind.targetis anEDot;is_drop_key(:1901) is false with onlyappdropped;is_dropped_name(:1924) returns true; the call is rewritten toE::Arrow::NOOP_RETURN_UNDEFINEDat :1897. On the base the same input becameundefined.