ElementTree.iselement() returns true for the Element type

[serhiy-storchaka]

According to documentation, ElementTree.iselement() check if the argument is an element object.

But it returns true also for the Element type which has all attributes of an Element instance. The way how the Element class is defined in Python suggests that this may be intentional, but the intention was lost in the documentation.

There is also an example of dataclasses.is_dataclass() which returns true both for a dataclass type and for a dataclass instance.

So, there are two ways:

1. Fix the documentation to match the current behavior. All uses of iselement() (see Calling ElementTree.write on an ElementTree instance where ElementTree._root is the wrong type causes possible data loss #135640) should also check if the argument is a type is they need an element object.
2. Fix the implementation to check if the argument is a type. This may break the user code that uses it to check for the Element type (I don't know if it exists) but may fix bugs in the user code which expects that iselement() works as documented.

cc @scoder

[abstractedfox]

Oh interesting, what about the class suggests that it could have been deliberate behavior?

I wonder if this is the sort of situation where it would make sense to update the documentation and internal usage in the near term to avoid breaking changes, then fix the implementation for a future release? Based on how it's documented it feels unlikely that there would be too many cases where someone's deliberately using it to check for an Element type; I also feel that since it's been documented to work that way, changing it to do so more exactly (in a future release) wouldn't break any promises

[serhiy-storchaka]

The Element class has the class variable tag initialized to None.

There is a practical reason to define class variables text and tail. They are None in many elements, so this allows to not set an instance attribute to save memory and time. But tag and attrib are always set in the constructor. Setting the class variables may be intentionally, to make the Element class having all attributes of the Element instance, or it may be just without any reason, just for completeness or internal documentation (more likely).

So, if there was no intention to use iselement() for types, it is better to change the implementation, because users most likely are not aware of such pitfall. It can be qualified as a bug fix.

@scoder is the lxml.etree maintainer, he can shed light on this.

[scoder]

Thanks for asking me in.

The 2007 code base of ElementTree already had this comment:

def iselement(element):
    # FIXME: not sure about this; might be a better idea to look
    # for tag/attrib/text attributes
    return isinstance(element, Element) or hasattr(element, "tag")

A changelog comment in cElementTree.c can be read as an indication that the iselement() function might have appeared in ElementTree in 2005, when cElementTree was implemented:

 * 2005-03-01 fl  added iselement function; fixed makeelement aliasing (1.0.2)

And an assertion in ElementTree at the time suggests that the input was intended to be strict.

class ElementTree:

    def __init__(self, element=None, file=None):
        assert element is None or iselement(element)
        self._root = element # first node

My guess is that the effect on classes was not intended. It probably snuck in when cElementTree was added and the Element type was no longer unique. But then, for a more than twenty year old library, you can never know who uses it in what way, and what breaks if you change the behaviour. (Which PR #135643 also does, BTW. It seems a bit of a jump to solve the problem at hand, but it escapes me what you could reasonably do with an ElementTree that doesn't have an Element instance (or None) as root node…)

I would also guess that the difference almost never matters in real life. How often does it happen that you have an object in your hands and you do not know if it's a class or instance? Data structures tend to be pretty clean in that regard. NB, it apparently took more than twenty years to start this discussion. In my experience, developers rather tend to forget about the possibility of receiving a class when testing the type of an object (really reading "object" as "instance").

For comparison, lxml was always strict on the types. It prevented ElementTree(not_an_element) and returned False for iselement(not_an_element_instance). That is due to the internal C implementation where types matter a lot more than in Python.

If you consider it the right moment to make ElementTree() strict on the input, then it's probably also the right time to make iselement() stricter in one go. My intuition is that very few people will notice it. But as always with long living code, a few might.

[serhiy-storchaka]

Thank you for your insights @scoder.

Type-checking in append(), extend() and insert() methods was added in 396e8fc (bpo-13782/gh-57991) and corrected in ee32931 (bpo-16089/gh-60293). It was 13 years ago. Well, it is perhaps time to strengthened iselement() and type-checking in ElementTree too.

Is it possible to use the stdlib's ElementTree with lxml's Element? The documentation avoids referring to the Element class if possible and just use "element" (lowercase), so it implies that more implementations can be supported, but I do not know if this is used on practice.

[scoder]

Is it possible to use the stdlib's ElementTree with lxml's Element?

I just added an integration test for that to lxml, yes, it works to use the stdlib serialiser with lxml Elements. It's not going to be hyper fast but speed isn't always the main priority if users are forced to make tools work together at all. We should make sure to keep that working. Thus, a specific type check would not be the right thing to guard ElementTree() or the serialiser (including tostring()).

That makes me wonder if rejecting types is something we really want. lxml allows user defined Element classes and users could potentially use multiple inheritance to inject subtypes of type for that. I've never heard anyone do that, but it's possible in theory. A scenario could be to instantiate object trees by parsing an XML structure into a tree of types and then recursively calling the types to construct the object tree from the XML tree. That's a hypothetical scenario and I'd personally use Element classes with some kind of factory functions instead, but the scenario seems valid as such. The mere fact that we have a type in our hands doesn't necessarily make the object an invalid Element.

[abstractedfox]

Hm, then perhaps it does only call for updating the documentation? Maybe it should mention those types of patterns too? Thanks for the explanation and context, I wasn't even aware that it could have been interoperating with other implementations before I opened my issue. I could picture that happening in practice if someone were using two modules that had been developed separately from one another, or something to that effect

[serhiy-storchaka]

Inheriting from type is a bad idea. For example, pickling and copying will no longer work (because x.__reduce__() and x.__copy__() work differently for the type and the instance).