Add Bun.XML (parse/stringify) and an .xml loader - #37048
Conversation
Bun.XML.parse turns an XML 1.0 document into a plain object (or, with
{ compact: false }, an ordered { name, attributes, children } tree) and
Bun.XML.stringify writes either shape back out. `.xml` files can be
imported, required, bundled and used with `with { type: "xml" }`, like
.toml/.yaml/.json5.
- src/parsers/xml.rs: a non-validating processor that does not read
external entities. The scanner owns the bytes and hands the
recursive-descent parser one token per grammar position; internal
entities are expanded as scanner input frames with expat-style
amplification/depth limits; attribute values are normalized and
internal-subset ATTLIST defaults applied; BOM / UTF-16 / ISO-8859-1
input is decoded per the spec.
- test/js/bun/xml/xml-test-suite.test.ts is generated from the W3C XML
Conformance Test Suite (xmlts20130923) by generate_xml_test_suite.ts:
927 must-reject, 752 must-accept (262 compared against canonical
output), 316 processor-class-dependent cases pinned.
- Loader::Xml is threaded through the bundler, printer, module loader,
plugin APIs and types. src/api/schema.js also gains the json5/md/xml
entries it was missing, so Bun.build plugins see and can return those
loaders, and schema.js is now a declared input of the JS-modules
codegen step.
- Docs, bun-types, and a bench against fast-xml-parser/xml2js.
WalkthroughChangesXML support now includes a native XML 1.0 parser, XML support
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🤖 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 `@bench/xml/xml.mjs`:
- Around line 85-90: Update the XMLBuilder construction near the parsed object
setup to pass attributeNamePrefix as "@" for Bun and "`@_`" for fast-xml-parser,
using the existing isBun condition. Keep the builder aligned with the parser
that produced object so attributes such as Bun's `@xmlns` are serialized
correctly.
In `@docs/runtime/xml.mdx`:
- Line 20: Update the XML parsing documentation near the external DTD and entity
behavior to remove the claim that undeclared external-DTD entities are preserved
literally. State that external resources are not loaded or read, while any
entity not declared in the internal subset causes Bun.XML.parse to throw
SyntaxError.
- Line 134: Update the documentation sentence describing skipped values and the
JSON.stringify comparison: keep undefined, functions, and symbols as the values
skipped like JSON.stringify, while explicitly state that bigint values are
converted to text by Bun.XML.stringify even though JSON.stringify throws for
them.
In `@src/bundler/transpiler.rs`:
- Around line 1875-1884: Update the XML loader branch in
parse_maybe_return_file_only_allow_shared_buffer to construct
bun_parsers::xml::Options with InputEncoding::Bytes instead of
InputEncoding::File, preserving compact mode and the existing parse error
handling for raw virtual sources and decoded data URL bytes.
In `@src/parsers/xml.rs`:
- Around line 808-833: Add a short comment in transcode_utf16 adjacent to the
simdutf UTF-16 length and conversion calls documenting that the units are
host-order values and the ::le APIs rely on little-endian targets. Do not change
the conversion behavior.
In `@src/runtime/api/XMLObject.rs`:
- Around line 329-382: The XML serializer currently reads live containers twice,
allowing getters, proxies, or coercion hooks to change layout decisions between
passes. In src/runtime/api/XMLObject.rs lines 329-382, update the children
serialization path to snapshot resolved items once, derive count and has_text
from that snapshot, and emit from it; ensure self.indent is restored on errors
between lines 353-378. In src/runtime/api/XMLObject.rs lines 556-619, snapshot
each property key and resolved value once, then derive has_elements, has_text,
attributes, and child output from the snapshot without a second
JSPropertyIterator.
- Around line 782-792: Move the maximum-length clamp for the indent string from
the Space::Str branch in newline() into Space::init, storing an OwnedString
limited to 10 code units. Update newline() to append the already-clamped stored
value directly and remove the per-line length check, substring call, and
temporary clamped value.
In `@src/runtime/jsc_hooks.rs`:
- Around line 2712-2716: Update the XML branch in the loader export flow around
expr_to_js so materialization failures propagate by converting the ToJSError
variant into the corresponding crate::Error and returning it. Remove the
bun_core::Output::panic path for XML failures while preserving the existing
behavior for successful materialization and other loaders.
In `@test/js/bun/resolve/xml/xml-latin1.xml`:
- Line 1: Configure .gitattributes to mark
test/js/bun/resolve/xml/xml-latin1.xml and
test/js/bun/resolve/xml/xml-utf8-bom.xml as binary or -text so their raw Latin-1
and UTF-8 BOM bytes are preserved; also include xml-utf16le-bom.xml and
xml-utf16be-bom.xml if present in this change. No direct fixture content changes
are needed.
In `@test/js/bun/transpiler/transpiler-unsupported-loader.test.ts`:
- Around line 49-53: Update the "xml" test to also verify that the transformed
output preserves the child element and its text content, such as asserting the
generated representation of <c>x</c> alongside the existing attribute assertion;
alternatively, snapshot the complete output using normalizeBunSnapshot.
In `@test/js/bun/xml/generate_xml_test_suite.ts`:
- Around line 633-641: Update the check-mode flow around readFileSync, rmSync,
and the fresh/committed comparison so outPath is not removed when the files
differ, allowing developers to inspect the regenerated output; remove it only
after a successful match while preserving the existing mismatch exit and success
logging.
- Around line 149-152: Scope the filter in the test-case generation logic to
skip only entries from the japanese collection whose ID starts with pr-xml-.
Update the condition around skippedBig so other collections with matching IDs
remain included, while preserving the existing skip count and generated
reporting.
In `@test/js/bun/xml/xml-test-suite.test.ts`:
- Around line 58-94: Add a concise comment near the generator template’s
writeCanonical logic documenting that it and the emitted canonicalize
implementation are intentionally independent and must remain behaviorally
aligned. Keep the comment focused on preserving the canonicalization
cross-check, without changing either implementation.
In `@test/js/bun/xml/xml.test.ts`:
- Around line 797-805: Reduce the depth used by the “deep values are a catchable
error” test to a substantially smaller value that still triggers RangeError on
release builds, and release the first deep object before constructing the second
chain so both are not retained simultaneously. Keep both XML.stringify
assertions validating RangeError.
- Line 719: Update the XML.stringify assertion in the surrounding test to
compare against the exact documented serialized output instead of only asserting
that no exception is thrown. Preserve the case covering an element-content
object with a toString method, and use the expected output for that behavior.
🪄 Autofix
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: 02c2b768-4fde-4756-8f94-ab48c8b71457
⛔ Files ignored due to path filters (1)
bench/xml/bun.lockis excluded by!**/*.lock
📒 Files selected for processing (54)
bench/xml/package.jsonbench/xml/xml.mjsdocs/bundler/loaders.mdxdocs/docs.jsondocs/guides/runtime/import-xml.mdxdocs/runtime/bun-apis.mdxdocs/runtime/file-types.mdxdocs/runtime/xml.mdxpackages/bun-native-bundler-plugin-api/bundler_plugin.hpackages/bun-types/bun.d.tspackages/bun-types/extensions.d.tsscripts/build/codegen.tssrc/analytics/lib.rssrc/api/schema.d.tssrc/api/schema.jssrc/ast/loader.rssrc/bun_core/Global.rssrc/bundler/LinkerContext.rssrc/bundler/ParseTask.rssrc/bundler/options.rssrc/bundler/transpiler.rssrc/bundler_jsc/options_jsc.rssrc/js_printer/lib.rssrc/jsc/bindings/BunObject+exports.hsrc/jsc/bindings/BunObject.cppsrc/jsc/bindings/ModuleLoader.cppsrc/jsc/bindings/headers-handwritten.hsrc/options_types/bundle_enums.rssrc/options_types/schema.rssrc/parsers/lib.rssrc/parsers/xml.rssrc/runtime/api.rssrc/runtime/api/BunObject.rssrc/runtime/api/XMLObject.rssrc/runtime/cli/test/parallel/runner.rssrc/runtime/jsc_hooks.rstest/bundler/bundler_loader.test.tstest/bundler/bundler_plugin.test.tstest/integration/bun-types/fixture/xml.tstest/js/bun/resolve/fixtures/require/obj.xmltest/js/bun/resolve/require.test.tstest/js/bun/resolve/xml/xml-empty.xmltest/js/bun/resolve/xml/xml-fixture.xmltest/js/bun/resolve/xml/xml-fixture.xml.txttest/js/bun/resolve/xml/xml-latin1.xmltest/js/bun/resolve/xml/xml-malformed.xmltest/js/bun/resolve/xml/xml-utf16be-bom.xmltest/js/bun/resolve/xml/xml-utf16le-bom.xmltest/js/bun/resolve/xml/xml-utf8-bom.xmltest/js/bun/resolve/xml/xml.test.jstest/js/bun/transpiler/transpiler-unsupported-loader.test.tstest/js/bun/xml/generate_xml_test_suite.tstest/js/bun/xml/xml-test-suite.test.tstest/js/bun/xml/xml.test.ts
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: 15
🤖 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 `@bench/xml/xml.mjs`:
- Around line 85-90: Update the XMLBuilder construction near the parsed object
setup to pass attributeNamePrefix as "@" for Bun and "`@_`" for fast-xml-parser,
using the existing isBun condition. Keep the builder aligned with the parser
that produced object so attributes such as Bun's `@xmlns` are serialized
correctly.
In `@docs/runtime/xml.mdx`:
- Line 20: Update the XML parsing documentation near the external DTD and entity
behavior to remove the claim that undeclared external-DTD entities are preserved
literally. State that external resources are not loaded or read, while any
entity not declared in the internal subset causes Bun.XML.parse to throw
SyntaxError.
- Line 134: Update the documentation sentence describing skipped values and the
JSON.stringify comparison: keep undefined, functions, and symbols as the values
skipped like JSON.stringify, while explicitly state that bigint values are
converted to text by Bun.XML.stringify even though JSON.stringify throws for
them.
In `@src/bundler/transpiler.rs`:
- Around line 1875-1884: Update the XML loader branch in
parse_maybe_return_file_only_allow_shared_buffer to construct
bun_parsers::xml::Options with InputEncoding::Bytes instead of
InputEncoding::File, preserving compact mode and the existing parse error
handling for raw virtual sources and decoded data URL bytes.
In `@src/parsers/xml.rs`:
- Around line 808-833: Add a short comment in transcode_utf16 adjacent to the
simdutf UTF-16 length and conversion calls documenting that the units are
host-order values and the ::le APIs rely on little-endian targets. Do not change
the conversion behavior.
In `@src/runtime/api/XMLObject.rs`:
- Around line 329-382: The XML serializer currently reads live containers twice,
allowing getters, proxies, or coercion hooks to change layout decisions between
passes. In src/runtime/api/XMLObject.rs lines 329-382, update the children
serialization path to snapshot resolved items once, derive count and has_text
from that snapshot, and emit from it; ensure self.indent is restored on errors
between lines 353-378. In src/runtime/api/XMLObject.rs lines 556-619, snapshot
each property key and resolved value once, then derive has_elements, has_text,
attributes, and child output from the snapshot without a second
JSPropertyIterator.
- Around line 782-792: Move the maximum-length clamp for the indent string from
the Space::Str branch in newline() into Space::init, storing an OwnedString
limited to 10 code units. Update newline() to append the already-clamped stored
value directly and remove the per-line length check, substring call, and
temporary clamped value.
In `@src/runtime/jsc_hooks.rs`:
- Around line 2712-2716: Update the XML branch in the loader export flow around
expr_to_js so materialization failures propagate by converting the ToJSError
variant into the corresponding crate::Error and returning it. Remove the
bun_core::Output::panic path for XML failures while preserving the existing
behavior for successful materialization and other loaders.
In `@test/js/bun/resolve/xml/xml-latin1.xml`:
- Line 1: Configure .gitattributes to mark
test/js/bun/resolve/xml/xml-latin1.xml and
test/js/bun/resolve/xml/xml-utf8-bom.xml as binary or -text so their raw Latin-1
and UTF-8 BOM bytes are preserved; also include xml-utf16le-bom.xml and
xml-utf16be-bom.xml if present in this change. No direct fixture content changes
are needed.
In `@test/js/bun/transpiler/transpiler-unsupported-loader.test.ts`:
- Around line 49-53: Update the "xml" test to also verify that the transformed
output preserves the child element and its text content, such as asserting the
generated representation of <c>x</c> alongside the existing attribute assertion;
alternatively, snapshot the complete output using normalizeBunSnapshot.
In `@test/js/bun/xml/generate_xml_test_suite.ts`:
- Around line 633-641: Update the check-mode flow around readFileSync, rmSync,
and the fresh/committed comparison so outPath is not removed when the files
differ, allowing developers to inspect the regenerated output; remove it only
after a successful match while preserving the existing mismatch exit and success
logging.
- Around line 149-152: Scope the filter in the test-case generation logic to
skip only entries from the japanese collection whose ID starts with pr-xml-.
Update the condition around skippedBig so other collections with matching IDs
remain included, while preserving the existing skip count and generated
reporting.
In `@test/js/bun/xml/xml-test-suite.test.ts`:
- Around line 58-94: Add a concise comment near the generator template’s
writeCanonical logic documenting that it and the emitted canonicalize
implementation are intentionally independent and must remain behaviorally
aligned. Keep the comment focused on preserving the canonicalization
cross-check, without changing either implementation.
In `@test/js/bun/xml/xml.test.ts`:
- Around line 797-805: Reduce the depth used by the “deep values are a catchable
error” test to a substantially smaller value that still triggers RangeError on
release builds, and release the first deep object before constructing the second
chain so both are not retained simultaneously. Keep both XML.stringify
assertions validating RangeError.
- Line 719: Update the XML.stringify assertion in the surrounding test to
compare against the exact documented serialized output instead of only asserting
that no exception is thrown. Preserve the case covering an element-content
object with a toString method, and use the expected output for that behavior.
🪄 Autofix
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: 02c2b768-4fde-4756-8f94-ab48c8b71457
⛔ Files ignored due to path filters (1)
bench/xml/bun.lockis excluded by!**/*.lock
📒 Files selected for processing (54)
bench/xml/package.jsonbench/xml/xml.mjsdocs/bundler/loaders.mdxdocs/docs.jsondocs/guides/runtime/import-xml.mdxdocs/runtime/bun-apis.mdxdocs/runtime/file-types.mdxdocs/runtime/xml.mdxpackages/bun-native-bundler-plugin-api/bundler_plugin.hpackages/bun-types/bun.d.tspackages/bun-types/extensions.d.tsscripts/build/codegen.tssrc/analytics/lib.rssrc/api/schema.d.tssrc/api/schema.jssrc/ast/loader.rssrc/bun_core/Global.rssrc/bundler/LinkerContext.rssrc/bundler/ParseTask.rssrc/bundler/options.rssrc/bundler/transpiler.rssrc/bundler_jsc/options_jsc.rssrc/js_printer/lib.rssrc/jsc/bindings/BunObject+exports.hsrc/jsc/bindings/BunObject.cppsrc/jsc/bindings/ModuleLoader.cppsrc/jsc/bindings/headers-handwritten.hsrc/options_types/bundle_enums.rssrc/options_types/schema.rssrc/parsers/lib.rssrc/parsers/xml.rssrc/runtime/api.rssrc/runtime/api/BunObject.rssrc/runtime/api/XMLObject.rssrc/runtime/cli/test/parallel/runner.rssrc/runtime/jsc_hooks.rstest/bundler/bundler_loader.test.tstest/bundler/bundler_plugin.test.tstest/integration/bun-types/fixture/xml.tstest/js/bun/resolve/fixtures/require/obj.xmltest/js/bun/resolve/require.test.tstest/js/bun/resolve/xml/xml-empty.xmltest/js/bun/resolve/xml/xml-fixture.xmltest/js/bun/resolve/xml/xml-fixture.xml.txttest/js/bun/resolve/xml/xml-latin1.xmltest/js/bun/resolve/xml/xml-malformed.xmltest/js/bun/resolve/xml/xml-utf16be-bom.xmltest/js/bun/resolve/xml/xml-utf16le-bom.xmltest/js/bun/resolve/xml/xml-utf8-bom.xmltest/js/bun/resolve/xml/xml.test.jstest/js/bun/transpiler/transpiler-unsupported-loader.test.tstest/js/bun/xml/generate_xml_test_suite.tstest/js/bun/xml/xml-test-suite.test.tstest/js/bun/xml/xml.test.ts
🛑 Comments failed to post (2)
src/parsers/xml.rs (1)
808-833: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Description: List the simdutf utf16->utf8 conversion and length entry points exposed by bun_simdutf_sys. set -euo pipefail fd -t f -e rs . src/simdutf 2>/dev/null | head -20 rg -nP --type=rust -C 3 'utf16.*to.*utf8|from::utf16' -g '!target' | head -80Repository: oven-sh/bun
Length of output: 149
🏁 Script executed:
set -euo pipefail printf '%s\n' '--- candidate files ---' fd -t f -e rs -e toml -e lock . | rg '(^|/)(xml\.rs|simdutf|Cargo\.(toml|lock))$|simdutf' | head -100 printf '%s\n' '--- transcode implementation and call sites ---' rg -n -C 8 'fn transcode_utf16|simdutf::(length|convert)|transcode_utf16' src/parsers/xml.rs printf '%s\n' '--- simdutf references and dependency sources ---' rg -n -C 3 'simdutf|utf16.*utf8|utf8.*utf16' . -g '!target' -g '!node_modules' | head -200Repository: oven-sh/bun
Length of output: 17029
🏁 Script executed:
set -euo pipefail printf '%s\n' '--- simdutf_sys API definitions ---' rg -n -C 6 'utf16|utf8|with_errors|length' src/simdutf_sys/lib.rs src/simdutf_sys/simdutf.rs | head -240 printf '%s\n' '--- wrapper and native implementation references ---' rg -n -C 5 'utf16_to_utf8|convert_utf16|utf16.*to.*utf8|from_utf16' src packages -g '*.rs' -g '*.h' -g '*.hpp' -g '*.cpp' -g '*.cc' | head -300 printf '%s\n' '--- simdutf_sys layout ---' printf '%s\n' 'src/simdutf_sys/lib.rs' "$(wc -l < src/simdutf_sys/lib.rs)" printf '%s\n' 'src/simdutf_sys/simdutf.rs' "$(wc -l < src/simdutf_sys/simdutf.rs)" sed -n '1,220p' src/simdutf_sys/lib.rsRepository: oven-sh/bun
Length of output: 38270
Document the little-endian assumption in
transcode_utf16.
unitscontains host-orderu16values, so the::lecalls are correct only on little-endian targets. Add a short comment that records this assumption.🤖 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/parsers/xml.rs` around lines 808 - 833, Add a short comment in transcode_utf16 adjacent to the simdutf UTF-16 length and conversion calls documenting that the units are host-order values and the ::le APIs rely on little-endian targets. Do not change the conversion behavior.test/js/bun/xml/xml-test-suite.test.ts (1)
58-94: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Description: Check whether generate_xml_test_suite.ts defines its own copy of # canonicalize/codePointCompare/CANON_ESCAPES, or imports them from a shared module. fd generate_xml_test_suite.ts test/js/bun/xml rg -n -A5 'CANON_ESCAPES|codePointCompare|function canonicalize' test/js/bun/xml/generate_xml_test_suite.tsRepository: oven-sh/bun
Length of output: 2047
🏁 Script executed:
#!/bin/bash printf '%s\n' '--- test file imports and header ---' sed -n '1,115p' test/js/bun/xml/xml-test-suite.test.ts printf '%s\n' '--- generator canonicalization helpers ---' sed -n '220,290p' test/js/bun/xml/generate_xml_test_suite.ts printf '%s\n' '--- generator embedded test generation ---' sed -n '520,610p' test/js/bun/xml/generate_xml_test_suite.ts printf '%s\n' '--- references and generation instructions ---' rg -n -C3 'xml-test-suite|generate_xml_test_suite|canonicalize|writeCanonical|CANON_ESCAPES' test/js/bun/xml README.md REVIEW.md 2>/dev/nullRepository: oven-sh/bun
Length of output: 24885
🏁 Script executed:
#!/bin/bash sed -n '600,665p' test/js/bun/xml/generate_xml_test_suite.ts printf '%s\n' '--- generator output assembly ---' sed -n '400,530p' test/js/bun/xml/generate_xml_test_suite.tsRepository: oven-sh/bun
Length of output: 7164
Document the intentional canonicalization cross-check in the generator template.
writeCanonicaland the emittedcanonicalizeare separate implementations. Add a comment stating that they must remain aligned and independent.🤖 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 `@test/js/bun/xml/xml-test-suite.test.ts` around lines 58 - 94, Add a concise comment near the generator template’s writeCanonical logic documenting that it and the emitted canonicalize implementation are intentionally independent and must remain behaviorally aligned. Keep the comment focused on preserving the canonicalization cross-check, without changing either implementation.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
- Importing an .xml file nested too deeply to parse now fails with a "Nesting is too deep" build error instead of "Cannot find module" (the stack-overflow path never wrote to the log). - Keep the is_ascii flag exact when an unexpanded entity reference with a non-ASCII name is copied into text or an attribute value. - headers-handwritten.h: BunLoaderTypeMD is 21 (api::Loader::md), not 20 (json5); a runtime plugin returning loader "md" was decoded as json5. - Mark the encoded loader fixtures -text in .gitattributes so their bytes survive any checkout normalization. - bench: give fast-xml-parser's builder its own parser's object. - docs: spell out when an undeclared entity is kept vs. rejected, and that bigints are written as digits. - generator: scope the pr-xml-* skip to the japanese collection; keep the --check output around on mismatch. - tests: exact assertions for the toString-object case and the transformSync(xml) child element; free the first deep chain before building the second.
…to claude/bun-xml-parser-1e9a93
No-Verification-Needed: import ordering only (prettier organize-imports)
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 (2)
packages/bun-types/bun.d.ts (1)
824-955: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winCorrect the XML entity-reference documentation.
XML.parsedoes not expand every entity reference: external and certain undeclared references remain as literal&name;text. UpdateNode.childrenand@throws; includeDataViewin the byte-input list because the overload accepts it.🤖 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 `@packages/bun-types/bun.d.ts` around lines 824 - 955, Update the XML.parse documentation and Node.children description to state that only supported internal entities are expanded, while external and certain undeclared references remain literal “&name;” text; revise `@throws` accordingly so it does not claim every undeclared reference throws. Also add DataView to the documented byte-input types, matching the parse overloads.src/bundler/transpiler.rs (1)
2787-2814: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winPreserve the underlying generation error.
Lines 2801-2810 discard
errfor every non-stack-overflow failure. The user only seesFailed to generate code for "<path>"and cannot identify the cause.Route
errthrough the existing diagnostic formatter, or map each expected error variant to a diagnostic that includes its cause. As per coding guidelines, error messages must identify the cause and preserve rich underlying errors.🤖 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/bundler/transpiler.rs` around lines 2787 - 2814, Update the Err branch of build_with_resolve_result_eager in the surrounding transpilation flow to preserve and report the underlying err for all non-StackOverflow failures. Route err through the existing diagnostic formatter, while retaining the special stack-overflow message and current duplicate-error check; ensure the emitted diagnostic includes both the generation context and the original cause.Source: Coding guidelines
🤖 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 `@packages/bun-types/bun.d.ts`:
- Around line 824-955: Update the XML.parse documentation and Node.children
description to state that only supported internal entities are expanded, while
external and certain undeclared references remain literal “&name;” text; revise
`@throws` accordingly so it does not claim every undeclared reference throws. Also
add DataView to the documented byte-input types, matching the parse overloads.
In `@src/bundler/transpiler.rs`:
- Around line 2787-2814: Update the Err branch of
build_with_resolve_result_eager in the surrounding transpilation flow to
preserve and report the underlying err for all non-StackOverflow failures. Route
err through the existing diagnostic formatter, while retaining the special
stack-overflow message and current duplicate-error check; ensure the emitted
diagnostic includes both the generation context and the original cause.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6faf1093-fa97-4d0f-a4c1-9370702176f5
📒 Files selected for processing (12)
.gitattributesbench/xml/xml.mjsdocs/runtime/xml.mdxpackages/bun-types/bun.d.tssrc/bundler/transpiler.rssrc/jsc/bindings/headers-handwritten.hsrc/parsers/xml.rssrc/runtime/jsc_hooks.rstest/js/bun/resolve/xml/xml.test.jstest/js/bun/transpiler/transpiler-unsupported-loader.test.tstest/js/bun/xml/generate_xml_test_suite.tstest/js/bun/xml/xml.test.ts
There was a problem hiding this comment.
No new issues found after 042d317 addressed the earlier findings. This adds a new public API (Bun.XML), a ~3k-line XML 1.0 parser, and changes the default .xml loader behavior, so it warrants a human sign-off on the API shape and the loader-default change.
What was reviewed:
- Parser correctness against the W3C conformance suite; entity-expansion limits (billion-laughs, depth, stack overflow) surface as catchable errors, not panics.
- Encoding detection paths (
InputEncoding::Bytes/File/Text) and theis_asciibookkeeping across allpush_referencesites. - Loader-id tables kept in sync across
schema.rs/headers-handwritten.h/schema.js/bundler_plugin.h/jsc_hooks.rs(the pre-existingBunLoaderTypeMDoff-by-one is now fixed). XML.stringifyre-entrancy: property iteration runs user getters while holding no raw pointers; circular/depth/invalid-name/invalid-char paths all throw.
Extended reasoning...
Overview
This PR adds Bun.XML.parse / Bun.XML.stringify and an .xml loader, following the Bun.TOML/Bun.YAML/Bun.JSON5 pattern. The core is src/parsers/xml.rs (~3,000 lines: an XML 1.0 5th-edition non-validating scanner/parser with internal-entity expansion, attribute normalization, and UTF-8/UTF-16/ISO-8859-1 decoding) and src/runtime/api/XMLObject.rs (~800 lines: the JS-facing parse/stringify host functions). Around that are loader-registry additions across ~15 files, docs, .d.ts types, a benchmark, and ~18k lines of tests (mostly generated from the W3C conformance suite, plus hand-written parse/stringify/loader tests).
Security risks
XML parsers are a classic attack surface. This one is designed defensively: external entities/DTDs are never fetched (no XXE); internal-entity expansion is bounded by expat-style amplification and depth limits (billion-laughs throws); nesting depth is guarded by StackCheck and surfaces as RangeError/build error rather than a crash; all size arithmetic uses u64/saturating ops; input is UTF-8-validated up front via simdutf. stringify refuses non-Char code points and non-Name element/attribute names, so output is always well-formed. I did not find a reachable path to memory unsafety or DoS beyond the documented limits.
Level of scrutiny
High. This is a new user-facing API with a documented compact-object shape and a behavior change (.xml files now default to the xml loader instead of file, so existing import x from './a.xml' returns a parsed object rather than a path). Those are product decisions a maintainer should sign off on. The parser itself is thoroughly tested (1,679 W3C cases with required outcomes all pass, plus round-trip and adversarial tests), and the loader plumbing follows the exact pattern of the YAML/JSON5 additions, but the sheer volume of new native code parsing untrusted input is beyond what an automated pass should approve alone.
Other factors
My two prior inline findings (the is_ascii flag on unexpanded non-ASCII entity names, and the pre-existing BunLoaderTypeMD = 20 mismatch) were both addressed in 042d317, along with all CodeRabbit comments. The author's responses show a solid grasp of the XML spec edge cases (WFC: Entity Declared conditionality, InputEncoding::File vs Bytes for the fs-cache path). Test coverage is unusually strong for a new-feature PR. This run's bug-hunting pass found nothing further.
… helpers
The scanner had grown a scan_*_item() function per grammar position,
most of them opening with skip_ws()/skip_decl_ws()/require_decl_ws(),
and a good part of the DTD grammar (ATTLIST/ENTITY keyword sequences,
external IDs, enumerations) lived in the scanner as byte-level code.
Restructure it the way yaml.rs is built:
- Scanner::next(literal) is a single byte loop for everything outside
element content: whitespace is a match arm that advances and records
`spaced` on the token, and every other byte identifies the token to
scan (names, `#keyword`, `%ref;`, quoted literals, `<!…`/`<?…`
openers, single-byte punctuation). Element content keeps its own
loop (next_content). The quoted literal is the one context-sensitive
lexeme, so the parser says which literal production applies.
- The parser is recursive descent over tokens with one current token
and never reads bytes or scanner state: XML's required/forbidden `S`
rules are require_spaced()/`!tok.spaced` checks, and the DTD grammar
is ordinary productions (parse_external_id, parse_group/particle/
occurrence, parse_enumeration, parse_pseudo_attribute, ...).
- Tokenized-attribute space collapsing moves into scan_att_value;
parameter-entity text is no longer copied to pad it.
Diagnostics name whole tokens now ("found 'NUTOKEN'" rather than
"found 'N'"), so the generated conformance suite's pinned messages are
regenerated; accept/reject verdicts are unchanged for all 1995 cases.
Also fixes a few diagnostics found on the way: invalid UTF-8 ahead of
or inside the XML declaration is reported as such, a `<?xml` after a
Latin-1 declaration no longer poses as the declaration, `%name` typos
and `]` inside parameter-entity text get accurate messages, and
character references are echoed as written.
…wording No-Verification-Needed: JSDoc-only change to a .d.ts file
There was a problem hiding this comment.
No bugs found in this pass, but this adds a new public API (Bun.XML), a ~3,100-line hand-written XML 1.0 parser handling adversarial input (entity expansion, encoding detection, DTD internal subset), an ~800-line JS-facing stringifier, and changes the default .xml loader behavior — worth a human look at the API shape and parser.
What was reviewed:
- Entity-expansion limits (amplification/depth) and that external entities are never read — no XXE surface found.
- Loader enum wiring across
schema.rs/headers-handwritten.h/schema.js/bundle_enums.rs— the pre-existingBunLoaderTypeMDoff-by-one was fixed here;XML = 22lines up everywhere. - The earlier
is_ascii/push_referencefinding and the "Nesting is too deep" loader diagnostic were both addressed in 042d317; all review threads are resolved.
Extended reasoning...
Overview
This PR adds Bun.XML.parse / Bun.XML.stringify and an .xml module loader, following the Bun.TOML / Bun.YAML / Bun.JSON5 pattern. The core is a new ~3,100-line XML 1.0 (5th ed.) non-validating processor in src/parsers/xml.rs (scanner + recursive-descent parser over an input-frame stack for entity inclusion) and an ~800-line XMLObject.rs stringifier that walks JS values. Loader plumbing touches ast/loader.rs, options_types/schema.rs, bundler/{ParseTask,transpiler,options,LinkerContext}.rs, jsc_hooks.rs, ModuleLoader.cpp, headers-handwritten.h, bundler_plugin.h, schema.js, and the printer. Docs, .d.ts, a benchmark, and ~2,000 W3C-conformance-suite tests plus a hand-written xml.test.ts round it out.
Security risks
XML parsing is a classic attack surface. The parser explicitly never reads external DTDs or external entities (EntityValue::External → Resolved::Unexpanded), so there is no XXE / SSRF vector. Billion-laughs is bounded by expat-style MAX_AMPLIFICATION (100×) and MAX_ENTITY_DEPTH (256), and element/content-model recursion is guarded by StackCheck. UTF-8 is validated with simdutf before any decoding; UTF-16/Latin-1 are transcoded up front. Encoding-declaration handling refuses contradictions and unsupported encodings. The stringifier uses a visiting set for cycles, StackCheck for depth, escapes & < > " \t \n \r, and rejects non-Char code points and non-Name element/attribute names. I did not find a reachable panic or unbounded-work path, but this is exactly the class of code (adversarial-input parser, new public API) where a maintainer should sanity-check the design.
Level of scrutiny
High. This is a new user-facing API with design decisions a human should own: the compact-vs-node output shapes, the @attr / #text key conventions, stringify's treatment of bigint / Date / null / skipped values, and the documented behavior change that .xml imports now yield a parsed object instead of a file path. The parser is large, arena-allocated, and byte-oriented — well-structured and heavily tested against the W3C suite, but not the kind of change a bot should approve unilaterally.
Other factors
All prior review threads (CodeRabbit and my earlier inline findings on push_reference / is_ascii and the BunLoaderTypeMD = 20 mismatch) are resolved; the author addressed or justified each in 042d317. Test coverage is unusually strong (927 must-reject + 752 must-accept W3C cases with 262 canonical-output checks, plus round-trip, encoding, loader, bundler, plugin, and type-fixture tests). The current bug-hunting pass found nothing new. Deferring solely because scope and API surface warrant a maintainer sign-off, not because of any open concern.
2 participants
What does this PR do?
Adds
Bun.XML.parse/Bun.XML.stringifyand makes.xmlfiles importable (import,require,bun build,with { type: "xml" }), following the shape ofBun.TOML/Bun.YAML/Bun.JSON5. Refs #9304; supersedes #29154.src/parsers/xml.rsis an XML 1.0 (5th ed.) non-validating processor that never reads external entities: structured likeyaml.rs— one scanner loop walks the bytes and produces tokens (whitespace is just an arm that advances; the token records whether it was preceded by whitespace so the parser can enforce XML's required/forbiddenS), and a recursive-descent parser consumes tokens and never touches source bytes. Internal entities are expanded (with expat-style amplification/depth limits, so billion-laughs throws), attribute values are normalized and internal-subset defaults applied, and UTF-8 / UTF-16 / ISO-8859-1 input is decoded per §4.3.3.parsereturns the compact object by default ("@attr","#text", arrays for repeated names, values always strings);{ compact: false }returns the ordered{ name, attributes, children }tree.stringifyaccepts either, withJSON.stringify-stylespace.xml-test-suite.test.tsis generated from the W3C conformance suite (xmlts20130923): 927 must-reject, 752 must-accept (262 checked against canonical output), 316 processor-class-dependent cases pinned with the upstream verdict noted..yaml/.json5landed:.xmlnow has a default loader, so importing one yields the parsed object rather than afile-loader path;--loader .xml:filerestores that.src/api/schema.jsgains thejson5/md/xmlloader entries it lacked, soBun.buildplugins can see and return them.How did you verify your code works?
bun bd testontest/js/bun/xml/(1995 + 54 pass),resolve/xml,require.test.ts,bundler_loader,bundler_plugin -t Xml,transpiler-unsupported-loader, and the bun-types test; the new tests fail underUSE_SYSTEM_BUN=1.bun run rust:check-allis clean.bench/xml(release, M4 Max): parse is ~5× fast-xml-parser / xml2js on ~200 KB feeds and on par for tiny documents.