Support import with { type: "json" } and others - #16624
Merged
Merged
Conversation
Collaborator
|
Updated 2:41 PM PT - Mar 6th, 2025
✅ @pfgithub, your commit 70762635fb05956778705490806c72dde19bd3da passed in 🧪 try this PR locally: bunx bun-pr 16624 |
pfgithub
force-pushed
the
pfg/fix-with-type
branch
from
February 20, 2025 01:11
1e8a23c to
fd8456a
Compare
pfgithub
marked this pull request as ready for review
February 21, 2025 01:24
pfgithub
marked this pull request as draft
February 22, 2025 01:46
pfgithub
force-pushed
the
pfg/fix-with-type
branch
from
February 26, 2025 05:17
2deb98c to
9c3eaef
Compare
pfgithub
marked this pull request as ready for review
March 1, 2025 03:36
pfgithub
force-pushed
the
pfg/fix-with-type
branch
from
March 6, 2025 21:53
c52c0a5 to
7076263
Compare
| } | ||
| } | ||
|
|
||
| if (strings.eqlComptime(query, "?raw")) { |
Jarred-Sumner
pushed a commit
that referenced
this pull request
Sep 23, 2026
#43745) ### Problem - 16 struct fields in 11 crates are never read. Two options are never set: `BufferWriter.append_null_byte` is never `true`, `TransposeState.import_record_tag` is never `Some`. - No lint reports them. rustc's `dead_code` pass counts `x.f = v` as a use of `f`. hawk counts the initializer as a reference. ### Fix - Delete each field with its initializers and stores, and the two branches that the options guard. 18 files, 105 lines removed. - Candidates come from a compiler probe: `#[deprecated]` on 12,134 struct fields, `cargo check --force-warn deprecated` for Linux, Windows, and macOS, then a syn pass marks each use as read or write. 134 fields have no read. Most stay (Notes). - Verified: `bun bd`, `bun run rust:check-all` (12 of 12 targets), `cargo check --workspace --all-targets`, and the tests in the Notes. - Self-reviewed: 4 concerns raised, 2 addressed. Rejected as outside a deletion: the now vestigial `written_without_trailing_zero()` and an older stale comment. ### Background - `MultiArrayList<T>` keeps each field of `T` in its own column. Code reads a column by name as a string (`items::<"name", _>()`), so the compiler sees no read. Those fields stay. - `BufferWriter` is the JS printer's output buffer. `done()` could append a NUL byte. No caller asks for it. ### Downsides - `bun_core::output::Source` becomes `Send + Sync`, because its raw pointer fields are gone. It lives in a `thread_local!` only. - The `bun_resolver::Result` in `_resolve` now drops when `_resolve` returns. It has no `Drop` effect: paths are borrowed, fds are `Copy`. <details><summary>Notes</summary> #### Removed, one line each - `bun_core::output::Source`: `buffered_stream`, `buffered_error_stream`, `stream`, `error_stream` (`*mut io::Writer`). `init()` cached them, nothing read them. The accessors of the same names return `*_backing.new_interface()`, which is a pointer cast with no side effect. - `bun_bundler`: `InputFileInfo.import_count` (`MetafileBuilder.rs`). - `bun_install`: `SecurityScanSubprocess.stderr_data` (an empty `Vec` that is never filled, the scanner's stderr is inherited), `PackageManager.root_progress_node`. The `progress.start(b"", 0)` call stays, because it starts the progress root. - `bun_js_parser`: `PropertyOpts.async_range`, `ScanPassResult.approximate_newline_count`, `TransposeState.import_record_tag`. #16624 replaced the only setter of the tag with `import_loader`, which stays. - `bun_jsc`: `VirtualMachine.has_terminated` (its readers were debug panics in `enqueue_task_concurrent`, which #37075 removed), `ResolveFunctionResult.result` (seven stores, no read). Nothing borrows from the stored value: `path` and `query_string` point into the resolver's arena and the specifier. - `bun_js_printer`: `BufferWriter.append_null_byte`, with the seven `= false` stores in `bun_jsc` and `bun_runtime`. - `bun_libarchive`: `BufferReadStream.reading`. - `bun_runtime`: `WindowsState.is_server` (`ipc.rs`, Windows only). Its last reader went away in #18688, before the Rust port. - `bun_csrf`: `GenerateOptions.encoding`. `csrf_jsc.rs` encodes the token itself, and `VerifyOptions.encoding` stays. - `bun_resolver`: `DataURL.url` (always `String::EMPTY`). - `bun_s3_signing`: `S3CredentialsWithOptions.virtual_hosted_style`. No code names it. Every reader uses `credentials.virtual_hosted_style` on the inner `S3Credentials`. #### Passed the probe, kept on purpose - `MultiArrayList` columns: fields of `JSMeta`, `File`, `InputFile`, `BundledAst`, `Entry`, `Node`, `WatchItem`, `LineOffsetTable`, `ServerComponentBoundary`, and others. - Owners of memory that other fields borrow: `ParseResult.source_contents_backing`, `LinkerContext.unique_key_buf`, `OutputFile.owned_src_path_text`, `HTTPResponseMetadata.owned_buf`, `PackageJSON.source_contents` and `json_tape`, `MatchedRoute.pathname_backing`, `SignResult.content_md5`, `KEventWaker.machport_buf`. - Values whose drop has an effect: `Repl.last_error` (a GC protect), `SecurityScanSubprocess.process`, `Watcher.thread`. - Options that are unfinished features or open bugs, not dead code: `md::Options.hard_soft_breaks` and `underline` (#39495, #39493), `jsc::virtual_machine::Options.dns_result_order` (#40703), `P.has_top_level_return` (#40840), `ReactRefresh.last_hook_seen` and `force_reset`, `DebugOptions.dump_environment_variables` (the `--dump-environment-variables` flag is parsed and ignored). - Kept by a comment in the source: `AllocatorConfiguration.long_running`, `DumpStackTraceOptions.skip_*`, `NameOfSymbol.has_property_key_comment`. - Removal needs more than a deletion: `PostgresSQLQuery` `Flags.is_done` (set by the JS `done()` call), `WorkerPipe.done` (leaves two empty reader callbacks), `WTFTimer.repeat` (leaves an unused FFI parameter), `MaxHeapAllocator.len` (leaves an empty `reset()`), `ReadToEndResult.err` (callers drop read errors, which looks like a bug), `Subcommand::Pack` with `PACK_PARAMS` (`bun pm pack` runs as `Subcommand::Pm`, so the pack help text is unreachable). #### Self-review Three independent read-only passes tried to prove each deletion wrong (readers through raw pointers, `offset_of!`, C++ layout mirrors, macros, other `cfg`s, unit tests, the Zig originals, drop effects). None found a reader. Concerns raised: the `Send + Sync` change of `Source` (now in Downsides), the style of the kept `progress.start` call (now `let _ =`, as in `install_with_manager.rs`), `BufferWriter::written_without_trailing_zero()` (nine callers, it only strips NUL bytes that the printer never appends now, a follow-up), and a comment above `ResolveFunctionResult.path` that names a function which no longer exists (it predates this change). #### Other scans of this run, all clean or already in an open pull request - A relink of the debug binary with `--gc-sections --print-gc-sections`, and a zero-mention identifier index over `src/`, `packages/`, `scripts/` and `build/debug/codegen/`. - `macro_rules!` without an invocation, Cargo features that nothing enables, `#if 0` and never-true `#if` blocks in the C++ bindings, patches that no dependency script applies, unused exports under `scripts/`, `tsc --noUnusedLocals` over `src/js` and `scripts/`. - A syn scan for enum variants that no expression constructs. The hits are FFI code tables, derive-constructed variants, or already claimed. #### Overlap with the open dead-code pull requests - Every removed line was checked per file against the diffs of the 36 open dead-code pull requests. None of them deletes these lines. Several touch the same files in other places (`output.rs`, `VirtualMachine.rs`, `parser.rs`, `p.rs`, `js_printer/lib.rs`, `jsc_hooks.rs`, `PackageManager.rs`, `data_url.rs`). #### Tests run with the debug build `test/js/bun/util/csrf.test.ts` (31 pass), `test/bundler/metafile.test.ts` (65 pass), `test/js/bun/archive.test.ts` (108 pass), `test/bundler/transpiler/transpiler.test.js` (222 pass), `test/js/bun/resolve/import-empty.test.js`, `test/js/bun/resolve/esModule.test.ts`, `test/cli/install/bun-install-security-provider.test.ts` (43 pass), `test/internal/source-lints/` (195 pass). Also a `data:` URL import, and `bun install` under a pty so that the progress root starts. No test is added. The change deletes fields that nothing reads, so there is no behavior to assert, and `test/internal/source-lints/CLAUDE.md` asks for no tests that pin dead symbols. </details>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Fixes:
Breaking change: must now use
with {type: "jsonc"}or change the file path to.jsoncwhen usingbun build(bun.lock/tsconfig.json/... are unaffected)fixes #12175