gh-83122: Deprecate testing element truth values in ElementTree

[jacobtylerwalls]

gh-83122

When testing element truth values, emit an identical FutureWarning in the C implementation as in the Python implementation. EDIT: changed to DeprecationWarning in both implementations per comments.

Matching an element in a tree search but having it test False can be unexpected. Raising the warning in both places enables making the choice to finally raise an exception for this ambiguous behavior in the future.

Documentation promising FutureWarnings:
Element.remove()
2.7 release notes

Fixes #83122

[JelleZijlstra]

Thanks!

@gvanrossum I'm planning to merge this PR (you approved the idea on BPO a few years back)

[gvanrossum]

One question -- how likely is it that ElementTree is actually ever going to change this behavior? Hasn't it been super-stable for many years?

[JelleZijlstra]

That's a fair point. On the bug report people were supportive of changing the behavior, but I don't know that it's ever actually going to happen.

[gvanrossum]

Let’s find out first.

[gvanrossum]

If the warning was only shown by the Python version, very few people have seen it, right, because (I presume) the C version is always used if it can be imported.

I'm guessing that the idea is that nodes will always be truthy, so you can use if node as a shorthand (in most contexts) for if node is not None right? I do endorse that in principle, but I'm not sure if it's worth requiring all elementtree applications to change.

[jacobtylerwalls]

Forgive me if this is stating the obvious, but as I see the status quo, users are tempted to write if node under the mistaken belief that all nodes are truthy, and then get bitten when the condition excludes childless nodes. I made this mistake a few times when learning ElementTree, and it's something I have to remember to look for in code review. I would have much preferred to see the warning! :-)

I'm guessing that the idea is that nodes will always be truthy, so you can use if node as a shorthand (in most contexts) for if node is not None right?

I might be misunderstanding you, but since nodes are not always truthy, I'm hoping for ElementTree to discourage this usage.

I'm not sure if it's worth requiring all elementtree applications to change.

The applications that need to change will be ones that desired if node to mean if node is not None and len(node), which seems like strange cases to commingle (especially in the negative, commingling None and childless nodes).

If the warning was only shown by the Python version, very few people have seen it, right, because (I presume) the C version is always used if it can be imported.

Right. Of course if the ultimate resolution here is a warning rather than an exception, we could just change FutureWarning to RuntimeWarning and be done with it. Then it could be something for linters to make an effort to detect and warn about.

[jacobtylerwalls]

The applications that need to change

Ah, I didn't put that very well. Of course applications will have to deal with the warning proposed in the issue (and in the python implementation). I meant something like, applications "that might find this change unwelcome".

[gvanrossum]

My understanding is that the warning put in the Python code (many, many releases ago?) was a FutureWarning because the plan of record at the time was to change the behavior, after a few releases of issuing warnings. (The error message says so: r"The behavior of this method will change in future versions. ")

But since the warning is not printed in reality, there may also be code that has used the falsity of child-less nodes as a feature -- after all it was put in intentionally as a feature (long, long ago). Your experience of being bitten by this explains why it was a bad feature.

It is not necessarily the case that code using if node meant it to mean if node is not None and len(node) -- the code could already know that node is an elementtree node, and the user could have meant it to mean if len(node).

I dislike warnings that live forever -- they have a place, but I don't have to like them, and if we're putting in a FutureWarning we promise to eventually change the behavior. So we need to have a think about that.

[JelleZijlstra]

I posted on the issue to make sure we gather consensus on the right way forward here. Until we do that I'm unassigning myself from this PR. Sorry for getting your hopes up @jacobtylerwalls!

[gpshead]

Lets change the warning in the existing code and the new equivalent added to the C code to DeprecationWarning.

FutureWarning was never intended to be used for this purpose. If we go forward with this, it'll become a behavior change in >= 3.14.

[bedevere-bot]

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

[jacobtylerwalls]

I have made the requested changes; please review again

[bedevere-bot]

Thanks for making the requested changes!

@JelleZijlstra, @gpshead: please review the changes made to this pull request.

[gpshead]

thanks! I'm so glad to see this long documented as a "bad idea" API wart on the way towards being cleaned up! :)

[serhiy-storchaka]

It could be simply

return self->extra && self->extra->length;