Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Boolean constants get confused with integers #2919

Closed
st-pasha opened this issue Apr 11, 2019 · 2 comments
Closed

Boolean constants get confused with integers #2919

st-pasha opened this issue Apr 11, 2019 · 2 comments

Comments

@st-pasha
Copy link

We found that our regular Python code compiled with Cython behaves differently from the original. The issue appears to be with boolean constants being treated as if they were integers 0/1 (or the other way around). The following MRE reproduces the problem:

dt.pyx

# cython: language_level=3

def test():
    j = (False, False)
    k = (0, 0)
    if k[0] is False or k[1] is False:
        raise AssertionError("Oh noes! k = %r" % (k,))
    print("ok")

setup.py

from distutils.core import setup
from Cython.Build import cythonize
setup(ext_modules = cythonize('dt.pyx'))

Buillding

$ python setup.py build_ext --inplace
Compiling dt.pyx because it changed.
[1/1] Cythonizing dt.pyx
running build_ext
building 'dt' extension
gcc -Wno-unused-result -Wsign-compare -Wunreachable-code -fno-common -dynamic -DNDEBUG -g -fwrapv -O3 -Wall -arch x86_64 -g -I/Library/Frameworks/Python.framework/Versions/3.6/include/python3.6m -c dt.c -o build/temp.macosx-10.9-x86_64-3.6/dt.o
gcc -bundle -undefined dynamic_lookup -arch x86_64 -g build/temp.macosx-10.9-x86_64-3.6/dt.o -o /Users/pasha/github/datatable/temp/cy/dt.cpython-36m-darwin.so

Running

$ python
Python 3.6.6 (v3.6.6:4cf1f54eb7, Jun 26 2018, 19:50:54) 
[GCC 4.2.1 Compatible Apple LLVM 6.0 (clang-600.0.57)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> from dt import test
>>> test()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "dt.pyx", line 7, in dt.test
    raise AssertionError("Oh noes! k = %r" % (k,))
AssertionError: Oh noes! k = (False, False)
>>> 
>>> def test():
...     j = (False, False)
...     k = (0, 0)
...     if k[0] is False or k[1] is False:
...         raise AssertionError("Oh noes! k = %r" % (k,))
...     print("ok")
... 
>>> test()
ok
>>> 

Using Cython version 0.29.6 (https://files.pythonhosted.org/packages/d0/27/d7e796420dd1c69135ccf1362cd8ecf61a09db990a8335d65cd715b275b6/Cython-0.29.6-cp36-cp36m-macosx_10_6_intel.macosx_10_9_intel.macosx_10_9_x86_64.macosx_10_10_intel.macosx_10_10_x86_64.whl)

@scoder
Copy link
Contributor

scoder commented Apr 12, 2019

Thanks for the report. My guess is that both tuples are treated as typed ctuples, but that the type deduplication for them does not distinguish the int and bint types and joins both together into (bint, bint). It probably also depends on the order in which you define the tuples.

The deduplication happens in declare_tuple_type() in Symtab.py. It's worth investigating how the hashing and comparison works for the types at hand, to see what can be improved there.

scoder added a commit that referenced this issue Apr 28, 2019
@scoder scoder added this to the 0.29.8 milestone Apr 28, 2019
@scoder
Copy link
Contributor

scoder commented Apr 28, 2019

Wasn't the ctuples after all. What happened, instead, was that the deduplication of Python tuple constants, while taking the type into account, didn't know the concrete Python object type in this case, considering it "some Python object", and thus throwing the two tuple constants together. I pushed a fix that also considers the Python type of the known constant value, so that False, 0 and 0.0 are assigned different deduplication keys.

@scoder scoder closed this as completed Apr 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants