-
Notifications
You must be signed in to change notification settings - Fork 5.1k
bun_runtime: restore the bun_css dependency edge #39583
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,159 @@ | ||
| import { file } from "bun"; | ||
| import { expect, test } from "bun:test"; | ||
| import path from "path"; | ||
|
|
||
| // Every `bun_*::` path a crate's code names must have a matching dependency | ||
| // edge in that crate's Cargo.toml. cargo enforces this at compile time, but | ||
| // only for the crates a given change rebuilds — a dead-code sweep that prunes | ||
| // a "textually unreferenced" edge can race a PR that adds new uses of it, and | ||
| // both land green (#37301 removed bun_runtime's `bun_css` edge after being | ||
| // verified against a tree that predated #39229's `bun_css::StyleSheet` calls | ||
| // in pm_diff_normalize.rs; merged main then failed to build everywhere). | ||
| // | ||
| // This lint re-checks the pairing textually on every tree, so whichever PR | ||
| // merges second goes red on its own CI instead of breaking main. | ||
|
Check warning on line 14 in test/internal/source-lints/cargo-dep-edges.test.ts
|
||
| // | ||
| // Matching is deliberately conservative: | ||
| // - only crate names that exist as workspace members count; | ||
| // - only `bun_x::` path uses count (`::bun_x::` absolute paths, the | ||
| // macro-definition idiom that resolves at the expansion site, do not); | ||
| // - comments and string literals are stripped before matching. | ||
|
|
||
| const root = path.resolve(import.meta.dir, "..", "..", ".."); | ||
|
|
||
| // Workspace member crates under src/*/ (the only layout the workspace uses; | ||
| // src/install/windows-shim is a standalone nested crate, excluded below). | ||
| interface Crate { | ||
| name: string; | ||
| dir: string; // repo-relative, posix separators | ||
| deps: Set<string>; | ||
| } | ||
|
|
||
| const crates: Crate[] = []; | ||
| for (const manifest of new Bun.Glob("src/*/Cargo.toml").scanSync({ cwd: root })) { | ||
| const dir = path.dirname(manifest).replaceAll(path.sep, "/"); | ||
| const toml = await file(path.join(root, manifest)).text(); | ||
| const name = /^\s*name\s*=\s*"([^"]+)"/m.exec(toml)?.[1]; | ||
| if (!name) continue; | ||
|
|
||
| // Keys of every [*dependencies*] section: [dependencies], | ||
| // [dev-dependencies], [build-dependencies], [target.'...'.dependencies]. | ||
| // `alias = { package = "real_name", ... }` declares `real_name`, but the | ||
| // in-code path uses the alias, so record both. | ||
| const deps = new Set<string>(); | ||
| let inDeps = false; | ||
| for (const line of toml.split("\n")) { | ||
| const section = /^\s*\[(.+)\]\s*$/.exec(line); | ||
| if (section) { | ||
| inDeps = /dependencies/.test(section[1]); | ||
| // `[dependencies.foo]` declares `foo` in the header itself. | ||
| const headerDep = /(?:^|\.)dependencies\.([A-Za-z0-9_-]+)$/.exec(section[1]); | ||
| if (headerDep) deps.add(headerDep[1].replaceAll("-", "_")); | ||
| continue; | ||
| } | ||
| if (!inDeps) continue; | ||
| // `name = ...`, `name.workspace = true`, `name.version = "..."`. | ||
| const key = /^\s*([A-Za-z0-9_-]+)\s*[.=]/.exec(line); | ||
| if (!key) continue; | ||
| deps.add(key[1].replaceAll("-", "_")); | ||
| const renamed = /\bpackage\s*=\s*"([^"]+)"/.exec(line); | ||
| if (renamed) deps.add(renamed[1].replaceAll("-", "_")); | ||
| } | ||
| crates.push({ name, dir, deps }); | ||
| } | ||
|
|
||
| const workspaceNames = new Set(crates.map(c => c.name)); | ||
|
|
||
| // Tracked .rs files only (a `git stash` round-trip can leave stray files). | ||
| const trackedRs: string[] = (() => { | ||
| const r = Bun.spawnSync({ | ||
| cmd: ["git", "-C", root, "ls-tree", "-r", "--name-only", "-z", "HEAD"], | ||
| stdout: "pipe", | ||
| stderr: "ignore", | ||
| }); | ||
| if (!r.success) throw new Error("git ls-tree failed"); | ||
| return r.stdout | ||
| .toString() | ||
| .split("\0") | ||
| .filter(p => p.startsWith("src/") && p.endsWith(".rs")); | ||
| })(); | ||
|
|
||
| // Nested standalone crates: their files belong to themselves, not to the | ||
| // src/*/ crate that contains them. | ||
| const nestedCrateDirs = ["src/install/windows-shim/"]; | ||
|
|
||
| // Files mounted into a different crate's module tree via `#[path]`; their | ||
| // references resolve against the mounting crate's dependencies. | ||
| const mountedElsewhere: Record<string, string> = { | ||
| "src/jsc/generated_classes_list.rs": "bun_runtime", // src/runtime/lib.rs `#[path] pub mod generated_classes_list` | ||
| }; | ||
|
Check warning on line 89 in test/internal/source-lints/cargo-dep-edges.test.ts
|
||
|
Comment on lines
+87
to
+89
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. 🟡 Extended reasoning...What the gap is
#[path = "../bun.js.rs"]
pub mod run_main; // line 49
...
#[path = "../jsc/generated_classes_list.rs"]
pub mod generated_classes_list; // line 55
Why it falls through
|
||
|
|
||
| function crateFor(rel: string): Crate | undefined { | ||
| if (nestedCrateDirs.some(d => rel.startsWith(d))) return undefined; | ||
| const mounted = mountedElsewhere[rel]; | ||
| if (mounted) return crates.find(c => c.name === mounted); | ||
| return crates.find(c => rel.startsWith(c.dir + "/")); | ||
| } | ||
|
|
||
| /** Remove comments and string literals so prose mentions don't count. */ | ||
| function stripNonCode(src: string): string { | ||
| return src.replace( | ||
| // raw strings, byte strings, plain strings, char literals, line and block | ||
| // comments. The char-literal arm matches exactly one unit (`'x'`, `'\n'`, | ||
| // `'\u{1F600}'`) so a lifetime's lone `'` can never open a match that | ||
| // swallows code. | ||
| /r#*"[\s\S]*?"#*|b?"(?:[^"\\]|\\[\s\S])*"|b?'(?:\\u\{[^}']*\}|\\[^\n]|[^'\\\n])'|\/\/[^\n]*|\/\*[\s\S]*?\*\//g, | ||
| " ", | ||
| ); | ||
| } | ||
|
|
||
| // `bun_x::` as a path head. The lookbehind rejects `::bun_x::` (absolute | ||
| // paths inside macro definitions resolve where the macro expands) and | ||
| // identifier tails like `other_bun_x::`. | ||
| const PATH_USE = /(?<![\w:])(bun_[a-z0-9_]+)(?=::)/g; | ||
|
|
||
| // A `bun_*` name can be bound locally instead of naming the crate: | ||
| // extern crate bun_core as bun_output; (crate-wide rename) | ||
| // use bun_core::output as bun_output; (module alias) | ||
| // pub mod bun_spawn { ... } (local module shadows the crate) | ||
| // Any such binding anywhere in a crate takes that name out of this lint for | ||
| // the whole crate; rustc resolves those for real. | ||
|
Check failure on line 120 in test/internal/source-lints/cargo-dep-edges.test.ts
|
||
|
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. 🔴 The Extended reasoning...What the bug is
Step-by-step proofRunning the actual regex against real source lines: const re = /\b(?:mod\s+(bun_[a-z0-9_]+)\b(?!\s*::)|as\s+(bun_[a-z0-9_]+)\b)/g;
[..."s.st_mode as bun_sys::Mode".matchAll(re)] // → m[2] === 'bun_sys'
[..."<HTMLBundle as bun_jsc::JsClass>::from_js(x)".matchAll(re)] // → m[2] === 'bun_jsc'Both capture the crate name into
Why existing code doesn't prevent itThe ImpactIf any of the eight edges above were dropped from FixAdd the same negative lookahead the const LOCAL_BINDING = /\b(?:mod\s+(bun_[a-z0-9_]+)\b(?!\s*::)|as\s+(bun_[a-z0-9_]+)\b(?!\s*::))/g;This still matches |
||
| const LOCAL_BINDING = /\b(?:mod\s+(bun_[a-z0-9_]+)\b(?!\s*::)|as\s+(bun_[a-z0-9_]+)\b)/g; | ||
|
|
||
| // The scan runs at module scope (like the other lints here): per-test | ||
| // timeouts then cover only the assertion, not the repo walk, which is slow | ||
| // under a debug + ASAN build. | ||
| const byCrate = new Map<Crate, { files: string[]; shadowed: Set<string> }>(); | ||
| const stripped = new Map<string, string>(); | ||
|
|
||
| for (const rel of trackedRs) { | ||
| const crate = crateFor(rel); | ||
| if (!crate) continue; | ||
| const code = stripNonCode(await file(path.join(root, rel)).text()); | ||
| stripped.set(rel, code); | ||
| let entry = byCrate.get(crate); | ||
| if (!entry) byCrate.set(crate, (entry = { files: [], shadowed: new Set() })); | ||
| entry.files.push(rel); | ||
| for (const m of code.matchAll(LOCAL_BINDING)) entry.shadowed.add(m[1] ?? m[2]); | ||
| } | ||
|
|
||
| const violations: string[] = []; | ||
| for (const [crate, { files, shadowed }] of byCrate) { | ||
| for (const rel of files) { | ||
| const seen = new Set<string>(); | ||
| for (const m of stripped.get(rel)!.matchAll(PATH_USE)) { | ||
| const used = m[1]; | ||
| if (seen.has(used)) continue; | ||
| seen.add(used); | ||
| if (used === crate.name) continue; | ||
| if (!workspaceNames.has(used)) continue; | ||
| if (crate.deps.has(used)) continue; | ||
| if (shadowed.has(used)) continue; | ||
| violations.push(`${rel}: uses \`${used}::\` but ${crate.dir}/Cargo.toml has no \`${used}\` dependency`); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| test("every bun_*:: reference has a Cargo.toml dependency edge", () => { | ||
| expect(violations).toEqual([]); | ||
| }); | ||
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.
🟡 This lint reads
src/*/Cargo.toml, but.github/workflows/source-lints.yml'spaths:filters (bothpushandpull_request) don't includesrc/**/Cargo.toml— so a dead-edge sweep PR that only edits a Cargo.toml (exactly the #37301 shape this lint targets) won't trigger the workflow at PR-check time. Pertest/internal/source-lints/README.md, lints that read files outside the listed paths need them added; add"src/**/Cargo.toml"to bothpaths:lists. The baremerge_group:trigger backstops main, so this only costs PR-time feedback.Extended reasoning...
What the bug is
test/internal/source-lints/README.md:19-22documents an explicit contract for adding lints to this directory:The new
cargo-dep-edges.test.tsreadssrc/*/Cargo.toml(line 33:new Bun.Glob("src/*/Cargo.toml").scanSync), but.github/workflows/source-lints.ymllines 14-33 (push) and 35-54 (pull_request) listsrc/**/*.rs,src/**/*.classes.ts,src/jsc/bindings/**, etc. — no Cargo.toml pattern. This PR's changed-files list also doesn't includesource-lints.yml, so the pattern isn't being added.Why it matters for this specific lint
The lint's own header comment (lines 5-14) states its purpose is to catch a race between two PR classes: one that adds a
bun_*::reference and one that prunes the corresponding dependency edge from aCargo.toml. The PR description says it should catch "whichever PR merges second."Consider the two orderings:
src/**/*.rs, which is in the paths filter → workflow runs on that PR ✓bun_css.workspace = truefromsrc/runtime/Cargo.toml. Such a PR touches onlysrc/*/Cargo.toml+ rootCargo.lock, neither of which matches any pattern in thepull_requestpaths filter → the source-lints workflow does not run on that PR's own CI, and the author gets no PR-time signal from this lint.So one of the two race directions the lint is designed to catch doesn't get PR-level feedback.
Why this is a nit, not blocking
Two backstops mean main won't actually break:
merge_group:(line 55) has nopaths:filter, so every PR entering the merge queue runs source-lints against the merged tree regardless of what files it touches. The Cargo.toml-only PR would fail there.rust-lints.ymldoes listsrc/**/Cargo.tomlin its own paths and runscargo clippyat merge-queue time, which would catch the missing edge as a compile error on the merged tree independently.So the only consequence of merging as-is is that the failure surfaces at merge-queue time instead of at PR-check time for Cargo.toml-only PRs. That's a degraded feedback loop, not a concrete failure — hence nit rather than blocking.
Step-by-step proof
bun_foo.workspace = truefromsrc/bar/Cargo.toml(a dead-edge sweep, verified against a stale tree).pull_requestpaths filter insource-lints.yml:35-54:src/bar/Cargo.tomlmatches none ofsrc/**/*.rs,src/**/*.classes.ts,src/codegen/class-definitions.ts,src/js/builtins.d.ts,src/jsc/bindings/**, or any other listed pattern. RootCargo.lockmatches nothing either.source-lintscheck does not appear on the PR. Per README line 6, these tests are also excluded from the Buildkite shards, so no other lane runscargo-dep-edges.test.tsagainst this PR either.merge_group:trigger run source-lints against the merged tree and fail — later than intended.Fix
Add
"src/**/Cargo.toml"to both thepush:paths list (after line 15) and thepull_request:paths list (after line 36) in.github/workflows/source-lints.yml.