GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Implement pyexpat ordered_attributes by name-of-okja · Pull Request #8305 · RustPython/RustPython

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.

Copy link

Copy Markdown

Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

fanninpm reacted with eyes emoji

All reactions

👀 1 reaction

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

coderabbitai Bot commented Jul 17, 2026 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

github-actions Bot commented Jul 17, 2026 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

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: 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:

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

Copilot AI left a comment

Copy link

Copy Markdown

There was a problem hiding this 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 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

crates/stdlib/src/pyexpat.rs Outdated

Comment on lines +358 to +363

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

name-of-okja Jul 17, 2026

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Good catch

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

fanninpm Jul 17, 2026

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@coderabbitai is this correct?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

coderabbitai Bot Jul 17, 2026

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

coderabbitai Bot reviewed

Jul 17, 2026

View reviewed changes

coderabbitai Bot left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

crates/stdlib/src/pyexpat.rs Resolved

Uh oh!

There was an error while loading. Please reload this page.

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

coderabbitai Bot left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

crates/stdlib/src/pyexpat.rs Resolved

Uh oh!

There was an error while loading. Please reload this page.

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

fanninpm commented Jul 17, 2026

Copy link

Copy Markdown

Contributor

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

name-of-okja and moreal reacted with thumbs up emoji

All reactions

👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          Implement pyexpat ordered_attributes

437b912

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 <[email protected]>

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

moreal left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

name-of-okja reacted with thumbs up emoji

All reactions

👍 1 reaction

youknowone approved these changes

Jul 18, 2026

View reviewed changes

youknowone left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

great! you break a core blocker

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

name-of-okja reacted with hooray emoji

All reactions

🎉 1 reaction

youknowone merged commit 1877adc into RustPython:main

Jul 18, 2026

27 checks passed

Uh oh!

There was an error while loading. Please reload this page.

name-of-okja deleted the feat/xml-expat-ordered-attributes branch

July 18, 2026 07:54

youknowone pushed a commit that referenced this pull request

Sep 16, 2026


          Implement pyexpat ordered_attributes (#8305)

f74e725

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 <[email protected]>

Reviewers

coderabbitai[bot]

coderabbitai[bot] left review comments

Copilot code review Copilot

Copilot review effort

Applies to this pull request for everyone.Learn more about Copilot code review.

Copilot left review comments

youknowone

youknowone approved these changes

moreal

moreal approved these changes

fanninpm

Awaiting requested review from fanninpm

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

z-ca-2026

Tag to track Contribution Academy 2026

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

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.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

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

Uh oh!

There was an error while loading. Please reload this page.

coderabbitai Bot commented Jul 17, 2026 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

github-actions Bot commented Jul 17, 2026 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.