py3: ZZ for large int

[fchapoton]

in a python3-build-sage:

sage: ZZ(int(2**62))
4611686018427387904
sage: ZZ(int(2**63))
---------------------------------------------------------------------------
OverflowError                             Traceback (most recent call last)
OverflowError: Python int too large to convert to C long

The above exception was the direct cause of the following exception:

SystemError                               Traceback (most recent call last)
<ipython-input-41-47ff15a43d5f> in <module>()
----> 1 ZZ(int(Integer(2)**Integer(64)))

SystemError: Integer Ring returned a result with an error set
sage: int(2**63)
18446744073709551616

related to #16072

CC: @jdemeyer @embray

Component: python3

Author: Jeroen Demeyer

Branch/Commit: 190a90c

Reviewer: Frédéric Chapoton

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

[jdemeyer]

comment:2

Is the text that you quoted above the literal text that you see on your screen? Just wondering why there is no traceback shown...

[fchapoton]

comment:3

yes, this the literal text that I see in my terminal. No better traceback.

By the way, maybe you should try by yourself to use the branch public/python3-experiment-8.1.rc1 ?

[jdemeyer]

Branch: u/jdemeyer/py3__zz_for_large_int

[jdemeyer]

Commit: f27e59b

[jdemeyer]

comment:5

There is a general pattern here:

In Cython code in Sage, when checking for an int-like object, this is often done like

if isinstance(x, int):
    # Handle int
elif isinstance(x, long):
    # Handle long

This works fine in Python 2 because int and long are distinct types. In Python 3, there is only one type, which is the long type from Python 2 but which is called int in Python 3 (similar to how the unicode type from Python 2 is called str in Python 3).

Now Cython understands the name long to refer to the type which is called long in Python 2 and int in Python 3. It also understands int to refer to the type which is called int in Python 2 and the different type which is called int in Python 3.

So, in Cython code for Python 3, the checks isinstance(x, int) and isinstance(x, long) are equivalent. However, the code which needs to be executed is the code for the long type, so we should do that check first.

---

New commits:

f27e59b

Check "long" before "int"

[jdemeyer]

Author: Jeroen Demeyer

[fchapoton]

comment:6

This does not fix the issue:

sage: ZZ(int(2**63))
---------------------------------------------------------------------------
OverflowError                             Traceback (most recent call last)
OverflowError: Python int too large to convert to C long

The above exception was the direct cause of the following exception:

SystemError                               Traceback (most recent call last)
<ipython-input-3-9949e9fafd8e> in <module>()
----> 1 ZZ(int(Integer(2)**Integer(63)))

SystemError: Integer Ring returned a result with an error set

[jdemeyer]

comment:7

Sorry, I can't help further without a traceback.

[sagetrac-git]

Changed commit from f27e59b to 190a90c

[sagetrac-git]

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

190a90c

Check "long" before "int"

[jdemeyer]

comment:9

Never mind, I just realized that the example uses coercion, not direct initialization of an Integer.

[fchapoton]

comment:10

Indeed

sage: ZZ(2**63)
9223372036854775808

does work (at least with you change)

[fchapoton]

comment:11

That fixes the issue ! Thanks !

[fchapoton]

Reviewer: Frédéric Chapoton

[jdemeyer]

comment:13

There will certainly be more places in Sage with the same bug. But the solution should be the same: whenever you want to check int and long, check long first.

[jdemeyer]

comment:14

See also #23792

[fchapoton]

comment:15

follow up in #24225

[fchapoton]

comment:16

ok, let us use the opportuniy here to make some similar changes in the same file

[fchapoton]

New commits:

0fb286d

trac 24221 Eric's suggestions of additional changes

[fchapoton]

Changed branch from u/jdemeyer/py3__zz_for_large_int to public/longint24221

[fchapoton]

Changed commit from 190a90c to 0fb286d

[jdemeyer]

Changed commit from 0fb286d to 190a90c

[jdemeyer]

comment:18

Moving the follow-up changes to #24227.

[jdemeyer]

Changed branch from public/longint24221 to u/jdemeyer/py3__zz_for_large_int

[vbraun]

Changed branch from u/jdemeyer/py3__zz_for_large_int to 190a90c