Conversation
|
Updated 5:56 PM PT - Aug 20th, 2026
✅ @robobun, your commit f52c5be629c213c8a338eee2466a674937478f12 passed in 🧪 To try this PR locally: bunx bun-pr 31999That installs a local version of the PR into your bun-31999 --bun |
|
@robobun adopt |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds default rejection behavior to CSS parser traits, removes explicit rejection methods from several rule parsers, updates browser compatibility data and Rust compat generation, changes one font-size feature lookup, refactors custom property URL serialization, and adds CSS tests. ChangesParser trait defaults and implementation cleanup
CSS browser compatibility and Rust code generation
Custom properties URL serialization refactoring
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Adopted. The compat.rs table conversion that was the headline here has since landed on main via #39770, so this PR is now only the parser dedup (trait defaults plus six deleted impls, and the url_to_css shim removal), +89/-305. Merged with main at f52c5be and mergeable; CSS suites green on the merged head. Waiting on alii to either merge the remaining dedup or close it as not worth a separate landing. |
|
@robobun does anything get faster because of this table? |
|
Runtime: no. Binary size: yes. The CI size check on this build compares against main exactly at the merge point (build 61606 = 09703da), so the delta is this diff alone: -16 KB darwin-x64, -32 KB linux-x64 and x64-baseline, -48 KB darwin-aarch64, -64 KB linux-aarch64. The bigger aarch64 savings fit deleting ~5.2k lines of compare-and-branch codegen (fixed-width instructions make branchy code larger there), replaced by a 7.6 KB static table. Compile time of bun_css: I timed old vs new on this container (3 runs each, dev and release) and the difference is within run-to-run noise, so no claim there. So the honest answer is smaller, not faster; the motivation remains the -5.2k lines and build-prefixes.js being able to regenerate the file. |
# Conflicts: # src/css/properties/custom.rs
|
Merge conflicts resolved (741eebd). The only real conflict was in |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/css/css_parser.rs`:
- Around line 2200-2217: The code uses i32::try_from(...).expect("int cast") on
parser offsets which can panic on large input values, but since this parser path
processes external CSS, panics should be avoided. Replace the expect calls on
the i32::try_from casts for both the location and len values in the
PropertyUsage range creation with proper error handling that returns a Result
instead of panicking, allowing errors to be propagated gracefully to the caller
rather than aborting the process.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c38e13da-fdb2-41f7-9282-10bb84d178b7
📒 Files selected for processing (7)
src/css/css_parser.rssrc/css/declaration.rssrc/css/properties/custom.rssrc/css/properties/font.rssrc/css/rules/font_face.rssrc/css/rules/font_palette_values.rssrc/css/rules/keyframes.rs
💤 Files with no reviewable changes (4)
- src/css/rules/keyframes.rs
- src/css/properties/font.rs
- src/css/rules/font_palette_values.rs
- src/css/rules/font_face.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/css/css_parser.rs`:
- Around line 2200-2217: The code uses i32::try_from(...).expect("int cast") on
parser offsets which can panic on large input values, but since this parser path
processes external CSS, panics should be avoided. Replace the expect calls on
the i32::try_from casts for both the location and len values in the
PropertyUsage range creation with proper error handling that returns a Result
instead of panicking, allowing errors to be propagated gracefully to the caller
rather than aborting the process.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c38e13da-fdb2-41f7-9282-10bb84d178b7
📒 Files selected for processing (7)
src/css/css_parser.rssrc/css/declaration.rssrc/css/properties/custom.rssrc/css/properties/font.rssrc/css/rules/font_face.rssrc/css/rules/font_palette_values.rssrc/css/rules/keyframes.rs
💤 Files with no reviewable changes (4)
- src/css/rules/keyframes.rs
- src/css/properties/font.rs
- src/css/rules/font_palette_values.rs
- src/css/rules/font_face.rs
🛑 Comments failed to post (1)
src/css/css_parser.rs (1)
2200-2217:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid panicking integer casts on parser offsets.
i32::try_from(...).expect("int cast")can panic on large input offsets/lengths. This parser path is fed by external CSS, so it should not abort the process.Suggested fix
- let len = input.position() - location; + let len = input.position() - location; + let start_i32 = i32::try_from(location).unwrap_or(i32::MAX); + let max_len = i32::MAX.saturating_sub(start_i32.max(0)); + let len_i32 = i32::try_from(len).map_or(max_len, |v| v.min(max_len)); let mut usage = PropertyBitset::init_empty(); let mut custom_properties: Vec<&'static [u8]> = Vec::new(); fill_property_bit_set(&mut usage, &declarations, &mut custom_properties); @@ range: bun_ast::Range { loc: bun_ast::Loc { - start: i32::try_from(location).expect("int cast"), + start: start_i32, }, - len: i32::try_from(len).expect("int cast"), + len: len_i32, }, ..Default::default() });As per coding guidelines, “Do not use
.unwrap()on fallible paths that user input or the OS can hit at runtime — return the error instead.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/css/css_parser.rs` around lines 2200 - 2217, The code uses i32::try_from(...).expect("int cast") on parser offsets which can panic on large input values, but since this parser path processes external CSS, panics should be avoided. Replace the expect calls on the i32::try_from casts for both the location and len values in the PropertyUsage range creation with proper error handling that returns a Result instead of panicking, allowing errors to be propagated gracefully to the caller rather than aborting the process.Source: Coding guidelines
|
re the coderabbit finding on On the substance: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs, but this restructures the 215-entry browser-compat dispatch into an order-dependent static table and includes a noted behavior change in custom-property URL serialization (tag.is_internal() → flags.IS_INTERNAL), so it's worth a human sign-off.
Extended reasoning...
Overview
This PR touches 12 files in src/css/: it replaces the ~5.4k-line generated match in compat.rs with a 215-row static MIN_VERSIONS table indexed by Feature as usize, adds default reject-all method bodies to the AtRuleParser/QualifiedRuleParser traits in css_parser.rs and deletes six copy-pasted impls that now fall through to those defaults, drops the inlined url_to_css shim in properties/custom.rs in favor of the shared Url::to_css, and extends build-prefixes.js to regenerate compat.rs (plus restores allBrowsers and maps webview_ios). New tests in css.test.ts pin three compat-table boundaries and the at-rule rejection paths.
Security risks
None identified. The changes are confined to CSS parsing/printing and browser-feature-compat lookup; no auth, network, filesystem, or untrusted-input handling beyond what already existed.
Level of scrutiny
Medium-high. The compat table drives which CSS syntax gets lowered for browser targets — user-visible bundler output. Correctness hinges on the 215 table rows being in exact enum-declaration order (FEATURE_COUNT enforces length but not ordering); the author reports a mechanical 215×9 equivalence audit and the generator reproduces the checked-in file byte-for-byte, but that verification is self-reported. Separately, the url_to_css shim deletion is not a pure no-op: the shim checked import_record.tag.is_internal() while the shared Url::to_css (src/css/values/url.rs:100) checks flags.contains(IS_INTERNAL). The description frames this as the shim having drifted from the Zig reference, which is plausible, but it's an intentional behavior change worth a human eye.
Other factors
The trait-default refactor and the six impl deletions are mechanical and clearly equivalent (the deleted bodies match the new defaults verbatim). The build-prefixes.js changes are dev-tooling not exercised in CI. Existing CSS suites (~2300 tests) plus the new boundary tests provide reasonable coverage, and a maintainer (alii) is already engaged on the thread. Given the scale of the compat.rs rewrite and the small intentional semantic change, I'm deferring rather than auto-approving.
|
On the two points the review bot raised: Table ordering.
Agree it warrants a human sign-off given the table size; leaving it as-is for alii. |
# Conflicts: # src/css/build-prefixes.js # src/css/compat.rs
|
Merge conflicts resolved (091df37). #32621 rewrote
Verified the emission block is byte-identical to the checked-in |
|
@robobun get this mergeable — or close it if the changes are already covered on main / too stale to salvage. |
# Conflicts: # src/css/compat.rs # test/js/bun/css/css.test.ts
|
Mergeable again at 1f544bf (merged main in; GitHub reports mergeable, CI running on the new head). Not covered on main: compat.rs there is still the 5.5k-line match, the AtRuleParser/QualifiedRuleParser defaults do not exist, and the url_to_css shim is still in custom.rs, so the PR is still doing its job rather than being stale. Despite ~1060 commits of drift only two files conflicted:
Everything else (trait defaults, the six impl deletions, the shim removal, |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Given the ~5k-line data-table transformation in compat.rs and the fact that alii is already shepherding it, a human sign-off on the equivalence claim still makes sense.
What was reviewed:
- Spot-checked table rows for
HexAlphaColors,MediaRangeSyntax,FontFamilySystemUi, and the threeNoneentries against the deleted match arms — values andUNSUPPORTEDsentinels line up, and the encoded chrome minimums match the new test's56/62/104 << 16boundaries. - Confirmed
is_compatible/is_partially_compatibletable semantics are equivalent to the old per-browserif version < N/.is_some()chains, including theNone-row early return. - Verified each deleted
AtRuleParser/QualifiedRuleParserimpl body matches the new trait defaults exactly (includingPageRuleParserkeeping its realparse_prelude/parse_blockand only droppingrule_without_block);BasicParseErrorKind::at_rule_invalidaccepts&[u8]via*const [u8]coercion so the droppedstd::ptr::from_refwrapping is a no-op. - Compared the deleted
ext::url_to_cssagainstUrl::to_cssinsrc/css/values/url.rs— the only semantic difference istag.is_internal()vsflags.IS_INTERNAL, already argued as unreachable for CSS url records in the thread.
Extended reasoning...
Overview
This PR refactors src/css/compat.rs from a ~5.2k-line generated match into a 215-row static MIN_VERSIONS table indexed by Feature as usize, rewrites the build-prefixes.js generator to emit that table form, adds default reject-all method bodies to the AtRuleParser/QualifiedRuleParser traits and deletes the six copy-pasted implementations, drops the ext::url_to_css shim in properties/custom.rs in favor of the shared Url::to_css, renames Feature::FontSizeXXXLarge → FontSizeXxxLarge, and adds two test blocks pinning the compat-table boundaries and the trait-default rejection paths.
Security risks
None. This is a pure refactor of CSS bundler internals — no auth, network, filesystem, or user-input validation surface changes. The generated data table drives feature-lowering decisions during CSS transforms; the worst-case failure mode of a wrong entry is emitting a fallback (or not) for a given browser target, not a security issue.
Level of scrutiny
Medium-high. The individual code changes (trait defaults, shim deletion, enum rename) are straightforward and I verified each against what it replaces. The compat table itself is the load-bearing part: 215 rows × 9 browsers of numeric data that cannot be eyeballed. The PR description documents a mechanical row-by-row equivalence audit against the old match form plus a generator round-trip producing byte-identical output, and my spot checks of four features (plus the three None rows) against the deleted match arms all agree. The FEATURE_COUNT const ties table length to the enum, and both enum and rows are emitted from the same sorted list in the generator, so ordering drift on regen is prevented structurally.
Other factors
alii has been actively engaged (asked about perf, asked robobun to get it mergeable) and robobun's own 2026-06-20 comment already flagged that the table size warrants a human sign-off. The 1105-test CSS suite plus the bundler CSS suite pass, and new tests pin three spread table positions at ±1 of their chrome minimum. I found nothing wrong, but the correctness of the full 215×9 data transformation ultimately rests on trusting the author's mechanical audit — which is reasonable, but is exactly the kind of thing a maintainer should nod at rather than a bot.
|
Re the human sign-off on the 215x9 data: agreed, and to make it cheap rather than a trust-me, here is the checker that backs the equivalence claim so anyone can re-run it on the current head. It parses every match arm out of Current head (1f544bf): Sanity that it can fail: bumping one cell by 1 reports verify_compat_table.js (run from the repo root on this branch)// Proves src/css/compat.rs (table form, this branch) encodes exactly the same
// data as the match form on origin/main, for every Feature and every browser,
// and that the table rows are in enum-declaration order.
//
// node verify_compat_table.js (run from the repo root on this branch)
const fs = require("fs");
const { execSync } = require("child_process");
const BROWSERS = ["android", "chrome", "edge", "firefox", "ie", "ios_saf", "opera", "safari", "samsung"];
const UNSUPPORTED = 0xffffffff;
const RENAMES = { FontSizeXXXLarge: "FontSizeXxxLarge" };
// ---- old: match form from main ------------------------------------------
const old = execSync("git show origin/main:src/css/compat.rs", { maxBuffer: 64 << 20 }).toString();
const oldEnum = old
.match(/pub enum Feature \{([\s\S]*?)\n\}/)[1]
.split(",")
.map(s => s.trim())
.filter(Boolean)
.map(n => RENAMES[n] ?? n);
const body = old.slice(old.indexOf("fn is_compatible"), old.indexOf("fn is_partially_compatible"));
const armRe = /((?:Feature::\w+\s*(?:\|\s*)?)+)=>\s*\{([\s\S]*?)\n \}/g;
const oldData = new Map();
let m;
while ((m = armRe.exec(body)) !== null) {
const names = [...m[1].matchAll(/Feature::(\w+)/g)].map(x => RENAMES[x[1]] ?? x[1]);
const arm = m[2];
let row;
if (/^\s*return false;\s*$/.test(arm)) {
row = null;
} else {
row = Object.fromEntries(BROWSERS.map(b => [b, UNSUPPORTED]));
const minRe = /if let Some\(version\) = browsers\.(\w+) \{\s*if version < (\d+) \{/g;
let mm;
while ((mm = minRe.exec(arm)) !== null) row[mm[1]] = Number(mm[2]);
// Every browser must be mentioned either as a min check or in the
// `.is_some()` unsupported clause; anything else means the parser missed
// a shape and the comparison would be meaningless.
for (const b of BROWSERS) {
if (!arm.includes(`browsers.${b}`)) throw new Error(`${names[0]}: browser ${b} not mentioned in arm`);
}
const leftover = arm
.replace(/if let Some\(version\) = browsers\.\w+ \{\s*if version < \d+ \{\s*return false;\s*\}\s*\}/g, "")
.replace(/if (?:browsers\.\w+\.is_some\(\)(?:\s*\|\|\s*)?)+\s*\{\s*return false;\s*\}/g, "")
.trim();
if (leftover) throw new Error(`${names[0]}: unparsed arm content: ${leftover.slice(0, 120)}`);
}
for (const n of names) oldData.set(n, row);
}
// ---- new: table form on this branch ---------------------------------------
const nu = fs.readFileSync("src/css/compat.rs", "utf8");
const nuEnum = nu
.match(/pub enum Feature \{([\s\S]*?)\n\}/)[1]
.split(",")
.map(s => s.trim())
.filter(Boolean);
const rows = nu
.match(/static MIN_VERSIONS[\s\S]*?= \[\n([\s\S]*?)\n\];/)[1]
.split("\n")
.map(s => s.trim())
.filter(Boolean);
const rowOrder = [];
const nuData = new Map();
for (const line of rows) {
const name = line.match(/\/\/ (\w+)\s*$/)[1];
rowOrder.push(name);
if (line.startsWith("None,")) {
nuData.set(name, null);
continue;
}
const row = {};
for (const [, k, v] of line.matchAll(/(\w+): (UNSUPPORTED|\d+)/g)) row[k] = v === "UNSUPPORTED" ? UNSUPPORTED : Number(v);
nuData.set(name, row);
}
// ---- compare ---------------------------------------------------------------
let failures = 0;
const fail = msg => {
failures++;
console.error("FAIL:", msg);
};
if (oldEnum.length !== nuEnum.length) fail(`enum length ${oldEnum.length} (main) vs ${nuEnum.length} (branch)`);
oldEnum.forEach((n, i) => n !== nuEnum[i] && fail(`enum[${i}]: ${n} (main) vs ${nuEnum[i]} (branch)`));
nuEnum.forEach((n, i) => n !== rowOrder[i] && fail(`row[${i}] is ${rowOrder[i]} but enum[${i}] is ${n}`));
for (const n of oldEnum) {
if (!oldData.has(n)) fail(`${n}: no match arm on main`);
if (!nuData.has(n)) fail(`${n}: no table row on branch`);
const a = oldData.get(n), b = nuData.get(n);
if (a === null || b === null) {
const aNone = a === null || BROWSERS.every(k => a[k] === UNSUPPORTED);
const bNone = b === null || BROWSERS.every(k => b[k] === UNSUPPORTED);
if (aNone !== bNone) fail(`${n}: None-row mismatch`);
continue;
}
for (const k of BROWSERS) if (a[k] !== b[k]) fail(`${n}.${k}: ${a[k]} (main) vs ${b[k]} (branch)`);
}
console.log(`features: ${oldEnum.length}, browsers: ${BROWSERS.length}, cells compared: ${oldEnum.length * BROWSERS.length}`);
console.log(failures ? `${failures} failure(s)` : "EQUIVALENT: every feature x browser matches and rows are in enum order");
process.exit(failures ? 1 : 0); |
|
@robobun retry ci — darwin lanes expired (runner timeout) |
|
Retriggered: build 91722 is running on 11f51b9 (empty commit on top of 1f544bf; my Buildkite token is read-only so a push was the only way to get a fresh build). On the old build 91653 the 6 expired darwin jobs had already been auto-retried once and were sitting in scheduled with no runner picking them up; the other 190 jobs had all passed. It was canceled when the new build started. |
There was a problem hiding this comment.
I reviewed this PR and found no bugs. Given the size of the 215×9 compat data table and that alii is already shepherding this, a human sign-off still makes sense.
What was reviewed:
is_compatible/is_partially_compatibletable logic is equivalent to the old match (UNSUPPORTED sentinel ↔.is_some()clause;Nonerow ↔return falsearm); spot-checked several rows against the deleted arms.- New trait defaults return the same errors as every deleted impl body;
PageRuleParsercorrectly keeps its customparse_prelude/parse_blockand only dropsrule_without_block. Url::to_cssinvalues/url.rsmatches the deleted shim except forflags.IS_INTERNALvstag.is_internal(), which the PR shows are both always false for CSS url records.
Extended reasoning...
Overview
Twelve files in src/css/ plus one test file. Four independent refactors bundled: (1) compat.rs — a ~5.2k-line generated match replaced by a 215-row static MIN_VERSIONS table with generic is_compatible/is_partially_compatible; (2) css_parser.rs — default reject-all method bodies added to AtRuleParser/QualifiedRuleParser, with six copy-pasted impls deleted; (3) custom.rs — an inlined url_to_css shim removed in favor of the shared Url::to_css; (4) build-prefixes.js — the generator updated to emit the table form and handle webview_ios. One enum variant renamed (FontSizeXXXLarge → FontSizeXxxLarge) with its one caller updated. Net +430/-5601.
Security risks
None. This is CSS parser/printer internals with no auth, crypto, path handling, or untrusted-length arithmetic in the diff. The only semantic hinge is browser-compat lowering decisions (whether to emit a fallback), and getting those wrong produces suboptimal CSS output rather than a vulnerability.
Level of scrutiny
Medium-high. Each piece is mechanical, but the compat table encodes 1935 data points that cannot be hand-verified in review — correctness rests on the equivalence script the author posted plus spot checks. I verified the new is_compatible logic is algebraically identical to the old per-arm form (including the None → return false case and the UNSUPPORTED sentinel matching the old .is_some() unsupported clause), spot-checked Selectors2, Fullscreen, and the three None rows against the deleted match arms, and confirmed is_partially_compatible's direct table lookup is equivalent to the old probe-via-is_compatible approach. The trait-default dedup is trivially equivalent (deleted bodies match the new defaults byte-for-byte modulo a ptr::from_ref no-op). The url_to_css removal is the one place with a real semantic difference (tag.is_internal() vs flags.IS_INTERNAL); I read src/css/values/url.rs:67-139 and it matches the deleted shim otherwise, and the PR's argument that both flags are always false for CSS import records is well-sourced.
Other factors
The bug-hunting system found nothing. The PR adds boundary tests for three spread table rows and rejection-default tests for all five affected rule contexts. alii has been actively engaged (perf question, mergeability, CI retry) and the author's own agent noted on 2026-06-20 that the table size warrants a human sign-off. Given that context — a large, provably-but-not-visually-verifiable data refactor with an engaged human reviewer already in the loop — deferring rather than auto-approving is the right call.
|
The retry (build 91722) hit the same thing: 190/196 jobs passed, and all 6 darwin test jobs (14 x64, 14 aarch64, 26 aarch64) were canceled without ever starting; same as 91653 before it. It does not look specific to this branch: the last main build that ran darwin test jobs was 91391 about 17 hours ago, and the current main build 91743 has its darwin jobs sitting in waiting too, so the darwin runners appear to be down or badly backed up. Another retrigger from my side would just queue the same 6 jobs again, so I am holding off; once darwin agents are picking up jobs again, a rebuild of 91722 (or a fresh push) should go green. Everything non-darwin, including all the CSS suites, is green on this head. |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Given the size of the data-table conversion (215×9 cells) and the url_to_css shim removal that swaps tag.is_internal() for flags.IS_INTERNAL, a human sign-off still makes sense — which lines up with what was already agreed in-thread.
What was checked:
is_compatible/is_partially_compatibletable-lookup semantics are equivalent to the deleted match form (including theUNSUPPORTEDsentinel andNonerows); spot-checked HexAlphaColors, MediaRangeSyntax, FontFamilySystemUi, and the threeNoneentries against the deleted arms.- All
AtRuleParser/QualifiedRuleParserimplementers enumerated: only the six edited in this PR pick up the new defaults;TopLevelRuleParser/NestedRuleParserkeep their explicit impls. Url::to_cssinvalues/url.rsmatches the removed shim except for theis_internalsource, which the description shows is unreachable for CSS import records.
Extended reasoning...
Overview
Twelve files in src/css/ plus one test file. The bulk of the diff (-5.2k lines) is compat.rs, where a generated per-feature match of browser-version checks is replaced by a static MIN_VERSIONS: [Option<MinVersions>; FEATURE_COUNT] table indexed by Feature as usize, with generic is_compatible/is_partially_compatible functions that walk the row via a check! macro. build-prefixes.js is updated to emit the table form (plus webview_ios: null and the fontSizeXxxLarge casing fix). css_parser.rs gains default reject-all bodies on the AtRuleParser/QualifiedRuleParser traits, and six impl sites drop their identical hand-written reject-all bodies. custom.rs deletes an inlined url_to_css shim in favor of the shared Url::to_css. font.rs follows the FontSizeXXXLarge → FontSizeXxxLarge rename.
Security risks
None identified. This is a build-time CSS lowering data table and parser-trait boilerplate; no untrusted-input parsing, auth, crypto, or filesystem paths change. The generator script is developer-run, not shipped.
Level of scrutiny
Higher than a typical mechanical refactor because a single wrong cell in the 215×9 table would silently mis-lower CSS for one browser target with no compile error. The equivalence claim is well-supported (a re-runnable verifier script comparing every cell against main's match form is posted in-thread, and boundary tests for three spread features are added), but the sheer volume of data warrants a maintainer glance rather than a bot approval. The url_to_css shim removal also swaps which ImportRecord field gates the internal-URL branch — argued to be a no-op for CSS records, but that argument depends on no bundler path ever setting either field on a CSS import record.
Other factors
I verified the new is_partially_compatible is algebraically identical to the old probe-one-browser-at-a-time form. I enumerated every AtRuleParser/QualifiedRuleParser implementer via grep and confirmed only the six edited files rely on the new defaults; the top-level and nested rule parsers retain explicit method bodies and are unaffected. PageRuleParser correctly keeps its real parse_prelude/parse_block and only drops rule_without_block. New tests pin the three probed table rows and the at-rule rejection error message. CI on the previous head was green on 190/196 lanes with the remaining 6 being darwin runner-availability failures, not test failures. The maintainer driving this has already asked for it to be made mergeable, so deferring rather than staying silent gives them the no-bugs-found signal.
|
Good prompt on the Repo-wide, Why it still cannot diverge from Confirmed empirically rather than just by reading: bundled |
|
Build 91770 on your merge (c201cfd) finished 187/190. Two observations:
|
There was a problem hiding this comment.
I reviewed this PR and the automated bug hunt found no issues. Given the size of the 215×9 compat data table and the tag.is_internal() → flags.IS_INTERNAL change in the url_to_css path, a human sign-off still makes sense (as already noted in the thread).
What was reviewed:
is_compatible/is_partially_compatibletable-lookup semantics vs. the old match arms — equivalent for supported, unsupported (.is_some()), andreturn false(→None) cases; spot-checkedHexAlphaColorsand several list-style rows against the deleted arms.- The new
AtRuleParser/QualifiedRuleParserdefaults produce the sameBasicParseErrorKindvariants as every deleted impl;PageRuleParsercorrectly keeps its customparse_prelude/parse_blockand only dropsrule_without_block. Url::to_cssinvalues/url.rsmatches the deleted shim except for theis_internalpredicate, which the thread's analysis covers.
Extended reasoning...
Overview
Twelve files in src/css/: (1) compat.rs replaces a ~5.2k-line generated match with a 215-row MIN_VERSIONS static table indexed by Feature as usize, plus generic is_compatible/is_partially_compatible; (2) css_parser.rs gives AtRuleParser/QualifiedRuleParser default reject-all method bodies and six rule parsers drop their copy-pasted impls; (3) properties/custom.rs deletes the inlined url_to_css shim in favor of the shared Url::to_css; (4) build-prefixes.js emits the table form and adds webview_ios: null / the fontSizeXxxLarge key; (5) Feature::FontSizeXXXLarge → FontSizeXxxLarge with the one call site in font.rs updated; (6) two new test blocks in css.test.ts.
Security risks
None. No untrusted-input parsing, auth, crypto, or filesystem paths are touched. The compat table only influences which CSS syntax variants are emitted for given browser targets.
Level of scrutiny
Medium-high. The line count is dominated by generated-data reshaping, and the trait-default dedup is mechanical, but two aspects need human judgement: (a) the 1935-cell data table drives user-visible CSS lowering — a single wrong cell silently changes output for some browser target, and while the posted verify_compat_table.js checker is convincing, that's exactly what a maintainer should confirm rather than an auto-approval; (b) the url_to_css shim removal changes the is_internal predicate from tag.is_internal() to flags.IS_INTERNAL, argued equivalent for CSS records via a chain of reasoning about where tags/flags are written in the bundler — well-argued and empirically tested in the thread, but subtle enough to warrant maintainer sign-off.
Other factors
The new is_compatible logic is a straightforward encoding of the old per-arm shape (version < min → false; .is_some() on unsupported → false via UNSUPPORTED sentinel; empty arm → None row → false), and is_partially_compatible is now a direct table check equivalent to the old probe-one-browser-at-a-time loop. FEATURE_COUNT = last_variant as usize + 1 ties table length to the enum at compile time, and the generator emits both from the same sorted list so ordering can't drift. The trait defaults use the same BasicParseErrorKind::at_rule_invalid(name) call form that declaration.rs already used on main. CI is green on all lanes that ran (darwin fleet offline is infrastructure, not this diff), css.test.ts and test/bundler/css/ pass, and clippy is clean. alii is actively reviewing; the thread already flagged that the table size warrants human sign-off, which I agree with.
|
Correction to my earlier note about darwin jobs being absent: that is by design, not a skipped step. #37354 (which your merge pulled in) only schedules darwin tests when If you want darwin coverage on this diff before merging anyway, say so and I will push an empty commit with |
# Conflicts: # src/css/build-prefixes.js # src/css/compat.rs
robobun
changed the title
|
Status change worth a decision from you: #39770 landed the compat.rs table conversion on main (a What is left is the parser dedup: the So per your earlier instruction: it is mergeable as a small dedup, or if you consider the remaining ~300 lines not worth a separate landing now that the table is done, close it and nothing is lost. Your call; I will not push again unless you want something changed. |
This branch has not been deployed
2 participants
What this does
Split from #31912. Behavior-preserving dedup in the CSS parser, net +89/-305 across 9 files.
This PR originally also converted
compat.rsfrom a generatedmatchto a data table. #39770 landed an equivalent conversion on main (a[[u32; 9]; 215]table emitted bybuild-prefixes.js), so that part, the generator changes, and theFontSizeXxxLargerename that only existed to go with it were dropped here in favor of main's version. Also dropped, and not shipped by this PR: the one-linewebview_ios: nullentries this branch used to add to the generator's browser maps so it runs against newer BCD data; that belongs with the generator now owned by #39770 and can land on its own. What remains:src/css/css_parser.rs:AtRuleParserandQualifiedRuleParserget default method bodies that reject the rule (at_rule_invalid/at_rule_body_invalid/qualified_rule_invalid, andErr(())forrule_without_block).declaration.rs,rules/font_face.rs,rules/font_palette_values.rs,rules/keyframes.rs,rules/page.rs,rules/property.rs: delete the copy-pasted reject-all impls, which were byte-for-byte the new defaults.PageRuleParserkeeps its realparse_prelude/parse_blockand only dropsrule_without_block.TopLevelRuleParser/NestedRuleParserkeep their explicit impls and are unaffected.src/css/properties/custom.rs: drop the inlinedurl_to_cssshim and call the sharedUrl::to_css(already used byfont_face.rs,image.rs,syntax.rs). The shim differed only in testingtag.is_internal()whereUrl::to_csstestsflags.IS_INTERNAL. For CSS url records the two cannot diverge:IS_INTERNALis only set by the JS parser, and the one bundler site that can put a>= Runtimetag (the thresholdTag::is_internaluses) on a CSS record isbun:wrap, which is rejected at resolve time before anything prints; builtin aliases setBuiltin, below the threshold. Verified by bundlingurl()ofbun:wrap/node:fs/bun:sqlite/bun:ffi/fsunder both targets on this branch and on the release build: identical output in all ten cases.Tests
test/js/bun/css/css.test.tsgains two blocks:at-rule and qualified-rule rejection defaults: unknown at-rules and qualified rules inside@font-face,@keyframes,@font-palette-values,@property, and style attributes now hit the trait defaults; the tests pin the recovery output and theUnknown at-rule @boguserror text.browser compat feature table: three features probed one version below and at their chrome minimum. These were written against this PR's table and now run against the Trim ~2 MB from the release binary without touching hot paths #39770 table on main, where they pass, so they double as a cross-check of that table and are kept.Both blocks pass before and after by construction (the change is a dedup); they were run on the merge base and on this branch with identical results.
Verification
cargo check/cargo clippy -p bun_cssclean on the merged head.bun bd test test/js/bun/css/css.test.ts1193 pass,bun bd test test/bundler/css/169 pass.