classification
Title: Inconsistent exception usage in PyLong_As* C functions
Type: behavior Stage: resolved
Components: Interpreter Core Versions: Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: rhettinger Nosy List: mark.dickinson, nadeem.vawda, pitrou, python-dev, rhettinger
Priority: normal Keywords: patch

Created on 2011-09-06 07:40 by nadeem.vawda, last changed 2011-09-07 22:03 by rhettinger. This issue is now closed.

Files
File name Uploaded Description Edit
pylong-exceptions.diff nadeem.vawda, 2011-09-06 14:06 review
pylong-exceptions.diff nadeem.vawda, 2011-09-06 22:00 review
Messages (11)
msg143588 - (view) Author: Nadeem Vawda (nadeem.vawda) * (Python committer) Date: 2011-09-06 07:40
The C functions for converting a Python 'int' object to a C integer are
inconsistent about what exception gets raised when the object passed to
them is not an integer. Most of these functions raise a TypeError, but
PyLong_AsUnsignedLongLong() and PyLong_AsDouble() raise a SystemError
instead.

Raising a SystemError here is quite unhelpful. My understanding is that
it is intended to indicate internal programming errors, so an extension
module should not raise one when (for example) a function is passed an
argument of the incorrect type. In such a case, raising a TypeError is a
reasonable default.

Is there any reason not to change the behaviour of these two functions to
be consistent with their siblings?
msg143593 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2011-09-06 10:46
+1 for turning these into TypeErrors.  It makes little sense that PyLong_AsLongLong and PyLong_AsUnsignedLongLong behave differently here.

Do you have a patch handy?
msg143605 - (view) Author: Nadeem Vawda (nadeem.vawda) * (Python committer) Date: 2011-09-06 14:06
> Do you have a patch handy?

See attached.
msg143606 - (view) Author: Nadeem Vawda (nadeem.vawda) * (Python committer) Date: 2011-09-06 14:18
This probably shouldn't be backported to 3.2; it could break 3rd-party
extension modules (though I would hope that nothing depends on this
behaviour...).

Also, it's worth noting that the error handling between conversion
functions still isn't completely consistent - some attempt to coerce
the argument to an integer using type->tp_as_number->nb_int, while
others fail immediately when they see that PyLong_Check() fails.
That's a less pressing issue, though.
msg143613 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2011-09-06 15:27
> This probably shouldn't be backported to 3.2

Agreed;  I don't see this as a bugfix (especially since AFAIK it's not documented that TypeError should be raised here);  rather, as a design improvement.

> Also, it's worth noting that the error handling between conversion
> functions still isn't completely consistent

True;  there are a number of inconsistencies left.  We'll get them all eventually. :-)
msg143614 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2011-09-06 15:30
The patch still needs tests (e.g., in test_capi).  I'm not sure whether it would be good to add information about the TypeError to the docs.
msg143654 - (view) Author: Nadeem Vawda (nadeem.vawda) * (Python committer) Date: 2011-09-06 22:00
Attached is an updated patch with tests.

There don't seem to be any tests for PyLong_AsS[s]ize_t() and
PyLong_AsDouble(), so I added new ones for this issue. They should still
be expanded on at some point in the future, but for the meanwhile this
should be sufficient.

> I'm not sure whether it would be good to add information about the TypeError to the docs.

Yeah, that doesn't seem necessary; raising TypeError in this sort of
situation is sufficiently typical behavior that explicitly documenting
it feels redundant.
msg143701 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2011-09-07 19:19
Looks good to me.

I'd prefer 'test_long_as_size' to be called 'test_long_as_size_t' (even though that's inaccurate for the ssize_t bits :-).

The 'Py_None' reference counting in test_long_as_size and test_long_as_double looked a little odd at first glance---particularly the lack of a Py_INCREF just before the return Py_None.  I see that it works; it just caught my eye.

Anyway, those are just nitpicks;  I leave it to you whether you want to change anything.  Otherwise, please go ahead and apply.  Thanks for the patches!
msg143703 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2011-09-07 19:41
New changeset 4a66a35da3fd by Nadeem Vawda in branch 'default':
Issue #12909: Make PyLong_As* functions consistent in their use of exceptions.
http://hg.python.org/cpython/rev/4a66a35da3fd
msg143704 - (view) Author: Nadeem Vawda (nadeem.vawda) * (Python committer) Date: 2011-09-07 19:46
> The 'Py_None' reference counting in test_long_as_size and
> test_long_as_double looked a little odd at first glance

Indeed, it is rather roundabout, so I added a comment to avoid confusion.

> Anyway, those are just nitpicks;  I leave it to you whether you want to
> change anything.  Otherwise, please go ahead and apply.  Thanks for the
> patches!

Thanks for the review.
msg143710 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2011-09-07 22:03
+1
History
Date User Action Args
2011-09-07 22:03:02rhettingersetassignee: nadeem.vawda -> rhettinger
messages: + msg143710
2011-09-07 19:46:50nadeem.vawdasetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2011-09-07 19:46:29nadeem.vawdasetmessages: + msg143704
2011-09-07 19:41:19python-devsetnosy: + python-dev
messages: + msg143703
2011-09-07 19:19:41mark.dickinsonsetmessages: + msg143701
2011-09-06 22:00:40nadeem.vawdasetfiles: + pylong-exceptions.diff
assignee: nadeem.vawda
messages: + msg143654
2011-09-06 15:30:35mark.dickinsonsetmessages: + msg143614
2011-09-06 15:27:44mark.dickinsonsetmessages: + msg143613
2011-09-06 14:18:28nadeem.vawdasetstage: needs patch -> patch review
messages: + msg143606
versions: - Python 3.2
2011-09-06 14:06:05nadeem.vawdasetfiles: + pylong-exceptions.diff
keywords: + patch
messages: + msg143605
2011-09-06 10:46:00mark.dickinsonsetmessages: + msg143593
2011-09-06 07:47:06pitrousetnosy: + rhettinger, mark.dickinson
2011-09-06 07:40:15nadeem.vawdacreate