PyWeakref_NewRef docs are misleading

[tekknolagi]

BPO	36203
Nosy	@Windsooon, @tekknolagi, @iritkatriel
PRs	bpo-36203: Check callback is callable in PyWeakref_NewRef #12244 bpo-36203: Check callback is callable in PyWeakref_NewRef #26273

^{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 = None
created_at = <Date 2019-03-06.01:03:50.658>
labels = ['type-bug', '3.9', '3.10', '3.11', 'ctypes', 'docs']
title = 'PyWeakref_NewRef docs are misleading'
updated_at = <Date 2021-05-20.20:08:43.797>
user = 'https://github.com/tekknolagi'

bugs.python.org fields:

activity = <Date 2021-05-20.20:08:43.797>
actor = 'tekknolagi'
assignee = 'docs@python'
closed = False
closed_date = None
closer = None
components = ['Documentation', 'ctypes']
creation = <Date 2019-03-06.01:03:50.658>
creator = 'tekknolagi'
dependencies = []
files = []
hgrepos = []
issue_num = 36203
keywords = ['patch']
message_count = 6.0
messages = ['337255', '337292', '337321', '337371', '337372', '394045']
nosy_count = 4.0
nosy_names = ['docs@python', 'Windson Yang', 'tekknolagi', 'iritkatriel']
pr_nums = ['12244', '26273']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue36203'
versions = ['Python 3.9', 'Python 3.10', 'Python 3.11']

Linked PRs

• gh-80384: Check that callback is callable at weak reference creation #151145
• [3.15] gh-80384: Fix docs for PyWeakref_NewRef() and PyWeakref_NewProxy() #151146
• [3.14] gh-80384: Fix docs for PyWeakref_NewRef() and PyWeakref_NewProxy() (GH-151146) #151232
• [3.13] gh-80384: Fix docs for PyWeakref_NewRef() and PyWeakref_NewProxy() (GH-151146) #151233

[tekknolagi]

The docs for PyWeakref_NewRef state "if callback is not callable, None, or NULL, this will return NULL and raise TypeError". It does not appear as though there is a callable check for the callback.

[Windsooon]

Yes, Maxwell. I guess the docs are misleading, the code locate in https://github.com/python/cpython/blob/master/Objects/weakrefobject.c#L748

if (callback == Py_None)
 callback = NULL;
 if (callback == NULL)
 /* return existing weak reference if it exists */
 result = ref;
 if (result != NULL)
 Py_INCREF(result);
 else {
 ...
 }

However, I'm not sure we should fix the docs or the code here.

[tekknolagi]

NewProxy checks if it's callable, so I suppose the code should be fixed.

On Wed, Mar 6, 2019, 03:32 Windson Yang <report@bugs.python.org> wrote:

Windson Yang <wiwindson@outlook.com> added the comment:

Yes, Maxwell. I guess the docs are misleading, the code locate in
https://github.com/python/cpython/blob/master/Objects/weakrefobject.c#L748

if (callback == Py_None)
callback = NULL;
if (callback == NULL)
/* return existing weak reference if it exists */
result = ref;
if (result != NULL)
Py_INCREF(result);
else {
...
}

However, I'm not sure we should fix the docs or the code here.

----------
nosy: +Windson Yang

---

Python tracker <report@bugs.python.org>
<https://bugs.python.org/issue36203\>

---

[Windsooon]

It looks to me the fix is easy, we just will return NULL and raise TypeError when the callback is not callable, None, or NULL. I'm not an expert in C, but I would love to create a PR for it if you don't have time.

[tekknolagi]

I can likely do it tomorrow. If not I'll update this.

On Thu, Mar 7, 2019, 00:20 Windson Yang <report@bugs.python.org> wrote:

Windson Yang <wiwindson@outlook.com> added the comment:

It looks to me the fix is easy, we just will return NULL and raise
TypeError when the callback is not callable, None, or NULL. I'm not an
expert in C, but I would love to create a PR for it if you don't have time.

----------

---

Python tracker <report@bugs.python.org>
<https://bugs.python.org/issue36203\>

---

[iritkatriel]

Please add unit tests to the patch.