Skip to content

Comments

fix: improve JSON parsing with serde_json to emit proper diagnostic#7442

Merged
shulaoda merged 1 commit intomainfrom
12-11-fix_use_serde_json_to_parse_json_file
Dec 11, 2025
Merged

fix: improve JSON parsing with serde_json to emit proper diagnostic#7442
shulaoda merged 1 commit intomainfrom
12-11-fix_use_serde_json_to_parse_json_file

Conversation

@IWANABETHATGUY
Copy link
Member

@IWANABETHATGUY IWANABETHATGUY commented Dec 11, 2025

closed #7403

Copy link
Member Author

IWANABETHATGUY commented Dec 11, 2025


How to use the Graphite Merge Queue

Add the label graphite: merge to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@IWANABETHATGUY IWANABETHATGUY marked this pull request as ready for review December 11, 2025 06:42
Copilot AI review requested due to automatic review settings December 11, 2025 06:42
@netlify
Copy link

netlify bot commented Dec 11, 2025

Deploy Preview for rolldown-rs canceled.

Name Link
🔨 Latest commit f50d7f9
🔍 Latest deploy log https://app.netlify.com/projects/rolldown-rs/deploys/693a8f139b4d8700088effd1

@IWANABETHATGUY IWANABETHATGUY changed the title fix: use serde_json to parse json file fix: improve JSON parsing with dedicated AST conversion Dec 11, 2025
@IWANABETHATGUY IWANABETHATGUY changed the title fix: improve JSON parsing with dedicated AST conversion fix: improve JSON parsing with serde_json to emit proper diagnostic Dec 11, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR switches from custom string-based JSON parsing to using serde_json for parsing JSON module files. This change fixes issue #7403 by providing more accurate JSON parsing with proper error handling. The key improvement is that serde_json properly validates JSON syntax (e.g., rejecting BigInt literals like 123n which are not valid JSON), whereas the previous implementation may have allowed invalid JSON through.

Key Changes

  • Replaced custom JSON parsing with serde_json parser for ModuleType::Json files
  • Added a new json_to_program module that converts serde_json::Value to oxc AST nodes
  • Updated error handling to correctly map serde_json error positions to byte offsets

Reviewed changes

Copilot reviewed 12 out of 13 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
crates/rolldown/src/utils/parse_to_ecma_ast.rs Switched to using serde_json::from_str for JSON parsing and calling json_value_to_ecma_ast to convert to AST
crates/rolldown_common/src/ecmascript/json_to_program.rs Added new module with functions to convert serde_json::Value to ECMAScript AST nodes
crates/rolldown_common/src/ecmascript/mod.rs Exported the new json_to_program module
crates/rolldown_common/src/lib.rs Re-exported the JSON conversion functions
crates/rolldown_ecmascript/src/ecma_ast/mod.rs Added from_allocator_and_source helper method for constructing EcmaAst with custom builder
crates/rolldown_error/src/build_diagnostic/constructors.rs Updated json_parse to accept 0-based line/column (conversion now happens at call site)
crates/rolldown_common/Cargo.toml Added insta as dev-dependency for snapshot testing
crates/rolldown/tests/esbuild/loader/loader_json_with_big_int/* Updated test to expect error for BigInt in JSON (correct behavior)
crates/rolldown/tests/rolldown/errors/json_parse_error/*/artifacts.snap Updated error message snapshots to reflect serde_json error messages
crates/rolldown/tests/snapshots/integration_rolldown__filename_with_hash.snap Removed bigint test from hash snapshot since it now errors
Cargo.lock Added insta dependency

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 11, 2025

Benchmarks Rust

  • target: main(3869162)
  • pr: 12-11-fix_use_serde_json_to_parse_json_file(f50d7f9)
group                                                        pr                                     target
-----                                                        --                                     ------
bundle/bundle@multi-duplicated-top-level-symbol              1.04     64.3±1.30ms        ? ?/sec    1.00     62.1±1.41ms        ? ?/sec
bundle/bundle@multi-duplicated-top-level-symbol-sourcemap    1.03     70.3±1.62ms        ? ?/sec    1.00     68.3±1.38ms        ? ?/sec
bundle/bundle@rome_ts                                        1.01    107.5±2.41ms        ? ?/sec    1.00    106.3±1.64ms        ? ?/sec
bundle/bundle@rome_ts-sourcemap                              1.01    120.1±1.73ms        ? ?/sec    1.00    118.9±1.67ms        ? ?/sec
bundle/bundle@threejs                                        1.02     40.2±2.52ms        ? ?/sec    1.00     39.6±2.35ms        ? ?/sec
bundle/bundle@threejs-sourcemap                              1.00     43.1±0.84ms        ? ?/sec    1.00     42.9±0.86ms        ? ?/sec
bundle/bundle@threejs10x                                     1.01    393.8±4.87ms        ? ?/sec    1.00    391.6±4.04ms        ? ?/sec
bundle/bundle@threejs10x-sourcemap                           1.00    454.1±3.93ms        ? ?/sec    1.00    452.7±5.65ms        ? ?/sec
scan/scan@rome_ts                                            1.00     84.4±1.44ms        ? ?/sec    1.01     85.1±1.70ms        ? ?/sec
scan/scan@threejs                                            1.00     28.2±0.37ms        ? ?/sec    1.03     28.9±1.90ms        ? ?/sec
scan/scan@threejs10x                                         1.01    297.5±4.37ms        ? ?/sec    1.00    294.4±4.04ms        ? ?/sec

@sapphi-red
Copy link
Member

Would you also remove this line?

'loader/loader_json_with_big_int':
'https://github.com/rolldown/rolldown/issues/7403',

Copilot AI review requested due to automatic review settings December 11, 2025 09:00
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 12 out of 13 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@IWANABETHATGUY IWANABETHATGUY force-pushed the 12-11-fix_use_serde_json_to_parse_json_file branch from 038774f to 9de1121 Compare December 11, 2025 09:16
Copilot AI review requested due to automatic review settings December 11, 2025 09:29
@IWANABETHATGUY IWANABETHATGUY force-pushed the 12-11-fix_use_serde_json_to_parse_json_file branch from 9de1121 to f50d7f9 Compare December 11, 2025 09:29
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 13 out of 14 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@shulaoda shulaoda merged commit b3257c7 into main Dec 11, 2025
49 of 52 checks passed
@shulaoda shulaoda deleted the 12-11-fix_use_serde_json_to_parse_json_file branch December 11, 2025 09:54
This was referenced Dec 11, 2025
shulaoda added a commit that referenced this pull request Dec 11, 2025
## [1.0.0-beta.54] - 2025-12-11

⏱️ Plugin Timing Analysis
- Added `checks.pluginTimings` option to emit warnings when plugins significantly impact build performance
- More details: https://rolldown.rs/options/checks#plugintimings
<img width="834" height="119" alt="image" src="https://github.com/user-attachments/assets/e819f906-6192-4503-8d5c-854fa92ecb45" />

> [!WARNING]
> For hooks using `ctx.resolve()` or `ctx.load()`, the reported time includes waiting for other plugins, which may overestimate that plugin's actual cost.
>
> Additionally, since plugin hooks execute concurrently, the statistics represent accumulated time rather than wall-clock time. The measured duration also includes Rust-side processing overhead, Tokio async scheduling overhead, NAPI data conversion overhead, and JavaScript event loop overhead.

### 🚀 Features

- rolldown_plugin_vite_reporter: add newline after build summary for better log separation (#7458) by @shulaoda
- plugin: collect plugin hook execution timings (#7364) by @shulaoda
- test-dev-server: reload page when detecting hmr reload message (#7422) by @hyf0
- rolldown_plugin_vite_dynamic_import_vars: add transform-based v2 implementation (#7400) by @shulaoda
- rolldown_plugin_vite_import_glob: add transform-based v2 implementation (#7394) by @shulaoda
- rolldown_plugin_vite_wasm_helper: add v2 implementation (#7402) by @shulaoda
- expose error location and context fields on JS API error objects (#7341) by @Copilot
- add `CIRCULAR_REEXPORT` error (#7337) by @Copilot
- support emit prebuilt chunk (#7277) by @Copilot

### 🐛 Bug Fixes

- preserve object key order when parse json with serde_json (#7443) by @IWANABETHATGUY
- improve JSON parsing with serde_json to emit proper diagnostic (#7442) by @IWANABETHATGUY
- deconflict external symbols in CJS modules (#7447) by @IWANABETHATGUY
- rolldown_plugin_vite_transform,rolldown_plugin_vite_resolve: enable `yarnPnp` option when pnp is detected (#5791) by @sapphi-red
- skip deconflicting top-level symbols for CJS modules (#7425) by @IWANABETHATGUY
- rolldown_plugin_esm_external_require: run resolveId hook before other plugins (#7426) by @shulaoda
- avoid duplicate underscores in legitimized identifiers (#7418) by @IWANABETHATGUY
- use `process.on('exit')` instead of `signal-exit` on webcontainers (#7421) by @sapphi-red
- dev: remove `imports` when an import is removed (#7348) by @sapphi-red
- bench: access latency.mean instead of mean in tinybench result (#7417) by @shulaoda
- path compression in symbol linking (#7392) by @IWANABETHATGUY
- rolldown_plugin_vite_resolve: add RwLock to avoid clearing cache while resolving (#7386) by @sapphi-red
- handle JSON prototype properties correctly (#7383) by @IWANABETHATGUY
- preserve entry signature strict chunk merging (#7343) by @IWANABETHATGUY
- support eliminating multiple unused dynamic imports in a single statement (#7361) by @IWANABETHATGUY
- eliminate unreachable dynamic entry (#7356) by @IWANABETHATGUY
- `NormalizedInputOptions#cwd` should always exists (#7360) by @hyf0
- cli: support multiple comma-separated define arguments (#7340) by @Copilot
- remove redundant symbol param in __reExport runtime helper (#7346) by @IWANABETHATGUY
- always use __export for empty namespace objects when symbols enabled (#7345) by @IWANABETHATGUY
- relax the restriction of preserveEntrySignatures when merging chunks (#7339) by @IWANABETHATGUY

### 🚜 Refactor

- builtin-plugin: make config parameter required for vite plugins (#7451) by @shulaoda
- move esbuild test related scripts to `scripts/src/esbuild-tests` (#7377) by @sapphi-red
- rewrite gen-esbuild-test in TypeScript (#7376) by @sapphi-red
- remove unnecessary Option in current_stmt_idx (#7359) by @IWANABETHATGUY
- move more code into chunk optimizer (#7335) by @IWANABETHATGUY

### 📚 Documentation

- checks: clarify pluginTimings measures CPU time due to concurrent execution (#7448) by @shulaoda
- checks: add accuracy note to pluginTimings documentation (#7441) by @shulaoda
- development-guide: add esbuild test description (#7439) by @sapphi-red
- checks: expand `pluginTimings` documentation with detection mechanism (#7428) by @shulaoda
- add checks options documentation (#7427) by @shulaoda
- development-guide: add test262 integration test description (#7430) by @sapphi-red
- guide: fix grammer in getting-started (#7331) by @jakeparis

### 🧪 Testing

- rollup-tests: make `--grep` work and document it (#7431) by @sapphi-red
- add a way to run some test262 test cases by name (#7429) by @sapphi-red
- rolldown_plugin_vite_dynamic_import_vars: add test cases for v2 implementation (#7401) by @shulaoda
- rolldown_plugin_vite_import_glob: add test cases for v2 implementation (#7395) by @shulaoda
- triage new esbuild tests (#7405) by @sapphi-red
- hmr: delete file used (#5933) by @sapphi-red
- watch: add `watchChange` hook `create` / `delete` tests (#7349) by @sapphi-red
- hmr: delete file not used anymore (#5932) by @sapphi-red
- triage esbuild failed reasons (#7391) by @sapphi-red
- change esbuild tests stats and diff generation (#7379) by @sapphi-red
- dev: add `continuous generate hmr patch` and `debounce bundle` tests (#7368) by @hyf0
- add new esbuild tests (#7353) by @sapphi-red

### ⚙️ Miscellaneous Tasks

- setup Node before updating snapshots for test262 update (#7435) by @sapphi-red
- run tests when submodules are updated (#7455) by @sapphi-red
- exclude NUL path pattern to prevent crash on Windows (#7450) by @IWANABETHATGUY
- run cargo-test for esbuild script changes and update esbuild snapshot (#7440) by @sapphi-red
- jsx_import_meta tests to `ignoreReasons` due to architectural limitation (#7434) by @Copilot
- deps: update dependency oxlint-tsgolint to v0.8.5 (#7433) by @renovate[bot]
- update test dependencies automatically (#7432) by @sapphi-red
- remove unused code (#7420) by @IWANABETHATGUY
- add back `just update-esbuild-diff` and add it to CI (#7404) by @sapphi-red
- deps: update oxc-resolver to 11.15.0 (#7415) by @shulaoda
- deps: update rust crate napi to v3.7.0 (#7393) by @renovate[bot]
- fix esbuild compatibility metrics calculation (#7390) by @sapphi-red
- support `tsconfig: true` in rust tests (#7389) by @sapphi-red
- hide oxc runtime in snapshots (#7388) by @sapphi-red
- deps: update oxc to v0.102.0 (#7385) by @camc314
- improve esbuild tests to download snapshots automatically (#7378) by @sapphi-red
- deps: update dependency tinybench to v6 (#7371) by @renovate[bot]
- deps: update actions/checkout action to v6 (#7370) by @renovate[bot]
- deps: update rust crates (#7367) by @renovate[bot]
- deps: update dependency oxlint to v1.32.0 (#7374) by @renovate[bot]
- deps: update dependency oxlint-tsgolint to v0.8.4 (#7373) by @renovate[bot]
- node/test-dev-server: support using custom html file (#7357) by @hyf0
- test: setup browser-based e2e test for `test-dev-server` (#7351) by @hyf0
- pin pnpm to version 10.23.0 (#7369) by @IWANABETHATGUY
- deps: update dependency rolldown-plugin-dts to v0.18.3 (#7372) by @renovate[bot]
- deps: update github-actions (#7365) by @renovate[bot]
- deps: update npm packages (#7366) by @renovate[bot]
- polish the comments about `SymbolRefFlags` (#7358) by @IWANABETHATGUY
- deps: update dependency rolldown-plugin-dts to v0.18.2 (#7355) by @renovate[bot]
- fix esbuild test generation script (#7352) by @sapphi-red
- apply cargo shear suggestion (#7347) by @IWANABETHATGUY
- deps: update oxc to v11.15.0 (#7344) by @renovate[bot]
- deps: update dependency @napi-rs/cli to v3.5.0 (#7338) by @renovate[bot]
- deps: update dependency oxlint to v1.31.0 (#7305) by @renovate[bot]
- update rollup-tests results (#7333) by @sapphi-red
- update rollup-tests Rollup version (#7332) by @sapphi-red
- add ignore config for test262 submodule (#7326) by @Copilot

### ❤️ New Contributors

* @jakeparis made their first contribution in [#7331](#7331)

Co-authored-by: shulaoda <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: BigInt literals are allowed in JSON files

3 participants