Argument Clinic: add unsigned integers converters

[serhiy-storchaka]

BPO	20260
Nosy	@birkenfeld, @mdickinson, @larryhastings, @serhiy-storchaka, @MojoVampire
PRs	bpo-20260: Implement non-bitwise unsigned int converters for Argument Clinic. #8434
Files	clinic_unsigned_converters.patch clinic_unsigned_converters.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 2018-07-26.11:35:25.681>
created_at = <Date 2014-01-14.17:31:58.746>
labels = ['interpreter-core', 'type-feature', '3.8', 'expert-argument-clinic']
title = 'Argument Clinic: add unsigned integers converters'
updated_at = <Date 2018-07-26.11:35:25.679>
user = 'https://github.com/serhiy-storchaka'

bugs.python.org fields:

activity = <Date 2018-07-26.11:35:25.679>
actor = 'serhiy.storchaka'
assignee = 'none'
closed = True
closed_date = <Date 2018-07-26.11:35:25.681>
closer = 'serhiy.storchaka'
components = ['Demos and Tools', 'Interpreter Core', 'Argument Clinic']
creation = <Date 2014-01-14.17:31:58.746>
creator = 'serhiy.storchaka'
dependencies = []
files = ['33464', '33483']
hgrepos = []
issue_num = 20260
keywords = ['patch']
message_count = 15.0
messages = ['208102', '208103', '208107', '208174', '208180', '208181', '208201', '208229', '208376', '208730', '208731', '208734', '208778', '209092', '322411']
nosy_count = 6.0
nosy_names = ['georg.brandl', 'mark.dickinson', 'larry', 'rmsr', 'serhiy.storchaka', 'josh.r']
pr_nums = ['8434']
priority = 'low'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue20260'
versions = ['Python 3.8']

[serhiy-storchaka]

Proposed patch adds support for non-bitwise unsigned integer arguments in Argument Clinic. I.e. now unsigned_int(bitwise=False) (or just unsigned_int) converts Python int in range from 0 to UINT_MAX to C unsigned int. Added support for unsigned_short, unsigned_int, unsigned_long, unsigned_PY_LONG_LONG, and size_t.

Also added global private functions _Py_UnsignedShort_Converter(), _Py_UnsignedInt_Converter(), _Py_UnsignedLong_Converter(), _Py_UnsignedLongLong_Converter(), and _Py_Size_t_Converter(), which are used by Argument Clinic and in C code still not converted to Argument Clinic.

[larryhastings]

Very nice! I had an idea to do this in the back of my head too. But my plate is already full.

I haven't reviewed the patch yet--and I have a huge backlog of reviews already. But I can give this higher priority if you need it for other patches. When do you need this reviewed?

[serhiy-storchaka]

Currently these converters are used only in zlib (unsigned int) and select (unsigned short). But perhaps during conversion to Argument Clinic we will discover that they are more appropriate than bitwise converters in other places. So there is no hurry.

[larryhastings]

We can use this now. So I bumped it to the top of my queue and reviewed it. I had only minor feedback--thanks!

[serhiy-storchaka]

Unfortunately I have discovered that there is significant difference between uint_converter in Modules/zlibmodule.c and proposed _PyLong_UnsignedInt_Converter. And there are tests in Lib/test/test_zlib.py which fail when _PyLong_UnsignedInt_Converter is used instead of uint_converter. uint_converter raises ValueError for negative integers, and _PyLong_UnsignedInt_Converter raises OverflowError.

Possible solutions:

1. Change tests. I don't like this, ValueError looks reasonable for these cases.

2. Continue to use uint_converter in Modules/zlibmodule.c. Then new converters become less useful.

3. Raise ValueError in new converters for negative integers. ValueError looks reasonable in many cases.

4. Raise ValueError in PyLong_AsUnsignedLong etc for negative integers.

Here is a patch which incorporates Larry's suggestions and implements option #3.

[serhiy-storchaka]

Oh, and of course 5th option: do nothing.

[skrah]

I think we cannot change PyLong_AsUnsignedLong() without a deprecation period,
if at all. That leaves the option of changing the converters.

My preference is to raise either OverflowError or ValueError for *both*
out-of-range conditions.

People may me used to OverflowError by now -- the usage in PyLong_AsUnsignedLong()
dates back to very early revisions -- but there are equally good reasons to use
ValueError.

[larryhastings]

I'm glad you caught that! First things first: the converted code should behave identically to the existing code, including raising the same exceptions.

If you examine the exception hierarchy:
http://docs.python.org/3.4/library/exceptions.html#exception-hierarchy

you'll see that "OverflowError" is a subclass of "ArithmeticError". In other words, it represents when you perform an arithmetic operation that overflows the result type. Using it to also represent "you specified a value that is out of range for this conversion" seems wrong.

So I like #3 as well.

Could _PyLong_UnsignedInt_Converter catch the OverflowError raised by PyLong_AsUnsignedLong and reraise it as ValueError?

[rmsr]

socketmodule has three builtins which use PyLong_AsUnsignedLong on their arguments and would benefit from these converters, but only if they raise OverflowError. So I vote for #2.

I don't think it's unreasonable to continue to have locally-defined argument converters.

[larryhastings]

Is this waiting on something? I agree that we can't change the behavior
of existing functions. But your new converters should raise ValueError.

[larryhastings]

Also, you didn't remove the _ in front of "Converter" in the names, e.g "_PyLong_UnsignedShort_Converter" should be "_PyLong_UnsignedShortConverter".

[larryhastings]

Actually, here's another data point to consider. The underlying implementation of PyArg_Parse* is the function convertsimple() in Python/getargs.c. For all the non-bitwise integer format units ('bhi'), if they overflow or underflow the native integer they raise OverflowError.

[serhiy-storchaka]

I think that we do not yet have enough data. Too little cases are known for which unsigned integer converters are needed, and different cases require a different behavior. I prefer to defer until we convert the majority of the code. Then we will see what behavior is most prevalent. We even can change and unify behavior in 3.5.

There is no hurry. We can use in each module its own Converter (if needed). There should not be many such places.

[larryhastings]

I can start citing data points if you like.

socket.if_indextoname just calls PyLong_AsUnsignedLong(). Not sure what that throws.

[serhiy-storchaka]

New changeset 7cb7bcf by Serhiy Storchaka in branch 'master':
bpo-20260: Implement non-bitwise unsigned int converters for Argument Clinic. (GH-8434)
7cb7bcf