Skip to content

Conversation

@tomocrafter
Copy link

What does this PR do?

  • Fixes Bun’s TS/TSX parser so declare is only treated as an ambient modifier when followed by a valid declaration keyword (var/let/const/function/class/enum/import or contextual
    namespace/module/global/abstract/interface/type). Otherwise declare remains a normal identifier.
  • This prevents the TS/TSX path from stripping legitimate assignments like declare = "i = 0, len = arr.length" in plain JS code that happens to be parsed as TypeScript (e.g., extensionless
    entrypoints). Previously this led to miscompiled output such as for (undefined; i < len; i++).

Repro snippet

  function makeFor(svar) {
    var declare;
    declare = "i = 0, len = " + svar + ".length";
    var compare = "i < len";
    var increment = "++i";
    return declare + "; " + compare + "; " + increment;
  }
  makeFor("arr");
Here is full repro code with latest bun, and other transpliers
// Minimal repro showing Bun's TS/TSX transpiler stripping a `declare = ...` assignment
// while other transpilers keep it intact.
//
// Source is intentionally tiny and mirrors the CoffeeScript compiler pattern that
// builds a `for (...)` header string using a local variable named `declare`.

const source = `
function makeFor(svar) {
  var declare
  declare = "i = 0, len = " + svar + ".length";
  var compare = "i < len";
  var increment = "++i";
  return declare + "; " + compare + "; " + increment;
}
function run() { return makeFor("arr"); }
run();`;

function runCode(label, code) {
  try {
    console.log(`${label}: `, code);
    const fn = new Function(`${code}; return run();`);
    const result = fn();
    console.log(`${label}:`, result);
  } catch (err) {
    console.log(`${label}: ERROR`, err.message);
  }
}

function tryRequire(name) {
  try {
    return require(name);
  } catch {
    console.log(`${name} not available, skipping`);
    return null;
  }
}

console.log("=== Source run (no transpile) ===");
runCode("raw", source);

console.log("\n=== Bun transpiler ===");
if (typeof Bun !== "undefined" && Bun.Transpiler) {
  const bunTranspiler = new Bun.Transpiler({});
  const bunJs = bunTranspiler.transformSync(source, "js");
  const bunTs = bunTranspiler.transformSync(source, "ts"); // mirrors extensionless .tsx default
  runCode("bun loader=js", bunJs);
  runCode("bun loader=ts", bunTs);
} else {
  console.log(
    "Bun not available in this runtime, skipping Bun transpiler checks"
  );
}

console.log("\n=== TypeScript (ts.transpileModule) ===");
const ts = tryRequire("typescript");
if (ts) {
  const out = ts.transpileModule(source, {
    compilerOptions: {
      module: ts.ModuleKind.CommonJS,
      target: ts.ScriptTarget.ES2020,
    },
  }).outputText;
  runCode("tsc transpileModule", out);
}

console.log("\n=== esbuild transform (loader=ts) ===");
const esbuild = tryRequire("esbuild");
if (esbuild) {
  const out = esbuild.transformSync(source, {
    loader: "ts",
    format: "cjs",
  }).code;
  runCode("esbuild loader=ts", out);
}

console.log("\n=== swc transform (@swc/core, syntax=typescript) ===");
const swc = tryRequire("@swc/core");
if (swc) {
  const out = swc.transformSync(source, {
    filename: "file.ts",
    jsc: {
      parser: { syntax: "typescript" },
      target: "es2020",
    },
    module: { type: "commonjs" },
  }).code;
  runCode("swc loader=ts", out);
}

console.log("\n=== oxc transform (oxc-transform, syntax=ts) ===");
const oxc = tryRequire("oxc-transform");
if (oxc) {
  try {
    const { transformSync } = oxc;
    const { code, errors } = transformSync("file.ts", source, {
      typescript: {
        // declaration: false,
      },
    });
    if (errors && errors.length) {
      console.log("oxc errors:", errors.map((e) => e.message).join("; "));
    }
    runCode("oxc loader=ts", code);
  } catch (err) {
    console.log("oxc error:", err.message);
  }
}

Output:

❯ bun repro-bun-ts-declare.js
=== Source run (no transpile) ===
raw: i = 0, len = arr.length; i < len; ++i

=== Bun transpiler ===
bun loader=js: i = 0, len = arr.length; i < len; ++i
bun loader=ts: undefined; i < len; ++i

=== TypeScript (ts.transpileModule) ===
tsc transpileModule: i = 0, len = arr.length; i < len; ++i

=== esbuild transform (loader=ts) ===
esbuild loader=ts: i = 0, len = arr.length; i < len; ++i

=== swc transform (@swc/core, syntax=typescript) ===
swc loader=ts: i = 0, len = arr.length; i < len; ++i

=== oxc transform (oxc-transform, syntax=ts) ===
oxc loader=ts: i = 0, len = arr.length; i < len; ++i
  • Before (Bun loader=ts/tsx): undefined; i < len; ++i
  • After: i = 0, len = arr.length; i < len; ++i
  • Other transpilers (TypeScript, esbuild, swc, oxc) already keep the assignment.

How did you verify your code works?

  • Ran the minimal repro (repro-bun-ts-declare.js), which exercises:
    • Bun transpiler loader=js (baseline, correct)
    • Bun transpiler loader=ts (previously wrong, now matches JS)
    • Optional checks against typescript, esbuild, @swc/core, and oxc-transform when installed; all retain the assignment.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 3, 2025

Walkthrough

Adds validation in the TypeScript "declare" ambient modifier parsing flow to inspect the next token. If "declare" is followed by declaration keywords (var, let, const, function, class, enum, import, namespace, module, global, abstract, interface, type), it proceeds as ambient declaration; otherwise, "declare" falls back to being treated as a plain identifier.

Changes

Cohort / File(s) Summary
TypeScript "declare" keyword validation
src/ast/parseStmt.zig
Adds next-token inspection after "declare" to validate whether it should be treated as TypeScript ambient modifier or plain identifier

Possibly related PRs

  • oven-sh/bun#23284: Modifies the same file to tighten ambient declaration parsing, adding a no-newline-before check for TypeScript "declare" handling.

Suggested reviewers

  • pfgithub
  • dylan-conway

Pre-merge checks

✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: preventing declare assignments from being stripped in TypeScript/TSX parsing.
Description check ✅ Passed The PR description includes both required template sections with detailed explanations, code examples, and comprehensive verification details.

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between e9e9324 and 5abe8bc.

📒 Files selected for processing (1)
  • src/ast/parseStmt.zig (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.{cpp,zig}

📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)

src/**/*.{cpp,zig}: Use bun bd or bun run build:debug to build debug versions for C++ and Zig source files; creates debug build at ./build/debug/bun-debug
Run tests using bun bd test <test-file> with the debug build; never use bun test directly as it will not include your changes
Execute files using bun bd <file> <...args>; never use bun <file> directly as it will not include your changes
Enable debug logs for specific scopes using BUN_DEBUG_$(SCOPE)=1 environment variable
Code generation happens automatically as part of the build process; no manual code generation commands are required

Files:

  • src/ast/parseStmt.zig
src/**/*.zig

📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)

Use bun.Output.scoped(.${SCOPE}, .hidden) for creating debug logs in Zig code

Implement core functionality in Zig, typically in its own directory in src/

src/**/*.zig: Private fields in Zig are fully supported using the # prefix: struct { #foo: u32 };
Use decl literals in Zig for declaration initialization: const decl: Decl = .{ .binding = 0, .value = 0 };
Prefer @import at the bottom of the file (auto formatter will move them automatically)

Be careful with memory management in Zig code - use defer for cleanup with allocators

Files:

  • src/ast/parseStmt.zig
**/*.zig

📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)

**/*.zig: Expose generated bindings in Zig structs using pub const js = JSC.Codegen.JS<ClassName> with trait conversion methods: toJS, fromJS, and fromJSDirect
Use consistent parameter name globalObject instead of ctx in Zig constructor and method implementations
Use bun.JSError!JSValue return type for Zig methods and constructors to enable proper error handling and exception propagation
Implement resource cleanup using deinit() method that releases resources, followed by finalize() called by the GC that invokes deinit() and frees the pointer
Use JSC.markBinding(@src()) in finalize methods for debugging purposes before calling deinit()
For methods returning cached properties in Zig, declare external C++ functions using extern fn and callconv(JSC.conv) calling convention
Implement getter functions with naming pattern get<PropertyName> in Zig that accept this and globalObject parameters and return JSC.JSValue
Access JavaScript CallFrame arguments using callFrame.argument(i), check argument count with callFrame.argumentCount(), and get this with callFrame.thisValue()
For reference-counted objects, use .deref() in finalize instead of destroy() to release references to other JS objects

Files:

  • src/ast/parseStmt.zig
🧠 Learnings (2)
📓 Common learnings
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: packages/bun-otel/tsconfig.json:1-15
Timestamp: 2025-10-19T02:52:37.412Z
Learning: In the Bun repository, packages under packages/ (e.g., bun-otel) can follow a TypeScript-first pattern where package.json exports point directly to .ts files (not compiled .js files). Bun natively runs TypeScript, so consumers import .ts sources directly and receive full type information without needing compiled .d.ts declaration files. For such packages, adding "declaration": true or "outDir" in tsconfig.json is unnecessary and would break the export structure.
<!-- [remove_learning]
ceedde95-980e-4898-a2c6-40ff73913664
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:11.466Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.ts : Builtin functions must include `this` parameter typing in TypeScript to enable direct method binding in C++
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:11.466Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js} : Use `$debug()` for debug logging and `$assert()` for assertions; both are stripped in release builds
Learnt from: RiskyMH
Repo: oven-sh/bun PR: 24719
File: docs/bundler/executables.mdx:527-560
Timestamp: 2025-11-14T16:07:01.064Z
Learning: In the Bun repository, certain bundler features like compile with code splitting (--compile --splitting) are CLI-only and not supported in the Bun.build() JavaScript API. Tests for CLI-only features use backend: "cli" flag (e.g., test/bundler/bundler_compile_splitting.test.ts). The CompileBuildConfig interface correctly restricts these with splitting?: never;. When documenting CLI-only bundler features, add a note clarifying they're not available via the programmatic API.
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:11.466Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js} : Use string literal `require()` statements only; dynamic requires are not permitted
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 22946
File: test/js/sql/sql.test.ts:195-202
Timestamp: 2025-09-25T22:07:13.851Z
Learning: PR oven-sh/bun#22946: JSON/JSONB result parsing updates (e.g., returning parsed arrays instead of legacy strings) are out of scope for this PR; tests keep current expectations with a TODO. Handle parsing fixes in a separate PR.
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:08.612Z
Learning: Applies to test/bake/dev/bundle.test.ts : Organize bundle tests in bundle.test.ts for tests concerning bundling bugs that only occur in DevServer
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:08.612Z
Learning: Applies to test/bake/dev/sourcemap.test.ts : Organize source-map tests in sourcemap.test.ts for tests verifying source-maps are correct
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add corresponding test cases to test/v8/v8.test.ts using checkSameOutput() function to compare Node.js and Bun output
📚 Learning: 2025-11-24T18:36:38.104Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:38.104Z
Learning: Applies to src/**/*.zig : Use decl literals in Zig for declaration initialization: `const decl: Decl = .{ .binding = 0, .value = 0 };`

Applied to files:

  • src/ast/parseStmt.zig

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant