py3: several long/int related fixes

[embray]

These fix a number of major crashes related to handling of Python ints on Python 3, especially in cases that were trying to cram them into C longs.

CC: @jdemeyer @fchapoton

Component: python3

Author: Erik Bray, Jeroen Demeyer

Branch/Commit: 5ed4bda

Reviewer: Jeroen Demeyer

Issue created by migration from https://trac.sagemath.org/ticket/24588

[embray]

comment:1

I think this uses integer_check_long more or less as intended, but Jeroen will probably want to make sure, as its author.

[jdemeyer]

comment:2

Several quick comments:

1. In Cython, don't use six unless you have a very good reason. Cython supports long(x) both on Python 2 and Python 3.

2. In integer_mod.pyx you write

elif (isinstance(value, int) and
      integer_check_long_py(value, &longval, &err) and not err):

And you sure that you need the isinstance(value, int) check? Part of the point of integer_check_long_py is that there is no need for explicit type checking.

3. In rational.pyx, do you really need IF PY_MAJOR_VERSION <= 2:?

[embray]

comment:4

Replying to @jdemeyer:

Several quick comments:

1. In Cython, don't use six unless you have a very good reason. Cython supports long(x) both on Python 2 and Python 3.

Yes, I think when I wrote that one bit I just forgot I was in a Cython module.

[embray]

comment:5

Replying to @jdemeyer:

3. In rational.pyx, do you really need IF PY_MAJOR_VERSION <= 2:?

I believe so. In Python 2 we need both __long__ and __int__, but on Python 3 if you have both defined Cython will take __int__ (really it should take __long__). But it seems to be confused. However, if you have just __long__ it will use it correctly.

[sagetrac-git]

Changed commit from 23ade97 to 9527794

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

6d1b3b5	These checks are unnecessary
9527794	Fix coerction from long to work correctly on Python 3

[sagetrac-git]

Changed commit from 9527794 to 3342910

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

3342910

Fix some of the tests on Python 3

[fchapoton]

comment:9

does not apply

[sagetrac-git]

Changed commit from 3342910 to ec2f4ae

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

2203f96	py3: fixes to sage.rings.finite_rings.integer_mod to better handle conversion of Python ints to C longs where possible
b3413eb	py3: some int/long fixes for sage.rings.integer
c4b44d7	py3: simple long/int fix in sage.rings.number_field.order
1f009e1	py3: long/int fixes for sage.rings.rational
ff90f27	py3: similarly to int_to_Q, update int_toRR to handle any size of Python int
f7bc2e4	py3: update IntegerMulAction to properly handle int/long to C long conversion on Python 2 and 3
b23ca52	Split int_to_Q into a separate long_to_Q, correcting some other mistakes I had in the implementation.
a7b7fd8	These checks are unnecessary
1358b96	Fix coerction from long to work correctly on Python 3
ec2f4ae	Fix some of the tests on Python 3

[embray]

comment:11

Well, it does now...

[jdemeyer]

comment:12

1. In src/sage/rings/finite_rings/integer_mod.pyx, what does "nor can it be coerced to an int" in the error message mean? You're only checking for int/long, not for something that can be coerced to an int (whatever that would mean).

2. In integer.pyx and rational.pyx, you have

        if type(a) is not int:
            raise ValueError("must be a Python int object")

        do_something_with(PyInt_AS_LONG(a))

First of all, the check for int seems wrong since it will also pass the int == long type on Python 3. Second, the exception should be a TypeError. Since there was no check before, maybe you shouldn't add a check.

3. Don't use PY_NEW(Rational). Just use Rational.__new__(Rational).

4. int_toRR could be made more efficient by directly using an mpz_t instead of an Integer.

5. In src/sage/structure/coerce_actions.pyx, I would replace

if integer_check_long(nn, &n_long, &err) and not err:

by

integer_check_long(nn, &n_long, &err)
if not err:

[embray]

comment:14

Don't use PY_NEW(Rational). Just use Rational.__new__(Rational)

Can you explain this a little more? I used to have the latter but I changed it because in the past you've told me to use PY_NEW, but for Integer. Why for Integer and not Rational?

[jdemeyer]

comment:15

Also, I'm not entirely convinced by the logic in integer_mod.pyx. It seems that the type of the result should only depend on the parent, not on the input number.

[jdemeyer]

Changed commit from ec26452 to 3a7a87e

[jdemeyer]

Last 10 new commits:

fc24a54	py3: similarly to int_to_Q, update int_toRR to handle any size of Python int
0692e41	py3: update IntegerMulAction to properly handle int/long to C long conversion on Python 2 and 3
2826e0b	Split int_to_Q into a separate long_to_Q, correcting some other mistakes I had in the implementation.
993bfc2	These checks are unnecessary
f200fb3	Fix coerction from long to work correctly on Python 3
5748b28	Fix some of the tests on Python 3
857af9d	Addresses some minor review comments
6eeb864	If somehow integer_check_long_py returns False return the appropriate TypeError
0865e8f	Apparently this class should also work with a domain that is not int/long.
3a7a87e	Reviewer fixes

[embray]

comment:39

I had already fixed those things...

I'm not sure why this change:

-        elif err == ERR_OVERFLOW:
+        elif isinstance(x, long):

Why a relatively slow isinstance when the whole point of ERR_OVERFLOW is that the value was already an int/long too big to fit in a C long? At any rate, there's now an unused cimport of ERR_OVERFLOW...

[jdemeyer]

comment:40

Replying to @embray:

I had already fixed those things...

But not in a branch on this ticket.

I'm not sure why this change:

-        elif err == ERR_OVERFLOW:
+        elif isinstance(x, long):

Why a relatively slow isinstance

First of all, it's not at all slow. This uses PyLong_Check() which has an optimized code path:

#define PyLong_Check(op) \
        PyType_FastSubclass(Py_TYPE(op), Py_TPFLAGS_LONG_SUBCLASS)

This shouldn't take any measurable time.

The reason that I changed it is because I want to check the actual condition that I need: it must be an actual Python long since that is what mpz_set_pylong takes (without checking). It is really just an extra safety check.

[embray]

comment:41

I think by the time I pushed my fixes you changed it to your branch.

I see now, upon closer inspection, that integer_check_long_py does not set err = ERR_TYPE if it was not passed an int/long. So indeed in that case this additional check would be needed. But I think that's a defect in integer_check_long_py. It has already checked whether or not the argument is a long, so if it fails that check (as well as the int check) it should set err=ERR_TYPE. Then the additional isinstance(...) (fast or not--it's not micro-optimization I'm concerned with here) is unnecessary.

[embray]

comment:42

While you're at it, one (small) gripe I have with integer_check_long(_py) is that it requires a non-NULL err argument at all. There are times I've used it where I didn't need the err value at all, and it would be nice if it could accept a NULL pointer (and in that case just not set the error).

[jdemeyer]

comment:43

To be honest, it was not easy to decide the API for integer_check_long(_py). My idea was to look at various use cases and then maybe reconsider how the API should be done.

But that shouldn't affect the status of this ticket.

[jdemeyer]

comment:44

Replying to @embray:

There are times I've used it where I didn't need the err value at all

Then the only information that you have is that the object is an integer of some sort where you don't care about its value. To me, that sounds unlikely so I wonder where it came up.

[embray]

comment:45

Replying to @jdemeyer:

Replying to @embray:

There are times I've used it where I didn't need the err value at all

Then the only information that you have is that the object is an integer of some sort where you don't care about its value. To me, that sounds unlikely so I wonder where it came up.

Perhaps you're right. I definitely remember times during development where I wanted this, but I think maybe it was only while testing things and ignoring the error value. Unless a specific example comes up again I won't worry about it.

The other thing I would still change. It doesn't make sense to have a superfluous isinstance(..., long) there. A small fix to integer_check_long_py would make more sense.

[jdemeyer]

comment:46

Replying to @embray:

It doesn't make sense to have a superfluous isinstance(..., long) there.

I think it does make sense. It doesn't hurt and it's a safety net against calling mpz_set_pylong with a non-long which could potentially segfault Python.

[embray]

comment:47

Please re-read #24588 comment:41.

The (as you wrote, somewhat loose to begin with) API for integer_check_long_py doesn't make a whole lot of sense. It already checks isinstance(x, long) and should probably set the relevant error in that case.

[embray]

comment:48

The following small change is all I'm suggesting. This brings the API for integer_check_long_py more in line with the general integer_check_long, which to me makes much more sense, especially since it seems integer_check_long_py is more useful on its own than you maybe initially intended. You are willing to take it, or leave it if you still disagree.

---

New commits:

0df9f62

Slight change to integer_check_long_py--it can now set err=ERR_TYPE when returning false if the argument is neither an int or a long.

[embray]

Changed commit from 3a7a87e to 0df9f62

[embray]

Changed branch from u/jdemeyer/python3/long-int-fixes to u/embray/python3/long-int-fixes

[jdemeyer]

comment:49

I don't agree entirely with this change:

@@ -176,10 +176,11 @@ cdef inline bint integer_check_long(x, long* value, int* err) except -1:
             err[0] = ERR_OVERFLOW
         return 1
     elif PyIndex_Check(x):
-        err[0] = ERR_INDEX
+        err[0] = 0
         try:
             x = PyNumber_Index(x)
         except TypeError:
+            err[0] = ERR_INDEX
             return 0
         return integer_check_long_py(x, value, err)
     else:

It seems safer and more natural to set err[0] = ERR_INDEX whenever any exception in __index__ occurs. In any case, setting err[0] = 0 is pointless because integer_check_long_py() is now guaranteed to set err[0] to some value.

I just reverted that. If you agree, you can set positive review.

[jdemeyer]

Changed branch from u/embray/python3/long-int-fixes to u/jdemeyer/python3/long-int-fixes

[embray]

Changed author from Erik Bray to Erik Bray, Jeroen Demeyer

[embray]

Changed commit from 0df9f62 to 5ed4bda

[embray]

comment:51

Yep, I think I was misunderstanding what ERR_INDEX was supposed to be. Finally we've reached an agreement :)

---

New commits:

5ed4bda

Slight change to integer_check_long_py--it can now set err=ERR_TYPE when returning false if the argument is neither an int or a long.

[vbraun]

Changed branch from u/jdemeyer/python3/long-int-fixes to 5ed4bda