Skip to content

Forward pyexpat lexical events#8264

Open
doma17 wants to merge 1 commit into
RustPython:mainfrom
doma17:fix-pyexpat-lexical-events
Open

Forward pyexpat lexical events#8264
doma17 wants to merge 1 commit into
RustPython:mainfrom
doma17:fix-pyexpat-lexical-events

Conversation

@doma17

@doma17 doma17 commented Jul 12, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

Forward processing instructions, comments, and CDATA boundaries from xml-rs to the corresponding pyexpat handlers.

Problem

RustPython exposed ProcessingInstructionHandler, CommentHandler, and the CDATA handlers, but the parser discarded those events. Code using the handlers received only character data.

Fix

Preserve comment and CDATA events in the parser configuration, then dispatch them through the existing handler bridge. Processing instructions without data are passed as an empty string, matching CPython.

Testing

  • cargo run -- extra_tests/snippets/stdlib_xml.py
  • cargo run --release -- -m test test_pyexpat
  • cargo run --release -- -m test test_sax
  • prek run --all-files
  • Full extra_tests: 402 passed
  • Workspace test suite
  • Clippy

Summary by CodeRabbit

  • Bug Fixes

    • Improved XML parsing compatibility by correctly reporting processing instructions, comments, and CDATA sections.
    • Preserved CDATA section boundaries and content when invoking configured parser callbacks.

  • Tests

    • Added coverage verifying event order and callback behavior for processing instructions, comments, CDATA, and character data.

@doma17
Restore pyexpat lexical callback compatibility
8d6d9cb 
Forward processing instructions, comments, and CDATA boundaries already emitted by xml-rs so registered Python handlers observe CPython-compatible events.

Constraint: Reuse existing xml-rs events without changing the dependency
Confidence: high
Scope-risk: narrow
Tested: prek; test_pyexpat; test_sax; 402 extra tests; workspace tests; clippy
Assisted-by: Codex:gpt-5.6-sol
@coderabbitai

coderabbitai Bot commented Jul 12, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The expat-compatible parser preserves CDATA and comment events, dispatches processing instruction, comment, and CDATA handlers, and adds a test verifying the ordered callbacks for mixed XML content.

Changes

Expat event compatibility

Layer / File(s) Summary
Parser event configuration and dispatch
crates/stdlib/src/pyexpat.rs		 Configures xml-rs to preserve CDATA and comments, then dispatches processing instruction, comment, character, and CDATA boundary handlers.
Mixed-event compatibility validation
extra_tests/snippets/stdlib_xml.py		 Registers expat callbacks, parses mixed XML content, and asserts the resulting event order.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant XMLDocument
  participant PyExpatLikeXmlParser
  participant xml-rs
  participant ExpatHandlers
  XMLDocument->>PyExpatLikeXmlParser: Parse mixed XML content
  PyExpatLikeXmlParser->>xml-rs: Preserve CDATA and comments
  xml-rs->>PyExpatLikeXmlParser: Emit XML events
  PyExpatLikeXmlParser->>ExpatHandlers: Invoke matching callbacks
Loading

Suggested reviewers: ShaharNaveh

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: forwarding additional pyexpat lexical events such as processing instructions, comments, and CDATA boundaries.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

📦 Library Dependencies

The following Lib/ modules were modified. Here are their dependencies:

[x] lib: cpython/Lib/xml
[x] test: cpython/Lib/test/test_xml_etree.py (TODO: 61)
[x] test: cpython/Lib/test/test_xml_etree_c.py
[x] test: cpython/Lib/test/test_minidom.py (TODO: 25)
[x] test: cpython/Lib/test/test_pulldom.py (TODO: 3)
[ ] test: cpython/Lib/test/test_pyexpat.py (TODO: 49)
[x] test: cpython/Lib/test/test_sax.py (TODO: 31)
[x] test: cpython/Lib/test/test_xml_dom_minicompat.py
[x] test: cpython/Lib/test/test_xml_dom_xmlbuilder.py

dependencies:

  • xml

dependent tests: (34 tests)

  • xml: test_doctest test_minidom test_pulldom test_pydoc test_pyexpat test_regrtest test_sax test_typing test_xml_dom_minicompat test_xml_dom_xmlbuilder test_xml_etree
    • plistlib: test_plistlib
      • platform: test__locale test__osx_support test_asyncio test_baseexception test_builtin test_cmath test_ctypes test_fcntl test_math test_mimetypes test_os test_platform test_posix test_shutil test_socket test_ssl test_strptime test_sysconfig test_time test_winreg test_wsgiref
    • xmlrpc.client: test_xmlrpc

Legend:

  • [+] path exists in CPython
  • [x] up-to-date, [ ] outdated

ShaharNaveh
ShaharNaveh reviewed Jul 12, 2026
View reviewed changes
Comment thread crates/stdlib/src/pyexpat.rs
let str = PyStr::from(chars).into_ref(&vm.ctx);
invoke_handler(vm, &self.character_data, (str,));
}
Ok(XmlEvent::ProcessingInstruction { name, data }) => {

@ShaharNaveh ShaharNaveh Jul 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I can't comment on things outside the diff. can you please make the match to be:

- match e {
+ match e? {

then we could do:

- Ok(XmlEvent::Foo { .. })
+ XmlEvent::Foo { ..}

ShaharNaveh
ShaharNaveh approved these changes Jul 12, 2026
View reviewed changes

@ShaharNaveh ShaharNaveh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

tysm!

can you please unmark the passing tests?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ShaharNaveh ShaharNaveh ShaharNaveh approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@doma17 @ShaharNaveh