Implement pyexpat ordered_attributes by name-of-okja · Pull Request #8305 · RustPython/RustPython

name-of-okja · 2026-07-17T12:51:07Z

Closes #xxxx
This PR follows our AI policy

Summary

pyexpat ignored the ordered_attributes flag and always passed the
StartElementHandler a dict. But xml.etree.ElementTree (and
xml.dom.minidom via expatbuilder) set ordered_attributes = 1 and
expect a flat [name, value, ...] list. The mismatch made _start
integer-index a dict, raising KeyError, which the handler invoker
swallowed — so parsing any element with an attribute silently failed
(e.g. ET.XML('<a b="c"/>') raised "missing toplevel element").

This honors the flag, matching CPython's my_StartElementHandler:
https://github.com/python/cpython/blob/3.14/Modules/pyexpat.c#L439-L443

ordered_attributes true → flat list [name, value, ...] (document order preserved)
otherwise → dict (unchanged)

Only two consumers in the stdlib enable the flag — xml.etree.ElementTree
and xml.dom.minidom — and both rely on the list contract, so the change
is confined to them; the else branch is byte-identical to the previous
behavior.

Test Plan

Removed @unittest.expectedFailure from tests that now pass
(23 in test_xml_etree, 15 in test_minidom).
Verified locally: test_xml_etree, test_minidom, test_pyexpat
(and test_sax, test_plistlib unaffected — no new failures or
unexpected successes).

Summary by CodeRabbit

Bug Fixes
- Fixed XML start-element attribute handling to respect the parser’s configured attribute output mode.
- With ordered attributes enabled, start-element handlers now receive an ordered list of alternating attribute name/value strings, preserving the original iteration order.
- With ordered attributes disabled, attribute handling remains dictionary-based (mapping attribute names to their stringified values).

coderabbitai · 2026-07-17T12:51:38Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 6d4c8b0a-c9ed-44ef-a0d7-bcf12f45da23

📥 Commits

Reviewing files that changed from the base of the PR and between 159404e and 437b912.

⛔ Files ignored due to path filters (3)

Lib/test/test_minidom.py is excluded by !Lib/**
Lib/test/test_regrtest.py is excluded by !Lib/**
Lib/test/test_xml_etree.py is excluded by !Lib/**

📒 Files selected for processing (1)

crates/stdlib/src/pyexpat.rs

📝 Walkthrough

Walkthrough

Start-element attribute handling now checks ordered_attributes, emitting ordered name/value pairs when enabled and a dictionary otherwise. The VM import list is reformatted.

Changes

XML attribute handling

Layer / File(s)	Summary
Start-element attribute conversion `crates/stdlib/src/pyexpat.rs`	`StartElement` events now produce ordered attribute lists when `ordered_attributes` is enabled and dictionaries otherwise; the VM import list is also reformatted.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: bschoenmaeckers, doma17, shaharnaveh

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main change: adding pyexpat support for ordered_attributes.
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.

✨ 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 · 2026-07-17T12:51:58Z

📦 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: 36)
[x] test: cpython/Lib/test/test_xml_etree_c.py
[x] test: cpython/Lib/test/test_minidom.py (TODO: 5)
[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

[x] lib: cpython/Lib/test/libregrtest

dependencies:

regrtest

dependent tests: (no tests depend on regrtest)

Legend:

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

Copilot

Pull request overview

This PR fixes RustPython’s pyexpat parser to honor the ordered_attributes flag: when enabled, StartElementHandler receives a flat [name, value, ...] list in document order (matching CPython), instead of always receiving a dict. This restores correct behavior for stdlib consumers like xml.etree.ElementTree and xml.dom.minidom that rely on ordered attributes.

Changes:

Implemented ordered_attributes handling in pyexpat start-element dispatch (list vs dict).
Removed @unittest.expectedFailure markers from test_xml_etree and test_minidom tests that now pass.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
`crates/stdlib/src/pyexpat.rs`	Implements list-vs-dict attribute container based on `ordered_attributes`.
`Lib/test/test_xml_etree.py`	Removes expected-failure decorators for ElementTree tests now passing.
`Lib/test/test_minidom.py`	Removes expected-failure decorators for minidom tests now passing.

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

name-of-okja · 2026-07-17T13:29:01Z

+                        let ordered = self
+                            .ordered_attributes
+                            .read()
+                            .clone()
+                            .is_true(vm)
+                            .unwrap_or(false);


@coderabbitai is this correct?

@fanninpm, yes—Copilot’s comment is correct.

create_bool_property! normalizes every assignment via is_true() and stores a newly created canonical bool, while the constructor initializes ordered_attributes to False. Therefore, checking identity with vm.ctx.true_value is sufficient:

let ordered = self.ordered_attributes.read().is(&vm.ctx.true_value);

This also avoids the redundant clone and prevents silently treating a hypothetical truthiness error as false.

coderabbitai

Actionable comments posted: 1

🤖 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 `@crates/stdlib/src/pyexpat.rs`:
- Around line 358-363: In the ordered attribute handling, split the
`self.ordered_attributes.read().clone()` operation from the subsequent
`is_true(vm)` call so the `PyRwLockReadGuard` is dropped before executing Python
code. Update the `ordered` calculation near `is_true(vm)` to use the cloned
value, following the safe pattern in `invoke_handler`.

🪄 Autofix (Beta)

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.yml

Review profile: CHILL

Plan: Pro

Run ID: 62d0e65a-e5d3-4de9-a313-d4135c03d048

📥 Commits

Reviewing files that changed from the base of the PR and between 1205fd2 and a6bcdcc.

⛔ Files ignored due to path filters (2)

Lib/test/test_minidom.py is excluded by !Lib/**
Lib/test/test_xml_etree.py is excluded by !Lib/**

📒 Files selected for processing (1)

crates/stdlib/src/pyexpat.rs

coderabbitai

Actionable comments posted: 1

🤖 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 `@crates/stdlib/src/pyexpat.rs`:
- Line 359: Update the ordered_attributes handling in the surrounding method to
clone the stored attribute reference before evaluating it, ensuring the read
guard is released before truthiness evaluation. Replace the strict
true-singleton identity check with the VM’s normal truthiness conversion so
values such as integer 1 remain truthy while avoiding the deadlock.

🪄 Autofix (Beta)

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.yml

Review profile: CHILL

Plan: Pro

Run ID: cae68bbd-0a15-4422-ba1a-868a393086e7

📥 Commits

Reviewing files that changed from the base of the PR and between 159404e and fb1fce0.

⛔ Files ignored due to path filters (2)

Lib/test/test_minidom.py is excluded by !Lib/**
Lib/test/test_xml_etree.py is excluded by !Lib/**

📒 Files selected for processing (1)

crates/stdlib/src/pyexpat.rs

fanninpm · 2026-07-17T18:52:01Z

There's also an unexpected success in Lib/test/test_regrtest.py (ArgsTestCase.test_xml). Hooray! 🎉

When `ordered_attributes` is set, report attributes to the StartElementHandler as a flat `[name, value, ...]` list matching CPython's my_StartElementHandler, instead of always passing a dict. `xml.etree.ElementTree` and `xml.dom.minidom` (via expatbuilder) both enable this flag and rely on the flat-list contract, so parsing any element with attributes previously failed: the dict made `_start` do integer indexing, raising KeyError which was silently swallowed by the handler invoker, leaving the tree empty ("missing toplevel element"). Reference (CPython 3.14): https://github.com/python/cpython/blob/3.14/Modules/pyexpat.c#L439-L443 Remove `@unittest.expectedFailure` from tests that now pass (23 in test_xml_etree, 15 in test_minidom, 1 in test_regrtest). Assisted-by: Claude:claude-opus-4-8 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

moreal

It seems related with https://github.com/python/cpython/blob/323c59a5e348347be2ce2b7ea55fcb30bf68b2d3/Modules/pyexpat.c#L430-L497 lines in CPython.

It removed so many @unittest.expectedFailure marks. Thank you!

youknowone

great! you break a core blocker

When `ordered_attributes` is set, report attributes to the StartElementHandler as a flat `[name, value, ...]` list matching CPython's my_StartElementHandler, instead of always passing a dict. `xml.etree.ElementTree` and `xml.dom.minidom` (via expatbuilder) both enable this flag and rely on the flat-list contract, so parsing any element with attributes previously failed: the dict made `_start` do integer indexing, raising KeyError which was silently swallowed by the handler invoker, leaving the tree empty ("missing toplevel element"). Reference (CPython 3.14): https://github.com/python/cpython/blob/3.14/Modules/pyexpat.c#L439-L443 Remove `@unittest.expectedFailure` from tests that now pass (23 in test_xml_etree, 15 in test_minidom, 1 in test_regrtest). Assisted-by: Claude:claude-opus-4-8 Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>

Copilot AI review requested due to automatic review settings July 17, 2026 12:51

Copilot started reviewing on behalf of name-of-okja July 17, 2026 12:51

View session

moreal added the z-ca-2026 Tag to track Contribution Academy 2026 label Jul 17, 2026

Copilot AI reviewed Jul 17, 2026

View reviewed changes

coderabbitai Bot reviewed Jul 17, 2026

View reviewed changes

Comment thread crates/stdlib/src/pyexpat.rs Resolved

name-of-okja force-pushed the feat/xml-expat-ordered-attributes branch 2 times, most recently from 28ffe9d to a347bcb Compare July 17, 2026 13:09

name-of-okja marked this pull request as draft July 17, 2026 13:31

name-of-okja force-pushed the feat/xml-expat-ordered-attributes branch from 159404e to fb1fce0 Compare July 17, 2026 13:35

name-of-okja marked this pull request as ready for review July 17, 2026 13:36

coderabbitai Bot reviewed Jul 17, 2026

View reviewed changes

Comment thread crates/stdlib/src/pyexpat.rs Resolved

name-of-okja marked this pull request as draft July 17, 2026 13:43

fanninpm mentioned this pull request Jul 17, 2026

XML-related test failures #8083

Open

name-of-okja force-pushed the feat/xml-expat-ordered-attributes branch from fb1fce0 to 437b912 Compare July 18, 2026 00:04

name-of-okja marked this pull request as ready for review July 18, 2026 00:05

name-of-okja requested a review from fanninpm July 18, 2026 01:12

moreal approved these changes Jul 18, 2026

View reviewed changes

youknowone approved these changes Jul 18, 2026

View reviewed changes

youknowone merged commit 1877adc into RustPython:main Jul 18, 2026
27 checks passed

name-of-okja deleted the feat/xml-expat-ordered-attributes branch July 18, 2026 07:54

📋 Server Request & Proxy Logs

⚙️ Pengaturan Proxy Target

Conversation

name-of-okja commented Jul 17, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jul 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

github-actions Bot commented Jul 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📦 Library Dependencies

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

name-of-okja Jul 17, 2026

Choose a reason for hiding this comment

Uh oh!

fanninpm Jul 17, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Jul 17, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fanninpm commented Jul 17, 2026

Uh oh!

moreal left a comment

Choose a reason for hiding this comment

Uh oh!

youknowone left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

name-of-okja commented Jul 17, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jul 17, 2026 •

edited

Loading

github-actions Bot commented Jul 17, 2026 •

edited

Loading