GeezerVison

I don't think the minimum deprecation period is appropriate here.
I would prefer only deprecating calls to PyObject_SetAttr(v, name, NULL) with an exception set.

I modified the PR: it no longer plans to convert this feature to an error (in Python 3.15): it only deprecates the feature (emit DeprecationWarning).

I also rebased the PR and fixed the issue number (issue #106572 is the correct issue!).

I don't think the minimum deprecation period is appropriate here.

I proposed the bare minimum. If you consider that it's too short, honestly, I would prefer to just unschedule the removal and discuss it later, once we will have a better idea of how many C extensions in the wild are impacted.

cc @serhiy-storchaka

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

And if you don't make the requested changes, you will be put in the comfy chair!

I extracted the non-controversial part into a separated PR: PR #106611 "gh-106572: Convert PyObject_DelAttr() to a function".

For smoother future changes we can add a way to enable or disable this at compile time. Just surround the warning code with #ifdef SOMENAME/#endif but keep SOMENAME undefined for now.

That sounds like capi-workgroup/problems#54

If updated my PR to only emit a DeprecationWarning in the Python Development Mode or if Python is built in debug mode.

I would prefer only deprecating calls to PyObject_SetAttr(v, name, NULL) with an exception set.

The value can be NULL without an exception being set. What if you use PyDict_GetItem() with PyObject_SetAttr()? If the key exists in the dict, you're good. If the key doesn't exist in the dict (for whatever reason), you delete the attribute: is it what you wanted to do? There is no exception set with such weird API.

Apparently, some developers don't know that this function can be used to delete attributes: see the old #69877 issue for example.

The value can be NULL without an exception being set. What if you use PyDict_GetItem() with PyObject_SetAttr()? If the key exists in the dict, you're good. If the key doesn't exist in the dict (for whatever reason), you delete the attribute: is it what you wanted to do?

Yes, it is. I was trying to make the object's attributes match the dict.
I mean -- when you misuse the API, you get an unexpected result. That happens. There are no segfaults or undefined behaviour here.

I believe deprecating what was the way to remove attributes, just because some uses are error-prone, is too drastic and will hurt Python and its users.

My objection stands.

Well, I propose an PR to remove the deprecation: @serhiy-storchaka disagreed.

Now I propose a PR to emit a warning, @encukou disagree.

@serhiy-storchaka and @encukou: can you try to agree on what should be done?

My concern is that if the feature is deprecated, most people will not be aware of it without a deprecation warning emitted at runtime. Do you want to help users to discover potential bugs in their code? Or do we want to leave them alone and just write doc?

@serhiy-storchaka and @encukou: can you try to agree on what should be done?

IMO we roughly agree, at least on what should happen for 3.13?

Serhiy:

We should first implement PyObject_DelAttr() as a separate function, then in some distant future we decide to break binary compatibility with Python older than 3.13 and add a warning, then error, in PyObject_SetAttr().

me:

I would prefer only deprecating calls to PyObject_SetAttr(v, name, NULL) with an exception set.

Are you aware of any actual users who got confused by this? (PyObject_SetAttr, not tp_setattro)

Are you aware of any actual users who got confused by this?

@serhiy-storchaka who asked to deprecate using these functions to delete attributes: see links to previous discussions in above comments.

then in some distant future we decide to break binary compatibility with Python older than 3.13

We can leave the old ABI unchanged, add a new function with a different name, and leave the API unchanged: no need to expose the new name in the public API.

Old ABI name, left unchanged (no warnings): SetAttr
New ABI name, with warning: SetAttr2
Public API: SetAttr, implemented as calling SetAttr2

See also: capi-workgroup/problems#39 "Supporting multiple ABI versions at once".

I do not disagree with @encukou. I proposed to add a runtime warning few versions after making PyObject_DelAttr() a function, so the user code built with older Python version will not start emit warnings when linked with newer Python version (or at least the chance of this should be minimized). We can add warnings right now, but disable them by default.

The goal is not to break the user code, not to force them to change their code, but to help them to find potential bugs which are difficult to find now.

We can add warnings right now, but disable them by default.

DeprecationWarning is ignored by default.

My proposed PR don't even emit DeprecationWarning by default, but only in Python Development Mode (-X dev).

I'm +1 on Victor, emit warning only in devmode.

Some developers run their tests with enabled DeprecationWarning. Could it be at least PendingDeprecationWarning?

Some developers run their tests with enabled DeprecationWarning. Could it be at least PendingDeprecationWarning?

I don't get it. Isn't the purpose of this change helping developers to identify unsafe C API usage?

If the intent is to hardly ignore the deprecation warning, maybe the whole idea of emitting a warning should be abandoned? I don't get it.

We should be less persistent in forcing this change than in case of other deprecated warnings. I think it is a compromise between breaking user code and keeping the status quo.

I think you're solving a non-issue -- a minor API wart we should avoid in the future, but that's it.
If you don't know what a function does with NULL and pass NULL to it, you can expect much worse things that a cleanly deleted attribute. The behaviour is about as surprising as, say, the two-argument form of iter().
I still don't see any evidence of this actually causing a problem (as opposed to tp_setattro which is very different -- there, user code must handle NULL).
In the PyDict_GetItem+PyObject_SetAttr case, I'd put all the blame of being a bad API on PyDict_GetItem.

I don't think even making the docs more complex is worth it, but, I won't block this iteration of the PR.

I close the issue. I don't care enough about this issue to argue about the exact migration plan. PyObject_DelAttr() and PyObject_DelAttrString() are now implemented as a function in Python 3.13.

Serhiy:

We should first implement PyObject_DelAttr() as a separate function, then in some distant future we decide to break binary compatibility with Python older than 3.13 and add a warning, then error, in PyObject_SetAttr().

That done in commit 1f2921b.

Petr:

I would prefer only deprecating calls to PyObject_SetAttr(v, name, NULL) with an exception set.

I'm not convinced of the benefits of emitting a warning only if an exception is set.