gh-135640: Adds type checking to ElementTree.ElementTree constructor #135643
Conversation
Need to switch machines to test on Linux
Add tests verifying the new type checking behavior
|
All commit authors signed the Contributor License Agreement. |
vstinner
added
needs backport to 3.13
|
Code changes and test look good to me. And I'd suggest to add test on |
|
@abstractedfox: Can you add a NEWS entry? You can also try https://blurb-it.herokuapp.com/ |
| @@ -527,7 +527,9 @@ class ElementTree: | |||
|
|
|||
| """ | |||
| def __init__(self, element=None, file=None): | |||
| # assert element is None or iselement(element) | |||
| if element is not None and not iselement(element): | |||
| raise TypeError(f"element must be etree.Element, " | |||
There was a problem hiding this comment.
I would appreciate either writing xml.etree.Element or Element only.
I also suspect that the isinstance() check was removed so that elements not inheriting from xml.Element still work, or elements that implement the XML interface without directly inheriting from it.
And the minimal element interface is "tag".
| @@ -247,6 +247,13 @@ def check_element(element): | |||
| self.assertRegex(repr(element), r"^<Element 't\xe4g' at 0x.*>$") | |||
| element = ET.Element("tag", key="value") | |||
|
|
|||
| # Verify type checking for ElementTree constructor | |||
There was a problem hiding this comment.
Can you make a separate test method, say "test_constructor" and put that test before test_interface? TiA
| @@ -247,6 +247,13 @@ def check_element(element): | |||
| self.assertRegex(repr(element), r"^<Element 't\xe4g' at 0x.*>$") | |||
| element = ET.Element("tag", key="value") | |||
|
|
|||
| # Verify type checking for ElementTree constructor | |||
|
|
|||
There was a problem hiding this comment.
Add also a test where only an Element-like class is tested (only with a tag attribute, nothing else, not even inheriting from xml.etree.Element).
picnixz
changed the title
Addresses issue #135640 by adding type checking to the constructor and to
ElementTree._setroot, since both have the opportunity to assign _root. In addition to addressing the primary issue (possible data loss), this makes relevant tracebacks more descriptive.It looks like there had once been asserts that did these checks. I couldn't find rationale for why they were commented out in blame (it looks like this was done on svn, and I'm not sure where to look to find those commit messages). I opted to use
raiseinstead as I felt it would be more descriptive.At present, the way
iselementis written still allows an object of the wrong type to pass through if it has atagattribute. Commit a72a98f shows there were once comments indicating that the current implementation was done for a reason, but it isn't clear to me why that is. I left it alone for now, but changing it to this:...also passed tests. If there's no disagreement, I think it would be good to change that too.