code_richcompare() don't use constant type when comparing code constants

@arigo

BPO	25843
Nosy	@arigo, @rhettinger, @mdickinson, @ncoghlan, @vstinner, @larryhastings, @ezio-melotti, @bitdancer, @Bluehorn, @serhiy-storchaka, @Vgr255
Files	code_richcompare.patch code_richcompare-2.patch code_eq.py code_richcompare-3.patch code_richcompare-4.patch code_richcompare-5.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2016-01-22.15:54:05.277>
created_at = <Date 2015-12-11.19:23:13.886>
labels = ['type-bug']
title = "code_richcompare() don't use constant type when comparing code constants"
updated_at = <Date 2016-06-14.22:44:29.013>
user = 'https://bugs.python.org/TijsVanOevelen'

bugs.python.org fields:

activity = <Date 2016-06-14.22:44:29.013>
actor = 'larry'
assignee = 'none'
closed = True
closed_date = <Date 2016-01-22.15:54:05.277>
closer = 'vstinner'
components = []
creation = <Date 2015-12-11.19:23:13.886>
creator = 'Tijs Van Oevelen'
dependencies = []
files = ['41307', '41308', '41309', '41671', '41676', '41682']
hgrepos = []
issue_num = 25843
keywords = ['patch']
message_count = 53.0
messages = ['256229', '256230', '256233', '256234', '256235', '256245', '256250', '256251', '256252', '256274', '256285', '256290', '256317', '256318', '256320', '256321', '256323', '256327', '256330', '256332', '256381', '256382', '256383', '256386', '256390', '256398', '256399', '256400', '256401', '256402', '256403', '256404', '256405', '256406', '256409', '258532', '258683', '258707', '258724', '258725', '258753', '258756', '258795', '258796', '258814', '258815', '258817', '258818', '259016', '268555', '268556', '268557', '268588']
nosy_count = 17.0
nosy_names = ['arigo', 'rhettinger', 'mark.dickinson', 'ncoghlan', 'vstinner', 'larry', 'donmez', 'ezio.melotti', 'r.david.murray', 'torsten', 'fijall', 'python-dev', 'serhiy.storchaka', 'David MacIver', 'abarry', 'Kevin Shweh', 'Tijs Van Oevelen']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'patch review'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue25843'
versions = ['Python 2.7', 'Python 3.5', 'Python 3.6']

Linked PRs

gh-70030: Remove _PyCode_ConstantKey() function #139735

The bug is described on Stack Overflow in length:

http://stackoverflow.com/questions/34100732/unexpected-output-using-pythons-ternary-operator-in-combination-with-lambda

For reference, the minimal reproducer is:

>>> f1, f2 = lambda: 1, lambda: 1.0
>>> f2()
1

The cause (according to the answer in the link) is that the two lambda's incorrectly compare as equal and end up sharing the same code object...one of the inequality checks is line number, so this only happens when the lambdas are on the same line.

Aside: it would be helpful if people would post bug summaries instead of just linking to an outside article. The link is of course very valuable information, but it saves us time if the poster summarizes the bug so the right people know to look at it.

Apologies for not posting a summary of the bug. I really had no idea how to describe the problem, as it is over my head. I could only refer to my question on Stack Overflow that triggered the discovery of the bug.

It's definitely also in 3.4 by the way.

Thanks for posting the bug. I added the aside because this is the third or fourth of these reference-only things I've seen in the past couple weeks and I finally figured out what bothered me about them. We should put something about this in the devguide on bug reporting, but of course most people reporting bugs will for good reason not read that. So, I just have to live with it :)

And yes, the reproducer reproduces the problem in all python versions currently under maintenance.

I've now changed the title to reflect the underlying bug.

The equality of code objects is determined by the code_richcompare() logic in Objects/codeobject.c.

Two code objects are equal if all of their attributes compare equal. That includes co_name, co_argcount, co_kwonlyargcount, co_nlocals, co_flags, co_firstlineno, co_code, co_consts, co_names, co_varnames, co_freevars, and co_cellvars.

At the heart of David Murray's minimal example, the reason the two distinct code objects compare equal is that their co_consts compare as equal.

If you wanted to fix this, code objects would need to recursively check for both normal equality and type equality.

>>> f1 = lambda: 1
>>> f2 = lambda: 1.0
>>> f1.__code__.co_consts == f2.__code__.co_consts
True
>>> map(type, f1.__code__.co_consts) == map(type, f2.__code__.co_consts)
False

A type-based check runs into problems with 0.0 vs -0.0. For example, on Python 2.7.11:

>>> x, y = lambda: 0.0, lambda: -0.0
>>> y()
0.0

I wasn't able to reproduce the -0.0 problem with Python 3.4 on Ideone; y.__code__.co_consts seems to have an unused 0.0 in it on 3.4. I don't have access to Python 3.5, so I don't know what the situation is like on that version.

Here's another variant (courtesy of Nick Coghlan):

python3.5 -c "seq1 = [1.0 for x in range(5)]; seq2 = [True for x in range(5)]; print(seq1); print(seq2)"

One possible solution for all these variants is to let code objects track both the co.firstlineno and co.firstrowno.

FWIW, the bug is present in PyPy as well.

From what I can see:

checking constant types in addition to their values should be a two line change (plus tests)
checking the column number in addition to the line number would be a more comprehensive fix, but also a constructor API change (since PyCode_New doesn't currently accept a column parameter)

The "values of the same constant type that are distinct but equivalent may still compare equal" case is obscure enough that I think the lower impact change is likely a better option, especially as 3.x currently "handles" the "lambda: -0.0" case by having both the unfolded 0.0 and the folded -0.0 in the constant list.

------------
Additional detail for those interested:

The lowest impact fix from a code change would be to add a type equivalence check for constants as Raymond first suggested, as that only involves adding an extra check to code_richcompare: https://hg.python.org/cpython/file/tip/Objects/codeobject.c#l416

However, the idea of tracking "co_firstcolno" in addition to "co_firstlineno" is a more robust fix, as it means any lexically distinct code objects will necessarily be considered distinct by the interpreter. The downside is that it means touching more code and adding a new public API, since PyCode_New doesn't currently accept a "firstcolno" parameter - code objects are implicitly assumed to be one-per-line.

Out of curiosity, I also went looking for the code in the code generator that collapses the "equivalent" code objects together. The issue is that constants are stored in a dict (mapping them to their co_consts index) during compilation and assembly, so equivalent objects will be merged together (and refer to the index of the first one defined).

Interestingly, co_filename is not used as part of the equivalence criteria, so code object equivalence can be fooled across multiple input files. Fortunately in this case, the false equivalence isn't relied on by the code generator.

  $ cat a.py
  def f():
      return 1

  $ cat b.py
  def f():
      return 1.0

  $ ./python.exe -q
  >>> import a, b
  >>> a.f.__code__ == b.f.__code__  # False equivalence
  True
  >>> a.f()
  1
  >>> b.f()                         # Generated code is correct
  1.0

Besides aliasing int/float/decimal/fraction/complex/bool, codeobj.__eq__() can also alias str/unicode on Python 2.7. Likewise, 0.0 and -0.0 can be conflated. NaNs don't seem to be a problem.

I think we should focus on fixing the spec for code object equivalents. Perhaps the test can be simplified to use (co_firstlineno, co_firstrowno, co_filename).

Updated patch version 5.

I meant that set shouldn't be handled in _PyCode_ConstantKey at all. Only frozenset constants can be considered equal. Sets as well as lists all should be different.

Ok, I dropped support for set type to only keep support for frozenset. set are now always considered as different, as list and other "unsupported types".

There is yet one issue with string and bytes literals. When run Python with the -b option:
(...)
May be the type should be the first item in the key.

Oh, good catch. It's an old bug.

Yeah, putting the type as the first parameter fixes the issue. I made this change. I had to update compile.c to exchange the type and the value in the tuple.

New changeset 6c33d4cc9b8f by Victor Stinner in branch 'default':
code_richcompare() now uses the constants types
https://hg.python.org/cpython/rev/6c33d4cc9b8f

I pushed my latest patch with minor changes in comments.

I will wait for buildbots before backporting the change to Python 2.7 and 3.5. For the backport, I will probably remove the optimization on frozenset since it's only useful for AST optimizers (the optimization is a new feature, I considered that it was worth it to add it Python 3.6 as part of my work on the PEP-511).

New changeset 8e0a736b82ff by Victor Stinner in branch '3.5':
code_richcompare() now uses the constants types
https://hg.python.org/cpython/rev/8e0a736b82ff

I will probably remove the optimization on frozenset since it's only useful for AST optimizers (the optimization is a new feature, I considered that it was worth it to add it Python 3.6 as part of my work on the PEP-511).

Hum, it doesn't work: test_compile_ast() of test_compile fails without this code. The test relies (indirectly) on the fact that two code objects using a frozenset constant are equal.

New changeset 9e13d97ceca2 by Victor Stinner in branch '2.7':
code_richcompare() now uses the constants types
https://hg.python.org/cpython/rev/9e13d97ceca2

Ok, the fix is now pushed to Python 2.7, 3.5 and 3.6.

Thanks Tijs Van Oevelen for your bug report ;-)

A workaround look to define the two lambda functions on two different lines. Or maybe cast explicitly to float? I don't think that it's a common bug so you should be able to survive with it until the next bugfix version is released :-)

New changeset 16f60cd918e0 by Victor Stinner in branch 'default':
PEP-511
https://hg.python.org/peps/rev/16f60cd918e0

Someone asked on reddit. The Misc/NEWS entry for this reads:

Issue bpo-25843: When compiling code, don’t merge constants if they are equal but have a different types. For example, f1, f2 = lambda: 1, lambda: 1.0 is now correctly compiled to two different functions: f1() returns 1 (int) and f2() returns 1.0 (int), even if 1 and 1.0 are equal.

Shouldn't that last part read

"and f2() returns 1.0 (float), even if 1 and 1.0 are equal."
^^^^^

As in, f2 returns a float, not an int.

If this is a mistake, let me fix it for 3.5.2 final and I'll merge it back into trunk etc. If you fix it it wouldn't ship in 3.5.2 final.

New changeset a36238de31ae by Victor Stinner in branch '3.5':
Issue bpo-25843: Fix the NEWS entry
https://hg.python.org/cpython/rev/a36238de31ae

Oh, I fixed the typo in the Misc/NEWS in 3.5 and default branches.

Yes, which doesn't help 3.5.2 final as I don't pull revisions by default after rc1. :p

gh-70030: Remove _PyCode_ConstantKey() function (#139735)

Navigation Menu

Repository navigation

code_richcompare() don't use constant type when comparing code constants #70030

Linked PRs

30 remaining items

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

code_richcompare() don't use constant type when comparing code constants #70030

Description

Linked PRs

Activity

TijsVanOevelen commented on Dec 11, 2015

bitdancer commented on Dec 11, 2015

TijsVanOevelen commented on Dec 11, 2015

TijsVanOevelen commented on Dec 11, 2015

bitdancer commented on Dec 11, 2015

rhettinger commented on Dec 11, 2015

KevinShweh commented on Dec 12, 2015

rhettinger commented on Dec 12, 2015

rhettinger commented on Dec 12, 2015

rhettinger commented on Dec 12, 2015

ncoghlan commented on Dec 12, 2015

rhettinger commented on Dec 12, 2015

30 remaining items

vstinner commented on Jan 21, 2016

python-dev commented on Jan 22, 2016

vstinner commented on Jan 22, 2016

python-dev commented on Jan 22, 2016

vstinner commented on Jan 22, 2016

python-dev commented on Jan 22, 2016

vstinner commented on Jan 22, 2016

python-dev commented on Jan 27, 2016

larryhastings commented on Jun 14, 2016

python-dev commented on Jun 14, 2016

vstinner commented on Jun 14, 2016

larryhastings commented on Jun 14, 2016

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions