Skip to content

Remove the dead code from the bake error overlay syntax highlighter - #44514

Open
robobun wants to merge 2 commits into
mainfrom
robobun/b13033e5/bake-highlighter-dead-jsx-state
Open

robobun wants to merge 2 commits into
mainfrom
robobun/b13033e5/bake-highlighter-dead-jsx-state

Conversation

@robobun

@robobun robobun commented Oct 3, 2026 •

Copy link
Copy Markdown
Collaborator

Behaviour change: none

Problem

  • The bake error overlay highlighter has JSX and generic-type state that never runs. lexPunctuator() tests < and > inside "[](){}.,;".includes(char) (src/runtime/bake/client/JavaScriptSyntaxHighlighter.ts:475), and lexOperator() takes both characters first.
  • The type-reference test (:372-382) is always false. It reads prevChar after the identifier is consumed, and the call that sets isAfterExtendsOrImplements also clears it.
  • highlight(), three helper methods and sensitivePatterns have no user.

Fix

  • Delete the dead state: five flags, the 109-entry htmlTags Set, their arms and tests, 21 enum members, the .syntax-red rule, the line/column counters. Delete the unused code: highlight(), the three helpers, sensitivePatterns, three options, one local.
  • Correct: the output is identical. syntaxHighlight output: 0 of 1,878,476 repo lines and 0 of 300,000 seeded fuzz strings differ between main and PR.
  • bake.client.js: 48,402 -> 44,840 bytes (zstd -19: 16,258 -> 15,186). bake.error.js: 23,750 -> 20,188 bytes (zstd -19: 7,824 -> 6,720).
  • Verified: no new test, because no output changes. tsc -p src/runtime/bake/tsconfig.json and test/bake/dev/{bundle,css,esm,harness,hot,html}.test.ts pass on the debug build. Self-reviewed: 4 concerns raised, 3 addressed (Notes).

Background

Downsides

  • JSX tags and attributes keep the plain identifier colour, as on main.
  • No other cost found. Checked the only importer (overlay.ts) and both bundles.
Notes

See the dead state on main

bun -e 'import { syntaxHighlight } from "./src/runtime/bake/client/JavaScriptSyntaxHighlighter.ts"; console.log(syntaxHighlight(`return <div className="a">{x}</div>;`))'

div and className come out as syntax-fg, the plain identifier class. The output is the same on this branch.

Why each piece cannot run (line numbers on main)

  • :475: the < and > arms are inside "[](){}.,;".includes(char). nextToken() also tries lexOperator() before lexPunctuator(), and OPERATORS has < and >. Nothing else sets isInJSXTag, isJSXTagStart or isInGenericType, so the JSX branch (:337-348) and htmlTags (:96-207) are never read.
  • :374: prevChar is this.text[this.pos - 1] after consumeIdentifier(). It is always the last identifier character, never : or <. Three of the four disjuncts of the type-reference test need one of those.
  • :361 and :512: the fourth disjunct is isAfterExtendsOrImplements. The keyword token that sets it is the same token that the reset at the end of nextToken() sees.
  • :487-491: isInDestructuring is written on { and } and read only in the prevChar === ":" conjunct.
  • Token.line and Token.column are filled in createToken() and never read. The per-character loop in consume() exists only to feed them.
  • The 21 enum members are 11 of TokenType and 10 of TokenClass. Five lose their last reference here (TokenType.JSXTag, TokenType.JSXAttribute, TokenClass.JSXTag, TokenClass.JSXComponent, TokenClass.JSXAttribute). The other 16 had none. .syntax-red has no producer once TokenClass.Error is gone (git grep syntax-red).
  • highlight() (:526) lost its only caller when Remove dead code from the streams bindings, node:http, bun_sys, lsquic_sys, and orphaned files #38213 deleted JavaScriptSyntaxHighlighterComponent.tsx. buildHtmlElement() is called only from highlight(). languageName and showLineNumbers are read only there, and redactSensitiveInformation is read nowhere.
  • consumeTemplateString(), shouldRedactSensitive(), sensitivePatterns and the pos local in consumeString(): tsc -p src/runtime/bake/tsconfig.json --noUnusedLocals --noUnusedParameters names this file in 5 diagnostics on main and in 0 on this branch.

Measurements

  • Output identity: the script at the end imports syntaxHighlight from main's file and from this branch and compares the strings. Input: every line of the 11,494 git-tracked .ts, .tsx, .js, .jsx, .mjs, .cjs, .mts and .cts files (216 of them .tsx or .jsx), then 300,000 strings from a seeded generator. The same script reports 18,205 differing lines when main's file is patched to make the < arms reachable, so it does detect a change.
  • Unminified bundles: bake.client.js and bake.error.js each lose 249 lines and change 1 (the CSS string). bake.server.js is byte-identical. Release binaries embed the zstd forms of the first two.
  • Module load builds 7 Sets and 1 Map (main: 9 and 1); the 109-entry htmlTags Set is gone. Counted by wrapping the Set and Map constructors. The release bundler already drops the unused sensitivePatterns, so the shipped bundles build one Set fewer.
  • Per highlighted line, averaged over the same 1,878,476 lines: consume() loop iterations 37.5 -> 0, dead flag reads 5.16 -> 0, dead flag writes 19.38 -> 0, line and column writes 67.62 -> 0, type-reference test runs 2.22 -> 0. Counted with instrumented copies of both files.
  • Conflict hunks against open PRs in this file: Remove dead code from the build scripts, the vscode and debug-adapter packages, the bake overlay, and two WebCore files #40122 1 (it has 1 against main today), Fix the bake error overlay highlighter dropping template literal text after ${ #40125 1 (0 against main), Remove dead code from bun_core, the JSC bindings, the debugger, and 24 other crates #41385 0 (git merge-tree, 7ccf060 against their heads on 2026-10-03).
  • The dev server suites that read build errors from the real overlay (test/bake/dev/{bundle,css,esm,harness,hot,html}.test.ts, 82 tests) pass on the debug build of this branch.

Related open PRs

Self-review

  1. The file kept 116 dead lines that Remove dead code from the build scripts, the vscode and debug-adapter packages, the bake overlay, and two WebCore files #40122 and Remove dead code from bun_core, the JSC bindings, the debugger, and 24 other crates #41385 own. Addressed: this PR deletes them.
  2. The description left out Remove dead code from bun_core, the JSC bindings, the debugger, and 24 other crates #41385 and described the overlap with Fix the bake error overlay highlighter dropping template literal text after ${ #40125 wrongly. Addressed above.
  3. The output check used a script that is not in the repo. Addressed: the script is below.
  4. This PR makes Fix the bake error overlay highlighter dropping template literal text after ${ #40125 conflict in one hunk. Not addressed in the diff: both PRs edit the same block, so one of them must rebase. The result of that rebase is tested (see Related open PRs).

Left alone on purpose

  • typeModifiers repeats what keywordColorMap already says. It runs, so it is not dead.
  • enableColors is always true for the one caller. The constructor still accepts it.
  • The class strings italic, bold and syntax-fg have no CSS rule in overlay.css. A rule or a rename changes what the overlay shows.
  • The < arm of the terminal highlighter (src/bun_core/fmt.rs:2272) scans a tag name and discards it. A comment there says that is intended.

The output check

// git show origin/main:src/runtime/bake/client/JavaScriptSyntaxHighlighter.ts > /tmp/base.ts
// bun check.ts /tmp/base.ts "$PWD/src/runtime/bake/client/JavaScriptSyntaxHighlighter.ts"
import { $ } from "bun";
const { syntaxHighlight: base } = await import(process.argv[2]);
const { syntaxHighlight: head } = await import(process.argv[3]);
const run = (fn: (s: string) => string, s: string) => {
  try {
    return fn(s);
  } catch (e) {
    return "THROW " + String(e);
  }
};
const exts = /\.(?:ts|tsx|js|jsx|mjs|cjs|mts|cts)$/;
let lines = 0, differing = 0;
for (const rel of (await $`git ls-files`.text()).split("\n").filter(f => exts.test(f))) {
  for (const line of (await Bun.file(rel).text()).split("\n")) {
    lines++;
    if (run(base, line) !== run(head, line)) differing++;
  }
}
function mulberry32(a: number) {
  return () => {
    a |= 0; a = (a + 0x6d2b79f5) | 0;
    let t = Math.imul(a ^ (a >>> 15), 1 | a);
    t = (t + Math.imul(t ^ (t >>> 7), 61 | t)) ^ t;
    return ((t ^ (t >>> 14)) >>> 0) / 4294967296;
  };
}
const atoms = [
  "<", ">", "</", "/>", "<>", "</>", "{", "}", "(", ")", "[", "]", ":", ";", ",", ".", "=", "=>", "==", "===", "<=", ">=", "<<", ">>", ">>>",
  "?", "!", "&&", "||", "??", "+", "-", "*", "/", "%", "@", "#", "\\", "\n", "\r", "\t", " ", " ", " ", "  ",
  "'", '"', "`", "${", "//", "/*", "*/",
  "extends", "implements", "interface", "type", "enum", "class", "function", "const", "let", "return", "new", "this", "as", "import", "from",
  "private", "public", "static", "readonly", "abstract", "declare", "namespace", "true", "null", "undefined", "async", "await",
  "div", "span", "a", "b", "p", "Foo", "Bar", "T", "x", "y", "n", "i", "className", "onClick", "Array", "string", "number", "Map", "$", "_",
  "0", "1", "42", "0x1F", "1.5e3", "é", "日本", "😀", "\u2028",
];
const rnd = mulberry32(0xb07a11);
let differingFuzz = 0;
for (let i = 0; i < 300_000; i++) {
  let s = "";
  for (let j = 1 + Math.floor(rnd() * 24); j > 0; j--) s += atoms[Math.floor(rnd() * atoms.length)];
  if (run(base, s) !== run(head, s)) differingFuzz++;
}
console.log({ lines, differing, differingFuzz });

no test proof · iteration 1 · the description declares no behaviour change, so there is no failing test to prove; the existing suite in CI is the check

lexPunctuator() tests '<' and '>' inside a check for "[](){}.,;", and
lexOperator() consumes both characters first. So isInJSXTag,
isJSXTagStart and isInGenericType are never set. The type-reference
test reads the character before the cursor after the identifier is
consumed, and the nextToken() call that sets isAfterExtendsOrImplements
also clears it.

Remove that state, the htmlTags table, the enum members and the CSS
rule that only that state can produce, and the line and column
counters that nothing reads. Also remove highlight() and its helper,
consumeTemplateString(), shouldRedactSensitive(), sensitivePatterns,
three options and a local that nothing uses. syntaxHighlight() returns
the same string for every input.
@github-actions github-actions Bot added the claude label Oct 3, 2026
@robobun

robobun commented Oct 3, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status: ready for review. The diff is green in CI. The one red job comes from a test that this PR does not touch.

CI

  • Build 123170 (7ccf060): 180 of 181 jobs passed. Build 123276 (same tree, one rerun): 178 jobs passed, and 2 darwin x64 test jobs were still queued when I wrote this.
  • The failed job is the same in both builds: debian 13 x64-asan - test-bun, one test in test/js/bun/spawn/spawn.test.ts (stdout reader of an unref'd child and process lifetime > an idle reader stopped at the highwater ...). The same test fails in the final CI builds of recently merged pull requests. This PR changes only src/runtime/bake/client/JavaScriptSyntaxHighlighter.ts and overlay.css.
  • The test/bake suites passed on every lane. The other annotations are tests that passed on a retry.
  • I do not plan another CI rerun.

How to see the dead state on main

bun -e 'import { syntaxHighlight } from "./src/runtime/bake/client/JavaScriptSyntaxHighlighter.ts"; console.log(syntaxHighlight(`return <div className="a">{x}</div>;`))'

div and className get the plain syntax-fg class. The JSX branch that would colour them cannot run, because lexPunctuator() tests < inside a check for "[](){}.,;". This PR deletes that branch and the other unused code in the file. The output of syntaxHighlight() is the same on this branch.

@coderabbitai

coderabbitai Bot commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: oven-sh/bun/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Essentials
  • Run ID: e29a1fd6-bbb7-4e3d-9010-61daf521e2cc
📥 Commits

Reviewing files that changed from the base of the PR and between 7a503a7 and 7ccf060.

📒 Files selected for processing (2)
  • src/runtime/bake/client/JavaScriptSyntaxHighlighter.ts
  • src/runtime/bake/client/overlay.css
💤 Files with no reviewable changes (1)
  • src/runtime/bake/client/overlay.css

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review.


Walkthrough

The bake client highlighter removes several JSX and TypeScript tokenization paths, position tracking, sensitive-information redaction, and its multiline renderer. The highlightLine() method remains. The overlay stylesheet removes the red syntax color.

Changes

JavaScript highlighter

Layer / File(s) Summary
Token model and lexing simplification
src/runtime/bake/client/JavaScriptSyntaxHighlighter.ts
The options and token model lose removed settings and position data. The lexer drops JSX and several type-classification paths, state tracking, and unused string-consumption code.
Renderer and redaction removal
src/runtime/bake/client/JavaScriptSyntaxHighlighter.ts, src/runtime/bake/client/overlay.css
The public highlight() renderer, HTML element builder, and sensitive-value redaction method are removed. The stylesheet removes the red syntax color.

Suggested reviewers: dylan-conway

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to 7ccf0

No actionable merge-blocking risk is established; the change is mergeable after normal checks.

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly and concisely describes the main change: removing dead code from the bake error overlay syntax highlighter.
Description check ✅ Passed The description explains the problem and fix, reports that behavior is unchanged, and gives detailed verification results. It covers both required template topics, even though it does not use the temp…
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — pure dead-code removal in the Bake overlay highlighter; I traced the removed paths and found no output change.

What was reviewed:

  • The removed "type reference" heuristic in lexIdentifierOrKeyword: prevChar is read after consumeIdentifier(), which only consumes IDENTIFIER_PART chars, so it can never be : or <; isAfterExtendsOrImplements is set and cleared inside the same nextToken() call, so it is always false when read. All four disjuncts were unreachable.
  • The JSX/generic state in lexPunctuator: the </> arms sit inside "[](){}.,;".includes(char) and lexOperator runs first, so isInJSXTag/isInGenericType/htmlTags were never set or read.
  • Retained path: overlay.ts is the only importer and uses syntaxHighlight() -> highlightLine(), which is untouched; no remaining references to highlight(), the deleted options, enum members, or .syntax-red in src/runtime/bake/ or test/bake/.
Extended reasoning...

The change deletes 332 lines and adds 1 across src/runtime/bake/client/JavaScriptSyntaxHighlighter.ts and overlay.css, removing unused options, enum members, JSX/generic/destructuring state flags, line/column tracking, the multi-line highlight() method and helpers, and the matching unreferenced CSS rule. It touches no security-sensitive surface; the HTML-escaping in wrap()/escapeHtml() and the retained highlightLine() path are unchanged. I statically verified the one hunk that looked like a behavior change (the type-reference heuristic) is unreachable, so the PR's "no output change" claim holds and no new test is required. The files have no CODEOWNERS entry and the only consumer is overlay.ts, which still compiles against the retained API.

@robobun

robobun commented Oct 3, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 4:22 AM PT - Oct 3rd, 2026

❌ @robobun, your commit e5e9338 has 1 failures in Build #123276 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 44514

That installs a local version of the PR into your bun-44514 executable, so you can run:

bun-44514 --bun

This branch has not been deployed

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant