PyList_SET_ITEM could be safer

[espie]

BPO	30459
Nosy	@rhettinger, @vstinner, @encukou, @skrah, @serhiy-storchaka, @ctismer, @ZackerySpytz
PRs	bpo-30459: Cast the result of PyList_SET_ITEM() to void #19975 bpo-30459: Convert PyTuple_SET_ITEM() to a function #23645 bpo-30459: Cast the result of PyCell_SET to void #23654 bpo-30459: Use (void) in macros setting variables #28982

^{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 2020-12-07.10:57:40.893>
created_at = <Date 2017-05-24.15:14:12.544>
labels = ['interpreter-core', 'type-feature', '3.10']
title = 'PyList_SET_ITEM  could be safer'
updated_at = <Date 2021-10-15.17:44:47.694>
user = 'https://bugs.python.org/espie'

bugs.python.org fields:

activity = <Date 2021-10-15.17:44:47.694>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2020-12-07.10:57:40.893>
closer = 'vstinner'
components = ['Interpreter Core']
creation = <Date 2017-05-24.15:14:12.544>
creator = 'espie'
dependencies = []
files = []
hgrepos = []
issue_num = 30459
keywords = ['patch']
message_count = 23.0
messages = ['294362', '294421', '294450', '294451', '294472', '294490', '294499', '294502', '294507', '294511', '382550', '382560', '382561', '382562', '382626', '382627', '382825', '382830', '382831', '382832', '382971', '384058', '404040']
nosy_count = 8.0
nosy_names = ['rhettinger', 'vstinner', 'petr.viktorin', 'skrah', 'serhiy.storchaka', 'Christian.Tismer', 'espie', 'ZackerySpytz']
pr_nums = ['19975', '23645', '23654', '28982']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue30459'
versions = ['Python 3.10']

[espie]

Documentation says PyList_SET_ITEM is void, but it lies. The macro is such that it yields the actual element being set.

wrapping the macro content in a do {} while (0)  makes sure PyList_SET_ITEM is really void, e.g.:
#define PyList_SET_ITEM(op, i, v) do { (((PyListObject *)(op))->ob_item[i] = (v)); } while (0)

I just ran into the problem while compiling py-qt4 with clang.
There was some confusion between PyList_SET_ITEM and PyList_SetItem:

if (obj == NULL || PyList_SET_ITEM (l, i, obj) < 0)

g++ didn't catch it (because it doesn't see negative pointers as a problem), but clang++ instantly broke.

With PyList_SET_ITEM truly void the problem disappears.

[rhettinger]

The docs do make the claim of returning void: https://docs.python.org/3/c-api/list.html#c.PyList_SET_ITEM

However, I think we should change the docs rather than changing the macro. This macro is very old and very widely used. Changing it is likely to break existing code which may rely on the current behavior.

Also, I don't want to encourage code like what you saw in py-qt4. It is very indirect about what it is trying to express.

[espie]

Well, there is not going to be a lot of breakage. This problem is the only instance I've encountered in the full OpenBSD ports tree.

I thought python was supposed to be a clean language, and didn't shy away from removing stuff/tweaking stuff to achieve that goal ?...

The py-kde4 error was deadly. I'm lucky clang finally caught it, but I'd rather this kind of stuff just not compile.

I think we're in a world where *correctness* is finally beginning to matter a bit more than *compatibility forever whatever the cost*.

[serhiy-storchaka]

I think it would be safer to just cast the result of the expression to void. This decreases the chance of breaking valid code, that for example uses PyList_SET_ITEM() in a comma expression.

[espie]

yep, casting to (void) would be safer indeed. didn't think of that one ;)

[rhettinger]

Well, there is not going to be a lot of breakage. This
problem is the only instance I've encountered in the
full OpenBSD ports tree.

That is no basis for knowing what the entire rest of the world has done with done with this API (perhaps valid working code using chained assignments).

[espie]

Note that the API is fully documented for returning void... not anything else.

"No basis" right. We're taling 10000 pieces of software. a lot of what is actually used in the world.

I'm very surprised, considering python has routinely done "spring cleanup" by breaking fairly old apis.

If this breaks, people will fix their code, seriously.

In most places, we would rather have undocumented, unportable code, break *cleanly*, rather than rely on a fuzzy behavior that could possibly change at any moment, and that was never documented as doing anything.

Or is there some kind of mystique that, because this is low-level C implementation, somehow, python programmers are not going to be able to cope with the internals ?....

[skrah]

I guess since it's documented, it would be ok to change for 3.7. BTW, we also allow "static inline" now in headers, so perhaps we could make it a function.

[rhettinger]

I prefer to leave this as macro rather than assuming the compiler will heed the inline hint.

[espie]

it's still 100% safe as a macro since each parameter is not used more than once. only the return type is an issue.