codegen: reject builtin macro calls with type arguments instead of silently skipping them

.github/workflows/source-lints.yml

@@ -13,6 +13,7 @@ on:
     branches: [main]
     paths:
       - "src/**/*.rs"
+      - "src/codegen/**"
       - "src/jsc/bindings/**"
       - "scripts/build/**"
       - "scripts/glob-sources.ts"
@@ -25,6 +26,7 @@ on:
   pull_request:
     paths:
       - "src/**/*.rs"
+      - "src/codegen/**"
       - "src/jsc/bindings/**"
       - "scripts/build/**"
       - "scripts/glob-sources.ts"

src/codegen/builtin-parser.ts

@@ -9,7 +9,8 @@ function createStopRegex(allow_comma: boolean) {
     "((?:[(,=;:{]|return|\\=\\>)\\s*)\\/[^\\/\\*]|\\/\\*|\\/\\/|['\"}`\\)" +
       (allow_comma ? "," : "") +
       "]|(?<!\\$)\\brequire\\(|(" +
-      function_replacements.map(x => escapeRegex(x) + "\\(").join("|") +
+      // `$macro(` is expanded by applyReplacements; `$macro<` (type arguments) is rejected there.
+      function_replacements.map(x => escapeRegex(x) + "[(<]").join("|") +
       ")",
   );
 }

src/codegen/replacements.ts

@@ -184,7 +184,8 @@ export const function_replacements = [
   "$isPromisePending",
   "$bindgenFn",
 ];
-const function_regexp = new RegExp(`__intrinsic__(${function_replacements.join("|").replaceAll("$", "")})`);
+// Anchored: builtin-parser.ts ends the chunk right after the macro being called; any earlier macro name is a plain reference.
+const function_regexp = new RegExp(`__intrinsic__(${function_replacements.join("|").replaceAll("$", "")})$`);
 
 /** Applies source code replacements as defined in `replacements` */
 export function applyReplacements(src: string, length: number) {
@@ -197,8 +198,14 @@ export function applyReplacements(src: string, length: number) {
       replacement.toRaw ?? replacement.to!.replaceAll("$", "__intrinsic__").replaceAll("%", "$"),
     );
   }
-  let match;
-  if ((match = slice.match(function_regexp)) && rest.startsWith("(")) {
+  const match = slice.match(function_regexp);
+  if (match && rest.startsWith("<")) {
+    throw new Error(
+      `$${match[1]} does not accept type arguments; only '$${match[1]}(' is expanded at bundle time. ` +
+        `Cast the result instead: '$${match[1]}(...) as T'. Found: '$${match[1]}${rest.slice(0, 80)}'`,
+    );
+  }
+  if (match && rest.startsWith("(")) {
     const name = match[1];
     if (name === "debug") {
       const innerSlice = sliceSourceCode(rest, true);

src/js/private.d.ts

@@ -212,7 +212,7 @@ interface JSCommonJSModule {
  *
  * @returns whatever the binding function returns.
  */
-declare function $cpp<T = any>(filename: NativeFilenameCPP, symbol: string): T;
+declare function $cpp(filename: NativeFilenameCPP, symbol: string): any;
 /**
  * Call a native Rust binding function, getting whatever it returns.
  *
@@ -230,25 +230,21 @@ declare function $cpp<T = any>(filename: NativeFilenameCPP, symbol: string): T;
  *
  * @returns whatever the binding function returns.
  */
-declare function $rust<T = any>(filename: NativeFilenameRust, symbol: string): T;
-declare function $newCppFunction<T = (...args: any) => any>(
-  filename: NativeFilenameCPP,
-  symbol: string,
-  argCount: number,
-): T;
-declare function $newRustFunction<T = (...args: any) => any>(
+declare function $rust(filename: NativeFilenameRust, symbol: string): any;
+declare function $newCppFunction(filename: NativeFilenameCPP, symbol: string, argCount: number): (...args: any) => any;
+declare function $newRustFunction(
   filename: NativeFilenameRust,
   symbol: string,
   argCount: number,
-): T;
+): (...args: any) => any;
 /**
  * Retrieves a handle to a function defined in native code, defined in a
  * `.bind.ts` file. For more information on how to define bindgen functions, see
  * [bindgen's documentation](https://bun.com/docs/project/bindgen).
  * @param filename - The basename of the `.bind.ts` file.
  * @param symbol - The name of the function to call.
  */
-declare function $bindgenFn<T = (...args: any) => any>(filename: string, symbol: string): T;
+declare function $bindgenFn(filename: string, symbol: string): (...args: any) => any;
 // NOTE: $debug, $assert, and $isPromiseFulfilled omitted
 
 declare module "node:net" {

test/internal/source-lints/codegen-builtin-macros.test.ts

@@ -0,0 +1,93 @@
+/** Unit tests for the `$macro(...)` expansion that bundle-modules.ts and
+ * bundle-functions.ts apply to the builtin JS in src/js
+ * (src/codegen/builtin-parser.ts + src/codegen/replacements.ts). Nothing here
+ * needs the built binary: the preprocessor is plain TypeScript run at build time. */
+import { describe, expect, test } from "bun:test";
+
+import { sliceSourceCode } from "../../../src/codegen/builtin-parser.ts";
+import { function_replacements } from "../../../src/codegen/replacements.ts";
+
+/** One well-formed call per macro and what it expands to. The native macros
+ * are resolved against the real tree (generate-js2native.ts), so their
+ * arguments name files that exist in src/. */
+const macros: [macro: string, args: string, expansion: string | RegExp][] = [
+  ["$debug", `("x")`, `(IS_BUN_DEVELOPMENT?$debug_log("x"):void 0)`],
+  ["$assert", `(ok, "x")`, `!(IS_BUN_DEVELOPMENT?$assert(ok,"ok", "x"):void 0)`],
+  ["$rust", `("node_util_binding.rs", "internalErrorName")`, /^__intrinsic__lazy\(\d+\)$/],
+  ["$newRustFunction", `("node_util_binding.rs", "internalErrorName", 1)`, /^__intrinsic__lazy\(\d+\)$/],
+  ["$cpp", `("NodeValidator.cpp", "validateInteger")`, /^__intrinsic__lazy\(\d+\)$/],
+  ["$newCppFunction", `("NodeValidator.cpp", "validateInteger", 4)`, /^__intrinsic__lazy\(\d+\)$/],
+  ["$bindgenFn", `("bindgen_test.bind.ts", "add")`, /^__intrinsic__lazy\(\d+\)$/],
+  ["$isPromisePending", `(p)`, `(__intrinsic__peekPromiseStatus(p) === 0)`],
+  ["$isPromiseFulfilled", `(p)`, `(__intrinsic__peekPromiseStatus(p) === 1)`],
+  ["$isPromiseRejected", `(p)`, `(__intrinsic__peekPromiseStatus(p) === 2)`],
+];
+
+function expand(code: string): string {
+  const { result, rest } = sliceSourceCode(`{ const v = ${code}; }`, true);
+  expect(rest).toBe("");
+  expect(result).toStartWith("{ const v = ");
+  expect(result).toEndWith("; }");
+  return result.slice("{ const v = ".length, -"; }".length);
+}
+
+test("every macro the preprocessor expands is covered below", () => {
+  expect(macros.map(([macro]) => macro).sort()).toEqual([...function_replacements].sort());
+});
+
+describe.each(macros)("%s", (macro, args, expansion) => {
+  test("a plain call is expanded", () => {
+    const expanded = expand(`${macro}${args}`);
+    if (typeof expansion === "string") {
+      expect(expanded).toBe(expansion);
+    } else {
+      expect(expanded).toMatch(expansion);
+    }
+  });
+
+  // private.d.ts used to declare the native macros as generic, so
+  // `$newRustFunction<() => number>(...)` type-checked, and the preprocessor
+  // only looked for `$name(`: the call went into the bundle unexpanded and the
+  // native side was never registered, which only surfaced later as a dead-code
+  // error in cargo or as a module that fails to load.
+  test("a call with type arguments is a bundle-time error", () => {
+    expect(() => expand(`${macro}<() => number>${args}`)).toThrow(
+      `${macro} does not accept type arguments; only '${macro}(' is expanded at bundle time. ` +
+        `Cast the result instead: '${macro}(...) as T'. Found: '${macro}<() => number>${args}`,
+    );
+  });
+});
+
+test("type arguments are rejected inside macro arguments and template substitutions too", () => {
+  // $assert's condition is sliced in end-on-comma mode, which uses the other
+  // stop regexp; template substitutions re-enter the slicer.
+  expect(() => expand(`$assert($rust<T>("node_util_binding.rs", "internalErrorName"))`)).toThrow(
+    "$rust does not accept type arguments",
+  );
+  expect(() => expand(`$debug("x", $newCppFunction<T>("NodeValidator.cpp", "validateInteger", 4))`)).toThrow(
+    "$newCppFunction does not accept type arguments",
+  );
+  expect(() => expand('`${$cpp<T>("NodeValidator.cpp", "validateInteger")}`')).toThrow(
+    "$cpp does not accept type arguments",
+  );
+});
+
+test("type arguments inside strings, comments and template text are not code", () => {
+  const code = `{ const s = "$rust<T>(a, b)" + \`$bindgenFn<T>(a, b) \${1}\`; /* $cpp<T>(a, b) */ // $newRustFunction<T>(a, b, 0)\n}`;
+  expect(sliceSourceCode(code, true)).toEqual({ result: code, rest: "" });
+});
+
+test("a macro name that is not being called passes through as a plain intrinsic", () => {
+  // `$debug` is also a value: bundle-modules defines `__intrinsic__debug` to the
+  // debug-logging flag, so `if ($debug)` is a real pattern in src/js.
+  expect(sliceSourceCode(`{ if ($debug) { x(); } }`, true)).toEqual({
+    result: `{ if (__intrinsic__debug) { x(); } }`,
+    rest: "",
+  });
+  expect(expand(`$debug && !handle`)).toBe(`__intrinsic__debug && !handle`);
+});
+
+test("a bare reference earlier in the same chunk does not hijack the call being expanded", () => {
+  expect(expand(`$debug && $debug("x")`)).toBe(`__intrinsic__debug && (IS_BUN_DEVELOPMENT?$debug_log("x"):void 0)`);
+  expect(expand(`$assert && $debug("x")`)).toBe(`__intrinsic__assert && (IS_BUN_DEVELOPMENT?$debug_log("x"):void 0)`);
+});