classification
Title: PyObject_RichCompare differs in behaviour from PyObject_RichCompareBool ; difference not noted in documentation
Type: behavior Stage:
Components: Documentation Versions: Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Devin Jeanpierre, docs@python, eli.bendersky, georg.brandl, mark.dickinson, ncoghlan, python-dev, rhettinger, terry.reedy
Priority: low Keywords: patch

Created on 2011-01-15 01:00 by Devin Jeanpierre, last changed 2011-04-30 05:56 by eli.bendersky. This issue is now closed.

Files
File name Uploaded Description Edit
issue10912.py3k.1.patch eli.bendersky, 2011-01-15 09:00
Messages (15)
msg126306 - (view) Author: Devin Jeanpierre (Devin Jeanpierre) * Date: 2011-01-15 01:00
PyObject_RichCoareBool is, according to the documentation in trunk (Doc\c-api\object.rst), exactly the same as PyObject_RichCompare, except it 1, 0, or -1 for error, false, and true respectively. However, it differs in behavior by both that, and also by assuming that identity implies equality. This noted in a two year-old ML post (sadly, no bug report was posted as best as I can find): http://mail.python.org/pipermail/python-list/2009-March/1195170.html

Ideally PyObject_RichCompareBool should probably be named something else, since it can no longer be used, strictly, as "PyObject_RichCompare, but returning a C bool" (or, rather, a C int). Some suggestions were offered in the mailing list thread above.

I'm filing this as a documentation bug because I find that outcome unlikely. At least the documentation should note the difference in behavior, so that people do not accidentally write C code that does not behave as intended.

This issue is related to, but different from issue 4296, which objected to the new container behavior created by the change to PyObject_RichCompareBool. My only objection here is that the latter change does not appear to be documented.

I would supply a patch for the tests, but PyObject_RichCompareBool is apparently not directly tested anywhere, just tested obliquely through testing the containment operator, and this was changed by the same commit that changed PyObject_RichCompareBool. I don't know how to word the (very small!) change to the docs.
msg126320 - (view) Author: Eli Bendersky (eli.bendersky) * (Python committer) Date: 2011-01-15 09:00
I agree that the difference in behavior between the two functions is unfortunate, but that's unlikely to change now.

The least we can do is make the documentation precise. I'm attaching a proposed patch to Doc/c-api/object.rst

Nick/Terry/Georg - if this looks OK I can commit.
msg126321 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2011-01-15 09:17
Sure.
msg126322 - (view) Author: Eli Bendersky (eli.bendersky) * (Python committer) Date: 2011-01-15 10:23
Committed the fix to py3k in r88009
msg126323 - (view) Author: Eli Bendersky (eli.bendersky) * (Python committer) Date: 2011-01-15 10:35
Backport to release31 branch in r88010
msg126330 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2011-01-15 14:32
For the record, the gory details as to *why* RichCompareBool makes more assumptions as to the meaning of equality than the basic RichCompare function can be found in issue 4296 (I just found that issue myself by looking at Mark's response to the python-list thread).

It may be worth explicitly pointing out that use cases where this assumption is unacceptable would be better served by direct invocation RichCompare function.
msg126331 - (view) Author: Eli Bendersky (eli.bendersky) * (Python committer) Date: 2011-01-15 14:37
Nick, I read the protocol of issue 4296 and I understand the reasoning behind the code, but I still think the naming is mightily confusing. Two distinct changes went into a single function (A) Return -1/0/+1 instead of PyObject and (B) the id() shortcut, and its name is inappropriate. Were an API change feasible, some other naming would be better.

> "It may be worth explicitly pointing out that use cases where this assumption is unacceptable would be better served by direct invocation RichCompare function."

Do you mean write it down in the docs?
msg126332 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2011-01-15 14:50
Yeah, to prevent perfectly reasonable "why" questions, it is probably worth providing a little extra justification as an addendum to your new note (which is already an improvement on the complete silence on the topic that existed before).

A possible addition:

"... This assumption allows invariants such as "x in [x]" to be more easily guaranteed by the interpreter. If the assumption is not valid for a given use case, call PyObject_RichCompare() directly instead of using this function."

For 3.3, it *may* make sense to provide a PyObject_RichCompareBoolEx() function which includes an additional "reflexive" parameter. Then the existing PyObject_RichCompareBool() semantics would just be the new function with the reflexive argument set to 1.
msg126350 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2011-01-15 23:23
-1 on PyObject_RichCompareBoolEx() for 3.3 -- it is simply an invitation to shoot yourself (or others) in the foot.

I've not seen real world code using bool(a==b) or its C equivalent, so it's hard to believe that there is a use case here (unless you're trying to commit atrocities with NaN-like objects).
msg126352 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2011-01-15 23:52
Indeed. I was actually wondering if it would be worth trying to write up a section for the language reference to describe the cases where a Python implementation is *expected* to assume reflexive equality. We (IMO) have a problem at the moment due to situations like:

>>> class PyContains(list):
...   def __contains__(self, obj):
...     return any(x==obj for x in self)
... 
>>> nan = float("nan")
>>> nan in [nan]
True
>>> nan in PyContains([nan])
False

This is a bug in the __contains__ definition (it should use "x is obj or x == obj" rather than just the latter expression) but there isn't anything in the language reference to point that out.

Assuming reflexive equality in some places and not in others based on the underlying implementation language is going to be a source of subtle bugs relating to types like float and decimal.Decimal.
msg126356 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2011-01-16 00:50
If something needs to be written about NaNs or other invariant destroying objects, perhaps a FAQ entry would suffice (perhaps referencing http://bertrandmeyer.com/2010/02/06/reflexivity-and-other-pillars-of-civilization/ ).
msg126359 - (view) Author: Eli Bendersky (eli.bendersky) * (Python committer) Date: 2011-01-16 05:43
Raymond, 
I initially set "easy" on this issue because I considered it a documentation issue, not the place to resolve the harder debate of the semantics of these functions. Perhaps I was wrong?

Also, I agree with Nick that the difference must be further clarified in "stronger words". If there's no debate on this issue, I will commit a fix after the freeze is over.
msg126360 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2011-01-16 06:01
Yes, it is a doc issue but sometimes those aren't easy to get right (in terms of being beneficial to the reader and in good alignment with the design intentions).  Please attach the doc patch you want to go in and I'll review it sometime the freeze.
msg134554 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2011-04-27 12:19
Reopening, as a 2.7 backport of this would be a nice thing to have.
msg134837 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2011-04-30 05:53
New changeset d27f95e3b52f by Eli Bendersky in branch '2.7':
Issue #10912: add clarification for PyObject_RichCompareBool comparing identical objects
http://hg.python.org/cpython/rev/d27f95e3b52f
History
Date User Action Args
2011-04-30 05:56:48eli.benderskysetstatus: open -> closed
2011-04-30 05:53:12python-devsetnosy: + python-dev
messages: + msg134837
2011-04-27 12:19:32ncoghlansetstatus: closed -> open
assignee: rhettinger ->
messages: + msg134554

versions: + Python 2.7
2011-01-16 06:01:30rhettingersetpriority: normal -> low
nosy: georg.brandl, rhettinger, terry.reedy, mark.dickinson, ncoghlan, Devin Jeanpierre, eli.bendersky, docs@python
messages: + msg126360
2011-01-16 05:43:53eli.benderskysetnosy: georg.brandl, rhettinger, terry.reedy, mark.dickinson, ncoghlan, Devin Jeanpierre, eli.bendersky, docs@python
messages: + msg126359
2011-01-16 00:50:08rhettingersetassignee: docs@python -> rhettinger
messages: + msg126356
keywords: - easy
nosy: georg.brandl, rhettinger, terry.reedy, mark.dickinson, ncoghlan, Devin Jeanpierre, eli.bendersky, docs@python
2011-01-15 23:52:47ncoghlansetnosy: georg.brandl, rhettinger, terry.reedy, mark.dickinson, ncoghlan, Devin Jeanpierre, eli.bendersky, docs@python
messages: + msg126352
2011-01-15 23:23:10rhettingersetnosy: georg.brandl, rhettinger, terry.reedy, mark.dickinson, ncoghlan, Devin Jeanpierre, eli.bendersky, docs@python
messages: + msg126350
2011-01-15 23:10:49rhettingersetnosy: + rhettinger
2011-01-15 19:26:34eric.araujosetnosy: + Devin Jeanpierre
2011-01-15 14:50:53ncoghlansetnosy: georg.brandl, terry.reedy, mark.dickinson, ncoghlan, eli.bendersky, docs@python
messages: + msg126332
2011-01-15 14:37:00eli.benderskysetnosy: georg.brandl, terry.reedy, mark.dickinson, ncoghlan, eli.bendersky, docs@python
messages: + msg126331
2011-01-15 14:32:47ncoghlansetnosy: + mark.dickinson
messages: + msg126330
2011-01-15 10:35:08eli.benderskysetstatus: open -> closed

messages: + msg126323
resolution: fixed
nosy: georg.brandl, terry.reedy, ncoghlan, eli.bendersky, docs@python
2011-01-15 10:23:52eli.benderskysetnosy: georg.brandl, terry.reedy, ncoghlan, eli.bendersky, docs@python
messages: + msg126322
2011-01-15 09:17:01georg.brandlsetnosy: + georg.brandl
messages: + msg126321
2011-01-15 09:00:38eli.benderskysetfiles: + issue10912.py3k.1.patch

nosy: + terry.reedy, eli.bendersky, ncoghlan, - Devin Jeanpierre
messages: + msg126320

keywords: + easy, patch
type: behavior
2011-01-15 01:00:55Devin Jeanpierrecreate