bundler: spell elided Graph<'_> in signatures (mismatched_lifetime_syntaxes)#31033
bundler: spell elided Graph<'_> in signatures (mismatched_lifetime_syntaxes)#31033Jarred-Sumner wants to merge 1 commit into
Conversation
|
Updated 7:17 PM PT - May 19th, 2026
❌ @Jarred-Sumner, your commit 0a4d711 has 4 failures in
🧪 To try this PR locally: bunx bun-pr 31033That installs a local version of the PR into your bun-31033 --bun |
|
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 modernizes Rust port semantics and documentation across the codebase: parameterizing Graph lifetime annotations in bundler APIs, adding explicit RAII guards for resource cleanup, refactoring pending-item accounting logic, and systematically replacing Zig-style defer port-notes with accurate Rust ownership and control-flow patterns. ChangesRust Port Cleanup and RAII Modernization
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — comment-only cleanup + lint fix; the two orphaned comment fragments flagged inline are cosmetic and can be tidied before merge or in a follow-up.
Extended reasoning...
Overview
Two commits, both mechanical:
10f75afspells&Graph<'_>(or&Graph<'a>inLinkerContext::parse_graph) in 7 signatures where #30971 already wrote&LinkerGraph<'_>alongside a bare&Graph. Pure type-annotation change to silencemismatched_lifetime_syntaxes; semantically identical (or, in the'acase, a strict relaxation the compiler verifies).c7d4bf0deletes/rewords ~50// Zig: \defer ...`/// defer X → Drop` porting comments across 35 files. Net −144 lines. I walked the full diff: every hunk in this commit touches only comment lines or rewraps existing comment prose — no executable code is added, removed, or reordered.
Security risks
None. No logic, control flow, I/O, or data handling is changed. The lifetime annotations are compiler-checked no-ops.
Level of scrutiny
Low. Despite the 38-file footprint, this is a comment sweep plus a lint fix. The compiler is the ground truth for the Graph<'_> half, and the comment half cannot affect behavior.
Other factors
The bug-hunting pass found one nit: two spots (resolver/lib.rs:1245, runtime/jsc_hooks.rs:4001) where only the first line of a two-line comment was removed, leaving a lowercase sentence fragment. The same PR handles the identical pattern correctly at resolver/lib.rs:1485, so these are clearly oversights. Cosmetic only — flagged inline, not a blocker.
No prior reviews on the timeline; build #56039 is in flight.
There was a problem hiding this comment.
LGTM — thanks for addressing the earlier orphaned-fragment nits in 757317a; the two remaining ones flagged inline are cosmetic and fine to fold in or leave for a follow-up.
Extended reasoning...
Overview
Two-commit follow-up to #30971: (1) 10f75af adds <'_> to 7 &Graph parameters where the adjacent &LinkerGraph<'_> was already updated, silencing the mismatched_lifetime_syntaxes lint; (2) c7d4bf0 deletes/rewords ~50 // Zig: \defer ...`porting comments across 35 files (net −144 lines). A third commit757317acapitalizes two orphaned comment fragments I flagged in the previous run. Every hunk in the diff is either a comment edit or aGraph→Graph<'_>` signature tweak — no executable logic changes.
Security risks
None. Comment-only edits plus elided-lifetime syntax that the compiler already inferred; no control-flow, allocation, FFI, or auth surface touched.
Level of scrutiny
Low. The Graph<'_> changes are pure lint conformance (lifetime elision was already in effect). The comment sweep removes Zig-port scaffolding; the only failure mode is leaving prose fragments that read awkwardly, which is exactly what the two new nit reports cover (a dangling "those" at bundle_v2.rs:4898 and a now-inaccurate "Captured by reference" at js_bundle_completion_task.rs:277). Both are zero-runtime-impact wording issues.
Other factors
The author already responded to my prior round of the same defect class (757317a fixed resolver/lib.rs:1245 and jsc_hooks.rs:4001), and the prior inline thread is resolved. CodeRabbit found nothing actionable. Build #56236 is running on the latest commit. The remaining nits are filed inline and don't warrant blocking a comment-cleanup PR.
Jarred-Sumner
force-pushed
the
claude/bundler-perfile-followup
branch
from
3fe1741 to
18afdd8
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/js_printer/lib.rs (1)
8126-8147:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't unconditionally drop
line_offset_tableshere.
get_source_map_builder()bitwise-copies either a freshly-built table oropts.line_offset_tablesinto thisManuallyDrop. This guard always callsManuallyDrop::drop(tables), soprint_ast()will free caller-owned storage if it is ever invoked with a borrowed table. Please track whetheropts.line_offset_tableswasSome(_)before building theSourceMap::chunk::Builder, and only run this cleanup when the table was allocated on this path. Apply the same gate to the identical block inprint_common_js.Suggested shape
- let source_map_builder = get_source_map_builder::<ASCII_ONLY>( + let borrowed_line_offset_tables = opts.line_offset_tables.is_some(); + let source_map_builder = get_source_map_builder::<ASCII_ONLY>( GenerateSourceMap::lazy_if(GENERATE_SOURCE_MAP), &mut opts, source, tree, ); ... - if GENERATE_SOURCE_MAP { + if GENERATE_SOURCE_MAP && !borrowed_line_offset_tables { let tables = unsafe { &mut *p }; for v in tables.items_mut::<"columns_for_non_ascii", Box<[i32]>>() { core::mem::take(v); } unsafe { core::mem::ManuallyDrop::drop(tables) }; }🤖 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/js_printer/lib.rs` around lines 8126 - 8147, The guard unconditionally drops Builder.line_offset_tables even when it contains a borrowed table from opts.line_offset_tables; modify the logic so you record whether get_source_map_builder allocated a new line_offset_tables (e.g. a boolean like created_line_offset_tables) by checking opts.line_offset_tables before building, and only install/run the scopeguard/ManuallyDrop::drop branch when that flag is true; apply the same gated check to the identical block in print_common_js, referencing get_source_map_builder, Builder.line_offset_tables, print_ast, print_common_js, opts.line_offset_tables and the existing scopeguard/ManuallyDrop usage to locate and conditionally execute the cleanup.
🤖 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.
Outside diff comments:
In `@src/js_printer/lib.rs`:
- Around line 8126-8147: The guard unconditionally drops
Builder.line_offset_tables even when it contains a borrowed table from
opts.line_offset_tables; modify the logic so you record whether
get_source_map_builder allocated a new line_offset_tables (e.g. a boolean like
created_line_offset_tables) by checking opts.line_offset_tables before building,
and only install/run the scopeguard/ManuallyDrop::drop branch when that flag is
true; apply the same gated check to the identical block in print_common_js,
referencing get_source_map_builder, Builder.line_offset_tables, print_ast,
print_common_js, opts.line_offset_tables and the existing
scopeguard/ManuallyDrop usage to locate and conditionally execute the cleanup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6afbd4d7-1c87-41e4-8cc4-9914e8de81f3
📒 Files selected for processing (38)
src/ast/nodes.rssrc/bun_alloc/lib.rssrc/bun_core/util.rssrc/bundler/Chunk.rssrc/bundler/HTMLImportManifest.rssrc/bundler/LinkerContext.rssrc/bundler/ParseTask.rssrc/bundler/ServerComponentParseTask.rssrc/bundler/bundle_v2.rssrc/bundler/cache.rssrc/bundler/linker_context/MetafileBuilder.rssrc/bundler/linker_context/computeCrossChunkDependencies.rssrc/bundler/linker_context/doStep5.rssrc/bundler/linker_context/findAllImportedPartsInJSOrder.rssrc/bundler/linker_context/findImportedFilesInCSSOrder.rssrc/bundler/linker_context/generateCodeForLazyExport.rssrc/bundler/linker_context/postProcessJSChunk.rssrc/bundler/linker_context/scanImportsAndExports.rssrc/bundler/transpiler.rssrc/bundler/ungate_support.rssrc/css/css_parser.rssrc/js_parser/lexer.rssrc/js_parser/p.rssrc/js_parser/parse/parse_entry.rssrc/js_parser/parser.rssrc/js_printer/lib.rssrc/jsc/AsyncModule.rssrc/jsc/RuntimeTranspilerStore.rssrc/jsc/lib.rssrc/resolver/lib.rssrc/resolver/package_json.rssrc/resolver/resolver.rssrc/runtime/api/JSTranspiler.rssrc/runtime/api/js_bundle_completion_task.rssrc/runtime/bake/DevServer.rssrc/runtime/cli/create/SourceFileProjectGenerator.rssrc/runtime/jsc_hooks.rssrc/runtime/test_runner/snapshot.rs
💤 Files with no reviewable changes (10)
- src/bundler/linker_context/generateCodeForLazyExport.rs
- src/bundler/linker_context/findAllImportedPartsInJSOrder.rs
- src/bundler/linker_context/doStep5.rs
- src/jsc/lib.rs
- src/css/css_parser.rs
- src/runtime/cli/create/SourceFileProjectGenerator.rs
- src/bundler/ServerComponentParseTask.rs
- src/ast/nodes.rs
- src/bundler/linker_context/MetafileBuilder.rs
- src/bundler/linker_context/computeCrossChunkDependencies.rs
Silences mismatched_lifetime_syntaxes lint introduced when Graph became Graph<'a> in 303cd28 but a few sibling signatures kept bare `&Graph`.
Jarred-Sumner
force-pushed
the
claude/bundler-perfile-followup
branch
from
b141fba to
ea74518
Compare
|
Regenerated the defer-cleanup commit from scratch (force-pushed |
Jarred-Sumner
force-pushed
the
claude/bundler-perfile-followup
branch
from
876b09b to
0a4d711
Compare
Jarred-Sumner
changed the title
There was a problem hiding this comment.
LGTM — mechanical mismatched_lifetime_syntaxes fix now that the defer-comment cleanup has been dropped.
Extended reasoning...
Overview
The PR now touches exactly 4 files with 7 one-line signature edits: &Graph → &Graph<'_> in Chunk.rs (×3), HTMLImportManifest.rs (×2), ungate_support.rs (×1), and -> &Graph → -> &Graph<'a> on LinkerContext::parse_graph. These spell out the lifetime that was already being elided, to silence rustc's mismatched_lifetime_syntaxes lint after #30971 made Graph lifetime-generic. No bodies, comments, or call sites change; the adjacent &LinkerGraph<'_> parameters already used this style, and parse_graph_mut already returns &mut Graph<'a>, so this is pure consistency.
Security risks
None. Type-signature-only edits with no behavioral effect — the elided and explicit forms compile to identical borrow regions.
Level of scrutiny
Low. This is a mechanical lint fix equivalent to a typo correction. The previously-contentious portion of this PR (the Zig-defer comment sweep that generated six rounds of orphaned-fragment review) has been dropped entirely per the updated PR description, and the changed-files list confirms only the four bundler files remain. All my prior inline comments targeted files that are no longer in the diff and are now moot.
Other factors
No CODEOWNERS match for these paths. No bugs found by the bug-hunting system on the current revision. The robobun comment references a single 03830.test.ts asan failure, but that status is pinned to commit 876b09ba (the old state including the dropped cleanup + autofix churn); a pure <'_> spelling change cannot affect that regression test, so it's unrelated/flaky. cargo check -p bun_bin is reported clean.
2 participants
Spell the elided lifetime as
&Graph<'_>/-> &Graph<'a>in the four signatures where #30971 madeGraphlifetime-generic and updated the adjacent&LinkerGraph<'_>parameter but left&Graphbare — which triggers rustc'smismatched_lifetime_syntaxeslint (surfaced by robobun on the build job).Sites:
LinkerContext::parse_graph(→&Graph<'a>, matchingparse_graph_mut),Chunk.rs(×3),HTMLImportManifest.rs(×2),ungate_support.rs— all&Graph→&Graph<'_>.cargo check -p bun_binclean.